Bug 181330 – Fetch algorithm does not strip out URL credentials in case of redirections

• Component: Service Workers
• Status: NEW

[youenn fablet]

Instead, when doing CORS, it fails loading if the redirect URL contains credentials

[youenn fablet]

Created attachment 330555 [details]
Patch

[youenn fablet]

This is probably sensitive here so will need additional discussions and tests to go forward.

[EWS Watchlist]

Comment on attachment 330555 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-credentials.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-credentials.any.worker.html
http/tests/xmlhttprequest/access-control-and-redirects-async.html

[EWS Watchlist]

Created attachment 330560 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

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

[EWS Watchlist]

Comment on attachment 330555 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-credentials.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-credentials.any.worker.html
http/tests/xmlhttprequest/access-control-and-redirects-async.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 330555 [details]
Patch

Attachment 330555 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5943485

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-credentials.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-credentials.any.worker.html
http/tests/xmlhttprequest/access-control-and-redirects-async.html

[EWS Watchlist]

Created attachment 330563 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 330555 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-credentials.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-credentials.any.worker.html
http/tests/xmlhttprequest/access-control-and-redirects-async.html

[EWS Watchlist]

Created attachment 330564 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

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

[youenn fablet]

Created attachment 330587 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 330587 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        * http/tests/xmlhttprequest/access-control-and-redirects-async.html:

Do other browsers behave in accordance with the updated test?

[youenn fablet]

(In reply to Alexey Proskuryakov from comment #12)
> Comment on attachment 330587 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330587&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        * http/tests/xmlhttprequest/access-control-and-redirects-async.html:
> 
> Do other browsers behave in accordance with the updated test?

Chrome and Firefox are behaving consistently with the proposed patch.
The updated test is somehow redundant with rebased fetch API tests which are also passing consistently in Chrome and Firefox (see https://wpt.fyi/fetch/api/cors/cors-redirect-credentials.any.html).

[youenn fablet]

I guess we should also update the soup backend, not sure about curl backend...

[Michael Catanzaro]

(In reply to youenn fablet from comment #14)
> I guess we should also update the soup backend, not sure about curl
> backend...

CCing Don for curl

[Michael Catanzaro]

(In reply to youenn fablet from comment #14)
> I guess we should also update the soup backend, not sure about curl
> backend...

I updated the soup backend.

Not sure how to handle curl either... it's different.

[Michael Catanzaro]

Created attachment 330652 [details]
Patch

[Basuke Suzuki]

Okay, we'll send our patch pretty soon. About redirect on CORS, as youenn said in the beginning, do we have the agreement for the behavior when the credential will be attached on the redirected request and it may fail?

[youenn fablet]

(In reply to Basuke Suzuki from comment #18)
> Okay, we'll send our patch pretty soon. About redirect on CORS, as youenn
> said in the beginning, do we have the agreement for the behavior when the
> credential will be attached on the redirected request and it may fail?

Other browsers seem to do so consistently for fetch/XHR. That seems a safe change
I want to add some more tests though:
- cors image loading
- no-cors image loading
- main resource loading

If there is consistency amongst other browsers for those cases as well, I think we should align.

[youenn fablet]

> Other browsers seem to do so consistently for fetch/XHR. That seems a safe
> change
... safe change for fetch/XHR.

[Basuke Suzuki]

(In reply to youenn fablet from comment #20)
> > Other browsers seem to do so consistently for fetch/XHR. That seems a safe
> > change
> ... safe change for fetch/XHR.

Got it.

[Basuke Suzuki]

Created attachment 330726 [details]
Patch

Added Curl patch

[Basuke Suzuki]

youenn, I failed to create a patch. Can you just please delete a line from Curl port?

Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.cpp:355

[youenn fablet]

(In reply to Basuke Suzuki from comment #23)
> youenn, I failed to create a patch. Can you just please delete a line from
> Curl port?
> 
> Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.cpp:355

Sure, thanks for the filename, I'll update accordingly.

[youenn fablet]

Created attachment 330894 [details]
Added a new test

[youenn fablet]

Created attachment 330897 [details]
Added a new test

[youenn fablet]

Based on the testing (see patch), Firefox is as per spec and WebKit with this patch would be aligned.
Chrome is forbidding any subresource load containing credentials in redirects (https://www.chromestatus.com/feature/5669008342777856).

[youenn fablet]

Filed https://github.com/whatwg/fetch/issues/660

[EWS Watchlist]

Comment on attachment 330897 [details]
Added a new test

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

New failing tests:
http/tests/workers/service/postmessage-after-sw-process-crash.https.html

[EWS Watchlist]

Created attachment 330963 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

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

[EWS Watchlist]

Comment on attachment 330897 [details]
Added a new test

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[EWS Watchlist]

Created attachment 330985 [details]
Archive of layout-test-results from ews100 for mac-sierra

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

[Radar WebKit Bug Importer]

<rdar://problem/36881479>