Bug 1337743 - More data validation checks for Marionette messages; r=whimboo

This change introduces more data validation checks on unmarshaling
Marionette protocol messages.  Specifically, validation of
message.Command's and message.Response's constructor arguments and packet
contents in their respective fromMsg functions are tested.

Doing these tests ensures more safety further down the pipeline with
respect to the data integrity in Marionette commands.

MozReview-Commit-ID: BxYipX5zfo9

--HG--
extra : rebase_source : 5cd9edab8801323b19688f871ba78ccf70a05c5e

testing/marionette/message.js

@@ -9,6 +9,7 @@ var {utils: Cu} = Components;
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 
+Cu.import("chrome://marionette/content/assert.js");
 Cu.import("chrome://marionette/content/error.js");
 
 this.EXPORTED_SYMBOLS = [
@@ -67,18 +68,18 @@ Message.fromMsg = function (data) {
  *
  * where
  *
- *   type:
+ *   type (integer)
  *     Must be zero (integer). Zero means that this message is a command.
  *
- *   id:
- *     Number used as a sequence number.  The server replies with a
- *     requested id.
+ *   id (integer)
+ *     Integer used as a sequence number.  The server replies with the
+ *     same ID for the response.
  *
- *   name:
+ *   name (string)
  *     String representing the command name with an associated set of
  *     remote end steps.
  *
- *   params:
+ *   params (JSON Object or null)
  *     Object of command function arguments.  The keys of this object
  *     must be strings, but the values can be arbitrary values.
  *
@@ -99,9 +100,9 @@ Message.fromMsg = function (data) {
  */
 this.Command = class {
   constructor(msgID, name, params = {}) {
-    this.id = msgID;
-    this.name = name;
-    this.parameters = params;
+    this.id = assert.integer(msgID);
+    this.name = assert.string(name);
+    this.parameters = assert.object(params);
 
     this.onerror = null;
     this.onresult = null;
@@ -119,9 +120,9 @@ this.Command = class {
    *     {@code onerror} or {@code onresult} handlers to.
    */
   onresponse(resp) {
-    if (resp.error && this.onerror) {
+    if (this.onerror && resp.error) {
       this.onerror(resp.error);
-    } else if (resp.body && this.onresult) {
+    } else if (this.onresult && resp.body) {
       this.onresult(resp.body);
     }
   }
@@ -137,7 +138,8 @@ this.Command = class {
   }
 
   static fromMsg(msg) {
-    let [msgID, name, params] = [msg[1], msg[2], msg[3]];
+    let [type, msgID, name, params] = msg;
+    assert.that(n => n === Command.TYPE)(type);
 
     // if parameters are given but null, treat them as undefined
     if (params === null) {
@@ -201,23 +203,22 @@ this.ResponseBody = () => new Proxy({}, validator);
  * has finished executing, and any modifications made subsequent to that
  * will have no effect.
  *
- * @param {number} msgId
+ * @param {number} msgID
  *     Message ID tied to the corresponding command request this is a
  *     response for.
  * @param {function(Response|Message)} respHandler
  *     Function callback called on sending the response.
  */
 this.Response = class {
-  constructor(msgId, respHandler) {
-    this.id = msgId;
+  constructor(msgID, respHandler = () => {}) {
+    this.id = assert.integer(msgID);
+    this.respHandler_ = assert.callable(respHandler);
 
     this.error = null;
     this.body = ResponseBody();
 
     this.origin = MessageOrigin.Server;
     this.sent = false;
-
-    this.respHandler_ = respHandler;
   }
 
   /**
@@ -282,9 +283,13 @@ this.Response = class {
   }
 
   static fromMsg(msg) {
-    let resp = new Response(msg[1], null);
-    resp.error = msg[2];
-    resp.body = msg[3];
+    let [type, msgID, err, body] = msg;
+    assert.that(n => n === Response.TYPE)(type);
+
+    let resp = new Response(msgID);
+    resp.error = assert.string(err);
+
+    resp.body = body;
     return resp;
   }
 };

testing/marionette/test_message.js

@@ -17,6 +17,7 @@ add_test(function test_MessageOrigin() {
 add_test(function test_Message_fromMsg() {
   let cmd = new Command(4, "foo");
   let resp = new Response(5, () => {});
+  resp.error = "foo";
 
   ok(Message.fromMsg(cmd.toMsg()) instanceof Command);
   ok(Message.fromMsg(resp.toMsg()) instanceof Response);
@@ -43,14 +44,14 @@ add_test(function test_Command_onresponse() {
   let onerrorOk = false;
   let onresultOk = false;
 
-  let cmd = new Command();
+  let cmd = new Command(7, "foo");
   cmd.onerror = () => onerrorOk = true;
   cmd.onresult = () => onresultOk = true;
 
-  let errorResp = new Response();
+  let errorResp = new Response(8, () => {});
   errorResp.error = new WebDriverError("foo");
 
-  let bodyResp = new Response();
+  let bodyResp = new Response(9, () => {});
   bodyResp.body = "bar";
 
   cmd.onresponse(errorResp);
@@ -63,7 +64,7 @@ add_test(function test_Command_onresponse() {
   run_next_test();
 });
 
-add_test(function test_Command_fromMsg() {
+add_test(function test_Command_ctor() {
   let cmd = new Command(42, "bar", {bar: "baz"});
   let msg = cmd.toMsg();
 
@@ -95,6 +96,15 @@ add_test(function test_Command_fromMsg() {
   equal(c1.name, c2.name);
   equal(c1.parameters, c2.parameters);
 
+  Assert.throws(() => Command.fromMsg([null, 2, "foo", {}]));
+  Assert.throws(() => Command.fromMsg([1, 2, "foo", {}]));
+  Assert.throws(() => Command.fromMsg([0, null, "foo", {}]));
+  Assert.throws(() => Command.fromMsg([0, 2, null, {}]));
+  Assert.throws(() => Command.fromMsg([0, 2, "foo", false]));
+
+  let nullParams = Command.fromMsg([0, 2, "foo", null]);
+  equal("[object Object]", Object.prototype.toString.call(nullParams.parameters));
+
   run_next_test();
 });
 
@@ -103,7 +113,7 @@ add_test(function test_Command_TYPE() {
   run_next_test();
 });
 
-add_test(function test_Response() {
+add_test(function test_Response_ctor() {
   let handler = () => run_next_test();
 
   let resp = new Response(42, handler);
@@ -159,7 +169,7 @@ add_test(function test_Response_sendError() {
 });
 
 add_test(function test_Response_toMsg() {
-  let resp = new Response(42);
+  let resp = new Response(42, () => {});
   let msg = resp.toMsg();
 
   equal(Response.TYPE, msg[0]);
@@ -171,7 +181,7 @@ add_test(function test_Response_toMsg() {
 });
 
 add_test(function test_Response_toString() {
-  let resp = new Response(42);
+  let resp = new Response(42, () => {});
   resp.error = "foo";
   resp.body = "bar";
 
@@ -184,7 +194,7 @@ add_test(function test_Response_toString() {
 });
 
 add_test(function test_Response_fromMsg() {
-  let r1 = new Response(42);
+  let r1 = new Response(42, () => {});
   r1.error = "foo";
   r1.body = "bar";
 
@@ -195,6 +205,12 @@ add_test(function test_Response_fromMsg() {
   equal(r1.error, r2.error);
   equal(r1.body, r2.body);
 
+  Assert.throws(() => Response.fromMsg([null, 2, "foo", {}]));
+  Assert.throws(() => Response.fromMsg([0, 2, "foo", {}]));
+  Assert.throws(() => Response.fromMsg([1, null, "foo", {}]));
+  Assert.throws(() => Response.fromMsg([1, 2, null, {}]));
+  Response.fromMsg([1, 2, "foo", null]);
+
   run_next_test();
 });