Bug 65694 - Make it possible to explicitly prevent a preflight via ThreadableLoaderOptions
Summary: Make it possible to explicitly prevent a preflight via ThreadableLoaderOptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 61862
  Show dependency treegraph
 
Reported: 2011-08-04 09:20 PDT by Per-Erik Brodin
Modified: 2011-08-16 17:16 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (6.31 KB, patch)
2011-08-04 09:28 PDT, Per-Erik Brodin
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
updated patch (6.32 KB, patch)
2011-08-04 13:15 PDT, Per-Erik Brodin
ap: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch 3 (7.82 KB, patch)
2011-08-05 07:20 PDT, Per-Erik Brodin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per-Erik Brodin 2011-08-04 09:20:16 PDT
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.
Comment 1 Per-Erik Brodin 2011-08-04 09:28:00 PDT
Created attachment 102933 [details]
proposed patch
Comment 2 Alexey Proskuryakov 2011-08-04 09:37:36 PDT
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.
Comment 3 Per-Erik Brodin 2011-08-04 09:47:21 PDT
(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 4 Early Warning System Bot 2011-08-04 09:49:36 PDT
Comment on attachment 102933 [details]
proposed patch

Attachment 102933 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9303514
Comment 5 WebKit Review Bot 2011-08-04 10:17:11 PDT
Comment on attachment 102933 [details]
proposed patch

Attachment 102933 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9305486
Comment 6 Alexey Proskuryakov 2011-08-04 11:47:20 PDT
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.
Comment 7 Ian 'Hixie' Hickson 2011-08-04 13:14:04 PDT
In what sense is the request a new kind of request? Do you just mean that new HTTP headers?
Comment 8 Per-Erik Brodin 2011-08-04 13:15:52 PDT
Created attachment 102966 [details]
updated patch

Tweaked if-statement to avoid compiler warning.
Comment 9 Per-Erik Brodin 2011-08-04 13:21:08 PDT
(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 10 WebKit Review Bot 2011-08-04 13:49:30 PDT
Comment on attachment 102966 [details]
updated patch

Attachment 102966 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9304609
Comment 11 Alexey Proskuryakov 2011-08-04 14:19:10 PDT
(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.
Comment 12 Ian 'Hixie' Hickson 2011-08-04 14:34:30 PDT
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 13 Alexey Proskuryakov 2011-08-04 14:47:02 PDT
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"?
Comment 14 Per-Erik Brodin 2011-08-05 07:20:17 PDT
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.
Comment 15 Per-Erik Brodin 2011-08-05 07:25:44 PDT
(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.
Comment 16 Ian 'Hixie' Hickson 2011-08-05 07:33:57 PDT
Last-Event-ID could end up sent to a random host if there is a redirect after a reconnect.
Comment 17 Per-Erik Brodin 2011-08-05 07:39:16 PDT
(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 18 Alexey Proskuryakov 2011-08-05 09:23:11 PDT
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.
Comment 19 Per-Erik Brodin 2011-08-16 12:00:43 PDT
(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.
Comment 20 Alexey Proskuryakov 2011-08-16 13:35:30 PDT
As far as I'm concerned, yes, you can land this patch.
Comment 21 Per-Erik Brodin 2011-08-16 14:43:20 PDT
(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 22 WebKit Review Bot 2011-08-16 17:16:35 PDT
Comment on attachment 103070 [details]
patch 3

Clearing flags on attachment: 103070

Committed r93182: <http://trac.webkit.org/changeset/93182>
Comment 23 WebKit Review Bot 2011-08-16 17:16:41 PDT
All reviewed patches have been landed.  Closing bug.