JS: Fix jump steps generated by IIFEs and exception flow

javascript/ql/lib/semmle/javascript/StandardLibrary.qll

@@ -69,7 +69,7 @@ private class ArrayIterationCallbackAsPartialInvoke extends DataFlow::PartialInv
  * A flow step propagating the exception thrown from a callback to a method whose name coincides
  * a built-in Array iteration method, such as `forEach` or `map`.
  */
-private class IteratorExceptionStep extends DataFlow::SharedFlowStep {
+private class IteratorExceptionStep extends DataFlow::LegacyFlowStep {
   override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
     exists(DataFlow::MethodCallNode call |
       call.getMethodName() = ["forEach", "each", "map", "filter", "some", "every", "fold", "reduce"] and

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowImplConsistency.qll

@@ -26,6 +26,8 @@ private module ConsistencyConfig implements InputSig<Location, JSDataFlow> {
     or
     n instanceof FlowSummaryDynamicParameterArrayNode
     or
+    n instanceof FlowSummaryDefaultExceptionalReturn
+    or
     n instanceof GenericSynthesizedNode
     or
     n = DataFlow::globalAccessPathRootPseudoNode()

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowNode.qll

@@ -100,6 +100,9 @@ private module Cached {
       //       So it doesn't cause negative recursion but it might look a bit surprising.
       FlowSummaryPrivate::Steps::summaryStoreStep(sn, MkAwaited(), _)
     } or
+    TFlowSummaryDefaultExceptionalReturn(FlowSummaryImpl::Public::SummarizedCallable callable) {
+      not DataFlowPrivate::mentionsExceptionalReturn(callable)
+    } or
     TSynthCaptureNode(VariableCapture::VariableCaptureOutput::SynthesizedCaptureNode node) or
     TGenericSynthesizedNode(AstNode node, string tag, DataFlowPrivate::DataFlowCallable container) {
       any(AdditionalFlowInternal flow).needsSynthesizedNode(node, tag, container)

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll

@@ -112,6 +112,33 @@ class FlowSummaryIntermediateAwaitStoreNode extends DataFlow::Node,
   }
 }
 
+predicate mentionsExceptionalReturn(FlowSummaryImpl::Public::SummarizedCallable callable) {
+  exists(FlowSummaryImpl::Private::SummaryNode node | node.getSummarizedCallable() = callable |
+    FlowSummaryImpl::Private::summaryReturnNode(node, MkExceptionalReturnKind())
+    or
+    FlowSummaryImpl::Private::summaryOutNode(_, node, MkExceptionalReturnKind())
+  )
+}
+
+/**
+ * Exceptional return node in a summarized callable whose summary does not mention `ReturnValue[exception]`.
+ *
+ * By default, every call inside such a callable will forward their exceptional return to the caller's
+ * exceptional return, i.e. exceptions are not caught.
+ */
+class FlowSummaryDefaultExceptionalReturn extends DataFlow::Node,
+  TFlowSummaryDefaultExceptionalReturn
+{
+  private FlowSummaryImpl::Public::SummarizedCallable callable;
+
+  FlowSummaryDefaultExceptionalReturn() { this = TFlowSummaryDefaultExceptionalReturn(callable) }
+
+  FlowSummaryImpl::Public::SummarizedCallable getSummarizedCallable() { result = callable }
+
+  cached
+  override string toString() { result = "[default exceptional return] " + callable }
+}
+
 class CaptureNode extends DataFlow::Node, TSynthCaptureNode {
   /** Gets the underlying node from the variable-capture library. */
   VariableCaptureOutput::SynthesizedCaptureNode getNode() {
@@ -296,6 +323,9 @@ private predicate returnNodeImpl(DataFlow::Node node, ReturnKind kind) {
   )
   or
   FlowSummaryImpl::Private::summaryReturnNode(node.(FlowSummaryNode).getSummaryNode(), kind)
+  or
+  node instanceof FlowSummaryDefaultExceptionalReturn and
+  kind = MkExceptionalReturnKind()
 }
 
 private DataFlow::Node getAnOutNodeImpl(DataFlowCall call, ReturnKind kind) {
@@ -311,6 +341,10 @@ private DataFlow::Node getAnOutNodeImpl(DataFlowCall call, ReturnKind kind) {
   or
   FlowSummaryImpl::Private::summaryOutNode(call.(SummaryCall).getReceiver(),
     result.(FlowSummaryNode).getSummaryNode(), kind)
+  or
+  kind = MkExceptionalReturnKind() and
+  result.(FlowSummaryDefaultExceptionalReturn).getSummarizedCallable() =
+    call.(SummaryCall).getSummarizedCallable()
 }
 
 class ReturnNode extends DataFlow::Node {
@@ -404,6 +438,19 @@ abstract class LibraryCallable extends string {
   DataFlow::InvokeNode getACallSimple() { none() }
 }
 
+/** Internal subclass of `LibraryCallable`, whose member predicates should not be visible on `SummarizedCallable`. */
+abstract class LibraryCallableInternal extends LibraryCallable {
+  bindingset[this]
+  LibraryCallableInternal() { any() }
+
+  /**
+   * Gets a call to this library callable.
+   *
+   * Same as `getACall()` but is evaluated later and may depend negatively on `getACall()`.
+   */
+  DataFlow::InvokeNode getACallStage2() { none() }
+}
+
 private predicate isParameterNodeImpl(Node p, DataFlowCallable c, ParameterPosition pos) {
   exists(Parameter parameter |
     parameter = c.asSourceCallable().(Function).getParameter(pos.asPositional()) and
@@ -505,6 +552,8 @@ DataFlowCallable nodeGetEnclosingCallable(Node node) {
   or
   result.asLibraryCallable() = node.(FlowSummaryIntermediateAwaitStoreNode).getSummarizedCallable()
   or
+  result.asLibraryCallable() = node.(FlowSummaryDefaultExceptionalReturn).getSummarizedCallable()
+  or
   node = TGenericSynthesizedNode(_, _, result)
 }
 
@@ -865,6 +914,8 @@ class SummaryCall extends DataFlowCall, MkSummaryCall {
 
   /** Gets the receiver node. */
   FlowSummaryImpl::Private::SummaryNode getReceiver() { result = receiver }
+
+  FlowSummaryImpl::Public::SummarizedCallable getSummarizedCallable() { result = enclosingCallable }
 }
 
 /**
@@ -976,7 +1027,11 @@ DataFlowCallable viableCallable(DataFlowCall node) {
   or
   exists(LibraryCallable callable |
     result = MkLibraryCallable(callable) and
-    node.asOrdinaryCall() = [callable.getACall(), callable.getACallSimple()]
+    node.asOrdinaryCall() =
+      [
+        callable.getACall(), callable.getACallSimple(),
+        callable.(LibraryCallableInternal).getACallStage2()
+      ]
   )
   or
   result.asSourceCallableNotExterns() = node.asImpliedLambdaCall()
@@ -1217,14 +1272,29 @@ predicate simpleLocalFlowStep(Node node1, Node node2) {
 
 predicate localMustFlowStep(Node node1, Node node2) { node1 = node2.getImmediatePredecessor() }
 
+/**
+ * Holds if `node1 -> node2` should be removed as a jump step.
+ *
+ * Currently this is done as a workaround for the local steps generated from IIFEs.
+ */
+private predicate excludedJumpStep(Node node1, Node node2) {
+  exists(ImmediatelyInvokedFunctionExpr iife |
+    iife.argumentPassing(node2.asExpr(), node1.asExpr())
+    or
+    node1 = iife.getAReturnedExpr().flow() and
+    node2 = iife.getInvocation().flow()
+  )
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` through a non-local step
  * that does not follow a call edge. For example, a step through a global
  * variable.
  */
 predicate jumpStep(Node node1, Node node2) {
   valuePreservingStep(node1, node2) and
-  node1.getContainer() != node2.getContainer()
+  node1.getContainer() != node2.getContainer() and
+  not excludedJumpStep(node1, node2)
   or
   FlowSummaryPrivate::Steps::summaryJumpStep(node1.(FlowSummaryNode).getSummaryNode(),
     node2.(FlowSummaryNode).getSummaryNode())

javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll

@@ -9,6 +9,7 @@ private import sharedlib.DataFlowImplCommon
 private import sharedlib.FlowSummaryImpl::Private as Private
 private import sharedlib.FlowSummaryImpl::Public
 private import codeql.dataflow.internal.AccessPathSyntax as AccessPathSyntax
+private import semmle.javascript.internal.flow_summaries.ExceptionFlow
 
 /**
  * A class of callables that are candidates for flow summary modeling.
@@ -131,7 +132,11 @@ ReturnKind getStandardReturnValueKind() { result = MkNormalReturnKind() and Stag
 private module FlowSummaryStepInput implements Private::StepsInputSig {
   DataFlowCall getACall(SummarizedCallable sc) {
     exists(LibraryCallable callable | callable = sc |
-      result.asOrdinaryCall() = [callable.getACall(), callable.getACallSimple()]
+      result.asOrdinaryCall() =
+        [
+          callable.getACall(), callable.getACallSimple(),
+          callable.(LibraryCallableInternal).getACallStage2()
+        ]
     )
   }
 }

javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll

@@ -8,7 +8,7 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
   private js::Function getLambdaFromVariable(js::LocalVariable variable) {
     result.getVariable() = variable
     or
-    result = variable.getAnAssignedExpr()
+    result = variable.getAnAssignedExpr().getUnderlyingValue()
     or
     exists(js::ClassDeclStmt cls |
       result = cls.getConstructor().getBody() and
@@ -23,35 +23,11 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
     or
     isTopLevelLike(container.(js::ImmediatelyInvokedFunctionExpr).getEnclosingContainer())
     or
-    // Functions declared in a top-level with no parameters and can't generate flow-through, except through 'this'
-    // which we rule out with a few syntactic checks. In this case we treat its captured variables as singletons.
-    // NOTE: This was done to prevent a blow-up in fiddlesalad where a function called 'Runtime' captures 7381 variables but is only called once.
-    exists(js::Function fun |
-      container = fun and
-      fun.getNumParameter() = 0 and
-      isTopLevelLike(fun.getEnclosingContainer()) and
-      not mayHaveFlowThroughThisArgument(fun)
-    )
-    or
-    // Container declaring >100 captured variables tend to be singletons and are too expensive anyway
+    // Containers declaring >100 captured variables tend to be singletons and are too expensive anyway
     strictcount(js::LocalVariable v | v.isCaptured() and v.getDeclaringContainer() = container) >
       100
   }
 
-  private predicate hasLocalConstructorCall(js::Function fun) {
-    fun = getLambdaFromVariable(any(js::NewExpr e).getCallee().(js::VarAccess).getVariable())
-  }
-
-  private predicate mayHaveFlowThroughThisArgument(js::Function fun) {
-    any(js::ThisExpr e).getBinder() = fun and
-    not hasLocalConstructorCall(fun) and // 'this' argument is assumed to be a fresh object
-    (
-      exists(fun.getAReturnedExpr())
-      or
-      exists(js::YieldExpr e | e.getContainer() = fun)
-    )
-  }
-
   class CapturedVariable extends LocalVariableOrThis {
     CapturedVariable() {
       DataFlowImplCommon::forceCachingInSameStage() and
@@ -172,9 +148,11 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
     predicate hasAliasedAccess(Expr e) {
       e = this
       or
+      e.(js::Expr).getUnderlyingValue() = this
+      or
       exists(js::LocalVariable variable |
         this = getLambdaFromVariable(variable) and
-        e = variable.getAnAccess()
+        e.(js::Expr).getUnderlyingValue() = variable.getAnAccess()
       )
     }
   }

javascript/ql/lib/semmle/javascript/filters/ClassifyFiles.qll

@@ -61,6 +61,8 @@ predicate isTestFile(File f) {
   )
   or
   f.getAbsolutePath().regexpMatch(".*/__(mocks|tests)__/.*")
+  or
+  f.getBaseName().matches("%.test.%")
 }
 
 /**

javascript/ql/lib/semmle/javascript/internal/flow_summaries/AllFlowSummaries.qll

@@ -1,6 +1,7 @@
 private import AmbiguousCoreMethods
 private import Arrays
 private import AsyncAwait
+private import ExceptionFlow
 private import ForOfLoops
 private import Generators
 private import Iterators

javascript/ql/lib/semmle/javascript/internal/flow_summaries/ExceptionFlow.qll

@@ -0,0 +1,48 @@
+/**
+ * Contains a summary for propagating exceptions out of callbacks
+ */
+
+private import javascript
+private import FlowSummaryUtil
+private import semmle.javascript.dataflow.internal.AdditionalFlowInternal
+private import semmle.javascript.dataflow.internal.DataFlowPrivate
+private import semmle.javascript.dataflow.FlowSummary
+private import semmle.javascript.internal.flow_summaries.Promises
+
+private predicate isCallback(DataFlow::SourceNode node) {
+  node instanceof DataFlow::FunctionNode
+  or
+  node instanceof DataFlow::PartialInvokeNode
+  or
+  exists(DataFlow::SourceNode prev |
+    isCallback(prev) and
+    DataFlow::argumentPassingStep(_, prev.getALocalUse(), _, node)
+  )
+}
+
+/**
+ * Summary that propagates exceptions out of callbacks back to the caller.
+ *
+ * This summary only applies to calls that have no other call targets.
+ * See also `FlowSummaryDefaultExceptionalReturn`, which handles calls that have a summary target,
+ * but where the summary does not mention `ReturnValue[exception]`.
+ */
+private class ExceptionFlowSummary extends SummarizedCallable, LibraryCallableInternal {
+  ExceptionFlowSummary() { this = "Exception propagator" }
+
+  override DataFlow::CallNode getACallStage2() {
+    not exists(result.getACallee()) and
+    not exists(SummarizedCallable c | result = [c.getACall(), c.getACallSimple()]) and
+    // Avoid a few common cases where the exception should not propagate back
+    not result.getCalleeName() = ["addEventListener", EventEmitter::on()] and
+    not result = promiseConstructorRef().getAnInvocation() and
+    // Restrict to cases where a callback is known to flow in, as lambda flow in DataFlowImplCommon blows up otherwise
+    isCallback(result.getAnArgument().getALocalSource())
+  }
+
+  override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+    preservesValue = true and
+    input = "Argument[0..].ReturnValue[exception]" and
+    output = "ReturnValue[exception]"
+  }
+}

javascript/ql/lib/semmle/javascript/internal/flow_summaries/Promises.qll

@@ -6,7 +6,7 @@ private import javascript
 private import semmle.javascript.dataflow.FlowSummary
 private import FlowSummaryUtil
 
-private DataFlow::SourceNode promiseConstructorRef() {
+DataFlow::SourceNode promiseConstructorRef() {
   result = Promises::promiseConstructorRef()
   or
   result = DataFlow::moduleImport("bluebird")

javascript/ql/lib/semmle/javascript/security/dataflow/InsecureRandomnessQuery.qll

@@ -11,6 +11,7 @@ import javascript
 private import semmle.javascript.security.SensitiveActions
 import InsecureRandomnessCustomizations::InsecureRandomness
 private import InsecureRandomnessCustomizations::InsecureRandomness as InsecureRandomness
+private import semmle.javascript.filters.ClassifyFiles as ClassifyFiles
 
 /**
  * A taint tracking configuration for random values that are not cryptographically secure.
@@ -20,7 +21,11 @@ module InsecureRandomnessConfig implements DataFlow::ConfigSig {
 
   predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-  predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof Sanitizer
+    or
+    ClassifyFiles::isTestFile(node.getFile())
+  }
 
   predicate isBarrierOut(DataFlow::Node node) {
     // stop propagation at the sinks to avoid double reporting

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -17,6 +17,7 @@ legacyDataFlowDifference
 | callbacks.js:44:17:44:24 | source() | callbacks.js:38:35:38:35 | x | only flow with NEW data flow library |
 | capture-flow.js:89:13:89:20 | source() | capture-flow.js:89:6:89:21 | test3c(source()) | only flow with NEW data flow library |
 | capture-flow.js:101:12:101:19 | source() | capture-flow.js:102:6:102:20 | test5("safe")() | only flow with OLD data flow library |
+| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint | only flow with OLD data flow library |
 | constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:40:8:40:14 | e.taint | only flow with NEW data flow library |
 | constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:44:8:44:19 | f_safe.taint | only flow with NEW data flow library |
 | constructor-calls.js:20:15:20:22 | source() | constructor-calls.js:39:8:39:14 | e.param | only flow with NEW data flow library |
@@ -109,7 +110,6 @@ flow
 | capture-flow.js:101:12:101:19 | source() | capture-flow.js:101:6:101:22 | test5(source())() |
 | capture-flow.js:110:12:110:19 | source() | capture-flow.js:106:14:106:14 | x |
 | capture-flow.js:118:37:118:44 | source() | capture-flow.js:114:14:114:14 | x |
-| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint |
 | capture-flow.js:126:25:126:32 | source() | capture-flow.js:129:14:129:26 | orderingTaint |
 | capture-flow.js:177:26:177:33 | source() | capture-flow.js:173:14:173:14 | x |
 | capture-flow.js:187:34:187:41 | source() | capture-flow.js:183:14:183:14 | x |

javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected

@@ -11,6 +11,7 @@ legacyDataFlowDifference
 | callbacks.js:44:17:44:24 | source() | callbacks.js:38:35:38:35 | x | only flow with NEW data flow library |
 | capture-flow.js:89:13:89:20 | source() | capture-flow.js:89:6:89:21 | test3c(source()) | only flow with NEW data flow library |
 | capture-flow.js:101:12:101:19 | source() | capture-flow.js:102:6:102:20 | test5("safe")() | only flow with OLD data flow library |
+| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint | only flow with OLD data flow library |
 | constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:40:8:40:14 | e.taint | only flow with NEW data flow library |
 | constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:44:8:44:19 | f_safe.taint | only flow with NEW data flow library |
 | constructor-calls.js:20:15:20:22 | source() | constructor-calls.js:39:8:39:14 | e.param | only flow with NEW data flow library |
@@ -84,7 +85,6 @@ flow
 | capture-flow.js:101:12:101:19 | source() | capture-flow.js:101:6:101:22 | test5(source())() |
 | capture-flow.js:110:12:110:19 | source() | capture-flow.js:106:14:106:14 | x |
 | capture-flow.js:118:37:118:44 | source() | capture-flow.js:114:14:114:14 | x |
-| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint |
 | capture-flow.js:126:25:126:32 | source() | capture-flow.js:129:14:129:26 | orderingTaint |
 | capture-flow.js:177:26:177:33 | source() | capture-flow.js:173:14:173:14 | x |
 | capture-flow.js:187:34:187:41 | source() | capture-flow.js:183:14:183:14 | x |

javascript/ql/test/library-tests/TaintTracking/capture-flow.js

@@ -120,7 +120,7 @@ global.doEscape(testEscapeViaReturn(source()));
 function ordering() {
     var orderingTaint;
     global.addEventListener('click', () => {
-        sink(orderingTaint); // NOT OK
+        sink(orderingTaint); // NOT OK [INCONSISTENCY]
     });
     global.addEventListener('load', () => {
         orderingTaint = source();