Bug 65694 – Make it possible to explicitly prevent a preflight via ThreadableLoaderOptions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per-Erik Brodin]

Some APIs, like EventSource, will never trigger a preflight so there is really no need to check the preflight conditions. We should make it possible to explicitly prevent a preflight via an option in ThreadableLoaderOptions. One way would be to let the existing forcePreflight option become a condition/force/prevent policy option.

[Per-Erik Brodin]

Created attachment 102933 [details]
proposed patch

[Alexey Proskuryakov]

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.

[Per-Erik Brodin]

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

[Early Warning System Bot]

Comment on attachment 102933 [details]
proposed patch

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

[WebKit Review Bot]

Comment on attachment 102933 [details]
proposed patch

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

[Alexey Proskuryakov]

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.

[Ian 'Hixie' Hickson]

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

[Per-Erik Brodin]

Created attachment 102966 [details]
updated patch

Tweaked if-statement to avoid compiler warning.

[Per-Erik Brodin]

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

[WebKit Review Bot]

Comment on attachment 102966 [details]
updated patch

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

[Alexey Proskuryakov]

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

[Ian 'Hixie' Hickson]

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

[Alexey Proskuryakov]

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

[Per-Erik Brodin]

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.

[Per-Erik Brodin]

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

[Ian 'Hixie' Hickson]

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

[Per-Erik Brodin]

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

[Alexey Proskuryakov]

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.

[Per-Erik Brodin]

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

[Alexey Proskuryakov]

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

[Per-Erik Brodin]

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

[WebKit Review Bot]

Comment on attachment 103070 [details]
patch 3

Clearing flags on attachment: 103070

Committed r93182: <http://trac.webkit.org/changeset/93182>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.