From 2b778afdf5c0d5a56e64159d56b9656bef22bdf0 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Wed, 13 Mar 2019 09:40:30 +0000
Subject: [PATCH 01/12] JavaScript: Cache a bunch of flow steps to avoid
 recomputation.

---
 .../dataflow/internal/FlowSteps.qll           | 445 +++++++++---------
 1 file changed, 232 insertions(+), 213 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
index 2559d6fb656..65c08b24ea4 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
@@ -17,36 +17,6 @@ predicate shouldTrackProperties(AbstractValue obj) {
   obj instanceof AbstractModuleObject
 }
 
-/**
- * Holds if `invk` may invoke `f`.
- */
-predicate calls(DataFlow::InvokeNode invk, Function f) { f = invk.getACallee(0) }
-
-/**
- * Holds if `invk` may invoke `f` indirectly through the given `callback` argument.
- *
- * This only holds for explicitly modeled partial calls.
- */
-private predicate partiallyCalls(
-  DataFlow::AdditionalPartialInvokeNode invk, DataFlow::AnalyzedNode callback, Function f
-) {
-  invk.isPartialArgument(callback, _, _) and
-  exists(AbstractFunction callee | callee = callback.getAValue() |
-    if callback.getAValue().isIndefinite("global")
-    then f = callee.getFunction() and f.getFile() = invk.getFile()
-    else f = callee.getFunction()
-  )
-}
-
-/**
- * Holds if `f` captures the variable defined by `def` in `cap`.
- */
-cached
-predicate captures(Function f, SsaExplicitDefinition def, SsaVariableCapture cap) {
-  def.getSourceVariable() = cap.getSourceVariable() and
-  f = cap.getContainer()
-}
-
 /**
  * Holds if `source` corresponds to an expression returned by `f`, and
  * `sink` equals `source`.
@@ -84,206 +54,255 @@ predicate localFlowStep(
 }
 
 /**
- * Holds if `arg` is passed as an argument into parameter `parm`
- * through invocation `invk` of function `f`.
+ * Implements a set of data flow predicates that are used by multiple predicates and
+ * hence should only be computed once.
  */
-predicate argumentPassing(
-  DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, DataFlow::ParameterNode parm
-) {
-  calls(invk, f) and
-  exists(int i |
-    f.getParameter(i) = parm.getParameter() and
-    not parm.isRestParameter() and
-    arg = invk.getArgument(i)
-  )
-  or
-  exists(DataFlow::Node callback, int i |
-    invk.(DataFlow::AdditionalPartialInvokeNode).isPartialArgument(callback, arg, i) and
-    partiallyCalls(invk, callback, f) and
-    parm.getParameter() = f.getParameter(i) and
-    not parm.isRestParameter()
-  )
-}
+cached
+private module CachedSteps {
+  /**
+   * Holds if `f` captures the variable defined by `def` in `cap`.
+   */
+  cached
+  predicate captures(Function f, SsaExplicitDefinition def, SsaVariableCapture cap) {
+    def.getSourceVariable() = cap.getSourceVariable() and
+    f = cap.getContainer()
+  }
 
-/**
- * Holds if there is a flow step from `pred` to `succ` through parameter passing
- * to a function call.
- */
-predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { argumentPassing(_, pred, _, succ) }
+  /**
+   * Holds if `invk` may invoke `f`.
+   */
+  cached
+  predicate calls(DataFlow::InvokeNode invk, Function f) { f = invk.getACallee(0) }
 
-/**
- * Holds if there is a flow step from `pred` to `succ` through returning
- * from a function call or the receiver flowing out of a constructor call.
- */
-predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) {
-  exists(Function f | calls(succ, f) |
-    returnExpr(f, pred, _)
+  /**
+   * Holds if `invk` may invoke `f` indirectly through the given `callback` argument.
+   *
+   * This only holds for explicitly modeled partial calls.
+   */
+  private predicate partiallyCalls(
+    DataFlow::AdditionalPartialInvokeNode invk, DataFlow::AnalyzedNode callback, Function f
+  ) {
+    invk.isPartialArgument(callback, _, _) and
+    exists(AbstractFunction callee | callee = callback.getAValue() |
+      if callback.getAValue().isIndefinite("global")
+      then f = callee.getFunction() and f.getFile() = invk.getFile()
+      else f = callee.getFunction()
+    )
+  }
+
+  /**
+   * Holds if `arg` is passed as an argument into parameter `parm`
+   * through invocation `invk` of function `f`.
+   */
+  cached
+  predicate argumentPassing(
+    DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, DataFlow::ParameterNode parm
+  ) {
+    calls(invk, f) and
+    exists(int i |
+      f.getParameter(i) = parm.getParameter() and
+      not parm.isRestParameter() and
+      arg = invk.getArgument(i)
+    )
     or
-    succ instanceof DataFlow::NewNode and
-    DataFlow::thisNode(pred, f)
-  )
-}
+    exists(DataFlow::Node callback, int i |
+      invk.(DataFlow::AdditionalPartialInvokeNode).isPartialArgument(callback, arg, i) and
+      partiallyCalls(invk, callback, f) and
+      parm.getParameter() = f.getParameter(i) and
+      not parm.isRestParameter()
+    )
+  }
 
-/**
- * Holds if there is an assignment to property `prop` of an object represented by `obj`
- * with right hand side `rhs` somewhere, and properties of `obj` should be tracked.
- */
-pragma[noinline]
-private predicate trackedPropertyWrite(AbstractValue obj, string prop, DataFlow::Node rhs) {
-  exists(AnalyzedPropertyWrite pw |
-    pw.writes(obj, prop, rhs) and
-    shouldTrackProperties(obj) and
-    // avoid introducing spurious global flow
-    not pw.baseIsIncomplete("global")
-  )
-}
+  /**
+   * Holds if there is a flow step from `pred` to `succ` through parameter passing
+   * to a function call.
+   */
+  cached
+  predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { argumentPassing(_, pred, _, succ) }
 
-/**
- * Holds if there is a flow step from `pred` to `succ` through an object property.
- */
-predicate propertyFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
-  exists(AbstractValue obj, string prop |
-    trackedPropertyWrite(obj, prop, pred) and
-    succ.(AnalyzedPropertyRead).reads(obj, prop)
-  )
-}
+  /**
+   * Holds if there is a flow step from `pred` to `succ` through returning
+   * from a function call or the receiver flowing out of a constructor call.
+   */
+  cached
+  predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(Function f | calls(succ, f) |
+      returnExpr(f, pred, _)
+      or
+      succ instanceof DataFlow::NewNode and
+      DataFlow::thisNode(pred, f)
+    )
+  }
 
-/**
- * Gets a node whose value is assigned to `gv` in `f`.
- */
-pragma[noinline]
-private DataFlow::ValueNode getADefIn(GlobalVariable gv, File f) {
-  exists(VarDef def |
-    def.getFile() = f and
-    def.getTarget() = gv.getAReference() and
-    result = DataFlow::valueNode(def.getSource())
-  )
-}
+  /**
+   * Holds if there is an assignment to property `prop` of an object represented by `obj`
+   * with right hand side `rhs` somewhere, and properties of `obj` should be tracked.
+   */
+  pragma[noinline]
+  private predicate trackedPropertyWrite(AbstractValue obj, string prop, DataFlow::Node rhs) {
+    exists(AnalyzedPropertyWrite pw |
+      pw.writes(obj, prop, rhs) and
+      shouldTrackProperties(obj) and
+      // avoid introducing spurious global flow
+      not pw.baseIsIncomplete("global")
+    )
+  }
 
-/**
- * Gets a use of `gv` in `f`.
- */
-pragma[noinline]
-DataFlow::ValueNode getAUseIn(GlobalVariable gv, File f) {
-  result.getFile() = f and
-  result = DataFlow::valueNode(gv.getAnAccess())
-}
+  /**
+   * Holds if there is a flow step from `pred` to `succ` through an object property.
+   */
+  cached
+  predicate propertyFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(AbstractValue obj, string prop |
+      trackedPropertyWrite(obj, prop, pred) and
+      succ.(AnalyzedPropertyRead).reads(obj, prop)
+    )
+  }
 
-/**
- * Holds if there is a flow step from `pred` to `succ` through a global
- * variable. Both `pred` and `succ` must be in the same file.
- */
-predicate globalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
-  exists(GlobalVariable gv, File f |
-    pred = getADefIn(gv, f) and
-    succ = getAUseIn(gv, f)
-  )
-}
+  /**
+   * Gets a node whose value is assigned to `gv` in `f`.
+   */
+  pragma[noinline]
+  private DataFlow::ValueNode getADefIn(GlobalVariable gv, File f) {
+    exists(VarDef def |
+      def.getFile() = f and
+      def.getTarget() = gv.getAReference() and
+      result = DataFlow::valueNode(def.getSource())
+    )
+  }
 
-/**
- * Holds if there is a write to property `prop` of global variable `gv`
- * in file `f`, where the right-hand side of the write is `rhs`.
- */
-pragma[noinline]
-predicate globalPropertyWrite(GlobalVariable gv, File f, string prop, DataFlow::Node rhs) {
-  exists(DataFlow::PropWrite pw | pw.writes(getAUseIn(gv, f), prop, rhs))
-}
+  /**
+   * Gets a use of `gv` in `f`.
+   */
+  pragma[noinline]
+  private DataFlow::ValueNode getAUseIn(GlobalVariable gv, File f) {
+    result.getFile() = f and
+    result = DataFlow::valueNode(gv.getAnAccess())
+  }
 
-/**
- * Holds if there is a read from property `prop` of `base`, which is
- * an access to global variable `base` in file `f`.
- */
-pragma[noinline]
-predicate globalPropertyRead(GlobalVariable gv, File f, string prop, DataFlow::Node base) {
-  exists(DataFlow::PropRead pr |
-    base = getAUseIn(gv, f) and
-    pr.accesses(base, prop)
-  )
-}
+  /**
+   * Holds if there is a flow step from `pred` to `succ` through a global
+   * variable. Both `pred` and `succ` must be in the same file.
+   */
+  cached
+  predicate globalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(GlobalVariable gv, File f |
+      pred = getADefIn(gv, f) and
+      succ = getAUseIn(gv, f)
+    )
+  }
 
-/**
- * Holds if there is a store step from `pred` to `succ` under property `prop`,
- * that is, `succ` is the local source of the base of a write of property
- * `prop` with right-hand side `pred`.
- *
- * For example, for this code snippet:
- *
- * ```
- * var a = new A();
- * a.p = e;
- * ```
- *
- * there is a store step from `e` to `new A()` under property `prop`.
- *
- * As a special case, if the base of the property write is a global variable,
- * then there is a store step from the right-hand side of the write to any
- * read of the same property from the same global variable in the same file.
- */
-predicate basicStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-  succ.(DataFlow::SourceNode).hasPropertyWrite(prop, pred)
-  or
-  exists(GlobalVariable gv, File f |
-    globalPropertyWrite(gv, f, prop, pred) and
-    globalPropertyRead(gv, f, prop, succ)
-  )
-}
+  /**
+   * Holds if there is a write to property `prop` of global variable `gv`
+   * in file `f`, where the right-hand side of the write is `rhs`.
+   */
+  pragma[noinline]
+  private predicate globalPropertyWrite(GlobalVariable gv, File f, string prop, DataFlow::Node rhs) {
+    exists(DataFlow::PropWrite pw | pw.writes(getAUseIn(gv, f), prop, rhs))
+  }
 
-/**
- * Holds if there is a load step from `pred` to `succ` under property `prop`,
- * that is, `succ` is a read of property `prop` from `pred`.
- */
-predicate loadStep(DataFlow::Node pred, DataFlow::PropRead succ, string prop) {
-  succ.accesses(pred, prop)
-}
+  /**
+   * Holds if there is a read from property `prop` of `base`, which is
+   * an access to global variable `base` in file `f`.
+   */
+  pragma[noinline]
+  private predicate globalPropertyRead(GlobalVariable gv, File f, string prop, DataFlow::Node base) {
+    exists(DataFlow::PropRead pr |
+      base = getAUseIn(gv, f) and
+      pr.accesses(base, prop)
+    )
+  }
 
-/**
- * Holds if there is a higher-order call with argument `arg`, and `cb` is the local
- * source of an argument that flows into the callee position of that call:
- *
- * ```
- * function f(x, g) {
- *   g(
- *     x                 // arg
- *   );
- * }
- *
- * function cb() {      // cb
- * }
- *
- * f(arg, cb);
- *
- * This is an over-approximation of a possible data flow step through a callback
- * invocation.
- */
-predicate callback(DataFlow::Node arg, DataFlow::SourceNode cb) {
-  exists(DataFlow::InvokeNode invk, DataFlow::ParameterNode cbParm, DataFlow::Node cbArg |
-    arg = invk.getAnArgument() and
-    cbParm.flowsTo(invk.getCalleeNode()) and
-    callStep(cbArg, cbParm) and
-    cb.flowsTo(cbArg)
-  )
-  or
-  exists(DataFlow::ParameterNode cbParm, DataFlow::Node cbArg |
-    callback(arg, cbParm) and
-    callStep(cbArg, cbParm) and
-    cb.flowsTo(cbArg)
-  )
-}
+  /**
+   * Holds if there is a store step from `pred` to `succ` under property `prop`,
+   * that is, `succ` is the local source of the base of a write of property
+   * `prop` with right-hand side `pred`.
+   *
+   * For example, for this code snippet:
+   *
+   * ```
+   * var a = new A();
+   * a.p = e;
+   * ```
+   *
+   * there is a store step from `e` to `new A()` under property `prop`.
+   *
+   * As a special case, if the base of the property write is a global variable,
+   * then there is a store step from the right-hand side of the write to any
+   * read of the same property from the same global variable in the same file.
+   */
+  cached
+  predicate basicStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    succ.(DataFlow::SourceNode).hasPropertyWrite(prop, pred)
+    or
+    exists(GlobalVariable gv, File f |
+      globalPropertyWrite(gv, f, prop, pred) and
+      globalPropertyRead(gv, f, prop, succ)
+    )
+  }
 
-/**
- * Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`.
- */
-predicate returnedPropWrite(Function f, DataFlow::SourceNode base, string prop, DataFlow::Node rhs) {
-  base.hasPropertyWrite(prop, rhs) and
-  base.flowsToExpr(f.getAReturnedExpr())
-}
+  /**
+   * Holds if there is a load step from `pred` to `succ` under property `prop`,
+   * that is, `succ` is a read of property `prop` from `pred`.
+   */
+  cached
+  predicate loadStep(DataFlow::Node pred, DataFlow::PropRead succ, string prop) {
+    succ.accesses(pred, prop)
+  }
 
-/**
- * Holds if `f` may assign `rhs` to `this.prop`.
- */
-predicate receiverPropWrite(Function f, string prop, DataFlow::Node rhs) {
-  DataFlow::thisNode(f).hasPropertyWrite(prop, rhs)
+  /**
+   * Holds if there is a higher-order call with argument `arg`, and `cb` is the local
+   * source of an argument that flows into the callee position of that call:
+   *
+   * ```
+   * function f(x, g) {
+   *   g(
+   *     x                 // arg
+   *   );
+   * }
+   *
+   * function cb() {      // cb
+   * }
+   *
+   * f(arg, cb);
+   *
+   * This is an over-approximation of a possible data flow step through a callback
+   * invocation.
+   */
+  cached
+  predicate callback(DataFlow::Node arg, DataFlow::SourceNode cb) {
+    exists(DataFlow::InvokeNode invk, DataFlow::ParameterNode cbParm, DataFlow::Node cbArg |
+      arg = invk.getAnArgument() and
+      cbParm.flowsTo(invk.getCalleeNode()) and
+      callStep(cbArg, cbParm) and
+      cb.flowsTo(cbArg)
+    )
+    or
+    exists(DataFlow::ParameterNode cbParm, DataFlow::Node cbArg |
+      callback(arg, cbParm) and
+      callStep(cbArg, cbParm) and
+      cb.flowsTo(cbArg)
+    )
+  }
+
+  /**
+   * Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`.
+   */
+  cached
+  predicate returnedPropWrite(Function f, DataFlow::SourceNode base, string prop, DataFlow::Node rhs) {
+    base.hasPropertyWrite(prop, rhs) and
+    base.flowsToExpr(f.getAReturnedExpr())
+  }
+
+  /**
+   * Holds if `f` may assign `rhs` to `this.prop`.
+   */
+  cached
+  predicate receiverPropWrite(Function f, string prop, DataFlow::Node rhs) {
+    DataFlow::thisNode(f).hasPropertyWrite(prop, rhs)
+  }
 }
+import CachedSteps
 
 /**
  * A utility class that is equivalent to `boolean` but does not require type joining.

From c17f4d7d410a5c2319c4fc529e4cb24530ab0756 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Wed, 13 Mar 2019 12:52:05 +0000
Subject: [PATCH 02/12] JavaScript: Cache `SourceNode::track` and
 `SourceNode::backtrack`.

---
 javascript/ql/src/semmle/javascript/dataflow/Sources.qll | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll
index 436855732d2..d424f98f58c 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll
@@ -162,6 +162,7 @@ class SourceNode extends DataFlow::Node {
    *
    * See `TypeTracker` for more details about how to use this.
    */
+  cached
   DataFlow::SourceNode track(TypeTracker t2, TypeTracker t) {
     exists(StepSummary summary |
       StepSummary::step(this, result, summary) and
@@ -176,6 +177,7 @@ class SourceNode extends DataFlow::Node {
    *
    * See `TypeBackTracker` for more details about how to use this.
    */
+  cached
   DataFlow::SourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) {
     exists(StepSummary summary |
       StepSummary::step(result, this, summary) and

From 0e0fe2545dd8979787cda34097a3cf32d87eb0e4 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Tue, 19 Mar 2019 09:15:08 +0000
Subject: [PATCH 03/12] JavaScript: Refactor `Closure::isTopLevelExpr` to avoid
 unhelpful magic.

---
 javascript/ql/src/semmle/javascript/Closure.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/javascript/ql/src/semmle/javascript/Closure.qll b/javascript/ql/src/semmle/javascript/Closure.qll
index 937e70dd226..991a3568930 100644
--- a/javascript/ql/src/semmle/javascript/Closure.qll
+++ b/javascript/ql/src/semmle/javascript/Closure.qll
@@ -64,7 +64,7 @@ module Closure {
    * a top-level expression statement.
    */
   private predicate isTopLevelExpr(DataFlow::Node node) {
-    node.getTopLevel().getAChildStmt().(ExprStmt).getExpr().flow() = node
+    any(TopLevel tl).getAChildStmt().(ExprStmt).getExpr().flow() = node
   }
 
   /**

From 4702790696b65b0bfec2ba5c6fed81c5796798f1 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Tue, 19 Mar 2019 09:45:06 +0000
Subject: [PATCH 04/12] JavaScript: Refactor AMD/CommonJS path expression
 analysis to avoid bad magic.

---
 javascript/ql/src/semmle/javascript/AMD.qll   | 21 +++++++++---------
 .../ql/src/semmle/javascript/NodeJS.qll       | 22 +++++++++----------
 javascript/ql/src/semmle/javascript/Paths.qll | 17 ++++++++++++++
 3 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/AMD.qll b/javascript/ql/src/semmle/javascript/AMD.qll
index 0323154e1dc..70b4970960e 100644
--- a/javascript/ql/src/semmle/javascript/AMD.qll
+++ b/javascript/ql/src/semmle/javascript/AMD.qll
@@ -170,20 +170,19 @@ class AMDModuleDefinition extends CallExpr {
   }
 }
 
-/** A path expression appearing in the list of dependencies of an AMD module. */
-private class AMDDependencyPath extends PathExprInModule, ConstantString {
-  AMDDependencyPath() {
-    exists(AMDModuleDefinition amd | this.getParentExpr*() = amd.getDependencies().getAnElement())
+/** An AMD dependency, considered as a path expression. */
+private class AmdDependencyPath extends PathExprCandidate {
+  AmdDependencyPath() {
+    exists(AMDModuleDefinition amd |
+      this = amd.getDependencies().getAnElement() or
+      this = amd.getARequireCall().getAnArgument()
+    )
   }
-
-  override string getValue() { result = this.(ConstantString).getStringValue() }
 }
 
-/** A path expression appearing in a `require` call in an AMD module. */
-private class AMDRequirePath extends PathExprInModule, ConstantString {
-  AMDRequirePath() {
-    exists(AMDModuleDefinition amd | this.getParentExpr*() = amd.getARequireCall().getAnArgument())
-  }
+/** A constant path element appearing in an AMD dependency expression. */
+private class ConstantAmdDependencyPathElement extends PathExprInModule, ConstantString {
+  ConstantAmdDependencyPathElement() { this = any(AmdDependencyPath amd).getAPart() }
 
   override string getValue() { result = this.(ConstantString).getStringValue() }
 }
diff --git a/javascript/ql/src/semmle/javascript/NodeJS.qll b/javascript/ql/src/semmle/javascript/NodeJS.qll
index c6da1a96c90..9d9bea4f096 100644
--- a/javascript/ql/src/semmle/javascript/NodeJS.qll
+++ b/javascript/ql/src/semmle/javascript/NodeJS.qll
@@ -239,22 +239,22 @@ class Require extends CallExpr, Import {
   }
 }
 
-/** A literal path expression appearing in a `require` import. */
-private class LiteralRequiredPath extends PathExprInModule, ConstantString {
-  LiteralRequiredPath() { exists(Require req | this.getParentExpr*() = req.getArgument(0)) }
-
-  override string getValue() { result = this.getStringValue() }
-}
-
-/** A literal path expression appearing in a call to `require.resolve`. */
-private class LiteralRequireResolvePath extends PathExprInModule, ConstantString {
-  LiteralRequireResolvePath() {
+/** An argument to `require` or `require.resolve`, considered as a path expression. */
+private class RequirePath extends PathExprCandidate {
+  RequirePath() {
+    this = any(Require req).getArgument(0)
+    or
     exists(RequireVariable req, MethodCallExpr reqres |
       reqres.getReceiver() = req.getAnAccess() and
       reqres.getMethodName() = "resolve" and
-      this.getParentExpr*() = reqres.getArgument(0)
+      this = reqres.getArgument(0)
     )
   }
+}
+
+/** A constant path element appearing in a call to `require` or `require.resolve`. */
+private class ConstantRequirePathElement extends PathExprInModule, ConstantString {
+  ConstantRequirePathElement() { this = any(RequirePath rp).getAPart() }
 
   override string getValue() { result = this.getStringValue() }
 }
diff --git a/javascript/ql/src/semmle/javascript/Paths.qll b/javascript/ql/src/semmle/javascript/Paths.qll
index 516f235dd06..9dceefcfdda 100644
--- a/javascript/ql/src/semmle/javascript/Paths.qll
+++ b/javascript/ql/src/semmle/javascript/Paths.qll
@@ -236,3 +236,20 @@ private class ConcatPath extends PathExpr {
     result = this.(AddExpr).getAnOperand().(PathExpr).getSearchRoot(priority)
   }
 }
+
+/**
+ * An expression that appears in a syntactic position where it may represent a path.
+ *
+ * Examples include arguments to the CommonJS `require` function or AMD dependency arguments.
+ */
+abstract class PathExprCandidate extends Expr {
+  /**
+   * Gets an expression that is nested inside this expression.
+   *
+   * Equivalent to `getAChildExpr*()`, but useful to enforce a better join order (in spite of
+   * what the optimizer thinks, there are generally far fewer `PathExprCandidate`s than
+   * `ConstantString`s).
+   */
+  pragma[nomagic]
+  Expr getAPart() { result = this or result = getAPart().getAChildExpr() }
+}

From 276f216ef92d3e84203656eeed38041dc4bae9a1 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Tue, 5 Mar 2019 11:17:02 +0000
Subject: [PATCH 05/12] JavaScript: Use type tracking to improve modelling of
 socket.io.

---
 .../semmle/javascript/frameworks/SocketIO.qll | 234 +++++++++++-------
 1 file changed, 138 insertions(+), 96 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll b/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll
index 8cddd068dc6..1d83be0755f 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll
@@ -24,16 +24,20 @@ module SocketIO {
     result = DataFlow::moduleImport("socket.io").getAMemberCall("listen")
   }
 
-  /** A data flow node that may produce (that is, create or return) a socket.io server. */
-  class ServerNode extends DataFlow::SourceNode {
-    ServerObject srv;
-
-    ServerNode() {
-      this = newServer() and
-      srv = MkServer(this)
+  /**
+   * Gets a data flow node that may refer to the socket.io server created at `srv`, with
+   * type tracking info stored in `t`.
+   */
+  private DataFlow::SourceNode server(ServerObject srv, DataFlow::TypeTracker t) {
+    result = newServer() and
+    srv = MkServer(result) and
+    t.start()
+    or
+    exists(DataFlow::TypeTracker s, DataFlow::SourceNode pred | pred = server(srv, s) |
+      result = pred.track(s, t)
       or
       // invocation of a chainable method
-      exists(ServerNode base, DataFlow::MethodCallNode mcn, string m |
+      exists(DataFlow::MethodCallNode mcn, string m |
         m = "adapter" or
         m = "attach" or
         m = "bind" or
@@ -44,96 +48,118 @@ module SocketIO {
         m = "serveClient" or
         m = "set"
       |
-        mcn = base.getAMethodCall(m) and
+        mcn = pred.getAMethodCall(m) and
         // exclude getter versions
         exists(mcn.getAnArgument()) and
-        this = mcn and
-        srv = base.getServer()
+        result = mcn and
+        t = s
       )
-    }
+    )
+  }
+
+  /** A data flow node that may produce (that is, create or return) a socket.io server. */
+  class ServerNode extends DataFlow::SourceNode {
+    ServerObject srv;
+
+    ServerNode() { this = server(srv, _) }
 
     /** Gets the server to which this node refers. */
     ServerObject getServer() { result = srv }
   }
 
+  /**
+   * Gets the name of a chainable method on socket.io namespace objects, which servers forward
+   * to their default namespace.
+   */
+  private string namespaceChainableMethod() {
+    result = "binary" or
+    result = "clients" or
+    result = "compress" or
+    result = "emit" or
+    result = "in" or
+    result = "send" or
+    result = "to" or
+    result = "use" or
+    result = "write" or
+    result = EventEmitter::chainableMethod()
+  }
+
+  /**
+   * Gets a data flow node that may refer to the socket.io namespace created at `ns`, with
+   * type tracking info stored in `t`.
+   */
+  private DataFlow::SourceNode namespace(NamespaceObject ns, DataFlow::TypeTracker t) {
+    t.start() and
+    exists(ServerNode srv |
+      // namespace lookup on `srv`
+      result = srv.getAPropertyRead("sockets") and
+      ns = srv.getServer().getDefaultNamespace()
+      or
+      exists(DataFlow::MethodCallNode mcn, string path |
+        mcn = srv.getAMethodCall("of") and
+        mcn.getArgument(0).mayHaveStringValue(path) and
+        result = mcn and
+        ns = MkNamespace(srv.getServer(), path)
+      )
+      or
+      // invocation of a method that `srv` forwards to its default namespace
+      result = srv.getAMethodCall(namespaceChainableMethod()) and
+      ns = srv.getServer().getDefaultNamespace()
+    )
+    or
+    exists(DataFlow::SourceNode pred, DataFlow::TypeTracker s | pred = namespace(ns, s) |
+      result = pred.track(s, t)
+      or
+      // invocation of a chainable method
+      result = pred.getAMethodCall(namespaceChainableMethod()) and
+      t = s
+      or
+      // invocation of chainable getter method
+      exists(string m |
+        m = "json" or
+        m = "local" or
+        m = "volatile"
+      |
+        result = pred.getAPropertyRead(m) and
+        t = s
+      )
+    )
+  }
+
   /** A data flow node that may produce a namespace object. */
   class NamespaceNode extends DataFlow::SourceNode {
     NamespaceObject ns;
 
-    NamespaceNode() {
-      // namespace lookup
-      exists(ServerNode srv |
-        this = srv.getAPropertyRead("sockets") and
-        ns = srv.getServer().getDefaultNamespace()
-        or
-        exists(DataFlow::MethodCallNode mcn, string path |
-          mcn = srv.getAMethodCall("of") and
-          mcn.getArgument(0).mayHaveStringValue(path) and
-          this = mcn and
-          ns = MkNamespace(srv.getServer(), path)
-        )
-      )
-      or
-      // invocation of a chainable method
-      exists(string m |
-        m = "binary" or
-        m = "clients" or
-        m = "compress" or
-        m = "emit" or
-        m = "in" or
-        m = "send" or
-        m = "to" or
-        m = "use" or
-        m = "write" or
-        m = EventEmitter::chainableMethod()
-      |
-        exists(NamespaceNode base |
-          this = base.getAMethodCall(m) and
-          ns = base.getNamespace()
-        )
-        or
-        // server objects forward these methods to their default namespace
-        exists(ServerNode srv |
-          this = srv.getAMethodCall(m) and
-          ns = srv.getServer().getDefaultNamespace()
-        )
-      )
-      or
-      // invocation of chainable getter method
-      exists(NamespaceNode base, string m |
-        m = "json" or
-        m = "local" or
-        m = "volatile"
-      |
-        this = base.getAPropertyRead(m) and
-        ns = base.getNamespace()
-      )
-    }
+    NamespaceNode() { this = namespace(ns, _) }
 
     /** Gets the namespace to which this node refers. */
     NamespaceObject getNamespace() { result = ns }
   }
 
-  /** A data flow node that may produce a socket object. */
-  class SocketNode extends DataFlow::SourceNode {
-    NamespaceObject ns;
-
-    SocketNode() {
-      // callback accepting a socket
-      exists(DataFlow::SourceNode base, string connect, DataFlow::MethodCallNode on |
-        (
-          ns = base.(ServerNode).getServer().getDefaultNamespace() or
-          ns = base.(NamespaceNode).getNamespace()
-        ) and
-        (connect = "connect" or connect = "connection")
-      |
-        on = base.getAMethodCall(EventEmitter::on()) and
-        on.getArgument(0).mayHaveStringValue(connect) and
-        this = on.getCallback(1).getParameter(0)
-      )
+  /**
+   * Gets a data flow node that may refer to a socket.io socket belonging to namespace `ns`, with
+   * type tracking info stored in `t`.
+   */
+  private DataFlow::SourceNode socket(NamespaceObject ns, DataFlow::TypeTracker t) {
+    // callback accepting a socket
+    t.start() and
+    exists(DataFlow::SourceNode base, string connect, DataFlow::MethodCallNode on |
+      (
+        ns = base.(ServerNode).getServer().getDefaultNamespace() or
+        ns = base.(NamespaceNode).getNamespace()
+      ) and
+      (connect = "connect" or connect = "connection")
+    |
+      on = base.getAMethodCall(EventEmitter::on()) and
+      on.getArgument(0).mayHaveStringValue(connect) and
+      result = on.getCallback(1).getParameter(0)
+    )
+    or
+    exists(DataFlow::SourceNode pred, DataFlow::TypeTracker s | pred = socket(ns, s) |
+      result = pred.track(s, t)
       or
       // invocation of a chainable method
-      exists(SocketNode base, string m |
+      exists(string m |
         m = "binary" or
         m = "compress" or
         m = "disconnect" or
@@ -147,21 +173,28 @@ module SocketIO {
         m = "write" or
         m = EventEmitter::chainableMethod()
       |
-        this = base.getAMethodCall(m) and
-        ns = base.getNamespace()
+        result = pred.getAMethodCall(m) and
+        t = s
       )
       or
       // invocation of a chainable getter method
-      exists(SocketNode base, string m |
+      exists(string m |
         m = "broadcast" or
         m = "json" or
         m = "local" or
         m = "volatile"
       |
-        this = base.getAPropertyRead(m) and
-        ns = base.getNamespace()
+        result = pred.getAPropertyRead(m) and
+        t = s
       )
-    }
+    )
+  }
+
+  /** A data flow node that may produce a socket object. */
+  class SocketNode extends DataFlow::SourceNode {
+    NamespaceObject ns;
+
+    SocketNode() { this = socket(ns, _) }
 
     /** Gets the namespace to which this socket belongs. */
     NamespaceObject getNamespace() { result = ns }
@@ -361,21 +394,30 @@ module SocketIO {
  * (npm package `socket.io-client`).
  */
 module SocketIOClient {
+  /**
+   * Gets a data flow node that may refer to the socket.io socket created at `invk`, with
+   * type tracking info stored in `t`.
+   */
+  private DataFlow::SourceNode socket(DataFlow::InvokeNode invk, DataFlow::TypeTracker t) {
+    t.start() and
+    exists(DataFlow::SourceNode io |
+      io = DataFlow::globalVarRef("io") or
+      io = DataFlow::globalVarRef("io").getAPropertyRead("connect") or
+      io = DataFlow::moduleImport("socket.io-client") or
+      io = DataFlow::moduleMember("socket.io-client", "connect")
+    |
+      invk = io.getAnInvocation() and
+      result = invk
+    )
+    or
+    exists(DataFlow::TypeTracker s | result = socket(invk, s).track(s, t))
+  }
+
   /** A data flow node that may produce a socket object. */
   class SocketNode extends DataFlow::SourceNode {
     DataFlow::InvokeNode invk;
 
-    SocketNode() {
-      exists(DataFlow::SourceNode io |
-        io = DataFlow::globalVarRef("io") or
-        io = DataFlow::globalVarRef("io").getAPropertyRead("connect") or
-        io = DataFlow::moduleImport("socket.io-client") or
-        io = DataFlow::moduleMember("socket.io-client", "connect")
-      |
-        invk = io.getAnInvocation() and
-        this = invk
-      )
-    }
+    SocketNode() { this = socket(invk, _) }
 
     /** Gets the path of the namespace this socket belongs to, if it can be determined. */
     string getNamespacePath() {

From 74db8b19796fdf59e7f97d0c5309c4d5a4658d14 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Thu, 7 Mar 2019 10:45:43 +0000
Subject: [PATCH 06/12] JavaScript: Use type tracking instead of tracked nodes
 in `Express`.

---
 .../src/semmle/javascript/frameworks/Electron.qll   | 13 +++++++++++--
 .../frameworks/Electron/BrowserObject.expected      |  2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
index 744648469fe..b2e6ff09ced 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
@@ -16,7 +16,7 @@ module Electron {
   /**
    * An instantiation of `BrowserWindow` or `BrowserView`.
    */
-  abstract private class NewBrowserObject extends BrowserObject, DataFlow::SourceNode {
+  abstract private class NewBrowserObject extends BrowserObject {
     DataFlow::NewNode self;
 
     NewBrowserObject() { this = self }
@@ -56,11 +56,20 @@ module Electron {
     }
   }
 
+  private DataFlow::SourceNode browserObject(DataFlow::TypeTracker t) {
+    t.start() and
+    result instanceof NewBrowserObject
+    or
+    exists(DataFlow::TypeTracker prev |
+      result = browserObject(prev).track(prev, t)
+    )
+  }
+
   /**
    * A data flow node whose value may originate from a browser object instantiation.
    */
   private class BrowserObjectByFlow extends BrowserObject {
-    BrowserObjectByFlow() { any(NewBrowserObject nbo).flowsTo(this) }
+    BrowserObjectByFlow() { browserObject(_).flowsTo(this) }
   }
 
   /**
diff --git a/javascript/ql/test/library-tests/frameworks/Electron/BrowserObject.expected b/javascript/ql/test/library-tests/frameworks/Electron/BrowserObject.expected
index 76865b0c1db..fca1b6a91a3 100644
--- a/javascript/ql/test/library-tests/frameworks/Electron/BrowserObject.expected
+++ b/javascript/ql/test/library-tests/frameworks/Electron/BrowserObject.expected
@@ -4,6 +4,8 @@
 | electron.js:3:10:3:48 | new Bro ... s: {}}) |
 | electron.js:4:5:4:46 | bv |
 | electron.js:4:10:4:46 | new Bro ... s: {}}) |
+| electron.js:35:14:35:14 | x |
+| electron.js:36:12:36:12 | x |
 | electron.js:39:5:39:6 | bw |
 | electron.js:40:5:40:6 | bv |
 | electron.ts:3:12:3:13 | bw |

From 55394df96ff937d3961e9ccd53f0d6450b05d09d Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Thu, 7 Mar 2019 10:50:06 +0000
Subject: [PATCH 07/12] JavaScript: Refactor HTTP libraries to use type
 tracking instead of tracked nodes.

---
 .../semmle/javascript/frameworks/Connect.qll  | 29 ++++++++----------
 .../semmle/javascript/frameworks/Express.qll  | 29 ++++++++----------
 .../src/semmle/javascript/frameworks/HTTP.qll | 22 ++++++++++++--
 .../src/semmle/javascript/frameworks/Hapi.qll | 28 ++++++++---------
 .../src/semmle/javascript/frameworks/Koa.qll  | 15 +++++++++-
 .../javascript/frameworks/NodeJSLib.qll       | 30 ++++++++-----------
 .../HTTP-heuristics/RouteHandler.expected     |  2 --
 .../UnpromotedRouteHandlerCandidate.expected  | 17 +++++++++++
 .../UnpromotedRouteSetupCandidate.expected    | 17 -----------
 9 files changed, 102 insertions(+), 87 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Connect.qll b/javascript/ql/src/semmle/javascript/frameworks/Connect.qll
index 3dcac8ab62b..f01a995b9ef 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Connect.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Connect.qll
@@ -118,8 +118,16 @@ module Connect {
     }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result.(DataFlow::SourceNode).flowsTo(getARouteHandlerExpr().flow()) or
-      result.(DataFlow::TrackedNode).flowsTo(getARouteHandlerExpr().flow())
+      result = getARouteHandler(_)
+    }
+
+    private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result = getARouteHandlerExpr().flow().getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker next |
+        result = getARouteHandler(next).backtrack(next, t)
+      )
     }
 
     override Expr getServer() { result = server }
@@ -169,24 +177,13 @@ module Connect {
   }
 
   /**
-   * Tracking for `RouteHandlerCandidate`.
-   */
-  private class TrackedRouteHandlerCandidate extends DataFlow::TrackedNode {
-    TrackedRouteHandlerCandidate() { this instanceof RouteHandlerCandidate }
-  }
-
-  /**
-   * A function that looks like a Connect route handler and flows to a route setup.
+   * A function that flows to a route setup.
    */
   private class TrackedRouteHandlerCandidateWithSetup extends RouteHandler,
-    HTTP::Servers::StandardRouteHandler, DataFlow::ValueNode {
-    override Function astNode;
+    HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode {
 
     TrackedRouteHandlerCandidateWithSetup() {
-      exists(TrackedRouteHandlerCandidate tracked |
-        tracked.flowsTo(any(RouteSetup s).getARouteHandlerExpr().flow()) and
-        this = tracked
-      )
+      this = any(RouteSetup s).getARouteHandler()
     }
 
     override SimpleParameter getRouteHandlerParameter(string kind) {
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
index b6f97f4dde4..3b1b8e53ef3 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
@@ -111,8 +111,16 @@ module Express {
     Expr getLastRouteHandlerExpr() { result = max(int i | | getRouteHandlerExpr(i) order by i) }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result.(DataFlow::SourceNode).flowsTo(getARouteHandlerExpr().flow()) or
-      result.(DataFlow::TrackedNode).flowsTo(getARouteHandlerExpr().flow())
+      result = getARouteHandler(_)
+    }
+
+    private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result = getARouteHandlerExpr().flow().getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker next |
+        result = getARouteHandler(next).backtrack(next, t)
+      )
     }
 
     override Expr getServer() { result.(Application).getARouteHandler() = getARouteHandler() }
@@ -766,24 +774,13 @@ module Express {
   }
 
   /**
-   * Tracking for `RouteHandlerCandidate`.
-   */
-  private class TrackedRouteHandlerCandidate extends DataFlow::TrackedNode {
-    TrackedRouteHandlerCandidate() { this instanceof RouteHandlerCandidate }
-  }
-
-  /**
-   * A function that looks like an Express route handler and flows to a route setup.
+   * A function that flows to a route setup.
    */
   private class TrackedRouteHandlerCandidateWithSetup extends RouteHandler,
-    HTTP::Servers::StandardRouteHandler, DataFlow::ValueNode {
-    override Function astNode;
+    HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode {
 
     TrackedRouteHandlerCandidateWithSetup() {
-      exists(TrackedRouteHandlerCandidate tracked |
-        tracked.flowsTo(any(RouteSetup s).getARouteHandlerExpr().flow()) and
-        this = tracked
-      )
+      this = any(RouteSetup s).getARouteHandler()
     }
 
     override SimpleParameter getRouteHandlerParameter(string kind) {
diff --git a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
index a9c7813f2da..6bce331e7f3 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
@@ -276,11 +276,20 @@ module HTTP {
      * a request object enters the flow graph, such as the request
      * parameter of a route handler.
      */
-    abstract class RequestSource extends DataFlow::TrackedNode {
+    abstract class RequestSource extends DataFlow::Node {
       /**
        * Gets the route handler that handles this request.
        */
       abstract RouteHandler getRouteHandler();
+
+      predicate flowsTo(DataFlow::Node nd) { flowsToSourceNode(_).flowsTo(nd) }
+
+      private DataFlow::SourceNode flowsToSourceNode(DataFlow::TypeTracker t) {
+        t.start() and
+        result = this
+        or
+        exists(DataFlow::TypeTracker prev | result = flowsToSourceNode(prev).track(prev, t))
+      }
     }
 
     /**
@@ -288,11 +297,20 @@ module HTTP {
      * a response object enters the flow graph, such as the response
      * parameter of a route handler.
      */
-    abstract class ResponseSource extends DataFlow::TrackedNode {
+    abstract class ResponseSource extends DataFlow::Node {
       /**
        * Gets the route handler that provides this response.
        */
       abstract RouteHandler getRouteHandler();
+
+      predicate flowsTo(DataFlow::Node nd) { flowsToSourceNode(_).flowsTo(nd) }
+
+      private DataFlow::SourceNode flowsToSourceNode(DataFlow::TypeTracker t) {
+        t.start() and
+        result = this
+        or
+        exists(DataFlow::TypeTracker prev | result = flowsToSourceNode(prev).track(prev, t))
+      }
     }
 
     /**
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll b/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll
index bf1ccc8b250..ab553a12f29 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll
@@ -193,8 +193,16 @@ module Hapi {
     }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result.(DataFlow::SourceNode).flowsTo(handler.flow()) or
-      result.(DataFlow::TrackedNode).flowsTo(handler.flow())
+      result = getARouteHandler(_)
+    }
+
+    private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result = handler.flow().getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker next |
+        result = getARouteHandler(next).backtrack(next, t)
+      )
     }
 
     Expr getRouteHandlerExpr() { result = handler }
@@ -223,25 +231,13 @@ module Hapi {
     }
   }
 
-  /**
-   * Tracking for `RouteHandlerCandidate`.
-   */
-  private class TrackedRouteHandlerCandidate extends DataFlow::TrackedNode {
-    TrackedRouteHandlerCandidate() { this instanceof RouteHandlerCandidate }
-  }
-
   /**
    * A function that looks like a Hapi route handler and flows to a route setup.
    */
   private class TrackedRouteHandlerCandidateWithSetup extends RouteHandler,
-    HTTP::Servers::StandardRouteHandler, DataFlow::ValueNode {
-    override Function astNode;
-
+    HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode {
     TrackedRouteHandlerCandidateWithSetup() {
-      exists(TrackedRouteHandlerCandidate tracked |
-        tracked.flowsTo(any(RouteSetup s).getRouteHandlerExpr().flow()) and
-        this = tracked
-      )
+      this = any(RouteSetup s).getARouteHandler()
     }
   }
 }
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Koa.qll b/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
index 98f7856d1d0..6c3e9f7848c 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
@@ -64,7 +64,7 @@ module Koa {
    * A Koa context source, that is, the context parameter of a
    * route handler, or a `this` access in a route handler.
    */
-  private class ContextSource extends DataFlow::TrackedNode {
+  private class ContextSource extends DataFlow::Node {
     RouteHandler rh;
 
     ContextSource() {
@@ -77,6 +77,19 @@ module Koa {
      * Gets the route handler that handles this request.
      */
     RouteHandler getRouteHandler() { result = rh }
+
+    predicate flowsTo(DataFlow::Node nd) {
+      flowsToSourceNode(_).flowsTo(nd)
+    }
+
+    private DataFlow::SourceNode flowsToSourceNode(DataFlow::TypeTracker t) {
+      t.start() and
+      result = this
+      or
+      exists(DataFlow::TypeTracker prev |
+        result = flowsToSourceNode(prev).track(prev, t)
+      )
+    }
   }
 
   /**
diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
index 5959eb261bf..a1550f2dddb 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
@@ -189,8 +189,16 @@ module NodeJSLib {
     }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result.(DataFlow::SourceNode).flowsTo(handler.flow()) or
-      result.(DataFlow::TrackedNode).flowsTo(handler.flow())
+      result = getARouteHandler(_)
+    }
+
+    private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result = handler.flow().getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker next |
+        result = getARouteHandler(next).backtrack(next, t)
+      )
     }
 
     override Expr getServer() { result = server }
@@ -613,24 +621,12 @@ module NodeJSLib {
   }
 
   /**
-   * Tracking for `RouteHandlerCandidate`.
-   */
-  private class TrackedRouteHandlerCandidate extends DataFlow::TrackedNode {
-    TrackedRouteHandlerCandidate() { this instanceof RouteHandlerCandidate }
-  }
-
-  /**
-   * A function that looks like a Node.js route handler and flows to a route setup.
+   * A function that flows to a route setup.
    */
   private class TrackedRouteHandlerCandidateWithSetup extends RouteHandler,
-    HTTP::Servers::StandardRouteHandler, DataFlow::ValueNode {
-    override Function astNode;
-
+    HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode {
     TrackedRouteHandlerCandidateWithSetup() {
-      exists(TrackedRouteHandlerCandidate tracked |
-        tracked.flowsTo(any(RouteSetup s).getRouteHandlerExpr().flow()) and
-        this = tracked
-      )
+      this = any(RouteSetup s).getARouteHandler()
     }
   }
 
diff --git a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/RouteHandler.expected b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/RouteHandler.expected
index 4f21ca278f4..a5a22b0cf84 100644
--- a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/RouteHandler.expected
+++ b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/RouteHandler.expected
@@ -11,8 +11,6 @@
 | src/exported-handler.js:1:19:1:55 | functio ... res) {} |
 | src/exported-middleware-attacher-2.js:2:13:2:32 | function(req, res){} |
 | src/exported-middleware-attacher.js:2:13:2:32 | function(req, res){} |
-| src/handler-in-property.js:5:14:5:33 | function(req, res){} |
-| src/handler-in-property.js:12:18:12:37 | function(req, res){} |
 | src/middleware-attacher-getter.js:4:17:4:36 | function(req, res){} |
 | src/middleware-attacher-getter.js:19:19:19:38 | function(req, res){} |
 | src/middleware-attacher.js:3:13:3:32 | function(req, res){} |
diff --git a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected
index 445223631c0..12dfcd4af93 100644
--- a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected
+++ b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected
@@ -1,8 +1,13 @@
 | src/bound-handler.js:4:1:4:28 | functio ...  res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/bound-handler.js:9:12:9:31 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/handler-in-property.js:5:14:5:33 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/handler-in-property.js:12:18:12:37 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/hapi.js:1:1:1:30 | functio ... t, h){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/iterated-handlers.js:4:2:4:22 | functio ...  res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/middleware-attacher-getter.js:29:32:29:51 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/nodejs.js:5:22:5:41 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/nodejs.js:11:23:11:42 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/nodejs.js:12:25:12:44 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/route-objects.js:7:19:7:38 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/route-objects.js:8:12:10:5 | (req, res) {\\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/route-objects.js:20:16:22:9 | (req, r ...       } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
@@ -10,8 +15,20 @@
 | src/route-objects.js:40:12:42:5 | (req, res) {\\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/route-objects.js:50:12:52:5 | (req, res) {\\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/route-objects.js:56:12:58:5 | functio ... ;\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:6:32:6:52 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:8:32:8:61 | functio ... nse) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:30:36:30:56 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:33:18:33:38 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:34:18:34:38 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:37:5:37:25 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:38:5:38:25 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:43:18:43:38 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/tst.js:46:1:46:23 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/tst.js:52:1:54:1 | functio ... req()\\n} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/tst.js:61:1:63:1 | functio ... turn;\\n} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/tst.js:70:5:72:5 | functio ... \\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:79:5:81:5 | functio ... \\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:84:5:86:5 | functio ... \\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/tst.js:109:16:111:9 | functio ...       } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:124:16:126:9 | functio ...       } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:132:16:134:9 | functio ...       } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
diff --git a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteSetupCandidate.expected b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteSetupCandidate.expected
index a349499ef27..e69de29bb2d 100644
--- a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteSetupCandidate.expected
+++ b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteSetupCandidate.expected
@@ -1,17 +0,0 @@
-| src/nodejs.js:5:1:5:42 | unknown ... res){}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/nodejs.js:11:1:11:43 | unknown ... res){}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/nodejs.js:12:1:12:45 | unknown ... res){}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:6:1:6:53 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:8:1:8:62 | someOth ... se) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:30:1:30:57 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:32:1:34:39 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:36:1:39:2 | someOth ... ) {}\\n]) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:41:1:43:39 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:87:5:87:57 | unknown ... cSetup) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:96:5:96:36 | unknown ... h', rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:98:5:98:38 | unknown ... , [rh]) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:104:5:104:45 | unknown ... wn, rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:137:5:137:57 | unknown ... cSetup) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:149:5:149:36 | unknown ... h', rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:151:5:151:38 | unknown ... , [rh]) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:157:5:157:45 | unknown ... wn, rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |

From 8e926333a93bd9dada41150389d66723b3deebb3 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Thu, 21 Mar 2019 08:30:11 +0000
Subject: [PATCH 08/12] JavaScript: Simplify a few `newtype`s and remove unused
 predicates.

---
 .../javascript/dataflow/TypeTracking.qll      | 34 ++-----------------
 1 file changed, 3 insertions(+), 31 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
index f1b2dce3eeb..ac2a6590982 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
@@ -14,10 +14,7 @@ private import internal.FlowSteps
  *
  * Identical to `TPathSummary` except without flow labels.
  */
-private newtype TStepSummary = MkStepSummary(boolean hasReturn, boolean hasCall) {
-  (hasReturn = true or hasReturn = false) and
-  (hasCall = true or hasCall = false)
-}
+private newtype TStepSummary = MkStepSummary(Boolean hasReturn, Boolean hasCall)
 
 /**
  * INTERNAL: Use `TypeTracker` or `TypeBackTracker` instead.
@@ -37,27 +34,6 @@ class StepSummary extends TStepSummary {
   /** Indicates whether the path represented by this summary contains any call steps. */
   boolean hasCall() { result = hasCall }
 
-  /**
-   * Gets the summary for the path obtained by appending `that` to `this`.
-   *
-   * Note that a path containing a `return` step cannot be appended to a path containing
-   * a `call` step in order to maintain well-formedness.
-   */
-  StepSummary append(StepSummary that) {
-    exists(Boolean hasReturn2, Boolean hasCall2 |
-      that = MkStepSummary(hasReturn2, hasCall2)
-    |
-      result = MkStepSummary(hasReturn.booleanOr(hasReturn2), hasCall.booleanOr(hasCall2)) and
-      // avoid constructing invalid paths
-      not (hasCall = true and hasReturn2 = true)
-    )
-  }
-
-  /**
-   * Gets the summary for the path obtained by appending `this` to `that`.
-   */
-  StepSummary prepend(StepSummary that) { result = that.append(this) }
-
   /** Gets a textual representation of this path summary. */
   string toString() {
     exists(string withReturn, string withCall |
@@ -133,9 +109,7 @@ module StepSummary {
   }
 }
 
-private newtype TTypeTracker = MkTypeTracker(boolean hasCall) {
-  hasCall = true or hasCall = false
-}
+private newtype TTypeTracker = MkTypeTracker(Boolean hasCall)
 
 /**
  * EXPERIMENTAL.
@@ -193,9 +167,7 @@ class TypeTracker extends TTypeTracker {
   }
 }
 
-private newtype TTypeBackTracker = MkTypeBackTracker(boolean hasReturn) {
-  hasReturn = true or hasReturn = false
-}
+private newtype TTypeBackTracker = MkTypeBackTracker(Boolean hasReturn)
 
 /**
  * EXPERIMENTAL.

From 9fbc0eb7179e923e913c3b1cb3ddf88353f0b53c Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Thu, 21 Mar 2019 08:39:24 +0000
Subject: [PATCH 09/12] JavaScript: Switch from path summaries to step
 summaries for type tracking.

This is sufficient since we are not doing summarisation.
---
 .../javascript/dataflow/TypeTracking.qll      | 81 ++++++-------------
 .../TypeTracking/PredicateStyle.expected      |  1 +
 2 files changed, 27 insertions(+), 55 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
index ac2a6590982..1c06cf564a7 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
@@ -10,81 +10,60 @@ import javascript
 private import internal.FlowSteps
 
 /**
- * A pair of booleans, indicating whether a path goes through a return and/or a call.
- *
- * Identical to `TPathSummary` except without flow labels.
+ * A description of a step on an inter-procedural data flow path.
  */
-private newtype TStepSummary = MkStepSummary(Boolean hasReturn, Boolean hasCall)
+private newtype TStepSummary =
+  LevelStep() or
+  CallStep() or
+  ReturnStep()
 
 /**
  * INTERNAL: Use `TypeTracker` or `TypeBackTracker` instead.
  *
- * Summary of the steps needed to track a value to a given dataflow node.
+ * A description of a step on an inter-procedural data flow path.
  */
 class StepSummary extends TStepSummary {
-  Boolean hasReturn;
+  /** Indicates whether the step represented by this summary is a return. */
+  boolean hasReturn() { if this instanceof ReturnStep then result = true else result = false }
 
-  Boolean hasCall;
+  /** Indicates whether the step represented by this summary is a call. */
+  boolean hasCall() { if this instanceof CallStep then result = true else result = false }
 
-  StepSummary() { this = MkStepSummary(hasReturn, hasCall) }
-
-  /** Indicates whether the path represented by this summary contains any return steps. */
-  boolean hasReturn() { result = hasReturn }
-
-  /** Indicates whether the path represented by this summary contains any call steps. */
-  boolean hasCall() { result = hasCall }
-
-  /** Gets a textual representation of this path summary. */
+  /** Gets a textual representation of this step summary. */
   string toString() {
-    exists(string withReturn, string withCall |
-      (if hasReturn = true then withReturn = "with" else withReturn = "without") and
-      (if hasCall = true then withCall = "with" else withCall = "without")
-    |
-      result = "path " + withReturn + " return steps and " + withCall + " call steps"
-    )
+    this instanceof LevelStep and result = "level"
+    or
+    this instanceof CallStep and result = "call"
+    or
+    this instanceof ReturnStep and result = "return"
   }
 }
 
 module StepSummary {
-  /**
-   * Gets a summary describing a path without any calls or returns.
-   */
-  StepSummary level() { result = MkStepSummary(false, false) }
-
-  /**
-   * Gets a summary describing a path with one or more calls, but no returns.
-   */
-  StepSummary call() { result = MkStepSummary(false, true) }
-
-  /**
-   * Gets a summary describing a path with one or more returns, but no calls.
-   */
-  StepSummary return() { result = MkStepSummary(true, false) }
-
   /**
    * INTERNAL: Use `SourceNode.track()` or `SourceNode.backtrack()` instead.
    */
   predicate step(DataFlow::SourceNode pred, DataFlow::SourceNode succ, StepSummary summary) {
-    exists (DataFlow::Node predNode | pred.flowsTo(predNode) |
+    exists(DataFlow::Node predNode | pred.flowsTo(predNode) |
       // Flow through properties of objects
       propertyFlowStep(predNode, succ) and
-      summary = level()
+      summary = LevelStep()
       or
       // Flow through global variables
       globalFlowStep(predNode, succ) and
-      summary = level()
+      summary = LevelStep()
       or
       // Flow into function
       callStep(predNode, succ) and
-      summary = call()
+      summary = CallStep()
       or
       // Flow out of function
       returnStep(predNode, succ) and
-      summary = return()
+      summary = ReturnStep()
       or
       // Flow through an instance field between members of the same class
       DataFlow::localFieldStep(predNode, succ) and
-      summary = level()
+      summary = LevelStep()
     )
   }
 
@@ -153,18 +132,14 @@ class TypeTracker extends TTypeTracker {
   /**
    * Holds if this is the starting point of type tracking.
    */
-  predicate start() {
-    hasCall = false
-  }
+  predicate start() { hasCall = false }
 
   /**
    * INTERNAL. DO NOT USE.
    *
    * Holds if this type has been tracked into a call.
    */
-  boolean hasCall() {
-    result = hasCall
-  }
+  boolean hasCall() { result = hasCall }
 }
 
 private newtype TTypeBackTracker = MkTypeBackTracker(Boolean hasReturn)
@@ -210,16 +185,12 @@ class TypeBackTracker extends TTypeBackTracker {
   /**
    * Holds if this is the starting point of type tracking.
    */
-  predicate start() {
-    hasReturn = false
-  }
+  predicate start() { hasReturn = false }
 
   /**
    * INTERNAL. DO NOT USE.
    *
    * Holds if this type has been back-tracked into a call through return edge.
    */
-  boolean hasReturn() {
-    result = hasReturn
-  }
+  boolean hasReturn() { result = hasReturn }
 }
diff --git a/javascript/ql/test/library-tests/TypeTracking/PredicateStyle.expected b/javascript/ql/test/library-tests/TypeTracking/PredicateStyle.expected
index 0c0fea7a928..8507edce86f 100644
--- a/javascript/ql/test/library-tests/TypeTracking/PredicateStyle.expected
+++ b/javascript/ql/test/library-tests/TypeTracking/PredicateStyle.expected
@@ -5,6 +5,7 @@ apiObject
 connection
 | type tracker with call steps | tst.js:6:15:6:18 | conn |
 | type tracker with call steps | tst.js:10:5:10:19 | this.connection |
+| type tracker with call steps with property connection | tst.js:6:14:6:13 | this |
 | type tracker without call steps | tst.js:15:10:15:49 | api.cha ... ction() |
 | type tracker without call steps | tst.js:18:7:18:21 | getConnection() |
 | type tracker without call steps | tst.js:30:9:30:23 | getConnection() |

From 084159dcfde68c5e5655af2c288b64272d74b91f Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Thu, 21 Mar 2019 10:16:57 +0000
Subject: [PATCH 10/12] JavaScript: Teach type trackers to track flow through
 one level of properties.

---
 .../javascript/dataflow/TypeTracking.qll      | 106 ++++++++++++++----
 .../semmle/javascript/frameworks/Connect.qll  |   2 +-
 .../semmle/javascript/frameworks/Electron.qll |   2 +-
 .../semmle/javascript/frameworks/Express.qll  |   2 +-
 .../src/semmle/javascript/frameworks/HTTP.qll |   4 +-
 .../src/semmle/javascript/frameworks/Hapi.qll |   2 +-
 .../src/semmle/javascript/frameworks/Koa.qll  |   2 +-
 .../javascript/frameworks/NodeJSLib.qll       |   2 +-
 .../semmle/javascript/frameworks/SocketIO.qll |   8 +-
 .../library-tests/TypeTracking/ClassStyle.ql  |  54 +++------
 .../TypeTracking/PredicateStyle.ql            |  32 ++----
 .../HTTP-heuristics/RouteHandler.expected     |   2 +
 .../UnpromotedRouteHandlerCandidate.expected  |   2 -
 13 files changed, 128 insertions(+), 92 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
index 1c06cf564a7..b2decce2abe 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
@@ -9,13 +9,19 @@
 import javascript
 private import internal.FlowSteps
 
+private class PropertyName extends string {
+  PropertyName() { this = any(DataFlow::PropRef pr).getPropertyName() }
+}
+
 /**
  * A description of a step on an inter-procedural data flow path.
  */
 private newtype TStepSummary =
   LevelStep() or
   CallStep() or
-  ReturnStep()
+  ReturnStep() or
+  StoreStep(PropertyName prop) or
+  LoadStep(PropertyName prop)
 
 /**
  * INTERNAL: Use `TypeTracker` or `TypeBackTracker` instead.
@@ -64,6 +70,14 @@ module StepSummary {
       // Flow through an instance field between members of the same class
       DataFlow::localFieldStep(predNode, succ) and
       summary = LevelStep()
+      or
+      exists(string prop |
+        basicStoreStep(predNode, succ, prop) and
+        summary = StoreStep(prop)
+        or
+        loadStep(predNode, succ, prop) and
+        summary = LoadStep(prop)
+      )
     )
   }
 
@@ -73,8 +87,22 @@ module StepSummary {
    * Appends a step summary onto a type-tracking summary.
    */
   TypeTracker append(TypeTracker type, StepSummary summary) {
-    not (type.hasCall() = true and summary.hasReturn() = true) and
-    result.hasCall() = type.hasCall().booleanOr(summary.hasCall())
+    exists(boolean hadCall, boolean hasCall, string oldProp, string newProp |
+      hadCall = type.hasCall() and
+      oldProp = type.getProp()
+    |
+      not (hadCall = true and summary.hasReturn() = true) and
+      hasCall = hadCall.booleanOr(summary.hasCall()) and
+      (
+        if summary instanceof StoreStep
+        then oldProp = "" and summary = StoreStep(newProp)
+        else
+          if summary instanceof LoadStep
+          then summary = LoadStep(oldProp) and newProp = ""
+          else newProp = oldProp
+      ) and
+      result = MkTypeTracker(hasCall, newProp)
+    )
   }
 
   /**
@@ -83,12 +111,27 @@ module StepSummary {
    * Prepends a step summary before a backwards type-tracking summary.
    */
   TypeBackTracker prepend(StepSummary summary, TypeBackTracker type) {
-    not (type.hasReturn() = true and summary.hasCall() = true) and
-    result.hasReturn() = type.hasReturn().booleanOr(summary.hasReturn())
+    exists(boolean hadReturn, boolean hasReturn, string oldProp, string newProp |
+      hadReturn = type.hasReturn() and
+      oldProp = type.getProp()
+    |
+      not (hadReturn = true and summary.hasCall() = true) and
+      hasReturn = hadReturn.booleanOr(summary.hasReturn()) and
+      (
+        if summary instanceof StoreStep
+        then summary = StoreStep(oldProp) and newProp = ""
+        else
+          if summary instanceof LoadStep
+          then oldProp = "" and summary = LoadStep(newProp)
+          else newProp = oldProp
+      ) and
+      result = MkTypeBackTracker(hasReturn, newProp)
+    )
   }
 }
 
-private newtype TTypeTracker = MkTypeTracker(Boolean hasCall)
+private newtype TTypeTracker =
+  MkTypeTracker(Boolean hasCall, string prop) { prop = "" or prop instanceof PropertyName }
 
 /**
  * EXPERIMENTAL.
@@ -112,7 +155,7 @@ private newtype TTypeTracker = MkTypeTracker(Boolean hasCall)
  *   )
  * }
  *
- * DataFlow::SourceNode myType() { result = myType(_) }
+ * DataFlow::SourceNode myType() { result = myType(DataFlow::TypeTracker::end()) }
  * ```
  *
  * To track values backwards, which can be useful for tracking
@@ -121,18 +164,24 @@ private newtype TTypeTracker = MkTypeTracker(Boolean hasCall)
 class TypeTracker extends TTypeTracker {
   Boolean hasCall;
 
-  TypeTracker() { this = MkTypeTracker(hasCall) }
+  string prop;
+
+  TypeTracker() { this = MkTypeTracker(hasCall, prop) }
 
   string toString() {
-    hasCall = true and result = "type tracker with call steps"
-    or
-    hasCall = false and result = "type tracker without call steps"
+    exists(string withCall, string withProp |
+      (if hasCall = true then withCall = "with" else withCall = "without") and
+      (if prop != "" then withProp = " with property " + prop else withProp = "") and
+      result = "type tracker " + withCall + " call steps" + withProp
+    )
   }
 
   /**
    * Holds if this is the starting point of type tracking.
    */
-  predicate start() { hasCall = false }
+  predicate start() { hasCall = false and prop = "" }
+
+  predicate end() { prop = "" }
 
   /**
    * INTERNAL. DO NOT USE.
@@ -140,9 +189,16 @@ class TypeTracker extends TTypeTracker {
    * Holds if this type has been tracked into a call.
    */
   boolean hasCall() { result = hasCall }
+
+  string getProp() { result = prop }
 }
 
-private newtype TTypeBackTracker = MkTypeBackTracker(Boolean hasReturn)
+module TypeTracker {
+  TypeTracker end() { result.end() }
+}
+
+private newtype TTypeBackTracker =
+  MkTypeBackTracker(Boolean hasReturn, string prop) { prop = "" or prop instanceof PropertyName }
 
 /**
  * EXPERIMENTAL.
@@ -168,24 +224,30 @@ private newtype TTypeBackTracker = MkTypeBackTracker(Boolean hasReturn)
  *   )
  * }
  *
- * DataFlow::SourceNode myCallback() { result = myCallback(_) }
+ * DataFlow::SourceNode myCallback() { result = myCallback(DataFlow::TypeBackTracker::end()) }
  * ```
  */
 class TypeBackTracker extends TTypeBackTracker {
   Boolean hasReturn;
 
-  TypeBackTracker() { this = MkTypeBackTracker(hasReturn) }
+  string prop;
+
+  TypeBackTracker() { this = MkTypeBackTracker(hasReturn, prop) }
 
   string toString() {
-    hasReturn = true and result = "type back-tracker with return steps"
-    or
-    hasReturn = false and result = "type back-tracker without return steps"
+    exists(string withReturn, string withProp |
+      (if hasReturn = true then withReturn = "with" else withReturn = "without") and
+      (if prop != "" then withProp = " with property " + prop else withProp = "") and
+      result = "type back-tracker " + withReturn + " return steps" + withProp
+    )
   }
 
   /**
    * Holds if this is the starting point of type tracking.
    */
-  predicate start() { hasReturn = false }
+  predicate start() { hasReturn = false and prop = "" }
+
+  predicate end() { prop = "" }
 
   /**
    * INTERNAL. DO NOT USE.
@@ -193,4 +255,10 @@ class TypeBackTracker extends TTypeBackTracker {
    * Holds if this type has been back-tracked into a call through return edge.
    */
   boolean hasReturn() { result = hasReturn }
+
+  string getProp() { result = prop }
+}
+
+module TypeBackTracker {
+  TypeBackTracker end() { result.end() }
 }
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Connect.qll b/javascript/ql/src/semmle/javascript/frameworks/Connect.qll
index f01a995b9ef..31e561d66b0 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Connect.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Connect.qll
@@ -118,7 +118,7 @@ module Connect {
     }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result = getARouteHandler(_)
+      result = getARouteHandler(DataFlow::TypeBackTracker::end())
     }
 
     private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
index b2e6ff09ced..8ea429110ad 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
@@ -69,7 +69,7 @@ module Electron {
    * A data flow node whose value may originate from a browser object instantiation.
    */
   private class BrowserObjectByFlow extends BrowserObject {
-    BrowserObjectByFlow() { browserObject(_).flowsTo(this) }
+    BrowserObjectByFlow() { browserObject(DataFlow::TypeTracker::end()).flowsTo(this) }
   }
 
   /**
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
index 3b1b8e53ef3..8f68d437786 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
@@ -111,7 +111,7 @@ module Express {
     Expr getLastRouteHandlerExpr() { result = max(int i | | getRouteHandlerExpr(i) order by i) }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result = getARouteHandler(_)
+      result = getARouteHandler(DataFlow::TypeBackTracker::end())
     }
 
     private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
diff --git a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
index 6bce331e7f3..2ac592c20d6 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
@@ -282,7 +282,7 @@ module HTTP {
        */
       abstract RouteHandler getRouteHandler();
 
-      predicate flowsTo(DataFlow::Node nd) { flowsToSourceNode(_).flowsTo(nd) }
+      predicate flowsTo(DataFlow::Node nd) { flowsToSourceNode(DataFlow::TypeTracker::end()).flowsTo(nd) }
 
       private DataFlow::SourceNode flowsToSourceNode(DataFlow::TypeTracker t) {
         t.start() and
@@ -303,7 +303,7 @@ module HTTP {
        */
       abstract RouteHandler getRouteHandler();
 
-      predicate flowsTo(DataFlow::Node nd) { flowsToSourceNode(_).flowsTo(nd) }
+      predicate flowsTo(DataFlow::Node nd) { flowsToSourceNode(DataFlow::TypeTracker::end()).flowsTo(nd) }
 
       private DataFlow::SourceNode flowsToSourceNode(DataFlow::TypeTracker t) {
         t.start() and
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll b/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll
index ab553a12f29..5bca1aa5378 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll
@@ -193,7 +193,7 @@ module Hapi {
     }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result = getARouteHandler(_)
+      result = getARouteHandler(DataFlow::TypeBackTracker::end())
     }
 
     private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Koa.qll b/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
index 6c3e9f7848c..34ef023c815 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
@@ -79,7 +79,7 @@ module Koa {
     RouteHandler getRouteHandler() { result = rh }
 
     predicate flowsTo(DataFlow::Node nd) {
-      flowsToSourceNode(_).flowsTo(nd)
+      flowsToSourceNode(DataFlow::TypeTracker::end()).flowsTo(nd)
     }
 
     private DataFlow::SourceNode flowsToSourceNode(DataFlow::TypeTracker t) {
diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
index a1550f2dddb..f578f2eb5c9 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
@@ -189,7 +189,7 @@ module NodeJSLib {
     }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result = getARouteHandler(_)
+      result = getARouteHandler(DataFlow::TypeBackTracker::end())
     }
 
     private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
diff --git a/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll b/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll
index 1d83be0755f..a42f028b48c 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll
@@ -61,7 +61,7 @@ module SocketIO {
   class ServerNode extends DataFlow::SourceNode {
     ServerObject srv;
 
-    ServerNode() { this = server(srv, _) }
+    ServerNode() { this = server(srv, DataFlow::TypeTracker::end()) }
 
     /** Gets the server to which this node refers. */
     ServerObject getServer() { result = srv }
@@ -130,7 +130,7 @@ module SocketIO {
   class NamespaceNode extends DataFlow::SourceNode {
     NamespaceObject ns;
 
-    NamespaceNode() { this = namespace(ns, _) }
+    NamespaceNode() { this = namespace(ns, DataFlow::TypeTracker::end()) }
 
     /** Gets the namespace to which this node refers. */
     NamespaceObject getNamespace() { result = ns }
@@ -194,7 +194,7 @@ module SocketIO {
   class SocketNode extends DataFlow::SourceNode {
     NamespaceObject ns;
 
-    SocketNode() { this = socket(ns, _) }
+    SocketNode() { this = socket(ns, DataFlow::TypeTracker::end()) }
 
     /** Gets the namespace to which this socket belongs. */
     NamespaceObject getNamespace() { result = ns }
@@ -417,7 +417,7 @@ module SocketIOClient {
   class SocketNode extends DataFlow::SourceNode {
     DataFlow::InvokeNode invk;
 
-    SocketNode() { this = socket(invk, _) }
+    SocketNode() { this = socket(invk, DataFlow::TypeTracker::end()) }
 
     /** Gets the path of the namespace this socket belongs to, if it can be determined. */
     string getNamespacePath() {
diff --git a/javascript/ql/test/library-tests/TypeTracking/ClassStyle.ql b/javascript/ql/test/library-tests/TypeTracking/ClassStyle.ql
index f965a40102f..6bf92c53424 100644
--- a/javascript/ql/test/library-tests/TypeTracking/ClassStyle.ql
+++ b/javascript/ql/test/library-tests/TypeTracking/ClassStyle.ql
@@ -6,86 +6,68 @@ string chainableMethod() {
 }
 
 class ApiObject extends DataFlow::NewNode {
-  ApiObject() {
-    this = DataFlow::moduleImport("@test/myapi").getAnInstantiation()
-  }
+  ApiObject() { this = DataFlow::moduleImport("@test/myapi").getAnInstantiation() }
 
   DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
     t.start() and
     result = this
     or
     t.start() and
-    result = ref(_).getAMethodCall(chainableMethod())
+    result = ref(DataFlow::TypeTracker::end()).getAMethodCall(chainableMethod())
     or
-    exists(DataFlow::TypeTracker t2 |
-      result = ref(t2).track(t2, t)
-    )
-  }
-  
-  DataFlow::SourceNode ref() {
-    result = ref(_)
+    exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
   }
+
+  DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
 }
 
 class Connection extends DataFlow::SourceNode {
   ApiObject api;
 
-  Connection() {
-    this = api.ref().getAMethodCall("createConnection")
-  }
+  Connection() { this = api.ref().getAMethodCall("createConnection") }
 
   DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
     t.start() and
     result = this
     or
-    exists(DataFlow::TypeTracker t2 |
-      result = ref(t2).track(t2, t)
-    )
-  }
-  
-  DataFlow::SourceNode ref() {
-    result = ref(_)
+    exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
   }
 
+  DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
+
   DataFlow::SourceNode getACallbackNode(DataFlow::TypeBackTracker t) {
     t.start() and
     result = ref().getAMethodCall("getData").getArgument(0).getALocalSource()
     or
-    exists(DataFlow::TypeBackTracker t2 |
-      result = getACallbackNode(t2).backtrack(t2, t)
-    )
+    exists(DataFlow::TypeBackTracker t2 | result = getACallbackNode(t2).backtrack(t2, t))
   }
 
   DataFlow::FunctionNode getACallback() {
-    result = getACallbackNode(_).getAFunctionValue()
+    result = getACallbackNode(DataFlow::TypeBackTracker::end()).getAFunctionValue()
   }
 }
 
 class DataValue extends DataFlow::SourceNode {
   Connection connection;
 
-  DataValue() {
-    this = connection.getACallback().getParameter(0)
-  }
+  DataValue() { this = connection.getACallback().getParameter(0) }
 
   DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
     t.start() and
     result = this
     or
-    exists(DataFlow::TypeTracker t2 |
-      result = ref(t2).track(t2, t)
-    )
-  }
-  
-  DataFlow::SourceNode ref() {
-    result = ref(_)
+    exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
   }
+
+  DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
 }
 
 query DataFlow::SourceNode test_ApiObject() { result = any(ApiObject obj).ref() }
 
 query DataFlow::SourceNode test_Connection() { result = any(Connection c).ref() }
 
-query DataFlow::SourceNode test_DataCallback() { result = any(Connection c).getACallbackNode(_) }
+query DataFlow::SourceNode test_DataCallback() {
+  result = any(Connection c).getACallbackNode(DataFlow::TypeBackTracker::end())
+}
 
 query DataFlow::SourceNode test_DataValue() { result = any(DataValue v).ref() }
diff --git a/javascript/ql/test/library-tests/TypeTracking/PredicateStyle.ql b/javascript/ql/test/library-tests/TypeTracking/PredicateStyle.ql
index 678dc12de0a..9f265ecc47c 100644
--- a/javascript/ql/test/library-tests/TypeTracking/PredicateStyle.ql
+++ b/javascript/ql/test/library-tests/TypeTracking/PredicateStyle.ql
@@ -10,52 +10,38 @@ DataFlow::SourceNode apiObject(DataFlow::TypeTracker t) {
   result = DataFlow::moduleImport("@test/myapi").getAnInstantiation()
   or
   t.start() and
-  result = apiObject(_).getAMethodCall(chainableMethod())
+  result = apiObject(DataFlow::TypeTracker::end()).getAMethodCall(chainableMethod())
   or
-  exists(DataFlow::TypeTracker t2 |
-    result = apiObject(t2).track(t2, t)
-  )
+  exists(DataFlow::TypeTracker t2 | result = apiObject(t2).track(t2, t))
 }
 
-query DataFlow::SourceNode apiObject() {
-  result = apiObject(_)
-}
+query DataFlow::SourceNode apiObject() { result = apiObject(DataFlow::TypeTracker::end()) }
 
 query DataFlow::SourceNode connection(DataFlow::TypeTracker t) {
   t.start() and
   result = apiObject().getAMethodCall("createConnection")
   or
-  exists(DataFlow::TypeTracker t2 |
-    result = connection(t2).track(t2, t)
-  )
+  exists(DataFlow::TypeTracker t2 | result = connection(t2).track(t2, t))
 }
 
-DataFlow::SourceNode connection() {
-  result = connection(_)
-}
+DataFlow::SourceNode connection() { result = connection(DataFlow::TypeTracker::end()) }
 
 DataFlow::SourceNode dataCallback(DataFlow::TypeBackTracker t) {
   t.start() and
   result = connection().getAMethodCall("getData").getArgument(0).getALocalSource()
   or
-  exists(DataFlow::TypeBackTracker t2 |
-    result = dataCallback(t2).backtrack(t2, t)
-  )
+  exists(DataFlow::TypeBackTracker t2 | result = dataCallback(t2).backtrack(t2, t))
 }
 
 query DataFlow::SourceNode dataCallback() {
-  result = dataCallback(_)
+  result = dataCallback(DataFlow::TypeBackTracker::end())
 }
 
 DataFlow::SourceNode dataValue(DataFlow::TypeTracker t) {
   t.start() and
   result = dataCallback().getAFunctionValue().getParameter(0)
   or
-  exists(DataFlow::TypeTracker t2 |
-    result = dataValue(t2).track(t2, t)
-  )
+  exists(DataFlow::TypeTracker t2 | result = dataValue(t2).track(t2, t))
 }
 
-query DataFlow::SourceNode dataValue() {
-  result = dataValue(_)
-}
+query DataFlow::SourceNode dataValue() { result = dataValue(DataFlow::TypeTracker::end()) }
diff --git a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/RouteHandler.expected b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/RouteHandler.expected
index a5a22b0cf84..4f21ca278f4 100644
--- a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/RouteHandler.expected
+++ b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/RouteHandler.expected
@@ -11,6 +11,8 @@
 | src/exported-handler.js:1:19:1:55 | functio ... res) {} |
 | src/exported-middleware-attacher-2.js:2:13:2:32 | function(req, res){} |
 | src/exported-middleware-attacher.js:2:13:2:32 | function(req, res){} |
+| src/handler-in-property.js:5:14:5:33 | function(req, res){} |
+| src/handler-in-property.js:12:18:12:37 | function(req, res){} |
 | src/middleware-attacher-getter.js:4:17:4:36 | function(req, res){} |
 | src/middleware-attacher-getter.js:19:19:19:38 | function(req, res){} |
 | src/middleware-attacher.js:3:13:3:32 | function(req, res){} |
diff --git a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected
index 12dfcd4af93..62cf78ad113 100644
--- a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected
+++ b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected
@@ -1,7 +1,5 @@
 | src/bound-handler.js:4:1:4:28 | functio ...  res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/bound-handler.js:9:12:9:31 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
-| src/handler-in-property.js:5:14:5:33 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
-| src/handler-in-property.js:12:18:12:37 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/hapi.js:1:1:1:30 | functio ... t, h){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/iterated-handlers.js:4:2:4:22 | functio ...  res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/middleware-attacher-getter.js:29:32:29:51 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |

From c50067b597a491fa2029f091b51ebf4b3647f422 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Thu, 21 Mar 2019 16:12:14 +0000
Subject: [PATCH 11/12] JavaScript: Refactor type tracking to avoid computing
 very large relations.

---
 .../semmle/javascript/dataflow/Sources.qll    |  8 +-
 .../javascript/dataflow/TypeTracking.qll      | 98 ++++++++-----------
 2 files changed, 46 insertions(+), 60 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll
index d424f98f58c..d51c544217d 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll
@@ -162,11 +162,11 @@ class SourceNode extends DataFlow::Node {
    *
    * See `TypeTracker` for more details about how to use this.
    */
-  cached
+  pragma[inline]
   DataFlow::SourceNode track(TypeTracker t2, TypeTracker t) {
     exists(StepSummary summary |
       StepSummary::step(this, result, summary) and
-      t = StepSummary::append(t2, summary)
+      t = t2.append(summary)
     )
   }
 
@@ -177,11 +177,11 @@ class SourceNode extends DataFlow::Node {
    *
    * See `TypeBackTracker` for more details about how to use this.
    */
-  cached
+  pragma[inline]
   DataFlow::SourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) {
     exists(StepSummary summary |
       StepSummary::step(result, this, summary) and
-      t = StepSummary::prepend(summary, t2)
+      t = t2.prepend(summary)
     )
   }
 }
diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
index b2decce2abe..2164426f6af 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
@@ -13,6 +13,10 @@ private class PropertyName extends string {
   PropertyName() { this = any(DataFlow::PropRef pr).getPropertyName() }
 }
 
+private class OptionalPropertyName extends string {
+  OptionalPropertyName() { this instanceof PropertyName or this = "" }
+}
+
 /**
  * A description of a step on an inter-procedural data flow path.
  */
@@ -29,12 +33,6 @@ private newtype TStepSummary =
  * A description of a step on an inter-procedural data flow path.
  */
 class StepSummary extends TStepSummary {
-  /** Indicates whether the step represented by this summary is a return. */
-  boolean hasReturn() { if this instanceof ReturnStep then result = true else result = false }
-
-  /** Indicates whether the step represented by this summary is a call. */
-  boolean hasCall() { if this instanceof CallStep then result = true else result = false }
-
   /** Gets a textual representation of this step summary. */
   string toString() {
     this instanceof LevelStep and result = "level"
@@ -42,6 +40,14 @@ class StepSummary extends TStepSummary {
     this instanceof CallStep and result = "call"
     or
     this instanceof ReturnStep and result = "return"
+    or
+    exists(string prop | this = StoreStep(prop) |
+      result = "store " + prop
+    )
+    or
+    exists(string prop | this = LoadStep(prop) |
+      result = "load" + prop
+    )
   }
 }
 
@@ -80,58 +86,10 @@ module StepSummary {
       )
     )
   }
-
-  /**
-   * INTERNAL. Do not use.
-   *
-   * Appends a step summary onto a type-tracking summary.
-   */
-  TypeTracker append(TypeTracker type, StepSummary summary) {
-    exists(boolean hadCall, boolean hasCall, string oldProp, string newProp |
-      hadCall = type.hasCall() and
-      oldProp = type.getProp()
-    |
-      not (hadCall = true and summary.hasReturn() = true) and
-      hasCall = hadCall.booleanOr(summary.hasCall()) and
-      (
-        if summary instanceof StoreStep
-        then oldProp = "" and summary = StoreStep(newProp)
-        else
-          if summary instanceof LoadStep
-          then summary = LoadStep(oldProp) and newProp = ""
-          else newProp = oldProp
-      ) and
-      result = MkTypeTracker(hasCall, newProp)
-    )
-  }
-
-  /**
-   * INTERNAL. Do not use.
-   *
-   * Prepends a step summary before a backwards type-tracking summary.
-   */
-  TypeBackTracker prepend(StepSummary summary, TypeBackTracker type) {
-    exists(boolean hadReturn, boolean hasReturn, string oldProp, string newProp |
-      hadReturn = type.hasReturn() and
-      oldProp = type.getProp()
-    |
-      not (hadReturn = true and summary.hasCall() = true) and
-      hasReturn = hadReturn.booleanOr(summary.hasReturn()) and
-      (
-        if summary instanceof StoreStep
-        then summary = StoreStep(oldProp) and newProp = ""
-        else
-          if summary instanceof LoadStep
-          then oldProp = "" and summary = LoadStep(newProp)
-          else newProp = oldProp
-      ) and
-      result = MkTypeBackTracker(hasReturn, newProp)
-    )
-  }
 }
 
 private newtype TTypeTracker =
-  MkTypeTracker(Boolean hasCall, string prop) { prop = "" or prop instanceof PropertyName }
+  MkTypeTracker(Boolean hasCall, OptionalPropertyName prop)
 
 /**
  * EXPERIMENTAL.
@@ -168,6 +126,20 @@ class TypeTracker extends TTypeTracker {
 
   TypeTracker() { this = MkTypeTracker(hasCall, prop) }
 
+  TypeTracker append(StepSummary step) {
+    step = LevelStep() and result = this
+    or
+    step = CallStep() and result = MkTypeTracker(true, prop)
+    or
+    step = ReturnStep() and hasCall = false and result = this
+    or
+    step = LoadStep(prop) and result = MkTypeTracker(hasCall, "")
+    or
+    exists(string p |
+      step = StoreStep(p) and prop = "" and result = MkTypeTracker(hasCall, p)
+    )
+  }
+
   string toString() {
     exists(string withCall, string withProp |
       (if hasCall = true then withCall = "with" else withCall = "without") and
@@ -198,7 +170,7 @@ module TypeTracker {
 }
 
 private newtype TTypeBackTracker =
-  MkTypeBackTracker(Boolean hasReturn, string prop) { prop = "" or prop instanceof PropertyName }
+  MkTypeBackTracker(Boolean hasReturn, OptionalPropertyName prop)
 
 /**
  * EXPERIMENTAL.
@@ -234,6 +206,20 @@ class TypeBackTracker extends TTypeBackTracker {
 
   TypeBackTracker() { this = MkTypeBackTracker(hasReturn, prop) }
 
+  TypeBackTracker prepend(StepSummary step) {
+    step = LevelStep() and result = this
+    or
+    step = CallStep() and hasReturn = false and result = this
+    or
+    step = ReturnStep() and result = MkTypeBackTracker(true, prop)
+    or
+    exists(string p |
+      step = LoadStep(p) and prop = "" and result = MkTypeBackTracker(hasReturn, p)
+    )
+    or
+    step = StoreStep(prop) and result = MkTypeBackTracker(hasReturn, "")
+  }
+
   string toString() {
     exists(string withReturn, string withProp |
       (if hasReturn = true then withReturn = "with" else withReturn = "without") and

From 3e16d16525c88e647e9b427fcd6d3a59269f3022 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Tue, 26 Mar 2019 12:56:36 +0000
Subject: [PATCH 12/12] JavaScript: Make type tracking-related parameter and
 predicate names more consistent.

---
 .../semmle/javascript/frameworks/Connect.qll  |  4 +--
 .../semmle/javascript/frameworks/Electron.qll |  4 +--
 .../semmle/javascript/frameworks/Express.qll  |  4 +--
 .../src/semmle/javascript/frameworks/HTTP.qll | 12 +++----
 .../src/semmle/javascript/frameworks/Hapi.qll |  4 +--
 .../src/semmle/javascript/frameworks/Koa.qll  |  8 ++---
 .../javascript/frameworks/NodeJSLib.qll       |  4 +--
 .../semmle/javascript/frameworks/SocketIO.qll | 36 +++++++++----------
 8 files changed, 36 insertions(+), 40 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Connect.qll b/javascript/ql/src/semmle/javascript/frameworks/Connect.qll
index 31e561d66b0..dd5da051650 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Connect.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Connect.qll
@@ -125,8 +125,8 @@ module Connect {
       t.start() and
       result = getARouteHandlerExpr().flow().getALocalSource()
       or
-      exists(DataFlow::TypeBackTracker next |
-        result = getARouteHandler(next).backtrack(next, t)
+      exists(DataFlow::TypeBackTracker t2 |
+        result = getARouteHandler(t2).backtrack(t2, t)
       )
     }
 
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
index 8ea429110ad..d050338f2d8 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
@@ -60,8 +60,8 @@ module Electron {
     t.start() and
     result instanceof NewBrowserObject
     or
-    exists(DataFlow::TypeTracker prev |
-      result = browserObject(prev).track(prev, t)
+    exists(DataFlow::TypeTracker t2 |
+      result = browserObject(t2).track(t2, t)
     )
   }
 
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
index 8f68d437786..9bf2399e2ed 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
@@ -118,8 +118,8 @@ module Express {
       t.start() and
       result = getARouteHandlerExpr().flow().getALocalSource()
       or
-      exists(DataFlow::TypeBackTracker next |
-        result = getARouteHandler(next).backtrack(next, t)
+      exists(DataFlow::TypeBackTracker t2 |
+        result = getARouteHandler(t2).backtrack(t2, t)
       )
     }
 
diff --git a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
index 2ac592c20d6..17a41c8ba3d 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
@@ -282,13 +282,13 @@ module HTTP {
        */
       abstract RouteHandler getRouteHandler();
 
-      predicate flowsTo(DataFlow::Node nd) { flowsToSourceNode(DataFlow::TypeTracker::end()).flowsTo(nd) }
+      predicate flowsTo(DataFlow::Node nd) { ref(DataFlow::TypeTracker::end()).flowsTo(nd) }
 
-      private DataFlow::SourceNode flowsToSourceNode(DataFlow::TypeTracker t) {
+      private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
         t.start() and
         result = this
         or
-        exists(DataFlow::TypeTracker prev | result = flowsToSourceNode(prev).track(prev, t))
+        exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
       }
     }
 
@@ -303,13 +303,13 @@ module HTTP {
        */
       abstract RouteHandler getRouteHandler();
 
-      predicate flowsTo(DataFlow::Node nd) { flowsToSourceNode(DataFlow::TypeTracker::end()).flowsTo(nd) }
+      predicate flowsTo(DataFlow::Node nd) { ref(DataFlow::TypeTracker::end()).flowsTo(nd) }
 
-      private DataFlow::SourceNode flowsToSourceNode(DataFlow::TypeTracker t) {
+      private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
         t.start() and
         result = this
         or
-        exists(DataFlow::TypeTracker prev | result = flowsToSourceNode(prev).track(prev, t))
+        exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
       }
     }
 
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll b/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll
index 5bca1aa5378..2ace343d6ff 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Hapi.qll
@@ -200,8 +200,8 @@ module Hapi {
       t.start() and
       result = handler.flow().getALocalSource()
       or
-      exists(DataFlow::TypeBackTracker next |
-        result = getARouteHandler(next).backtrack(next, t)
+      exists(DataFlow::TypeBackTracker t2 |
+        result = getARouteHandler(t2).backtrack(t2, t)
       )
     }
 
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Koa.qll b/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
index 34ef023c815..20d80059ff7 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
@@ -79,15 +79,15 @@ module Koa {
     RouteHandler getRouteHandler() { result = rh }
 
     predicate flowsTo(DataFlow::Node nd) {
-      flowsToSourceNode(DataFlow::TypeTracker::end()).flowsTo(nd)
+      ref(DataFlow::TypeTracker::end()).flowsTo(nd)
     }
 
-    private DataFlow::SourceNode flowsToSourceNode(DataFlow::TypeTracker t) {
+    private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
       t.start() and
       result = this
       or
-      exists(DataFlow::TypeTracker prev |
-        result = flowsToSourceNode(prev).track(prev, t)
+      exists(DataFlow::TypeTracker t2 |
+        result = ref(t2).track(t2, t)
       )
     }
   }
diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
index f578f2eb5c9..01528080f46 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
@@ -196,8 +196,8 @@ module NodeJSLib {
       t.start() and
       result = handler.flow().getALocalSource()
       or
-      exists(DataFlow::TypeBackTracker next |
-        result = getARouteHandler(next).backtrack(next, t)
+      exists(DataFlow::TypeBackTracker t2 |
+        result = getARouteHandler(t2).backtrack(t2, t)
       )
     }
 
diff --git a/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll b/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll
index a42f028b48c..c7264ab0b1d 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll
@@ -25,16 +25,15 @@ module SocketIO {
   }
 
   /**
-   * Gets a data flow node that may refer to the socket.io server created at `srv`, with
-   * type tracking info stored in `t`.
+   * Gets a data flow node that may refer to the socket.io server created at `srv`.
    */
   private DataFlow::SourceNode server(ServerObject srv, DataFlow::TypeTracker t) {
     result = newServer() and
     srv = MkServer(result) and
     t.start()
     or
-    exists(DataFlow::TypeTracker s, DataFlow::SourceNode pred | pred = server(srv, s) |
-      result = pred.track(s, t)
+    exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred | pred = server(srv, t2) |
+      result = pred.track(t2, t)
       or
       // invocation of a chainable method
       exists(DataFlow::MethodCallNode mcn, string m |
@@ -52,7 +51,7 @@ module SocketIO {
         // exclude getter versions
         exists(mcn.getAnArgument()) and
         result = mcn and
-        t = s
+        t = t2
       )
     )
   }
@@ -85,8 +84,7 @@ module SocketIO {
   }
 
   /**
-   * Gets a data flow node that may refer to the socket.io namespace created at `ns`, with
-   * type tracking info stored in `t`.
+   * Gets a data flow node that may refer to the socket.io namespace created at `ns`.
    */
   private DataFlow::SourceNode namespace(NamespaceObject ns, DataFlow::TypeTracker t) {
     t.start() and
@@ -107,12 +105,12 @@ module SocketIO {
       ns = srv.getServer().getDefaultNamespace()
     )
     or
-    exists(DataFlow::SourceNode pred, DataFlow::TypeTracker s | pred = namespace(ns, s) |
-      result = pred.track(s, t)
+    exists(DataFlow::SourceNode pred, DataFlow::TypeTracker t2 | pred = namespace(ns, t2) |
+      result = pred.track(t2, t)
       or
       // invocation of a chainable method
       result = pred.getAMethodCall(namespaceChainableMethod()) and
-      t = s
+      t = t2
       or
       // invocation of chainable getter method
       exists(string m |
@@ -121,7 +119,7 @@ module SocketIO {
         m = "volatile"
       |
         result = pred.getAPropertyRead(m) and
-        t = s
+        t = t2
       )
     )
   }
@@ -137,8 +135,7 @@ module SocketIO {
   }
 
   /**
-   * Gets a data flow node that may refer to a socket.io socket belonging to namespace `ns`, with
-   * type tracking info stored in `t`.
+   * Gets a data flow node that may refer to a socket.io socket belonging to namespace `ns`.
    */
   private DataFlow::SourceNode socket(NamespaceObject ns, DataFlow::TypeTracker t) {
     // callback accepting a socket
@@ -155,8 +152,8 @@ module SocketIO {
       result = on.getCallback(1).getParameter(0)
     )
     or
-    exists(DataFlow::SourceNode pred, DataFlow::TypeTracker s | pred = socket(ns, s) |
-      result = pred.track(s, t)
+    exists(DataFlow::SourceNode pred, DataFlow::TypeTracker t2 | pred = socket(ns, t2) |
+      result = pred.track(t2, t)
       or
       // invocation of a chainable method
       exists(string m |
@@ -174,7 +171,7 @@ module SocketIO {
         m = EventEmitter::chainableMethod()
       |
         result = pred.getAMethodCall(m) and
-        t = s
+        t = t2
       )
       or
       // invocation of a chainable getter method
@@ -185,7 +182,7 @@ module SocketIO {
         m = "volatile"
       |
         result = pred.getAPropertyRead(m) and
-        t = s
+        t = t2
       )
     )
   }
@@ -395,8 +392,7 @@ module SocketIO {
  */
 module SocketIOClient {
   /**
-   * Gets a data flow node that may refer to the socket.io socket created at `invk`, with
-   * type tracking info stored in `t`.
+   * Gets a data flow node that may refer to the socket.io socket created at `invk`.
    */
   private DataFlow::SourceNode socket(DataFlow::InvokeNode invk, DataFlow::TypeTracker t) {
     t.start() and
@@ -410,7 +406,7 @@ module SocketIOClient {
       result = invk
     )
     or
-    exists(DataFlow::TypeTracker s | result = socket(invk, s).track(s, t))
+    exists(DataFlow::TypeTracker t2 | result = socket(invk, t2).track(t2, t))
   }
 
   /** A data flow node that may produce a socket object. */