Bug 28313 – Combine ThreadableLoaderOptions::CrossOriginRequestPolicy and CrossOriginRedirectPolicy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Aaron Boodman]

They are always set the same way. Also, I believe that ::resolveURL() no longer has to do the same origin check -- we can rely on ThreadableLoader to do it.

[Aaron Boodman]

(In reply to comment #0)
> Also, I believe that ::resolveURL() no longer has to do the same origin check

Talking here about AbstractWorker::resolveURL().

[Aaron Boodman]

Created attachment 34929 [details]
Combines flags and fixes some edge case behavior with redirects

Requesting ap because I imagine he's he best person for this review.

[Aaron Boodman]

Created attachment 34930 [details]
Fix formatting in WebCore/ChangeLog

[Alexey Proskuryakov]

Comment on attachment 34930 [details]
Fix formatting in WebCore/ChangeLog

How does this fix relate to bug 27740?

+        Bug 28313: Combine ThreadableLoaderOptions::crossOriginRequestPolicy and ThreadableLoaderOptions::crossOriginRedirectPolicy
+        since they are always set the same way.

Please also include bug URL for easy clicking. Personally, I think that "Bug 28313: " prefix becomes unnecessary with that, and do not use it, but in the past, some people have indicated that they liked it.

+        for access control, so we should never redirect across origins. But in two edge cases, we were:
+
+        * Synchronous XHR: cross-origin request that redirects to same-origin resource.

Were we? I think this is caught by checks in ResourceHandle, e.g. in connection:willSendRequest:redirectResponse: in ResourceHandleMac.mm. The first time this function is called, original url is stored to m_url, and then, the redirected URL is compared to that, not to document origin.

+        (WebCore::DocumentThreadableLoader::isLegalRedirect): Ditto.

Maybe "allowed" would be more neutral? "Legal" makes me think that this is a precisely defined term from some spec (like characters legal in some production), which it isn't.

Just asking for your opinion.

+    // request and response URLs. This isn't a perfect though, since a URL could redirect to itself.

Do you mean that a series of redirects could return us to the same host? "Redirect to itself" seem confusing.

+#include "KURL.h"

No need to include the header, KURL is only used as a reference here.

+        Also, tightened up behavior of XMLHttpRequest with cross-origin redirects and access control. We have not implemented redirects
+        for access control, so we should never redirect across origins. But in two edge cases, we were:

LayoutTests ChangeLog lacks bug URL, and starts with "also".

+Tests that a cross-origin request that redirects to a same-origin resource succeeds.

Looks like all subtests actually expect failure, not success.

There are tabs in layout test.

r- since you are not a committer yet, and this needs some fix-up - but all these comments could be addressed during landing otherwise.

[Aaron Boodman]

Created attachment 34956 [details]
respond to comments

Sorry, switched to git, so this patch will not diff with the previous super cleanly.

[Aaron Boodman]

Created attachment 34957 [details]
isLegalRedirect->isAllowedRedirect in WebCore/ChangeLog

[Aaron Boodman]

(In reply to comment #4)
> (From update of attachment 34930 [details])
> How does this fix relate to bug 27740?

Sorry, I wasn't aware of that bug. The checks here are a superset (more strict) than the existing checks.  I think that this patch makes the check in WebCoreSynchronousLoader unnecessary, but I'm not sure. I don't think this patch is incompatible with it.

> +        Bug 28313: Combine ThreadableLoaderOptions::crossOriginRequestPolicy
> and ThreadableLoaderOptions::crossOriginRedirectPolicy
> +        since they are always set the same way.
> 
> Please also include bug URL for easy clicking. Personally, I think that "Bug
> 28313: " prefix becomes unnecessary with that, and do not use it, but in the
> past, some people have indicated that they liked it.

Done.

> +        for access control, so we should never redirect across origins. But in
> two edge cases, we were:
> +
> +        * Synchronous XHR: cross-origin request that redirects to same-origin
> resource.
> 
> Were we? I think this is caught by checks in ResourceHandle, e.g. in
> connection:willSendRequest:redirectResponse: in ResourceHandleMac.mm. The first
> time this function is called, original url is stored to m_url, and then, the
> redirected URL is compared to that, not to document origin.

[Side note: I messed up the change log in my first patch. It was not synchronous loads this case didn't work with, but asynchronous load. I've fixed that now.]

For the case of page on origin A requesting resource from origin B that redirects back to a resource on origin A, we do trigger the check in ResourceHandleMac.mm.  However, it does not seem to stop the redirect from proceeding. Without this patch, the redirect occurs and we load the same-origin resource.

> +        (WebCore::DocumentThreadableLoader::isLegalRedirect): Ditto.
> 
> Maybe "allowed" would be more neutral? "Legal" makes me think that this is a
> precisely defined term from some spec (like characters legal in some
> production), which it isn't.
> 
> Just asking for your opinion.

Good idea, I've changed this.

> +    // request and response URLs. This isn't a perfect though, since a URL
> could redirect to itself.
> 
> Do you mean that a series of redirects could return us to the same host?
> "Redirect to itself" seem confusing.
> 
> +#include "KURL.h"
>
> No need to include the header, KURL is only used as a reference here.

Right. I did need a forward declare of KURL though. I changed this.

> +        Also, tightened up behavior of XMLHttpRequest with cross-origin
> redirects and access control. We have not implemented redirects
> +        for access control, so we should never redirect across origins. But in
> two edge cases, we were:
> 
> LayoutTests ChangeLog lacks bug URL, and starts with "also".

Done.

> +Tests that a cross-origin request that redirects to a same-origin resource
> succeeds.
> 
> Looks like all subtests actually expect failure, not success.

Fixed.

> There are tabs in layout test.

Fixed.

[Alexey Proskuryakov]

Comment on attachment 34957 [details]
isLegalRedirect->isAllowedRedirect in WebCore/ChangeLog

r=me

[Eric Seidel (no email)]

Comment on attachment 34957 [details]
isLegalRedirect->isAllowedRedirect in WebCore/ChangeLog

Rejecting patch 34957 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/build-webkit failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 34957 [details]
isLegalRedirect->isAllowedRedirect in WebCore/ChangeLog

Sorry.  There was an error in the commit-queue over the weekend.  :(  Adding it back in...

[Eric Seidel (no email)]

Comment on attachment 34957 [details]
isLegalRedirect->isAllowedRedirect in WebCore/ChangeLog

Rejecting patch 34957 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/build-webkit failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 34957 [details]
isLegalRedirect->isAllowedRedirect in WebCore/ChangeLog

Looks like the tree is just red at the moment.  The commit queue is not smart enough to detect that before attempting to build the patch in question.  We'll wait for the tree to green up again and then add this back.

[Eric Seidel (no email)]

Comment on attachment 34957 [details]
isLegalRedirect->isAllowedRedirect in WebCore/ChangeLog

Bots have rolled green adding back to the queue.

[Eric Seidel (no email)]

Comment on attachment 34957 [details]
isLegalRedirect->isAllowedRedirect in WebCore/ChangeLog

Rejecting patch 34957 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'rebase']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-expected.txt
	A	LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects.html
	M	WebCore/ChangeLog
	M	WebCore/loader/DocumentThreadableLoader.cpp
	M	WebCore/loader/DocumentThreadableLoader.h
	M	WebCore/loader/ThreadableLoader.h
	M	WebCore/page/EventSource.cpp
	M	WebCore/workers/Worker.cpp
	M	WebCore/workers/WorkerContext.cpp
	M	WebCore/workers/WorkerScriptLoader.cpp
	M	WebCore/workers/WorkerScriptLoader.h
	M	WebCore/xml/XMLHttpRequest.cpp
Committed r47388
	M	WebCore/ChangeLog
	M	WebCore/page/EventSource.cpp
	M	WebCore/workers/WorkerScriptLoader.cpp
	M	WebCore/workers/Worker.cpp
	M	WebCore/workers/WorkerContext.cpp
	M	WebCore/workers/WorkerScriptLoader.h
	M	WebCore/xml/XMLHttpRequest.cpp
	M	WebCore/loader/DocumentThreadableLoader.h
	M	WebCore/loader/ThreadableLoader.h
	M	WebCore/loader/DocumentThreadableLoader.cpp
	A	LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects.html
	A	LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-expected.txt
	M	LayoutTests/ChangeLog
r47388 = f058c85c1ae34ce90d2d0d8e2cf53ca0cd80d221 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47388