Bug 1602090 part 2. Create separate CheckMayLoad and CheckMayLoadWith…

…Reporting APIs. r=ckerschb CheckMayLoadAndReport takes a window ID. This allows us to report errors from it to the web console as needed. Most consumers know statically whether they want reporting or not, so there's no reason to force the ones that don't to provide window ids. Differential Revision: https://phabricator.services.mozilla.com/D56388 --HG-- extra : moz-landing-system : lando
dothq · Dec 12, 2019 · e0acf6e · e0acf6e
1 parent 9736474
commit e0acf6e
Show file tree

Hide file tree

Showing 19 changed files with 165 additions and 43 deletions.
diff --git a/browser/actors/ContextMenuChild.jsm b/browser/actors/ContextMenuChild.jsm
@@ -1137,7 +1137,7 @@ class ContextMenuChild extends JSWindowActorChild {
           try {
             if (elem.download) {
               // Ignore download attribute on cross-origin links
-              context.principal.checkMayLoad(context.linkURI, false, true);
+              context.principal.checkMayLoad(context.linkURI, true);
               context.linkDownload = elem.download;
             }
           } catch (ex) {}

diff --git a/caps/BasePrincipal.cpp b/caps/BasePrincipal.cpp
@@ -349,9 +349,26 @@ BasePrincipal::SubsumesConsideringDomainIgnoringFPD(nsIPrincipal* aOther,
 }
 
 NS_IMETHODIMP
-BasePrincipal::CheckMayLoad(nsIURI* aURI, bool aReport,
-                            bool aAllowIfInheritsPrincipal) {
+BasePrincipal::CheckMayLoad(nsIURI* aURI, bool aAllowIfInheritsPrincipal) {
+  return CheckMayLoadHelper(aURI, aAllowIfInheritsPrincipal, false, 0);
+}
+
+NS_IMETHODIMP
+BasePrincipal::CheckMayLoadWithReporting(nsIURI* aURI,
+                                         bool aAllowIfInheritsPrincipal,
+                                         uint64_t aInnerWindowID) {
+  return CheckMayLoadHelper(aURI, aAllowIfInheritsPrincipal, true,
+                            aInnerWindowID);
+}
+
+nsresult BasePrincipal::CheckMayLoadHelper(nsIURI* aURI,
+                                           bool aAllowIfInheritsPrincipal,
+                                           bool aReport,
+                                           uint64_t aInnerWindowID) {
   NS_ENSURE_ARG_POINTER(aURI);
+  MOZ_ASSERT(
+      aReport || aInnerWindowID == 0,
+      "Why do we have an inner window id if we're not supposed to report?");
 
   // Check the internal method first, which allows us to quickly approve loads
   // for the System Principal.
@@ -385,7 +402,7 @@ BasePrincipal::CheckMayLoad(nsIURI* aURI, bool aReport,
     if (NS_SUCCEEDED(rv) && prinURI) {
       nsScriptSecurityManager::ReportError(
           "CheckSameOriginError", prinURI, aURI,
-          mOriginAttributes.mPrivateBrowsingId > 0);
+          mOriginAttributes.mPrivateBrowsingId > 0, aInnerWindowID);
     }
   }
 

diff --git a/caps/BasePrincipal.h b/caps/BasePrincipal.h
@@ -110,8 +110,10 @@ class BasePrincipal : public nsJSPrincipals {
                                        bool* _retval) final;
   NS_IMETHOD SubsumesConsideringDomainIgnoringFPD(nsIPrincipal* other,
                                                   bool* _retval) final;
-  NS_IMETHOD CheckMayLoad(nsIURI* uri, bool report,
-                          bool allowIfInheritsPrincipal) final;
+  NS_IMETHOD CheckMayLoad(nsIURI* uri, bool allowIfInheritsPrincipal) final;
+  NS_IMETHOD CheckMayLoadWithReporting(nsIURI* uri,
+                                       bool allowIfInheritsPrincipal,
+                                       uint64_t innerWindowID) final;
   NS_IMETHOD GetAddonPolicy(nsISupports** aResult) final;
   NS_IMETHOD GetIsNullPrincipal(bool* aResult) override;
   NS_IMETHOD GetIsContentPrincipal(bool* aResult) override;
@@ -257,6 +259,10 @@ class BasePrincipal : public nsJSPrincipals {
   virtual bool MayLoadInternal(nsIURI* aURI) = 0;
   friend class ::ExpandedPrincipal;
 
+  // Helper for implementing CheckMayLoad and CheckMayLoadWithReporting.
+  nsresult CheckMayLoadHelper(nsIURI* aURI, bool aAllowIfInheritsPrincipal,
+                              bool aReport, uint64_t aInnerWindowID);
+
   void SetHasExplicitDomain() { mHasExplicitDomain = true; }
 
   // Either of these functions should be called as the last step of the

diff --git a/caps/nsIPrincipal.idl b/caps/nsIPrincipal.idl
@@ -140,16 +140,23 @@ interface nsIPrincipal : nsISerializable
      *
      *
      * @param uri    The URI about to be loaded.
-     * @param report If true, will report a warning to the console service
-     *               if the load is not allowed.
      * @param allowIfInheritsPrincipal   If true, the load is allowed if the
      *                                   loadee inherits the principal of the
      *                                   loader.
      * @throws NS_ERROR_DOM_BAD_URI if the load is not allowed.
      */
-    void checkMayLoad(in nsIURI uri, in boolean report,
+    void checkMayLoad(in nsIURI uri,
                       in boolean allowIfInheritsPrincipal);
 
+    /**
+     * Like checkMayLoad, but if returning an error will also report that error
+     * to the console, using the provided window id.  The window id may be 0 to
+     * report to just the browser console, not web consoles.
+     */
+    void checkMayLoadWithReporting(in nsIURI uri,
+                                   in boolean allowIfInheritsPrincipal,
+                                   in unsigned long long innerWindowID);
+
     /**
     * Checks if the provided URI is concidered third-party to the
     * URI of the principal.

diff --git a/caps/nsScriptSecurityManager.cpp b/caps/nsScriptSecurityManager.cpp
@@ -676,7 +676,8 @@ nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal* aPrincipal,
         aPrincipal->OriginAttributesRef().mPrivateBrowsingId > 0);
     NS_ENSURE_SUCCESS(rv, rv);
     // Check the principal is allowed to load the target.
-    return aPrincipal->CheckMayLoad(targetBaseURI, true, false);
+    // Unfortunately we don't have a window id to work with here...
+    return aPrincipal->CheckMayLoadWithReporting(targetBaseURI, false, 0);
   }
 
   //-- get the source scheme

diff --git a/devtools/client/webconsole/test/browser/browser.ini b/devtools/client/webconsole/test/browser/browser.ini
@@ -141,6 +141,7 @@ support-files =
   test-reopen-closed-tab.html
   test-simple-function.html
   test-simple-function.js
+  test-same-origin-required-load.html
   test-sourcemap-error-01.html
   test-sourcemap-error-01.js
   test-sourcemap-error-02.html
@@ -452,6 +453,7 @@ skip-if = fission
 [browser_webconsole_reverse_search_keyboard_navigation.js]
 [browser_webconsole_reverse_search_mouse_navigation.js]
 [browser_webconsole_reverse_search_toggle.js]
+[browser_webconsole_same_origin_errors.js]
 [browser_webconsole_sandbox_update_after_navigation.js]
 fail-if = fission
 [browser_webconsole_script_errordoc_urls.js]

diff --git a/devtools/client/webconsole/test/browser/browser_webconsole_same_origin_errors.js b/devtools/client/webconsole/test/browser/browser_webconsole_same_origin_errors.js
@@ -0,0 +1,29 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Ensure that same-origin errors are logged to the console.
+
+"use strict";
+
+const TEST_URI =
+  "http://example.com/browser/devtools/client/webconsole/test/browser/test-same-origin-required-load.html";
+
+add_task(async function() {
+  const hud = await openNewTabAndConsole(TEST_URI);
+
+  const targetURL = "http://example.org";
+  const onErrorMessage = waitForMessage(hud, "may not load data");
+  SpecialPowers.spawn(gBrowser.selectedBrowser, [targetURL], url => {
+    XPCNativeWrapper.unwrap(content).testTrack(url);
+  });
+  const message = await onErrorMessage;
+  const node = message.node;
+  ok(
+    node.classList.contains("error"),
+    "The message has the expected classname"
+  );
+  ok(
+    node.textContent.includes(targetURL),
+    "The message is about the thing we were expecting"
+  );
+});
diff --git a/devtools/client/webconsole/test/browser/test-same-origin-required-load.html b/devtools/client/webconsole/test/browser/test-same-origin-required-load.html
@@ -0,0 +1,26 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+   - License, v. 2.0. If a copy of the MPL was not distributed with this
+   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+<!doctype html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Test loads that are required to be same-origin (no CORS involved)</title>
+    <script>
+      /* exported testTrack */
+      "use strict";
+
+      function testTrack(url) {
+        const body = document.body;
+        const video = document.createElement("video");
+        const track = document.createElement("track");
+        track.src = url;
+        track.default = true;
+        video.append(track);
+        body.append(video);
+      }
+    </script>
+  </head>
+  <body>
+  </body>
+</html>
diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
@@ -10886,8 +10886,8 @@ nsDocShell::AddState(JS::Handle<JS::Value> aData, const nsAString& aTitle,
       // It's a file:// URI
       nsCOMPtr<nsIPrincipal> principal = document->GetPrincipal();
 
-      if (!principal ||
-          NS_FAILED(principal->CheckMayLoad(newURI, true, false))) {
+      if (!principal || NS_FAILED(principal->CheckMayLoadWithReporting(
+                            newURI, false, document->InnerWindowID()))) {
         return NS_ERROR_DOM_SECURITY_ERR;
       }
     }

diff --git a/dom/base/nsContentSink.cpp b/dom/base/nsContentSink.cpp
@@ -920,7 +920,7 @@ nsresult nsContentSink::SelectDocAppCache(
   } else {
     // The document was not loaded from an application cache
     // Here we know the manifest has the same origin as the
-    // document. There is call to CheckMayLoad() on it above.
+    // document. There is call to CheckMayLoadWithReporting() on it above.
 
     if (!aFetchedWithHTTPGetOrEquiv) {
       // The document was not loaded using HTTP GET or equivalent
@@ -1058,7 +1058,8 @@ void nsContentSink::ProcessOfflineManifest(const nsAString& aManifestSpec) {
     }
 
     // Documents must list a manifest from the same origin
-    rv = mDocument->NodePrincipal()->CheckMayLoad(manifestURI, true, false);
+    rv = mDocument->NodePrincipal()->CheckMayLoadWithReporting(
+        manifestURI, false, mDocument->InnerWindowID());
     if (NS_FAILED(rv)) {
       action = CACHE_SELECTION_RESELECT_WITHOUT_MANIFEST;
     } else {

diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp
@@ -5117,8 +5117,7 @@ void nsContentUtils::TriggerLink(nsIContent* aContent, nsIURI* aLinkURI,
          !aContent->IsSVGElement(nsGkAtoms::a)) ||
         !aContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::download,
                                         fileName) ||
-        NS_FAILED(
-            aContent->NodePrincipal()->CheckMayLoad(aLinkURI, false, true))) {
+        NS_FAILED(aContent->NodePrincipal()->CheckMayLoad(aLinkURI, true))) {
       fileName.SetIsVoid(true);  // No actionable download attribute was found.
     }
 
@@ -5665,9 +5664,9 @@ nsresult nsContentUtils::CheckSameOrigin(nsIChannel* aOldChannel,
 
   NS_ENSURE_STATE(oldPrincipal && newURI && newOriginalURI);
 
-  nsresult rv = oldPrincipal->CheckMayLoad(newURI, false, false);
+  nsresult rv = oldPrincipal->CheckMayLoad(newURI, false);
   if (NS_SUCCEEDED(rv) && newOriginalURI != newURI) {
-    rv = oldPrincipal->CheckMayLoad(newOriginalURI, false, false);
+    rv = oldPrincipal->CheckMayLoad(newOriginalURI, false);
   }
 
   return rv;
@@ -5814,7 +5813,7 @@ bool nsContentUtils::CheckMayLoad(nsIPrincipal* aPrincipal,
   NS_ENSURE_SUCCESS(rv, false);
 
   return NS_SUCCEEDED(
-      aPrincipal->CheckMayLoad(channelURI, false, aAllowIfInheritsPrincipal));
+      aPrincipal->CheckMayLoad(channelURI, aAllowIfInheritsPrincipal));
 }
 
 /* static */
@@ -6422,7 +6421,7 @@ bool nsContentUtils::ChannelShouldInheritPrincipal(
         // based on its own codebase later.
         //
         (URIIsLocalFile(aURI) &&
-         NS_SUCCEEDED(aLoadingPrincipal->CheckMayLoad(aURI, false, false)) &&
+         NS_SUCCEEDED(aLoadingPrincipal->CheckMayLoad(aURI, false)) &&
          // One more check here.  CheckMayLoad will always return true for the
          // system principal, but we do NOT want to inherit in that case.
          !aLoadingPrincipal->IsSystemPrincipal());

diff --git a/dom/cache/PrincipalVerifier.cpp b/dom/cache/PrincipalVerifier.cpp
@@ -144,7 +144,7 @@ void PrincipalVerifier::VerifyOnMainThread() {
       DispatchToInitiatingThread(rv);
       return;
     }
-    rv = principal->CheckMayLoad(uri, false, false);
+    rv = principal->CheckMayLoad(uri, false);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       DispatchToInitiatingThread(rv);
       return;

diff --git a/dom/fetch/Request.cpp b/dom/fetch/Request.cpp
@@ -233,7 +233,7 @@ class ReferrerSameOriginChecker final : public WorkerMainThreadRunnable {
     if (NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), mReferrerURL))) {
       nsCOMPtr<nsIPrincipal> principal = mWorkerPrivate->GetPrincipal();
       if (principal) {
-        mResult = principal->CheckMayLoad(uri, /* report */ false,
+        mResult = principal->CheckMayLoad(uri,
                                           /* allowIfInheritsPrincipal */ false);
       }
     }
@@ -362,7 +362,7 @@ already_AddRefed<Request> Request::Constructor(const GlobalObject& aGlobal,
           nsCOMPtr<nsIPrincipal> principal = global->PrincipalOrNull();
           if (principal) {
             nsresult rv =
-                principal->CheckMayLoad(uri, /* report */ false,
+                principal->CheckMayLoad(uri,
                                         /* allowIfInheritsPrincipal */ false);
             if (NS_FAILED(rv)) {
               referrerURL.AssignLiteral(kFETCH_CLIENT_REFERRER_STR);

diff --git a/dom/notification/Notification.cpp b/dom/notification/Notification.cpp
@@ -295,7 +295,8 @@ class FocusWindowRunnable final : public Runnable {
   }
 };
 
-nsresult CheckScope(nsIPrincipal* aPrincipal, const nsACString& aScope) {
+nsresult CheckScope(nsIPrincipal* aPrincipal, const nsACString& aScope,
+                    uint64_t aWindowID) {
   AssertIsOnMainThread();
   MOZ_ASSERT(aPrincipal);
 
@@ -305,8 +306,9 @@ nsresult CheckScope(nsIPrincipal* aPrincipal, const nsACString& aScope) {
     return rv;
   }
 
-  return aPrincipal->CheckMayLoad(scopeURI, /* report = */ true,
-                                  /* allowIfInheritsPrincipal = */ false);
+  return aPrincipal->CheckMayLoadWithReporting(
+      scopeURI,
+      /* allowIfInheritsPrincipal = */ false, aWindowID);
 }
 }  // anonymous namespace
 
@@ -2075,7 +2077,7 @@ class CheckLoadRunnable final : public WorkerMainThreadRunnable {
 
   bool MainThreadRun() override {
     nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();
-    mRv = CheckScope(principal, mScope);
+    mRv = CheckScope(principal, mScope, mWorkerPrivate->WindowID());
 
     if (NS_FAILED(mRv)) {
       return true;
@@ -2120,7 +2122,13 @@ already_AddRefed<Promise> Notification::ShowPersistentNotification(
       return nullptr;
     }
 
-    aRv = CheckScope(principal, NS_ConvertUTF16toUTF8(aScope));
+    uint64_t windowID = 0;
+    nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(aGlobal);
+    if (win) {
+      windowID = win->WindowID();
+    }
+
+    aRv = CheckScope(principal, NS_ConvertUTF16toUTF8(aScope), windowID);
     if (NS_WARN_IF(aRv.Failed())) {
       aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
       return nullptr;

diff --git a/dom/security/nsContentSecurityManager.cpp b/dom/security/nsContentSecurityManager.cpp
@@ -987,9 +987,9 @@ nsresult nsContentSecurityManager::CheckChannel(nsIChannel* aChannel) {
                nsIContentPolicy::TYPE_DOCUMENT);
     nsIPrincipal* loadingPrincipal = loadInfo->LoadingPrincipal();
 
-    // It doesn't matter what we pass for the third, data-inherits, argument.
+    // It doesn't matter what we pass for the second, data-inherits, argument.
     // Any protocol which inherits won't pay attention to cookies anyway.
-    rv = loadingPrincipal->CheckMayLoad(uri, false, false);
+    rv = loadingPrincipal->CheckMayLoad(uri, false);
     if (NS_FAILED(rv)) {
       AddLoadFlags(aChannel, nsIRequest::LOAD_ANONYMOUS);
     }

diff --git a/dom/serviceworkers/ServiceWorkerManager.cpp b/dom/serviceworkers/ServiceWorkerManager.cpp
@@ -910,8 +910,12 @@ class GetRegistrationsRunnable final : public Runnable {
         break;
       }
 
-      rv = principal->CheckMayLoad(scopeURI, true /* report */,
-                                   false /* allowIfInheritsPrincipal */);
+      // Unfortunately we don't seem to have an obvious window id here; in
+      // particular ClientInfo does not have one, and neither do service worker
+      // registrations, as far as I can tell.
+      rv = principal->CheckMayLoadWithReporting(
+          scopeURI, false /* allowIfInheritsPrincipal */,
+          0 /* innerWindowID */);
       if (NS_WARN_IF(NS_FAILED(rv))) {
         continue;
       }
@@ -973,8 +977,11 @@ class GetRegistrationRunnable final : public Runnable {
       return NS_OK;
     }
 
-    rv = principal->CheckMayLoad(uri, true /* report */,
-                                 false /* allowIfInheritsPrinciple */);
+    // Unfortunately we don't seem to have an obvious window id here; in
+    // particular ClientInfo does not have one, and neither do service worker
+    // registrations, as far as I can tell.
+    rv = principal->CheckMayLoadWithReporting(
+        uri, false /* allowIfInheritsPrincipal */, 0 /* innerWindowID */);
     if (NS_FAILED(rv)) {
       mPromise->Reject(NS_ERROR_DOM_SECURITY_ERR, __func__);
       return NS_OK;

diff --git a/dom/serviceworkers/ServiceWorkerUtils.cpp b/dom/serviceworkers/ServiceWorkerUtils.cpp
@@ -86,12 +86,14 @@ nsresult ServiceWorkerScopeAndScriptAreValid(const ClientInfo& aClientInfo,
   Unused << aScriptURI->GetRef(ref);
   NS_ENSURE_TRUE(ref.IsEmpty(), NS_ERROR_DOM_SECURITY_ERR);
 
-  rv = principal->CheckMayLoad(aScopeURI, true /* report */,
-                               false /* allowIfInheritsPrincipal */);
+  // Unfortunately we don't seem to have an obvious window id here; in
+  // particular ClientInfo does not have one.
+  rv = principal->CheckMayLoadWithReporting(
+      aScopeURI, false /* allowIfInheritsPrincipal */, 0 /* innerWindowID */);
   NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SECURITY_ERR);
 
-  rv = principal->CheckMayLoad(aScriptURI, true /* report */,
-                               false /* allowIfInheritsPrincipal */);
+  rv = principal->CheckMayLoadWithReporting(
+      aScriptURI, false /* allowIfInheritsPrincipal */, 0 /* innerWindowID */);
   NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SECURITY_ERR);
 
   return NS_OK;