Closed Bug 634906 Opened 11 years ago Closed 11 years ago
Allow CORS from ns
Xml Http Request when script is loaded from file
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...
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?
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.
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...
Assignee: nobody → bjarne
Status: NEW → ASSIGNED
Attachment #513250 - Flags: review?(jonas)
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).
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?
(In reply to comment #5) > Patch pushed to tryserver to be sure I don't miss > anything else... Tryserver is ok.
Attachment #513291 - Flags: review?(jonas) → review+
Attachment #513291 - Flags: approval2.0? → approval2.0+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 513291 [details] [diff] [review] Updated w/modified test This may also be something for 3.6. Requesting approval...
Attachment #513291 - Flags: approval22.214.171.124?
Comment on attachment 513291 [details] [diff] [review] Updated w/modified test Approved for 126.96.36.199, a=dveditz for release-drivers
Attachment #513291 - Flags: approval188.8.131.52? → approval184.108.40.206+
The test which was modified as part of this patch does not exist in 1.9.2 - is it ok to land without it?
I'm pretty sure it exists, just with a different name. Check test_CrossSiteXHR.html
Jonas is right - test had a different name in 1.9.2. Updated patch. Passes local testing.
Seems like this missed 220.127.116.11 - any chance to get it into 18.104.22.168?
> any chance to get it into 22.214.171.124? 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 126.96.36.199 relbranch at this point; I just pushed it to 1.9.2 for 188.8.131.52. Please let me know if it does need to go on the relbranch.
Verified for 184.108.40.206 based on passing test_CrossSiteXHR.html
You need to log in before you can comment on or make changes to this bug.