Merge pull request #294 from asger-semmle/canonical-this-source

JS: Canonicalize 'this' in the data-flow graph

Author: Max Schaefer

change-notes/1.19/analysis-javascript.md

@@ -35,4 +35,8 @@
 
 ## Changes to QL libraries
 
-* The flow configuration framework now supports distinguishing and tracking different kinds of taint, specified by an extensible class `FlowLabel` (which can also be referred to by its alias `TaintKind`).
+* The flow configuration framework now supports distinguishing and tracking different kinds of taint, specified by an extensible class `FlowLabel` (which can also be referred to by its alias `TaintKind`).
+
+* The `DataFlow::ThisNode` class now corresponds to the implicit receiver parameter of a function, as opposed to an indivdual `this` expression. This means `getALocalSource` now maps all `this` expressions within a given function to the same source. The data-flow node associated with a `ThisExpr` can no longer be cast to `DataFlow::SourceNode` or `DataFlow::ThisNode` - it is recomended to use `getALocalSource` before casting or instead of casting.
+
+* `ReactComponent::getAThisAccess` has been renamed to `getAThisNode`. The old name is still usable but is deprecated. It no longer gets individual `this` expressions, but the `ThisNode` mentioned above.

javascript/ql/src/semmle/javascript/Expr.qll

@@ -303,6 +303,16 @@ class ThisExpr extends @thisexpr, Expr {
   Function getBinder() {
     result = getEnclosingFunction().getThisBinder()
   }
+
+  /**
+   * Gets the function or top-level whose `this` binding this expression refers to,
+   * which is the nearest enclosing non-arrow function or top-level.
+   */
+  StmtContainer getBindingContainer() {
+    result = getContainer().(Function).getThisBindingContainer()
+    or
+    result = getContainer().(TopLevel)
+  }
 }
 
 /** An array literal. */

javascript/ql/src/semmle/javascript/Functions.qll

@@ -206,6 +206,17 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
     result = this
   }
 
+  /**
+   * Gets the function or top-level whose `this` binding a `this` expression in this function refers to,
+   * which is the nearest enclosing non-arrow function or top-level.
+   */
+  StmtContainer getThisBindingContainer() {
+    result = getThisBinder()
+    or
+    not exists(getThisBinder()) and
+    result = getTopLevel()
+  }
+
   /**
    * Holds if this function has a mapped `arguments` variable whose indices are aliased
    * with the function's parameters.

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

@@ -62,9 +62,9 @@ class JsonParseCall extends MethodCallExpr {
  * However, since the function could be invoked in another way, we additionally
  * still infer the ordinary abstract value.
  */
-private class AnalyzedThisInArrayIterationFunction extends AnalyzedValueNode, DataFlow::ThisNode {
+private class AnalyzedThisInArrayIterationFunction extends AnalyzedNode, DataFlow::ThisNode {
 
-  AnalyzedValueNode thisSource;
+  AnalyzedNode thisSource;
 
   AnalyzedThisInArrayIterationFunction() {
     exists(DataFlow::MethodCallNode bindingCall, string name |
@@ -82,7 +82,7 @@ private class AnalyzedThisInArrayIterationFunction extends AnalyzedValueNode, Da
 
   override AbstractValue getALocalValue() {
     result = thisSource.getALocalValue() or
-    result = AnalyzedValueNode.super.getALocalValue()
+    result = AnalyzedNode.super.getALocalValue()
   }
 
 }

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

@@ -32,6 +32,7 @@ module DataFlow {
   or TReflectiveCallNode(MethodCallExpr ce, string kind) {
        ce.getMethodName() = kind and (kind = "call" or kind = "apply")
      }
+  or TThisNode(StmtContainer f) { f.(Function).getThisBinder() = f or f instanceof TopLevel }
 
   /**
    * A node in the data flow graph.
@@ -867,6 +868,13 @@ module DataFlow {
     nd = TDestructuringPatternNode(p)
   }
 
+  /**
+   * INTERNAL: Use `thisNode(StmtContainer container)` instead.
+   */
+  predicate thisNode(DataFlow::Node node, StmtContainer container) {
+    node = TThisNode(container)
+  }
+
   /**
    * A classification of flows that are not modeled, or only modeled incompletely, by
    * `DataFlowNode`:
@@ -970,6 +978,11 @@ module DataFlow {
       pred = valueNode(defSourceNode(def)) and
       succ = TDestructuringPatternNode(def.getTarget())
     )
+    or
+    // flow from 'this' parameter into 'this' expressions
+    exists (ThisExpr thiz |
+      pred = TThisNode(thiz.getBindingContainer()) and
+      succ = valueNode(thiz))
   }
 
   /**

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

@@ -198,16 +198,36 @@ class NewNode extends InvokeNode {
   override DataFlow::Impl::NewNodeDef impl;
 }
 
-/** A data flow node corresponding to a `this` expression. */
-class ThisNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
-  override ThisExpr astNode;
+/** 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 {
+  ThisNode() {
+    DataFlow::thisNode(this, _)
+  }
 
   /**
    * Gets the function whose `this` binding this expression refers to,
    * which is the nearest enclosing non-arrow function.
    */
   FunctionNode getBinder() {
-    result = DataFlow::valueNode(astNode.getBinder())
+    exists (Function binder |
+      DataFlow::thisNode(this, binder) and
+      result = DataFlow::valueNode(binder))
+  }
+
+  /**
+   * Gets the function or top-level whose `this` binding this expression refers to,
+   * which is the nearest enclosing non-arrow function or top-level.
+   */
+  StmtContainer getBindingContainer() {
+    DataFlow::thisNode(this, result)
+  }
+
+  override string toString() { result = "this" }
+
+  override predicate hasLocationInfo(string filepath, int startline, int startcolumn,
+                                     int endline, int endcolumn) {
+    // Use the function entry as the location
+    getBindingContainer().getEntry().getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
 }
 

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

@@ -185,7 +185,6 @@ class DefaultSourceNode extends SourceNode {
       astNode instanceof ObjectExpr or
       astNode instanceof ArrayExpr or
       astNode instanceof JSXNode or
-      astNode instanceof ThisExpr or
       astNode instanceof GlobalVarAccess or
       astNode instanceof ExternalModuleReference
     )
@@ -198,5 +197,7 @@ class DefaultSourceNode extends SourceNode {
     DataFlow::parameterNode(this, _)
     or
     this instanceof DataFlow::Impl::InvokeNodeDef
+    or
+    DataFlow::thisNode(this, _)
   }
 }

javascript/ql/src/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll

@@ -10,7 +10,7 @@ import AbstractValuesImpl
 /**
  * Flow analysis for `this` expressions inside functions.
  */
-private abstract class AnalyzedThisExpr extends DataFlow::AnalyzedValueNode, DataFlow::ThisNode {
+private abstract class AnalyzedThisExpr extends DataFlow::AnalyzedNode, DataFlow::ThisNode {
   DataFlow::FunctionNode binder;
 
   AnalyzedThisExpr() {
@@ -29,7 +29,7 @@ private abstract class AnalyzedThisExpr extends DataFlow::AnalyzedValueNode, Dat
  */
 private class AnalyzedThisInBoundFunction extends AnalyzedThisExpr {
 
-  AnalyzedValueNode thisSource;
+  AnalyzedNode thisSource;
 
   AnalyzedThisInBoundFunction() {
     exists(string name |

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

@@ -27,9 +27,9 @@ module LodashUnderscore {
  * However, since the function could be invoked in another way, we additionally
  * still infer the ordinary abstract value.
  */
-private class AnalyzedThisInBoundCallback extends AnalyzedValueNode, DataFlow::ThisNode {
+private class AnalyzedThisInBoundCallback extends AnalyzedNode, DataFlow::ThisNode {
 
-  AnalyzedValueNode thisSource;
+  AnalyzedNode thisSource;
 
   AnalyzedThisInBoundCallback() {
     exists(DataFlow::CallNode bindingCall, string binderName, int callbackIndex, int contextIndex, int argumentCount |
@@ -128,7 +128,7 @@ private class AnalyzedThisInBoundCallback extends AnalyzedValueNode, DataFlow::T
 
   override AbstractValue getALocalValue() {
     result = thisSource.getALocalValue() or
-    result = AnalyzedValueNode.super.getALocalValue()
+    result = AnalyzedNode.super.getALocalValue()
   }
 
 }

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

@@ -52,10 +52,20 @@ abstract class ReactComponent extends ASTNode {
   }
 
   /**
-   * Gets a `this` access in an instance method of this component.
+   * Gets the `this` node in an instance method of this component.
    */
+  DataFlow::SourceNode getAThisNode() {
+    result.(DataFlow::ThisNode).getBinder().getFunction() = getInstanceMethod(_)
+  }
+
+  /**
+   * Gets the `this` node in an instance method of this component.
+   *
+   * DEPRECATED: Use `getAThisNode` instead.
+   */
+  deprecated
   DataFlow::SourceNode getAThisAccess() {
-    result.asExpr().(ThisExpr).getBinder() = getInstanceMethod(_)
+    result = getAThisNode()
   }
 
   /**
@@ -515,9 +525,9 @@ private class FactoryDefinition extends ReactElementDefinition {
  * However, since the function could be invoked in another way, we additionally
  * still infer the ordinary abstract value.
  */
-private class AnalyzedThisInBoundCallback extends AnalyzedValueNode, DataFlow::ThisNode {
+private class AnalyzedThisInBoundCallback extends AnalyzedNode, DataFlow::ThisNode {
 
-  AnalyzedValueNode thisSource;
+  AnalyzedNode thisSource;
 
   AnalyzedThisInBoundCallback() {
     exists(DataFlow::CallNode bindingCall, string binderName |
@@ -533,7 +543,7 @@ private class AnalyzedThisInBoundCallback extends AnalyzedValueNode, DataFlow::T
 
   override AbstractValue getALocalValue() {
     result = thisSource.getALocalValue() or
-    result = AnalyzedValueNode.super.getALocalValue()
+    result = AnalyzedNode.super.getALocalValue()
   }
 
 }