Merge pull request #3782 from erik-krogh/promiseSteps

Approved by asgerf
2026-04-30 19:26:02 +02:00 · 2020-06-26 10:11:10 +01:00
parent 92cc59b47b 690bde47aa
commit f81fc77e9e
3 changed files with 124 additions and 205 deletions
--- a/javascript/ql/src/semmle/javascript/Promises.qll
+++ b/javascript/ql/src/semmle/javascript/Promises.qll
@@ -185,18 +185,20 @@ 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.
+   *
+   * These type-tracking steps are already included in the default type-tracking steps (through `PreCallGraphStep`).
   */
  pragma[inline]
  DataFlow::Node promiseStep(DataFlow::Node pred, StepSummary summary) {
-    exists(PromiseFlowStep step, string field | field = Promises::valueProp() |
+    exists(string field | field = Promises::valueProp() |
      summary = LoadStep(field) and
-      step.load(pred, result, field)
+      PromiseFlow::loadStep(pred, result, field)
      or
      summary = StoreStep(field) and
-      step.store(pred, result, field)
+      PromiseFlow::storeStep(pred, result, field)
      or
      summary = CopyStep(field) and
-      step.loadStore(pred, result, field)
+      PromiseFlow::loadStoreStep(pred, result, field)
    )
  }

@@ -221,55 +223,25 @@ module PromiseTypeTracking {
  }
 }

+private import semmle.javascript.dataflow.internal.PreCallGraphStep
+
 /**
- * An `AdditionalFlowStep` used to model a data-flow step related to promises.
+ * A step related to promises.
 *
- * 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.)
- *
- * The class is private and is only intended to be used inside the `PromiseTypeTracking` and `PromiseFlow` modules.
+ * These steps are for `await p`, `new Promise()`, `Promise.resolve()`,
+ * `Promise.then()`, `Promise.catch()`, and `Promise.finally()`.
 */
-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()
+private class PromiseStep extends PreCallGraphStep {
+  override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
+    PromiseFlow::loadStep(obj, element, prop)
  }

-  /**
-   * 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)
+  override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
+    PromiseFlow::storeStep(element, obj, prop)
  }

-  /**
-   * Holds if `pred` should be stored in the object `succ` under the property `prop`.
-   */
-  predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
-
-  final override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode 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)
-  }
-
-  final override predicate loadStoreStep(
-    DataFlow::Node pred, DataFlow::Node succ, string loadProp, string storeProp
-  ) {
-    none()
+  override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    PromiseFlow::loadStoreStep(pred, succ, prop)
  }
 }

@@ -283,188 +255,143 @@ private module PromiseFlow {
  private predicate errorProp = Promises::errorProp/0;

  /**
-   * A flow step describing a promise definition.
-   *
-   * The resolved/rejected value is written to a pseudo-field on the promise.
+   * Holds if there is a step for loading a `value` from a `promise`.
+   * `prop` is either `valueProp()` if the value is a resolved value, or `errorProp()` if the promise has been rejected.
   */
-  class PromiseDefitionStep extends PromiseFlowStep {
-    PromiseDefinition promise;
-
-    PromiseDefitionStep() { this = promise }
-
-    override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+  predicate loadStep(DataFlow::Node promise, DataFlow::Node value, string prop) {
+    // await promise.
+    exists(AwaitExpr await | await.getOperand() = promise.asExpr() |
      prop = valueProp() and
-      pred = promise.getResolveParameter().getACall().getArgument(0) and
-      succ = this
+      value.asExpr() = await
      or
      prop = errorProp() and
-      (
-        pred = promise.getRejectParameter().getACall().getArgument(0) or
-        pred = promise.getExecutor().getExceptionalReturn()
-      ) 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.
+      value = await.getExceptionTarget()
+    )
+    or
+    // promise.then()
+    exists(DataFlow::MethodCallNode call |
+      call.getMethodName() = "then" and promise = call.getReceiver()
+    |
      prop = valueProp() and
-      pred = promise.getResolveParameter().getACall().getArgument(0) and
-      succ = this
-    }
+      value = call.getCallback(0).getParameter(0)
+      or
+      prop = errorProp() and
+      value = call.getCallback(1).getParameter(0)
+    )
+    or
+    // promise.catch()
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "catch" |
+      prop = errorProp() and
+      promise = call.getReceiver() and
+      value = call.getCallback(0).getParameter(0)
+    )
  }

  /**
-   * A flow step describing the a Promise.resolve (and similar) call.
+   * Holds if there is a step for storing a `value` into a promise `obj`.
+   * `prop` is either `valueProp()` if the value is a resolved value, or `errorProp()` if the promise has been rejected.
   */
-  class CreationStep extends PromiseFlowStep {
-    PromiseCreationCall promise;
-
-    CreationStep() { this = promise }
-
-    override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+  predicate storeStep(DataFlow::Node value, DataFlow::SourceNode obj, string prop) {
+    // promise definition, e.g. `new Promise()`
+    exists(PromiseDefinition promise | obj = promise |
+      prop = valueProp() and
+      value = promise.getResolveParameter().getACall().getArgument(0)
+      or
+      prop = errorProp() and
+      value =
+        [promise.getRejectParameter().getACall().getArgument(0),
+            promise.getExecutor().getExceptionalReturn()]
+    )
+    or
+    // promise creation call, e.g. `Promise.resolve`.
+    exists(PromiseCreationCall promise | obj = promise |
      not promise instanceof PromiseAllCreation and
      prop = valueProp() and
-      pred = promise.getValue() and
-      succ = this
+      value = promise.getValue()
      or
      prop = valueProp() and
-      pred = promise.(PromiseAllCreation).getArrayNode() and
-      succ = this
-    }
+      value = promise.(PromiseAllCreation).getArrayNode()
+    )
+    or
+    // promise.then()
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "then" and obj = call |
+      prop = valueProp() and
+      value = call.getCallback([0 .. 1]).getAReturn()
+      or
+      prop = errorProp() and
+      value = call.getCallback([0 .. 1]).getExceptionalReturn()
+    )
+    or
+    // promise.catch()
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "catch" and obj = call |
+      prop = errorProp() and
+      value = call.getCallback(0).getExceptionalReturn()
+      or
+      prop = valueProp() and
+      value = call.getCallback(0).getAReturn()
+    )
+    or
+    // promise.finally()
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "finally" |
+      prop = errorProp() and
+      value = call.getCallback(0).getExceptionalReturn() and
+      obj = call
+    )
+  }

-    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+  /**
+   * Holds if there is a step copying a resolved/rejected promise value from promise `pred` to promise `succ`.
+   * `prop` is either `valueProp()` if the value is a resolved value, or `errorProp()` if the promise has been rejected.
+   */
+  predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    // promise definition, e.g. `new Promise()`
+    exists(PromiseDefinition promise | succ = promise |
+      // Copy the value of a resolved promise to the value of this promise.
+      prop = valueProp() and
+      pred = promise.getResolveParameter().getACall().getArgument(0)
+    )
+    or
+    // promise creation call, e.g. `Promise.resolve`.
+    exists(PromiseCreationCall promise | succ = promise |
      // Copy the value of a resolved promise to the value of this promise.
      not promise instanceof PromiseAllCreation and
-      prop = valueProp() and
      pred = promise.getValue() and
-      succ = this
+      prop = valueProp()
      or
-      promise instanceof PromiseAllCreation and
-      prop = valueProp() and
      pred = promise.(PromiseAllCreation).getArrayNode() and
-      succ = this
-    }
-  }
-
-  /**
-   * A load step loading the pseudo-field describing that the promise is rejected.
-   * The rejected value is thrown as a exception.
-   */
-  class AwaitStep extends PromiseFlowStep {
-    DataFlow::Node operand;
-    AwaitExpr await;
-
-    AwaitStep() {
-      this.getEnclosingExpr() = await and
-      operand.getEnclosingExpr() = await.getOperand()
-    }
-
-    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = valueProp() and
-      succ = this and
-      pred = operand
-      or
+      prop = valueProp()
+    )
+    or
+    // promise.then()
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "then" and succ = call |
+      not exists(call.getArgument(1)) and
      prop = errorProp() and
-      succ = await.getExceptionTarget() and
-      pred = operand
-    }
-  }
-
-  /**
-   * A flow step describing the data-flow related to the `.then` method of a promise.
-   */
-  class ThenStep extends PromiseFlowStep, DataFlow::MethodCallNode {
-    ThenStep() { this.getMethodName() = "then" }
-
-    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = valueProp() and
-      pred = getReceiver() and
-      succ = getCallback(0).getParameter(0)
-      or
-      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 = errorProp() and
-      pred = getReceiver() and
-      succ = this
+      pred = call.getReceiver()
      or
      // read the value of a resolved/rejected promise that is returned
      (prop = errorProp() or prop = valueProp()) and
-      pred = getCallback([0 .. 1]).getAReturn() and
-      succ = this
-    }
-
-    override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+      pred = call.getCallback([0 .. 1]).getAReturn()
+    )
+    or
+    // promise.catch()
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "catch" and succ = call |
      prop = valueProp() and
-      pred = getCallback([0 .. 1]).getAReturn() and
-      succ = this
-      or
-      prop = errorProp() and
-      pred = getCallback([0 .. 1]).getExceptionalReturn() and
-      succ = this
-    }
-  }
-
-  /**
-   * A flow step describing the data-flow related to the `.catch` method of a promise.
-   */
-  class CatchStep extends PromiseFlowStep, DataFlow::MethodCallNode {
-    CatchStep() { this.getMethodName() = "catch" }
-
-    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = errorProp() and
-      pred = getReceiver() and
-      succ = getCallback(0).getParameter(0)
-    }
-
-    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = valueProp() and
-      pred = getReceiver().getALocalSource() and
-      succ = this
+      pred = call.getReceiver()
      or
      // read the value of a resolved/rejected promise that is returned
      (prop = errorProp() or prop = valueProp()) and
-      pred = getCallback(0).getAReturn() and
-      succ = this
-    }
-
-    override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
-      prop = errorProp() and
-      pred = getCallback(0).getExceptionalReturn() and
-      succ = this
-      or
-      prop = valueProp() and
-      pred = getCallback(0).getAReturn() and
-      succ = this
-    }
-  }
-
-  /**
-   * A flow step describing the data-flow related to the `.finally` method of a promise.
-   */
-  class FinallyStep extends PromiseFlowStep, DataFlow::MethodCallNode {
-    FinallyStep() { this.getMethodName() = "finally" }
-
-    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      pred = call.getCallback(0).getAReturn()
+    )
+    or
+    // promise.finally()
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "finally" and succ = call |
      (prop = valueProp() or prop = errorProp()) and
-      pred = getReceiver() and
-      succ = this
+      pred = call.getReceiver()
      or
      // read the value of a rejected promise that is returned
      prop = errorProp() and
-      pred = getCallback(0).getAReturn() and
-      succ = this
-    }
-
-    override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
-      prop = errorProp() and
-      pred = getCallback(0).getExceptionalReturn() and
-      succ = this
-    }
+      pred = call.getCallback(0).getAReturn()
+    )
  }
 }

--- a/javascript/ql/src/semmle/javascript/dataflow/internal/StepSummary.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/internal/StepSummary.qll
@@ -153,9 +153,6 @@ 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 |
      (
--- a/javascript/ql/test/library-tests/Promises/tests.expected
+++ b/javascript/ql/test/library-tests/Promises/tests.expected
@@ -281,7 +281,6 @@ typetrack
 | flow.js:34:2:34:60 | Promise ... ink(a)) | flow.js:34:53:34:59 | sink(a) | copy $PromiseResolveField$ |
 | flow.js:34:2:34:60 | Promise ... ink(a)) | flow.js:34:53:34:59 | sink(a) | store $PromiseResolveField$ |
 | flow.js:34:48:34:48 | a | flow.js:34:2:34:41 | Promise ... => { }) | load $PromiseResolveField$ |
-| flow.js:37:11:37:29 | p5.catch(() => { }) | flow.js:36:11:36:33 | Promise ... source) | copy $PromiseResolveField$ |
 | flow.js:38:11:38:31 | p6.then ... ink(a)) | flow.js:38:24:38:30 | sink(a) | copy $PromiseResolveField$ |
 | flow.js:38:11:38:31 | p6.then ... ink(a)) | flow.js:38:24:38:30 | sink(a) | store $PromiseResolveField$ |
 | flow.js:40:2:40:85 | new Pro ... ink(x)) | flow.js:40:2:40:65 | new Pro ... => { }) | copy $PromiseResolveField$ |
@@ -306,10 +305,8 @@ typetrack
 | flow.js:53:2:53:41 | createP ... ink(v)) | flow.js:53:34:53:40 | sink(v) | copy $PromiseResolveField$ |
 | flow.js:53:2:53:41 | createP ... ink(v)) | flow.js:53:34:53:40 | sink(v) | store $PromiseResolveField$ |
 | flow.js:53:29:53:29 | v | flow.js:53:2:53:22 | createP ... source) | load $PromiseResolveField$ |
-| flow.js:58:2:58:26 | p10.cat ... ink(x)) | flow.js:57:12:57:31 | p9.finally(() => {}) | copy $PromiseResolveField$ |
 | flow.js:58:2:58:26 | p10.cat ... ink(x)) | flow.js:58:19:58:25 | sink(x) | copy $PromiseResolveField$ |
 | flow.js:58:2:58:26 | p10.cat ... ink(x)) | flow.js:58:19:58:25 | sink(x) | store $PromiseResolveField$ |
-| flow.js:62:2:62:24 | p12.cat ... ink(x)) | flow.js:61:12:61:29 | p11.then(() => {}) | copy $PromiseResolveField$ |
 | flow.js:62:2:62:24 | p12.cat ... ink(x)) | flow.js:62:17:62:23 | sink(x) | copy $PromiseResolveField$ |
 | flow.js:62:2:62:24 | p12.cat ... ink(x)) | flow.js:62:17:62:23 | sink(x) | store $PromiseResolveField$ |
 | flow.js:65:3:65:56 | await n ... ource)) | flow.js:65:9:65:56 | new Pro ... ource)) | load $PromiseResolveField$ |
@@ -318,7 +315,6 @@ typetrack
 | flow.js:76:2:76:52 | chained ... ink(e)) | flow.js:76:45:76:51 | sink(e) | store $PromiseResolveField$ |
 | flow.js:79:3:79:22 | p.then(x => sink(x)) | flow.js:79:15:79:21 | sink(x) | copy $PromiseResolveField$ |
 | flow.js:79:3:79:22 | p.then(x => sink(x)) | flow.js:79:15:79:21 | sink(x) | store $PromiseResolveField$ |
-| flow.js:84:3:84:23 | p.catch ... ink(e)) | flow.js:83:32:83:32 | p | copy $PromiseResolveField$ |
 | flow.js:84:3:84:23 | p.catch ... ink(e)) | flow.js:84:16:84:22 | sink(e) | copy $PromiseResolveField$ |
 | flow.js:84:3:84:23 | p.catch ... ink(e)) | flow.js:84:16:84:22 | sink(e) | store $PromiseResolveField$ |
 | flow.js:89:3:89:47 | ("foo", ... ink(e)) | flow.js:89:3:89:27 | ("foo", ...  => {}) | copy $PromiseResolveField$ |
@@ -375,7 +371,6 @@ typetrack
 | flow.js:131:2:131:45 | Promise ... ink(x)) | flow.js:131:38:131:44 | sink(x) | store $PromiseResolveField$ |
 | flow.js:131:33:131:33 | x | flow.js:131:2:131:26 | Promise ... solved) | load $PromiseResolveField$ |
 | interflow.js:6:3:9:23 | loadScr ... eError) | interflow.js:6:3:8:26 | loadScr ... () { }) | copy $PromiseResolveField$ |
-| promises.js:23:3:25:4 | promise ... v;\\n  }) | promises.js:10:18:17:4 | new Pro ... );\\n  }) | copy $PromiseResolveField$ |
 | promises.js:33:19:35:6 | new Pro ... \\n    }) | promises.js:34:17:34:22 | source | copy $PromiseResolveField$ |
 | promises.js:33:19:35:6 | new Pro ... \\n    }) | promises.js:34:17:34:22 | source | store $PromiseResolveField$ |
 | promises.js:43:19:45:6 | Q.Promi ... \\n    }) | promises.js:44:17:44:22 | source | copy $PromiseResolveField$ |