Bug 158565 - Compute fetch response type in case of cross-origin requests
Summary: Compute fetch response type in case of cross-origin requests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 151937
  Show dependency treegraph
 
Reported: 2016-06-09 07:11 PDT by youenn fablet
Modified: 2017-10-14 16:52 PDT (History)
9 users (show)

See Also:


Attachments
WIP (24.19 KB, patch)
2016-06-09 08:14 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing according 158728 patch (15.18 KB, patch)
2016-06-22 08:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (69.42 KB, patch)
2016-07-27 08:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (797.39 KB, application/zip)
2016-07-27 09:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.43 MB, application/zip)
2016-07-27 09:51 PDT, Build Bot
no flags Details
Fixing filtered response init (69.54 KB, patch)
2016-07-27 13:53 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (70.02 KB, patch)
2016-07-28 01:01 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Local file doing XHR and trying getting access to Set-Cookie (2.10 KB, patch)
2017-04-03 11:07 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-06-09 07:11:23 PDT
Fetch response type should be set to Opaque or CrossOrigin according fetch spec.
Comment 1 youenn fablet 2016-06-09 08:14:54 PDT
Created attachment 280922 [details]
WIP
Comment 2 youenn fablet 2016-06-22 08:30:26 PDT
Created attachment 281839 [details]
Rebasing according 158728 patch
Comment 3 youenn fablet 2016-07-27 08:49:21 PDT
Created attachment 284700 [details]
Patch
Comment 4 Build Bot 2016-07-27 09:42:35 PDT
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
Comment 5 Build Bot 2016-07-27 09:42:39 PDT
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
Comment 6 Build Bot 2016-07-27 09:51:21 PDT
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
Comment 7 Build Bot 2016-07-27 09:51:25 PDT
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
Comment 8 youenn fablet 2016-07-27 13:53:59 PDT
Created attachment 284726 [details]
Fixing filtered response init
Comment 9 Alex Christensen 2016-07-27 23:59:53 PDT
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.
Comment 10 Alex Christensen 2016-07-28 00:08:54 PDT
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.
Comment 11 Alex Christensen 2016-07-28 00:10:34 PDT
There's a Document.  Let's keep the messages.
Comment 12 youenn fablet 2016-07-28 00:49:04 PDT
(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.
Comment 13 youenn fablet 2016-07-28 00:52:51 PDT
> > > 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
Comment 14 youenn fablet 2016-07-28 01:01:30 PDT
Created attachment 284759 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2016-07-28 01:31:28 PDT
Comment on attachment 284759 [details]
Patch for landing

Clearing flags on attachment: 284759

Committed r203815: <http://trac.webkit.org/changeset/203815>
Comment 16 WebKit Commit Bot 2016-07-28 01:31:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Geoffrey Garen 2017-03-31 17:00:07 PDT
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?
Comment 18 youenn fablet 2017-03-31 17:31:16 PDT
(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.
Comment 19 Geoffrey Garen 2017-04-02 12:47:17 PDT
> 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?
Comment 20 youenn fablet 2017-04-02 15:02:39 PDT
> 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.
Comment 21 youenn fablet 2017-04-02 15:16:31 PDT
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.
Comment 22 youenn fablet 2017-04-03 11:07:45 PDT
Reopening to attach new patch.
Comment 23 youenn fablet 2017-04-03 11:07:46 PDT
Created attachment 306085 [details]
Local file doing XHR and trying getting access to Set-Cookie
Comment 24 youenn fablet 2017-04-03 11:13:25 PDT
(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.
Comment 25 youenn fablet 2017-10-14 16:51:41 PDT
Closing bug as there does not seem to be anything remaining.
Comment 26 Radar WebKit Bug Importer 2017-10-14 16:52:22 PDT
<rdar://problem/34995881>