713980 - Warning for cross-site XMLHttpRequest (CORS error logging)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marco Castelluccio [:marco]]

We should print a warning message in the console when a cross-site XMLHttpRequest gets blocked. This could save a lot of headhaches to developers.

Comment 1

[Josh Matthews [:jdm]]

Yes please. This has been killing me frequently in the recent past, and I end up having to launch Chrome just to figure out what's going on. Maybe we can piggyback on whatever bug 712859 ends up doing.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Sicking?  Do we want to do this for all CORS stuff?  Seems like we do....

Comment 3

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

Yeah, I think we should.

Comment 4

[worker.thread]

Attachment: Fix for bug 713980, with test

Comment 5

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

Comment on attachment 641046 [details] [diff] [review]
Fix for bug 713980, with test

Review of attachment 641046 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with those things fixed.

::: content/base/src/nsCrossSiteListenerProxy.cpp
@@ +44,5 @@
> +	nsCOMPtr<nsIConsoleService> console(do_GetService(NS_CONSOLESERVICE_CONTRACTID));
> +	if (!console) return NS_ERROR_NOT_AVAILABLE;
> +
> +	//Generate the error message
> +	nsCString msg("XMLHttpRequest blocked");

Would be nice to include the URI that they tried to load too.

@@ +53,5 @@
> +			msg.Append("bad URI or cross-site access not allowed");
> +			break;
> +		default:
> +			msg.Append("status=");
> +			msg.AppendInt(aStatus);

Would be great if you could write this in hex format.

::: content/base/test/Makefile.in
@@ +1,2 @@
> +#
> +# This Source Code Form is subject to the terms of the Mozilla Public

Seems like something is wrong with this file. Did you by any chance change all line-endings?

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Do we not want to localize this?

Comment 7

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

I believe we've always said that it's ok that developer error messages can't be localized.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Really? Since when?

Comment 9

[Axel Hecht [:Pike]]

I would think that all our error messages are localized, most of them somewhere in http://mxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/.

Comment 10

[worker.thread]

(In reply to Jonas Sicking (:sicking) from comment #5)
> Comment on attachment 641046 [details] [diff] [review]
> Fix for bug 713980, with test
> 
> Review of attachment 641046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with those things fixed.
> 
> ::: content/base/src/nsCrossSiteListenerProxy.cpp
> @@ +44,5 @@
> > +	nsCOMPtr<nsIConsoleService> console(do_GetService(NS_CONSOLESERVICE_CONTRACTID));
> > +	if (!console) return NS_ERROR_NOT_AVAILABLE;
> > +
> > +	//Generate the error message
> > +	nsCString msg("XMLHttpRequest blocked");
> 
> Would be nice to include the URI that they tried to load too.
> 
> @@ +53,5 @@
> > +			msg.Append("bad URI or cross-site access not allowed");
> > +			break;
> > +		default:
> > +			msg.Append("status=");
> > +			msg.AppendInt(aStatus);
> 
> Would be great if you could write this in hex format.


Fixed those changes.


> ::: content/base/test/Makefile.in
> @@ +1,2 @@
> > +#
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> 
> Seems like something is wrong with this file. Did you by any chance change
> all line-endings?

My editor might have done that, I'm not sure. I did add the line to include the test case.

As for the locale issue, I'm clueless on how that works, and if it's even necessary. Can you guys point me in the right direction? I can attach the patch if the locale stuff is not needed.

Comment 11

[worker.thread]

Attachment: Fix for bug 713980, with test. Added blocked source URI to the warning message, and error code in hex.

Comment 12

[worker.thread]

Comment on attachment 644351 [details] [diff] [review]
Fix for bug 713980, with test. Added blocked source URI to the warning message, and error code in hex.

# HG changeset patch
# Parent eea94a9b40a1c5d88913adf597f5b84dd89fd4fc
# User WorkerThread <[e-mail]>
Bug 713980 - Added a console logging function which print a warning message in the console when a cross-site XMLHttpRequest gets blocked.
* * *
(Original) Bug 713980 - Added a console logging function which print a warning message in the console when a cross-site XMLHttpRequest gets blocked.
(Revised) Prints blocked source URI with the warning message, displays the error code in hex.

diff --git a/content/base/src/nsCrossSiteListenerProxy.cpp b/content/base/src/nsCrossSiteListenerProxy.cpp
--- a/content/base/src/nsCrossSiteListenerProxy.cpp
+++ b/content/base/src/nsCrossSiteListenerProxy.cpp
@@ -20,24 +20,99 @@
 #include "nsCharSeparatedTokenizer.h"
 #include "nsAsyncRedirectVerifyHelper.h"
 #include "prclist.h"
 #include "prtime.h"
 #include "nsClassHashtable.h"
 #include "nsHashKeys.h"
 #include "nsStreamUtils.h"
 #include "mozilla/Preferences.h"
+#include "nsIScriptError.h"
+#include "nsILoadGroup.h"
+#include "nsILoadContext.h"
+#include "nsIConsoleService.h"
+#include "nsIDOMWindowUtils.h"
 
 using namespace mozilla;
 
 #define PREFLIGHT_CACHE_SIZE 100
 
 static bool gDisableCORS = false;
 static bool gDisableCORSPrivateData = false;
 
+static nsresult
+LogBlockedMessage(nsIChannel *aChannel, nsresult aStatus)
+{
+	nsCOMPtr<nsIConsoleService> console(do_GetService(NS_CONSOLESERVICE_CONTRACTID));
+	if (!console) return NS_ERROR_NOT_AVAILABLE;
+
+	//Generate the error message
+	nsCString msg("XMLHttpRequest blocked");
+	if (aStatus != 0) {
+		msg.Append(": ");
+		switch (aStatus) {
+		case NS_ERROR_DOM_BAD_URI:
+			msg.Append("bad URI or cross-site access not allowed");
+			break;
+		default:
+			msg.Append("status=0x");
+			msg.AppendInt(aStatus,16);
+		}
+	}
+
+	nsresult rv;
+	nsCOMPtr<nsIScriptError> scriptError = do_CreateInstance(NS_SCRIPTERROR_CONTRACTID, &rv);
+	NS_ENSURE_SUCCESS(rv, rv);
+
+	//Get the innerWindowID associated with the XMLHTTPRequest
+
+	PRUint64 innerWindowID = 0;
+	nsCOMPtr<nsIInterfaceRequestor> callbacks;
+	aChannel->GetNotificationCallbacks(getter_AddRefs(callbacks));
+	if (!callbacks) {
+		nsCOMPtr<nsILoadGroup> aLG;
+		aChannel->GetLoadGroup(getter_AddRefs(aLG));
+		aLG->GetNotificationCallbacks(getter_AddRefs(callbacks));
+	}
+
+	if (callbacks) {
+		nsCOMPtr<nsILoadContext> nsILC = do_GetInterface(callbacks);
+		if (nsILC) {
+				nsCOMPtr<nsIDOMWindow> win;
+				nsILC->GetAssociatedWindow(getter_AddRefs(win));
+				if (win) {
+					nsCOMPtr<nsIDOMWindowUtils> du = do_GetInterface(win);
+					du->GetCurrentInnerWindowID(&innerWindowID);
+				}
+		}
+	}
+	
+	nsCAutoString src;
+	nsCOMPtr<nsIURI> aUri;
+	aChannel->GetURI(getter_AddRefs(aUri));
+	if (aUri) aUri->GetSpec(src);
+
+	//Generate a scriptError and log it to the console
+	rv = scriptError->InitWithWindowID(NS_ConvertUTF8toUTF16(msg).get(),
+												NS_ConvertUTF8toUTF16(src).get(),
+												NULL,
+												0,
+												0,
+												nsIScriptError::warningFlag,
+												"Content Security Policy",
+												innerWindowID);
+
+	if (NS_SUCCEEDED(rv)) {
+		console->LogMessage(scriptError);
+		return NS_OK;
+	}
+
+	return rv;
+}
+
 //////////////////////////////////////////////////////////////////////////
 // Preflight cache
 
 class nsPreflightCache
 {
 public:
   struct TokenTime
   {
@@ -422,18 +497,21 @@ nsCORSListenerProxy::nsCORSListenerProxy
     mOuterNotificationCallbacks = nsnull;
   }
 }
 
 NS_IMETHODIMP
 nsCORSListenerProxy::OnStartRequest(nsIRequest* aRequest,
                                     nsISupports* aContext)
 {
-  mRequestApproved = NS_SUCCEEDED(CheckRequestApproved(aRequest));
+  nsresult rv = CheckRequestApproved(aRequest);
+  mRequestApproved = NS_SUCCEEDED(rv);
   if (!mRequestApproved) {
+	nsCOMPtr<nsIChannel> nsChan = do_QueryInterface(aRequest);
+	LogBlockedMessage(nsChan, rv);
     if (sPreflightCache) {
       nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
       if (channel) {
         nsCOMPtr<nsIURI> uri;
         NS_GetFinalChannelURI(channel, getter_AddRefs(uri));
         if (uri) {
           sPreflightCache->RemoveEntries(uri, mRequestingPrincipal);
         }
@@ -644,16 +722,17 @@ nsCORSListenerProxy::AsyncOnChannelRedir
                                             nsIChannel *aNewChannel,
                                             PRUint32 aFlags,
                                             nsIAsyncVerifyRedirectCallback *cb)
 {
   nsresult rv;
   if (!NS_IsInternalSameURIRedirect(aOldChannel, aNewChannel, aFlags)) {
     rv = CheckRequestApproved(aOldChannel);
     if (NS_FAILED(rv)) {
+	  LogBlockedMessage(aOldChannel, rv);
       if (sPreflightCache) {
         nsCOMPtr<nsIURI> oldURI;
         NS_GetFinalChannelURI(aOldChannel, getter_AddRefs(oldURI));
         if (oldURI) {
           sPreflightCache->RemoveEntries(oldURI, mRequestingPrincipal);
         }
       }
       aOldChannel->Cancel(NS_ERROR_DOM_BAD_URI);
diff --git a/content/base/test/Makefile.in b/content/base/test/Makefile.in
--- a/content/base/test/Makefile.in
+++ b/content/base/test/Makefile.in
@@ -549,16 +549,17 @@ MOCHITEST_FILES_B = \
 		test_bug749367.html \
 		test_bug753278.html \
 		test_bug761120.html \
 		test_XHR_onuploadprogress.html \
 		test_XHR_anon.html \
 		file_XHR_anon.sjs \
 		test_XHR_system.html \
 		test_XHR_parameters.html \
+		test_bug713980.html \
 		$(NULL)
 
 MOCHITEST_CHROME_FILES =	\
 		test_bug357450.js \
 		$(NULL)
 
 MOCHITEST_FILES_PARTS = $(foreach s,A B,MOCHITEST_FILES_$(s))
 
diff --git a/content/base/test/test_bug713980.html b/content/base/test/test_bug713980.html
new file mode 100644
--- /dev/null
+++ b/content/base/test/test_bug713980.html
@@ -0,0 +1,67 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=713980
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 713980</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+  const Cc = SpecialPowers.wrap(Components).classes;
+  const Ci = SpecialPowers.wrap(Components).interfaces;
+  const Cr = SpecialPowers.wrap(Components).results;
+
+var consoleService =
+    Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService);
+
+  var consoleListener = {
+    windowID: 0,
+    seenMsg: false,
+
+    observe: function(message) {
+	try {
+          var se = SpecialPowers.wrap(message).QueryInterface(Ci.nsIScriptError);
+          if (se.errorMessage.indexOf("XMLHttpRequest blocked") == -1)
+            return;
+        } catch(ex) {
+          return;
+        }
+        this.seenMsg = true;
+        try {
+                this.windowID = SpecialPowers.wrap(message).QueryInterface(Ci.nsIScriptError).innerWindowID;
+		todo(this.windowID != 0, "No windowID for the XMLHttpRequest blocked message.");
+		consoleService.unregisterListener(consoleListener);
+		ok(this.seenMsg,"Didn't get XMLHttpRequest blocked warning message.");
+		SimpleTest.finish();
+        } catch (ex) { }
+    },
+    
+
+    QueryInterface: function(iid) {
+      if (iid.equals(Ci.nsIConsoleListener) ||
+          iid.equals(Ci.nsISupports)) {
+        return this;
+      }
+      throw Cr.NS_NOINTERFACE;
+    }
+  };
+
+	consoleService.reset();
+	consoleService.registerListener(consoleListener);
+	SimpleTest.waitForExplicitFinish();
+	var xhr = new XMLHttpRequest();
+	xhr.open("GET", "http://example.org/tests/content/base/test/file_CrossSiteXHR_server.sjs?allowOrigin=http://invalid", true);
+	xhr.send(null);
+
+  
+</script>
+</pre>
+Testing...
+</body>
+</html>

Comment 13

[worker.thread]

Attachment: Fix for bug 713980, with test. Added blocked source URI to the warning message, and error code in hex.

Comment 14

[Josh Matthews [:jdm]]

[14:54:25] <sicking> jdm: ideally it should be localizable
[14:54:39] <jdm> sicking: ok. do you have some examples/docs/whatever on how to do that?
[14:55:03] <sicking> jdm: apparently i was wrong and the only reason we "cheat" sometime is to get stuff in past string freeze
[14:55:10] <sicking> jdm: sure, lemme find it
[14:56:57] <sicking> jdm: this is probably the best function to use: http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentUtils.h#766
[14:57:18] <sicking> jdm: you basically stick the error message in a properties file. And then point to that message when reporting the error

Comment 15

[Josh Matthews [:jdm]]

[14:58:04] <sicking> jdm: you can probably make the message take two arguments. The URL trying to load something, and the URL it was trying to load
[14:58:42] <sicking> jdm: actually, that makes it just one argument, and then specify the aURI parameter
[14:58:59] <sicking> jdm: you can leave the last three arguments empty

Comment 16

[Mark Goodwin [:mgoodwin]]

Attachment: Fix for bug 713980, with test. Added blocked source URI to the warning message, and error code in hex. Now with less bitrot.

Getting stuff working before I introduce localized strings.

Comment 17

[Mark Goodwin [:mgoodwin]]

Attachment: Fix for bug 713980, with test.

Changed to ensure innerWindowID makes it to the nsIScriptError (so warning messages appear in Web Console as well as the JS console).

Added entries for the 3 errors that can appear to dom/locales/en-US/chrome/dom/dom.properties to allow localized strings.

Could probably do with a mochitest to ensure the message actually appears in the webconsole, also grobinson is working on a patch for a new webconsole filter button for security items so I'm not attempting to land this yet (though feedback would be welcome).

Comment 18

[Mark Goodwin [:mgoodwin]]

Garrett, what's the sec. button bug? What magic do I need to make these web console messages appear in the right place?

Comment 19

[Mark Goodwin [:mgoodwin]]

I found it (thanks Mihai), it's bug 837351

Comment 20

[Garrett Robinson [:grobinson]]

The magic is:

1) declare a unique category for this type of error in the call to ReportToConsole/the nsIScriptError
2) In browser/devtools/webconsole/webconsole.js:4280 (in categoryForScriptError), add a case for your category so the function returns CATEGORY_SECURITY.

Additionally, I created a new security/security.properties file in the Content changes patch in 837351. I am using it for the Mixed Content blocker messages and as soon as I have the review I will also convert the CSP logging to use that file/this new code. I encourage you to use that instead of adding more to dom.properties.

Comment 21

[Mark Goodwin [:mgoodwin]]

Thanks Garrett; shall I start looking at what we need for web fonts now?

Comment 22

[Garrett Robinson [:grobinson]]

Sure! Also - can you confirm for me that you were able to get the test in the above patch to run (on top of the latest mozilla-central)? I receive the following error when I try to run it with "mach mochitest-plain content/base/test/test_bug713980.html":

failed | uncaught exception - TypeError: Cc is undefined at http://mochi.test:8888/tests/content/base/test/test_bug713980.html?autorun=1&closeWhenDone=1&consoleLevel=INFO&failureFile=/home/garrett/code/mozilla-central/obj-ff-dbg/.mozbuild/mochitest_failures.json:20

Comment 23

[Garrett Robinson [:grobinson]]

Attachment: Patch 5

This patch moves the localizable error messages from dom.properties to security.properties, which seems to be more appropriate. mgoodwin, you previously labeled these errors as "Content Security Policy" errors, which I have renamed to "CSP" so we can reuse the preexisting tag. Wouldn't something like "CORS" be more appropriate?

This patch "fixes" the previously mentioned error by using the Specialpowers.Cc, etc. convention seen elsewhere. The test now fails because it cannot find an innerWindowID. This appears to be a test failure, since manually inspecting the Security Pane in the Web Console while the test is running shows the expected error, which would be impossible if it was unable to correctly resolve the ID of the window associated with originating the XHR request.

Comment 24

[Garrett Robinson [:grobinson]]

Attachment: Patch 6

This patch creates a new category for these errors ("CORS" errors). It fixes the test that was previously failing and makes it more precise.

Comment 25

[Mark Goodwin [:mgoodwin]]

Comment on attachment 742004 [details] [diff] [review]
Patch 6

This looks great, Garrett. Thanks.

Comment 27

[Sebastian Zartner [:sebo]]

FWIW the Firebug team just had a request for this:
http://code.google.com/p/fbug/issues/detail?id=6558

So it would be great to get some progress here.

Sebastian

Comment 28

[Garrett Robinson [:grobinson]]

Sebastian - the patch here implements the basic functionality. I let it sit because I was working on trying to get useful information for developers (line number where the blocked XHR was sent, etc.). This ended up being harder than I thought, and then I was diverted to work on other projects.

If the current functionality is sufficient, I can get this patch reviewed and try to land it. If you think it would be worthwhile to wait until we get enhance it with better info for developers, I can return to it later this week and try to get that done.

Comment 29

[Sebastian Zartner [:sebo]]

I think it depends on how long it will take to get the enhanced information. If it just takes a few days, it's worth to wait for that. Otherwise the patch could be landed and a new bug could be opened for the improving the message by the additional info.

Sebastian

Comment 31

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

It's probably worth landing what is there, and try to get more detailed information later.

Comment 33

[Eldmannen]

No mention of same origin policy?
Does this apply to same origin policy too?

If so, then same origin policy should be mentioned in the log message, so the user knows *why* and can look it up to better understand it.

Comment 34

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

None of the failures discussed here happen for purely same-origin requests.

Comment 35

[Paul Rouget [:paul]]

Who can review this?

Comment 36

[Dirkjan Ochtman (:djc)]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #31)
> It's probably worth landing what is there, and try to get more detailed
> information later.

Yes, please!

Comment 37

[Garrett Robinson [:grobinson]]

I'm unbitrotting the patch now.

Comment 38

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

Smaug should be able to help if help is needed.

Comment 39

[Garrett Robinson [:grobinson]]

Attachment: 7.patch

Unbitrotted. The original test no longer worked due to API changes in SpecialPowers. I rewrote the entire test, and happily was able to simplify it greatly in the process.

I was unable to use some of the more advanced helpers in SimpleTest (such as runTestExpectingConsoleMessages) without making the test xhr sync, but I copied the technique it uses (SimpleTest.executeSoon) to avoid reentrancy during cleanup.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8336364 [details] [diff] [review]
7.patch


>+static nsresult
>+LogBlockedMessage(nsIRequest *aRequest, nsresult aStatus)
* goes with the type. nsIRequest* aRequest


>+{
>+  nsCOMPtr<nsIChannel> nsChan = do_QueryInterface(aRequest);
>+  nsCOMPtr<nsIConsoleService> console(do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>+  if (!console) return NS_ERROR_NOT_AVAILABLE;
Always {} with if.


>+
>+  //Generate the error message
>+  nsXPIDLString blockedMessage;
>+  nsresult rv = nsContentUtils::GetLocalizedString(nsContentUtils::eSECURITY_PROPERTIES,
>+      "XMLHttpRequestBlocked",
>+      blockedMessage);
2 space indentation


>+
>+  if (!NS_SUCCEEDED(aStatus)) {
>+    nsAutoString status;
>+    status.AppendInt((PRInt32) aStatus,16);
space before 16 and use C++ casting

>+    const PRUnichar* params[] = { status.get() };
>+
>+    if (NS_ERROR_DOM_BAD_URI == aStatus) {
>+      rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eSECURITY_PROPERTIES,
>+      "XMLHttpRequestBlockedBadURI",
>+      params, blockedMessage);
You're missing indentation here for the last two params

>+    } else {
>+      rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eSECURITY_PROPERTIES,
>+      "XMLHttpRequestBlockedWithStatus",
>+      params, blockedMessage);
ditto



>+
>+  nsCOMPtr<nsIScriptError> scriptError = do_CreateInstance(NS_SCRIPTERROR_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  //Get the innerWindowID associated with the XMLHTTPRequest
>+
>+  PRUint64 innerWindowID = 0;
>+  nsCOMPtr<nsIInterfaceRequestor> callbacks;
>+  nsChan->GetNotificationCallbacks(getter_AddRefs(callbacks));
>+  if (!callbacks) {
>+    nsCOMPtr<nsILoadGroup> aLG;
aFoo naming is reserved for arguments


>+    nsChan->GetLoadGroup(getter_AddRefs(aLG));
>+    aLG->GetNotificationCallbacks(getter_AddRefs(callbacks));
>+  }
>+
>+  if (callbacks) {
>+    nsCOMPtr<nsILoadContext> nsILC = do_GetInterface(callbacks);
just loadContext wouldn't be that long variable name

>+  if (!innerWindowID) {
>+    nsCOMPtr<nsILoadGroup> aLG;
again, aFoo naming


>+    aRequest->GetLoadGroup(getter_AddRefs(aLG));
>+    aLG->GetNotificationCallbacks(getter_AddRefs(callbacks));
>+    if (callbacks) {
>+      nsCOMPtr<nsILoadContext> nsILC = do_GetInterface(callbacks);
>+      if(nsILC) {
>+        nsCOMPtr<nsIDOMWindow> win;
>+        nsILC->GetAssociatedWindow(getter_AddRefs(win));
>+        if (win) {
>+          nsCOMPtr<nsIDOMWindowUtils> du = do_GetInterface(win);
>+          du->GetCurrentInnerWindowID(&innerWindowID);
>+        }
>+      }
>+    }
>+  }


So you have 
        if (win) {
          nsCOMPtr<nsIDOMWindowUtils> du = do_GetInterface(win);
          du->GetCurrentInnerWindowID(&innerWindowID);
        }
twice. Just move that outside those nested ifs?

>+  if (aUri) aUri->GetSpec(src);
{}

>+
>+  //Generate a scriptError and log it to the console
>+  nsAutoString tmpMsg(blockedMessage.get());
>+  nsAutoString tmpSrc = NS_ConvertUTF8toUTF16(src);
>+  rv = scriptError->InitWithWindowID(tmpMsg,
>+      tmpSrc,
>+      EmptyString(),
>+      0,
>+      0,
>+      nsIScriptError::warningFlag,
>+      "CORS",
>+      innerWindowID);
indentation

Comment 41

[Garrett Robinson [:grobinson]]

Attachment: 713980-warning-cross-site-xhr-8.patch

smaug, apologies for all of the issue in the last review. I just skimmed the code in nsCrossSiteListenerProxy, and had only wired in the logging to the Web Console.

Anyway, I did a more thorough job on this patch. All style issues are resolved. I refactored the function to make it easier to follow (IMHO). I fixed an issue with building the localized error message, so now we got the URL of the blocked XHR in the error message (yay!) The error message now correclty specifies when the XHR failed due to being cross-site.

I also removed a bunch of what seemed to be unnecessary QI'ing in LogBlockedMessage. Basically, the original patch QI'ed aRequest to an nsIChannel, then did a long QI chain. If that didn't work, the same procedure was repeated starting with aRequest. Thus, the version of the chain that started with the QI to nsIChannel seemed redundant. I removed it and the test still passes - let me know if that was somehow necessary!

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8345044 [details] [diff] [review]
713980-warning-cross-site-xhr-8.patch


>+  if (aUri) aUri->GetSpec(spec);

if (aUri) {
  aUri->GetSpec(spec);
}
>+      rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eSECURITY_PROPERTIES,
>+                                                 "XMLHttpRequestBlockedBadURI",
>+                                                 params,
>+                                                 blockedMessage);
CORS isn't XHR only

>+// Send a bad CORS request
>+var xhr = new XMLHttpRequest();
>+xhr.open("GET", "http://example.org/tests/content/base/test/file_CrossSiteXHR_server.sjs?allowOrigin=http://invalid", true);
I think we need tests for other things too which may use CORS

Comment 43

[Garrett Robinson [:grobinson]]

(In reply to Olli Pettay [:smaug] from comment #42)
> Comment on attachment 8345044 [details] [diff] [review]
> CORS isn't XHR only

Ah, good point. I was thinking too much about the original goal of this bug. So now we have code that can log *any* blocked cross-site request to the console. I think the best fix here would be to change the language in the error messages so it just says "Cross-Site Request Blocked". That way, we fix this bug (which just wanted logging for blocked cross-site XHR) and add some additional functionality as well.

> >+// Send a bad CORS request
> >+var xhr = new XMLHttpRequest();
> >+xhr.open("GET", "http://example.org/tests/content/base/test/file_CrossSiteXHR_server.sjs?allowOrigin=http://invalid", true);
> I think we need tests for other things too which may use CORS

Yes, I can write more tests. We should do a standard resource test (similar to the CORS tests), and keep the current XHR test. Anything else outstanding that should be tested?

Comment 44

[Garrett Robinson [:grobinson]]

Attachment: 713980-warning-cross-site-xhr-9.patch

The messages now report a generic "Cross-site request blocked" error, instead of a specific "XMLHttpRequest blocked error", which was inaccurate because XHR's are not the only thing that will be reportd by the CrossOriginListenerProxy.

I also cleaned up the message building logic in LogBlockedMessage. There was one branch that was incorrect - it would report a blocked request when aStatus was NS_SUCCESS, which is simply wrong. I removed that branch and added an assertion at the top that we should not call LogBlockedMessage unless the request was, indeed, blocked. The other branch was if aStatus == NS_DOM_ERROR_BAD_URI, which AFAICT is the only value it will ever have if a request is blocked for being cross-origin (from reading nsCORSListenerProxy::CheckRequestApproved).

I also added a test for loading a cross-origin webfont (which Firefox blocks by deafult). Both the cross-origin webfont and XHR loads are blocked and reported to the Web Console with their URI's. There is also a warning for blocked cross-origin web fonts specifically, so I'm not sure if we want the new error for those.

Comment 45

[Frederik Braun [:freddy]]

Just a nit, but can we make the message more explicit?

Something like this would be more helpful to web developers, who do not understand the SOP:
> Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at <originB>. This can be fixed by moving the resource to the same domain or enabling CORS on the remote end.

Arguably, the second sentence could be moved to the pages SOP and CORS pages on MDN (or SuMo?).

Comment 46

[Garrett Robinson [:grobinson]]

Yes, a more detailed message seems like a good idea and is in line with the original goal of the bug. I am not sure whether it would be better to put all that text in the message, or to make a "Learn More" link to an MDN page like we did with Insecure Passwords warnings. In this case, I'm leaning towards putting it all in the console message.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8349836 [details] [diff] [review]
713980-warning-cross-site-xhr-9.patch

>+  if (aUri) aUri->GetSpec(spec);
always {} with if
so
if (aURI) {
  ...
}
>+<body>
>+<pre id="test">
>+<div id="bad_webfont"></div>
>+
>+<script class="testbody" type="text/javascript">
The test looks racy. You should move 
<div id="bad_webfont"></div> to after the <script> so that the console listener is
guaranteed to be there when the font is loaded.

Comment 48

[Garrett Robinson [:grobinson]]

Attachment: 713980-warning-cross-site-xhr-10.patch

This patch fixes the issues from smuag's last comment, and carries over the r+. I also made the error message more descriptive based on freddyb's suggestion.

Try: https://tbpl.mozilla.org/?tree=Try&rev=c1f870772c0f

Comment 49

[Garrett Robinson [:grobinson]]

Try run uncovered some issues.

1. test_allowedDomainsXHR.js crashes
2. test_warning_for_blocked_cross_site_request.html times out on Android and B2G. Looks racy. Maybe the fix from Comment 47 wasn't enough? Interesting that it only manifested on Android and B2G.

Comment 50

[Garrett Robinson [:grobinson]]

Attachment: 713980-cors-logging-fix-xpcshell-test-crash

test_allowedDomainsXHR.js was failing due to the lack of a load group for the given request. This patch checks that we got a load group before continuing the QI chain so we don't dereference the null nsCOMPtr for the loadGroup.

I am not sure why just this test fails, but suspect it has something to do with creating and sending the XHR's in a Cu.Sandbox. Do XHR's in a sandbox not have a load group?

More generally, now if at any point the QI chain in LogBlockedMessage fails (innerWindowID == 0), it returns NS_ERROR_FAILURE. However, the caller does not check the return value so it seems like this logging code will fail silently. Would it be better to check the return value, or add an NS_ASSERTION here?

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

ah, yes, such XHR is quite different from the ones web pages create using
new XMLHttpRequest()

NS_ASSERTION doesn't help anything, and would be wrong.
NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "some warning message"); might make sense.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

The method doesn't need to return rv, but could be void.
For NS_ENSURE macros you could use _VOID versions.

Comment 53

[Garrett Robinson [:grobinson]]

Attachment: 713980-cors-logging

This patches fixes the unit test crash, and adds a warning message for failing to log blocked cross site requests. afaict this only happens for sandboxed XHR, which is so different from normal XHR that it's not a problem. The error is no longer silent, so future cases where this is an issue will be detectable.

This error also fixes the mochitest race condition, which was simply due to some misplaced braces. I have confirmed that the unit test now passes on desktop and B2G (emulator).

Try: https://tbpl.mozilla.org/?tree=Try&rev=bbfdfdaad450

Comment 54

[Garrett Robinson [:grobinson]]

Attachment: 713980-cors-logging

Updated commit message.

Comment 55

[Garrett Robinson [:grobinson]]

Try from Comment 53 looks good.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7ae8cbd023

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e7ae8cbd023