Bug 50773 – CORS origin header not set on GET when content type request header is set

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

[Sky Sanders]

This seems to only be an issue when source is localhost
reproduced on latest Chrome and Safari


Here is a POST (origin is set)

POST /RESTWebServices/session HTTP/1.1
Host: ec2-174-129-8-69.compute-1.amazonaws.com
Referer: http://localhost:10042/documentation/samples/ciapijs-newsdetail-steps.sample.aspx
Accept: */*
Accept-Language: en-US
Origin: http://localhost:10042
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/533.18.1 (KHTML, like Gecko) Version/5.0.2 Safari/533.18.5
X-Requested-With: XMLHttpRequest
Content-Type: application/json; charset=UTF-8
Accept-Encoding: gzip, deflate
Content-Length: 45
Connection: keep-alive
Connection: keep-alive

{"UserName":"CC735158","Password":"password"}
HTTP/1.1 200 OK
Cache-Control: private
Content-Length: 50
Content-Type: application/json; charset=utf-8
Server: Microsoft-IIS/7.0
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: POST, GET, OPTIONS
Access-Control-Allow-Headers: X-Requested-With, Content-Type
Access-Control-Max-Age: 1728000
X-AspNet-Version: 4.0.30319
X-Powered-By: ASP.NET
Date: Sun, 05 Dec 2010 12:48:47 GMT

{"Session":"D2FF3E4D-01EA-4741-86F0-437C919B5559"}


Here is a GET (Origin is missing)

GET /a valid url
Host: ec2-174-129-8-69.compute-1.amazonaws.com
Connection: keep-alive
Referer: http://localhost:10042/documentation/samples/ciapijs-newsdetail-steps.sample.aspx
X-Requested-With: XMLHttpRequest
Content-Type: application/json
Accept: */*
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.215 Safari/534.10
Accept-Encoding: gzip,deflate,sdch
Accept-Language: en-US,en;q=0.8
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.3


HTTP/1.1 200 OK
Cache-Control: private
Content-Length: 1152
Content-Type: application/json; charset=utf-8
Server: Microsoft-IIS/7.0
X-AspNet-Version: 4.0.30319
X-Powered-By: ASP.NET
Date: Sun, 05 Dec 2010 12:44:25 GMT

{ valid json respose }

[Adam Barth]

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?

[Sky Sanders]

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

[Adam Barth]

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

[Sky Sanders]

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

[Sky Sanders]

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.

[Alexey Proskuryakov]

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?

[Sky Sanders]

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();

[Martin Galpin]

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?

[Adam Barth]

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

[Martin Galpin]

Created attachment 81216 [details]
Proposed fix and regression test

[Martin Galpin]

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.

[David Levin]

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.

[David Levin]

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.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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

[Adam Barth]

> Why don't we get it automatically added by FrameLoader::addHTTPOriginIfNeeded(), for example?

That method does not understand CORS.

[Alexey Proskuryakov]

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

[Martin Galpin]

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

[Martin Galpin]

Created attachment 81272 [details]
Proposed fix after feedback

This should [hopefully] address the previous comments.

[WebKit Commit Bot]

Comment on attachment 81272 [details]
Proposed fix after feedback

Clearing flags on attachment: 81272

Committed r77680: <http://trac.webkit.org/changeset/77680>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

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.