Bug 57600 (sam)

Summary: cross-origin XMLHttpRequest doesn't work with redirect
Product: WebKit Reporter: Samir <samirsha>
Component: XMLAssignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adam, aestes, annevk, ap, bbudge, dglazkov, dpaddock, eric, japhet, jochen, webkit.review.bot, xKhorasan+webkit
Priority: P2 Keywords: InRadar
Version: 525.x (Safari 3.2)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 76198, 82964    
Attachments:
Description Flags
Patch
none
work in progress for fixing CORS redirects
none
Proposed Patch
none
Proposed Patch
none
Preliminary Patch
none
Proposed Patch
none
Proposed Patch
webkit.review.bot: commit-queue-
Proposed Patch
abarth: review+
Proposed Patch
none
Patch for landing none

Description Samir 2011-03-31 17:38:18 PDT
Steps:
1. Setup two origins blah.domain.com and blah2.domain.com
1. Enable CORS (Cross Origin Resource Sharing)  support on blah2 web server side by setting appropriate Access-Control-Allow-Origin: blah.origin.com
2. Do XHR (synchronous) HTTP POST call to blah2.domain.com/index1.htm from a page on blah.domain.com
3. Return a 302 redirect to blah2.domain.com/index2.htm from blah2 server

XHR returns "NETWORK_ERR: XMLHttpRequest Exception 101". 

According to CORS spec, this should have worked.
Comment 1 Alexey Proskuryakov 2011-04-29 10:35:05 PDT
<rdar://problem/9359014>
Comment 2 jochen 2012-01-12 16:20:38 PST
also doesn't work with async XHRs:

http://samples.msdn.microsoft.com/ietestcenter/CORS/CORS_014.htm
Comment 3 jochen 2012-01-13 07:37:47 PST
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?
Comment 4 Nate Chapin 2012-01-13 13:11:33 PST
(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.
Comment 5 jochen 2012-01-13 13:25:12 PST
Created attachment 122489 [details]
Patch
Comment 6 jochen 2012-01-13 13:26:12 PST
I added a test for starters
Comment 7 WebKit Review Bot 2012-01-13 16:13:54 PST
Comment on attachment 122489 [details]
Patch

Clearing flags on attachment: 122489

Committed r105009: <http://trac.webkit.org/changeset/105009>
Comment 8 WebKit Review Bot 2012-01-13 16:13:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 jochen 2012-01-13 23:14:46 PST
not yet fixed, that was just a test
Comment 10 Bill Budge 2012-01-17 14:08:00 PST
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
Comment 11 Nate Chapin 2012-01-17 14:57:43 PST
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.
Comment 12 jochen 2012-01-18 04:56:59 PST
ok, assigning to you then, Nate
Comment 13 Bill Budge 2012-01-18 16:12:36 PST
Created attachment 123030 [details]
Proposed Patch
Comment 14 Bill Budge 2012-01-18 16:15:39 PST
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 15 Alexey Proskuryakov 2012-01-18 16:22:01 PST
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.
Comment 16 Bill Budge 2012-01-18 16:49:58 PST
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.
Comment 17 Alexey Proskuryakov 2012-01-18 17:09:44 PST
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.
Comment 18 Bill Budge 2012-01-18 17:32:57 PST
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.
Comment 19 Bill Budge 2012-01-20 16:04:10 PST
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.
Comment 20 Alexey Proskuryakov 2012-01-20 16:20:05 PST
> 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.
Comment 21 Bill Budge 2012-01-23 16:11:14 PST
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.
Comment 23 Adam Barth 2012-02-08 14:43:28 PST
Comment on attachment 123030 [details]
Proposed Patch

What's the status of this patch?  Is this ready for review?
Comment 24 Alexey Proskuryakov 2012-02-08 15:10:52 PST
I think that we're waiting for it to be reconciled with editor's draft, and for more test coverage.
Comment 25 Bill Budge 2012-02-08 17:19:47 PST
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.
Comment 26 Adam Barth 2012-02-08 17:35:42 PST
Comment on attachment 123030 [details]
Proposed Patch

Ok.  I'm going to clear the review flag from this patch then.
Comment 27 Bill Budge 2012-02-21 11:35:42 PST
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.
Comment 28 Adam Barth 2012-02-21 15:15:41 PST
> 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?
Comment 29 Bill Budge 2012-02-21 15:50:39 PST
I think it's ready for checking against the spec (editor's draft). I don't know of any divergence, so it would be great if you'd have a look.
Comment 30 Bill Budge 2012-02-22 12:38:46 PST
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.
Comment 31 Bill Budge 2012-03-21 17:50:47 PDT
Created attachment 133157 [details]
Proposed Patch

Ready for review comments. I still need to rebaseline the existing access-control-and-redirects.html tests for the async case.
Comment 32 WebKit Review Bot 2012-03-21 18:36:27 PDT
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.
Comment 33 Bill Budge 2012-03-21 19:01:57 PDT
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.
Comment 34 WebKit Review Bot 2012-03-21 20:30:03 PDT
Comment on attachment 133166 [details]
Proposed Patch

Attachment 133166 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12070796

New failing tests:
http/tests/security/xhr-cors-redirect.html
Comment 35 WebKit Review Bot 2012-03-21 21:21:34 PDT
Comment on attachment 133166 [details]
Proposed Patch

Attachment 133166 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12070809

New failing tests:
http/tests/security/xhr-cors-redirect.html
Comment 36 Alexey Proskuryakov 2012-03-21 22:36:04 PDT
Comment on attachment 133166 [details]
Proposed Patch

Why do you mark these r+?
Comment 37 Bill Budge 2012-03-22 06:29:36 PDT
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 38 Adam Barth 2012-03-23 09:48:05 PDT
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->
Comment 39 Bill Budge 2012-03-23 10:30:53 PDT
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.
Comment 40 Adam Barth 2012-03-23 10:44:43 PDT
Comment on attachment 133508 [details]
Proposed Patch

This patch looks good to me.  Let's give Alexey a chance to comment before we land it.
Comment 41 WebKit Review Bot 2012-03-26 23:07:51 PDT
Comment on attachment 133508 [details]
Proposed Patch

Rejecting attachment 133508 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12146303
Comment 42 Adam Barth 2012-03-26 23:13:31 PDT
Comment on attachment 133508 [details]
Proposed Patch

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

> Source/WebCore/ChangeLog:5
> +        cross-origin XMLHttpRequest doesn't work with redirect
> +        https://bugs.webkit.org/show_bug.cgi?id=57600
> +

You need to leave the "Reviewed by NOBODY (OOPS!)." line in the ChangeLog for the bots to be smart enough to fill it out for you.
Comment 43 Adam Barth 2012-03-26 23:15:24 PDT
Created attachment 133983 [details]
Patch for landing
Comment 44 WebKit Review Bot 2012-03-27 00:39:07 PDT
Comment on attachment 133983 [details]
Patch for landing

Clearing flags on attachment: 133983

Committed r112217: <http://trac.webkit.org/changeset/112217>
Comment 45 WebKit Review Bot 2012-03-27 00:39:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 46 Andy Estes 2012-04-02 16:12:41 PDT
This broke the H&R Block tax website. See <https://bugs.webkit.org/show_bug.cgi?id=82964> for details.