Merge pull request #946 from esben-semmle/js/captured-nodes-query-and-type-inference-1

JS: Captured Nodes, type inference + a query

Author: Max Schaefer

change-notes/1.20/analysis-javascript.md

@@ -31,7 +31,7 @@
 | Incomplete URL substring sanitization | correctness, security, external/cwe/cwe-020 | Highlights URL sanitizers that are likely to be incomplete, indicating a violation of [CWE-020](https://cwe.mitre.org/data/definitions/20.html). Results shown on LGTM by default. |
 | Incorrect suffix check (`js/incorrect-suffix-check`) | correctness, security, external/cwe/cwe-020 | Highlights error-prone suffix checks based on `indexOf`, indicating a potential violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. |
 | Loop iteration skipped due to shifting (`js/loop-iteration-skipped-due-to-shifting`) | correctness | Highlights code that removes an element from an array while iterating over it, causing the loop to skip over some elements. Results are shown on LGTM by default. |
-| Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | Additional ways that class methods can be bound are recognized. |
+| Unused property (`js/unused-property`) | maintainability | Highlights properties that are unused. Results are shown on LGTM by default. |
 | Useless comparison test (`js/useless-comparison-test`) | correctness | Highlights code that is unreachable due to a numeric comparison that is always true or always false. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
@@ -44,9 +44,10 @@
 | Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. |
 | Reflected cross-site scripting             | Fewer false-positive results. | This rule now recognizes custom sanitizers. |
 | Stored cross-site scripting                | Fewer false-positive results. | This rule now recognizes custom sanitizers. |
+| Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | Additional ways that class methods can be bound are recognized. |
 | Uncontrolled data used in network request  | More results                 | This rule now recognizes host values that are vulnerable to injection. |
 | 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. |
+| Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, no longer flags variables with leading underscore, and no longer flags variables in dead code. |
 | 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. |

javascript/config/suites/javascript/maintainability-more

@@ -5,6 +5,7 @@
 + semmlecode-javascript-queries/Declarations/DeadStoreOfProperty.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/DuplicateVarDecl.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/UnusedParameter.ql: /Maintainability/Declarations
++ semmlecode-javascript-queries/Declarations/UnusedProperty.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/UnusedVariable.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Expressions/UnneededDefensiveProgramming.ql: /Maintainability/Expressions
 + semmlecode-javascript-queries/LanguageFeatures/ArgumentsCallerCallee.ql: /Maintainability/Language Features

javascript/ql/src/Declarations/UnusedParameter.qll

@@ -1,5 +1,5 @@
 /**
- * This library contains the majority of the 'js/unused-parameter' query implementation.
+ * Provides classes and predicates for the 'js/unused-parameter' query.
  *
  * In order to suppress alerts that are similar to the 'js/unused-parameter' alerts,
  * `isAnAccidentallyUnusedParameter` should be used since it holds iff that alert is active.

javascript/ql/src/Declarations/UnusedProperty.qhelp

@@ -0,0 +1,34 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+	<overview>
+		<p>
+			Unused object properties make code harder to maintain and use. Clients that are unaware that a
+			property is unused may perform nontrivial computations to compute a value that is ultimately
+			unused.
+		</p>
+
+	</overview>
+	<recommendation>
+		<p>Remove the unused property.</p>
+
+	</recommendation>
+	<example>
+
+		<p>
+			In this code, the function <code>f</code> initializes a property <code>prop_a</code> with a
+			call to the function <code>expensiveComputation</code>, but later on this property is never read.
+			Removing <code>prop</code> would improve code quality and performance.
+		</p>
+
+		<sample src="examples/UnusedProperty.js" />
+
+	</example>
+	<references>
+
+		<li>Coding Horror: <a href="http://blog.codinghorror.com/code-smells/">Code Smells</a>.</li>
+
+
+	</references>
+</qhelp>

javascript/ql/src/Declarations/UnusedProperty.ql

@@ -0,0 +1,78 @@
+/**
+ * @name Unused property
+ * @description Unused properties may be a symptom of a bug and should be examined carefully.
+ * @kind problem
+ * @problem.severity recommendation
+ * @id js/unused-property
+ * @tags maintainability
+ * @precision high
+ */
+
+import javascript
+import semmle.javascript.dataflow.LocalObjects
+import UnusedVariable
+import UnusedParameter
+import Expressions.ExprHasNoEffect
+
+predicate hasUnknownPropertyRead(LocalObject obj) {
+  // dynamic reads
+  exists(DataFlow::PropRead r | obj.getAPropertyRead() = r | not exists(r.getPropertyName()))
+  or
+  // reflective reads
+  obj.flowsToExpr(any(EnhancedForLoop l).getIterationDomain())
+  or
+  obj.flowsToExpr(any(InExpr l).getRightOperand())
+  or
+  obj.flowsToExpr(any(SpreadElement e).getOperand())
+  or
+  exists(obj.getAPropertyRead("hasOwnProperty"))
+  or
+  exists(obj.getAPropertyRead("propertyIsEnumerable"))
+}
+
+/**
+ * Holds if `obj` flows to an expression that must have a specific type.
+ */
+predicate flowsToTypeRestrictedExpression(LocalObject obj) {
+  exists (Expr restricted, TypeExpr type |
+    obj.flowsToExpr(restricted) and
+    not type.isAny() |
+    exists (TypeAssertion assertion |
+      type = assertion.getTypeAnnotation() and
+      restricted = assertion.getExpression()
+    )
+    or
+    exists (BindingPattern v |
+      type = v.getTypeAnnotation() and
+      restricted = v.getAVariable().getAnAssignedExpr()
+    )
+    // no need to reason about writes to typed fields, captured nodes do not reach them
+  )
+}
+
+from DataFlow::PropWrite write, LocalObject obj, string name
+where
+  write = obj.getAPropertyWrite(name) and
+  not exists(obj.getAPropertyRead(name)) and
+  // `obj` is the only base object for the write: it is not spurious
+  not write.getBase().analyze().getAValue() != obj.analyze().getAValue() and
+  not hasUnknownPropertyRead(obj) and
+  // avoid reporting if the definition is unreachable
+  write.getAstNode().getFirstControlFlowNode().getBasicBlock() instanceof ReachableBasicBlock and
+  // avoid implicitly read properties
+  not (
+    name = "toString" or
+    name = "valueOf" or
+    name.matches("@@%") // @@iterator, for example
+  ) and
+  // avoid flagging properties that a type system requires
+  not flowsToTypeRestrictedExpression(obj) and
+  // flagged by js/unused-local-variable
+  not exists(UnusedLocal l | l.getAnAssignedExpr().getUnderlyingValue().flow() = obj) and
+  // flagged by js/unused-parameter
+  not exists(Parameter p | isAnAccidentallyUnusedParameter(p) |
+    p.getDefault().getUnderlyingValue().flow() = obj
+  ) and
+  // flagged by js/useless-expression
+  not inVoidContext(obj.asExpr())
+select write, "Unused property " + name + "."

javascript/ql/src/Declarations/UnusedVariable.ql

@@ -10,23 +10,7 @@
  */
 
 import javascript
-
-/**
- * A local variable that is neither used nor exported, and is not a parameter
- * or a function name.
- */
-class UnusedLocal extends LocalVariable {
-  UnusedLocal() {
-    not exists(getAnAccess()) and
-    not exists(Parameter p | this = p.getAVariable()) and
-    not exists(FunctionExpr fe | this = fe.getVariable()) and
-    not exists(ClassExpr ce | this = ce.getVariable()) and
-    not exists(ExportDeclaration ed | ed.exportsAs(this, _)) and
-    not exists(LocalVarTypeAccess type | type.getVariable() = this) and
-    // common convention: variables with leading underscore are intentionally unused
-    getName().charAt(0) != "_"
-  }
-}
+import UnusedVariable
 
 /**
  * Holds if `v` is mentioned in a JSDoc comment in the same file, and that file
@@ -206,6 +190,10 @@ predicate unusedImports(ImportVarDeclProvider provider, string msg) {
 
 from ASTNode sel, string msg
 where
-  unusedNonImports(sel, msg) or
-  unusedImports(sel, msg)
+  (
+    unusedNonImports(sel, msg) or
+    unusedImports(sel, msg)
+  ) and
+  // avoid reporting if the definition is unreachable
+  sel.getFirstControlFlowNode().getBasicBlock() instanceof ReachableBasicBlock
 select sel, msg

javascript/ql/src/Declarations/UnusedVariable.qll

@@ -0,0 +1,22 @@
+/**
+ * Provides classes and predicates for the 'js/unused-local-variable' query.
+ */
+
+import javascript
+
+/**
+ * A local variable that is neither used nor exported, and is not a parameter
+ * or a function name.
+ */
+class UnusedLocal extends LocalVariable {
+  UnusedLocal() {
+    not exists(getAnAccess()) and
+    not exists(Parameter p | this = p.getAVariable()) and
+    not exists(FunctionExpr fe | this = fe.getVariable()) and
+    not exists(ClassExpr ce | this = ce.getVariable()) and
+    not exists(ExportDeclaration ed | ed.exportsAs(this, _)) and
+    not exists(LocalVarTypeAccess type | type.getVariable() = this) and
+    // common convention: variables with leading underscore are intentionally unused
+    getName().charAt(0) != "_"
+  }
+}

javascript/ql/src/Declarations/examples/UnusedProperty.js

@@ -0,0 +1,8 @@
+function f() {
+	var o = {
+		prop_a: expensiveComputation(),
+		prop_b: anotherComputation()
+	};
+
+	return o.prop_b;
+}

javascript/ql/src/Expressions/ExprHasNoEffect.ql

@@ -16,40 +16,7 @@ import javascript
 import DOMProperties
 import semmle.javascript.frameworks.xUnit
 import semmle.javascript.RestrictedLocations
-
-/**
- * Holds if `e` appears in a syntactic context where its value is discarded.
- */
-predicate inVoidContext(Expr e) {
-  exists(ExprStmt parent |
-    // e is a toplevel expression in an expression statement
-    parent = e.getParent() and
-    // but it isn't an HTML attribute or a configuration object
-    not exists(TopLevel tl | tl = parent.getParent() |
-      tl instanceof CodeInAttribute
-      or
-      // if the toplevel in its entirety is of the form `({ ... })`,
-      // it is probably a configuration object (e.g., a require.js build configuration)
-      tl.getNumChildStmt() = 1 and e.stripParens() instanceof ObjectExpr
-    )
-  )
-  or
-  exists(SeqExpr seq, int i, int n |
-    e = seq.getOperand(i) and
-    n = seq.getNumOperands()
-  |
-    i < n - 1 or inVoidContext(seq)
-  )
-  or
-  exists(ForStmt stmt | e = stmt.getUpdate())
-  or
-  exists(ForStmt stmt | e = stmt.getInit() |
-    // Allow the pattern `for(i; i < 10; i++)`
-    not e instanceof VarAccess
-  )
-  or
-  exists(LogicalBinaryExpr logical | e = logical.getRightOperand() and inVoidContext(logical))
-}
+import ExprHasNoEffect
 
 /**
  * Holds if `e` is of the form `x;` or `e.p;` and has a JSDoc comment containing a tag.

javascript/ql/src/Expressions/ExprHasNoEffect.qll

@@ -0,0 +1,39 @@
+/**
+ * Provides classes and predicates for the 'js/useless-expression' query.
+ */
+
+import javascript
+
+/**
+ * Holds if `e` appears in a syntactic context where its value is discarded.
+ */
+predicate inVoidContext(Expr e) {
+  exists(ExprStmt parent |
+    // e is a toplevel expression in an expression statement
+    parent = e.getParent() and
+    // but it isn't an HTML attribute or a configuration object
+    not exists(TopLevel tl | tl = parent.getParent() |
+      tl instanceof CodeInAttribute
+      or
+      // if the toplevel in its entirety is of the form `({ ... })`,
+      // it is probably a configuration object (e.g., a require.js build configuration)
+      tl.getNumChildStmt() = 1 and e.stripParens() instanceof ObjectExpr
+    )
+  )
+  or
+  exists(SeqExpr seq, int i, int n |
+    e = seq.getOperand(i) and
+    n = seq.getNumOperands()
+  |
+    i < n - 1 or inVoidContext(seq)
+  )
+  or
+  exists(ForStmt stmt | e = stmt.getUpdate())
+  or
+  exists(ForStmt stmt | e = stmt.getInit() |
+    // Allow the pattern `for(i; i < 10; i++)`
+    not e instanceof VarAccess
+  )
+  or
+  exists(LogicalBinaryExpr logical | e = logical.getRightOperand() and inVoidContext(logical))
+}