Merge pull request #2525 from asger-semmle/promise-missing-await

JS: New query: missing await

Author: max-schaefer

javascript/config/suites/javascript/correctness-core

@@ -16,6 +16,7 @@
 + semmlecode-javascript-queries/Expressions/DuplicateSwitchCase.ql: /Correctness/Expressions
 + semmlecode-javascript-queries/Expressions/HeterogeneousComparison.ql: /Correctness/Expressions
 + semmlecode-javascript-queries/Expressions/MisspelledVariableName.ql: /Correctness/Expressions
++ semmlecode-javascript-queries/Expressions/MissingAwait.ql: /Correctness/Expressions
 + semmlecode-javascript-queries/Expressions/MissingDotLengthInComparison.ql: /Correctness/Expressions
 + semmlecode-javascript-queries/Expressions/ShiftOutOfRange.ql: /Correctness/Expressions
 + semmlecode-javascript-queries/Expressions/RedundantExpression.ql: /Correctness/Expressions

javascript/ql/src/Expressions/MissingAwait.qhelp

@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+In JavaScript, <code>async</code> functions always return a promise object.
+To obtain the underlying value of the promise, use the <code>await</code> operator or call the <code>then</code> method.
+Attempting to use a promise object instead of its underlying value can lead to unexpected behavior.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Use the <code>await</code> operator to get the value contained in the promise.
+Alternatively, call <code>then</code> on the promise and use the value passed to the callback.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+In the following example, the <code>getData</code> function returns a promise,
+and the caller checks if the returned promise is <code>null</code>:
+</p>
+
+<sample src="examples/MissingAwait.js" />
+
+<p>
+However, the null check does not work as expected. The <code>return null</code> statement
+on line 2 actually returns a <em>promise</em> containing the <code>null</code> value.
+Since the promise object itself is not equal to <code>null</code>, the error check is bypassed.
+</p>
+
+<p>
+The issue can be corrected by inserting <code>await</code> before the promise:
+</p>
+
+<sample src="examples/MissingAwaitGood.js" />
+
+</example>
+<references>
+<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises">Using promises</a></li>
+<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function">Async functions</a></li>
+<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await">Await operator</a></li>
+</references>
+</qhelp>

javascript/ql/src/Expressions/MissingAwait.ql

@@ -0,0 +1,81 @@
+/**
+ * @name Missing await
+ * @description Using a promise without awaiting its result can lead to unexpected behavior.
+ * @kind problem
+ * @problem.severity warning
+ * @id js/missing-await
+ * @tags correctness
+ * @precision high
+ */
+
+import javascript
+
+/**
+ * Holds if `call` is a call to an `async` function.
+ */
+predicate isAsyncCall(DataFlow::CallNode call) {
+  // If a callee is known, and all known callees are async, assume all
+  // possible callees are async.
+  forex(Function callee | call.getACallee() = callee | callee.isAsync())
+}
+
+/**
+ * Holds if `node` is always a promise.
+ */
+predicate isPromise(DataFlow::SourceNode node, boolean nullable) {
+  isAsyncCall(node) and
+  nullable = false
+  or
+  not isAsyncCall(node) and
+  node.asExpr().getType() instanceof PromiseType and
+  nullable = true
+}
+
+/**
+ * Holds the result of `e` is used in a way that doesn't make sense for Promise objects.
+ */
+predicate isBadPromiseContext(Expr expr) {
+  exists(BinaryExpr binary |
+    expr = binary.getAnOperand() and
+    not binary instanceof LogicalExpr and
+    not binary instanceof InstanceofExpr
+  )
+  or
+  expr = any(LogicalBinaryExpr e).getLeftOperand()
+  or
+  expr = any(UnaryExpr e).getOperand()
+  or
+  expr = any(UpdateExpr e).getOperand()
+  or
+  expr = any(ConditionalExpr e).getCondition()
+  or
+  expr = any(IfStmt stmt).getCondition()
+  or
+  expr = any(ForInStmt stmt).getIterationDomain()
+  or
+  expr = any(IndexExpr e).getIndex()
+}
+
+string tryGetPromiseExplanation(Expr e) {
+  result = "The value '" + e.(VarAccess).getName() + "' is always a promise."
+  or
+  result = "The call to '" + e.(CallExpr).getCalleeName() + "' always returns a promise."
+}
+
+string getPromiseExplanation(Expr e) {
+  result = tryGetPromiseExplanation(e)
+  or
+  not exists(tryGetPromiseExplanation(e)) and
+  result = "This value is always a promise."
+}
+
+from Expr expr, boolean nullable
+where
+  isBadPromiseContext(expr) and
+  isPromise(expr.flow().getImmediatePredecessor*(), nullable) and
+  (
+    nullable = false
+    or
+    expr.inNullSensitiveContext()
+  )
+select expr, "Missing await. " + getPromiseExplanation(expr)

javascript/ql/src/Expressions/examples/MissingAwait.js

@@ -0,0 +1,14 @@
+async function getData(id) {
+  let req = await fetch(`https://example.com/data?id=${id}`);
+  if (!req.ok) return null;
+  return req.json();
+}
+
+async function showData(id) {
+  let data = getData(id);
+  if (data == null) {
+    console.warn("No data for: " + id);
+    return;
+  }
+  // ...
+}

javascript/ql/src/Expressions/examples/MissingAwaitGood.js

@@ -0,0 +1,14 @@
+async function getData(id) {
+  let req = await fetch(`https://example.com/data?id=${id}`);
+  if (!req.ok) return null;
+  return req.json();
+}
+
+async function showData(id) {
+  let data = await getData(id);
+  if (data == null) {
+    console.warn("No data for: " + id);
+    return;
+  }
+  // ...
+}

javascript/ql/src/semmle/javascript/SSA.qll

@@ -670,6 +670,29 @@ class SsaPhiNode extends SsaPseudoDefinition, TPhi {
     endcolumn = startcolumn and
     getBasicBlock().getLocation().hasLocationInfo(filepath, startline, startcolumn, _, _)
   }
+
+  /**
+   * If all inputs to this phi node are (transitive) refinements of the same variable,
+   * gets that variable.
+   */
+  SsaVariable getRephinedVariable() {
+    forex(SsaVariable input | input = getAnInput() |
+      result = getRefinedVariable(input)
+    )
+  }
+}
+
+/**
+ * Gets the input to the given refinement node or rephinement node.
+ */
+private SsaVariable getRefinedVariable(SsaVariable v) {
+  result = getRefinedVariable(v.(SsaRefinementNode).getAnInput())
+  or
+  result = getRefinedVariable(v.(SsaPhiNode).getRephinedVariable())
+  or
+  not v instanceof SsaRefinementNode and
+  not v instanceof SsaPhiNode and
+  result = v
 }
 
 /**

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

@@ -208,6 +208,11 @@ module DataFlow {
         result = TSsaDefNode(refinement.getAnInput())
       )
       or
+      exists(SsaPhiNode phi |
+        this = TSsaDefNode(phi) and
+        result = TSsaDefNode(phi.getRephinedVariable())
+      )
+      or
       // IIFE call -> return value of IIFE
       exists(Function fun |
         localCall(this.asExpr(), fun) and

javascript/ql/test/query-tests/Expressions/MissingAwait/MissingAwait.expected

@@ -0,0 +1,9 @@
+| tst.js:8:9:8:13 | thing | Missing await. The value 'thing' is always a promise. |
+| tst.js:10:9:10:13 | thing | Missing await. The value 'thing' is always a promise. |
+| tst.js:12:15:12:19 | thing | Missing await. The value 'thing' is always a promise. |
+| tst.js:14:19:14:23 | thing | Missing await. The value 'thing' is always a promise. |
+| tst.js:19:19:19:23 | thing | Missing await. The value 'thing' is always a promise. |
+| tst.js:20:9:20:13 | thing | Missing await. The value 'thing' is always a promise. |
+| tst.js:22:15:22:19 | thing | Missing await. The value 'thing' is always a promise. |
+| tst.js:25:13:25:17 | thing | Missing await. The value 'thing' is always a promise. |
+| tst.js:48:12:48:16 | thing | Missing await. The value 'thing' is always a promise. |

javascript/ql/test/query-tests/Expressions/MissingAwait/MissingAwait.qlref

@@ -0,0 +1 @@
+Expressions/MissingAwait.ql

javascript/ql/test/query-tests/Expressions/MissingAwait/tst.js

@@ -0,0 +1,63 @@
+async function getThing() {
+    return something();
+}
+
+function useThing() {
+    let thing = getThing();
+
+    if (thing === undefined) {} // NOT OK
+
+    if (thing == null) {} // NOT OK
+
+    something(thing ? 1 : 2); // NOT OK
+
+    for (let x in thing) { // NOT OK
+        something(x);
+    }
+
+    let obj = something();
+    something(obj[thing]); // NOT OK
+    obj[thing] = 5; // NOT OK
+
+    something(thing + "bar"); // NOT OK
+
+    if (something()) {
+        if (thing) { // NOT OK
+            something(3);
+        }
+    }
+}
+
+async function useThingCorrectly() {
+    let thing = await getThing();
+
+    if (thing === undefined) {} // OK
+
+    if (thing == null) {} // OK
+
+    return thing + "bar"; // OK
+}
+
+async function useThingCorrectly2() {
+    let thing = getThing();
+
+    if (await thing === undefined) {} // OK
+
+    if (await thing == null) {} // OK
+
+    return thing + "bar"; // NOT OK
+}
+
+function getThingSync() {
+    return something();
+}
+
+function useThingPossiblySync(b) {
+    let thing = b ? getThing() : getThingSync();
+
+    if (thing === undefined) {} // OK
+
+    if (thing == null) {} // OK
+
+    return thing + "bar"; // NOT OK - but we don't flag it
+}