Bug 162785 – [Fetch API] Forbid redirection to non-HTTP(s) URL in non-navigation mode

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

As per the spec, we should not follow redirections to other URLs than HTTP(s).

[youenn fablet]

There is some discussions at https://github.com/whatwg/fetch/issues/393 to relax this rule for data URLs since at least in WebKit, we can load data URLs in no-cors mode.
For the moment, it is better though to restrict to not allowing data URLs redirections for fetch API.

[youenn fablet]

Created attachment 290329 [details]
Patch

[Alex Christensen]

Comment on attachment 290329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290329&action=review

> Source/WebCore/loader/DocumentThreadableLoader.cpp:221
> +    client.didFail(ResourceError(errorDomainWebKitInternal, 0, url, "Redirection to URL with a scheme that is not HTTP(S).", ResourceError::Type::AccessControl));

Is this tested?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:656
> +    // We are passing url as well as request, as request url may contain a fragment identifier.

I think we should add a test with a fragment instead of this comment.

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 290329 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290329&action=review
> 
> > Source/WebCore/loader/DocumentThreadableLoader.cpp:221
> > +    client.didFail(ResourceError(errorDomainWebKitInternal, 0, url, "Redirection to URL with a scheme that is not HTTP(S).", ResourceError::Type::AccessControl));
> 
> Is this tested?

Without this check, the no-cors redirect URL would fail.
Ideally, we would like to log erros in the console, as done for xhr.
The issue is that this makes some tests flaky, see bug 160546 for the logging
See bug 161310 for improving the test framework.

> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:656
> > +    // We are passing url as well as request, as request url may contain a fragment identifier.
> 
> I think we should add a test with a fragment instead of this comment.

There are some tests that would break otherwise. I can remove the comment.
Ideally there should be no frag ids at loader level.

[WebKit Commit Bot]

Comment on attachment 290329 [details]
Patch

Clearing flags on attachment: 290329

Committed r206716: <http://trac.webkit.org/changeset/206716>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 162858

[youenn fablet]

Mac CMakbe Debug bot is failing due to the following error:

In file included from /Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebKit/mac/WebView/WebHTMLView.mm:79:
In file included from /Volumes/Data/slave/elcapitan-cmake-debug/build/WebKitBuild/Debug/DerivedSources/ForwardingHeaders/WebCore/CachedResourceLoader.h:1:
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebCore/loader/cache/CachedResourceLoader.h:33:10: fatal error: 'ContentSecurityPolicy.h' file not found
#include "ContentSecurityPolicy.h"
         ^
1 error generated.
make[2]: *** [Source/WebKit/CMakeFiles/WebKit.dir/mac/WebView/WebHTMLView.mm.o] Error 1


Any idea why it is not able to find this header file?

[youenn fablet]

Created attachment 290799 [details]
Patch for landing

[youenn fablet]

(In reply to comment #9)
> Created attachment 290799 [details]
> Patch for landing

Relanding it as CMake build bot is still broken.
I'll be happy to help on fixing the bot.

[WebKit Commit Bot]

Comment on attachment 290799 [details]
Patch for landing

Rejecting attachment 290799 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 290799, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2230263

[youenn fablet]

Created attachment 290800 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 290800 [details]
Patch for landing

Rejecting attachment 290800 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 290800, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
tching file Source/WebCore/loader/cache/CachedResourceLoader.h
patching file LayoutTests/imported/w3c/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-expected.txt
patching file LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-worker-expected.txt

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2230271

[youenn fablet]

Created attachment 290801 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 290801 [details]
Patch for landing

Clearing flags on attachment: 290801

Committed r206858: <http://trac.webkit.org/changeset/206858>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.