634906 - Allow CORS from nsXmlHttpRequest when script is loaded from file

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Bjarne (:bjarne)]

Current code prevents access to results from cross-origin calls if script is loaded from file, even if the server explicitly allows it.

To be more precise: If a script is loaded from file our nsXmlHttpRequest sends the CORS request-header "Origin: null". This is according to the W3C spec section 4.7 and the underlying IETF standard. However, even if the server respond with "Access-Control-Allow-Origin: null", this code-line

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCrossSiteListenerProxy.cpp#255

blocks access to the results from the call. It seems like the referred code implements step 3 of the algorithm in section 6.2 of the W3C CORS spec, however last part (the note) of that section indicates IMO that responding with "null" to a request for "null" is ok.

Furthermore, observe that the call to a (potentially bad) server has already happened, hence credentials are exposed. Blocking access to the results from the server is IMO unnecessary in this case; if we really wanted to enforce this we should IMO rather prevent the call to happen entirely.

Finally, MSIE8 and Safari allows this...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Why would a server ever respond with "Access-Control-Allow-Origin: null" as opposed to '*'?  "null" means "I have no idea who's getting the data", so allowing it is the same as allowing anyone at all to get the data....

That said, the spec does say this should work.  Jonas, why did we make it not do so?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, you can't use the "*" syntax for requests with cookies.

So yeah, I think we should remove our restriction. I think it's a left-over from an older revision of the spec.

Comment 3

[Bjarne (:bjarne)]

Attachment: Proposed solution

Ok - here we go...  pretty simple fix. I have not sent it to tryserver because I don't think that would be much point. Let me know if I'm wrong...

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 513250 [details] [diff] [review]
Proposed solution

Please keep the curlies. Makes future changes easier to read and follows the style of the file.

Also, please add tests (I'm surprised this passes existing tests).

Comment 5

[Bjarne (:bjarne)]

Attachment: Updated w/modified test

Uhh..  I was apparently not running the relevant tests. :}

There was a failing case in content/base/test/test_CrossSiteXHR_origin.html which has been updated. Patch pushed to tryserver to be sure I don't miss anything else...

A note about that test though: It doesn't seem to run anything with "withCredentials" set... is that on purpose?

Comment 6

[Bjarne (:bjarne)]

(In reply to comment #5)
> Patch pushed to tryserver to be sure I don't miss
> anything else...

Tryserver is ok.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in
http://hg.mozilla.org/mozilla-central/rev/7bc88a8f9095

Comment 8

[Bjarne (:bjarne)]

Comment on attachment 513291 [details] [diff] [review]
Updated w/modified test

This may also be something for 3.6. Requesting approval...

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 513291 [details] [diff] [review]
Updated w/modified test

Approved for 1.9.2.16, a=dveditz for release-drivers

Comment 10

[Bjarne (:bjarne)]

Attachment: Patch for 1.9.2 branch wo/ modified test

The test which was modified as part of this patch does not exist in 1.9.2 - is it ok to land without it?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm pretty sure it exists, just with a different name. Check test_CrossSiteXHR.html

Comment 12

[Bjarne (:bjarne)]

Attachment: Patch for 1.9.2 branch w/ modified test

Jonas is right - test had a different name in 1.9.2. Updated patch. Passes local testing.

Comment 13

[Bjarne (:bjarne)]

Requesting check-in based on approval in comment #9

Comment 14

[Bjarne (:bjarne)]

Seems like this missed 1.9.2.16 - any chance to get it into 1.9.2.17?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> any chance to get it into 1.9.2.17?

I pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e7ebe8a15894

Would have happened earlier if the patch had had the metadata I had to recreate.

I'm not sure whether this should be going in on the 1.9.2.17 relbranch at this point; I just pushed it to 1.9.2 for 1.9.2.18.  Please let me know if it does need to go on the relbranch.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.2.18 based on passing test_CrossSiteXHR.html