JS: Handle barrier guards on phi inputs

javascript/ql/lib/change-notes/2026-10-22-barrier-guard-phi-inputs.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* JavaScript barrier guards now sanitize values after branches that replace an unsafe value with a safe default.

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

@@ -68,6 +68,7 @@ deprecated module;
 private import javascript
 private import internal.FlowSteps
 private import internal.AccessPaths
+private import internal.DataFlowNode
 private import semmle.javascript.Unit
 private import semmle.javascript.internal.CachedStages
 private import AdditionalFlowSteps
@@ -526,7 +527,7 @@ deprecated private predicate barrierGuardBlocksEdge(
   |
     bb = getADominatedBasicBlock(guard, cond) and
     pred = DataFlow::ssaDefinitionNode(input) and
-    succ = DataFlow::ssaDefinitionNode(phi) and
+    succ = TSsaPhiInputNode(phi, bb) and
     input = phi.getInputFromBlock(bb) and
     outcome = cond.getOutcome() and
     barrierGuardBlocksExpr(guard, outcome, input.getAUse(), label)

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

@@ -298,6 +298,26 @@ module DataFlow {
     override AstNode getAstNode() { none() }
   }
 
+  /**
+   * A synthetic node representing an input edge to an SSA phi node.
+   */
+  private class SsaPhiInputNode extends Node, TSsaPhiInputNode {
+    SsaPhiNode phi;
+    BasicBlock input;
+
+    SsaPhiInputNode() { this = TSsaPhiInputNode(phi, input) }
+
+    override BasicBlock getBasicBlock() { result = input }
+
+    override Location getLocation() { result = input.getLocation() }
+
+    override string toString() { result = "[input] " + phi.getSourceVariable().getName() }
+
+    override File getFile() { result = input.getFile() }
+
+    override AstNode getAstNode() { none() }
+  }
+
   /**
    * A node in the data flow graph which corresponds to a `@property`.
    */
@@ -1743,9 +1763,20 @@ module DataFlow {
     // Flow through implicit SSA nodes
     exists(SsaImplicitDefinition ssa | succ = TSsaDefNode(ssa) |
       // from the inputs of phi and pi nodes into the node itself
+      not ssa instanceof SsaPhiNode and
       pred = TSsaDefNode(ssa.(SsaPseudoDefinition).getAnInput().getDefinition())
     )
     or
+    exists(SsaPhiNode phi, BasicBlock input, SsaVariable inputVar |
+      inputVar = phi.getInputFromBlock(input)
+    |
+      pred = TSsaDefNode(inputVar.getDefinition()) and
+      succ = TSsaPhiInputNode(phi, input)
+      or
+      pred = TSsaPhiInputNode(phi, input) and
+      succ = TSsaDefNode(phi)
+    )
+    or
     exists(Expr predExpr, Expr succExpr |
       pred = TValueNode(predExpr) and succ = TValueNode(succExpr)
     |

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

@@ -6,6 +6,7 @@
 
 private import javascript
 private import semmle.javascript.dataflow.internal.AccessPaths
+private import semmle.javascript.dataflow.internal.DataFlowNode
 private import semmle.javascript.dataflow.internal.DataFlowPrivate as DataFlowPrivate
 private import semmle.javascript.dataflow.internal.sharedlib.Ssa as Ssa2
 
@@ -389,6 +390,19 @@ module MakeStateBarrierGuard<
       nd = DataFlow::valueNode(use)
     )
     or
+    exists(
+      BarrierGuard guard, SsaPhiNode phi, SsaVariable input, BasicBlock bb, ConditionGuardNode cond,
+      boolean outcome
+    |
+      nd = TSsaPhiInputNode(phi, bb) and
+      input = phi.getInputFromBlock(bb) and
+      barrierGuardIsRelevant(guard) and
+      guard.getEnclosingExpr() = cond.getTest() and
+      cond.dominates(bb) and
+      outcome = cond.getOutcome() and
+      barrierGuardBlocksExpr(guard, outcome, input.getAUse(), state)
+    )
+    or
     ssa2BlocksNode(nd, state)
   }
 

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

@@ -34,6 +34,8 @@ private module Cached {
     TValueNode(AST::ValueNode nd) or
     /** An SSA node from the legacy SSA library */
     TSsaDefNode(SsaDefinition d) or
+    /** An input edge to a phi node from the legacy SSA library */
+    TSsaPhiInputNode(SsaPhiNode phi, BasicBlock input) { exists(phi.getInputFromBlock(input)) } or
     /** Use of a variable or 'this', with flow from a post-update node (from an earlier use) */
     TSsaUseNode(ControlFlowNode use) { use = any(Ssa2::SsaConfig::SourceVariable v).getAUse() } or
     /** A synthesized variable read (new SSA library). Definitions are represented by TSsaDefNode. */
@@ -130,8 +132,8 @@ private module Cached {
 import Cached
 
 private class TEarlyStageNode =
-  TValueNode or TSsaDefNode or TCapturedVariableNode or TPropNode or TRestPatternNode or
-      TElementPatternNode or TElementNode or TReflectiveCallNode or TThisNode or
+  TValueNode or TSsaDefNode or TSsaPhiInputNode or TCapturedVariableNode or TPropNode or
+      TRestPatternNode or TElementPatternNode or TElementNode or TReflectiveCallNode or TThisNode or
       TFunctionSelfReferenceNode or TDestructuredModuleImportNode or THtmlAttributeNode or
       TFunctionReturnNode or TExceptionalFunctionReturnNode or TExceptionalInvocationReturnNode or
       TGlobalAccessPathRoot or TTemplatePlaceholderTag or TReflectiveParametersNode or

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

@@ -683,6 +683,8 @@ predicate nodeIsHidden(Node node) {
   node instanceof SsaUseNode
   or
   node instanceof SsaSynthReadNode
+  or
+  node instanceof TSsaPhiInputNode
 }
 
 predicate neverSkipInPathGraph(Node node) {

javascript/ql/test/library-tests/BarrierGuardPhiInputs/test.ql

@@ -0,0 +1,47 @@
+import javascript
+import utils.test.InlineExpectationsTest
+
+module TestConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source.(DataFlow::InvokeNode).getCalleeName() = "SOURCE"
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(DataFlow::InvokeNode call |
+      call.getCalleeName() = "SINK" and
+      sink = call.getArgument(0)
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node = DataFlow::MakeBarrierGuard<SimpleBarrierGuard>::getABarrierNode()
+  }
+}
+
+module TestFlow = DataFlow::Global<TestConfig>;
+
+class SimpleBarrierGuard extends DataFlow::InvokeNode {
+  SimpleBarrierGuard() { this.getCalleeName() = "BARRIER" }
+
+  predicate blocksExpr(boolean outcome, Expr expr) {
+    outcome = true and
+    expr = this.getArgument(0).asExpr()
+  }
+}
+
+module InlineTest implements TestSig {
+  string getARelevantTag() { result = "flow" }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "flow" and
+    element = "" and
+    exists(DataFlow::InvokeNode source, DataFlow::Node sink |
+      TestFlow::flow(source, sink) and
+      source.getCalleeName() = "SOURCE" and
+      value = source.getArgument(0).getStringValue() and
+      location = sink.getLocation()
+    )
+  }
+}
+
+import MakeTest<InlineTest>

javascript/ql/test/library-tests/BarrierGuardPhiInputs/tst.js

@@ -0,0 +1,35 @@
+function overwriteOnFailure() {
+  let x = SOURCE("overwrite");
+  if (!BARRIER(x)) {
+    x = "safe";
+  }
+  SINK(x);
+}
+
+function explicitElse() {
+  let x = SOURCE("else");
+  if (BARRIER(x)) {
+    use(x);
+  } else {
+    x = "safe";
+  }
+  SINK(x);
+}
+
+function preserveUnguardedInput() {
+  let x = SOURCE("guarded");
+  if (!BARRIER(x)) {
+    x = SOURCE("fallback");
+  }
+  SINK(x); // $ flow=fallback
+}
+
+function returnOnFailure() {
+  let x = SOURCE("return");
+  if (!BARRIER(x)) {
+    return;
+  }
+  SINK(x);
+}
+
+SINK(SOURCE("control")); // $ flow=control

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

@@ -31,7 +31,6 @@ legacyDataFlowDifference
 | object-bypass-sanitizer.js:35:29:35:36 | source() | object-bypass-sanitizer.js:23:14:23:20 | obj.foo | only flow with OLD data flow library |
 | object-bypass-sanitizer.js:35:29:35:36 | source() | object-bypass-sanitizer.js:28:10:28:30 | sanitiz ... bj).foo | only flow with OLD data flow library |
 | promise.js:12:20:12:27 | source() | promise.js:13:8:13:23 | resolver.promise | only flow with OLD data flow library |
-| sanitizer-guards.js:57:11:57:18 | source() | sanitizer-guards.js:64:8:64:8 | x | only flow with NEW data flow library |
 | spread.js:4:15:4:22 | source() | spread.js:18:8:18:8 | y | only flow with NEW data flow library |
 | spread.js:4:15:4:22 | source() | spread.js:24:8:24:8 | y | only flow with NEW data flow library |
 | tst.js:2:13:2:20 | source() | tst.js:17:10:17:10 | a | only flow with OLD data flow library |
@@ -265,7 +264,6 @@ flow
 | sanitizer-guards.js:13:14:13:21 | source() | sanitizer-guards.js:26:9:26:14 | this.x |
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:45:8:45:8 | x |
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x |
-| sanitizer-guards.js:57:11:57:18 | source() | sanitizer-guards.js:64:8:64:8 | x |
 | sanitizer-guards.js:68:11:68:18 | source() | sanitizer-guards.js:75:8:75:8 | x |
 | sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:81:8:81:8 | x |
 | sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:84:10:84:10 | x |

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

@@ -22,7 +22,6 @@ legacyDataFlowDifference
 | nested-props.js:14:15:14:22 | source() | nested-props.js:15:10:15:16 | obj.x.y | only flow with NEW data flow library |
 | nested-props.js:27:18:27:25 | source() | nested-props.js:28:10:28:14 | obj.x | only flow with NEW data flow library |
 | nested-props.js:51:22:51:29 | source() | nested-props.js:52:10:52:16 | obj.x.y | only flow with NEW data flow library |
-| sanitizer-guards.js:57:11:57:18 | source() | sanitizer-guards.js:64:8:64:8 | x | only flow with NEW data flow library |
 | spread.js:4:15:4:22 | source() | spread.js:18:8:18:8 | y | only flow with NEW data flow library |
 | spread.js:4:15:4:22 | source() | spread.js:24:8:24:8 | y | only flow with NEW data flow library |
 | tst.js:2:13:2:20 | source() | tst.js:35:14:35:16 | ary | only flow with NEW data flow library |
@@ -174,7 +173,6 @@ flow
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:45:8:45:8 | x |
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x |
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:52:10:52:10 | x |
-| sanitizer-guards.js:57:11:57:18 | source() | sanitizer-guards.js:64:8:64:8 | x |
 | sanitizer-guards.js:68:11:68:18 | source() | sanitizer-guards.js:75:8:75:8 | x |
 | sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:81:8:81:8 | x |
 | sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:84:10:84:10 | x |

javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js

@@ -61,7 +61,7 @@ function phi() {
   } else {
     x = null;
   }
-  sink(x); // OK [INCONSISTENCY] - dataflow2 cannot block the phi edge
+  sink(x); // OK
 }
 
 function phi2() {

javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected

@@ -47,7 +47,6 @@
 | lib/lib.js:228:10:228:25 | "rm -rf " + name | lib/lib.js:227:39:227:42 | name | lib/lib.js:228:22:228:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:227:39:227:42 | name | library input | lib/lib.js:228:2:228:26 | cp.exec ... + name) | shell command |
 | lib/lib.js:236:10:236:25 | "rm -rf " + name | lib/lib.js:227:39:227:42 | name | lib/lib.js:236:22:236:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:227:39:227:42 | name | library input | lib/lib.js:236:2:236:26 | cp.exec ... + name) | shell command |
 | lib/lib.js:249:10:249:25 | "rm -rf " + name | lib/lib.js:248:42:248:45 | name | lib/lib.js:249:22:249:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:248:42:248:45 | name | library input | lib/lib.js:249:2:249:26 | cp.exec ... + name) | shell command |
-| lib/lib.js:253:10:253:28 | "rm -rf " + cleaned | lib/lib.js:248:42:248:45 | name | lib/lib.js:253:22:253:28 | cleaned | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:248:42:248:45 | name | library input | lib/lib.js:253:2:253:29 | cp.exec ... leaned) | shell command |
 | lib/lib.js:258:10:258:25 | "rm -rf " + name | lib/lib.js:257:35:257:38 | name | lib/lib.js:258:22:258:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:257:35:257:38 | name | library input | lib/lib.js:258:2:258:26 | cp.exec ... + name) | shell command |
 | lib/lib.js:261:11:261:33 | "rm -rf ...  + name | lib/lib.js:257:35:257:38 | name | lib/lib.js:261:30:261:33 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:257:35:257:38 | name | library input | lib/lib.js:261:3:261:34 | cp.exec ... + name) | shell command |
 | lib/lib.js:268:10:268:32 | "rm -rf ... version | lib/lib.js:267:46:267:48 | obj | lib/lib.js:268:22:268:32 | obj.version | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:267:46:267:48 | obj | library input | lib/lib.js:268:2:268:33 | cp.exec ... ersion) | shell command |
@@ -157,13 +156,7 @@ edges
 | lib/lib.js:216:39:216:42 | name | lib/lib.js:224:22:224:25 | name | provenance |  |
 | lib/lib.js:227:39:227:42 | name | lib/lib.js:228:22:228:25 | name | provenance |  |
 | lib/lib.js:227:39:227:42 | name | lib/lib.js:236:22:236:25 | name | provenance |  |
-| lib/lib.js:239:28:239:28 | s | lib/lib.js:245:9:245:9 | s | provenance |  |
 | lib/lib.js:248:42:248:45 | name | lib/lib.js:249:22:249:25 | name | provenance |  |
-| lib/lib.js:248:42:248:45 | name | lib/lib.js:251:27:251:30 | name | provenance |  |
-| lib/lib.js:251:6:251:12 | cleaned | lib/lib.js:253:22:253:28 | cleaned | provenance |  |
-| lib/lib.js:251:16:251:31 | cleanInput(name) | lib/lib.js:251:6:251:12 | cleaned | provenance |  |
-| lib/lib.js:251:27:251:30 | name | lib/lib.js:239:28:239:28 | s | provenance |  |
-| lib/lib.js:251:27:251:30 | name | lib/lib.js:251:16:251:31 | cleanInput(name) | provenance |  |
 | lib/lib.js:257:35:257:38 | name | lib/lib.js:258:22:258:25 | name | provenance |  |
 | lib/lib.js:257:35:257:38 | name | lib/lib.js:261:30:261:33 | name | provenance |  |
 | lib/lib.js:267:46:267:48 | obj | lib/lib.js:268:22:268:24 | obj | provenance |  |
@@ -329,14 +322,8 @@ nodes
 | lib/lib.js:227:39:227:42 | name | semmle.label | name |
 | lib/lib.js:228:22:228:25 | name | semmle.label | name |
 | lib/lib.js:236:22:236:25 | name | semmle.label | name |
-| lib/lib.js:239:28:239:28 | s | semmle.label | s |
-| lib/lib.js:245:9:245:9 | s | semmle.label | s |
 | lib/lib.js:248:42:248:45 | name | semmle.label | name |
 | lib/lib.js:249:22:249:25 | name | semmle.label | name |
-| lib/lib.js:251:6:251:12 | cleaned | semmle.label | cleaned |
-| lib/lib.js:251:16:251:31 | cleanInput(name) | semmle.label | cleanInput(name) |
-| lib/lib.js:251:27:251:30 | name | semmle.label | name |
-| lib/lib.js:253:22:253:28 | cleaned | semmle.label | cleaned |
 | lib/lib.js:257:35:257:38 | name | semmle.label | name |
 | lib/lib.js:258:22:258:25 | name | semmle.label | name |
 | lib/lib.js:261:30:261:33 | name | semmle.label | name |
@@ -452,6 +439,5 @@ nodes
 | lib/subLib/index.js:13:44:13:46 | arr | semmle.label | arr |
 | lib/subLib/index.js:14:22:14:24 | arr | semmle.label | arr |
 subpaths
-| lib/lib.js:251:27:251:30 | name | lib/lib.js:239:28:239:28 | s | lib/lib.js:245:9:245:9 | s | lib/lib.js:251:16:251:31 | cleanInput(name) |
 | lib/lib.js:340:25:340:25 | n | lib/lib.js:329:13:329:13 | x | lib/lib.js:330:9:330:9 | x | lib/lib.js:340:22:340:26 | id(n) |
 | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:431:23:431:26 | last | lib/lib.js:437:9:437:11 | arr [ArrayElement] | lib/lib.js:428:14:428:58 | build(" ...  + '-') |

javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js

@@ -250,7 +250,7 @@ module.exports.goodSanitizer = function (name) { // $ Source
 
 	var cleaned = cleanInput(name);
 
-	cp.exec("rm -rf " + cleaned); // $ SPURIOUS: Alert - SanitizingRegExpTest is not able to generate a barrier edge for an edge into a phi node.
+	cp.exec("rm -rf " + cleaned); // OK - sanitized by cleanInput
 }
 
 var fs = require("fs");