Bug 50773

We don't usually send Origin with GET requests. We do send it for XMLHttpRequest if we're using CORS. Maybe we're not using CORS for some reason?

(In reply to comment #1) > We don't usually send Origin with GET requests. We do send it for XMLHttpRequest if we're using CORS. Maybe we're not using CORS for some reason? The request logs shown represent XHR requests that should indeed be CORS. The POST behaves as expected while the GET does not. Again, this appears to only happen when the source is http://localhost. This also introduces a catch 22 situation with a CORS scenario in which the source and target are both on localhost with different port numbers. WebKit XHR demands CORS response headers due to the different port but it does not send an Origin request header which is the trigger for a server to send the response headers.

Indeed. We're supposed to send Origin with CORS requests. If we're not doing that, its our bug.

UPDATE: it is not the source (localhost) that is causing the dilemma rather setting Content-Type on a CORS GET induces the failure.

OOPS - Sorry, it is when GET content-type is set to application/json that the CORS GET fails. Setting content-type to text/plain produces a successful CORS GET. The problem with this, aside from the fact that the spec allows for content-type on a GET, is that the request content type is relevant to many json service implementations and omitting it requires, in most cases, modification to javascript frameworks and server side code, both of which I have had to implement to work around this issue. please correct me if I have erred in my observations/conclusions.

Can you provide a test URL for reproducing the problem? Or at least a test HTML page with a script that's sending the request?

This demonstrates the bug. var xhr = new XMLHttpRequest(); xhr.open("GET", "http://manu.sporny.org/rdfa/cors", true); xhr.setRequestHeader("Content-Type", "application/json"); xhr.onreadystatechange = function () { if (xhr.readyState == 4) { alert("Expected status: 200 responseText: SUCCESS ... \n got status" + xhr.status + "\nresponseText : " + xhr.responseText); } } xhr.send();

I can confirm this is still an issue on WebKit (and Chrome). Let me add some additional comments. * The issue is not with a specific Content-Type (although it can be triggered by using a content-type not listed in CORS spec. [2.1]) * The problem is that in non-simple GET requests (those that require a preflight OPTIONS request), the Origin header is correctly sent with the OPTIONS but not the subsequent GET. Therefore, a server does not include the Access-Control-Allow-Origin in its response. This has been previously reported in Chromium, see http://code.google.com/p/chromium/issues/detail?id=57836 and includes a matrix of requests. What can we do to get this fixed?

> What can we do to get this fixed? The easiest way to get this bug fixes is to post a patch for review that fixes the issue and contains a test.

Created attachment 81216 [details] Proposed fix and regression test

Comment on attachment 81216 [details] Proposed fix and regression test I would suggest the problem is this: Interest starts when a request reaches DocumentThreadableLoader (see Source/WebCore/loaders/DocumentThreadableLoader.cpp). After creating a new instance of ResourceRequest (for CORS) [line 87], it's checked if the request is "simple" or not (e.g. requires preflight). If the request is simple (or preflight is forced), the following sequence takes place: * DocumentThreadableLoader::makeSimpleCrossOriginAccessRequest is called * The request origin is set [line 115] * Request is submitted via DocumentThreadableLoader::loadRequest [line 311] However, if the request is *not* simple, the following happens: * DocumentThreadableLoader::makeCrossOriginAccessRequestWithPreflight is called * A new OPTIONS request is created (and request origin set [line 125]) * Request is submitted via DocumentThreadableLoader::loadRequest [line 149] * If the preflight succeeds, DocumentThreadableLoader::preflightSuccess() is called and the original request submitted [line 302] ... (see Source/WebCore/loaders/FrameLoader.cpp) * FrameLoader::addHTTPOriginIfNeeded [line 2680] is called for the CORS request and returns without setting the origin because it is not included by default on GET requests (for privacy concerns, see line 2690) * Request later fails WebCore::passesAccessControlCheck at line 111 (see Source/WebCore/loaders/CrossOriginAccessControl.cpp) So the problem, therefore, is that whilst the origin header is explictly set for a simple cross-origin GET request in (makeSimpleCrossOriginAccessRequest), it is not explicitly after a preflight (and fails the implicit check in FrameLoader). I attach a simple patch that fixes this by explictly setting the origin when the cross-origin preflight is completed successfully. Also included is a regression test for the issue.

Nicely done. Now that I look at DocumentThreadableLoader with this header in mind, it looks like every time that loadRequest is called the origin should be set "if (!m_sameOriginRequest)", but that introduces other wierdness. At the very least, it should assert that the header is set "if (!m_sameOriginRequest)" However that requires a fix at line 76: 75 if (m_sameOriginRequest || m_options.crossOriginRequestPolicy == AllowCrossOriginRequests) { 76 loadRequest(request, DoSecurityCheck); And a test which would involve worker import script which is a bit much to ask. I filed bug 53790 about this.

Comment on attachment 81216 [details] Proposed fix and regression test View in context: https://bugs.webkit.org/attachment.cgi?id=81216&action=review This looks good to me. I just have a few minor suggestions to clean up and then we can get this committed. > Source/WebCore/ChangeLog:5 > + Bug 50773: CORS origin header not set on GET when a preflight request Ideally this is formatted more like other ChangeLog entries: Bug title Bug link (on separate lines). > Source/WebCore/loader/DocumentThreadableLoader.cpp:301 > + // Explicitly set the origin of this request Add "." to end of sentence. > LayoutTests/http/tests/xmlhttprequest/cross-origin-preflight-get.html:3 > +<p>Test case for issue #50773 - the "Origin" header should be properly sent with a non-simple cross-origin resource sharing request that uses the GET method.</p> No need to mention the bug, so you can omit "Test case for issue #50773 -" > LayoutTests/http/tests/xmlhttprequest/cross-origin-preflight-get.html:20 > + xhr.setRequestHeader("X-Proprietary-Header", "foo"); // make this a non-simple CORS request Please make the comment a proper sentence. "Make this a non-simple CORS request." > LayoutTests/http/tests/xmlhttprequest/resources/cross-origin-preflight-get.php:2 > +// Test case for the preflight cross-origin request using GET (issue #50773) You don't need to reference the bug number. If someone needs to find it, they can look at when the test was added and see the commit message, ChangeLog, etc. which will all reference the bug.

> Add "." to end of sentence. Actually, I don't think that this comment adds any value as is. It should be removed, or it should explain what's so special about this code path that we need to set Origin manually. What is the normal way to set Origin? Why don't we get it automatically added by FrameLoader::addHTTPOriginIfNeeded(), for example?

> You don't need to reference the bug number. Personally, I like adding a link to the bug (not just mentioning the number). Not sure if that's useful in practice - I don't think I ever clicked on these links myself.

> Why don't we get it automatically added by FrameLoader::addHTTPOriginIfNeeded(), for example? That method does not understand CORS.

OK. We call setHTTPOrigin() without comments elsewhere in DocumentThreadableLoader, so this instance probably also doesn't need a comment.

Thanks for the comments. An amended patch will follow shortly. > Now that I look at DocumentThreadableLoader with this header in mind, it looks like every time that loadRequest is called the origin should be set "if (!m_sameOriginRequest)", but that introduces other wierdness. Yes, I found the same "weirdness". This patch seems to be the least disruptive option in the short term. > Why don't we get it automatically added by FrameLoader::addHTTPOriginIfNeeded(), for example? > That method does not understand CORS. Correct - FrameLoader::addHTTPOriginIfNeeded does not set "Origin" for a GET request (irrespective of whether it is CORS). Therefore we need to bring a preflighted request inline with a one which is simple [line 115].

Created attachment 81272 [details] Proposed fix after feedback This should [hopefully] address the previous comments.

Comment on attachment 81272 [details] Proposed fix after feedback Clearing flags on attachment: 81272 Committed r77680: <http://trac.webkit.org/changeset/77680>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 81272 [details]: http/tests/websocket/tests/handshake-fail-by-maxlength.html bug 53816 The commit-queue is continuing to process your patch.