Bug 65694

Created attachment 102933 [details] proposed patch

Comment on attachment 102933 [details] proposed patch I don't see why this change is needed. Both DenyCrossOriginRequests and AllowCrossOriginRequests never use preflight, so that's what EventSource should be using.

(In reply to comment #2) > (From update of attachment 102933 [details]) > I don't see why this change is needed. Both DenyCrossOriginRequests and AllowCrossOriginRequests never use preflight, so that's what EventSource should be using. The change is needed to add CORS support to EventSource. I think that both those two options disable CORS.

Comment on attachment 102933 [details] proposed patch Attachment 102933 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9303514

Comment on attachment 102933 [details] proposed patch Attachment 102933 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9305486

I guess the patch is OK then, although I'm still surprised that EventSource doesn't use preflight, and can hit unprepared servers with a new kind of request.

In what sense is the request a new kind of request? Do you just mean that new HTTP headers?

Created attachment 102966 [details] updated patch Tweaked if-statement to avoid compiler warning.

(In reply to comment #6) > I guess the patch is OK then, although I'm still surprised that EventSource doesn't use preflight, and can hit unprepared servers with a new kind of request. Since the request method will always be GET and the headers can't be affected by script authors I don't think the request will be any different from what can already be accomplished with an image or script load. Also, the Last-Event-ID header will never be present in the first request.

Comment on attachment 102966 [details] updated patch Attachment 102966 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9304609

(In reply to comment #7) > In what sense is the request a new kind of request? Do you just mean that new HTTP headers? Yes. Even though headers cannot be changed by script authors, the ability to choose which headers to hit a different origin server with by choosing a particular HTML API is a bit unsettling.

I agree that the new headers could be problematic. We should definitely keep an eye out for problems. However, I think the risk is worth it. (Much like the risk of the new headers introduced by CORS are probably worth it.)

Comment on attachment 102966 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=102966&action=review r=me assuming build fix and a better name for enum value. > Source/WebCore/loader/DocumentThreadableLoader.cpp:101 > + if ((m_options.preflightPolicy == ConditionPreflight && isSimpleCrossOriginAccessRequest(crossOriginRequest->httpMethod(), crossOriginRequest->httpHeaderFields())) || m_options.preflightPolicy == PreventPreflight) I'd order this differently to avoid calling into isSimpleCrossOriginAccessRequest() when PreventPreflight is specified. > Source/WebCore/loader/DocumentThreadableLoader.cpp:115 > + ASSERT(m_options.preflightPolicy == PreventPreflight || isSimpleCrossOriginAccessRequest(request.httpMethod(), request.httpHeaderFields())); Please add ASSERT(m_options.preflightPolicy != ForcePreflight) for completeness. > Source/WebCore/loader/ThreadableLoader.h:60 > + ConditionPreflight, Condition as a verb has a different meaning than you want here. Maybe "MayUsePreflight" or "AllowPreflight"?

Created attachment 103070 [details] patch 3 (In reply to comment #13) > (From update of attachment 102966 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102966&action=review > > r=me assuming build fix and a better name for enum value. > Fixed chromium issue. > > Source/WebCore/loader/DocumentThreadableLoader.cpp:101 > > + if ((m_options.preflightPolicy == ConditionPreflight && isSimpleCrossOriginAccessRequest(crossOriginRequest->httpMethod(), crossOriginRequest->httpHeaderFields())) || m_options.preflightPolicy == PreventPreflight) > > I'd order this differently to avoid calling into isSimpleCrossOriginAccessRequest() when PreventPreflight is specified. > The first evaluation will ensure that isSimpleCrossOriginAccessRequest() is not called if preflightPolicy is ForcePreflight or PreventPreflight. That is the default case and probably the most common one so that's why I put it first. > > Source/WebCore/loader/DocumentThreadableLoader.cpp:115 > > + ASSERT(m_options.preflightPolicy == PreventPreflight || isSimpleCrossOriginAccessRequest(request.httpMethod(), request.httpHeaderFields())); > > Please add ASSERT(m_options.preflightPolicy != ForcePreflight) for completeness. > Fixed. > > Source/WebCore/loader/ThreadableLoader.h:60 > > + ConditionPreflight, > > Condition as a verb has a different meaning than you want here. Maybe "MayUsePreflight" or "AllowPreflight"? I wasn't entirely happy with that name but I still would like the option to reflect that CORS rules will determine whether a preflight is made when that policy is set. My new proposal is 'ConsiderPreflight'. If you don't like that name I have no problem changing to one of the two that you suggested.

(In reply to comment #12) > I agree that the new headers could be problematic. We should definitely keep an eye out for problems. However, I think the risk is worth it. (Much like the risk of the new headers introduced by CORS are probably worth it.) What headers are you talking about? The only special headers sent in an initial EventSource request are (except any CORS headers): Accept: text/event-stream Cache-Control: no-cache (for the Last-Event-ID header ever to be sent the server must allow the origin and also provide a valid event-stream with id fields in it) Since Accept is in the simple header whitelist, it can already be sent without preflight using XHR. That leaves Cache-Control, but isn't that header sent cross-origin in some other cases? Anyway it seems rather harmless to me.

Last-Event-ID could end up sent to a random host if there is a redirect after a reconnect.

(In reply to comment #16) > Last-Event-ID could end up sent to a random host if there is a redirect after a reconnect. Ahh, clever. Should that be allowed?

Comment on attachment 103070 [details] patch 3 ConsiderPreflight is fine with me. I don't have a strong enough opinion about getting rid of preflight for EventSource. Note however that EventSource connections are long lived, so the cost of preflight is low.

(In reply to comment #18) > I don't have a strong enough opinion about getting rid of preflight for EventSource. Note however that EventSource connections are long lived, so the cost of preflight is low. It's more work for the event-stream provider to handle the preflight request, so I don't think we should have it unless we're convinced that it's needed. Can we land this patch? The patch to CORS-enable EventSource depends on it.

As far as I'm concerned, yes, you can land this patch.

(In reply to comment #20) > As far as I'm concerned, yes, you can land this patch. OK, then I would appreciate if you could cq+

Comment on attachment 103070 [details] patch 3 Clearing flags on attachment: 103070 Committed r93182: <http://trac.webkit.org/changeset/93182>

All reviewed patches have been landed. Closing bug.