Merge branch 'master' into js-team-sprint-merge2

javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java

@@ -77,8 +77,8 @@ import com.semmle.util.trap.TrapWriter;
  *   <li><code>LGTM_INDEX_EXCLUDE</code>: a newline-separated list of paths to exclude
  *   <li><code>LGTM_REPOSITORY_FOLDERS_CSV</code>: the path of a CSV file containing file
  *       classifications
- *   <li><code>LGTM_INDEX_FILTERS</code>: a newline-separated list of {@link ProjectLayout}-style
- *       patterns that can be used to refine the list of files to include and exclude
+ *   <li><code>LGTM_INDEX_FILTERS</code>: a newline-separated list of strings of form "include:PATTERN"
+ *      or "exclude:PATTERN" that can be used to refine the list of files to include and exclude.
  *   <li><code>LGTM_INDEX_TYPESCRIPT</code>: whether to extract TypeScript
  *   <li><code>LGTM_INDEX_FILETYPES</code>: a newline-separated list of ".extension:filetype" pairs
  *       specifying which {@link FileType} to use for the given extension; the additional file type

javascript/ql/src/LanguageFeatures/NonLinearPattern.qhelp

@@ -8,6 +8,11 @@ If the same pattern variable is bound multiple times in the same object or array
 binding overwrites all of the earlier ones. This is most likely unintended and should be avoided.
 </p>
 
+<p>
+In TypeScript, a common mistake is to try to write type annotations inside a pattern. This is not
+possible, and the type annotation should come after the pattern.
+</p>
+
 </overview>
 <recommendation>
 
@@ -34,6 +39,21 @@ From context, it appears that the second binding should have been for variable <
 
 <sample src="examples/NonLinearPatternGood.js" />
 
+<p>
+This can sometimes happen in TypeScript, due to the apparant similarity between property patterns
+and type annotations. In the following example, the function uses a pattern parameter with properties <code>x</code>
+and <code>y</code>. These appear to have type <code>number</code>, but are in fact untyped properties both stored in a variable named <code>number</code>.
+</p>
+
+<sample src="examples/NonLinearPatternTS.ts" />
+
+<p>
+It is not possible to specify type annotations inside a pattern. The correct way is to specify the type
+after the parameter:
+</p>
+
+<sample src="examples/NonLinearPatternTSGood.ts" />
+
 </example>
 <references>
 <li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment">Destructuring assignment</a>.</li>

javascript/ql/src/LanguageFeatures/NonLinearPattern.ql

@@ -14,12 +14,55 @@
 
 import javascript
 
-from BindingPattern p, string n, VarDecl v, VarDecl w
+class RootDestructuringPattern extends BindingPattern {
+  RootDestructuringPattern() {
+    this instanceof DestructuringPattern and
+    not this = any(PropertyPattern p).getValuePattern() and
+    not this = any(ArrayPattern p).getAnElement()
+  }
+
+  /** Holds if this pattern has multiple bindings for `name`. */
+  predicate hasConflictingBindings(string name) {
+    exists(VarRef v, VarRef w |
+      v = getABindingVarRef() and
+      w = getABindingVarRef() and
+      name = v.getName() and
+      name = w.getName() and
+      v != w
+    )
+  }
+
+  /** Gets the first occurrence of the conflicting binding `name`. */
+  VarDecl getFirstClobberedVarDecl(string name) {
+    hasConflictingBindings(name) and
+    result =
+      min(VarDecl decl |
+        decl = getABindingVarRef() and decl.getName() = name
+      |
+        decl order by decl.getLocation().getStartLine(), decl.getLocation().getStartColumn()
+      )
+  }
+
+  /** Holds if variables in this pattern may resemble type annotations. */
+  predicate resemblesTypeAnnotation() {
+    hasConflictingBindings(_) and // Restrict size of predicate.
+    this instanceof Parameter and
+    this instanceof ObjectPattern and
+    not exists(getTypeAnnotation()) and
+    getFile().getFileType().isTypeScript()
+  }
+}
+
+from RootDestructuringPattern p, string n, VarDecl v, VarDecl w, string message
 where
-  v = p.getABindingVarRef() and
+  v = p.getFirstClobberedVarDecl(n) and
   w = p.getABindingVarRef() and
-  v.getName() = n and
   w.getName() = n and
   v != w and
-  v.getLocation().startsBefore(w.getLocation())
-select w, "Repeated binding of pattern variable '" + n + "' previously bound $@.", v, "here"
+  if p.resemblesTypeAnnotation()
+  then
+    message =
+      "The pattern variable '" + n +
+        "' appears to be a type, but is a variable previously bound $@."
+  else message = "Repeated binding of pattern variable '" + n + "' previously bound $@."
+select w, message, v, "here"

javascript/ql/src/LanguageFeatures/examples/NonLinearPatternTS.ts

@@ -0,0 +1,3 @@
+function distance({x: number, y: number}) {
+    return Math.sqrt(x*x + y*y);
+}

javascript/ql/src/LanguageFeatures/examples/NonLinearPatternTSGood.ts

@@ -0,0 +1,3 @@
+function distance({x, y}: {x: number, y: number}) {
+    return Math.sqrt(x*x + y*y);
+}

javascript/ql/src/codeql-suites/javascript-security-and-quality.qls

@@ -0,0 +1,4 @@
+- description: Security-and-quality queries for JavaScript
+- qlpack: codeql-javascript
+- apply: security-and-quality-selectors.yml
+  from: codeql-suite-helpers

javascript/ql/src/codeql-suites/javascript-security-extended.qls

@@ -0,0 +1,4 @@
+- description: Security-extended queries for JavaScript
+- qlpack: codeql-javascript
+- apply: security-extended-selectors.yml
+  from: codeql-suite-helpers

javascript/ql/src/experimental/Security/CWE-020/PostMessageNoOriginCheck.qhelp

@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+
+<p>If you use cross-origin communication between Window objects and do expect to receive messages from other sites, always verify the sender's identity using the origin and possibly source properties of the recevied `MessageEvent`. </p>
+
+<p>Unexpected behaviours, like `DOM-based XSS` could occur, if the event handler for incoming data does not check the origin of the data received and handles the data in an unsafe way.</p>
+</overview>
+
+<recommendation>
+<p>
+Always verify the sender's identity of incoming messages.
+</p>
+
+</recommendation>
+
+<example>
+<p>In the first example, the `MessageEvent.data` is passed to the `eval` function withouth checking the origin. This means that any window can send arbitrary messages that will be executed in the window receiving the message</p>
+<sample src="examples/postMessageNoOriginCheck.js" />
+
+<p> In the second example, the `MessageEvent.origin` is verified with an unsecure check. For example, using `event.origin.indexOf('www.example.com') > -1` can be bypassed because the string `www.example.com` could appear anywhere in `event.origin` (i.e. `www.example.com.mydomain.com`)</p>
+<sample src="examples/postMessageInsufficientCheck.js" />
+
+<p> In the third example, the `MessageEvent.origin` is properly checked against a trusted origin. </p>
+<sample src="examples/postMessageWithOriginCheck.js" />
+
+</example>
+
+<references>
+
+<li><a href="https://cwe.mitre.org/data/definitions/20.html">CWE-20: Improper Input Validation</a></li>
+<li><a href="https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage">Window.postMessage()</a></li>
+<li><a href="https://portswigger.net/web-security/dom-based/web-message-manipulation">Web-message manipulation</a></li>
+<li><a href="https://labs.detectify.com/2016/12/08/the-pitfalls-of-postmessage/">The pitfalls of postMessage</a></li>
+
+</references>
+</qhelp>

javascript/ql/src/experimental/Security/CWE-020/PostMessageNoOriginCheck.ql

@@ -0,0 +1,64 @@
+/**
+ * @name Missing `MessageEvent.origin` verification in `postMessage` handlers
+ * @description Missing the `MessageEvent.origin` verification in `postMessage` handlers, allows any windows to send arbitrary data to the `MessageEvent` listener.
+ * This could lead to unexpected behaviour, especially when `MessageEvent.data` is used in an unsafe way.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id js/missing-postmessageorigin-verification
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-20
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.DOM
+
+/**
+ * A method call for the insecure functions used to verify the `MessageEvent.origin`.
+ */
+class InsufficientOriginChecks extends DataFlow::Node {
+  InsufficientOriginChecks() {
+    exists(DataFlow::Node node |
+      this.(StringOps::StartsWith).getSubstring() = node or
+      this.(StringOps::Includes).getSubstring() = node or
+      this.(StringOps::EndsWith).getSubstring() = node
+    )
+  }
+}
+
+/**
+ * A function handler for the `MessageEvent`.
+ */
+class PostMessageHandler extends DataFlow::FunctionNode {
+  PostMessageHandler() { this.getFunction() instanceof PostMessageEventHandler }
+}
+
+/**
+ * The `MessageEvent` parameter received by the handler
+ */
+class PostMessageEvent extends DataFlow::SourceNode {
+  PostMessageEvent() { exists(PostMessageHandler handler | this = handler.getParameter(0)) }
+
+  /**
+   * Holds if an access on `MessageEvent.origin` is in an `EqualityTest` and there is no call of an insufficient verification method on `MessageEvent.origin`
+   */
+  predicate hasOriginChecked() {
+    exists(EqualityTest test |
+      this.getAPropertyRead(["origin", "source"]).flowsToExpr(test.getAnOperand())
+    )
+  }
+
+  /**
+   * Holds if there is an insufficient method call (i.e indexOf) used to verify `MessageEvent.origin`
+   */
+  predicate hasOriginInsufficientlyChecked() {
+    exists(InsufficientOriginChecks insufficientChecks |
+      this.getAPropertyRead("origin").getAMethodCall*() = insufficientChecks
+    )
+  }
+}
+
+from PostMessageEvent event
+where not event.hasOriginChecked() or event.hasOriginInsufficientlyChecked()
+select event, "Missing or unsafe origin verification."

javascript/ql/src/experimental/Security/CWE-020/examples/postMessageInsufficientCheck.js

@@ -0,0 +1,14 @@
+function postMessageHandler(event) {
+    let origin = event.origin.toLowerCase();
+
+    let host = window.location.host;
+
+    // BAD
+    if (origin.indexOf(host) === -1)
+        return;
+
+
+    eval(event.data);
+}
+
+window.addEventListener('message', postMessageHandler, false);

javascript/ql/src/experimental/Security/CWE-020/examples/postMessageNoOriginCheck.js

@@ -0,0 +1,9 @@
+function postMessageHandler(event) {
+    let origin = event.origin.toLowerCase();
+
+    console.log(origin)
+    // BAD: the origin property is not checked
+    eval(event.data);
+}
+
+window.addEventListener('message', postMessageHandler, false);

javascript/ql/src/experimental/Security/CWE-020/examples/postMessageWithOriginCheck.js

@@ -0,0 +1,9 @@
+function postMessageHandler(event) {
+    console.log(event.origin)
+    // GOOD: the origin property is checked
+    if (event.origin === 'www.example.com') {
+        // do something
+    }
+}
+
+window.addEventListener('message', postMessageHandler, false);

javascript/ql/src/semmle/javascript/Arrays.qll

@@ -1,5 +1,6 @@
 import javascript
 private import semmle.javascript.dataflow.InferredTypes
+private import semmle.javascript.dataflow.internal.PreCallGraphStep
 
 /**
  * Classes and predicates for modelling TaintTracking steps for arrays.
@@ -227,29 +228,32 @@ private module ArrayDataFlow {
    *
    * And the second parameter in the callback is the array ifself, so there is a `loadStoreStep` from the array to that second parameter.
    */
-  private class ArrayIteration extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
-    ArrayIteration() {
-      this.getMethodName() = "map" or
-      this.getMethodName() = "forEach"
-    }
-
+  private class ArrayIteration extends PreCallGraphStep {
     override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
-      prop = arrayElement() and
-      obj = this.getReceiver() and
-      element = getCallback(0).getParameter(0)
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = ["map", "forEach"] and
+        prop = arrayElement() and
+        obj = call.getReceiver() and
+        element = call.getCallback(0).getParameter(0)
+      )
     }
 
     override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
-      this.getMethodName() = "map" and
-      prop = arrayElement() and
-      element = this.getCallback(0).getAReturn() and
-      obj = this
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = "map" and
+        prop = arrayElement() and
+        element = call.getCallback(0).getAReturn() and
+        obj = call
+      )
     }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = arrayElement() and
-      pred = this.getReceiver() and
-      succ = getCallback(0).getParameter(2)
+    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = ["map", "forEach"] and
+        prop = arrayElement() and
+        pred = call.getReceiver() and
+        succ = call.getCallback(0).getParameter(2)
+      )
     }
   }
 
@@ -316,16 +320,13 @@ private module ArrayDataFlow {
   /**
    * A step for modelling `for of` iteration on arrays.
    */
-  private class ForOfStep extends DataFlow::AdditionalFlowStep, DataFlow::ValueNode {
-    ForOfStmt forOf;
-    DataFlow::Node element;
-
-    ForOfStep() { this.asExpr() = forOf.getIterationDomain() }
-
+  private class ForOfStep extends PreCallGraphStep {
     override predicate loadStep(DataFlow::Node obj, DataFlow::Node e, string prop) {
-      obj = this and
-      e = DataFlow::lvalueNode(forOf.getLValue()) and
-      prop = arrayElement()
+      exists(ForOfStmt forOf |
+        obj = forOf.getIterationDomain().flow() and
+        e = DataFlow::lvalueNode(forOf.getLValue()) and
+        prop = arrayElement()
+      )
     }
   }
 }

javascript/ql/src/semmle/javascript/Collections.qll

@@ -31,7 +31,7 @@ private class PseudoProperty extends TypeTrackingPseudoProperty {
  * `load`/`store`/`loadStore` can be used in the `CollectionsTypeTracking` module.
  * (Thereby avoiding naming conflicts with a "cousin" `AdditionalFlowStep` implementation.)
  */
-abstract private class CollectionFlowStep extends DataFlow::AdditionalFlowStep {
+abstract class CollectionFlowStep extends DataFlow::AdditionalFlowStep {
   final override predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }
 
   final override predicate step(

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

@@ -1519,7 +1519,8 @@ class AddExpr extends @addexpr, BinaryExpr {
   override string getOperator() { result = "+" }
 
   override string getStringValue() {
-    result = getLeftOperand().getStringValue() + getRightOperand().getStringValue()
+    result = getLeftOperand().getStringValue() + getRightOperand().getStringValue() and
+    result.length() < 1000 * 1000
   }
 }
 

javascript/ql/src/semmle/javascript/GlobalAccessPaths.qll

@@ -33,7 +33,7 @@ deprecated module GlobalAccessPath {
 /**
  * Provides predicates for associating access paths with data flow nodes.
  *
- * For example, `AccessPath.getAReferenceTo(x)` can be used to obtain the global access path
+ * For example, `AccessPath::getAReferenceTo(x)` can be used to obtain the global access path
  * that `x` refers to, as in the following sample:
  * ```
  * function f() {
@@ -240,7 +240,7 @@ module AccessPath {
    * ```
    * function f(x) {
    *   x.foo.bar = class {};
-   *   x.foo = { bar: class() };
+   *   x.foo = { bar: class {} };
    *   let alias = x;
    *   alias.foo.bar = class {};
    * }
@@ -338,7 +338,7 @@ module AccessPath {
    * ```
    * function f(x) {
    *   x.foo.bar = class {};
-   *   x.foo = { bar: class() };
+   *   x.foo = { bar: class {} };
    *   let alias = x;
    *   alias.foo.bar = class {};
    * }
@@ -355,7 +355,7 @@ module AccessPath {
    * Only gets the immediate right-hand side of an assignment or property or a global declaration,
    * not nodes that transitively flow there.
    *
-   * For example, the class nodes below are all assignmetns to `foo.bar`:
+   * For example, the class nodes below are all assignments to `foo.bar`:
    * ```
    * foo.bar = class {};
    * foo = { bar: class {} };

javascript/ql/src/semmle/javascript/Modules.qll

@@ -174,7 +174,8 @@ abstract class Import extends ASTNode {
       result = resolveAsProvidedModule() or
       result = resolveImportedPath() or
       result = resolveFromTypeRoot() or
-      result = resolveFromTypeScriptSymbol()
+      result = resolveFromTypeScriptSymbol() or
+      result = resolveNeighbourPackage(this.getImportedPath().getValue())
     )
   }
 
@@ -196,3 +197,28 @@ abstract deprecated class PathExprInModule extends PathExpr {
     this.(Comment).getTopLevel() instanceof Module
   }
 }
+
+/**
+ * Gets a module imported from another package in the same repository.
+ *
+ * No support for importing from folders inside the other package.
+ */
+private Module resolveNeighbourPackage(PathString importPath) {
+  exists(PackageJSON json | importPath = json.getPackageName() and result = json.getMainModule())
+  or
+  exists(string package |
+    result.getFile().getParentContainer() = getPackageFolder(package) and
+    importPath = package + "/" + [result.getFile().getBaseName(), result.getFile().getStem()]
+  )
+}
+
+/**
+ * Gets the folder for a package that has name `package` according to a package.json file in the resulting folder.
+ */
+pragma[noinline]
+private Folder getPackageFolder(string package) {
+  exists(PackageJSON json |
+    json.getPackageName() = package and
+    result = json.getFile().getParentContainer()
+  )
+}

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

@@ -23,6 +23,7 @@ private import internal.CallGraphs
 private import internal.FlowSteps as FlowSteps
 private import internal.DataFlowNode
 private import internal.AnalyzedParameters
+private import internal.PreCallGraphStep
 
 module DataFlow {
   /**

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

@@ -335,5 +335,20 @@ abstract class AdditionalTypeTrackingStep extends DataFlow::Node {
   /**
    * Holds if type-tracking should step from `pred` to `succ`.
    */
-  abstract predicate step(DataFlow::Node pred, DataFlow::Node succ);
+  predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }
+
+  /**
+   * Holds if type-tracking should step from `pred` into the `prop` property of `succ`.
+   */
+  predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
+
+  /**
+   * Holds if type-tracking should step from the `prop` property of `pred` to `succ`.
+   */
+  predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  /**
+   * Holds if type-tracking should step from the `prop` property of `pred` to the same property in `succ`.
+   */
+  predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
 }

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

@@ -61,6 +61,19 @@ module CallGraph {
       function = cls.getConstructor() and
       cls.getAClassReference(t.continue()).flowsTo(result)
     )
+    or
+    imprecision = 0 and
+    exists(DataFlow::FunctionNode outer |
+      result = getAFunctionReference(outer, 0, t.continue()).getAnInvocation() and
+      locallyReturnedFunction(outer, function)
+    )
+  }
+
+  cached
+  private predicate locallyReturnedFunction(
+    DataFlow::FunctionNode outer, DataFlow::FunctionNode inner
+  ) {
+    inner.flowsTo(outer.getAReturn())
   }
 
   /**

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

@@ -0,0 +1,134 @@
+/**
+ * Provides an extension point for contributing flow edges prior
+ * to call graph construction and type tracking.
+ */
+
+private import javascript
+
+private newtype TUnit = MkUnit()
+
+private class Unit extends TUnit {
+  string toString() { result = "unit" }
+}
+
+/**
+ * Internal extension point for adding flow edges prior to call graph construction
+ * and type tracking.
+ *
+ * Steps added here will be added to both `AdditionalFlowStep` and `AdditionalTypeTrackingStep`.
+ *
+ * Contributing steps that rely on type tracking will lead to negative recursion.
+ */
+class PreCallGraphStep extends Unit {
+  /**
+   * Holds if there is a step from `pred` to `succ`.
+   */
+  predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }
+
+  /**
+   * Holds if there is a step from `pred` into the `prop` property of `succ`.
+   */
+  predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
+
+  /**
+   * Holds if there is a step from the `prop` property of `pred` to `succ`.
+   */
+  predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  /**
+   * Holds if there is a step from the `prop` property of `pred` to the same property in `succ`.
+   */
+  predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
+}
+
+module PreCallGraphStep {
+  /**
+   * Holds if there is a step from `pred` to `succ`.
+   */
+  cached
+  predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    any(PreCallGraphStep s).step(pred, succ)
+  }
+
+  /**
+   * Holds if there is a step from `pred` into the `prop` property of `succ`.
+   */
+  cached
+  predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    any(PreCallGraphStep s).storeStep(pred, succ, prop)
+  }
+
+  /**
+   * Holds if there is a step from the `prop` property of `pred` to `succ`.
+   */
+  cached
+  predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    any(PreCallGraphStep s).loadStep(pred, succ, prop)
+  }
+
+  /**
+   * Holds if there is a step from the `prop` property of `pred` to the same property in `succ`.
+   */
+  cached
+  predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    any(PreCallGraphStep s).loadStoreStep(pred, succ, prop)
+  }
+}
+
+private class NodeWithPreCallGraphStep extends DataFlow::Node {
+  NodeWithPreCallGraphStep() {
+    PreCallGraphStep::step(this, _)
+    or
+    PreCallGraphStep::storeStep(this, _, _)
+    or
+    PreCallGraphStep::loadStep(this, _, _)
+    or
+    PreCallGraphStep::loadStoreStep(this, _, _)
+  }
+}
+
+private class AdditionalFlowStepFromPreCallGraph extends NodeWithPreCallGraphStep,
+  DataFlow::AdditionalFlowStep {
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    pred = this and
+    PreCallGraphStep::step(this, succ)
+  }
+
+  override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    pred = this and
+    PreCallGraphStep::storeStep(this, succ, prop)
+  }
+
+  override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    pred = this and
+    PreCallGraphStep::loadStep(this, succ, prop)
+  }
+
+  override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    pred = this and
+    PreCallGraphStep::loadStoreStep(this, succ, prop)
+  }
+}
+
+private class AdditionalTypeTrackingStepFromPreCallGraph extends NodeWithPreCallGraphStep,
+  DataFlow::AdditionalTypeTrackingStep {
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    pred = this and
+    PreCallGraphStep::step(this, succ)
+  }
+
+  override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    pred = this and
+    PreCallGraphStep::storeStep(this, succ, prop)
+  }
+
+  override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    pred = this and
+    PreCallGraphStep::loadStep(this, succ, prop)
+  }
+
+  override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    pred = this and
+    PreCallGraphStep::loadStoreStep(this, succ, prop)
+  }
+}

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

@@ -110,6 +110,15 @@ module StepSummary {
       or
       basicLoadStep(pred, succ, prop) and
       summary = LoadStep(prop)
+      or
+      any(AdditionalTypeTrackingStep st).storeStep(pred, succ, prop) and
+      summary = StoreStep(prop)
+      or
+      any(AdditionalTypeTrackingStep st).loadStep(pred, succ, prop) and
+      summary = LoadStep(prop)
+      or
+      any(AdditionalTypeTrackingStep st).loadStoreStep(pred, succ, prop) and
+      summary = CopyStep(prop)
     )
     or
     any(AdditionalTypeTrackingStep st).step(pred, succ) and

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

@@ -44,6 +44,9 @@ module Express {
     isRouter(e, _)
     or
     e.getType().hasUnderlyingType("express", "Router")
+    or
+    // created by `webpack-dev-server`
+    WebpackDevServer::webpackDevServerApp().flowsToExpr(e)
   }
 
   /**
@@ -119,7 +122,14 @@ module Express {
       t.start() and
       result = getARouteHandlerExpr().flow().getALocalSource()
       or
-      exists(DataFlow::TypeBackTracker t2 | result = getARouteHandler(t2).backtrack(t2, t))
+      exists(DataFlow::TypeBackTracker t2, DataFlow::SourceNode succ | succ = getARouteHandler(t2) |
+        result = succ.backtrack(t2, t)
+        or
+        exists(HTTP::RouteHandlerCandidateContainer container |
+          result = container.getRouteHandler(succ)
+        ) and
+        t = t2
+      )
     }
 
     override Expr getServer() { result.(Application).getARouteHandler() = getARouteHandler() }
@@ -896,4 +906,32 @@ module Express {
 
     override DataFlow::ValueNode getARouteHandlerArg() { result = routeHandlerArg }
   }
+
+  private module WebpackDevServer {
+    /**
+     * Gets a source for the options given to an instantiation of `webpack-dev-server`.
+     */
+    private DataFlow::SourceNode devServerOptions(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result =
+        DataFlow::moduleImport("webpack-dev-server")
+            .getAnInstantiation()
+            .getArgument(1)
+            .getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker t2 | result = devServerOptions(t2).backtrack(t2, t))
+    }
+
+    /**
+     * Gets an instance of the `express` app created by `webpack-dev-server`.
+     */
+    DataFlow::ParameterNode webpackDevServerApp() {
+      result =
+        devServerOptions(DataFlow::TypeBackTracker::end())
+            .getAPropertyWrite(["after", "before", "setup"])
+            .getRhs()
+            .getAFunctionValue()
+            .getParameter(0)
+    }
+  }
 }

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

@@ -3,6 +3,8 @@
  */
 
 import javascript
+private import semmle.javascript.DynamicPropertyAccess
+private import semmle.javascript.dataflow.internal.StepSummary
 
 module HTTP {
   /**
@@ -496,4 +498,111 @@ module HTTP {
   class CookieCryptographicKey extends CryptographicKey {
     CookieCryptographicKey() { this = any(CookieMiddlewareInstance instance).getASecretKey() }
   }
+
+  /**
+   * An object that contains one or more potential route handlers.
+   */
+  class RouteHandlerCandidateContainer extends DataFlow::Node {
+    RouteHandlerCandidateContainer::Range self;
+
+    RouteHandlerCandidateContainer() { this = self }
+
+    /**
+     * Gets the route handler in this container that is accessed at `access`.
+     */
+    DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) {
+      result = self.getRouteHandler(access)
+    }
+  }
+
+  /**
+   * Provides classes for working with objects that may contain one or more route handlers.
+   */
+  module RouteHandlerCandidateContainer {
+    private DataFlow::SourceNode ref(DataFlow::TypeTracker t, RouteHandlerCandidateContainer c) {
+      t.start() and result = c
+      or
+      exists(DataFlow::TypeTracker t2 | result = ref(t2, c).track(t2, t))
+    }
+
+    private DataFlow::SourceNode ref(RouteHandlerCandidateContainer c) {
+      result = ref(DataFlow::TypeTracker::end(), c)
+    }
+
+    /**
+     * A container for one or more potential route handlers.
+     *
+     * Extend this class and implement its abstract member predicates to model additional
+     * containers.
+     */
+    abstract class Range extends DataFlow::SourceNode {
+      /**
+       * Gets the route handler in this container that is accessed at `access`.
+       */
+      abstract DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access);
+    }
+
+    /**
+     * An object that contains one or more potential route handlers.
+     */
+    private class ContainerObject extends Range {
+      ContainerObject() {
+        (
+          this instanceof DataFlow::ObjectLiteralNode
+          or
+          exists(DataFlow::CallNode create | this = create |
+            create = DataFlow::globalVarRef("Object").getAMemberCall("create") and
+            create.getArgument(0).asExpr() instanceof NullLiteral
+          )
+        ) and
+        exists(RouteHandlerCandidate candidate | candidate.flowsTo(getAPropertyWrite().getRhs()))
+      }
+
+      override DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) {
+        result instanceof RouteHandlerCandidate and
+        exists(DataFlow::PropWrite write, DataFlow::PropRead read |
+          access = read and
+          ref(this).getAPropertyRead() = read and
+          result.flowsTo(write.getRhs()) and
+          write = this.getAPropertyWrite()
+        |
+          write.getPropertyName() = read.getPropertyName()
+          or
+          exists(EnumeratedPropName prop | access = prop.getASourceProp())
+          or
+          read = DataFlow::lvalueNode(any(ForOfStmt stmt).getLValue())
+        )
+      }
+    }
+
+    /**
+     * A collection that contains one or more route potential handlers.
+     */
+    private class ContainerCollection extends HTTP::RouteHandlerCandidateContainer::Range {
+      ContainerCollection() {
+        this = DataFlow::globalVarRef("Map").getAnInstantiation() and // restrict to Map for now
+        exists(
+          CollectionFlowStep store, DataFlow::Node storeTo, DataFlow::Node input,
+          RouteHandlerCandidate candidate
+        |
+          this.flowsTo(storeTo) and
+          store.store(input, storeTo, _) and
+          candidate.flowsTo(input)
+        )
+      }
+
+      override DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) {
+        exists(
+          DataFlow::Node input, TypeTrackingPseudoProperty key, CollectionFlowStep store,
+          CollectionFlowStep load, DataFlow::Node storeTo, DataFlow::Node loadFrom
+        |
+          this.flowsTo(storeTo) and
+          store.store(input, storeTo, key) and
+          result.(RouteHandlerCandidate).flowsTo(input) and
+          ref(this).flowsTo(loadFrom) and
+          load.load(loadFrom, access, key)
+        )
+      }
+    }
+  }
 }

javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql

@@ -66,7 +66,8 @@ query predicate missingCallee(AnnotatedCall call, AnnotatedFunction target, int
 
 query predicate badAnnotation(string name) {
   name = any(AnnotatedCall cl).getCallTargetName() and
-  not name = any(AnnotatedFunction cl).getCalleeName()
+  not name = any(AnnotatedFunction cl).getCalleeName() and
+  name != "NONE"
   or
   not name = any(AnnotatedCall cl).getCallTargetName() and
   name = any(AnnotatedFunction cl).getCalleeName()

javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/returned-function.js

@@ -0,0 +1,32 @@
+import 'dummy';
+
+/** name:curry1 */
+function curry1() {
+    /** name:curry2 */
+    function curry2(x) {
+        /** name:curry3 */
+        function curry3(y) {
+
+        }
+        return curry3;
+    }
+    return curry2;
+};
+
+/** calls:curry1 */
+let r1 = curry1();
+
+/** calls:curry2 */
+let r2 = r1();
+
+/** calls:curry3 */
+r2();
+
+function callback(f) {
+    // Call graph should not include callback invocations.
+    /** calls:NONE */
+    f();
+}
+
+let w1 = callback(curry1);
+callback(() => {});

javascript/ql/test/library-tests/Portals/PortalEntry.expected

@@ -711,12 +711,14 @@
 | (parameter 0 (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:5:7:5:10 | "me" | false |
 | (parameter 0 (member foo (root https://www.npmjs.com/package/m2))) | src/m3/tst2.js:5:5:5:12 | { x: o } | false |
 | (parameter 0 (member m (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:15:4:18 | "hi" | false |
+| (parameter 0 (member m (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:15:4:18 | "hi" | true |
 | (parameter 0 (member m (instance (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:15:4:18 | "hi" | false |
 | (parameter 0 (member m (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:2:5:2:8 | "hi" | false |
 | (parameter 0 (member m (return (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:15:4:18 | "hi" | false |
 | (parameter 0 (member m (return (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:15:4:18 | "hi" | false |
 | (parameter 0 (member m (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:2:5:2:8 | "hi" | false |
 | (parameter 0 (member s (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:15:5:21 | "there" | false |
+| (parameter 0 (member s (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:15:5:21 | "there" | true |
 | (parameter 0 (member s (instance (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:5:15:5:21 | "there" | false |
 | (parameter 0 (member s (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:3:5:3:11 | "there" | false |
 | (parameter 0 (member s (return (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:15:5:21 | "there" | false |

javascript/ql/test/library-tests/Portals/PortalExit.expected

@@ -6,12 +6,15 @@
 | (instance (member default (root https://www.npmjs.com/package/m2))) | src/m2/main.js:11:4:11:3 | this | true |
 | (instance (member default (root https://www.npmjs.com/package/m2))) | src/m2/main.js:15:11:15:10 | this | true |
 | (instance (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:4:1:4:11 | new A("me") | false |
+| (instance (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:4:1:4:11 | new A("me") | true |
 | (instance (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:5:1:5:11 | new A("me") | false |
+| (instance (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:5:1:5:11 | new A("me") | true |
 | (instance (root https://www.npmjs.com/package/m2)) | src/m3/tst3.js:4:1:4:11 | new A("me") | false |
 | (instance (root https://www.npmjs.com/package/m2)) | src/m3/tst3.js:5:1:5:11 | new A("me") | false |
 | (member default (root https://www.npmjs.com/package/m2)) | src/m3/tst3.js:1:8:1:8 | A | false |
 | (member foo (root https://www.npmjs.com/package/m2)) | src/m3/tst2.js:1:10:1:12 | foo | false |
 | (member m (instance (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:1:4:13 | new A("me").m | false |
+| (member m (instance (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:1:4:13 | new A("me").m | true |
 | (member m (instance (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:4:1:4:13 | new A("me").m | false |
 | (member m (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:2:1:2:3 | A.m | false |
 | (member m (return (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:1:4:13 | new A("me").m | false |
@@ -19,6 +22,7 @@
 | (member m (root https://www.npmjs.com/package/m2)) | src/m3/tst3.js:2:1:2:3 | A.m | false |
 | (member name (instance (member default (root https://www.npmjs.com/package/m2)))) | src/m2/main.js:12:27:12:35 | this.name | true |
 | (member s (instance (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:5:1:5:13 | new A("me").s | false |
+| (member s (instance (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:5:1:5:13 | new A("me").s | true |
 | (member s (instance (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:5:1:5:13 | new A("me").s | false |
 | (member s (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:3:1:3:3 | A.s | false |
 | (member s (return (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:5:1:5:13 | new A("me").s | false |
@@ -734,12 +738,14 @@
 | (return (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:5:1:5:11 | new A("me") | false |
 | (return (member foo (root https://www.npmjs.com/package/m2))) | src/m3/tst2.js:5:1:5:13 | foo({ x: o }) | false |
 | (return (member m (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:1:4:19 | new A("me").m("hi") | false |
+| (return (member m (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:1:4:19 | new A("me").m("hi") | true |
 | (return (member m (instance (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:1:4:19 | new A("me").m("hi") | false |
 | (return (member m (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:2:1:2:9 | A.m("hi") | false |
 | (return (member m (return (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:1:4:19 | new A("me").m("hi") | false |
 | (return (member m (return (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:1:4:19 | new A("me").m("hi") | false |
 | (return (member m (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:2:1:2:9 | A.m("hi") | false |
 | (return (member s (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:1:5:22 | new A(" ... there") | false |
+| (return (member s (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:1:5:22 | new A(" ... there") | true |
 | (return (member s (instance (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:5:1:5:22 | new A(" ... there") | false |
 | (return (member s (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:3:1:3:12 | A.s("there") | false |
 | (return (member s (return (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:1:5:22 | new A(" ... there") | false |

javascript/ql/test/library-tests/frameworks/Express/RouteHandlerContainer.qll

@@ -0,0 +1,8 @@
+import javascript
+
+query predicate getRouteHandlerContainerStep(
+  HTTP::RouteHandlerCandidateContainer container, DataFlow::SourceNode handler,
+  DataFlow::SourceNode access
+) {
+  handler = container.getRouteHandler(access)
+}

javascript/ql/test/library-tests/frameworks/Express/src/advanced-routehandler-registration.js

@@ -0,0 +1,163 @@
+var express = require("express");
+var app = express();
+
+// registration of route handlers in bulk
+let routes0 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const p in routes0) {
+  app.get(p, routes0[p]);
+}
+
+// registration of route handlers in bulk
+let routes1 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const handler of routes1) {
+  app.use(handler);
+}
+
+// registration of route handlers in bulk, with indirection
+let routes2 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const p of Object.keys(routes2)) {
+  app.get(p, routes2[p]);
+}
+
+// registration of route handlers in bulk, with indirection
+let routes3 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const h of Object.values(routes3)) {
+  app.use(h);
+}
+
+// custom router indirection for all requests
+let myRouter1 = {
+  handlers: {},
+  add: function(n, h) {
+    this.handlers[n] = h;
+  },
+  handle: function(req, res, target) {
+    this.handlers[target](req, res);
+  }
+};
+myRouter1.add("whatever", (req, res) => console.log(req));
+app.use((req, res) => myRouter1.handle(req, res, "whatever"));
+
+// simpler custom router indirection for all requests
+let mySimpleRouter = {
+  handler: undefined,
+  add: function(h) {
+    this.handler = h;
+  },
+  handle: function(req, res) {
+    this.handler(req, res);
+  }
+};
+mySimpleRouter.add((req, res) => console.log(req));
+app.use((req, res) => mySimpleRouter.handle(req, res));
+
+// simplest custom router indirection for all requests
+let mySimplestRouter = {
+  handler: (req, res) => console.log(req),
+  handle: function(req, res) {
+    this.handler(req, res);
+  }
+};
+app.use((req, res) => mySimplestRouter.handle(req, res));
+
+// a combination of bulk registration and indirection through a custom router
+let myRouter3 = {
+  handlers: {},
+  add: function(n, h) {
+    this.handlers[n] = h;
+  },
+  handle: function(req, res, target) {
+    this.handlers[target](req, res);
+  }
+};
+let routes3 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const p of Object.keys(routes3)) {
+  myRouter3.add(p, routes3[p]);
+}
+app.use((req, res) => myRouter3.handle(req, res, "whatever"));
+
+// a combination of bulk registration and indirection through a custom router. Using a map instead of an object.
+let myRouter4 = {
+  handlers: new Map(),
+  add: function(n, h) {
+    this.handlers.set(n, h);
+  },
+  handle: function(req, res, target) {
+    this.handlers.get(target)(req, res);
+  }
+};
+let routes4 = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};
+for (const p of Object.keys(routes4)) {
+  myRouter4.add(p, routes4[p]);
+}
+app.use((req, res) => myRouter4.handle(req, res, "whatever"));
+
+// registration of imported route handlers in bulk
+let importedRoutes = require("./route-collection").routes;
+for (const p in importedRoutes) {
+  app.get(p, importedRoutes[p]);
+}
+app.get("a", importedRoutes.a);
+app.get("b", importedRoutes.b);
+
+// registration of imported route handlers in a map
+let routesMap = new Map();
+routesMap.set("a", (req, res) => console.log(req));
+routesMap.set("b", (req, res) => console.log(req));
+routesMap.forEach((v, k) => app.get(k, v));
+app.get("a", routesMap.get("a"));
+app.get("b", routesMap.get("b"));
+
+let method = "GET";
+app[method.toLowerCase()](path, (req, res) => undefined);
+
+let names = ["handler-in-dynamic-require"];
+names.forEach(name => {
+	let dynamicRequire = require("./controllers/" + name);
+	app.get(dynamicRequire.path, dynamicRequire.handler);
+});
+
+let bulkRequire = require("./controllers");
+app.get(bulkRequire.bulky.path, bulkRequire.bulky.handler);
+
+let options = { app: app };
+let args = [];
+args.push((req, res) => undefined);
+app.use.apply(options.app, args);
+
+let handlers = { handlerA: (req, res) => undefined};
+app.use(handlers.handlerA.bind(data));
+
+for ([k, v] of routesMap) {
+	app.get(k, v) // not supported - requires one too many heap steps
+}
+
+app.get("b", routesMap.get("NOT_A_KEY!")); // unknown route handler
+
+let routesMap2 = new Map();
+routesMap2.set("c", (req, res) => console.log(req));
+routesMap2.set(unknown(), (req, res) => console.log(req));
+routesMap2.set("e", (req, res) => console.log(req));
+
+app.get("c", routesMap2.get("c"));
+app.get("d", routesMap2.get(unknown()));
+app.get("e", unknown());
+app.get("d", routesMap2.get("f"));

javascript/ql/test/library-tests/frameworks/Express/src/controllers/handler-in-bulk-require.js

@@ -0,0 +1 @@
+module.exports = { path: "bulky", handler: (req, res) => undefined };

javascript/ql/test/library-tests/frameworks/Express/src/controllers/handler-in-dynamic-require.js

@@ -0,0 +1 @@
+module.exports = { path: "/A", handler: (req, res) => undefined };

javascript/ql/test/library-tests/frameworks/Express/src/controllers/index.js

@@ -0,0 +1,4 @@
+let bulky = require("./handler-in-bulk-require");
+module.exports = {
+	bulky: bulky
+};

javascript/ql/test/library-tests/frameworks/Express/src/route-collection.js

@@ -0,0 +1,4 @@
+exports.routes = {
+  a: (req, res) => console.log(req),
+  b: (req, res) => console.log(req)
+};

javascript/ql/test/library-tests/frameworks/Express/tests.ql

@@ -46,3 +46,4 @@ import RequestExpr
 import RouteHandlerExpr_getAsSubRouter
 import Credentials
 import RouteHandler_getARequestExpr
+import RouteHandlerContainer

javascript/ql/test/library-tests/frameworks/HTTP-heuristics/RouteHandler.expected

@@ -15,6 +15,7 @@
 | 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-getter.js:29:32:29:51 | function(req, res){} |
 | src/middleware-attacher.js:3:13:3:32 | function(req, res){} |
 | src/nodejs.js:3:19:3:38 | function(req, res){} |
 | src/nodejs.js:8:14:8:33 | function(req, res){} |

javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected

@@ -2,7 +2,6 @@
 | 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/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/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`. |

javascript/ql/test/library-tests/frameworks/SQL/SqlString.expected

@@ -41,4 +41,5 @@
 | spanner.js:19:23:19:32 | "SQL code" |
 | spannerImport.js:4:8:4:17 | "SQL code" |
 | sqlite.js:7:8:7:45 | "UPDATE ... id = ?" |
+| sqliteArray.js:6:12:6:49 | "UPDATE ... id = ?" |
 | sqliteImport.js:2:8:2:44 | "UPDATE ... id = ?" |

javascript/ql/test/library-tests/frameworks/SQL/sqliteArray.js

@@ -0,0 +1,7 @@
+var sqlite = require('sqlite3');
+
+let databaseNames = [":memory:", ":foo:"];
+let databases = databaseNames.map(name => new sqlite.Database(name));
+for (let db of databases) {
+    db.run("UPDATE tbl SET name = ? WHERE id = ?", "bar", 2);
+}

javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPattern.expected

@@ -1,6 +1,11 @@
+| NonLinearPatternTS.ts:1:34:1:39 | number | The pattern variable 'number' appears to be a type, but is a variable previously bound $@. | NonLinearPatternTS.ts:1:23:1:28 | number | here |
 | ts-test.ts:3:13:3:13 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:3:10:3:10 | x | here |
 | ts-test.ts:8:16:8:16 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:8:10:8:10 | x | here |
 | ts-test.ts:11:10:11:10 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:11:7:11:7 | x | here |
+| ts-test.ts:21:8:21:13 | string | The pattern variable 'string' appears to be a type, but is a variable previously bound $@. | ts-test.ts:20:8:20:13 | string | here |
+| ts-test.ts:32:16:32:16 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:30:12:30:12 | x | here |
+| ts-test.ts:34:20:34:20 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:30:12:30:12 | x | here |
+| ts-test.ts:40:27:40:32 | string | Repeated binding of pattern variable 'string' previously bound $@. | ts-test.ts:40:16:40:21 | string | here |
 | tst.js:3:13:3:13 | x | Repeated binding of pattern variable 'x' previously bound $@. | tst.js:3:10:3:10 | x | here |
 | tst.js:8:16:8:16 | x | Repeated binding of pattern variable 'x' previously bound $@. | tst.js:8:10:8:10 | x | here |
 | tst.js:11:10:11:10 | x | Repeated binding of pattern variable 'x' previously bound $@. | tst.js:11:7:11:7 | x | here |

javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPatternTS.ts

@@ -0,0 +1,3 @@
+function distance({x: number, y: number}) {
+    return Math.sqrt(x*x + y*y);
+}

javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPatternTSGood.ts

@@ -0,0 +1,3 @@
+function distance({x, y}: {x: number, y: number}) {
+    return Math.sqrt(x*x + y*y);
+}

javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/ts-test.ts

@@ -15,3 +15,27 @@ var { x: x, x: y } = o;
 
 // OK
 var { p = x, q = x } = o;
+
+function f({
+    x: string,
+    y: string  // NOT OK
+}) {
+}
+
+function g({x, y}: {x: string, y: string}) { // OK
+}
+
+function blah(arg) {
+    var {
+        x: x,
+        y: {
+            x: x, // NOT OK
+            y: {
+                x: x // NOT OK
+            }
+        }
+    } = arg;
+}
+
+function h({x: string, y: string}: any) {  // NOT OK
+}

javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected

@@ -48,6 +48,11 @@ nodes
 | child_process-test.js:70:25:70:31 | req.url |
 | child_process-test.js:72:29:72:31 | cmd |
 | child_process-test.js:72:29:72:31 | cmd |
+| child_process-test.js:80:19:80:36 | req.query.fileName |
+| child_process-test.js:80:19:80:36 | req.query.fileName |
+| child_process-test.js:80:19:80:36 | req.query.fileName |
+| child_process-test.js:82:37:82:54 | req.query.fileName |
+| child_process-test.js:82:37:82:54 | req.query.fileName |
 | execSeries.js:3:20:3:22 | arr |
 | execSeries.js:6:14:6:16 | arr |
 | execSeries.js:6:14:6:21 | arr[i++] |
@@ -64,6 +69,10 @@ nodes
 | execSeries.js:18:34:18:40 | req.url |
 | execSeries.js:19:12:19:16 | [cmd] |
 | execSeries.js:19:13:19:15 | cmd |
+| lib/subLib/index.js:7:32:7:35 | name |
+| lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
+| lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
+| lib/subLib/index.js:8:22:8:25 | name |
 | other.js:5:9:5:49 | cmd |
 | other.js:5:15:5:38 | url.par ... , true) |
 | other.js:5:15:5:44 | url.par ... ).query |
@@ -152,6 +161,9 @@ edges
 | child_process-test.js:70:15:70:49 | url.par ... ry.path | child_process-test.js:70:9:70:49 | cmd |
 | child_process-test.js:70:25:70:31 | req.url | child_process-test.js:70:15:70:38 | url.par ... , true) |
 | child_process-test.js:70:25:70:31 | req.url | child_process-test.js:70:15:70:38 | url.par ... , true) |
+| child_process-test.js:80:19:80:36 | req.query.fileName | child_process-test.js:80:19:80:36 | req.query.fileName |
+| child_process-test.js:82:37:82:54 | req.query.fileName | lib/subLib/index.js:7:32:7:35 | name |
+| child_process-test.js:82:37:82:54 | req.query.fileName | lib/subLib/index.js:7:32:7:35 | name |
 | execSeries.js:3:20:3:22 | arr | execSeries.js:6:14:6:16 | arr |
 | execSeries.js:6:14:6:16 | arr | execSeries.js:6:14:6:21 | arr[i++] |
 | execSeries.js:6:14:6:21 | arr[i++] | execSeries.js:14:24:14:30 | command |
@@ -168,6 +180,9 @@ edges
 | execSeries.js:18:34:18:40 | req.url | execSeries.js:18:13:18:47 | require ... , true) |
 | execSeries.js:19:12:19:16 | [cmd] | execSeries.js:13:19:13:26 | commands |
 | execSeries.js:19:13:19:15 | cmd | execSeries.js:19:12:19:16 | [cmd] |
+| lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name |
+| lib/subLib/index.js:8:22:8:25 | name | lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
+| lib/subLib/index.js:8:22:8:25 | name | lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
 | other.js:5:9:5:49 | cmd | other.js:7:33:7:35 | cmd |
 | other.js:5:9:5:49 | cmd | other.js:7:33:7:35 | cmd |
 | other.js:5:9:5:49 | cmd | other.js:8:28:8:30 | cmd |
@@ -228,7 +243,9 @@ edges
 | child_process-test.js:59:5:59:39 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:50:15:50:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:64:3:64:21 | cp.spawn(cmd, args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:72:29:72:31 | cmd | child_process-test.js:70:25:70:31 | req.url | child_process-test.js:72:29:72:31 | cmd | This command depends on $@. | child_process-test.js:70:25:70:31 | req.url | a user-provided value |
+| child_process-test.js:80:19:80:36 | req.query.fileName | child_process-test.js:80:19:80:36 | req.query.fileName | child_process-test.js:80:19:80:36 | req.query.fileName | This command depends on $@. | child_process-test.js:80:19:80:36 | req.query.fileName | a user-provided value |
 | execSeries.js:14:41:14:47 | command | execSeries.js:18:34:18:40 | req.url | execSeries.js:14:41:14:47 | command | This command depends on $@. | execSeries.js:18:34:18:40 | req.url | a user-provided value |
+| lib/subLib/index.js:8:10:8:25 | "rm -rf " + name | child_process-test.js:82:37:82:54 | req.query.fileName | lib/subLib/index.js:8:10:8:25 | "rm -rf " + name | This command depends on $@. | child_process-test.js:82:37:82:54 | req.query.fileName | a user-provided value |
 | other.js:7:33:7:35 | cmd | other.js:5:25:5:31 | req.url | other.js:7:33:7:35 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:8:28:8:30 | cmd | other.js:5:25:5:31 | req.url | other.js:8:28:8:30 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:9:32:9:34 | cmd | other.js:5:25:5:31 | req.url | other.js:9:32:9:34 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |

javascript/ql/test/query-tests/Security/CWE-078/child_process-test.js

@@ -72,3 +72,14 @@ http.createServer(function(req, res) {
     util.promisify(cp.exec)(cmd); // NOT OK
 });
 
+
+const webpackDevServer = require('webpack-dev-server');
+new webpackDevServer(compiler, {
+    before: function (app) {
+        app.use(function (req, res, next) {
+          cp.exec(req.query.fileName); // NOT OK
+
+          require("my-sub-lib").foo(req.query.fileName); // calls lib/subLib/index.js#foo
+        });
+    }
+});

javascript/ql/test/query-tests/Security/CWE-078/lib/subLib/index.js

@@ -2,4 +2,8 @@ var cp = require("child_process")
 
 module.exports = function (name) {
 	cp.exec("rm -rf " + name); // OK - this file belongs in a sub-"module", and is not the primary exported module.
+};
+
+module.exports.foo = function (name) {
+	cp.exec("rm -rf " + name); // NOT OK - this is being called explicitly from child_process-test.js
 };

javascript/ql/test/query-tests/Security/CWE-078/lib/subLib/package.json

@@ -1,5 +1,5 @@
 {
-  "name": "mySubLib",
+  "name": "my-sub-lib",
   "version": "0.0.7",
   "main": "./index.js"
 }