Merge pull request #804 from esben-semmle/js/sharpen-unneeded-defensive

JS: better handling of nested expressions in js/unneeded-defensive-code

Author: Max Schaefer

change-notes/1.20/analysis-javascript.md

@@ -32,6 +32,8 @@
 | Unused parameter                           | Fewer false-positive results | This rule no longer flags parameters with leading underscore. |
 | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. |
 | Uncontrolled data used in path expression | Fewer false-positive results | This rule now recognizes the Express `root` option, which prevents path traversal. |
+| Unneeded defensive code | More true-positive results, fewer false-positive results. | This rule now recognizes additional defensive code patterns. |
+| Useless conditional | Fewer results | Additional defensive coding patterns are now ignored. |
 | Useless assignment to property. | Fewer false-positive results | This rule now treats assignments with complex right-hand sides correctly. |
 | Unsafe dynamic method access              | Fewer false-positive results | This rule no longer flags concatenated strings as unsafe method names. |
 | Unvalidated dynamic method call           | More true-positive results | This rule now flags concatenated strings as unvalidated method names in more cases. |

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

@@ -53,7 +53,7 @@ module Internal {
    * `polarity` is true iff the inner expression is nested in an even number of negations.
    */
   private Expr stripNotsAndParens(Expr e, boolean polarity) {
-    exists(Expr inner | inner = e.getUnderlyingValue() |
+    exists(Expr inner | inner = e.stripParens() |
       if inner instanceof LogNotExpr
       then result = stripNotsAndParens(inner.(LogNotExpr).getOperand(), polarity.booleanNot())
       else (
@@ -199,11 +199,14 @@ module Internal {
     Expr target;
 
     UndefinedNullCrashUse() {
-      this.(InvokeExpr).getCallee().getUnderlyingValue() = target
-      or
-      this.(PropAccess).getBase().getUnderlyingValue() = target
-      or
-      this.(MethodCallExpr).getReceiver().getUnderlyingValue() = target
+      exists (Expr thrower |
+        stripNotsAndParens(this, _) = thrower |
+        thrower.(InvokeExpr).getCallee().getUnderlyingValue() = target
+        or
+        thrower.(PropAccess).getBase().getUnderlyingValue() = target
+        or
+        thrower.(MethodCallExpr).getReceiver().getUnderlyingValue() = target
+      )
     }
 
     /**
@@ -220,7 +223,8 @@ module Internal {
   private class NonFunctionCallCrashUse extends Expr {
     Expr target;
 
-    NonFunctionCallCrashUse() { this.(InvokeExpr).getCallee().getUnderlyingValue() = target }
+    NonFunctionCallCrashUse() {
+      stripNotsAndParens(this, _).(InvokeExpr).getCallee().getUnderlyingValue() = target }
 
     /**
      * Gets the subexpression that will cause an exception to be thrown if it is not a `function`.
@@ -273,7 +277,6 @@ module Internal {
         guardVar.getVariable() = useVar.getVariable()
       |
         getAGuardedExpr(this.asExpr())
-            .getUnderlyingValue()
             .(UndefinedNullCrashUse)
             .getVulnerableSubexpression() = useVar and
         // exclude types whose truthiness depend on the value
@@ -306,7 +309,6 @@ module Internal {
         guardVar.getVariable() = useVar.getVariable()
       |
         getAGuardedExpr(guard)
-            .getUnderlyingValue()
             .(UndefinedNullCrashUse)
             .getVulnerableSubexpression() = useVar
       )
@@ -375,7 +377,6 @@ module Internal {
         guardVar.getVariable() = useVar.getVariable()
       |
         getAGuardedExpr(guard)
-            .getUnderlyingValue()
             .(NonFunctionCallCrashUse)
             .getVulnerableSubexpression() = useVar
       ) and

javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.expected

@@ -70,3 +70,5 @@
 | tst.js:140:13:140:22 | x === null | This guard always evaluates to false. |
 | tst.js:156:23:156:31 | x != null | This guard always evaluates to true. |
 | tst.js:158:13:158:21 | x != null | This guard always evaluates to true. |
+| tst.js:177:2:177:2 | u | This guard always evaluates to false. |
+| tst.js:178:2:178:2 | u | This guard always evaluates to false. |

javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UselessConditional.expected

@@ -0,0 +1,2 @@
+| tst.js:175:2:175:2 | u | This use of variable 'u' always evaluates to false. |
+| tst.js:176:2:176:2 | u | This use of variable 'u' always evaluates to false. |

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

@@ -171,4 +171,9 @@
     if (typeof window !== "undefined" && window.document);
     if (typeof module !== "undefined" && module.exports);
     if (typeof global !== "undefined" && global.process);
+
+	u && (f(), u.p);
+	u && (u.p, f()); // technically not OK, but it seems like an unlikely pattern
+	u && !u.p; // NOT OK
+	u && !u(); // NOT OK
 });