Bug 1260511 part 4. Fix some of the places where registerProtocolHandler should be throwing a SECURITY_ERR to actually do so. r=gijs
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 22 Apr 2016 16:03:06 -0400
changeset 332477 7e8778ca64825cf31d106be737fd31d36a29b13c
parent 332476 7b4d87d3fbfdbd24c0d2680c6a3be868f208f815
child 332478 3d22a2e2a5948493a0ced8568b8c98a8486ecb37
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgijs
bugs1260511
milestone48.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1260511 part 4. Fix some of the places where registerProtocolHandler should be throwing a SECURITY_ERR to actually do so. r=gijs
browser/components/feeds/WebContentConverter.js
testing/web-platform/meta/html/webappapis/system-state-and-capabilities/the-navigator-object/content.html.ini
testing/web-platform/meta/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol.html.ini
--- a/browser/components/feeds/WebContentConverter.js
+++ b/browser/components/feeds/WebContentConverter.js
@@ -144,64 +144,79 @@ const Utils = {
       let baseURI = aContentWindow.document.baseURIObject;
       uri = this.makeURI(aURIString, null, baseURI);
     } catch (ex) {
       throw NS_ERROR_DOM_SYNTAX_ERR;
     }
 
     // For security reasons we reject non-http(s) urls (see bug 354316),
     // we may need to revise this once we support more content types
-    // XXX this should be a "security exception" according to spec, but that
-    // isn't defined yet.
-    if (uri.scheme != "http" && uri.scheme != "https")
-      throw("Permission denied to add " + uri.spec + " as a content or protocol handler");
+    if (uri.scheme != "http" && uri.scheme != "https") {
+      throw this.getSecurityError(
+        "Permission denied to add " + uri.spec + " as a content or protocol handler",
+        aContentWindow);
+    }
 
     // We also reject handlers registered from a different host (see bug 402287)
     // The pref allows us to test the feature
     let pb = Services.prefs;
     if (!pb.getBoolPref(PREF_ALLOW_DIFFERENT_HOST) &&
         (!["http:", "https:"].includes(aContentWindow.location.protocol) ||
          aContentWindow.location.hostname != uri.host)) {
-      throw("Permission denied to add " + uri.spec + " as a content or protocol handler");
+      throw this.getSecurityError(
+        "Permission denied to add " + uri.spec + " as a content or protocol handler",
+        aContentWindow);
     }
 
     // If the uri doesn't contain '%s', it won't be a good handler
     if (uri.spec.indexOf("%s") < 0)
       throw NS_ERROR_DOM_SYNTAX_ERR;
 
     return uri;
   },
 
   // NB: Throws if aProtocol is not allowed.
-  checkProtocolHandlerAllowed(aProtocol, aURIString) {
+  checkProtocolHandlerAllowed(aProtocol, aURIString, aWindowOrNull) {
     // First, check to make sure this isn't already handled internally (we don't
     // want to let them take over, say "chrome").
     let handler = Services.io.getProtocolHandler(aProtocol);
     if (!(handler instanceof Ci.nsIExternalProtocolHandler)) {
       // This is handled internally, so we don't want them to register
-      // XXX this should be a "security exception" according to spec, but that
-      // isn't defined yet.
-      throw(`Permission denied to add ${aURIString} as a protocol handler`);
+      throw this.getSecurityError(
+        `Permission denied to add ${aURIString} as a protocol handler`,
+        aWindowOrNull);
     }
 
     // check if it is in the black list
     let pb = Services.prefs;
     let allowed;
     try {
       allowed = pb.getBoolPref(PREF_HANDLER_EXTERNAL_PREFIX + "." + aProtocol);
     }
     catch (e) {
       allowed = pb.getBoolPref(PREF_HANDLER_EXTERNAL_PREFIX + "-default");
     }
     if (!allowed) {
-      // XXX this should be a "security exception" according to spec
-      throw(`Not allowed to register a protocol handler for ${aProtocol}`);
+      throw this.getSecurityError(
+        `Not allowed to register a protocol handler for ${aProtocol}`,
+        aWindowOrNull);
     }
   },
 
+  // Return a SecurityError exception from the given Window if one is given.  If
+  // none is given, just return the given error string, for lack of anything
+  // better.
+  getSecurityError(errorString, aWindowOrNull) {
+    if (!aWindowOrNull) {
+      return errorString;
+    }
+
+    return new aWindowOrNull.DOMException(errorString, "SecurityError");
+  },
+
   /**
    * Mappings from known feed types to our internal content type.
    */
   _mappings: {
     "application/rss+xml": TYPE_MAYBE_FEED,
     "application/atom+xml": TYPE_MAYBE_FEED,
   },
 
@@ -399,17 +414,18 @@ WebContentConverterRegistrar.prototype =
       // Inside the private browsing mode, we don't want to alert the user to save
       // a protocol handler.  We log it to the error console so that web developers
       // would have some way to tell what's going wrong.
       Services.console.
       logStringMessage("Web page denied access to register a protocol handler inside private browsing mode");
       return;
     }
 
-    Utils.checkProtocolHandlerAllowed(aProtocol, aURIString);
+    Utils.checkProtocolHandlerAllowed(aProtocol, aURIString,
+                                      haveWindow ? aBrowserOrWindow : null);
 
     // Now Ask the user and provide the proper callback
     let message = this._getFormattedString("addProtocolHandler",
                                            [aTitle, uri.host, aProtocol]);
 
     let notificationIcon = uri.prePath + "/favicon.ico";
     let notificationValue = "Protocol Registration: " + aProtocol;
     let addButton = {
@@ -467,21 +483,22 @@ WebContentConverterRegistrar.prototype =
     if (haveWindow) {
       uri = Utils.checkAndGetURI(aURIString, aWindowOrBrowser);
     } else if (aWindowOrBrowser) {
       // uri was vetted in the content process.
       uri = Utils.makeURI(aURIString, null);
     }
 
     // We only support feed types at present.
-    // XXX this should be a "security exception" according to spec, but that
-    // isn't defined yet.
     let contentType = Utils.resolveContentType(aContentType);
-    if (contentType != TYPE_MAYBE_FEED)
+    // XXX We should be throwing a Utils.getSecurityError() here in at least
+    // some cases.  See bug 1266492.
+    if (contentType != TYPE_MAYBE_FEED) {
       return;
+    }
 
     if (aWindowOrBrowser) {
       let notificationBox;
       if (haveWindow) {
         let browserWindow = this._getBrowserWindowForContentWindow(aWindowOrBrowser);
         let browserElement = this._getBrowserForContentWindow(browserWindow, aWindowOrBrowser);
         notificationBox = browserElement.getTabBrowser().getNotificationBox(browserElement);
       } else {
@@ -993,16 +1010,18 @@ WebContentConverterRegistrarContent.prot
     let messageManager = aBrowserOrWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                                          .getInterface(Ci.nsIWebNavigation)
                                          .QueryInterface(Ci.nsIDocShell)
                                          .QueryInterface(Ci.nsIInterfaceRequestor)
                                          .getInterface(Ci.nsITabChild)
                                          .messageManager;
 
     let uri = Utils.checkAndGetURI(aURIString, aBrowserOrWindow);
+    // XXX We should be throwing a Utils.getSecurityError() here in at least
+    // some cases.  See bug 1266492.
     if (Utils.resolveContentType(aContentType) != TYPE_MAYBE_FEED) {
       return;
     }
 
     messageManager.sendAsyncMessage("WCCR:registerContentHandler",
                                     { contentType: aContentType,
                                       uri: uri.spec,
                                       title: aTitle });
@@ -1013,17 +1032,17 @@ WebContentConverterRegistrarContent.prot
     let messageManager = aBrowserOrWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                                          .getInterface(Ci.nsIWebNavigation)
                                          .QueryInterface(Ci.nsIDocShell)
                                          .QueryInterface(Ci.nsIInterfaceRequestor)
                                          .getInterface(Ci.nsITabChild)
                                          .messageManager;
 
     let uri = Utils.checkAndGetURI(aURIString, aBrowserOrWindow);
-    Utils.checkProtocolHandlerAllowed(aProtocol, aURIString);
+    Utils.checkProtocolHandlerAllowed(aProtocol, aURIString, aBrowserOrWindow);
 
     messageManager.sendAsyncMessage("WCCR:registerProtocolHandler",
                                     { protocol: aProtocol,
                                       uri: uri.spec,
                                       title: aTitle });
   },
 
   /**
--- a/testing/web-platform/meta/html/webappapis/system-state-and-capabilities/the-navigator-object/content.html.ini
+++ b/testing/web-platform/meta/html/webappapis/system-state-and-capabilities/the-navigator-object/content.html.ini
@@ -4,25 +4,16 @@
     expected: FAIL
 
   [%s instead of subdomain name should throw syntax_err]
     expected: FAIL
 
   [a url argument pointing to a different domain name, without %s should throw SYNTAX_ERR]
     expected: FAIL
 
-  [a url argument pointing to a different domain name should throw SECURITY_ERR]
-    expected: FAIL
-
-  [a url argument pointing to a different domain name should throw SECURITY_ERR (2)]
-    expected: FAIL
-
-  [a url argument pointing to a different domain name should throw SECURITY_ERR (3)]
-    expected: FAIL
-
   [attempting to override the image/jpeg MIME type should throw SECURITY_ERR]
     expected: FAIL
 
   [attempting to override the text/html MIME type should throw SECURITY_ERR]
     expected: FAIL
 
   [attempting to override the text/javascript MIME type should throw SECURITY_ERR]
     expected: FAIL
--- a/testing/web-platform/meta/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol.html.ini
+++ b/testing/web-platform/meta/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol.html.ini
@@ -4,64 +4,22 @@
     expected: FAIL
 
   [%s instead of subdomain name should throw SYNTAX_ERR]
     expected: FAIL
 
   [a url argument pointing to a different domain name, without %s should throw SYNTAX_ERR]
     expected: FAIL
 
-  [a url argument pointing to a different domain name should throw SECURITY_ERR]
-    expected: FAIL
-
-  [a url argument pointing to a different domain name should throw SECURITY_ERR (2)]
-    expected: FAIL
-
-  [a url argument pointing to a different domain name should throw SECURITY_ERR (3)]
-    expected: FAIL
-
-  [looping handlers should throw SECURITY_ERR]
-    expected: FAIL
-
-  [a url argument pointing to a non-http[s\] scheme should throw SECURITY_ERR due to not being of the same origin]
-    expected: FAIL
-
-  [attempting to override the about protocol should throw SECURITY_ERR]
-    expected: FAIL
-
   [attempting to override the attachment protocol should throw SECURITY_ERR]
     expected: FAIL
 
-  [attempting to override the blob protocol should throw SECURITY_ERR]
-    expected: FAIL
-
-  [attempting to override the chrome protocol should throw SECURITY_ERR]
-    expected: FAIL
-
   [attempting to override the cid protocol should throw SECURITY_ERR]
     expected: FAIL
 
-  [attempting to override the data protocol should throw SECURITY_ERR]
-    expected: FAIL
-
-  [attempting to override the file protocol should throw SECURITY_ERR]
-    expected: FAIL
-
-  [attempting to override the ftp protocol should throw SECURITY_ERR]
-    expected: FAIL
-
-  [attempting to override the http protocol should throw SECURITY_ERR]
-    expected: FAIL
-
-  [attempting to override the https protocol should throw SECURITY_ERR]
-    expected: FAIL
-
-  [attempting to override the javascript protocol should throw SECURITY_ERR]
-    expected: FAIL
-
   [attempting to override the livescript protocol should throw SECURITY_ERR]
     expected: FAIL
 
   [attempting to override the mid protocol should throw SECURITY_ERR]
     expected: FAIL
 
   [attempting to override the mocha protocol should throw SECURITY_ERR]
     expected: FAIL
@@ -70,32 +28,14 @@
     expected: FAIL
 
   [attempting to override the operamail protocol should throw SECURITY_ERR]
     expected: FAIL
 
   [attempting to override the res protocol should throw SECURITY_ERR]
     expected: FAIL
 
-  [attempting to override the resource protocol should throw SECURITY_ERR]
-    expected: FAIL
-
   [attempting to override the shttp protocol should throw SECURITY_ERR]
     expected: FAIL
 
   [attempting to override the tcl protocol should throw SECURITY_ERR]
     expected: FAIL
 
-  [attempting to override the vbscript protocol should throw SECURITY_ERR]
-    expected: FAIL
-
-  [attempting to override the view-source protocol should throw SECURITY_ERR]
-    expected: FAIL
-
-  [attempting to override the ws protocol should throw SECURITY_ERR]
-    expected: FAIL
-
-  [attempting to override the wss protocol should throw SECURITY_ERR]
-    expected: FAIL
-
-  [attempting to override the wyciwyg protocol should throw SECURITY_ERR]
-    expected: FAIL
-