Address review comments

java/ql/test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.expected

@@ -14,7 +14,7 @@ edges
 | RuntimeExecTest.java:25:66:25:71 | script : String | RuntimeExecTest.java:25:42:25:72 | {...} : String[] [[]] : String | provenance |  |
 | RuntimeExecTest.java:31:17:31:29 | commandArray2 [post update] : String[] [[]] : String | RuntimeExecTest.java:32:43:32:55 | commandArray2 | provenance | Sink:MaD:1 |
 | RuntimeExecTest.java:31:36:31:41 | script : String | RuntimeExecTest.java:31:17:31:29 | commandArray2 [post update] : String[] [[]] : String | provenance |  |
-| RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) | provenance | MaD:5 |
+| RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) | provenance | MaD:5 Sink:MaD:1 |
 | RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [<element>] : String | RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String | provenance | MaD:4 |
 | RuntimeExecTest.java:38:39:38:58 | new String[] : String[] [[]] : String | RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [<element>] : String | provenance | MaD:3 |
 | RuntimeExecTest.java:38:39:38:58 | {...} : String[] [[]] : String | RuntimeExecTest.java:38:39:38:58 | new String[] : String[] [[]] : String | provenance |  |

ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected

@@ -292,8 +292,8 @@ edges
 | summaries.rb:128:1:128:1 | [post] x | summaries.rb:129:6:129:6 | x | provenance |  |
 | summaries.rb:128:14:128:20 | tainted | summaries.rb:128:1:128:1 | [post] x | provenance | MaD:21 |
 | summaries.rb:131:16:131:22 | tainted | summaries.rb:131:1:131:23 | synthetic splat argument | provenance | Sink:MaD:4 |
-| summaries.rb:157:14:160:3 | do ... end [captured tainted] | summaries.rb:158:15:158:21 | tainted | provenance | heuristic-callback |
+| summaries.rb:157:14:160:3 | do ... end [captured tainted] | summaries.rb:158:15:158:21 | tainted | provenance | heuristic-callback Sink:MaD:6 |
-| summaries.rb:157:14:160:3 | do ... end [captured tainted] | summaries.rb:158:15:158:21 | tainted | provenance | heuristic-callback |
+| summaries.rb:157:14:160:3 | do ... end [captured tainted] | summaries.rb:158:15:158:21 | tainted | provenance | heuristic-callback Sink:MaD:6 |
 nodes
 | summaries.rb:1:11:1:36 | call to identity | semmle.label | call to identity |
 | summaries.rb:1:11:1:36 | call to identity | semmle.label | call to identity |

shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll

@@ -164,20 +164,6 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
   module Impl<FullStateConfigSig Config> {
     private class FlowState = Config::FlowState;
 
-    private class NodeEx extends NodeExImpl {
-      NodeEx() {
-        Config::allowImplicitRead(any(Node n | this.isImplicitReadNode(n)), _)
-        or
-        not this.isImplicitReadNode(_)
-      }
-    }
-
-    private class ArgNodeEx extends NodeEx, ArgNodeExImpl { }
-
-    private class ParamNodeEx extends NodeEx, ParamNodeExImpl { }
-
-    private class RetNodeEx extends NodeEx, RetNodeExImpl { }
-
     private module SourceSinkFiltering {
       private import codeql.util.AlertFiltering
 
@@ -1043,10 +1029,15 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
     bindingset[label1, label2]
     pragma[inline_late]
     private string mergeLabels(string label1, string label2) {
-      // Big-step, hidden nodes, and summaries all may need to merge labels.
+      if label2.matches("Sink:%")
-      // These cases are expected to involve at most one non-empty label, so
+      then if label1 = "" then result = label2 else result = label1 + " " + label2
-      // we'll just discard the 2nd+ label for now.
+      else
-      if label1 = "" then result = label2 else result = label1
+        // Big-step, hidden nodes, and summaries all may need to merge labels.
+        // These cases are expected to involve at most one non-empty label, so
+        // we'll just discard the 2nd+ label for now.
+        if label1 = ""
+        then result = label2
+        else result = label1
     }
 
     pragma[nomagic]
@@ -2819,15 +2810,15 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
             pragma[nomagic]
             PathNodeImpl getAnImplicitReadSuccessorAtSink(string label) {
               exists(PathNodeMid readTarget |
-                result = this.getASuccessorImpl(label) and
+                result = this.getASuccessorImpl(_) and
                 localStep(this, readTarget, _) and
                 readTarget.getNodeEx().isImplicitReadNode(_)
               |
                 // last implicit read, leaving the access path empty
-                result = readTarget.projectToSink(_)
+                result = readTarget.projectToSink(label)
                 or
                 // implicit read, leaving the access path non-empty
-                exists(result.getAnImplicitReadSuccessorAtSink(_)) and
+                exists(result.getAnImplicitReadSuccessorAtSink(label)) and
                 result = readTarget
               )
             }
@@ -2859,7 +2850,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
               //  `x [Field]`               -> `x`
               //
               // which the restriction below ensures.
-              not result = this.getAnImplicitReadSuccessorAtSink(label)
+              not result = this.getAnImplicitReadSuccessorAtSink(_)
               or
               exists(string l1, string l2 |
                 result = this.getASuccessorFromNonHidden(l1).getASuccessorIfHidden(l2) and
@@ -2980,18 +2971,15 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
               )
               or
               // a final step to a sink
-              exists(string l2, string sinkmodel, string l2_ |
+              exists(string l2, string sinkLabel |
-                result = this.getSuccMid(l2).projectToSink(sinkmodel) and
+                result = this.getSuccMid(l2).projectToSink(sinkLabel)
-                if l2 = "" then l2_ = l2 else l2_ = l2 + " "
               |
                 not this.isSourceWithLabel(_) and
-                if sinkmodel != "" then label = l2_ + "Sink:" + sinkmodel else label = l2
+                label = mergeLabels(l2, sinkLabel)
                 or
                 exists(string l1 |
                   this.isSourceWithLabel(l1) and
-                  if sinkmodel != ""
+                  label = l1 + mergeLabels(l2, sinkLabel)
-                  then label = l1 + l2_ + "Sink:" + sinkmodel
-                  else label = l1 + l2
                 )
               )
             }
@@ -3057,11 +3045,14 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
                 else any()
             }
 
-            PathNodeSink projectToSink(string model) {
+            PathNodeSink projectToSink(string label) {
-              this.isAtSink() and
+              exists(string model |
-              sinkModel(node, model) and
+                this.isAtSink() and
-              result.getNodeEx() = this.toNormalSinkNodeEx() and
+                sinkModel(node, model) and
-              result.getState() = state
+                result.getNodeEx() = this.toNormalSinkNodeEx() and
+                result.getState() = state and
+                if model != "" then label = "Sink:" + model else label = ""
+              )
             }
           }
 

shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll

@@ -850,7 +850,7 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> Lang> {
 
   class SndLevelScopeOption = SndLevelScopeOption::Option;
 
-  final class NodeExImpl extends TNodeEx {
+  final class NodeEx extends TNodeEx {
     string toString() {
       result = this.asNode().toString()
       or
@@ -899,14 +899,14 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> Lang> {
     Location getLocation() { result = this.projectToNode().getLocation() }
   }
 
-  final class ArgNodeExImpl extends NodeExImpl {
+  final class ArgNodeEx extends NodeEx {
-    ArgNodeExImpl() { this.asNode() instanceof ArgNode }
+    ArgNodeEx() { this.asNode() instanceof ArgNode }
 
     DataFlowCall getCall() { this.asNode().(ArgNode).argumentOf(result, _) }
   }
 
-  final class ParamNodeExImpl extends NodeExImpl {
+  final class ParamNodeEx extends NodeEx {
-    ParamNodeExImpl() { this.asNode() instanceof ParamNode }
+    ParamNodeEx() { this.asNode() instanceof ParamNode }
 
     predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
       this.asNode().(ParamNode).isParameterOf(c, pos)
@@ -919,10 +919,10 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> Lang> {
    * A node from which flow can return to the caller. This is either a regular
    * `ReturnNode` or a synthesized node for flow out via a parameter.
    */
-  final class RetNodeExImpl extends NodeExImpl {
+  final class RetNodeEx extends NodeEx {
     private ReturnPosition pos;
 
-    RetNodeExImpl() { pos = getReturnPositionEx(this) }
+    RetNodeEx() { pos = getReturnPositionEx(this) }
 
     ReturnPosition getReturnPosition() { result = pos }
 
@@ -1713,7 +1713,7 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> Lang> {
      * Holds if data can flow in one local step from `node1` to `node2`.
      */
     cached
-    predicate localFlowStepExImpl(NodeExImpl node1, NodeExImpl node2, string model) {
+    predicate localFlowStepExImpl(NodeEx node1, NodeEx node2, string model) {
       exists(Node n1, Node n2 |
         node1.asNode() = n1 and
         node2.asNode() = n2 and
@@ -1730,7 +1730,7 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> Lang> {
     }
 
     cached
-    ReturnPosition getReturnPositionEx(NodeExImpl ret) {
+    ReturnPosition getReturnPositionEx(NodeEx ret) {
       result = getValueReturnPosition(ret.asNode())
       or
       exists(ParamNode p |