Bug 66340 – REGRESSION (r89086): All worker xhr requests trigger preflight requests.

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[David Levin]

This happens in WebKit nightlies and chromium for both shared and dedicated workers. After http://trac.webkit.org/changeset/89086 (which I helped review), the referer header is set before DocumentThreadableLoader::create is called.  DocumentThreadableLoader::create  checks to see if a preflight request is needed but the referer header isn't listed as a simple header (because it can't be set by the xhr request at all), so this header being present triggers the preflight request. 

Just adding Nate as an fyi.

In progress fix: 
* I'll add "referer" to the whitelist in isOnAccessControlSimpleRequestHeaderWhitelist in Source/WebCore/loader/CrossOriginAccessControl.cpp and all will be happiness.
* Add a simple test for cross origin stuff for workers/shared workers.

[Alexey Proskuryakov]

I guess we can add all headers in XMLHttpRequest::m_forbiddenRequestHeaders to CORS simple request whitelist now, since they can only be set by the engine, not by JS code.

[David Levin]

Created attachment 104136 [details]
Patch

[Adam Barth]

Comment on attachment 104136 [details]
Patch

Yeah, I agree with Alexey.

[David Levin]

I'll add another patch with tests and add the other headers after I land this one.

[David Levin]

Comment on attachment 104136 [details]
Patch

Committed as http://trac.webkit.org/changeset/93188

Keeping bug open to work on the bigger fix.

[Per-Erik Brodin]

(In reply to comment #1)
> I guess we can add all headers in XMLHttpRequest::m_forbiddenRequestHeaders to CORS simple request whitelist now, since they can only be set by the engine, not by JS code.

Another option would be to fix https://bugs.webkit.org/show_bug.cgi?id=63460
Now Referer and Origin will be incorrectly included in Access-Control-Request-Headers whenever there is a preflight.

[David Levin]

Created attachment 104552 [details]
Patch

[David Levin]

Comment on attachment 104552 [details]
Patch

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

It seemed reasonable to move these checks over here as you suggested previously. I suspect the added cost is negligible and this doesn't seem like a performance sensitive path.

However, I don't really care to have all of this moved over and mostly did it due to the suggestion :).

So I'll withdraw this patch and do one with only the test code which should help us catch any future regressions.

[David Levin]

Created attachment 104709 [details]
Test for preflight for simple request from web workers.

[Darin Adler]

Comment on attachment 104709 [details]
Test for preflight for simple request from web workers.

View in context: https://bugs.webkit.org/attachment.cgi?id=104709&action=review

> LayoutTests/http/tests/xmlhttprequest/workers/resources/access-control-basic-get-fail-non-simple-test.js:19
> +worker.onmessage = function(evt)

Why eve instead of event?

[David Levin]

Comment on attachment 104709 [details]
Test for preflight for simple request from web workers.

I'll change the test to event and submit it.

[David Levin]

Committed (tests) as http://trac.webkit.org/changeset/93536