Created attachment 280922 [details] WIP

Created attachment 281839 [details] Rebasing according 158728 patch

Created attachment 284700 [details] Patch

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

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

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

Created attachment 284726 [details] Fixing filtered response init

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

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

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

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

Created attachment 284759 [details] Patch for landing

Comment on attachment 284759 [details] Patch for landing Clearing flags on attachment: 284759 Committed r203815: <http://trac.webkit.org/changeset/203815>

All reviewed patches have been landed. Closing bug.

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?

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

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

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

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.

Reopening to attach new patch.

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

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

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

<rdar://problem/34995881>