Refactor Promises.qll to use PreCallGraphStep

Author: erik-krogh

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) {
-      not promise instanceof PromiseAllCreation and
+  predicate storeStep(DataFlow::Node value, DataFlow::SourceNode obj, string prop) {
+    // promise definition, e.g. `new Promise()`
+    exists(PromiseDefinition promise | obj = promise |
       prop = valueProp() and
-      pred = promise.getValue() and
-      succ = this
+      value = promise.getResolveParameter().getACall().getArgument(0)
       or
-      prop = valueProp() and
-      pred = promise.(PromiseAllCreation).getArrayNode() 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 = 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
-      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) {
+      value = promise.(PromiseAllCreation).getArrayNode()
+    )
+    or
+    // promise.then()
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "then" and obj = call |
       prop = valueProp() and
-      succ = this and
-      pred = operand
+      value = call.getCallback([0 .. 1]).getAReturn()
       or
       prop = errorProp() and
-      succ = await.getExceptionTarget() and
-      pred = operand
-    }
+      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
+    )
   }
 
   /**
-   * A flow step describing the data-flow related to the `.then` method of a promise.
+   * 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.
    */
-  class ThenStep extends PromiseFlowStep, DataFlow::MethodCallNode {
-    ThenStep() { this.getMethodName() = "then" }
-
-    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+  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 = getReceiver() and
-      succ = getCallback(0).getParameter(0)
+      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
+      pred = promise.getValue() and
+      prop = valueProp()
       or
+      pred = promise.(PromiseAllCreation).getArrayNode() and
+      prop = valueProp()
+    )
+    or
+    // promise.then()
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "then" and succ = call |
+      not exists(call.getArgument(1)) 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 = 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) {
-      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) {
+      pred = call.getCallback([0 .. 1]).getAReturn()
+    )
+    or
+    // promise.catch()
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "catch" and succ = call |
       prop = valueProp() and
-      pred = getReceiver().getALocalSource() and
-      succ = this
+      pred = call.getReceiver().getALocalSource()
       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()
+    )
   }
 }
 

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 |
       (