Bug 116075 – Fix problems with cross-origin redirects

• Component: Page Loading
• Status: RESOLVED FIXED

[Ryosuke Niwa]

We might want to merge
https://chromium.googlesource.com/chromium/blink/+/7ea774e478f84f355748108d2aaabca15355d512

Three problems exist in the current code:

1) If a same-origin request causes a redirect to a different origin,
   do not enforce access control checks for the redirect response
   itself, because the request which resulted in the redirect was
   same-origin.

2) If a same-origin request causes a redirect to a different origin,
   use the original request's URL as the origin for the new request;
   do not use a unique security origin.

3) Track whether the client (i.e., XMLHttpRequest) actually requested
   that credentials be sent in the first place. When a same-origin
   request redirects to a different origin, the original request will
   send cookies whether requested or not, because it is same-origin.
   The new cross-origin request should not send cookies unless they
   were requested, so that the access control checks on the response
   will succeed if the server granted "Access-Control-Allow-Origin=*".

[youenn fablet]

Created attachment 224351 [details]
Patch

[youenn fablet]

ping review

[Brent Fulgham]

This patch doesn't apply cleanly anymore. I'd like to try to integrate this if we could get something current. (Sorry it's sat here for so long).

Is there any hope we could get a revised patch?

[youenn fablet]

I can take a look tomorrow if that helps

[Brent Fulgham]

(In reply to comment #4)
> I can take a look tomorrow if that helps

Sure! I'm just so sorry no one looked at it until now. :-(

[youenn fablet]

Created attachment 268751 [details]
Rebasing

[Build Bot]

Comment on attachment 268751 [details]
Rebasing

Attachment 268751 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/682017

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-non-cors.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-cors.htm

[Build Bot]

Created attachment 268753 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268751 [details]
Rebasing

Attachment 268751 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/682007

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-non-cors.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-cors.htm

[Build Bot]

Created attachment 268754 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268751 [details]
Rebasing

Attachment 268751 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/682020

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-non-cors.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-cors.htm

[Build Bot]

Created attachment 268755 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 268757 [details]
Rebasing wpt tests

[Build Bot]

Comment on attachment 268757 [details]
Rebasing wpt tests

Attachment 268757 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/682411

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-non-cors.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-cors.htm

[Build Bot]

Created attachment 268758 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268757 [details]
Rebasing wpt tests

Attachment 268757 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/682413

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-non-cors.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-cors.htm

[Build Bot]

Created attachment 268759 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268757 [details]
Rebasing wpt tests

Attachment 268757 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/682417

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-non-cors.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-cors.htm

[Build Bot]

Created attachment 268760 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 268761 [details]
Rebasing wpt tests

[youenn fablet]

(In reply to comment #5)
> (In reply to comment #4)
> > I can take a look tomorrow if that helps

I did a quick rebase.

I did not check whether removing Accept-Encoding is still needed in Mac or not.
Is it still needed?

I skipped some WPT tests that require access to URLS like:
- example.not: no server can be reached
- www2.localhost: localhost server should be reached
These tests require DRT and WTR to stop blocking these URLS.
They also require to update the bots to map localhost-like urls (www2.localhost) to 127.0.0.1.
I filed bug 127676 some time ago for that. Another approach might be to define a list of allowed hostnames somewhere.

> Sure! I'm just so sorry no one looked at it until now. :-(

Yeah, we, WebKit community should definitely improve on this...

[Michael Catanzaro]

Comment on attachment 268761 [details]
Rebasing wpt tests

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

> LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-cors.htm:17
> +console.log(client.readyState);

Looks like a debugging leftover that needs removed.

> LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-cors.htm:33
> +//      redirect("303")

Ditto.

[youenn fablet]

Created attachment 268765 [details]
Removing WPT test changes

[youenn fablet]

(In reply to comment #23)
> Created attachment 268765 [details]
> Removing WPT test changes

Thanks, this is fixed in the latest patch.

[Brent Fulgham]

Comment on attachment 268765 [details]
Removing WPT test changes

I think this looks good. Let's wait for 'win' to finish, and for Dan Bates to double-check it, then land it. Dan, please r+ this if you agree with the change.

[Daniel Bates]

Comment on attachment 268765 [details]
Removing WPT test changes

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

> LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus-expected.txt:3
> +FAIL: Timed out waiting for notifyDone to be called

This is OK-as-is. I know that this test is expected to time out because WTR/DTR block access to www2.localhost and example.not. Even though this is a timeout failure it seems weird to both include an expected result that has "FAIL" and mark the test as skipped in the TestExpectation file. (I mean, assuming this was not a timeout failure - just a test that failed such sub-test, then the test would be considered to succeed if someone ever removed the test from the list of skipped tests in the TestExpectation file and the failure of the sub-test would likely not be noticed). Unless it is not straightforward to do, I suggest landing the expected success result for this test and mark this test as skipped. Then unskipping this test will lead to a noticeable test failure (currently a timeout) so long as WTR/DRT continue to block www2.localhost and example.not. This also has the benefit of making it straightforward to reason about the state of this test should there be other issues besides the blocking of www2.localhost and example.not since the expected result file represents the actual expected result.

> LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-cors-expected.txt:2
> +FAIL: Timed out waiting for notifyDone to be called

Ditto.

> LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-to-non-cors-expected.txt:5
> +FAIL: Timed out waiting for notifyDone to be called

Ditto.

[youenn fablet]

Created attachment 268943 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 268943 [details]
Patch for landing

Clearing flags on attachment: 268943

Committed r195010: <http://trac.webkit.org/changeset/195010>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.