remove ordinary return flow from generator functions

Author: erik-krogh

javascript/ql/src/semmle/javascript/Functions.qll

@@ -204,6 +204,9 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
   /** Holds if this function is an asynchronous function. */
   predicate isAsync() { isAsync(this) }
 
+  /** Holds if this function is not asynchronous and also not a generator. */
+  predicate isOrdinary() { not isAsync() and not isGenerator() }
+
   /** Gets the enclosing function or toplevel of this function. */
   override StmtContainer getEnclosingContainer() { result = getEnclosingStmt().getContainer() }
 

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

@@ -1031,7 +1031,7 @@ private predicate storeStep(
   summary = PathSummary::level()
   or
   exists(Function f, DataFlow::Node mid, DataFlow::Node invk |
-    not f.isAsync() and invk = succ
+    f.isOrdinary() and invk = succ
     or
     // store in an immediately awaited function call
     f.isAsync() and
@@ -1156,7 +1156,7 @@ private predicate loadStep(
   summary = PathSummary::level()
   or
   exists(Function f, DataFlow::Node read, DataFlow::Node invk |
-    not f.isAsync() and invk = succ
+    f.isOrdinary() and invk = succ
     or
     // load from an immediately awaited function call
     f.isAsync() and

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

@@ -1495,7 +1495,7 @@ module DataFlow {
     )
     or
     // from returned expr to the FunctionReturnNode.
-    exists(Function f | not f.isAsync() |
+    exists(Function f | f.isOrdinary() |
       DataFlow::functionReturnNode(succ, f) and pred = valueNode(f.getAReturnedExpr())
     )
   }

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

@@ -243,7 +243,7 @@ private module NodeTracking {
     basicStoreStep(pred, succ, prop) and
     summary = PathSummary::level()
     or
-    exists(Function f, DataFlow::Node mid | not f.isAsync() |
+    exists(Function f, DataFlow::Node mid | f.isOrdinary() |
       // `f` stores its parameter `pred` in property `prop` of a value that flows back to the caller,
       // and `succ` is an invocation of `f`
       reachableFromInput(f, succ, pred, mid, summary) and
@@ -266,7 +266,7 @@ private module NodeTracking {
     basicLoadStep(pred, succ, prop) and
     summary = PathSummary::level()
     or
-    exists(Function f, DataFlow::SourceNode parm | not f.isAsync() |
+    exists(Function f, DataFlow::SourceNode parm | f.isOrdinary() |
       argumentPassing(succ, pred, f, parm) and
       reachesReturn(f, parm.getAPropertyRead(prop), summary)
     )

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

@@ -71,7 +71,7 @@ predicate localExceptionStep(DataFlow::Node pred, DataFlow::Node succ) {
  */
 predicate localExceptionStepWithAsyncFlag(DataFlow::Node pred, DataFlow::Node succ, boolean async) {
   exists(DataFlow::Node target | target = getThrowTarget(pred) |
-    async = false and
+    async = false and // this also covers generators - as the behavior of exceptions is close enough to the behavior of ordinary functions when it comes to exceptions (assuming that the iterator does not cross function boundaries).
     succ = target and
     not succ = any(DataFlow::FunctionNode f | f.getFunction().isAsync()).getExceptionalReturn()
     or

javascript/ql/test/library-tests/Generators/DataFlow.expected

@@ -0,0 +1,12 @@
+import javascript
+import testUtilities.ConsistencyChecking
+
+class GeneratorFlowConfig extends DataFlow::Configuration {
+  GeneratorFlowConfig() { this = "GeneratorFlowConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source.asExpr().getStringValue() = "source" }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink = any(DataFlow::CallNode call | call.getCalleeName() = "sink").getAnArgument()
+  }
+}

javascript/ql/test/library-tests/Generators/DataFlow.ql

@@ -0,0 +1,17 @@
+(function () {
+  var source = "source";
+  sink(source); // NOT OK
+
+  function *gen1() {
+    yield source;
+  }
+  for (const x of gen1()) {
+    sink(x); // NOT OK - but not found yet [INCONSISTENCY]
+  }
+
+  function *gen2() {
+    yield "safe";
+    return source;
+  }
+  sink(gen2()); // OK
+});