Bug 1286712 - Validate runtime.sendMessage parameters draft
authorRob Wu <rob@robwu.nl>
Tue, 16 Aug 2016 21:36:42 -0700
changeset 403582 b7ebfdf5bace22d5341300cdd4e91ff5e544176b
parent 403581 f97a056ae6235de7855fd8aaa04fb1c8d183bd06
child 403583 47bf953c1f077f146942f580eeb842778aced6bf
push id26949
push userbmo:rob@robwu.nl
push dateSat, 20 Aug 2016 01:26:39 +0000
bugs1286712
milestone51.0a1
Bug 1286712 - Validate runtime.sendMessage parameters The main motive for this patch is to remove the use of the GlobalManager global (which was used to see if an extension ID is valid, which was specifically added in order to create thebrowser_ext_lastError.js test). To preserve test coverage I implemented a full validation of the runtime.sendMessage method. Now the error for a non-existent extension is identical in both the content script and background pages. Note that this also fixes a minor privacy leak: Previously extensions could see whether another extension is installed by sending a message to the specified extension and using the different responses to see whether another extension is installed. MozReview-Commit-ID: 82R97Ei25Xr
browser/components/extensions/test/browser/browser_ext_lastError.js
toolkit/components/extensions/MessageChannel.jsm
toolkit/components/extensions/ext-runtime.js
toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_errors.js
toolkit/components/extensions/test/xpcshell/xpcshell.ini
--- a/browser/components/extensions/test/browser/browser_ext_lastError.js
+++ b/browser/components/extensions/test/browser/browser_ext_lastError.js
@@ -1,17 +1,17 @@
 "use strict";
 
 function* sendMessage(options) {
   function background(options) {
-    browser.runtime.sendMessage("invalid-extension-id", {}, {}, result => {
+    browser.runtime.sendMessage(result => {
       browser.test.assertEq(undefined, result, "Argument value");
       if (options.checkLastError) {
         let lastError = browser[options.checkLastError].lastError;
-        browser.test.assertEq("Invalid extension ID",
+        browser.test.assertEq("runtime.sendMessage's message argument is missing",
                               lastError && lastError.message,
                               "lastError value");
       }
       browser.test.sendMessage("done");
     });
   }
 
   let extension = ExtensionTestUtils.loadExtension({
@@ -29,27 +29,27 @@ add_task(function* testLastError() {
   // Not necessary in browser-chrome tests, but monitorConsole gripes
   // if we don't call it.
   SimpleTest.waitForExplicitFinish();
 
   // Check that we have no unexpected console messages when lastError is
   // checked.
   for (let api of ["extension", "runtime"]) {
     let waitForConsole = new Promise(resolve => {
-      SimpleTest.monitorConsole(resolve, [{message: /Invalid extension ID/, forbid: true}]);
+      SimpleTest.monitorConsole(resolve, [{message: /message argument is missing/, forbid: true}]);
     });
 
     yield sendMessage({checkLastError: api});
 
     SimpleTest.endMonitorConsole();
     yield waitForConsole;
   }
 
   // Check that we do have a console message when lastError is not checked.
   let waitForConsole = new Promise(resolve => {
-    SimpleTest.monitorConsole(resolve, [{message: /Unchecked lastError value: Error: Invalid extension ID/}]);
+    SimpleTest.monitorConsole(resolve, [{message: /Unchecked lastError value: Error: runtime.sendMessage's message argument is missing/}]);
   });
 
   yield sendMessage({});
 
   SimpleTest.endMonitorConsole();
   yield waitForConsole;
 });
--- a/toolkit/components/extensions/MessageChannel.jsm
+++ b/toolkit/components/extensions/MessageChannel.jsm
@@ -512,17 +512,21 @@ this.MessageChannel = {
     let broker = this.responseManagers.get(target);
     broker.addHandler(channelId, deferred);
 
     let cleanup = () => {
       broker.removeHandler(channelId, deferred);
     };
     deferred.promise.then(cleanup, cleanup);
 
-    target.sendAsyncMessage(MESSAGE_MESSAGE, message);
+    try {
+      target.sendAsyncMessage(MESSAGE_MESSAGE, message);
+    } catch (e) {
+      deferred.reject(e);
+    }
     return deferred.promise;
   },
 
   _callHandlers(handlers, data) {
     let responseType = data.responseType;
 
     // At least one handler is required for all response types but
     // RESPONSE_ALL.
--- a/toolkit/components/extensions/ext-runtime.js
+++ b/toolkit/components/extensions/ext-runtime.js
@@ -69,30 +69,48 @@ extensions.registerSchemaAPI("runtime", 
         let recipient = {extensionId};
 
         return context.messenger.connect(Services.cpmm, name, recipient);
       },
 
       sendMessage: function(...args) {
         let options; // eslint-disable-line no-unused-vars
         let extensionId, message, responseCallback;
-        if (args.length == 1) {
+        if (typeof args[args.length - 1] == "function") {
+          responseCallback = args.pop();
+        }
+        if (!args.length) {
+          return Promise.reject({message: "runtime.sendMessage's message argument is missing"});
+        } else if (args.length == 1) {
           message = args[0];
         } else if (args.length == 2) {
-          [message, responseCallback] = args;
+          if (typeof args[0] == "string" && args[0]) {
+            [extensionId, message] = args;
+          } else {
+            [message, options] = args;
+          }
+        } else if (args.length == 3) {
+          [extensionId, message, options] = args;
+        } else if (args.length == 4 && !responseCallback) {
+          return Promise.reject({message: "runtime.sendMessage's last argument is not a function"});
         } else {
-          [extensionId, message, options, responseCallback] = args;
+          return Promise.reject({message: "runtime.sendMessage received too many arguments"});
+        }
+
+        if (extensionId != null && typeof extensionId != "string") {
+          return Promise.reject({message: "runtime.sendMessage's extensionId argument is invalid"});
         }
+        if (options != null && typeof options != "object") {
+          return Promise.reject({message: "runtime.sendMessage's options argument is invalid"});
+        }
+        // TODO(robwu): Validate option keys and values when we support it.
+
         extensionId = extensionId || extension.id;
         let recipient = {extensionId};
 
-        if (!GlobalManager.extensionMap.has(recipient.extensionId)) {
-          return context.wrapPromise(Promise.reject({message: "Invalid extension ID"}),
-                                     responseCallback);
-        }
         return context.messenger.sendMessage(Services.cpmm, message, recipient, responseCallback);
       },
 
       connectNative(application) {
         let app = new NativeApp(extension, context, application);
         return app.portAPI();
       },
 
new file mode 100644
--- /dev/null
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_errors.js
@@ -0,0 +1,64 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+"use strict";
+
+add_task(function* test_sendMessage_error() {
+  function background() {
+    let circ = {};
+    circ.circ = circ;
+    let testCases = [
+      // [arguments, expected error string],
+      [[], "runtime.sendMessage's message argument is missing"],
+      [[null, null, null, null], "runtime.sendMessage's last argument is not a function"],
+      [[null, null, 1], "runtime.sendMessage's options argument is invalid"],
+      [[1, null, null], "runtime.sendMessage's extensionId argument is invalid"],
+      [[null, null, null, null, null], "runtime.sendMessage received too many arguments"],
+
+      // Even when the parameters are accepted, we still expect an error
+      // because there is no onMessage listener.
+      [[null, null, null], "Could not establish connection. Receiving end does not exist."],
+
+      // Structural cloning doesn't work with DOM but we fall back
+      // JSON serialization, so we don't expect another error.
+      [[null, location, null], "Could not establish connection. Receiving end does not exist."],
+
+      // Structured cloning supports cyclic self-references.
+      [[null, [circ, location], null], "cyclic object value"],
+      // JSON serialization does not support cyclic references.
+      [[null, circ, null], "Could not establish connection. Receiving end does not exist."],
+      // (the last two tests shows whether sendMessage is implemented as structured cloning).
+    ];
+
+    // Repeat all tests with the undefined value instead of null.
+    for (let [args, expectedError] of testCases.slice()) {
+      args = args.map(arg => arg === null ? undefined : arg);
+      testCases.push([args, expectedError]);
+    }
+
+    function next() {
+      if (!testCases.length) {
+        browser.test.notifyPass("sendMessage parameter validation");
+        return;
+      }
+      let [args, expectedError] = testCases.shift();
+      let description = `runtime.sendMessage(${args.map(String).join(", ")})`;
+      return browser.runtime.sendMessage(...args)
+        .then(() => {
+          browser.test.fail(`Unexpectedly got no error for ${description}`);
+        }, err => {
+          browser.test.assertEq(expectedError, err.message, `expected error message for ${description}`);
+        }).then(next);
+    }
+    next();
+  }
+  let extensionData = {
+    background,
+  };
+
+  let extension = ExtensionTestUtils.loadExtension(extensionData);
+  yield extension.startup();
+
+  yield extension.awaitFinish("sendMessage parameter validation");
+
+  yield extension.unload();
+});
--- a/toolkit/components/extensions/test/xpcshell/xpcshell.ini
+++ b/toolkit/components/extensions/test/xpcshell/xpcshell.ini
@@ -34,16 +34,17 @@ skip-if = release_build
 [test_ext_localStorage.js]
 [test_ext_management.js]
 [test_ext_manifest_content_security_policy.js]
 [test_ext_manifest_incognito.js]
 [test_ext_onmessage_removelistener.js]
 [test_ext_runtime_connect_no_receiver.js]
 [test_ext_runtime_getPlatformInfo.js]
 [test_ext_runtime_sendMessage.js]
+[test_ext_runtime_sendMessage_errors.js]
 [test_ext_runtime_sendMessage_no_receiver.js]
 [test_ext_schemas.js]
 [test_ext_schemas_api_injection.js]
 [test_ext_simple.js]
 [test_ext_storage.js]
 [test_getAPILevelForWindow.js]
 [test_locale_converter.js]
 [test_locale_data.js]