Merge pull request #395 from esben-semmle/js/useless-defensive-code

JS: add query: js/useless-defensive-code

Author: Max Schaefer

change-notes/1.19/analysis-javascript.md

@@ -23,6 +23,7 @@
 | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. |
 | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
 | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. |
+| Unneeded defensive code | correctness, external/cwe/cwe-570, external/cwe/cwe-571 | Highlights locations where defensive code is not needed. Results are shown on LGTM by default. |
 | Useless assignment to property | maintainability | Highlights property assignments whose value is always overwritten. Results are shown on LGTM by default. |
 | User-controlled data in file | security, external/cwe/cwe-912 | Highlights locations where user-controlled data is written to a file. Results are not shown on LGTM by default. |
 

javascript/config/suites/javascript/maintainability-more

@@ -6,6 +6,7 @@
 + semmlecode-javascript-queries/Declarations/DuplicateVarDecl.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/UnusedParameter.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/UnusedVariable.ql: /Maintainability/Declarations
++ semmlecode-javascript-queries/Expressions/UselessDefensiveProgramming.ql: /Maintainability/Expressions
 + semmlecode-javascript-queries/LanguageFeatures/ArgumentsCallerCallee.ql: /Maintainability/Language Features
 + semmlecode-javascript-queries/LanguageFeatures/ConditionalComments.ql: /Maintainability/Language Features
 + semmlecode-javascript-queries/LanguageFeatures/Eval.ql: /Maintainability/Language Features

javascript/ql/src/Expressions/HeterogeneousComparison.ql

@@ -15,6 +15,7 @@
 
 import javascript
 private import semmle.javascript.dataflow.InferredTypes
+private import semmle.javascript.DefensiveProgramming
 
 /**
  * Holds if `left` and `right` are the left and right operands, respectively, of `nd`, which is
@@ -198,6 +199,7 @@ from ASTNode cmp,
      int leftTypeCount, int rightTypeCount ,
      string leftTypeDescription, string rightTypeDescription
 where isHeterogeneousComparison(cmp, left, right, leftTypes, rightTypes) and
+      not exists (cmp.(Expr).flow().(DefensiveExpressionTest).getTheTestResult()) and
       not whitelist(left.asExpr()) and
       not whitelist(right.asExpr()) and
       leftExprDescription = capitalize(getDescription(left.asExpr(), "this expression")) and

javascript/ql/src/Expressions/UnneededDefensiveProgramming.qhelp

@@ -0,0 +1,91 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+	<overview>
+		<p>
+
+			Defensive code can prevent unforeseen circumstances from
+			causing fatal program behaviors.
+
+			A common defensive code pattern is to guard
+			against dereferencing the values <code>null</code> or
+			<code>undefined</code>.
+
+			However, if the situation that some defensive code guards
+			against never can occur, then the defensive code serves no purpose and
+			can safely be removed.
+
+		</p>
+
+	</overview>
+	<recommendation>
+
+		<p>
+
+			Examine the surrounding code to determine if the defensive
+			code is worth keeping despite providing no practical use. If it is no
+			longer needed, remove it.
+
+		</p>
+
+	</recommendation>
+	<example>
+
+		<p>
+
+			The following example shows a <code>cleanupLater</code>
+			function that asynchronously will perform a cleanup task after some
+			delay.  When the cleanup task completes, the function invokes the
+			provided callback parameter <code>cb</code>.
+
+			To prevent a crash by invoking <code>cb</code> when it
+			has the value <code>undefined</code>, defensive code guards
+			the invocation by checking if <code>cb</code> is truthy.
+
+		</p>
+
+		<sample src="examples/UnneededDefensiveProgramming1_bad.js" />
+
+		<p>
+
+			However, the <code>cleanupLater</code> function is always
+			invoked with a callback argument, so the defensive code condition
+			always holds, and it is therefore not
+			required. The function can therefore be simplified to:
+
+		</p>
+
+		<sample src="examples/UnneededDefensiveProgramming1_good.js" />
+
+		<p>
+
+			Guarding against the same situation multiple times is
+			another example of defensive code that provides no practical use. The
+			example below shows a function that assigns a value to a property of
+			an object, where defensive code ensures that the assigned value is not
+			<code>undefined</code> or <code>null</code>.
+
+		</p>
+
+		<sample src="examples/UnneededDefensiveProgramming2_bad.js" />
+
+		<p>
+
+			However, due to coercion rules, <code>v ==
+			undefined</code> holds for both the situation where <code>v</code>
+			is<code>undefined</code> and the situation where <code>v</code>
+			is<code>null</code>, so the <code>v == null</code>
+			guard serves no purpose, and can be removed:
+
+		</p>
+
+		<sample src="examples/UnneededDefensiveProgramming2_good.js" />
+
+	</example>
+	<references>
+
+		<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Defensive_programming">Defensive programming</a>.</li>
+
+	</references>
+</qhelp>

javascript/ql/src/Expressions/UnneededDefensiveProgramming.ql

@@ -0,0 +1,31 @@
+/**
+ * @name Unneeded defensive code
+ * @description Defensive code that guards against a situation that never happens is not needed.
+ * @kind problem
+ * @problem.severity recommendation
+ * @id js/unneeded-defensive-code
+ * @tags correctness
+ *       external/cwe/cwe-570
+ *       external/cwe/cwe-571
+ * @precision very-high
+ */
+
+import javascript
+import semmle.javascript.DefensiveProgramming
+
+from DefensiveExpressionTest e, boolean cv
+where 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
+          access.getName() = name and
+          not exists (access.getVariable().getADeclaration())
+        )
+        or
+        // too benign in practice
+        e instanceof Internal::DefensiveInit
+      )
+select e, "This guard always evaluates to " + cv + "."

javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming1_bad.js

@@ -0,0 +1,10 @@
+function cleanupLater(delay, cb) {
+    setTimeout(function() {
+        cleanup();
+        if (cb) { // BAD: useless check, `cb` is always truthy
+            cb();
+        }
+    }, delay)
+}
+
+cleanupLater(1000, function(){console.log("Cleanup done")});

javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming1_good.js

@@ -0,0 +1,9 @@
+function cleanupLater(delay, cb) {
+    setTimeout(function() {
+        cleanupNow();
+        // GOOD: no need to guard the invocation
+        cb();
+    }, delay)
+}
+
+cleanupLater(function(){console.log("Cleanup done")});

javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming2_bad.js

@@ -0,0 +1,5 @@
+function setSafeStringProp(o, prop, v) {
+    // BAD: `v == null` is useless
+    var safe = v == undefined || v == null? '': v;
+    o[prop] = safe;
+}

javascript/ql/src/Expressions/examples/UnneededDefensiveProgramming2_good.js

@@ -0,0 +1,5 @@
+function setSafeStringProp(o, prop, v) {
+    // GOOD: `v == undefined` handles both `undefined` and `null`
+    var safe = v == undefined? '': v;
+    o[prop] = safe;
+}

javascript/ql/src/Statements/UselessConditional.ql

@@ -15,29 +15,7 @@
 import javascript
 import semmle.javascript.RestrictedLocations
 import semmle.javascript.dataflow.Refinements
-
-/**
- * Holds if `va` is a defensive truthiness check that may be worth keeping, even if it
- * is strictly speaking useless.
- *
- * We currently recognize three patterns:
- *
- *   - the first `x` in `x || (x = e)`
- *   - the second `x` in `x = (x || e)`
- *   - the second `x` in `var x = x || e`
- */
-predicate isDefensiveInit(VarAccess va) {
-  exists (LogOrExpr o, VarRef va2 |
-    va = o.getLeftOperand().getUnderlyingReference() and va2.getVariable() = va.getVariable() |
-    exists (AssignExpr assgn | va2 = assgn.getTarget() |
-      assgn = o.getRightOperand().stripParens() or
-      o = assgn.getRhs().getUnderlyingValue()
-    ) or
-    exists (VariableDeclarator vd | va2 = vd.getBindingPattern() |
-      o = vd.getInit().getUnderlyingValue()
-    )
-  )
-}
+import semmle.javascript.DefensiveProgramming
 
 /**
  * Holds if variable `v` looks like a symbolic constant, that is, it is assigned
@@ -95,6 +73,21 @@ predicate isConstantBooleanReturnValue(Expr e) {
   isConstantBooleanReturnValue(e.(LogNotExpr).getOperand())
 }
 
+private Expr maybeStripLogNot(Expr e) {
+  result = maybeStripLogNot(e.(LogNotExpr).getOperand()) or
+  result = e
+}
+
+/**
+ * Holds if `e` is a defensive expression with a fixed outcome.
+ */
+predicate isConstantDefensive(Expr e) {
+  exists(DefensiveExpressionTest defensive |
+    maybeStripLogNot(defensive.asExpr()) = e and
+    exists(defensive.getTheTestResult())
+  )
+}
+
 /**
  * Holds if `e` is an expression that should not be flagged as a useless condition.
  *
@@ -109,7 +102,7 @@ predicate isConstantBooleanReturnValue(Expr e) {
 predicate whitelist(Expr e) {
   isConstant(e) or
   isConstant(e.(LogNotExpr).getOperand()) or
-  isDefensiveInit(e) or
+  isConstantDefensive(e) or // flagged by js/useless-defensive-code
   isInitialParameterUse(e) or
   isConstantBooleanReturnValue(e)
 }