Bug 158565 – Compute fetch response type in case of cross-origin requests

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Fetch response type should be set to Opaque or CrossOrigin according fetch spec.

[youenn fablet]

Created attachment 280922 [details]
WIP

[youenn fablet]

Created attachment 281839 [details]
Rebasing according 158728 patch

[youenn fablet]

Created attachment 284700 [details]
Patch

[Build Bot]

Comment on attachment 284700 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/get-dangerous-headers.html

[Build Bot]

Created attachment 284703 [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 284700 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/get-dangerous-headers.html

[Build Bot]

Created attachment 284704 [details]
Archive of layout-test-results from ews113 for mac-yosemite

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

[youenn fablet]

Created attachment 284726 [details]
Fixing filtered response init

[Alex Christensen]

Comment on attachment 284726 [details]
Fixing filtered response init

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

> Source/WebCore/platform/network/ResourceResponseBase.cpp:102
> +ResourceResponse ResourceResponseBase::filterResponse(const ResourceResponse& response, ResourceResponse::Tainting tainting)

This should take a ResourceResponse&&.  Then we wouldn't need to copy the response.  DocumentThreadableLoader::didReceiveResponse would also need to do the same, but I think it's only called from DocumentThreadableLoader::loadRequest, so no problem!

> LayoutTests/http/tests/xmlhttprequest/get-dangerous-headers-expected.txt:-1
> -CONSOLE MESSAGE: line 15: Refused to get unsafe header "Set-Cookie"

I think we could keep these messages by passing a ScriptExecutionContext* around.

[Alex Christensen]

Comment on attachment 284726 [details]
Fixing filtered response init

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:291
> +        m_client->didReceiveResponse(identifier, ResourceResponse::filterResponse(response, tainting));

We do the filtering here.  There's no ScriptExecutionContext.  Chrome still has the messages in the console somehow, but Firefox does not.  If we want the messages in the console, we can find a way to put them back in.

[Alex Christensen]

There's a Document.  Let's keep the messages.

[youenn fablet]

(In reply to comment #9)
> Comment on attachment 284726 [details]
> Fixing filtered response init
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284726&action=review
> 
> > Source/WebCore/platform/network/ResourceResponseBase.cpp:102
> > +ResourceResponse ResourceResponseBase::filterResponse(const ResourceResponse& response, ResourceResponse::Tainting tainting)
> 
> This should take a ResourceResponse&&.  Then we wouldn't need to copy the
> response.  DocumentThreadableLoader::didReceiveResponse would also need to
> do the same, but I think it's only called from
> DocumentThreadableLoader::loadRequest, so no problem!

The response object comes from CachedResource and is made const.
I do not think we want to touch it at this point since it can be reused by other DocumentThreadableLoader.

To limit the perf penalty, I was thinking we could change ThreadableLoaderClient::didReceiveResponse to take a ResourceResponse&&
I will add a FIXME.

> > LayoutTests/http/tests/xmlhttprequest/get-dangerous-headers-expected.txt:-1
> > -CONSOLE MESSAGE: line 15: Refused to get unsafe header "Set-Cookie"
> 
> I think we could keep these messages by passing a ScriptExecutionContext*
> around.

It might be wordy to add a message for any filtered request.
Also, if the message is good for XHR, it would be good for fetch API as well.
I will file a bug to track the issue.
I am wondering whether we should not beef up the inspector so as to be able to inspect each request & response headers.
It would be cool if the inspector could let know which headers are visible and which are not.

[youenn fablet]

> > > LayoutTests/http/tests/xmlhttprequest/get-dangerous-headers-expected.txt:-1
> > > -CONSOLE MESSAGE: line 15: Refused to get unsafe header "Set-Cookie"
> > 
> > I think we could keep these messages by passing a ScriptExecutionContext*
> > around.
> 
> It might be wordy to add a message for any filtered request.
> Also, if the message is good for XHR, it would be good for fetch API as well.
> I will file a bug to track the issue.
> I am wondering whether we should not beef up the inspector so as to be able
> to inspect each request & response headers.
> It would be cool if the inspector could let know which headers are visible
> and which are not.

See bug 160286

[youenn fablet]

Created attachment 284759 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 284759 [details]
Patch for landing

Clearing flags on attachment: 284759

Committed r203815: <http://trac.webkit.org/changeset/203815>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Does the spec say that we're supposed to filter the Set-Cookie header even if the server responds with Access-Control-Allow-Credentials: true? Can you link to that if so?

[youenn fablet]

(In reply to Geoffrey Garen from comment #17)
> Does the spec say that we're supposed to filter the Set-Cookie header even
> if the server responds with Access-Control-Allow-Credentials: true? Can you
> link to that if so?

You might be interested in https://fetch.spec.whatwg.org/#concept-filtered-response-cors and https://fetch.spec.whatwg.org/#forbidden-response-header-name.

I don't really see the link between Set-Cookie and Access-Control-Allow-Credentials: true. If we are leaking "Set-Cookie" in some cases, we should probably fix it.

[Geoffrey Garen]

> You might be interested in
> https://fetch.spec.whatwg.org/#concept-filtered-response-cors and
> https://fetch.spec.whatwg.org/#forbidden-response-header-name.

Thanks!

> I don't really see the link between Set-Cookie and
> Access-Control-Allow-Credentials: true. If we are leaking "Set-Cookie" in
> some cases, we should probably fix it.

I've seen some documentation on the web that suggests the following method of authentication with a third party identity service:

1. Make a cross-origin XHR to the third party service, withCredentials: true.

2. The service responds with Set-Cookie: X, Access-Control-Allow-Credentials: true.

3. Extract the Set-Cookie header from the response, and now you know you're logged in with identity X.

This patch broke the above method of authentication in at least one app. Are we sure we're supposed to break that method? How else should developers authenticate with third party services?

At first glance, this part of the spec seems to indicate that the method is supported: <https://fetch.spec.whatwg.org/#example-cors-with-credentials>. Am I misreading?

[youenn fablet]

> I've seen some documentation on the web that suggests the following method
> of authentication with a third party identity service:
> 
> 1. Make a cross-origin XHR to the third party service, withCredentials: true.

OK

> 2. The service responds with Set-Cookie: X,
> Access-Control-Allow-Credentials: true.

Sounds good too, the user-agent will process 'Set-Cookie' and store the cookie, which will allow reuse of it in the future.

> 3. Extract the Set-Cookie header from the response, and now you know you're
> logged in with identity X.

OK if done by the user agent. Generally not ok if done by the web app.
Cookie and Set-Cookie are supposed to remain in browser control and not readable/writable from a web page.

> This patch broke the above method of authentication in at least one app. Are
> we sure we're supposed to break that method? How else should developers
> authenticate with third party services?
> 
> At first glance, this part of the spec seems to indicate that the method is
> supported: <https://fetch.spec.whatwg.org/#example-cors-with-credentials>.
> Am I misreading?

This example seems fine to me.
Might be worth a test to try exposing Set-Cookie with credentials mode set to include and Access-Control-Expose-Headers including Set-Cookie.

[youenn fablet]

Submitted https://github.com/w3c/web-platform-tests/pull/5315 for a test on Set-Cookie visibility in credentials=include mode.
There should be a testable link on that GitHub web site and results from Chrome and Firefox.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 306085 [details]
Local file doing XHR and trying getting access to Set-Cookie

[youenn fablet]

(In reply to youenn fablet from comment #23)
> Created attachment 306085 [details]
> Local file doing XHR and trying getting access to Set-Cookie

This patch contains a test showing file:// pages using XHR trying to get access to Set-Cookie.
Before that patch, it is able to do so. After the patch, it cannot.
Chrome and Firefox are also disallowing the page to get Set-Cookie header value.

[youenn fablet]

Closing bug as there does not seem to be anything remaining.

[Radar WebKit Bug Importer]

<rdar://problem/34995881>