I'd propose to make ResourceHandle::loadResourceSynchronously take a ResourceHandleClient, and have FrameLoader implement ResourceHandleClient
That way, FrameLoader is notified of redirects and can do the access control checks
wdyt?

(In reply to comment #3)
> I'd propose to make ResourceHandle::loadResourceSynchronously take a ResourceHandleClient, and have FrameLoader implement ResourceHandleClient
>
> That way, FrameLoader is notified of redirects and can do the access control checks
>
> wdyt?
I'd rather add a ResourceLoader::loadResourceSynchronously() and have FrameLoader call that than make FrameLoader a ResourceHandleClient. FrameLoader implemeneting ResourceHandeClient feels like a layering violation to me, but maybe others disagree.

Looking at the code, it seems like the natural place to check access control after a redirect would be in DocumentThreadableLoader. There's a FIXME there that states:
bool DocumentThreadableLoader::isAllowedRedirect(const KURL& url)
{
if (m_options.crossOriginRequestPolicy == AllowCrossOriginRequests)
return true;
// FIXME: We need to implement access control for each redirect. This will require some refactoring though, because the code
// that processes redirects doesn't know about access control and expects a synchronous answer from its client about whether
// a redirect should proceed.
return m_sameOriginRequest && securityOrigin()->canRequest(url);
}
In reading the CORS standard, it sounds like redirects should be transparent under access control, which means we would not call the synchronous client callback. In fact, we would hide the fact that the redirect happened at all. Hopefully, that would reduce the amount of refactoring required to make this work.
(In reply to comment #9)
> not yet fixed, that was just a test

Created attachment 122819[details]
work in progress for fixing CORS redirects
I made an attempt at fixing this a couple of months ago (see attachment).
I didn't clean it up and post it for review because chromium is missing some plumbing for this to work correctly (see codereview.chromium.org/8589032) and I got distracted in the middle of fixing the chromium issues.

I've added a proposed patch that modifies DocumentThreadableLoader to allow redirects. The basic approach is to intercept the redirect and start the load over with the new request. One of the issues is that this request is assembled by the browser, and may contain headers that aren't allowed by CORS. I added methods to remove these headers to ResourceRequestBase, but it might be better to have a method that removes all forbidden headers. However, that seemed like it might not be appropriate to implement on ResourceRequestBase.

Comment on attachment 123030[details]
Proposed Patch
Please include xhr-cors-redirect.html in the patch.
I suspect that one test is likely not enough. A good - although extremely time consuming - way to come up with more tests is to look through working group discussions about changes in the spec. If something was not obvious to spec authors, it should be explicitly tested by us.

I think the first patch on this bug contains xhr-cors-redirect.html and was reviewed, landed, and closed. I agree that more tests are needed, and will expand the patch. I may also have to change the existing test / expectations.
I would be interested in general comments about the suitability of the approach, potential problems, and security concerns.

Indeed - not sure why it got landed separately, but it confused me.
What was the reason to choose this design? If possible, it would be much better to let low level machinery take care of generating a new request, as it already does for non-CORS requests. There are many tricky cases, e.g. for whether credentials should be carried over.

The new request is generated at a lower level and passed up the loader stack. DocumentThreadableLoader only removes some headers so the request can pass the CORS request checks, and then restarts the load using the new request as if it was the original.
The header that causes problems in my manual testing was "User-Agent", added by FrameLoader. I could reduce the patch to only add a 'clearHttpUserAgent' method to ResourceRequestBase, but it seems safer to add methods for all browser-added headers and remove them all before kicking off the new request (only when using access control).
It seems natural to me that this redirect logic be in DocumentThreadableLoader, since that is where URL loads with access control are made.

The first patch for this bug committed layout tests for this functionality that don't work. The redirect response has the access control headers, but the final response for the redirected request doesn't, so it will fail.
I've added some .php and .cgi files and modified xmlhttprequest/access-control-and-redirects.html to test my patch. In the process, I noticed that there are tests (xmlhttprequest/redirect-xxx.html) that seem to expect CORS redirects to fail in cases where the spec says they should succeed. Alexey, could I get more guidance from you on what work needs to be done to get a solid test suite for this new feature? Are there tests that should be deleted, rewritten, or re-baselined? I'm thinking a good approach is to use redirect.php and write a bunch of redirect-xxx.html tests to request URLs that redirect to existing .php and .cgi resources.
From my reading of the spec, it seems like we want the following:
1) Simple requests that redirect should succeed or fail as if the redirect never happened (the loader just restarts when it gets the redirect, with the new URL). The redirect should be transparent to the XHR.
2) CORS requests with preflight that redirect should fail (either preflight or actual request).
I'd like to approach this conservatively, opening up redirects only where the spec is clear.

> In the process, I noticed that there are tests (xmlhttprequest/redirect-xxx.html) that
> seem to expect CORS redirects to fail in cases where the spec says they should succeed.
If you have a specific example you'd like me to comment on, I can look into its history.
> Alexey, could I get more guidance from you on what work needs to be done to get
> a solid test suite for this new feature? Are there tests that should be deleted,
> rewritten, or re-baselined?
I'm pretty sure that we don't have a complete test suite. For example, it's likely completely untested what happens with credentials on redirect. I do encourage you to take at least a brief look at working group discussions.
Existing tests that disagree with new behavior should be deleted or updated, depending on whether the case they're testing (as explained in each test's description) is still meaningful. Just changing results from "PASS" to "FAIL" would not be a correct course of action.

I haven't been able to find "working group discussions" on CORS using web search. Can you provide a link?
Also, while looking at the CORS standard, I noticed there is "Latest Editor Version" which appears to contain changes to the standard:
http://www.w3.org/TR/2009/WD-cors-20090317/
In particular, there is a "follow redirects flag" which allows the client to control this behavior. There is no corresponding new value on XMLHttpRequest. I'm wondering if I should try to add this to ThreadableLoaderOptions.

The code needs to be updated and I am still working on the tests. There are a bunch of existing tests that expect redirects to fail that need to be modified or converted to test the redirect steps as described in the editor's working draft.

Created attachment 128009[details]
Proposed Patch
Updating the patch. This patch is closer to how I read the CORS editor's draft as it stands today. It passes all current layout tests except access-control-and-redirects.html of course. I am working on a test plan.

> Updating the patch. This patch is closer to how I read the CORS editor's draft as it stands today. It passes all current layout tests except access-control-and-redirects.html of course. I am working on a test plan.
The shape of this patch looks good. I haven't done a detailed comparison with the spec. Would you like me to do that now, or should I wait for a future version of this patch?

Created attachment 128267[details]
Preliminary Patch
The previous patch incorrectly allows non-simple cross-origin requests to redirect (after the preflight). Added a m_simpleCrossOriginRequest field and test this rather than that m_actualRequest in non-null.

Attachment 133157[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
Source/WebCore/loader/DocumentThreadableLoader.cpp:192: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 133166[details]
Proposed Patch
Added test expectations and fixed style.
The existing access-control-and-redirects.html test doesn't appear to need rebaselining. Sync loads still fail and async ones appear to fail because the redirect doesn't pass the access control check as it has no CORS headers. We might still want to update the test.
An earlier attachment to this issue added some tests but we should probably remove those now.

Created attachment 133245[details]
Proposed Patch
Sorry, my mistake. I meant to select R? of course.
I updated the patch to remove the previously committed test, as what it intends to test is more thoroughly covered by the new test.

Comment on attachment 133245[details]
Proposed Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=133245&action=review> Source/WebCore/ChangeLog:4
> + DocumentThreadableLoader follows the CORS redirect steps when asynchronously loading a
> + cross origin request with access control.
You might consider adding a link to the spec in the ChangeLog. Traditionally, we use the line above the bug URL for the title of the bug and then put a description like this under the bug URL (after a blank line), but this way works too.
> Source/WebCore/loader/DocumentThreadableLoader.cpp:185
> + allowRedirect =
> + SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled(request.url().protocol())
WebKit would usually merge these two lines.
> Source/WebCore/loader/DocumentThreadableLoader.cpp:201
> + if (!originalOrigin.get()->isSameSchemeHostPort(requestOrigin.get()))
This line is pretty subtle. One complication that we need to worry about that the spec doesn't need to worry about is "universal" origins that can access any URL... However, in that case, we won't be in the UseAccessControl codepath, so I think this is actually correct.
nit: originalOrigin.get()->isSameSchemeHostPort can be written as originalOrigin->isSameSchemeHostPort because RefPtr overloads operator->

Created attachment 133508[details]
Proposed Patch
Addressed Adam's comments and redid the ChangeLogs.
One interesting result of this patch is that same origin XHRs which receive cross origin redirects still fail. This is because XHRs have 'withCredentials' set to 'true' when they begin to load a same origin request. Once they receive a redirect to a cross origin URL, they always fail, since the security origin gets set to a globally unique id and that plus 'allowCredentials' causes the access control check to fail.
We could fix this by clearing this flag (on the loader) but that might be confusing. A surprising (to me) result of this work is that of the 9 test cases, only 1 succeeds.