Bug 112471

Summary: cross-origin requests redirected fail or drop author requested headers
Product: WebKit Reporter: Sufian Rhazi <sufian>
Component: New BugsAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Major CC: abarth, ancil0504, annevk, ap, cdumez, commit-queue, dpaddock, japhet, kbr, longley.dave, matt, youennf
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://origin-a.sigusrone.com/cors-redirect-accept-header
See Also: https://bugs.webkit.org/show_bug.cgi?id=144817
https://bugs.webkit.org/show_bug.cgi?id=159056
Bug Depends on:    
Bug Blocks: 151937    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Sufian Rhazi 2013-03-15 15:29:49 PDT
URL for test  case:
http://origin-a.sigusrone.com/cors-redirect-accept-header


Steps to reproduce the problem:
1. Visit http://origin-a.sigusrone.com/cors-redirect-accept-header
2. Click the "Run Tests" button


What is the expected behavior?
The test output shows all PASSes.

Firefox 15.0.1 successfully passes all tests


What actually happens?
An XMLHttpRequest from origin-a to origin-a that gets redirected to origin-b fails with status 0. Both origin-a and origin-b in this case return the correct CORS response headers.

An XMLHttpRequest from origin-a to origin-b that gets redirected drops the author requested headers set via xhr.setRequestHeader(). The tests demonstrate this with the "Accept" header.
Comment 1 Alexey Proskuryakov 2013-03-18 16:56:28 PDT
Is this the same as bug 98838?
Comment 2 Sufian Rhazi 2013-03-18 19:11:08 PDT
(In reply to comment #1)
> Is this the same as bug 98838?

Yeah, looks like it is. However, 98838 isn't really a real bug as far as I can tell. Setting the request Origin to null is part of the spec http://www.w3.org/TR/cors/

See step 6 of section 7.1.7.

However, the requested headers should not be dropped, and the failure case (where the xhr.status is 0) is definitely a bug.
Comment 3 A George 2013-06-28 06:11:47 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Is this the same as bug 98838?
> 
> 
> However, the requested headers should not be dropped, and the failure case (where the xhr.status is 0) is definitely a bug.

In my understanding, 'Access-Control-Allow-Headers' is received in response to a preflight request with 'Access-Control-Request-Headers'. 
It indicates the headers(user) that can be used in the the actual request.
Is this applicable for redirection as well as expected in this test?. Please correct me if I am wrong.

> An XMLHttpRequest from origin-a to origin-b that gets redirected drops the author requested headers set via xhr.setRequestHeader(). The tests demonstrate this with the "Accept" header.

The fix for this may be dependent on the bug https://bugs.webkit.org/show_bug.cgi?id=63460
Comment 4 A George 2013-07-03 06:10:11 PDT
(In reply to comment #0)
 
> What actually happens?
> An XMLHttpRequest from origin-a to origin-a that gets redirected to origin-b fails with status 0. Both origin-a and origin-b in this case return the correct CORS response headers.
> 
In my understanding it fails the Resource Sharing check(http://www.w3.org/TR/cors/#resource-sharing-check-0) hence the browser returns 'network error' and doesn't make the the redirected request.
Fails passesAccessControlCheck() in DocumentThreadableLoader::redirectReceived()

> An XMLHttpRequest from origin-a to origin-b that gets redirected drops the author requested headers set via xhr.setRequestHeader(). The tests demonstrate this with the "Accept" header.
>
This fails because the user added header are removed in the following lines

            // Remove any headers that may have been added by the network layer that cause access control to fail.
            request.clearHTTPContentType();
            request.clearHTTPReferrer();
            request.clearHTTPOrigin();
            request.clearHTTPUserAgent();
            request.clearHTTPAccept(); //This remove the application/json header
            makeCrossOriginAccessRequest(request);

Bill, Adam is it required the clear the simple headers, does the CORS spec recommends this?
Comment 5 Kenneth Russell 2013-07-03 06:44:20 PDT
The fix for https://code.google.com/p/chromium/issues/detail?id=226897 may have slightly improved the behavior in this area. At least the second test:

"this page (origin-a) -> origin-a.sigusrone.com/first-hop -> origin-b.sigusrone.com/second-hop"

gets further in the current Chrome Canary than WebKit nightly. I'd suggest porting the fix for that from Blink to WebKit. Further fixes would be great.

Note also that https://code.google.com/p/chromium/issues/detail?id=173727 , which seems to be a problem with CORS preflight requests, might be related as well.
Comment 6 Alexey Proskuryakov 2013-07-03 10:24:38 PDT
Bug 63460 discusses some related topics.
Comment 7 Dasa Paddock 2015-09-15 12:41:13 PDT
The test "this page (origin-a) -> origin-a.sigusrone.com/first-hop -> origin-b.sigusrone.com/second-hop" may have been a regression based on this comment: https://bugs.webkit.org/show_bug.cgi?id=57600#c39
Comment 8 longley.dave 2016-07-06 14:05:43 PDT
If you read through this bug report:

https://bugs.chromium.org/p/chromium/issues/detail?id=162183

It appears to be the same/similar problem -- and there's a link to the patch that fixed it in chromium.
Comment 9 Matt Fenelon 2016-07-26 06:32:59 PDT
I've just come up against this bug in Safari Version 9.1.1 (11601.6.17).

I'm using pdf.js to preview PDFs on a website, the original requests comes to my server, responding with a 302 redirect to an S3 presigned URL, this works in Chrome but in Safari it fails with a status of 0. I've debugged pdf.js and the failure comes from the xhr object itself, i.e. Safari.
Comment 10 youenn fablet 2016-07-27 03:45:50 PDT
Created attachment 284683 [details]
Patch
Comment 11 youenn fablet 2016-07-27 03:56:55 PDT
Patch fixes that issue.

It has special handling for Authorization header which should probably be dropped.

We may want to assert 'Origin' is not in the list of original headers as well.
Maybe the same for 'Referrer'

We may also want to drop 'Content-Type' when being redirected from whatever to GET and nullifying the body.

I am not sure whether there is any additional header we should drop or check.
Comment 12 Anne van Kesteren 2016-07-27 04:19:18 PDT
Per Fetch, all headers that are not added by the network layer (i.e., at the HTTP level; which that are is somewhat defined but not completely) will have to be added for each redirect (basically all headers in request's header list). If they require a CORS-preflight, that preflight has to be done.

Having said that, Accept does not require a CORS-preflight.

(I heard some refactoring of the WebKit networking-related code is being done to align with Fetch, you might want to work with those doing that.)
Comment 13 youenn fablet 2016-07-27 06:30:26 PDT
Created attachment 284689 [details]
Patch
Comment 14 youenn fablet 2016-07-27 06:33:24 PDT
(In reply to comment #13)
> Created attachment 284689 [details]
> Patch

I added an assert on Origin since it should really not be set in the headers anyway.

I kept the special-casing for Authorization to keep consistency with the existing behaviour. We might want to discuss removing this in the future.
Comment 15 Alex Christensen 2016-07-27 08:43:11 PDT
Comment on attachment 284689 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:101
> +    if (m_async)

Why don't we want to do this for synchronous xhr?
Comment 16 youenn fablet 2016-07-27 08:47:46 PDT
> Why don't we want to do this for synchronous xhr?

DocumentThreadableLoader is only made aware of redirections for asynchronous loads, not for synchronous loads.
Comment 17 Alex Christensen 2016-07-27 09:27:06 PDT
Comment on attachment 284689 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:102
> +        m_originalHeaders = request.httpHeaderFields();

Copying all the headers for every load would probably significantly hurt performance.  Can we do less?
Comment 18 youenn fablet 2016-07-27 10:05:24 PDT
(In reply to comment #17)
> Comment on attachment 284689 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284689&action=review
> 
> > Source/WebCore/loader/DocumentThreadableLoader.cpp:102
> > +        m_originalHeaders = request.httpHeaderFields();
> 
> Copying all the headers for every load would probably significantly hurt
> performance.  Can we do less?

To solve this particular issue, we could store only headers that were previously removed.
To implement fetch api and preflight after cross-origin redirections, it is more difficult. We would need to remove all headers added by underlying loaders.
FWIW, fetch is defined in terms of cloning requests to replay it if needed.
An alternative might be to store original headers in an array.
Comment 19 youenn fablet 2016-07-27 10:12:14 PDT
Also to be noted that this is storing only application-defined headers. The number should be rather small in most cases.
Comment 20 Alex Christensen 2016-08-01 19:24:02 PDT
Comment on attachment 284689 [details]
Patch

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

I'm warming up to this idea

> LayoutTests/http/tests/xmlhttprequest/redirections-and-user-headers-expected.txt:9
> +FAIL Check headers after cross-origin redirection to same-origin resource (not simple request) promise_test: Unhandled rejection with value: "Loading failure"

What would it take to make these all pass?
Comment 21 youenn fablet 2016-08-01 23:14:45 PDT
> > LayoutTests/http/tests/xmlhttprequest/redirections-and-user-headers-expected.txt:9
> > +FAIL Check headers after cross-origin redirection to same-origin resource (not simple request) promise_test: Unhandled rejection with value: "Loading failure"
> 
> What would it take to make these all pass?

Activate preflighting for cross-origin redirections.
In the current code, this means removing one test in DocumentThreadableLoader redirection checking.
There is a FIXME in latest patch of bug 160399 about this.
If we remove that check, replay will start and preflighting will happen.

For the tests to pass, we need to precisely regenerate the headers, like proposed in this patch.

Then we are done, except we might need writing more tests probably!
Comment 22 youenn fablet 2016-08-21 06:09:03 PDT
Created attachment 286565 [details]
Patch for landing
Comment 23 youenn fablet 2016-08-21 06:11:11 PDT
(In reply to comment #22)
> Created attachment 286565 [details]
> Patch for landing

I restricted header copy to CORS fetch mode, since this is only used in this mode currently.
Comment 24 WebKit Commit Bot 2016-08-21 06:38:29 PDT
Comment on attachment 286565 [details]
Patch for landing

Clearing flags on attachment: 286565

Committed r204693: <http://trac.webkit.org/changeset/204693>
Comment 25 WebKit Commit Bot 2016-08-21 06:38:35 PDT
All reviewed patches have been landed.  Closing bug.