Bug 112471

Is this the same as bug 98838?

(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.

(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

(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?

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.

Bug 63460 discusses some related topics.

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

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.

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.

Created attachment 284683 [details] Patch

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.

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.)

Created attachment 284689 [details] Patch

(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 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?

> 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 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?

(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.

Also to be noted that this is storing only application-defined headers. The number should be rather small in most cases.

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?

> > 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!

Created attachment 286565 [details] Patch for landing

(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 on attachment 286565 [details] Patch for landing Clearing flags on attachment: 286565 Committed r204693: <http://trac.webkit.org/changeset/204693>

All reviewed patches have been landed. Closing bug.