Bug 63460

Created attachment 98735 [details] proposed patch

Interesting approach. I'm going to let ap take a first crack at reviewing this patch.

Comment on attachment 98735 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=98735&action=review The implementation cannot work on Mac, because header status will be lost after a round-trip through NSURLRequest (or any other HTTP library request object). WebKit needs this round-trip because it has delegate methods where it informs client applications about requests that are about to be made. ResourceRequest is ah HTTP level notion, and it shouldn't have any knowledge of Web platform concepts. Secondly, when we talk about author vs. implementation, where do headers added by client applications fit? It kind of sounds like it's "implementation", but we can't know if those applications use untrusted content to decide which headers to add. > LayoutTests/ChangeLog:6 > + CORS should only deal with request headers set by script authors. > + https://bugs.webkit.org/show_bug.cgi?id=63460 That's an interesting idea, although it seems somewhat questionable to me. The design principle use to be to avoid hitting servers with anything they couldn't be hit with through form submission, and the new rule deviates from that pretty far. > Source/WebCore/platform/network/ResourceRequestBase.h:47 > + enum ResourceRequestHeaderStatus { Naming nit: I would have liked something like "origination" or "source" more than "status".

One more question: what about Content-Type headers that are not set by authors? Currently XMLHttpRequest defaults to application/xml, which triggers preflight. Should the same logic be applied there?

(In reply to comment #3) I appreciate the quick review. > The implementation cannot work on Mac, because header status will be lost after a round-trip through NSURLRequest (or any other HTTP library request object). WebKit needs this round-trip because it has delegate methods where it informs client applications about requests that are about to be made. > I realize that the header status might get lost when platforms deal with the request. However, the header status is only meant to be used by the CORS implementation and that happens before the status is lost. I was not able to build on Mac with the patch applied because of the optional arguments but once that was fixed I achieved the desired behavior on Mac as well. > ResourceRequest is ah HTTP level notion, and it shouldn't have any knowledge of Web platform concepts. > Could you recommend a different approach to fixing this bug? The included test demonstrates a deviation in the implementation from the current CORS spec (Origin being sent in Access-Control-Request-Headers header). To enable CORS in EventSource this patch was going to prevent a preflight from the two headers added by the implementation (thus not in the list of simple headers) but I guess I could solve that with a flag in ThreadableLoaderOptions instead. > Secondly, when we talk about author vs. implementation, where do headers added by client applications fit? It kind of sounds like it's "implementation", but we can't know if those applications use untrusted content to decide which headers to add. > Can clients invoke CORS? I would say that client added headers are implementation headers and that it is up to clients to safely add headers. > > LayoutTests/ChangeLog:6 > > + CORS should only deal with request headers set by script authors. > > + https://bugs.webkit.org/show_bug.cgi?id=63460 > > That's an interesting idea, although it seems somewhat questionable to me. The design principle use to be to avoid hitting servers with anything they couldn't be hit with through form submission, and the new rule deviates from that pretty far. > I think the reasoning is that headers added by the implementation are safe even for cross origin requests. With this clarification the CORS specification does not have to be updated for every new specification that wants to add a header without causing a preflight request to be made. Also, there is only one list of simple headers so if Last-Event-ID was to be added back then you could use this header in XHR without preflight as well (not sure if this is a problem though, just an interesting consequence). > > Source/WebCore/platform/network/ResourceRequestBase.h:47 > > + enum ResourceRequestHeaderStatus { > > Naming nit: I would have liked something like "origination" or "source" more than "status". Yes, you are right, "source" is much better than "status". Anything related to "origin" should probably be avoided in this context though. (In reply to comment #4) > One more question: what about Content-Type headers that are not set by authors? Currently XMLHttpRequest defaults to application/xml, which triggers preflight. Should the same logic be applied there? The Content-Type headers set by the XHR implementation would fall under the category "implicitly set by authors", see the second paragraph in http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/1222.html

> Could you recommend a different approach to fixing this bug? I don't have a lot of detail to give, but the source of headers would need to be tracked outside of ResourceRequest. Or perhaps the code could be refactored so that implementation headers would be added after deciding whether to make a simple request. It seems that this change to the spec hasn't really settled yet, it that correct?

Need to figure out how to phrase the requirement on Content-Type adequately. People seemed to think however that this is what the specification should have said all along.

Created attachment 104586 [details] Patch

Added depends link since I wrote this code using the patch from 66340.

Comment on attachment 104586 [details] Patch Attachment 104586 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9439468

Comment on attachment 104586 [details] Patch Attachment 104586 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9435513

(In reply to comment #8) > Created an attachment (id=104586) [details] > Patch What about request headers set by the implementation that are not in UserAgentHeaderData such as Cache-Control and Last-Event-ID set by EventSource? Those headers should trigger preflight when set by authors using XHR and thus can't be added to the list. That's why I tried to keep track of which headers that are actually set by the implementation and which are set by the author.

Comment on attachment 104586 [details] Patch Attachment 104586 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9438514

+ m_userAgentHeaders.add("accept-charset"); + m_userAgentHeaders.add("accept-encoding"); + m_userAgentHeaders.add("access-control-request-headers"); Despite what I said in comment 1, I'm not sure if it's the best approach to move headers that are special cased in XMLHttpRequest to some shared location. It's ThreadableLoader client (in this case, XHR) who really knows if JavaScript code called one of those methods that added non-engine headers. Many other clients simply don't provide any way to specify custom headers, so doing checks on these is pointless. What do you think?

Comment on attachment 104586 [details] Patch Attachment 104586 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9438527

Comment on attachment 104586 [details] Patch Attachment 104586 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9438561

Comment on attachment 104586 [details] Patch Withdrawn. I'll leave this bug for others.

(In reply to comment #12) > (In reply to comment #8) > > Created an attachment (id=104586) [details] [details] > > Patch > > What about request headers set by the implementation that are not in UserAgentHeaderData such as Cache-Control and Last-Event-ID set by EventSource? Those headers should trigger preflight when set by authors using XHR and thus can't be added to the list. That's why I tried to keep track of which headers that are actually set by the implementation and which are set by the author. I don't think that keeping track of headers sent by the author and headers set by the implementation is the correct way of thinking about the problem. imo, instead one should think of what headers to whitelist. It really doesn't matter how they are set if the request could do malicious things on the server which seems to be the real purpose of deciding whether to do a preflight request or not. Regardless, I've withdrawn this patch to allow anyone else to take it up as they see fit.

> It really doesn't matter how they are set if the request could do malicious things on the server which seems to be the real purpose of deciding whether to do a preflight request or not. That's my thinking, too, but the current spec draft disagrees, only talking about how the headers are set.

One particularly unfortunate consequence of this is that when Do Not Track is enabled in Safari preferences, all CORS requests become non-simple ones, and start using preflight.

(In reply to comment #20) > One particularly unfortunate consequence of this is that when Do Not Track > is enabled in Safari preferences, all CORS requests become non-simple ones, > and start using preflight. I don't know where is happening the insertion of DNT header in that case. I guess that if they are inserted after core preflight checker, this should work nicely. For WebKit GStreamer (bug 131484), although non-simple headers may be added, I think preflight is never used. This makes sense to me.

(In reply to comment #21) > I guess that if they are inserted after core preflight checker, this should > work nicely. That is definitely how Fetch approaches this. DNT is set with other headers just before the request goes to the network. Notably, this is after service workers. See step 12 of https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch. It's a little vague still, but hopefully that will get better over time. Now, it is a problem that user agents are somehow exempt from the same-origin policy and we keep introducing new headers that we emit across origins and servers might get tripped up by. I don't have a good story for that yet. Nobody seems to really think about it that when they add DNT to all requests, they also violate the implicit agreements around the same-origin policy.

The DNT header is inserted by Safari injected bundle code in a client callback.

I believe this is by-and-large working as intended now.