From 69d8cf643dfe59ba8cc2301ff2fb8b2feaa8294b Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 10 Mar 2020 12:23:23 +0100
Subject: [PATCH 01/12] add type tracking predicates for promises

---
 .../ql/src/semmle/javascript/Promises.qll     | 157 +++++++++++++++---
 .../javascript/dataflow/TypeTracking.qll      |  12 +-
 2 files changed, 144 insertions(+), 25 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll
index 413938ec3f7..79a5cc45555 100644
--- a/javascript/ql/src/semmle/javascript/Promises.qll
+++ b/javascript/ql/src/semmle/javascript/Promises.qll
@@ -120,16 +120,125 @@ class AggregateES2015PromiseDefinition extends PromiseCreationCall {
   }
 }
 
+/**
+ * A module for supporting promises in type-tracking predicates.
+ * The `PromiseTypeTracking::promiseStep` predicate can be used to add type-tracking in and out of promises,
+ * and the `PromiseTypeTracking::valueInPromiseTracker` predicate can be used to initiate a type-tracker
+ * where the tracked value is inside a promise.
+ *
+ * The below is an example of a type-tracking predicate where the initial value is inside a promise:
+ * ```
+ * DataFlow::SourceNode myType(DataFlow::TypeTracker t) {
+ *  t = PromiseTypeTracking::valueInPromiseTracker()
+ *  result = <the promise value> and
+ *  or
+ *  exists(DataFlow::TypeTracker t2 | result = myType(t2).track(t2, t))
+ *  or
+ *  exists(DataFlow::TypeTracker t2, DataFlow::StepSummary summary |
+ *    result = PromiseTypeTracking::promiseStep(myType(t2), summary) and
+ *    t = t2.append(summary)
+ *  )
+ * }
+ * ```
+ * The above example uses all the standard type-tracking steps and the promise specific type-tracking steps.
+ * The standard type-tracking steps can be removed for a type-tracking predicate that only tracks flow out of a promise.
+ *
+ * Replace `t = PromiseTypeTracking::valueInPromiseTracker()` in the above example with `t.start()` to create a type-tracking predicate
+ * where the value is not initially inside a promise.
+ */
+module PromiseTypeTracking {
+  /**
+   * A type-tracker used to start a type-tracker where the tracked value is initially inside a promise.
+   */
+  DataFlow::TypeTracker valueInPromiseTracker() {
+    exists(DataFlow::TypeTracker start | start.start() |
+      result = start.append(DataFlow::StoreStep(resolveField()))
+    )
+  }
+
+  /**
+   * Gets the result from a single step through a promise, from `pred` to `result` summarized by `summary`.
+   * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another.
+   *
+   * See the qldoc for the `PromiseTypeTracking` module for an example of how to use this predicate.
+   */
+  DataFlow::SourceNode promiseStep(DataFlow::SourceNode pred, DataFlow::StepSummary summary) {
+    exists(PromiseFlowStep step, string field | field = resolveField() |
+      summary = DataFlow::LoadStep(field) and
+      step.load(pred, result, field)
+      or
+      summary = DataFlow::StoreStep(field) and
+      step.store(pred, result, field)
+      or
+      summary = DataFlow::LevelStep() and
+      step.loadStore(pred, result, field)
+    )
+  }
+
+  /**
+   * A class enabling the use of the `resolveField` as a pseudo-property in type-tracking predicates.
+   */
+  private class ResolveFieldAsTypeTrackingProperty extends DataFlow::TypeTrackingPseudoProperty {
+    ResolveFieldAsTypeTrackingProperty() { this = resolveField() }
+  }
+}
+
+/**
+ * An `AdditionalFlowStep` used to model a data-flow step related to promises.
+ *
+ * The `loadStep`/`storeStep`/`loadStoreStep` methods are overloaded such that the new overloads
+ * `load`/`store`/`loadStore` can be used in the `PromiseTypeTracking` module.
+ * (Thereby avoiding conflicts with a "cousin" `AdditionalFlowStep` implementation.)
+ *
+ * The class is private and is only intended to be used inside the `PromiseTypeTracking` and `PromiseFlow` modules.
+ */
+abstract private class PromiseFlowStep extends DataFlow::AdditionalFlowStep {
+  final override predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }
+
+  final override predicate step(
+    DataFlow::Node p, DataFlow::Node s, DataFlow::FlowLabel pl, DataFlow::FlowLabel sl
+  ) {
+    none()
+  }
+
+  /**
+   * Holds if the property `prop` of the object `pred` should be loaded into `succ`.
+   */
+  predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  final override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    this.load(pred, succ, prop)
+  }
+
+  /**
+   * Holds if `pred` should be stored in the object `succ` under the property `prop`.
+   */
+  predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  final override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    this.store(pred, succ, prop)
+  }
+
+  /**
+   * Holds if the property `prop` should be copied from the object `pred` to the object `succ`.
+   */
+  predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  final override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    this.loadStore(pred, succ, prop)
+  }
+}
+
+/**
+ * Gets the pseudo-field used to describe resolved values in a promise.
+ */
+private string resolveField() { result = "$PromiseResolveField$" }
+
 /**
  * This module defines how data-flow propagates into and out of a Promise.
  * The data-flow is based on pseudo-properties rather than tainting the Promise object (which is what `PromiseTaintStep` does).
  */
 private module PromiseFlow {
-  /**
-   * Gets the pseudo-field used to describe resolved values in a promise.
-   */
-  string resolveField() { result = "$PromiseResolveField$" }
-
   /**
    * Gets the pseudo-field used to describe rejected values in a promise.
    */
@@ -140,12 +249,12 @@ private module PromiseFlow {
    *
    * The resolved/rejected value is written to a pseudo-field on the promise.
    */
-  class PromiseDefitionStep extends DataFlow::AdditionalFlowStep {
+  class PromiseDefitionStep extends PromiseFlowStep {
     PromiseDefinition promise;
 
     PromiseDefitionStep() { this = promise }
 
-    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       prop = resolveField() and
       pred = promise.getResolveParameter().getACall().getArgument(0) and
       succ = this
@@ -158,7 +267,7 @@ private module PromiseFlow {
       succ = this
     }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       // Copy the value of a resolved promise to the value of this promise.
       prop = resolveField() and
       pred = promise.getResolveParameter().getACall().getArgument(0) and
@@ -169,18 +278,18 @@ private module PromiseFlow {
   /**
    * A flow step describing the a Promise.resolve (and similar) call.
    */
-  class CreationStep extends DataFlow::AdditionalFlowStep {
+  class CreationStep extends PromiseFlowStep {
     PromiseCreationCall promise;
 
     CreationStep() { this = promise }
 
-    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       prop = resolveField() and
       pred = promise.getValue() and
       succ = this
     }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       // Copy the value of a resolved promise to the value of this promise.
       prop = resolveField() and
       pred = promise.getValue() and
@@ -192,7 +301,7 @@ private module PromiseFlow {
    * A load step loading the pseudo-field describing that the promise is rejected.
    * The rejected value is thrown as a exception.
    */
-  class AwaitStep extends DataFlow::AdditionalFlowStep {
+  class AwaitStep extends PromiseFlowStep {
     DataFlow::Node operand;
     AwaitExpr await;
 
@@ -201,7 +310,7 @@ private module PromiseFlow {
       operand.getEnclosingExpr() = await.getOperand()
     }
 
-    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       prop = resolveField() and
       succ = this and
       pred = operand
@@ -215,10 +324,10 @@ private module PromiseFlow {
   /**
    * A flow step describing the data-flow related to the `.then` method of a promise.
    */
-  class ThenStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
+  class ThenStep extends PromiseFlowStep, DataFlow::MethodCallNode {
     ThenStep() { this.getMethodName() = "then" }
 
-    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       prop = resolveField() and
       pred = getReceiver() and
       succ = getCallback(0).getParameter(0)
@@ -228,7 +337,7 @@ private module PromiseFlow {
       succ = getCallback(1).getParameter(0)
     }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       not exists(this.getArgument(1)) and
       prop = rejectField() and
       pred = getReceiver() and
@@ -240,7 +349,7 @@ private module PromiseFlow {
       succ = this
     }
 
-    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       prop = resolveField() and
       pred = getCallback([0 .. 1]).getAReturn() and
       succ = this
@@ -254,16 +363,16 @@ private module PromiseFlow {
   /**
    * A flow step describing the data-flow related to the `.catch` method of a promise.
    */
-  class CatchStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
+  class CatchStep extends PromiseFlowStep, DataFlow::MethodCallNode {
     CatchStep() { this.getMethodName() = "catch" }
 
-    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       prop = rejectField() and
       pred = getReceiver() and
       succ = getCallback(0).getParameter(0)
     }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       prop = resolveField() and
       pred = getReceiver().getALocalSource() and
       succ = this
@@ -274,7 +383,7 @@ private module PromiseFlow {
       succ = this
     }
 
-    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       prop = rejectField() and
       pred = getCallback(0).getExceptionalReturn() and
       succ = this
@@ -288,10 +397,10 @@ private module PromiseFlow {
   /**
    * A flow step describing the data-flow related to the `.finally` method of a promise.
    */
-  class FinallyStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
+  class FinallyStep extends PromiseFlowStep, DataFlow::MethodCallNode {
     FinallyStep() { this.getMethodName() = "finally" }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       (prop = resolveField() or prop = rejectField()) and
       pred = getReceiver() and
       succ = this
@@ -302,7 +411,7 @@ private module PromiseFlow {
       succ = this
     }
 
-    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       prop = rejectField() and
       pred = getCallback(0).getExceptionalReturn() and
       succ = this
diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
index e7db4e2d660..9dbb12e050a 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
@@ -16,9 +16,19 @@ private class PropertyName extends string {
     AccessPath::isAssignedInUniqueFile(this)
     or
     exists(AccessPath::getAnAssignmentTo(_, this))
+    or
+    this instanceof TypeTrackingPseudoProperty
   }
 }
 
+/**
+ * A pseudo-property that can be used in type-tracking.
+ */
+abstract class TypeTrackingPseudoProperty extends string {
+  bindingset[this]
+  TypeTrackingPseudoProperty() { any() }
+}
+
 private class OptionalPropertyName extends string {
   OptionalPropertyName() { this instanceof PropertyName or this = "" }
 }
@@ -26,7 +36,7 @@ private class OptionalPropertyName extends string {
 /**
  * A description of a step on an inter-procedural data flow path.
  */
-private newtype TStepSummary =
+newtype TStepSummary =
   LevelStep() or
   CallStep() or
   ReturnStep() or

From 3ddfd7ba73be08ba9febe8223f8bcfe67dc9107b Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 10 Mar 2020 12:24:16 +0100
Subject: [PATCH 02/12] add extra promise test for `chrome-remote-interface`

---
 javascript/ql/test/query-tests/Security/CWE-918/tst.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/javascript/ql/test/query-tests/Security/CWE-918/tst.js b/javascript/ql/test/query-tests/Security/CWE-918/tst.js
index b663d536632..53a1ce5477c 100644
--- a/javascript/ql/test/query-tests/Security/CWE-918/tst.js
+++ b/javascript/ql/test/query-tests/Security/CWE-918/tst.js
@@ -60,6 +60,10 @@ var server = http.createServer(async function(req, res) {
     var client = await CDP(options);
 	client.Page.navigate({url: tainted}); // NOT OK.
 	
+	CDP(options).catch((ignored) => {}).then((client) => {
+		client.Page.navigate({url: tainted}); // NOT OK.	
+	})
+	
 	CDP(options, (client) => {
 		client.Page.navigate({url: tainted}); // NOT OK.	
 	});

From 6110f8574801bef200dd12f98a02c3b53f6206a6 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 10 Mar 2020 12:27:21 +0100
Subject: [PATCH 03/12] refactor chrome-remote-interface to use type-tracking
 promise steps

---
 .../javascript/frameworks/ClientRequests.qll  | 33 +++++++------------
 .../Security/CWE-918/RequestForgery.expected  |  5 +++
 2 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
index e97169264e0..4aecb61f552 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
@@ -566,35 +566,24 @@ module ClientRequest {
    * The `isPromise` parameter reflects whether the reference is a promise containing
    * an instance of `chrome-remote-interface`, or an instance of `chrome-remote-interface`.
    */
-  private DataFlow::SourceNode chromeRemoteInterface(DataFlow::TypeTracker t, boolean isPromise) {
-    t.start() and
+  private DataFlow::SourceNode chromeRemoteInterface(DataFlow::TypeTracker t) {
     exists(DataFlow::CallNode call |
       call = DataFlow::moduleImport("chrome-remote-interface").getAnInvocation()
     |
-      result = call and isPromise = true
+      // the client is inside in a promise.
+      t = PromiseTypeTracking::valueInPromiseTracker() and result = call
       or
-      result = call.getCallback([0 .. 1]).getParameter(0) and isPromise = false
+      // the client is accessed directly using a callback.
+      t.start() and result = call.getCallback([0 .. 1]).getParameter(0)
     )
     or
-    exists(DataFlow::TypeTracker t2 | result = chromeRemoteInterface(t2, isPromise).track(t2, t))
+    // standard type-tracking steps
+    exists(DataFlow::TypeTracker t2 | result = chromeRemoteInterface(t2).track(t2, t))
     or
     // Simple promise tracking.
-    exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred |
-      pred = chromeRemoteInterface(t2, true) and
-      isPromise = false and
-      (
-        t2 = t and
-        exists(AwaitExpr await | DataFlow::valueNode(await.getOperand()).getALocalSource() = pred |
-          result.getEnclosingExpr() = await
-        )
-        or
-        t2 = t and
-        exists(DataFlow::MethodCallNode thenCall |
-          thenCall.getMethodName() = "then" and pred = thenCall.getReceiver().getALocalSource()
-        |
-          result = thenCall.getCallback(0).getParameter(0)
-        )
-      )
+    exists(DataFlow::TypeTracker t2, DataFlow::StepSummary summary |
+      result = PromiseTypeTracking::promiseStep(chromeRemoteInterface(t2), summary) and
+      t = t2.append(summary)
     )
   }
 
@@ -606,7 +595,7 @@ module ClientRequest {
 
     ChromeRemoteInterfaceRequest() {
       exists(DataFlow::SourceNode instance |
-        instance = chromeRemoteInterface(DataFlow::TypeTracker::end(), false)
+        instance = chromeRemoteInterface(DataFlow::TypeTracker::end())
       |
         optionsArg = 0 and
         this = instance.getAPropertyRead("Page").getAMemberCall("navigate")
diff --git a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
index 80d9d41e196..57ff328afe3 100644
--- a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
@@ -47,6 +47,8 @@ nodes
 | tst.js:61:29:61:35 | tainted |
 | tst.js:64:30:64:36 | tainted |
 | tst.js:64:30:64:36 | tainted |
+| tst.js:68:30:68:36 | tainted |
+| tst.js:68:30:68:36 | tainted |
 edges
 | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
 | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
@@ -89,6 +91,8 @@ edges
 | tst.js:58:9:58:52 | tainted | tst.js:61:29:61:35 | tainted |
 | tst.js:58:9:58:52 | tainted | tst.js:64:30:64:36 | tainted |
 | tst.js:58:9:58:52 | tainted | tst.js:64:30:64:36 | tainted |
+| tst.js:58:9:58:52 | tainted | tst.js:68:30:68:36 | tainted |
+| tst.js:58:9:58:52 | tainted | tst.js:68:30:68:36 | tainted |
 | tst.js:58:19:58:42 | url.par ... , true) | tst.js:58:19:58:48 | url.par ... ).query |
 | tst.js:58:19:58:48 | url.par ... ).query | tst.js:58:19:58:52 | url.par ... ery.url |
 | tst.js:58:19:58:52 | url.par ... ery.url | tst.js:58:9:58:52 | tainted |
@@ -109,3 +113,4 @@ edges
 | tst.js:45:5:45:57 | request ... ainted) | tst.js:14:29:14:35 | req.url | tst.js:45:13:45:56 | 'http:/ ... tainted | The $@ of this request depends on $@. | tst.js:45:13:45:56 | 'http:/ ... tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
 | tst.js:61:2:61:37 | client. ... inted}) | tst.js:58:29:58:35 | req.url | tst.js:61:29:61:35 | tainted | The $@ of this request depends on $@. | tst.js:61:29:61:35 | tainted | URL | tst.js:58:29:58:35 | req.url | a user-provided value |
 | tst.js:64:3:64:38 | client. ... inted}) | tst.js:58:29:58:35 | req.url | tst.js:64:30:64:36 | tainted | The $@ of this request depends on $@. | tst.js:64:30:64:36 | tainted | URL | tst.js:58:29:58:35 | req.url | a user-provided value |
+| tst.js:68:3:68:38 | client. ... inted}) | tst.js:58:29:58:35 | req.url | tst.js:68:30:68:36 | tainted | The $@ of this request depends on $@. | tst.js:68:30:68:36 | tainted | URL | tst.js:58:29:58:35 | req.url | a user-provided value |

From 97f276058383145c8baaf6f0214e85f4d2df13c2 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 10 Mar 2020 12:34:20 +0100
Subject: [PATCH 04/12] refactor `Files.qll` to use type-tracking (without
 tracking anything)

---
 .../semmle/javascript/frameworks/Files.qll    | 87 +++++++++++--------
 1 file changed, 51 insertions(+), 36 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Files.qll b/javascript/ql/src/semmle/javascript/frameworks/Files.qll
index 161a2c13b37..56334203841 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Files.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Files.qll
@@ -57,52 +57,67 @@ private class GlobFileNameSource extends FileNameSource {
   }
 }
 
+/**
+ * Gets a file name or an array of file names from the `globby` library.
+ */
+private DataFlow::SourceNode globbyFileNameSource(DataFlow::TypeTracker t) {
+  exists(string moduleName | moduleName = "globby" |
+    // `require('globby').sync(_)`
+    t.start() and
+    result = DataFlow::moduleMember(moduleName, "sync").getACall()
+    or
+    // `files` in `require('globby')(_).then(files => ...)`
+    t.start() and
+    result =
+      DataFlow::moduleImport(moduleName)
+          .getACall()
+          .getAMethodCall("then")
+          .getCallback(0)
+          .getParameter(0)
+  )
+}
+
 /**
  * A file name or an array of file names from the `globby` library.
  */
 private class GlobbyFileNameSource extends FileNameSource {
-  GlobbyFileNameSource() {
-    exists(string moduleName | moduleName = "globby" |
-      // `require('globby').sync(_)`
-      this = DataFlow::moduleMember(moduleName, "sync").getACall()
+  GlobbyFileNameSource() { this = globbyFileNameSource(DataFlow::TypeTracker::end()) }
+}
+
+/**
+ * Gets a file name or an array of file names from the `fast-glob` library.
+ */
+private DataFlow::Node fastGlobFileNameSource(DataFlow::TypeTracker t) {
+  exists(string moduleName | moduleName = "fast-glob" |
+    // `require('fast-glob').sync(_)
+    t.start() and
+    result = DataFlow::moduleMember(moduleName, "sync").getACall()
+    or
+    exists(DataFlow::SourceNode f |
+      f = DataFlow::moduleImport(moduleName)
       or
-      // `files` in `require('globby')(_).then(files => ...)`
-      this =
-        DataFlow::moduleImport(moduleName)
-            .getACall()
-            .getAMethodCall("then")
-            .getCallback(0)
-            .getParameter(0)
+      f = DataFlow::moduleMember(moduleName, "async")
+    |
+      // `files` in `require('fast-glob')(_).then(files => ...)` and
+      // `files` in `require('fast-glob').async(_).then(files => ...)`
+      t.start() and
+      result = f.getACall().getAMethodCall("then").getCallback(0).getParameter(0)
     )
-  }
+    or
+    // `file` in `require('fast-glob').stream(_).on(_,  file => ...)`
+    t.start() and
+    result =
+      DataFlow::moduleMember(moduleName, "stream")
+          .getACall()
+          .getAMethodCall(EventEmitter::on())
+          .getCallback(1)
+          .getParameter(0)
+  )
 }
 
 /**
  * A file name or an array of file names from the `fast-glob` library.
  */
 private class FastGlobFileNameSource extends FileNameSource {
-  FastGlobFileNameSource() {
-    exists(string moduleName | moduleName = "fast-glob" |
-      // `require('fast-glob').sync(_)`
-      this = DataFlow::moduleMember(moduleName, "sync").getACall()
-      or
-      exists(DataFlow::SourceNode f |
-        f = DataFlow::moduleImport(moduleName)
-        or
-        f = DataFlow::moduleMember(moduleName, "async")
-      |
-        // `files` in `require('fast-glob')(_).then(files => ...)` and
-        // `files` in `require('fast-glob').async(_).then(files => ...)`
-        this = f.getACall().getAMethodCall("then").getCallback(0).getParameter(0)
-      )
-      or
-      // `file` in `require('fast-glob').stream(_).on(_,  file => ...)`
-      this =
-        DataFlow::moduleMember(moduleName, "stream")
-            .getACall()
-            .getAMethodCall(EventEmitter::on())
-            .getCallback(1)
-            .getParameter(0)
-    )
-  }
+  FastGlobFileNameSource() { this = fastGlobFileNameSource(DataFlow::TypeTracker::end()) }
 }

From a24bc564a4afcca34370611d72ef77d9d6e1aa9d Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 10 Mar 2020 12:35:34 +0100
Subject: [PATCH 05/12] add extra tests for file-name with promises

---
 .../frameworks/Concepts/tst-file-names.js              | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/tst-file-names.js b/javascript/ql/test/library-tests/frameworks/Concepts/tst-file-names.js
index 293f2aa86d1..efecab23471 100644
--- a/javascript/ql/test/library-tests/frameworks/Concepts/tst-file-names.js
+++ b/javascript/ql/test/library-tests/frameworks/Concepts/tst-file-names.js
@@ -27,3 +27,13 @@ fastGlob(_).then(files => files);
 fastGlob.async(_).then(files => files);
 
 fastGlob.stream(_).on(_,  file => file); // XXX
+
+async function foo() {
+  globby(_).catch(() => {}).then(files => files);
+
+  var files = await globby(_);
+
+  var files2 = await fastGlob.async(_);
+
+  var files2 = await fastGlob.async(_).catch((wat) => {});
+}

From 066568ea601c89f7cea741c8c97c74f9f4c3a001 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 10 Mar 2020 12:36:42 +0100
Subject: [PATCH 06/12] add promise tracking to `Files.qll`

---
 .../semmle/javascript/frameworks/Files.qll    | 27 ++++++++++++-------
 .../Concepts/FileNameSource.expected          |  4 +++
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Files.qll b/javascript/ql/src/semmle/javascript/frameworks/Files.qll
index 56334203841..f864c943b3f 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Files.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Files.qll
@@ -59,6 +59,7 @@ private class GlobFileNameSource extends FileNameSource {
 
 /**
  * Gets a file name or an array of file names from the `globby` library.
+ * The predicate uses type-tracking. However, type-tracking is only used to track a step out of a promise.
  */
 private DataFlow::SourceNode globbyFileNameSource(DataFlow::TypeTracker t) {
   exists(string moduleName | moduleName = "globby" |
@@ -67,13 +68,14 @@ private DataFlow::SourceNode globbyFileNameSource(DataFlow::TypeTracker t) {
     result = DataFlow::moduleMember(moduleName, "sync").getACall()
     or
     // `files` in `require('globby')(_).then(files => ...)`
-    t.start() and
-    result =
-      DataFlow::moduleImport(moduleName)
-          .getACall()
-          .getAMethodCall("then")
-          .getCallback(0)
-          .getParameter(0)
+    t = PromiseTypeTracking::valueInPromiseTracker() and
+    result = DataFlow::moduleImport(moduleName).getACall()
+  )
+  or
+  // Tracking out of a promise
+  exists(DataFlow::TypeTracker t2, DataFlow::StepSummary summary |
+    result = PromiseTypeTracking::promiseStep(globbyFileNameSource(t2), summary) and
+    t = t2.append(summary)
   )
 }
 
@@ -86,6 +88,7 @@ private class GlobbyFileNameSource extends FileNameSource {
 
 /**
  * Gets a file name or an array of file names from the `fast-glob` library.
+ * The predicate uses type-tracking. However, type-tracking is only used to track a step out of a promise.
  */
 private DataFlow::Node fastGlobFileNameSource(DataFlow::TypeTracker t) {
   exists(string moduleName | moduleName = "fast-glob" |
@@ -100,8 +103,8 @@ private DataFlow::Node fastGlobFileNameSource(DataFlow::TypeTracker t) {
     |
       // `files` in `require('fast-glob')(_).then(files => ...)` and
       // `files` in `require('fast-glob').async(_).then(files => ...)`
-      t.start() and
-      result = f.getACall().getAMethodCall("then").getCallback(0).getParameter(0)
+      t = PromiseTypeTracking::valueInPromiseTracker() and
+      result = f.getACall()
     )
     or
     // `file` in `require('fast-glob').stream(_).on(_,  file => ...)`
@@ -113,6 +116,12 @@ private DataFlow::Node fastGlobFileNameSource(DataFlow::TypeTracker t) {
           .getCallback(1)
           .getParameter(0)
   )
+  or
+  // Tracking out of a promise
+  exists(DataFlow::TypeTracker t2, DataFlow::StepSummary summary |
+    result = PromiseTypeTracking::promiseStep(fastGlobFileNameSource(t2), summary) and
+    t = t2.append(summary)
+  )
 }
 
 /**
diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/FileNameSource.expected b/javascript/ql/test/library-tests/frameworks/Concepts/FileNameSource.expected
index bf05cf4163e..d33d127d902 100644
--- a/javascript/ql/test/library-tests/frameworks/Concepts/FileNameSource.expected
+++ b/javascript/ql/test/library-tests/frameworks/Concepts/FileNameSource.expected
@@ -10,3 +10,7 @@
 | tst-file-names.js:25:18:25:22 | files |
 | tst-file-names.js:27:24:27:28 | files |
 | tst-file-names.js:29:27:29:30 | file |
+| tst-file-names.js:32:34:32:38 | files |
+| tst-file-names.js:34:15:34:29 | await globby(_) |
+| tst-file-names.js:36:16:36:38 | await f ... sync(_) |
+| tst-file-names.js:38:16:38:57 | await f ...  => {}) |

From 7f147221f50fbc0f0a9763b461df5961cc6186cd Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 10 Mar 2020 22:09:21 +0100
Subject: [PATCH 07/12] refactor to include promise tracking as a core part of
 type tracking

---
 .../ql/src/semmle/javascript/Promises.qll     | 109 +++++++++---------
 .../javascript/dataflow/TypeTracking.qll      |  17 +++
 .../javascript/frameworks/ClientRequests.qll  |   9 +-
 .../semmle/javascript/frameworks/Files.qll    |   8 +-
 4 files changed, 75 insertions(+), 68 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll
index 79a5cc45555..897400eac00 100644
--- a/javascript/ql/src/semmle/javascript/Promises.qll
+++ b/javascript/ql/src/semmle/javascript/Promises.qll
@@ -120,42 +120,48 @@ class AggregateES2015PromiseDefinition extends PromiseCreationCall {
   }
 }
 
+/**
+ * Common predicates shared between type-tracking and data-flow for promises.
+ */
+module Promises {
+  /**
+   * Gets the pseudo-field used to describe resolved values in a promise.
+   */
+  string valueProp() { result = "$PromiseResolveField$" }
+
+  /**
+   * Gets the pseudo-field used to describe rejected values in a promise.
+   */
+  string errorProp() { result = "$PromiseRejectField$" }
+}
+
 /**
  * A module for supporting promises in type-tracking predicates.
- * The `PromiseTypeTracking::promiseStep` predicate can be used to add type-tracking in and out of promises,
- * and the `PromiseTypeTracking::valueInPromiseTracker` predicate can be used to initiate a type-tracker
- * where the tracked value is inside a promise.
+ * The `PromiseTypeTracking::promiseStep` is used for type tracking in and out of promises,
+ * and is included in the standard type-tracking steps (`SourceNode::track`).
+ * The `TypeTracker::startInPromise()` predicate can be used to initiate a type-tracker
+ * where the tracked value is a promise.
  *
- * The below is an example of a type-tracking predicate where the initial value is inside a promise:
+ * The below is an example of a type-tracking predicate where the initial value is a promise:
  * ```
  * DataFlow::SourceNode myType(DataFlow::TypeTracker t) {
- *  t = PromiseTypeTracking::valueInPromiseTracker()
+ *  t.startInPromise() and 
  *  result = <the promise value> and
  *  or
  *  exists(DataFlow::TypeTracker t2 | result = myType(t2).track(t2, t))
- *  or
- *  exists(DataFlow::TypeTracker t2, DataFlow::StepSummary summary |
- *    result = PromiseTypeTracking::promiseStep(myType(t2), summary) and
- *    t = t2.append(summary)
- *  )
  * }
  * ```
- * The above example uses all the standard type-tracking steps and the promise specific type-tracking steps.
- * The standard type-tracking steps can be removed for a type-tracking predicate that only tracks flow out of a promise.
+ * 
+ * The type-tracking predicate above will only end (`t = DataFlow::TypeTracker::end()`) after the tracked value has been 
+ * extracted from the promise.
+ * 
+ * The `PromiseTypeTracking::promiseStep` predicate can be used instead of `SourceNode::track` 
+ * to get type-tracking only for promise steps.
  *
- * Replace `t = PromiseTypeTracking::valueInPromiseTracker()` in the above example with `t.start()` to create a type-tracking predicate
+ * Replace `t.startInPromise()` in the above example with `t.start()` to create a type-tracking predicate
  * where the value is not initially inside a promise.
  */
 module PromiseTypeTracking {
-  /**
-   * A type-tracker used to start a type-tracker where the tracked value is initially inside a promise.
-   */
-  DataFlow::TypeTracker valueInPromiseTracker() {
-    exists(DataFlow::TypeTracker start | start.start() |
-      result = start.append(DataFlow::StoreStep(resolveField()))
-    )
-  }
-
   /**
    * Gets the result from a single step through a promise, from `pred` to `result` summarized by `summary`.
    * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another.
@@ -163,7 +169,7 @@ module PromiseTypeTracking {
    * See the qldoc for the `PromiseTypeTracking` module for an example of how to use this predicate.
    */
   DataFlow::SourceNode promiseStep(DataFlow::SourceNode pred, DataFlow::StepSummary summary) {
-    exists(PromiseFlowStep step, string field | field = resolveField() |
+    exists(PromiseFlowStep step, string field | field = Promises::valueProp() |
       summary = DataFlow::LoadStep(field) and
       step.load(pred, result, field)
       or
@@ -179,14 +185,14 @@ module PromiseTypeTracking {
    * A class enabling the use of the `resolveField` as a pseudo-property in type-tracking predicates.
    */
   private class ResolveFieldAsTypeTrackingProperty extends DataFlow::TypeTrackingPseudoProperty {
-    ResolveFieldAsTypeTrackingProperty() { this = resolveField() }
+    ResolveFieldAsTypeTrackingProperty() { this = Promises::valueProp() }
   }
 }
 
 /**
  * An `AdditionalFlowStep` used to model a data-flow step related to promises.
  *
- * The `loadStep`/`storeStep`/`loadStoreStep` methods are overloaded such that the new overloads
+ * The `loadStep`/`storeStep`/`loadStoreStep` methods are overloaded such that the new predicates
  * `load`/`store`/`loadStore` can be used in the `PromiseTypeTracking` module.
  * (Thereby avoiding conflicts with a "cousin" `AdditionalFlowStep` implementation.)
  *
@@ -229,20 +235,13 @@ abstract private class PromiseFlowStep extends DataFlow::AdditionalFlowStep {
   }
 }
 
-/**
- * Gets the pseudo-field used to describe resolved values in a promise.
- */
-private string resolveField() { result = "$PromiseResolveField$" }
-
 /**
  * This module defines how data-flow propagates into and out of a Promise.
  * The data-flow is based on pseudo-properties rather than tainting the Promise object (which is what `PromiseTaintStep` does).
  */
 private module PromiseFlow {
-  /**
-   * Gets the pseudo-field used to describe rejected values in a promise.
-   */
-  string rejectField() { result = "$PromiseRejectField$" }
+  private predicate valueProp = Promises::valueProp/0;
+  private predicate errorProp = Promises::errorProp/0;
 
   /**
    * A flow step describing a promise definition.
@@ -255,11 +254,11 @@ private module PromiseFlow {
     PromiseDefitionStep() { this = promise }
 
     override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+      prop = valueProp() and
       pred = promise.getResolveParameter().getACall().getArgument(0) and
       succ = this
       or
-      prop = rejectField() and
+      prop = errorProp() and
       (
         pred = promise.getRejectParameter().getACall().getArgument(0) or
         pred = promise.getExecutor().getExceptionalReturn()
@@ -269,7 +268,7 @@ private module PromiseFlow {
 
     override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       // Copy the value of a resolved promise to the value of this promise.
-      prop = resolveField() and
+      prop = valueProp() and
       pred = promise.getResolveParameter().getACall().getArgument(0) and
       succ = this
     }
@@ -284,14 +283,14 @@ private module PromiseFlow {
     CreationStep() { this = promise }
 
     override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+      prop = valueProp() and
       pred = promise.getValue() and
       succ = this
     }
 
     override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       // Copy the value of a resolved promise to the value of this promise.
-      prop = resolveField() and
+      prop = valueProp() and
       pred = promise.getValue() and
       succ = this
     }
@@ -311,11 +310,11 @@ private module PromiseFlow {
     }
 
     override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+      prop = valueProp() and
       succ = this and
       pred = operand
       or
-      prop = rejectField() and
+      prop = errorProp() and
       succ = await.getExceptionTarget() and
       pred = operand
     }
@@ -328,33 +327,33 @@ private module PromiseFlow {
     ThenStep() { this.getMethodName() = "then" }
 
     override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+      prop = valueProp() and
       pred = getReceiver() and
       succ = getCallback(0).getParameter(0)
       or
-      prop = rejectField() and
+      prop = errorProp() and
       pred = getReceiver() and
       succ = getCallback(1).getParameter(0)
     }
 
     override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       not exists(this.getArgument(1)) and
-      prop = rejectField() and
+      prop = errorProp() and
       pred = getReceiver() and
       succ = this
       or
       // read the value of a resolved/rejected promise that is returned
-      (prop = rejectField() or prop = resolveField()) and
+      (prop = errorProp() or prop = valueProp()) and
       pred = getCallback([0 .. 1]).getAReturn() and
       succ = this
     }
 
     override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+      prop = valueProp() and
       pred = getCallback([0 .. 1]).getAReturn() and
       succ = this
       or
-      prop = rejectField() and
+      prop = errorProp() and
       pred = getCallback([0 .. 1]).getExceptionalReturn() and
       succ = this
     }
@@ -367,28 +366,28 @@ private module PromiseFlow {
     CatchStep() { this.getMethodName() = "catch" }
 
     override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = rejectField() and
+      prop = errorProp() and
       pred = getReceiver() and
       succ = getCallback(0).getParameter(0)
     }
 
     override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+      prop = valueProp() and
       pred = getReceiver().getALocalSource() and
       succ = this
       or
       // read the value of a resolved/rejected promise that is returned
-      (prop = rejectField() or prop = resolveField()) and
+      (prop = errorProp() or prop = valueProp()) and
       pred = getCallback(0).getAReturn() and
       succ = this
     }
 
     override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = rejectField() and
+      prop = errorProp() and
       pred = getCallback(0).getExceptionalReturn() and
       succ = this
       or
-      prop = resolveField() and
+      prop = valueProp() and
       pred = getCallback(0).getAReturn() and
       succ = this
     }
@@ -401,18 +400,18 @@ private module PromiseFlow {
     FinallyStep() { this.getMethodName() = "finally" }
 
     override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      (prop = resolveField() or prop = rejectField()) and
+      (prop = valueProp() or prop = errorProp()) and
       pred = getReceiver() and
       succ = this
       or
       // read the value of a rejected promise that is returned
-      prop = rejectField() and
+      prop = errorProp() and
       pred = getCallback(0).getAReturn() and
       succ = this
     }
 
     override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = rejectField() and
+      prop = errorProp() and
       pred = getCallback(0).getExceptionalReturn() and
       succ = this
     }
diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
index 9dbb12e050a..fe5b43bf518 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
@@ -136,6 +136,9 @@ module StepSummary {
       name != ""
     )
     or
+    // Step in/out of a promise
+    succ = PromiseTypeTracking::promiseStep(pred, summary)
+    or
     // Summarize calls with flow directly from a parameter to a return.
     exists(DataFlow::ParameterNode param, DataFlow::FunctionNode fun |
       (
@@ -226,6 +229,20 @@ class TypeTracker extends TTypeTracker {
    */
   predicate start() { hasCall = false and prop = "" }
 
+  /**
+   * Holds if this is the starting point of type tracking, and the value starts in the property named `propName`. 
+   * The type tracking only ends after the property has been loaded. 
+   */
+  predicate startInProp(PropertyName propName) { hasCall = false and prop = propName }
+
+  /**
+   * Holds if this is the starting point of type tracking, and the initial value is a promise.
+   * The type tracking only ends after the value has been extracted from the promise.
+   */
+  predicate startInPromise() {
+    startInProp(Promises::valueProp())
+  }
+
   /**
    * Holds if this is the starting point of type tracking
    * when tracking a parameter into a call, but not out of it.
diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
index 4aecb61f552..938f877420c 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
@@ -571,20 +571,13 @@ module ClientRequest {
       call = DataFlow::moduleImport("chrome-remote-interface").getAnInvocation()
     |
       // the client is inside in a promise.
-      t = PromiseTypeTracking::valueInPromiseTracker() and result = call
+      t.startInPromise() and result = call
       or
       // the client is accessed directly using a callback.
       t.start() and result = call.getCallback([0 .. 1]).getParameter(0)
     )
     or
-    // standard type-tracking steps
     exists(DataFlow::TypeTracker t2 | result = chromeRemoteInterface(t2).track(t2, t))
-    or
-    // Simple promise tracking.
-    exists(DataFlow::TypeTracker t2, DataFlow::StepSummary summary |
-      result = PromiseTypeTracking::promiseStep(chromeRemoteInterface(t2), summary) and
-      t = t2.append(summary)
-    )
   }
 
   /**
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Files.qll b/javascript/ql/src/semmle/javascript/frameworks/Files.qll
index f864c943b3f..e8c5aad7936 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Files.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Files.qll
@@ -68,7 +68,7 @@ private DataFlow::SourceNode globbyFileNameSource(DataFlow::TypeTracker t) {
     result = DataFlow::moduleMember(moduleName, "sync").getACall()
     or
     // `files` in `require('globby')(_).then(files => ...)`
-    t = PromiseTypeTracking::valueInPromiseTracker() and
+    t.startInPromise() and
     result = DataFlow::moduleImport(moduleName).getACall()
   )
   or
@@ -93,8 +93,7 @@ private class GlobbyFileNameSource extends FileNameSource {
 private DataFlow::Node fastGlobFileNameSource(DataFlow::TypeTracker t) {
   exists(string moduleName | moduleName = "fast-glob" |
     // `require('fast-glob').sync(_)
-    t.start() and
-    result = DataFlow::moduleMember(moduleName, "sync").getACall()
+    t.start() and result = DataFlow::moduleMember(moduleName, "sync").getACall()
     or
     exists(DataFlow::SourceNode f |
       f = DataFlow::moduleImport(moduleName)
@@ -103,8 +102,7 @@ private DataFlow::Node fastGlobFileNameSource(DataFlow::TypeTracker t) {
     |
       // `files` in `require('fast-glob')(_).then(files => ...)` and
       // `files` in `require('fast-glob').async(_).then(files => ...)`
-      t = PromiseTypeTracking::valueInPromiseTracker() and
-      result = f.getACall()
+      t.startInPromise() and result = f.getACall()
     )
     or
     // `file` in `require('fast-glob').stream(_).on(_,  file => ...)`

From 59d2d6d4fdf8e815e7e0bb0266f8af8d96069d19 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Thu, 12 Mar 2020 10:47:11 +0100
Subject: [PATCH 08/12] autoformat

---
 javascript/ql/src/semmle/javascript/Promises.qll      | 11 ++++++-----
 .../src/semmle/javascript/dataflow/TypeTracking.qll   |  8 +++-----
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll
index 897400eac00..12ae799ae01 100644
--- a/javascript/ql/src/semmle/javascript/Promises.qll
+++ b/javascript/ql/src/semmle/javascript/Promises.qll
@@ -145,17 +145,17 @@ module Promises {
  * The below is an example of a type-tracking predicate where the initial value is a promise:
  * ```
  * DataFlow::SourceNode myType(DataFlow::TypeTracker t) {
- *  t.startInPromise() and 
+ *  t.startInPromise() and
  *  result = <the promise value> and
  *  or
  *  exists(DataFlow::TypeTracker t2 | result = myType(t2).track(t2, t))
  * }
  * ```
- * 
- * The type-tracking predicate above will only end (`t = DataFlow::TypeTracker::end()`) after the tracked value has been 
+ *
+ * The type-tracking predicate above will only end (`t = DataFlow::TypeTracker::end()`) after the tracked value has been
  * extracted from the promise.
- * 
- * The `PromiseTypeTracking::promiseStep` predicate can be used instead of `SourceNode::track` 
+ *
+ * The `PromiseTypeTracking::promiseStep` predicate can be used instead of `SourceNode::track`
  * to get type-tracking only for promise steps.
  *
  * Replace `t.startInPromise()` in the above example with `t.start()` to create a type-tracking predicate
@@ -241,6 +241,7 @@ abstract private class PromiseFlowStep extends DataFlow::AdditionalFlowStep {
  */
 private module PromiseFlow {
   private predicate valueProp = Promises::valueProp/0;
+
   private predicate errorProp = Promises::errorProp/0;
 
   /**
diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
index fe5b43bf518..74235633c6a 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
@@ -230,8 +230,8 @@ class TypeTracker extends TTypeTracker {
   predicate start() { hasCall = false and prop = "" }
 
   /**
-   * Holds if this is the starting point of type tracking, and the value starts in the property named `propName`. 
-   * The type tracking only ends after the property has been loaded. 
+   * Holds if this is the starting point of type tracking, and the value starts in the property named `propName`.
+   * The type tracking only ends after the property has been loaded.
    */
   predicate startInProp(PropertyName propName) { hasCall = false and prop = propName }
 
@@ -239,9 +239,7 @@ class TypeTracker extends TTypeTracker {
    * Holds if this is the starting point of type tracking, and the initial value is a promise.
    * The type tracking only ends after the value has been extracted from the promise.
    */
-  predicate startInPromise() {
-    startInProp(Promises::valueProp())
-  }
+  predicate startInPromise() { startInProp(Promises::valueProp()) }
 
   /**
    * Holds if this is the starting point of type tracking

From cd6fe8115d5e22ea903fb29d09472e449c073b8d Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 16 Mar 2020 16:27:50 +0100
Subject: [PATCH 09/12] Update javascript/ql/src/semmle/javascript/Promises.qll

Co-Authored-By: Asger F <asgerf@github.com>
---
 javascript/ql/src/semmle/javascript/Promises.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll
index 12ae799ae01..860a7d82456 100644
--- a/javascript/ql/src/semmle/javascript/Promises.qll
+++ b/javascript/ql/src/semmle/javascript/Promises.qll
@@ -137,7 +137,7 @@ module Promises {
 
 /**
  * A module for supporting promises in type-tracking predicates.
- * The `PromiseTypeTracking::promiseStep` is used for type tracking in and out of promises,
+ * The `PromiseTypeTracking::promiseStep` predicate is used for type tracking in and out of promises,
  * and is included in the standard type-tracking steps (`SourceNode::track`).
  * The `TypeTracker::startInPromise()` predicate can be used to initiate a type-tracker
  * where the tracked value is a promise.

From 7145a57db3232ced95062ccba4ba629082bbe0c1 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 16 Mar 2020 17:52:04 +0100
Subject: [PATCH 10/12] refactor StepSummary into an internal .qll

---
 .../ql/src/semmle/javascript/Promises.qll     |  26 ++-
 .../javascript/dataflow/TypeTracking.qll      | 155 +----------------
 .../dataflow/internal/CallGraphs.qll          |   7 +-
 .../dataflow/internal/StepSummary.qll         | 157 ++++++++++++++++++
 .../semmle/javascript/frameworks/Files.qll    |  10 +-
 5 files changed, 185 insertions(+), 170 deletions(-)
 create mode 100644 javascript/ql/src/semmle/javascript/dataflow/internal/StepSummary.qll

diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll
index 860a7d82456..de2d0522e0e 100644
--- a/javascript/ql/src/semmle/javascript/Promises.qll
+++ b/javascript/ql/src/semmle/javascript/Promises.qll
@@ -3,6 +3,7 @@
  */
 
 import javascript
+private import dataflow.internal.StepSummary
 
 /**
  * A definition of a `Promise` object.
@@ -165,26 +166,37 @@ module PromiseTypeTracking {
   /**
    * Gets the result from a single step through a promise, from `pred` to `result` summarized by `summary`.
    * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another.
-   *
-   * See the qldoc for the `PromiseTypeTracking` module for an example of how to use this predicate.
    */
-  DataFlow::SourceNode promiseStep(DataFlow::SourceNode pred, DataFlow::StepSummary summary) {
+  DataFlow::SourceNode promiseStep(DataFlow::SourceNode pred, StepSummary summary) {
     exists(PromiseFlowStep step, string field | field = Promises::valueProp() |
-      summary = DataFlow::LoadStep(field) and
+      summary = LoadStep(field) and
       step.load(pred, result, field)
       or
-      summary = DataFlow::StoreStep(field) and
+      summary = StoreStep(field) and
       step.store(pred, result, field)
       or
-      summary = DataFlow::LevelStep() and
+      summary = LevelStep() and
       step.loadStore(pred, result, field)
     )
   }
 
+  /**
+   * Gets the result from a single step through a promise, from `pred` with tracker `t2` to `result` with tracker `t`.
+   * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another.
+   */ 
+  DataFlow::SourceNode promiseStep(
+    DataFlow::SourceNode pred, DataFlow::TypeTracker t, DataFlow::TypeTracker t2
+  ) {
+    exists(StepSummary summary |
+      result = PromiseTypeTracking::promiseStep(pred, summary) and
+      t = t2.append(summary)
+    )
+  }
+
   /**
    * A class enabling the use of the `resolveField` as a pseudo-property in type-tracking predicates.
    */
-  private class ResolveFieldAsTypeTrackingProperty extends DataFlow::TypeTrackingPseudoProperty {
+  private class ResolveFieldAsTypeTrackingProperty extends TypeTrackingPseudoProperty {
     ResolveFieldAsTypeTrackingProperty() { this = Promises::valueProp() }
   }
 }
diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
index 74235633c6a..ba49ac93dd5 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
@@ -8,160 +8,7 @@
 
 private import javascript
 private import internal.FlowSteps
-
-private class PropertyName extends string {
-  PropertyName() {
-    this = any(DataFlow::PropRef pr).getPropertyName()
-    or
-    AccessPath::isAssignedInUniqueFile(this)
-    or
-    exists(AccessPath::getAnAssignmentTo(_, this))
-    or
-    this instanceof TypeTrackingPseudoProperty
-  }
-}
-
-/**
- * A pseudo-property that can be used in type-tracking.
- */
-abstract class TypeTrackingPseudoProperty extends string {
-  bindingset[this]
-  TypeTrackingPseudoProperty() { any() }
-}
-
-private class OptionalPropertyName extends string {
-  OptionalPropertyName() { this instanceof PropertyName or this = "" }
-}
-
-/**
- * A description of a step on an inter-procedural data flow path.
- */
-newtype TStepSummary =
-  LevelStep() or
-  CallStep() or
-  ReturnStep() or
-  StoreStep(PropertyName prop) or
-  LoadStep(PropertyName prop)
-
-/**
- * INTERNAL: Use `TypeTracker` or `TypeBackTracker` instead.
- *
- * A description of a step on an inter-procedural data flow path.
- */
-class StepSummary extends TStepSummary {
-  /** Gets a textual representation of this step summary. */
-  string toString() {
-    this instanceof LevelStep and result = "level"
-    or
-    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)
-  }
-}
-
-module StepSummary {
-  /**
-   * INTERNAL: Use `SourceNode.track()` or `SourceNode.backtrack()` instead.
-   */
-  cached
-  predicate step(DataFlow::SourceNode pred, DataFlow::SourceNode succ, StepSummary summary) {
-    exists(DataFlow::Node mid | pred.flowsTo(mid) | smallstep(mid, succ, summary))
-  }
-
-  /**
-   * INTERNAL: Use `TypeBackTracker.smallstep()` instead.
-   */
-  predicate smallstep(DataFlow::Node pred, DataFlow::Node succ, StepSummary summary) {
-    // Flow through properties of objects
-    propertyFlowStep(pred, succ) and
-    summary = LevelStep()
-    or
-    // Flow through global variables
-    globalFlowStep(pred, succ) and
-    summary = LevelStep()
-    or
-    // Flow into function
-    callStep(pred, succ) and
-    summary = CallStep()
-    or
-    // Flow out of function
-    returnStep(pred, succ) and
-    summary = ReturnStep()
-    or
-    // Flow through an instance field between members of the same class
-    DataFlow::localFieldStep(pred, succ) and
-    summary = LevelStep()
-    or
-    exists(string prop |
-      basicStoreStep(pred, succ, prop) and
-      summary = StoreStep(prop)
-      or
-      basicLoadStep(pred, succ, prop) and
-      summary = LoadStep(prop)
-    )
-    or
-    any(AdditionalTypeTrackingStep st).step(pred, succ) and
-    summary = LevelStep()
-    or
-    // Store to global access path
-    exists(string name |
-      pred = AccessPath::getAnAssignmentTo(name) and
-      AccessPath::isAssignedInUniqueFile(name) and
-      succ = DataFlow::globalAccessPathRootPseudoNode() and
-      summary = StoreStep(name)
-    )
-    or
-    // Load from global access path
-    exists(string name |
-      succ = AccessPath::getAReferenceTo(name) and
-      AccessPath::isAssignedInUniqueFile(name) and
-      pred = DataFlow::globalAccessPathRootPseudoNode() and
-      summary = LoadStep(name)
-    )
-    or
-    // Store to non-global access path
-    exists(string name |
-      pred = AccessPath::getAnAssignmentTo(succ, name) and
-      summary = StoreStep(name)
-    )
-    or
-    // Load from non-global access path
-    exists(string name |
-      succ = AccessPath::getAReferenceTo(pred, name) and
-      summary = LoadStep(name) and
-      name != ""
-    )
-    or
-    // Step in/out of a promise
-    succ = PromiseTypeTracking::promiseStep(pred, summary)
-    or
-    // Summarize calls with flow directly from a parameter to a return.
-    exists(DataFlow::ParameterNode param, DataFlow::FunctionNode fun |
-      (
-        param.flowsTo(fun.getAReturn()) and
-        summary = LevelStep()
-        or
-        exists(string prop |
-          param.getAPropertyRead(prop).flowsTo(fun.getAReturn()) and
-          summary = LoadStep(prop)
-        )
-      ) and
-      if param = fun.getAParameter()
-      then
-        // Step from argument to call site.
-        argumentPassing(succ, pred, fun.getFunction(), param)
-      else (
-        // Step from captured parameter to local call sites
-        pred = param and
-        succ = fun.getAnInvocation()
-      )
-    )
-  }
-}
+private import internal.StepSummary
 
 private newtype TTypeTracker = MkTypeTracker(Boolean hasCall, OptionalPropertyName prop)
 
diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll
index 944214a94af..f082580d5b3 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll
@@ -3,6 +3,7 @@
  */
 
 private import javascript
+private import StepSummary
 
 cached
 module CallGraph {
@@ -83,7 +84,7 @@ module CallGraph {
       getAFunctionReference(function, 0, t.continue()).flowsTo(callback)
     )
     or
-    exists(DataFlow::StepSummary summary, DataFlow::TypeTracker t2 |
+    exists(StepSummary summary, DataFlow::TypeTracker t2 |
       result = getABoundFunctionReferenceAux(function, boundArgs, t2, summary) and
       t = t2.append(summary)
     )
@@ -92,11 +93,11 @@ module CallGraph {
   pragma[noinline]
   private DataFlow::SourceNode getABoundFunctionReferenceAux(
     DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t,
-    DataFlow::StepSummary summary
+    StepSummary summary
   ) {
     exists(DataFlow::SourceNode prev |
       prev = getABoundFunctionReferenceAux(function, boundArgs, t) and
-      DataFlow::StepSummary::step(prev, result, summary)
+      StepSummary::step(prev, result, summary)
     )
   }
 
diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/StepSummary.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/StepSummary.qll
new file mode 100644
index 00000000000..a848cd57434
--- /dev/null
+++ b/javascript/ql/src/semmle/javascript/dataflow/internal/StepSummary.qll
@@ -0,0 +1,157 @@
+import javascript
+private import semmle.javascript.dataflow.TypeTracking
+private import FlowSteps
+
+class PropertyName extends string {
+  PropertyName() {
+    this = any(DataFlow::PropRef pr).getPropertyName()
+    or
+    AccessPath::isAssignedInUniqueFile(this)
+    or
+    exists(AccessPath::getAnAssignmentTo(_, this))
+    or
+    this instanceof TypeTrackingPseudoProperty
+  }
+}
+
+class OptionalPropertyName extends string {
+  OptionalPropertyName() { this instanceof PropertyName or this = "" }
+}
+
+/**
+ * A pseudo-property that can be used in type-tracking.
+ */
+abstract class TypeTrackingPseudoProperty extends string {
+  bindingset[this]
+  TypeTrackingPseudoProperty() { any() }
+}
+
+/**
+ * A description of a step on an inter-procedural data flow path.
+ */
+newtype TStepSummary =
+  LevelStep() or
+  CallStep() or
+  ReturnStep() or
+  StoreStep(PropertyName prop) or
+  LoadStep(PropertyName prop)
+
+/**
+ * INTERNAL: Use `TypeTracker` or `TypeBackTracker` instead.
+ *
+ * A description of a step on an inter-procedural data flow path.
+ */
+class StepSummary extends TStepSummary {
+  /** Gets a textual representation of this step summary. */
+  string toString() {
+    this instanceof LevelStep and result = "level"
+    or
+    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)
+  }
+}
+
+module StepSummary {
+  /**
+   * INTERNAL: Use `SourceNode.track()` or `SourceNode.backtrack()` instead.
+   */
+  cached
+  predicate step(DataFlow::SourceNode pred, DataFlow::SourceNode succ, StepSummary summary) {
+    exists(DataFlow::Node mid | pred.flowsTo(mid) | smallstep(mid, succ, summary))
+  }
+
+  /**
+   * INTERNAL: Use `TypeBackTracker.smallstep()` instead.
+   */
+  predicate smallstep(DataFlow::Node pred, DataFlow::Node succ, StepSummary summary) {
+    // Flow through properties of objects
+    propertyFlowStep(pred, succ) and
+    summary = LevelStep()
+    or
+    // Flow through global variables
+    globalFlowStep(pred, succ) and
+    summary = LevelStep()
+    or
+    // Flow into function
+    callStep(pred, succ) and
+    summary = CallStep()
+    or
+    // Flow out of function
+    returnStep(pred, succ) and
+    summary = ReturnStep()
+    or
+    // Flow through an instance field between members of the same class
+    DataFlow::localFieldStep(pred, succ) and
+    summary = LevelStep()
+    or
+    exists(string prop |
+      basicStoreStep(pred, succ, prop) and
+      summary = StoreStep(prop)
+      or
+      basicLoadStep(pred, succ, prop) and
+      summary = LoadStep(prop)
+    )
+    or
+    any(AdditionalTypeTrackingStep st).step(pred, succ) and
+    summary = LevelStep()
+    or
+    // Store to global access path
+    exists(string name |
+      pred = AccessPath::getAnAssignmentTo(name) and
+      AccessPath::isAssignedInUniqueFile(name) and
+      succ = DataFlow::globalAccessPathRootPseudoNode() and
+      summary = StoreStep(name)
+    )
+    or
+    // Load from global access path
+    exists(string name |
+      succ = AccessPath::getAReferenceTo(name) and
+      AccessPath::isAssignedInUniqueFile(name) and
+      pred = DataFlow::globalAccessPathRootPseudoNode() and
+      summary = LoadStep(name)
+    )
+    or
+    // Store to non-global access path
+    exists(string name |
+      pred = AccessPath::getAnAssignmentTo(succ, name) and
+      summary = StoreStep(name)
+    )
+    or
+    // Load from non-global access path
+    exists(string name |
+      succ = AccessPath::getAReferenceTo(pred, name) and
+      summary = LoadStep(name) and
+      name != ""
+    )
+    or
+    // Step in/out of a promise
+    succ = PromiseTypeTracking::promiseStep(pred, summary)
+    or
+    // Summarize calls with flow directly from a parameter to a return.
+    exists(DataFlow::ParameterNode param, DataFlow::FunctionNode fun |
+      (
+        param.flowsTo(fun.getAReturn()) and
+        summary = LevelStep()
+        or
+        exists(string prop |
+          param.getAPropertyRead(prop).flowsTo(fun.getAReturn()) and
+          summary = LoadStep(prop)
+        )
+      ) and
+      if param = fun.getAParameter()
+      then
+        // Step from argument to call site.
+        argumentPassing(succ, pred, fun.getFunction(), param)
+      else (
+        // Step from captured parameter to local call sites
+        pred = param and
+        succ = fun.getAnInvocation()
+      )
+    )
+  }
+}
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Files.qll b/javascript/ql/src/semmle/javascript/frameworks/Files.qll
index e8c5aad7936..14a892ee656 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Files.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Files.qll
@@ -73,9 +73,8 @@ private DataFlow::SourceNode globbyFileNameSource(DataFlow::TypeTracker t) {
   )
   or
   // Tracking out of a promise
-  exists(DataFlow::TypeTracker t2, DataFlow::StepSummary summary |
-    result = PromiseTypeTracking::promiseStep(globbyFileNameSource(t2), summary) and
-    t = t2.append(summary)
+  exists(DataFlow::TypeTracker t2 |
+    result = PromiseTypeTracking::promiseStep(globbyFileNameSource(t2), t, t2)
   )
 }
 
@@ -116,9 +115,8 @@ private DataFlow::Node fastGlobFileNameSource(DataFlow::TypeTracker t) {
   )
   or
   // Tracking out of a promise
-  exists(DataFlow::TypeTracker t2, DataFlow::StepSummary summary |
-    result = PromiseTypeTracking::promiseStep(fastGlobFileNameSource(t2), summary) and
-    t = t2.append(summary)
+  exists(DataFlow::TypeTracker t2 |
+    result = PromiseTypeTracking::promiseStep(fastGlobFileNameSource(t2), t, t2)
   )
 }
 

From d7b69fcfeab214a83aec22e812ba76700c282be3 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 17 Mar 2020 09:52:08 +0100
Subject: [PATCH 11/12] autoformat

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

diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll
index de2d0522e0e..867fab13d66 100644
--- a/javascript/ql/src/semmle/javascript/Promises.qll
+++ b/javascript/ql/src/semmle/javascript/Promises.qll
@@ -183,7 +183,7 @@ module PromiseTypeTracking {
   /**
    * Gets the result from a single step through a promise, from `pred` with tracker `t2` to `result` with tracker `t`.
    * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another.
-   */ 
+   */
   DataFlow::SourceNode promiseStep(
     DataFlow::SourceNode pred, DataFlow::TypeTracker t, DataFlow::TypeTracker t2
   ) {

From 095d4d711af9f2fb2bf0d5ff73f7031e44c4849d Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 17 Mar 2020 11:21:46 +0100
Subject: [PATCH 12/12] change import to an absolute import to fix warning

---
 .../src/semmle/javascript/dataflow/internal/CallGraphs.qll   | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll
index f082580d5b3..44c9b364c2c 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll
@@ -3,7 +3,7 @@
  */
 
 private import javascript
-private import StepSummary
+private import semmle.javascript.dataflow.internal.StepSummary
 
 cached
 module CallGraph {
@@ -92,8 +92,7 @@ module CallGraph {
 
   pragma[noinline]
   private DataFlow::SourceNode getABoundFunctionReferenceAux(
-    DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t,
-    StepSummary summary
+    DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t, StepSummary summary
   ) {
     exists(DataFlow::SourceNode prev |
       prev = getABoundFunctionReferenceAux(function, boundArgs, t) and