Closed Bug 634906 Opened 9 years ago Closed 9 years ago

Allow CORS from nsXmlHttpRequest when script is loaded from file

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .18-fixed

People

(Reporter: bjarne, Assigned: bjarne)

Details

(Keywords: verified1.9.2)

Attachments

(2 files, 2 obsolete files)

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.
Attached patch Proposed solution (obsolete) — Splinter Review
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?
Attachment #513250 - Attachment is obsolete: true
Attachment #513291 - Flags: review?(jonas)
Attachment #513250 - Flags: review?(jonas)
(In reply to comment #5)
> Patch pushed to tryserver to be sure I don't miss
> anything else...

Tryserver is ok.
Attachment #513291 - Flags: approval2.0?
Checked in
http://hg.mozilla.org/mozilla-central/rev/7bc88a8f9095
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 513291 [details] [diff] [review]
Updated w/modified test

This may also be something for 3.6. Requesting approval...
Attachment #513291 - Flags: approval1.9.2.16?
Comment on attachment 513291 [details] [diff] [review]
Updated w/modified test

Approved for 1.9.2.16, a=dveditz for release-drivers
Attachment #513291 - Flags: approval1.9.2.16? → approval1.9.2.16+
The test which was modified as part of this patch does not exist in 1.9.2 - is it ok to land without it?
Attachment #519259 - Flags: approval1.9.2.16?
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.
Attachment #519259 - Attachment is obsolete: true
Attachment #519259 - Flags: approval1.9.2.16?
Requesting check-in based on approval in comment #9
Keywords: checkin-needed
Seems like this missed 1.9.2.16 - any chance to get it into 1.9.2.17?
> 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.
Attachment #513291 - Flags: approval1.9.2.17+ → approval1.9.2.18+
Verified for 1.9.2.18 based on passing test_CrossSiteXHR.html
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.