Bug 4872 – XMLHttpRequest fails to throw an exception when there is a security violation (mismatching domains)

• Component: XML
• Status: RESOLVED FIXED

[Damian Steer]

XMLHttpRequests are tied to the originating domain (host?) for security reasons. Safari does not appear 
to violate this, however it seems to be impossible to detect this reliably in javascript.

Mozilla (et al), Opera and IE (*) all raise exceptions in the following (although at slightly different 
points):

(taken from above url)

try
        {
          xmlhttp = new XMLHttpRequest();
          xmlhttp.open("GET", "http://example.com/", true);
          xmlhttp.send(null);
        }
catch(e)
        {
          alert(e);
        }

Safari remains on readyState == 0, which suggests something is wrong, but isn't definitive.

(*) http://rdfweb.org/people/damian/2005/09/test_xmlhttprequest_ie.html will check all platforms.

[Mark Rowe (bdash)]

Confirmed with WebKit 412.7 and ToT WebKit.

[Alexey Proskuryakov]

Created attachment 10476 [details]
proposed patch

[Darin Adler]

Comment on attachment 10476 [details]
proposed patch

-      if (args.size() < 2 || args.size() > 5)
-        return jsUndefined();
+      if (args.size() < 2)

-      if (args.size() >= 1) {
+      if (args.size() == 1) {

It looks like this change now tolerates extra arguments for open, but no longer tolerates extra parameters for send. Is that intentional? Do the tests cover this?

In general, I'd like to see tests that cover the "extra arguments" behavior since we keep tweaking it back and forth.

Does the XMLHttpRequest exception in setDOMException need to have the strange pattern where it thows the error without putting the exception code into the object? If so, we need a comment at least to explain why.

How can those checks of the response headers be turned into asserts? Where is the code that guarantees the headers will have that form?

[Darin Adler]

Comment on attachment 10476 [details]
proposed patch

I'm going to mark this review- because of the issue in setDOMException.

[Alexey Proskuryakov]

Created attachment 10501 [details]
proposed patch

(In reply to comment #3)
> It looks like this change now tolerates extra arguments for open, but no longer
> tolerates extra parameters for send. Is that intentional?

  This was a mistake, fixed!

> In general, I'd like to see tests that cover the "extra arguments" behavior
> since we keep tweaking it back and forth.

  Added http/tests/extra-parameters.html

> Does the XMLHttpRequest exception in setDOMException need to have the strange
> pattern where it thows the error without putting the exception code into the
> object? If so, we need a comment at least to explain why.

  Done. The current XMLHttpRequest spec draft does not specify any code for the exception (actually, it does not even specify that an exception should be thrown). And Firefox doesn't put any exception code either.

> How can those checks of the response headers be turned into asserts? Where is
> the code that guarantees the headers will have that form?

  The string is created in HeaderStringFromDictionary(), LoaderFunctionsMac.mm in the expected form. Actually, I think that the ResourceLoader interface should be changed to pass this information without serialization.

[Eric Seidel (no email)]

ap: I keep meaning to talk with you about this over irc... ping me next time you're around.

[Eric Seidel (no email)]

Comment on attachment 10501 [details]
proposed patch

I've read through this a couple times over the last week or so.  I also just talked with ap about it again over irc.

The code looks sane.  Most important here are the tests.  So long as we have excellent XHR testing coverage, I'm happy.

It's sad that this string has to be repeated so many times:
return throwError(exec, SyntaxError, "Not enough arguments");

You've noted these try/catches are for testing in IE:
+        try {
+            req.overrideMimeType('text/xml');
+        } catch (ex) {
+        }
IMO it's best to log any exceptions, even if they're specific to a single browser.

This should have a bug about it:
-    // FIXME: Should this abort instead if we already have a m_job going?
+    // FIXME: Should this abort or raise an exception instead if we already have a m_job going?


looks good.  r=me.

[Alexey Proskuryakov]

Committed revision 16543.

> IMO it's best to log any exceptions, even if they're specific to a single
> browser.

Done; also added empty-response.xml to make the tests work in WinIE without tweaking .htaccess.

> This should have a bug about it:
> -    // FIXME: Should this abort instead if we already have a m_job going?
> +    // FIXME: Should this abort or raise an exception instead if we already
> have a m_job going?

This is not something we can really resolve without help from the web-api WG; I have asked Anne van Kesteren for comments.

[Alexey Proskuryakov]

(In reply to comment #8)
> This is not something we can really resolve without help from the web-api WG

http://www.w3.org/2006/webapi/track/issues/91