fix: refactor local, read and store steps

Author: Alvaro Muñoz

ql/lib/codeql/actions/dataflow/FlowSteps.qll

@@ -40,7 +40,7 @@ class AdditionalTaintStep extends Unit {
  *      echo "foo=$(echo $TAINTED)" >> $GITHUB_OUTPUT
  *      echo "test=${{steps.step1.outputs.MSG}}" >> "$GITHUB_OUTPUT"
  */
-predicate runEnvToScriptstep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) {
+predicate runEnvToScriptStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) {
   exists(RunExpr r, string varName, string output |
     c = any(DataFlow::FieldContent ct | ct.getName() = output.replaceAll("output\\.", "")) and
     r.getEnvExpr(varName) = pred.asExpr() and

ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll

@@ -133,9 +133,9 @@ predicate typeStrongerThan(DataFlowType t1, DataFlowType t2) { none() }
 
 newtype TContent =
   TFieldContent(string name) {
+    // We only use field flow for steps and jobs outputs, not for accessing other context fields such as jobs, env or inputs
     name = any(StepsCtxAccessExpr a).getFieldName() or
-    name = any(NeedsCtxAccessExpr a).getFieldName() or
-    name = any(JobsCtxAccessExpr a).getFieldName()
+    name = any(NeedsCtxAccessExpr a).getFieldName()
   }
 
 /**
@@ -188,11 +188,12 @@ class ArgumentPosition extends string {
 predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos }
 
 /**
- * Holds if there is a local flow step between a ${{}} expression accesing a step output variable and the step output itself
- * But only for those cases where the step output is defined externally in a MaD specification.
- * The reason for this is that we don't currently have a way to specify that a source starts with a non-empty access
- * path so  the easiest thing is to add the corresponding read steps of that field as local flow steps as well.
- * e.g. ${{ steps.step1.output.foo }}
+ * Holds if there is a local flow step between a ${{ steps.xxx.outputs.yyy }} expression accesing a step output field
+ * and the step output itself. But only for those cases where the step output is defined externally in a MaD Source
+ * specification. The reason for this is that we don't currently have a way to specify that a source starts with a
+ * non-empty access path so we cannot write a Source that stores the taint in a Content, we can only do that for steps
+ * (storeStep). The easiest thing is to add this local flow step that simulates a read step from the source node for a specific
+ * field name.
  */
 predicate stepsCtxLocalStep(Node nodeFrom, Node nodeTo) {
   exists(StepStmt astFrom, StepsCtxAccessExpr astTo |
@@ -204,19 +205,6 @@ predicate stepsCtxLocalStep(Node nodeFrom, Node nodeTo) {
   )
 }
 
-/**
- * Holds if there is a local flow step between a ${{}} expression accesing a job output variable and the job output itself
- * e.g. ${{ needs.job1.output.foo }} or ${{ jobs.job1.output.foo }}
- */
-predicate jobsCtxLocalStep(Node nodeFrom, Node nodeTo) {
-  exists(Expression astFrom, CtxAccessExpr astTo |
-    astFrom = nodeFrom.asExpr() and
-    astTo = nodeTo.asExpr() and
-    astTo.getRefExpr() = astFrom and
-    (astTo instanceof NeedsCtxAccessExpr or astTo instanceof JobsCtxAccessExpr)
-  )
-}
-
 /**
  * Holds if there is a local flow step between a ${{}} expression accesing an input variable and the input itself
  * e.g. ${{ inputs.foo }}
@@ -252,7 +240,6 @@ predicate envCtxLocalStep(Node nodeFrom, Node nodeTo) {
 pragma[nomagic]
 predicate localFlowStep(Node nodeFrom, Node nodeTo) {
   stepsCtxLocalStep(nodeFrom, nodeTo) or
-  jobsCtxLocalStep(nodeFrom, nodeTo) or
   inputsCtxLocalStep(nodeFrom, nodeTo) or
   envCtxLocalStep(nodeFrom, nodeTo)
 }
@@ -272,17 +259,12 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) { localFlowStep(nodeFr
  */
 predicate jumpStep(Node nodeFrom, Node nodeTo) { none() }
 
-/**
- * A read step to read the value of a ReusableWork uses step  and connect it to its
- * corresponding JobOutputAccessExpr
- */
-predicate reusableWorkflowReturnReadStep(Node node1, Node node2, ContentSet c) {
-  exists(NeedsCtxAccessExpr expr, string fieldName |
-    expr.usesReusableWorkflow() and
-    expr.getRefExpr() = node1.asExpr() and
-    expr.getFieldName() = fieldName and
-    expr = node2.asExpr() and
-    c = any(FieldContent ct | ct.getName() = fieldName)
+predicate ctxFieldReadStep(Node node1, Node node2, ContentSet c) {
+  exists(CtxAccessExpr access |
+    (access instanceof NeedsCtxAccessExpr or access instanceof StepsCtxAccessExpr) and
+    c = any(FieldContent ct | ct.getName() = access.getFieldName()) and
+    node1.asExpr() = access.getRefExpr() and
+    node2.asExpr() = access
   )
 }
 
@@ -291,24 +273,14 @@ predicate reusableWorkflowReturnReadStep(Node node1, Node node2, ContentSet c) {
  * `node1` references an object with a content `c.getAReadContent()` whose
  * value ends up in `node2`.
  */
-predicate readStep(Node node1, ContentSet c, Node node2) {
-  // TODO: Extract to its own predicate
-  exists(StepsCtxAccessExpr access |
-    c = any(FieldContent ct | ct.getName() = access.getFieldName()) and
-    node1.asExpr() = access.getRefExpr() and
-    node2.asExpr() = access
-  )
-  or
-  reusableWorkflowReturnReadStep(node1, node2, c)
-}
+predicate readStep(Node node1, ContentSet c, Node node2) { ctxFieldReadStep(node1, node2, c) }
 
 /**
- * A store step to store the value of a ReusableWorkflowStmt output expr into the return node (node2)
+ * A store step to store an output expression (node1) into its OutputsStm node (node2)
  * with a given access path (fieldName)
  */
-predicate reusableWorkflowReturnStoreStep(Node node1, Node node2, ContentSet c) {
-  exists(ReusableWorkflowStmt stmt, OutputsStmt out, string fieldName |
-    out = stmt.getOutputsStmt() and
+predicate fieldStoreStep(Node node1, Node node2, ContentSet c) {
+  exists(OutputsStmt out, string fieldName |
     node1.asExpr() = out.getOutputExpr(fieldName) and
     node2.asExpr() = out and
     c = any(FieldContent ct | ct.getName() = fieldName)
@@ -321,13 +293,9 @@ predicate reusableWorkflowReturnStoreStep(Node node1, Node node2, ContentSet c)
  * contains the value of `node1`.
  */
 predicate storeStep(Node node1, ContentSet c, Node node2) {
-  reusableWorkflowReturnStoreStep(node1, node2, c)
-  or
-  // TODO: rename to xxxxStoreStep
-  externallyDefinedSummary(node1, node2, c)
-  or
-  // TODO: rename to xxxxStoreStep
-  runEnvToScriptstep(node1, node2, c)
+  fieldStoreStep(node1, node2, c) or
+  externallyDefinedStoreStep(node1, node2, c) or
+  runEnvToScriptStoreStep(node1, node2, c)
 }
 
 /**