Bug 1299411 - Error messages for native messaging r=aswan
authorRob Wu <rob@robwu.nl>
Sat, 24 Sep 2016 13:45:02 +0200
changeset 320067 c456c1797299e5dfc39187992e30c323f7d92ee1
parent 320066 ed1afd2aad61c64bb938cf1647975b4fda846d66
child 320068 c2b23c06acfe4086c26e844ffb9bd94e01c6a252
push id20749
push userryanvm@gmail.com
push dateSat, 29 Oct 2016 13:21:21 +0000
treeherderfx-team@1b170b39ed6b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaswan
bugs1299411
milestone52.0a1
Bug 1299411 - Error messages for native messaging r=aswan - Combine the errors for a non-existing app and lacking permissions to avoid information leakage. - Do not treat normal application exit as an error. - Create errors in the right context. - Add tests that check the error messages. MozReview-Commit-ID: HxBpeCSyyGN
toolkit/components/extensions/NativeMessaging.jsm
toolkit/components/extensions/test/xpcshell/test_ext_native_messaging.js
--- a/toolkit/components/extensions/NativeMessaging.jsm
+++ b/toolkit/components/extensions/NativeMessaging.jsm
@@ -158,16 +158,20 @@ this.HostManifestManager = {
     if (!VALID_APPLICATION.test(application)) {
       throw new Error(`Invalid application "${application}"`);
     }
     return this.init().then(() => this._lookup(application, context));
   },
 };
 
 this.NativeApp = class extends EventEmitter {
+  /**
+   * @param {BaseContext} context The context that initiated the native app.
+   * @param {string} application The identifier of the native app.
+   */
   constructor(context, application) {
     super();
 
     this.context = context;
     this.name = application;
 
     // We want a close() notification when the window is destroyed.
     this.context.callOnClose(this);
@@ -175,22 +179,20 @@ this.NativeApp = class extends EventEmit
     this.proc = null;
     this.readPromise = null;
     this.sendQueue = [];
     this.writePromise = null;
     this.sentDisconnect = false;
 
     this.startupPromise = HostManifestManager.lookupApplication(application, context)
       .then(hostInfo => {
-        if (!hostInfo) {
-          throw new Error(`No such native application ${application}`);
-        }
-
-        if (!hostInfo.manifest.allowed_extensions.includes(context.extension.id)) {
-          throw new Error(`This extension does not have permission to use native application ${application}`);
+        // Put the two errors together to not leak information about whether a native
+        // application is installed to addons that do not have the right permission.
+        if (!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id)) {
+          throw new context.cloneScope.Error(`This extension does not have permission to use native application ${application} (or the application is not installed)`);
         }
 
         let command = hostInfo.manifest.path;
         if (AppConstants.platform == "win") {
           // OS.Path.join() ignores anything before the last absolute path
           // it sees, so if command is already absolute, it remains unchanged
           // here.  If it is relative, we get the proper absolute path here.
           command = OS.Path.join(OS.Path.dirname(hostInfo.path), command);
@@ -225,17 +227,17 @@ this.NativeApp = class extends EventEmit
    * @param {string} portId A unique internal ID that identifies the port.
    * @param {object} sender The object describing the creator of the connection
    *     request.
    * @param {string} application The name of the native messaging host.
    */
   static onConnectNative(context, messageManager, portId, sender, application) {
     let app = new NativeApp(context, application);
     let port = new ExtensionUtils.Port(context, messageManager, [messageManager], "", portId, sender, sender);
-    app.once("disconnect", (what, err) => port.disconnect(err && err.message));
+    app.once("disconnect", (what, err) => port.disconnect(err));
 
     /* eslint-disable mozilla/balanced-listeners */
     app.on("message", (what, msg) => port.postMessage(msg));
     /* eslint-enable mozilla/balanced-listeners */
 
     port.registerOnMessage(msg => app.send(msg));
     port.registerOnDisconnect(msg => app.close());
   }
@@ -264,17 +266,17 @@ this.NativeApp = class extends EventEmit
 
   _startRead() {
     if (this.readPromise) {
       throw new Error("Entered _startRead() while readPromise is non-null");
     }
     this.readPromise = this.proc.stdout.readUint32()
       .then(len => {
         if (len > NativeApp.maxRead) {
-          throw new Error(`Native application tried to send a message of ${len} bytes, which exceeds the limit of ${NativeApp.maxRead} bytes.`);
+          throw new this.context.cloneScope.Error(`Native application tried to send a message of ${len} bytes, which exceeds the limit of ${NativeApp.maxRead} bytes.`);
         }
         return this.proc.stdout.readJSON(len);
       }).then(msg => {
         this.emit("message", msg);
         this.readPromise = null;
         this._startRead();
       }).catch(err => {
         if (err.errorCode != Subprocess.ERROR_END_OF_FILE) {
@@ -395,16 +397,19 @@ this.NativeApp = class extends EventEmit
     if (this.proc) {
       doCleanup();
     } else if (this.startupPromise) {
       this.startupPromise.then(doCleanup);
     }
 
     if (!this.sentDisconnect) {
       this.sentDisconnect = true;
+      if (err && err.errorCode == Subprocess.ERROR_END_OF_FILE) {
+        err = null;
+      }
       this.emit("disconnect", err);
     }
   }
 
   // Called from Context when the extension is shut down.
   close() {
     this._cleanup();
   }
--- a/toolkit/components/extensions/test/xpcshell/test_ext_native_messaging.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_native_messaging.js
@@ -324,17 +324,19 @@ add_task(function* test_read_limit() {
   function clearPref() {
     Services.prefs.clearUserPref(PREF_MAX_READ);
   }
   do_register_cleanup(clearPref);
 
   function background() {
     const PAYLOAD = "0123456789A";
     let port = browser.runtime.connectNative("echo");
-    port.onDisconnect.addListener(() => {
+    port.onDisconnect.addListener(msgPort => {
+      browser.test.assertEq(port, msgPort, "onDisconnect handler should receive the port as the first argument");
+      browser.test.assertEq("Native application tried to send a message of 13 bytes, which exceeds the limit of 10 bytes.", port.error && port.error.message);
       browser.test.sendMessage("result", "disconnected");
     });
     port.onMessage.addListener(msg => {
       browser.test.sendMessage("result", "message");
     });
     port.postMessage(PAYLOAD);
   }
 
@@ -378,17 +380,19 @@ add_task(function* test_ext_permission()
   yield extension.unload();
 });
 
 // Test that an extension that is not listed in allowed_extensions for
 // a native application cannot use that application.
 add_task(function* test_app_permission() {
   function background() {
     let port = browser.runtime.connectNative("echo");
-    port.onDisconnect.addListener(() => {
+    port.onDisconnect.addListener(msgPort => {
+      browser.test.assertEq(port, msgPort, "onDisconnect handler should receive the port as the first argument");
+      browser.test.assertEq("This extension does not have permission to use native application echo (or the application is not installed)", port.error && port.error.message);
       browser.test.sendMessage("result", "disconnected");
     });
     port.onMessage.addListener(msg => {
       browser.test.sendMessage("result", "message");
     });
     port.postMessage({test: "test"});
   }
 
@@ -439,17 +443,19 @@ add_task(function* test_child_process() 
   let exitPromise = waitForSubprocessExit();
   yield extension.unload();
   yield exitPromise;
 });
 
 add_task(function* test_stderr() {
   function background() {
     let port = browser.runtime.connectNative("stderr");
-    port.onDisconnect.addListener(() => {
+    port.onDisconnect.addListener(msgPort => {
+      browser.test.assertEq(port, msgPort, "onDisconnect handler should receive the port as the first argument");
+      browser.test.assertEq(null, port.error, "Normal application exit is not an error");
       browser.test.sendMessage("finished");
     });
   }
 
   let {messages} = yield promiseConsoleOutput(function* () {
     let extension = ExtensionTestUtils.loadExtension({
       background,
       manifest: {