Cross-site XMLHttpRequest reports security errors to the console even when successful

VERIFIED FIXED in mozilla1.9beta1

Status

()

Core
DOM
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: ted, Assigned: Surya Ismail)

Tracking

Trunk
mozilla1.9beta1
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
Created attachment 279033 [details]
attempt cross-site-XHR

Performing a cross-site-XMLHttpRequest reports multiple errors to the error console, even when the request is successful.  The attached testcase tries to load http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&json=1 (which sends Content-Access-Control: allow <*>) via XHR.

I see the following error three times:
Security Error: Content at ... may not load data from http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&json=1.

Updated

11 years ago
Summary: cross-site-XMLHttpRequest reports errors to the console even when successful → Cross-site XMLHttpRequest reports security errors to the console even when successful
This is due to the security manager being too happy about reporting errors just for a simple same-origin check. We could add a flag indicating weather an error should be reported or not.

Comment 2

11 years ago
please check "Issue I" of Bug 395625 is same as this
> We could add a flag indicating weather an error should be reported or not.

Or we could use the API that doesn't report errors: SecurityCompareURIs.  Or equivalent.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Syryai, this should be an easy bug to fix. Currently the CheckSameOrigin API i'm calling here: http://lxr.mozilla.org/mozilla/source/content/base/src/nsCrossSiteListenerProxy.cpp#136

warns in the error console that a security check failed. Comment 1 and comment 3 suggests alternative solutions that avoids that problem.

Let me know if you have any questions.
Oops, forgot to assign the bug as well. See comment above.
Assignee: jonas → suryaismail
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

11 years ago
Created attachment 283523 [details] [diff] [review]
Proposed patch

Followed comment #1 and added a "boolean reportError" flag to CheckSameOriginURI.
Security errors are gone. But there's still a JS error "Javascript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x8004005 (NS_ERROR_FAILURE) [nsIXMLHttpRequest.send]"  ....."
Firefox 2 seems to do the same thing. Is this correct behaviour?
(Reporter)

Comment 7

11 years ago
Are you seeing that error loading the testcase here?  I don't see that error (but I do see the security errors) using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092605 Minefield/3.0a9pre.
I'm still not sure why we're adding API when we already have an API that does this... especially since as I recall the plan is to change all this api stuff anyway to have a "principal same origin as uri" function, so using SecurityCompareURIs temporarily is ok...

If we da change the API, we need to change the iid.
SecurityCompareURIs isn't exposed in any public API. We could do that of course, but that's as much adding API as the flag, no?

I don't care much either way though, so if bz prefer that we expose on the idl instead I'm fine with that.
> SecurityCompareURIs isn't exposed in any public API.

Uh... right.  Bug 327243.  How could I forget?  ;)

In that case, adding the flag is the way to go, yeah.  Though I still think we want that "compare principal to URI" API here.
In fact, I said pretty much all that in bug 327243.  If we really don't have time to do the right thing here, I guess adding the boolean to the existing API is fine.  But if we do, I'd rather we got our API right.
(Assignee)

Comment 12

11 years ago
CheckSameOrigin only adds 4-5 lines of error reporting code to SecurityCompareURIs. Since they're so similar, I think it'd be easier to document/maintain just one API call.
I'd vote to just remove the error reporting code from CheckSameOrigin. That avoids the idl change. I think that's what bz said for bug 327243.
Am I missing any side-effects of removing the error reporting code?
Hmm.. I do in fact have a patch sitting around that adds a compare-principal-to-uri API. Not sure what state it's in though.

Bz, in what cases do you think we do want to have this reported to the console? I suspect the answer will affect my patch too.
I think we want to have it reported to the console any time the check actually blocks something from happening altogether (as opposed to just changing the codepath followed, as here).
Comment on attachment 283523 [details] [diff] [review]
Proposed patch

This would actually be awesome to get in for beta since this is something a lot of people have been complaining about when first starting to play with cross site xhr.

The patch is very safe so i'm not at all worried about regressions.

The only argument for not taking this patch is that we have a better one, but that one isn't safe enough for beta so I'd say lets take this for beta to enable better cross-site xhr testing.
Attachment #283523 - Flags: superreview+
Attachment #283523 - Flags: review+
Attachment #283523 - Flags: approvalM9?

Comment 16

11 years ago
Comment on attachment 283523 [details] [diff] [review]
Proposed patch

a+ schrep since this makes a web content feature a little more useful for b1
Attachment #283523 - Flags: approvalM9? → approvalM9+
jonas checked-in this patch at 2007-10-26 18:46. Should this bug be resolved as FIXED and a new bug opened for the past-beta fix?
Target Milestone: --- → mozilla1.9 M9
Marking FIXED.  Please open a new bug if there is to be a different fix.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
I do not see the error message that luser mentions in the initial report, but I still see the error that was noted in Comment 6. Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIXMLHttpRequest.send]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: https://bugzilla.mozilla.org/attachment.cgi?id=279033 :: tryXHR :: line 7"  data: no] Would like to get clarification if that is expected before verifying. Thanks.
(Reporter)

Comment 20

11 years ago
I think the testcase got broken because the spec changed.  :-/
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007103004 Minefield/3.0a9pre. No security errors in the console when loading the attachment. Comment 20 addresses the issue with the error that is coming up in the console.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.