Merge pull request #727 from xiemaisi/js/restructure-sourcenode

Approved by esben-semmle

Author: semmle-qlci

change-notes/1.20/analysis-javascript.md

@@ -33,3 +33,6 @@
 | Uncontrolled data used in path expression | Fewer false-positive results | This rule now recognizes the Express `root` option, which prevents path traversal. |
 
 ## Changes to QL libraries
+
+* `DataFlow::SourceNode` is no longer an abstract class; to add new source nodes, extend `DataFlow::SourceNode::Range` instead.
+* Subclasses of `DataFlow::PropRead` are no longer automatically made source nodes; you now need to additionally define a corresponding subclass of `DataFlow::SourceNode::Range` to achieve this.

javascript/ql/src/semmle/javascript/EmailClients.qll

@@ -3,7 +3,7 @@ import javascript
 /**
  * An operation that sends an email.
  */
-abstract class EmailSender extends DataFlow::DefaultSourceNode {
+abstract class EmailSender extends DataFlow::SourceNode {
   /**
    * Gets a data flow node holding the plaintext version of the email body.
    */

javascript/ql/src/semmle/javascript/StandardLibrary.qll

@@ -76,7 +76,7 @@ private class AnalyzedThisInArrayIterationFunction extends AnalyzedNode, DataFlo
 /**
  * A definition of a `Promise` object.
  */
-abstract class PromiseDefinition extends DataFlow::DefaultSourceNode {
+abstract class PromiseDefinition extends DataFlow::SourceNode {
   /** Gets the executor function of this promise object. */
   abstract DataFlow::FunctionNode getExecutor();
 

javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll

@@ -515,6 +515,20 @@ module DataFlow {
    */
   abstract class PropRead extends PropRef, SourceNode { }
 
+  /**
+   * A property read, considered as a source node.
+   *
+   * Note that we cannot simplify the characteristic predicate to `this instanceof PropRead`,
+   * since `PropRead` is itself a subclass of `SourceNode`.
+   */
+  private class PropReadAsSourceNode extends SourceNode::Range {
+    PropReadAsSourceNode() {
+      this = TPropNode(any(PropertyPattern p)) or
+      this instanceof RestPatternNode or
+      this instanceof ElementPatternNode
+    }
+  }
+
   /**
    * A property access in rvalue position.
    */

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

@@ -7,7 +7,7 @@
 import javascript
 
 /** A data flow node corresponding to a parameter. */
-class ParameterNode extends DataFlow::DefaultSourceNode {
+class ParameterNode extends DataFlow::SourceNode {
   Parameter p;
 
   ParameterNode() { DataFlow::parameterNode(this, p) }
@@ -20,7 +20,7 @@ class ParameterNode extends DataFlow::DefaultSourceNode {
 }
 
 /** A data flow node corresponding to a function invocation (with or without `new`). */
-class InvokeNode extends DataFlow::DefaultSourceNode {
+class InvokeNode extends DataFlow::SourceNode {
   DataFlow::Impl::InvokeNodeDef impl;
 
   InvokeNode() { this = impl }
@@ -168,7 +168,7 @@ class MethodCallNode extends CallNode {
 class NewNode extends InvokeNode { override DataFlow::Impl::NewNodeDef impl; }
 
 /** A data flow node corresponding to the `this` parameter in a function or `this` at the top-level. */
-class ThisNode extends DataFlow::Node, DataFlow::DefaultSourceNode {
+class ThisNode extends DataFlow::Node, DataFlow::SourceNode {
   ThisNode() { DataFlow::thisNode(this, _) }
 
   /**
@@ -202,7 +202,7 @@ class ThisNode extends DataFlow::Node, DataFlow::DefaultSourceNode {
 }
 
 /** A data flow node corresponding to a global variable access. */
-class GlobalVarRefNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
+class GlobalVarRefNode extends DataFlow::ValueNode, DataFlow::SourceNode {
   override GlobalVarAccess astNode;
 
   /** Gets the name of the global variable. */
@@ -216,7 +216,10 @@ class GlobalVarRefNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode
  */
 DataFlow::SourceNode globalObjectRef() {
   // top-level `this`
-  exists(ThisNode globalThis | result = globalThis | not exists(globalThis.getBinder()))
+  exists(StmtContainer sc |
+    sc = result.(ThisNode).getBindingContainer() and
+    not sc instanceof Function
+  )
   or
   // DOM
   result = globalVarRef("window")
@@ -244,7 +247,7 @@ DataFlow::SourceNode globalVarRef(string name) {
 }
 
 /** A data flow node corresponding to a function definition. */
-class FunctionNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
+class FunctionNode extends DataFlow::ValueNode, DataFlow::SourceNode {
   override Function astNode;
 
   /** Gets the `i`th parameter of this function. */
@@ -287,12 +290,12 @@ class FunctionNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
 }
 
 /** A data flow node corresponding to an object literal expression. */
-class ObjectLiteralNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
+class ObjectLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode {
   override ObjectExpr astNode;
 }
 
 /** A data flow node corresponding to an array literal expression. */
-class ArrayLiteralNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
+class ArrayLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode {
   override ArrayExpr astNode;
 
   /** Gets the `i`th element of this array literal. */
@@ -323,7 +326,7 @@ class ArrayConstructorInvokeNode extends DataFlow::InvokeNode {
  * A data flow node corresponding to the creation or a new array, either through an array literal
  * or an invocation of the `Array` constructor.
  */
-class ArrayCreationNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
+class ArrayCreationNode extends DataFlow::ValueNode, DataFlow::SourceNode {
   ArrayCreationNode() {
     this instanceof ArrayLiteralNode or
     this instanceof ArrayConstructorInvokeNode
@@ -346,7 +349,7 @@ class ArrayCreationNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode
  * For compatibility with old transpilers, we treat `import * from '...'`
  * as a default import as well.
  */
-class ModuleImportNode extends DataFlow::DefaultSourceNode {
+class ModuleImportNode extends DataFlow::SourceNode {
   string path;
 
   ModuleImportNode() {

javascript/ql/src/semmle/javascript/dataflow/Sources.qll

@@ -9,15 +9,15 @@
 import javascript
 
 /**
- * A source node for local data flow, that is, a node for which local
- * data flow cannot provide any information about its inputs.
+ * A source node for local data flow, that is, a node from which local data flow is tracked.
  *
- * By default, functions, object and array expressions and JSX nodes
- * are considered sources, as well as expressions that have non-local
- * flow (such as calls and property accesses). Additional sources
- * can be modelled by extending this class with additional subclasses.
+ * Examples include function parameters, imports and property accesses; see
+ * `DataFlow::SourceNode::DefaultRange` for details. You can introduce new kinds of
+ * source nodes by defining new subclasses of `DataFlow::SourceNode::Range`.
  */
-abstract class SourceNode extends DataFlow::Node {
+class SourceNode extends DataFlow::Node {
+  SourceNode() { this instanceof SourceNode::Range }
+
   /**
    * Holds if this node flows into `sink` in zero or more local (that is,
    * intra-procedural) steps.
@@ -167,45 +167,62 @@ abstract class SourceNode extends DataFlow::Node {
   }
 }
 
-/**
- * A data flow node that is considered a source node by default.
- *
- * Currently, the following nodes are source nodes:
- *   - import specifiers
- *   - non-destructuring function parameters
- *   - property accesses
- *   - function invocations
- *   - `this` expressions
- *   - global variable accesses
- *   - function definitions
- *   - class definitions
- *   - object expressions
- *   - array expressions
- *   - JSX literals.
- */
-class DefaultSourceNode extends SourceNode {
-  DefaultSourceNode() {
-    exists(ASTNode astNode | this = DataFlow::valueNode(astNode) |
-      astNode instanceof PropAccess or
-      astNode instanceof Function or
-      astNode instanceof ClassDefinition or
-      astNode instanceof ObjectExpr or
-      astNode instanceof ArrayExpr or
-      astNode instanceof JSXNode or
-      astNode instanceof GlobalVarAccess or
-      astNode instanceof ExternalModuleReference
-    )
-    or
-    exists(SsaExplicitDefinition ssa, VarDef def |
-      this = DataFlow::ssaDefinitionNode(ssa) and def = ssa.getDef()
-    |
-      def instanceof ImportSpecifier
-    )
-    or
-    DataFlow::parameterNode(this, _)
-    or
-    this instanceof DataFlow::Impl::InvokeNodeDef
-    or
-    DataFlow::thisNode(this, _)
+module SourceNode {
+  /**
+   * A data flow node that should be considered a source node.
+   *
+   * Subclass this class to introduce new kinds of source nodes. If you want to refine
+   * the definition of existing source nodes, subclass `DataFlow::SourceNode` instead.
+   */
+  cached
+  abstract class Range extends DataFlow::Node { }
+
+  /**
+   * A data flow node that is considered a source node by default.
+   *
+   * Currently, the following nodes are source nodes:
+   *   - import specifiers
+   *   - function parameters
+   *   - `this` nodes
+   *   - property accesses
+   *   - function invocations
+   *   - global variable accesses
+   *   - function definitions
+   *   - class definitions
+   *   - object expressions
+   *   - array expressions
+   *   - JSX literals
+   *
+   * This class is for internal use only and should not normally be used directly.
+   */
+  class DefaultRange extends Range {
+    DefaultRange() {
+      exists(ASTNode astNode | this = DataFlow::valueNode(astNode) |
+        astNode instanceof PropAccess or
+        astNode instanceof Function or
+        astNode instanceof ClassDefinition or
+        astNode instanceof ObjectExpr or
+        astNode instanceof ArrayExpr or
+        astNode instanceof JSXNode or
+        astNode instanceof GlobalVarAccess or
+        astNode instanceof ExternalModuleReference
+      )
+      or
+      exists(SsaExplicitDefinition ssa, VarDef def |
+        this = DataFlow::ssaDefinitionNode(ssa) and def = ssa.getDef()
+      |
+        def instanceof ImportSpecifier
+      )
+      or
+      DataFlow::parameterNode(this, _)
+      or
+      this instanceof DataFlow::Impl::InvokeNodeDef
+      or
+      DataFlow::thisNode(this, _)
+    }
   }
 }
+
+deprecated class DefaultSourceNode extends SourceNode {
+  DefaultSourceNode() { this instanceof SourceNode::DefaultRange }
+}

javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll

@@ -45,7 +45,7 @@ private predicate isAngularString(Expr s) {
  * String literals in Angular code are often used as identifiers or references, so we
  * want to track them.
  */
-private class TrackStringsInAngularCode extends DataFlow::SourceNode, DataFlow::ValueNode {
+private class TrackStringsInAngularCode extends DataFlow::SourceNode::Range, DataFlow::ValueNode {
   TrackStringsInAngularCode() { isAngularString(astNode) }
 }
 

javascript/ql/src/semmle/javascript/frameworks/HTTP.qll

@@ -435,6 +435,10 @@ module HTTP {
     abstract DataFlow::Node getASecretKey();
   }
 
+  private class CookieMiddlewareInstanceAsSourceNode extends DataFlow::SourceNode::Range {
+    CookieMiddlewareInstanceAsSourceNode() { this instanceof CookieMiddlewareInstance }
+  }
+
   /**
    * A key used for signed cookies, viewed as a `CryptographicKey`.
    */

javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll

@@ -14,6 +14,10 @@ module LodashUnderscore {
     abstract string getName();
   }
 
+  private class MemberAsSourceNode extends DataFlow::SourceNode::Range {
+    MemberAsSourceNode() { this instanceof Member }
+  }
+
   /**
    * An import of `lodash` or `underscore` accessing a given member of that package.
    */

javascript/ql/src/semmle/javascript/frameworks/ReactNative.qll

@@ -6,7 +6,7 @@ import javascript
 
 module ReactNative {
   /** A `WebView` JSX element. */
-  class WebViewElement extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
+  class WebViewElement extends DataFlow::ValueNode, DataFlow::SourceNode {
     override JSXElement astNode;
 
     WebViewElement() {