JS: Ignore document.all checks explicitly

Author: asgerf

javascript/ql/src/Expressions/UnneededDefensiveProgramming.ql

@@ -13,19 +13,43 @@
 import javascript
 import semmle.javascript.DefensiveProgramming
 
+/**
+ * Holds if `e` looks like a check for `document.all`, which is an unusual browser object which coerces
+ * to `false` and has typeof `undefined`.
+ */
+predicate isFalsyObjectCheck(LogicalBinaryExpr e) {
+  exists(Variable v |
+    e.getAnOperand().(DefensiveExpressionTest::TypeofUndefinedTest).getOperand() = v.getAnAccess() and
+    e.getAnOperand().(DefensiveExpressionTest::UndefinedComparison).getOperand() = v.getAnAccess()
+  )
+}
+
+/**
+ * Holds if `e` is part of a check for `document.all`.
+ */
+predicate isPartOfFalsyObjectCheck(Expr e) {
+  exists(LogicalBinaryExpr binary |
+    isFalsyObjectCheck(binary) and
+    e = binary.getAnOperand()
+  )
+  or
+  isFalsyObjectCheck(e)
+}
+
 from DefensiveExpressionTest e, boolean cv
 where
+  not isPartOfFalsyObjectCheck(e.asExpr()) and
   e.getTheTestResult() = cv and
   // whitelist
   not (
     // module environment detection
     exists(VarAccess access, string name | name = "exports" or name = "module" |
-      e.asExpr().(Internal::TypeofUndefinedTest).getOperand() = access and
+      e.asExpr().(DefensiveExpressionTest::TypeofUndefinedTest).getOperand() = access and
       access.getName() = name and
       not exists(access.getVariable().getADeclaration())
     )
     or
     // too benign in practice
-    e instanceof Internal::DefensiveInit
+    e instanceof DefensiveExpressionTest::DefensiveInit
   )
 select e, "This guard always evaluates to " + cv + "."

javascript/ql/src/semmle/javascript/DefensiveProgramming.qll

@@ -14,9 +14,9 @@ abstract class DefensiveExpressionTest extends DataFlow::ValueNode {
 }
 
 /**
- * INTERNAL: Do not use directly; use `DefensiveExpressionTest` instead.
+ * Provides classes for specific kinds of defensive programming patterns.
  */
-module Internal {
+module DefensiveExpressionTest {
   /**
    * A defensive truthiness check that may be worth keeping, even if it
    * is strictly speaking useless.
@@ -187,6 +187,13 @@ module Internal {
     override Expr getOperand() { result = operand }
   }
 
+  /**
+   * Comparison against `undefined`, such as `x === undefined`.
+   */
+  class UndefinedComparison extends NullUndefinedComparison {
+    UndefinedComparison() { op2type = TTUndefined() }
+  }
+
   /**
    * An expression that throws an exception if one of its subexpressions evaluates to `null` or `undefined`.
    *
@@ -380,7 +387,7 @@ module Internal {
   /**
    * A test for `undefined` using a `typeof` expression.
    *
-   * Example: `typeof x === undefined'.
+   * Example: `typeof x === "undefined"'.
    */
   class TypeofUndefinedTest extends UndefinedNullTest {
     TypeofTest test;

javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst2.js

@@ -8,3 +8,11 @@
         }
     });
 });
+
+const isFalsyObject = (v) => typeof v === 'undefined' && v !== undefined; // OK
+
+function f(v) {
+    if (typeof v === 'undefined' && v !== undefined) { // OK
+        doSomething(v);
+    }
+}