From 5d54fbc44c8e8297376646e528ea528e9fc8b3e3 Mon Sep 17 00:00:00 2001
From: Michael Kelly <mkelly@mozilla.com>
Date: Thu, 22 Jun 2017 18:00:16 -0700
Subject: [PATCH] Add tests for failure modes for keys and intersect.

---
 docs/user/filter_expressions.rst              |  6 ++-
 recipe-client-addon/lib/FilterExpressions.jsm | 17 +++++--
 .../test/browser/browser_FilterExpressions.js | 47 ++++++++++++++++++-
 3 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/docs/user/filter_expressions.rst b/docs/user/filter_expressions.rst
index 90ad2e8b..2cc8e329 100644
--- a/docs/user/filter_expressions.rst
+++ b/docs/user/filter_expressions.rst
@@ -279,7 +279,8 @@ parameters correspond to the operands.
 .. js:function:: intersect(list1, list2)
 
    Returns an array of all values in ``list1`` that are also present in
-   ``list2``. Values are compared using strict equality.
+   ``list2``. Values are compared using strict equality. If ``list1`` or
+   ``list2`` are not arrays, the returned value is ``undefined``.
 
    :param list1:
       The array to the left of the operator.
@@ -383,7 +384,8 @@ function is the value being transformed.
 .. js:function:: keys(obj)
 
    Return an array of the given object's own keys (specifically, its enumerable
-   properties). Equivalent to `Object.keys`_.
+   properties). Similar to `Object.keys`_, except that if given a non-object,
+   ``keys`` will return ``undefined``.
 
    :param obj:
       Object to get the keys for.
diff --git a/recipe-client-addon/lib/FilterExpressions.jsm b/recipe-client-addon/lib/FilterExpressions.jsm
index 7473f003..f7432313 100644
--- a/recipe-client-addon/lib/FilterExpressions.jsm
+++ b/recipe-client-addon/lib/FilterExpressions.jsm
@@ -46,20 +46,29 @@ this.FilterExpressions = {
 
 /**
  * Return an array of the given object's own keys (specifically, its enumerable
- * properties).
+ * properties), or undefined if the argument isn't an object.
  * @param {Object} obj
- * @return {Array[String]}
+ * @return {Array[String]|undefined}
  */
 function keys(obj) {
+  if (typeof obj !== "object" || obj === null) {
+    return undefined;
+  }
+
   return Object.keys(obj);
 }
 
 /**
- * Find all the values that are present in both lists.
+ * Find all the values that are present in both lists. Returns undefined if
+ * the arguments are not both Arrays.
  * @param {Array} listA
  * @param {Array} listB
- * @return {Array}
+ * @return {Array|undefined}
  */
 function operatorIntersect(listA, listB) {
+  if (!Array.isArray(listA) || !Array.isArray(listB)) {
+    return undefined;
+  }
+
   return listA.filter(item => listB.includes(item));
 }
diff --git a/recipe-client-addon/test/browser/browser_FilterExpressions.js b/recipe-client-addon/test/browser/browser_FilterExpressions.js
index 471d9c3e..56e3a299 100644
--- a/recipe-client-addon/test/browser/browser_FilterExpressions.js
+++ b/recipe-client-addon/test/browser/browser_FilterExpressions.js
@@ -105,7 +105,7 @@ add_task(async function testKeys() {
   );
 
   // Test an object in the context
-  const context = {ctxObject: {baz: "string", biff: NaN}};
+  let context = {ctxObject: {baz: "string", biff: NaN}};
   val = await FilterExpressions.eval("ctxObject|keys", context);
 
   Assert.deepEqual(
@@ -113,6 +113,34 @@ add_task(async function testKeys() {
     new Set(["baz", "biff"]),
     "keys returns the keys from an object in the context",
   );
+
+  // Test that values from the prototype are not included
+  context = {ctxObject: Object.create({fooProto: 7})};
+  context.ctxObject.baz = 8;
+  context.ctxObject.biff = 5;
+  is(
+    await FilterExpressions.eval("ctxObject.fooProto", context),
+    7,
+    "Prototype properties are accessible via property access",
+  );
+  val = await FilterExpressions.eval("ctxObject|keys", context);
+  Assert.deepEqual(
+    new Set(val),
+    new Set(["baz", "biff"]),
+    "keys does not return properties from the object's prototype chain",
+  );
+
+  // Return undefined for non-objects
+  is(
+    await FilterExpressions.eval("ctxObject|keys", {ctxObject: 45}),
+    undefined,
+    "keys returns undefined for numbers",
+  );
+  is(
+    await FilterExpressions.eval("ctxObject|keys", {ctxObject: null}),
+    undefined,
+    "keys returns undefined for null",
+  );
 });
 
 // intersect tests
@@ -140,4 +168,21 @@ add_task(async function testIntersect() {
     new Set(["string"]),
     "intersect can compare strings",
   );
+
+  // Return undefined when intersecting things that aren't lists.
+  is(
+    await FilterExpressions.eval("5 intersect 7"),
+    undefined,
+    "intersect returns undefined for numbers",
+  );
+  is(
+    await FilterExpressions.eval("val intersect other", {val: null, other: null}),
+    undefined,
+    "intersect returns undefined for null",
+  );
+  is(
+    await FilterExpressions.eval("5 intersect [1, 2, 5]"),
+    undefined,
+    "intersect returns undefined if only one operand is a list",
+  );
 });