Merge pull request github#3627 from asger-semmle/js/unneeded-defensive-return

Approved by erik-krogh

Author: semmle-qlci

change-notes/1.25/analysis-javascript.md

@@ -63,6 +63,7 @@
 | Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
 | Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
 | Unknown directive (`js/unknown-directive`) | Fewer results | This query no longer flags directives generated by the Babel compiler. |
+| Unneeded defensive code (`js/unneeded-defensive-code`) | Fewer false-positive results | This query now recognizes checks meant to handle the `document.all` object. |
 | Unused property (`js/unused-property`) | Fewer results | This query no longer flags properties of objects that are operands of `yield` expressions. |
 | Zip Slip (`js/zipslip`) | More results | This query now recognizes additional vulnerabilities. |
 

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/UnneededDefensiveProgramming.expected

@@ -1,4 +1,6 @@
 | module-environment-detection.js:23:8:23:36 | typeof  ... efined' | This guard always evaluates to true. |
+| regression.js:9:9:9:12 | date | This guard always evaluates to true. |
+| regression.js:13:25:13:40 | obj != undefined | This guard always evaluates to true. |
 | tst2.js:4:12:4:35 | typeof  ... efined" | This guard always evaluates to true. |
 | tst.js:18:5:18:5 | u | This guard always evaluates to false. |
 | tst.js:19:5:19:5 | n | This guard always evaluates to false. |

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

@@ -0,0 +1,15 @@
+function getDate() {
+	var date;
+	if (something()) {
+		date = new Date();
+	} else {
+		return null;
+	}
+	console.log(date);
+	return date && date.getTime(); // NOT OK
+}
+
+function isNotNullOrString(obj) {
+  return obj != null && obj != undefined && // NOT OK
+      typeof obj != 'string'; 
+}

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);
+    }
+}