From 1bf2431c992f2c45ba9d23b7c970fb271083ab34 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Wed, 13 Mar 2024 15:41:57 +0100
Subject: [PATCH 01/17] Improve UntrustedCheckout query

Account for more events, more triggers and heuristics to detect git checkouts
---
 ql/src/Security/CWE-829/UntrustedCheckout.ql  | 87 ++++++++++++-------
 .../CWE-829/.github/workflows/gitcheckout.yml | 23 +++++
 2 files changed, 81 insertions(+), 29 deletions(-)
 create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/gitcheckout.yml

diff --git a/ql/src/Security/CWE-829/UntrustedCheckout.ql b/ql/src/Security/CWE-829/UntrustedCheckout.ql
index 865169b3cd9..0b3a2873d51 100644
--- a/ql/src/Security/CWE-829/UntrustedCheckout.ql
+++ b/ql/src/Security/CWE-829/UntrustedCheckout.ql
@@ -15,39 +15,68 @@
 
 import actions
 
-/**
- * An If node that contains an `actor` check
- */
-class ActorCheck extends If {
-  ActorCheck() {
+/** An If node that contains an actor, user or label check */
+class ControlCheck extends If {
+  ControlCheck() {
     this.getCondition().regexpMatch(".*github\\.(triggering_)?actor.*") or
-    this.getCondition().regexpMatch(".*github\\.event\\.pull_request\\.user\\.login.*")
-  }
-}
-
-/**
- * An If node that contains a `label` check
- */
-class LabelCheck extends If {
-  LabelCheck() {
+    this.getCondition().regexpMatch(".*github\\.event\\.pull_request\\.user\\.login.*") or
     this.getCondition().regexpMatch(".*github\\.event\\.pull_request\\.labels.*") or
     this.getCondition().regexpMatch(".*github\\.event\\.label\\.name.*")
   }
 }
 
-from Workflow w, LocalJob job, UsesStep checkoutStep
+bindingset[s]
+predicate containsHeadRef(string s) {
+  s.matches("%" +
+      [
+        "github.event.number", // The pull request number.
+        "github.event.pull_request.head.ref", // The ref name of head.
+        "github.event.pull_request.head.sha", //  The commit SHA of head.
+        "github.event.pull_request.id", // The pull request ID.
+        "github.event.pull_request.number", // The pull request number.
+        "github.event.pull_request.merge_commit_sha", // The SHA of the merge commit.
+        "github.head_ref", // The head_ref or source branch of the pull request in a workflow run.
+        "github.event.workflow_run.head_branch", // The branch of the head commit.
+        "github.event.workflow_run.head_commit.id", // The SHA of the head commit.
+        "github.event.workflow_run.head_sha", // The SHA of the head commit.
+        "env.GITHUB_HEAD_REF",
+      ] + "%")
+}
+
+/** Checkout of a Pull Request HEAD ref */
+abstract class PRHeadCheckoutStep extends Step { }
+
+/** Checkout of a Pull Request HEAD ref using actions/checkout action */
+class ActionsCheckout extends PRHeadCheckoutStep instanceof UsesStep {
+  ActionsCheckout() {
+    this.getCallee() = "actions/checkout" and
+    containsHeadRef(this.getArgumentExpr("ref").getExpression())
+  }
+}
+
+/** Checkout of a Pull Request HEAD ref using git within a Run step */
+class GitCheckout extends PRHeadCheckoutStep instanceof Run {
+  GitCheckout() {
+    exists(string line |
+      this.getScript().splitAt("\n") = line and
+      line.regexpMatch(".*git\\s+fetch.*") and
+      (
+        containsHeadRef(line)
+        or
+        exists(string varname |
+          containsHeadRef(this.getInScopeEnvVarExpr(varname).getExpression()) and
+          line.matches("%" + varname + "%")
+        )
+      )
+    )
+  }
+}
+
+from Workflow w, PRHeadCheckoutStep checkout
 where
-  w.hasTriggerEvent("pull_request_target") and
-  w.getAJob() = job and
-  job.getAStep() = checkoutStep and
-  checkoutStep.getCallee() = "actions/checkout" and
-  checkoutStep
-      .getArgumentExpr("ref")
-      .getExpression()
-      .matches([
-          "%github.event.pull_request.head.ref%", "%github.event.pull_request.head.sha%",
-          "%github.event.pull_request.number%", "%github.event.number%", "%github.head_ref%"
-        ]) and
-  not exists(ActorCheck check | job.getIf() = check or checkoutStep.getIf() = check) and
-  not exists(LabelCheck check | job.getIf() = check or checkoutStep.getIf() = check)
-select checkoutStep, "Potential unsafe checkout of untrusted pull request on 'pull_request_target'."
+  w.hasTriggerEvent(["pull_request_target", "issue_comment", "workflow_run"]) and
+  w.getAJob().(LocalJob).getAStep() = checkout and
+  not exists(ControlCheck check |
+    checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check
+  )
+select checkout, "Potential unsafe checkout of untrusted pull request on privileged workflow."
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/gitcheckout.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/gitcheckout.yml
new file mode 100644
index 00000000000..ab121239c6e
--- /dev/null
+++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/gitcheckout.yml
@@ -0,0 +1,23 @@
+on:
+  pull_request_target
+
+jobs:
+  build:
+    name: Build and test
+    runs-on: ubuntu-latest
+    steps:
+        # 1. Check out the content from an incoming pull request
+        - run: |
+            git fetch origin $HEAD_BRANCH
+            git checkout origin/master
+            git config user.name "release-hash-check"
+            git config user.email "<>"
+            git merge --no-commit --no-edit origin/$HEAD_BRANCH
+          env:
+            HEAD_BRANCH: ${{ github.head_ref }}
+        - uses: actions/setup-node@v1
+        # 2. Potentially untrusted commands are being run during "npm install" or "npm build" as
+        #    the build scripts and referenced packages are controlled by the author of the pull request
+        - run: |
+            npm install
+            npm build

From 839d16cde563efe235bca401d6959e97a1bdaf47 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Wed, 13 Mar 2024 18:41:17 +0100
Subject: [PATCH 02/17] Treat If's values as expression no matter the
 delimiters

---
 ql/lib/codeql/actions/Ast.qll                 |  12 ++
 ql/lib/codeql/actions/ast/internal/Ast.qll    |  20 ++-
 ql/src/Security/CWE-829/UntrustedCheckout.ql  |  39 ++---
 ql/test/library-tests/test.expected           |   7 +-
 ql/test/library-tests/test.ql                 |  10 ++
 .../CriticalExpressionInjection.expected      |   4 +
 .../CWE-094/ExpressionInjection.expected      |   4 +
 .../CWE-829/.github/workflows/auto_ci.yml     | 135 ++++++++++++++++++
 .../CWE-829/UnpinnedActionsTag.expected       |   3 +
 .../CWE-829/UntrustedCheckout.expected        |   5 +-
 10 files changed, 215 insertions(+), 24 deletions(-)
 create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/auto_ci.yml

diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll
index 3123518d369..271182a05dd 100644
--- a/ql/lib/codeql/actions/Ast.qll
+++ b/ql/lib/codeql/actions/Ast.qll
@@ -1,6 +1,16 @@
 private import codeql.actions.ast.internal.Ast
 private import codeql.Locations
 
+module Utils {
+  bindingset[expr]
+  string normalizeExpr(string expr) {
+    result =
+      expr.regexpReplaceAll("[\\.\\'\\[\\]\"]+", ".")
+          .regexpReplaceAll("\\.$", "")
+          .regexpReplaceAll("\\.\\s", " ")
+  }
+}
+
 class AstNode instanceof AstNodeImpl {
   AstNode getAChildNode() { result = super.getAChildNode() }
 
@@ -188,6 +198,8 @@ class Step extends AstNode instanceof StepImpl {
  */
 class If extends AstNode instanceof IfImpl {
   string getCondition() { result = super.getCondition() }
+
+  Expression getConditionExpr() { result = super.getConditionExpr() }
 }
 
 abstract class Uses extends AstNode instanceof UsesImpl {
diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
index 028f2280680..14f3cd2ecd9 100644
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -45,6 +45,14 @@ private newtype TAstNode =
         )
       )
     )
+    or
+    // if's conditions do not need to be delimted with ${{}}
+    exists(YamlMapping m |
+      m.maps(key, value) and
+      key.(YamlScalar).getValue() = ["if"] and
+      value.getValue() = raw and
+      exprOffset = 1
+    )
   } or
   TCompositeAction(YamlMapping n) {
     n instanceof YamlDocument and
@@ -123,7 +131,7 @@ class ScalarValueImpl extends AstNodeImpl, TScalarValueNode {
 
   override Location getLocation() { result = value.getLocation() }
 
-  override YamlNode getNode() { result = value }
+  override YamlScalar getNode() { result = value }
 }
 
 class ExpressionImpl extends AstNodeImpl, TExpressionNode {
@@ -135,15 +143,16 @@ class ExpressionImpl extends AstNodeImpl, TExpressionNode {
 
   ExpressionImpl() {
     this = TExpressionNode(key, value, rawExpression, exprOffset - 1) and
-    expression =
-      rawExpression.regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\[\\]\\*\\((\\)\\.\\-]+)\\s*\\}\\}", 1)
+    if rawExpression.trim().regexpMatch("\\$\\{\\{.*\\}\\}")
+    then expression = rawExpression.trim().regexpCapture("\\$\\{\\{\\s*(.*)\\s*\\}\\}", 1).trim()
+    else expression = rawExpression.trim()
   }
 
   override string toString() { result = expression }
 
   override AstNodeImpl getAChildNode() { none() }
 
-  override AstNodeImpl getParentNode() { result.getNode() = value }
+  override ScalarValueImpl getParentNode() { result.getNode() = value }
 
   override string getAPrimaryQlClass() { result = "ExpressionNode" }
 
@@ -638,6 +647,9 @@ class IfImpl extends AstNodeImpl, TIfNode {
 
   /** Gets the condition that must be satisfied for this job to run. */
   string getCondition() { result = n.(YamlScalar).getValue() }
+
+  /** Gets the condition that must be satisfied for this job to run. */
+  ExpressionImpl getConditionExpr() { result.getParentNode().getNode() = n }
 }
 
 class EnvImpl extends AstNodeImpl, TEnvNode {
diff --git a/ql/src/Security/CWE-829/UntrustedCheckout.ql b/ql/src/Security/CWE-829/UntrustedCheckout.ql
index 0b3a2873d51..438e3dfe7fc 100644
--- a/ql/src/Security/CWE-829/UntrustedCheckout.ql
+++ b/ql/src/Security/CWE-829/UntrustedCheckout.ql
@@ -18,29 +18,32 @@ import actions
 /** An If node that contains an actor, user or label check */
 class ControlCheck extends If {
   ControlCheck() {
-    this.getCondition().regexpMatch(".*github\\.(triggering_)?actor.*") or
-    this.getCondition().regexpMatch(".*github\\.event\\.pull_request\\.user\\.login.*") or
-    this.getCondition().regexpMatch(".*github\\.event\\.pull_request\\.labels.*") or
-    this.getCondition().regexpMatch(".*github\\.event\\.label\\.name.*")
+    Utils::normalizeExpr(this.getCondition())
+        .regexpMatch([
+            ".*github\\.actor.*", ".*github\\.triggering_actor.*",
+            ".*github\\.event\\.pull_request\\.user\\.login.*",
+            ".*github\\.event\\.pull_request\\.labels.*", ".*github\\.event\\.label\\.name.*"
+          ])
   }
 }
 
 bindingset[s]
 predicate containsHeadRef(string s) {
-  s.matches("%" +
-      [
-        "github.event.number", // The pull request number.
-        "github.event.pull_request.head.ref", // The ref name of head.
-        "github.event.pull_request.head.sha", //  The commit SHA of head.
-        "github.event.pull_request.id", // The pull request ID.
-        "github.event.pull_request.number", // The pull request number.
-        "github.event.pull_request.merge_commit_sha", // The SHA of the merge commit.
-        "github.head_ref", // The head_ref or source branch of the pull request in a workflow run.
-        "github.event.workflow_run.head_branch", // The branch of the head commit.
-        "github.event.workflow_run.head_commit.id", // The SHA of the head commit.
-        "github.event.workflow_run.head_sha", // The SHA of the head commit.
-        "env.GITHUB_HEAD_REF",
-      ] + "%")
+  Utils::normalizeExpr(s)
+      .matches("%" +
+          [
+            "github.event.number", // The pull request number.
+            "github.event.pull_request.head.ref", // The ref name of head.
+            "github.event.pull_request.head.sha", //  The commit SHA of head.
+            "github.event.pull_request.id", // The pull request ID.
+            "github.event.pull_request.number", // The pull request number.
+            "github.event.pull_request.merge_commit_sha", // The SHA of the merge commit.
+            "github.head_ref", // The head_ref or source branch of the pull request in a workflow run.
+            "github.event.workflow_run.head_branch", // The branch of the head commit.
+            "github.event.workflow_run.head_commit.id", // The SHA of the head commit.
+            "github.event.workflow_run.head_sha", // The SHA of the head commit.
+            "env.GITHUB_HEAD_REF",
+          ] + "%")
 }
 
 /** Checkout of a Pull Request HEAD ref */
diff --git a/ql/test/library-tests/test.expected b/ql/test/library-tests/test.expected
index 4ef2a2e5875..df8c6ddf9cd 100644
--- a/ql/test/library-tests/test.expected
+++ b/ql/test/library-tests/test.expected
@@ -190,7 +190,7 @@ parentNodes
 | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} |
 | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} |
 | .github/workflows/test.yml:34:10:34:24 | always() | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} |
-| .github/workflows/test.yml:34:10:34:24 | always() | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} |
+| .github/workflows/test.yml:34:11:34:25 | always() | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} |
 | .github/workflows/test.yml:36:12:36:15 | job1 | .github/workflows/test.yml:1:1:40:53 | on: push |
 | .github/workflows/test.yml:36:12:36:15 | job1 | .github/workflows/test.yml:32:5:40:53 | Job: job2 |
 | .github/workflows/test.yml:36:12:36:15 | job1 | .github/workflows/test.yml:32:5:40:53 | Job: job2 |
@@ -415,3 +415,8 @@ calls
 | .github/workflows/test.yml:19:9:26:6 | Uses Step: step | mad9000/actions-find-and-replace-string |
 needs
 | .github/workflows/test.yml:40:20:40:53 | needs.job1.outputs.job_output |
+testNormalizeExpr
+| foo['bar'] == baz | foo.bar == baz |
+| github.event.pull_request.user["login"] | github.event.pull_request.user.login |
+| github.event.pull_request.user['login'] | github.event.pull_request.user.login |
+| github.event.pull_request['user']['login'] | github.event.pull_request.user.login |
diff --git a/ql/test/library-tests/test.ql b/ql/test/library-tests/test.ql
index 8cf97d58ab0..268396a711e 100644
--- a/ql/test/library-tests/test.ql
+++ b/ql/test/library-tests/test.ql
@@ -1,4 +1,5 @@
 import codeql.actions.Ast
+import codeql.actions.Ast::Utils as Utils
 import codeql.actions.Cfg as Cfg
 import codeql.actions.DataFlow
 import codeql.Locations
@@ -59,3 +60,12 @@ query predicate summaries(string action, string version, string input, string ou
 query predicate calls(DataFlow::CallNode call, string callee) { callee = call.getCallee() }
 
 query predicate needs(DataFlow::Node e) { e.asExpr() instanceof NeedsExpression }
+
+query string testNormalizeExpr(string s) {
+  s =
+    [
+      "github.event.pull_request.user['login']", "github.event.pull_request.user[\"login\"]",
+      "github.event.pull_request['user']['login']", "foo['bar'] == baz"
+    ] and
+  result = Utils::normalizeExpr(s)
+}
diff --git a/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected b/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected
index c9ac215666f..38884b3eaef 100644
--- a/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected
+++ b/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected
@@ -3,6 +3,7 @@ edges
 | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE |
 | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] |
 | .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files |
+| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log |
 | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:39:31:39:75 | steps.remove_quotations.outputs.replaced |
 | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:57:29:57:73 | steps.remove_quotations.outputs.replaced |
 | .github/workflows/cross3.yml:32:18:32:53 | github.event.commits[0].message | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] |
@@ -58,6 +59,8 @@ nodes
 | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | semmle.label | steps.remove_quotations.outputs.replaced |
 | .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | semmle.label | Uses Step: changed-files |
 | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files | semmle.label | steps.changed-files.outputs.all_changed_files |
+| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
+| .github/workflows/changelog.yml:58:26:58:39 | env.log | semmle.label | env.log |
 | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | semmle.label | github.event.comment.body |
 | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | semmle.label | github.event.comment.body |
 | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | semmle.label | github.event.issue.body |
@@ -187,6 +190,7 @@ nodes
 subpaths
 #select
 | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | steps.remove_quotations.outputs.replaced |
+| .github/workflows/changelog.yml:58:26:58:39 | env.log | .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/changelog.yml:58:26:58:39 | env.log | env.log |
 | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | github.event.comment.body |
 | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | github.event.comment.body |
 | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | github.event.issue.body |
diff --git a/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected b/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected
index cb924c97ea1..21a9978c54f 100644
--- a/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected
+++ b/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected
@@ -3,6 +3,7 @@ edges
 | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE |
 | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] |
 | .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files |
+| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log |
 | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:39:31:39:75 | steps.remove_quotations.outputs.replaced |
 | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:57:29:57:73 | steps.remove_quotations.outputs.replaced |
 | .github/workflows/cross3.yml:32:18:32:53 | github.event.commits[0].message | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] |
@@ -58,6 +59,8 @@ nodes
 | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | semmle.label | steps.remove_quotations.outputs.replaced |
 | .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | semmle.label | Uses Step: changed-files |
 | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files | semmle.label | steps.changed-files.outputs.all_changed_files |
+| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
+| .github/workflows/changelog.yml:58:26:58:39 | env.log | semmle.label | env.log |
 | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | semmle.label | github.event.comment.body |
 | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | semmle.label | github.event.comment.body |
 | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | semmle.label | github.event.issue.body |
@@ -188,6 +191,7 @@ subpaths
 #select
 | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | ${{steps.remove_quotations.outputs.replaced}} |
 | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files | .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files | ${{ steps.changed-files.outputs.all_changed_files }} |
+| .github/workflows/changelog.yml:58:26:58:39 | env.log | .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/changelog.yml:58:26:58:39 | env.log | ${{ env.log }} |
 | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | ${{ github.event.comment.body }} |
 | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | ${{ github.event.comment.body }} |
 | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | ${{ github.event.issue.body }} |
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/auto_ci.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/auto_ci.yml
new file mode 100644
index 00000000000..cb20cfe629b
--- /dev/null
+++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/auto_ci.yml
@@ -0,0 +1,135 @@
+name: Python CI
+
+on:
+  push:
+    branches: [ master ]
+  pull_request_target:
+    branches: [ master, stable ]
+
+concurrency:
+  group: ${{ format('ci-{0}', github.head_ref && format('pr-{0}', github.event.pull_request.number) || github.sha) }}
+  cancel-in-progress: ${{ github.event_name == 'pull_request_target' }}
+
+jobs:
+  lint:
+    runs-on: ubuntu-latest
+    env:
+      min-python-version: "3.10"
+
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v3
+        with:
+          fetch-depth: 0
+          ref: ${{ github.event.pull_request.head.ref }}
+          repository: ${{ github.event.pull_request.head.repo.full_name }}
+
+      - name: Set up Python ${{ env.min-python-version }}
+        uses: actions/setup-python@v2
+        with:
+          python-version: ${{ env.min-python-version }}
+
+      - name: Install dependencies
+        run: |
+          python -m pip install --upgrade pip
+          pip install -r requirements.txt
+
+      - name: Lint with flake8
+        run: flake8
+
+      - name: Check black formatting
+        run: black . --check
+        if: success() || failure()
+
+      - name: Check isort formatting
+        run: isort . --check
+        if: success() || failure()
+
+      - name: Check mypy formatting
+        run: mypy
+        if: success() || failure()
+
+  test:
+    permissions:
+      # Gives the action the necessary permissions for publishing new
+      # comments in pull requests.
+      pull-requests: write
+      # Gives the action the necessary permissions for pushing data to the
+      # python-coverage-comment-action branch, and for editing existing
+      # comments (to avoid publishing multiple comments in the same PR)
+      contents: write
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        python-version: ["3.10"]
+
+    steps:
+      - name: Check out repository
+        uses: actions/checkout@v3
+        with:
+          fetch-depth: 0
+          ref: ${{ github.event.pull_request.head.ref }}
+          repository: ${{ github.event.pull_request.head.repo.full_name }}
+
+      - name: Set up Python ${{ matrix.python-version }}
+        uses: actions/setup-python@v2
+        with:
+          python-version: ${{ matrix.python-version }}
+
+      - name: Install dependencies
+        run: |
+          python -m pip install --upgrade pip
+          pip install -r requirements.txt
+
+      - name: Run unittest tests with coverage
+        run: |
+          pytest -n auto --cov=autogpt --cov-report term-missing --cov-branch --cov-report xml --cov-report term
+        env:
+          CI: true
+          PROXY: ${{ secrets.PROXY }}
+          AGENT_MODE: ${{ vars.AGENT_MODE }}
+          AGENT_TYPE: ${{ vars.AGENT_TYPE }}
+
+      - name: Upload coverage reports to Codecov
+        uses: codecov/codecov-action@v3
+
+      - name: Stage new files and commit
+        id: stage_files
+        run: |
+          git add tests
+          git diff --cached --quiet && echo "No changes to commit" && exit 0
+          git config user.email "github-actions@github.com"
+          git config user.name "GitHub Actions"
+          git commit -m "Add new cassettes"
+          TIMESTAMP_COMMIT=$(date +%Y%m%d%H%M%S) # generate a timestamp
+          echo "TIMESTAMP_COMMIT=TIMESTAMP_COMMIT" >> $GITHUB_ENV
+      
+
+      - name: Create PR
+        id: create_pr
+        if:  ${{ env.TIMESTAMP_COMMIT != null }}
+        uses: peter-evans/create-pull-request@v5
+        with:
+          commit-message: Update cassettes
+          branch: cassette-diff-PR-${{ github.event.pull_request.number }}-${{ env.TIMESTAMP_COMMIT }}
+          title: "Update cassette-diff-PR${{ github.event.pull_request.number }}-${{ env.TIMESTAMP_COMMIT }}"
+          body: "This PR updates the cassettes. Please merge it."
+
+
+      - name: Check PR
+        if:  ${{ env.TIMESTAMP_COMMIT != null }}
+        run: |
+          echo "Pull Request Number - ${{ steps.create_pr.outputs.pull-request-number }}"
+          echo "Pull Request URL - ${{ steps.create_pr.outputs.pull-request-url }}"
+
+      - name: Comment PR URL in the current PR
+        if:  ${{ env.TIMESTAMP_COMMIT != null }}
+        uses: thollander/actions-comment-pull-request@v2
+        with:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+          message: |
+            Please click [HERE](${{ steps.create_pr.outputs.pull-request-url }}) and merge this PR to update the cassettes.
+
+      - name: Fail if new PR created
+        if:  ${{ env.TIMESTAMP_COMMIT != null }}
+        run: exit 1
diff --git a/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected b/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected
index 6620d2ac385..67fcc5555d1 100644
--- a/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected
+++ b/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected
@@ -1,5 +1,8 @@
 | .github/workflows/actor_trusted_checkout.yml:19:7:23:4 | Uses Step | Unpinned 3rd party Action 'actor_trusted_checkout.yml' step $@ uses 'completely/fakeaction' with ref 'v2', not a pinned commit hash | .github/workflows/actor_trusted_checkout.yml:19:7:23:4 | Uses Step | Uses Step |
 | .github/workflows/actor_trusted_checkout.yml:23:7:26:21 | Uses Step | Unpinned 3rd party Action 'actor_trusted_checkout.yml' step $@ uses 'fakerepo/comment-on-pr' with ref 'v1', not a pinned commit hash | .github/workflows/actor_trusted_checkout.yml:23:7:26:21 | Uses Step | Uses Step |
+| .github/workflows/auto_ci.yml:93:9:96:6 | Uses Step | Unpinned 3rd party Action 'Python CI' step $@ uses 'codecov/codecov-action' with ref 'v3', not a pinned commit hash | .github/workflows/auto_ci.yml:93:9:96:6 | Uses Step | Uses Step |
+| .github/workflows/auto_ci.yml:108:9:119:6 | Uses Step: create_pr | Unpinned 3rd party Action 'Python CI' step $@ uses 'peter-evans/create-pull-request' with ref 'v5', not a pinned commit hash | .github/workflows/auto_ci.yml:108:9:119:6 | Uses Step: create_pr | Uses Step: create_pr |
+| .github/workflows/auto_ci.yml:125:9:133:6 | Uses Step | Unpinned 3rd party Action 'Python CI' step $@ uses 'thollander/actions-comment-pull-request' with ref 'v2', not a pinned commit hash | .github/workflows/auto_ci.yml:125:9:133:6 | Uses Step | Uses Step |
 | .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'completely/fakeaction' with ref 'v2', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Uses Step |
 | .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'fakerepo/comment-on-pr' with ref 'v1', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Uses Step |
 | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Unpinned 3rd party Action 'unpinned_tags.yml' step $@ uses 'foo/bar' with ref 'v1', not a pinned commit hash | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Uses Step |
diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
index 7527a1e15f2..be1c7cbfebd 100644
--- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
+++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
@@ -1 +1,4 @@
-| .github/workflows/untrusted_checkout.yml:9:7:13:4 | Uses Step | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
+| .github/workflows/auto_ci.yml:20:9:27:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/untrusted_checkout.yml:9:7:13:4 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |

From 87b284e5e6b49b431aef3c23099ac80b0fe8753e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Wed, 13 Mar 2024 19:14:57 +0100
Subject: [PATCH 03/17] update

---
 ql/lib/codeql/actions/Ast.qll                 |  8 +-
 .../codeql/actions/dataflow/FlowSources.qll   | 89 +++++++++++--------
 2 files changed, 55 insertions(+), 42 deletions(-)

diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll
index 271182a05dd..3d675bebce0 100644
--- a/ql/lib/codeql/actions/Ast.qll
+++ b/ql/lib/codeql/actions/Ast.qll
@@ -5,9 +5,11 @@ module Utils {
   bindingset[expr]
   string normalizeExpr(string expr) {
     result =
-      expr.regexpReplaceAll("[\\.\\'\\[\\]\"]+", ".")
-          .regexpReplaceAll("\\.$", "")
-          .regexpReplaceAll("\\.\\s", " ")
+      expr.replaceAll("['", ".")
+          .replaceAll("']", "")
+          .replaceAll("[\"", ".")
+          .replaceAll("\"]", "")
+          .regexpReplaceAll("\\s*\\.\\s*", ".")
   }
 }
 
diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll
index 23ae225e07e..d3a96e1a2c7 100644
--- a/ql/lib/codeql/actions/dataflow/FlowSources.qll
+++ b/ql/lib/codeql/actions/dataflow/FlowSources.qll
@@ -1,6 +1,7 @@
 import actions
 import codeql.actions.DataFlow
 import codeql.actions.dataflow.ExternalFlow
+import codeql.actions.Ast::Utils as Utils
 
 /**
  * A data flow source.
@@ -24,8 +25,11 @@ abstract class RemoteFlowSource extends SourceNode {
 
 bindingset[context]
 private predicate isExternalUserControlledIssue(string context) {
-  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*issue\\s*\\.\\s*title\\b") or
-  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*issue\\s*\\.\\s*body\\b")
+  exists(string reg |
+    reg = ["\\bgithub\\.event\\.issue\\.title\\b", "\\bgithub\\.event\\.issue\\.body\\b"]
+  |
+    Utils::normalizeExpr(context).regexpMatch(reg)
+  )
 }
 
 bindingset[context]
@@ -33,35 +37,39 @@ private predicate isExternalUserControlledPullRequest(string context) {
   exists(string reg |
     reg =
       [
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*title\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*body\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*label\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*repo\\s*\\.\\s*default_branch\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*repo\\s*\\.\\s*description\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*repo\\s*\\.\\s*homepage\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*ref\\b",
-        "\\bgithub\\s*\\.\\s*head_ref\\b"
+        "\\bgithub\\.event\\.pull_request\\.title\\b", "\\bgithub\\.event\\.pull_request\\.body\\b",
+        "\\bgithub\\.event\\.pull_request\\.head\\.label\\b",
+        "\\bgithub\\.event\\.pull_request\\.head\\.repo\\.default_branch\\b",
+        "\\bgithub\\.event\\.pull_request\\.head\\.repo\\.description\\b",
+        "\\bgithub\\.event\\.pull_request\\.head\\.repo\\.homepage\\b",
+        "\\bgithub\\.event\\.pull_request\\.head\\.ref\\b", "\\bgithub\\.head_ref\\b"
       ]
   |
-    context.regexpMatch(reg)
+    Utils::normalizeExpr(context).regexpMatch(reg)
   )
 }
 
 bindingset[context]
 private predicate isExternalUserControlledReview(string context) {
-  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*review\\s*\\.\\s*body\\b")
+  Utils::normalizeExpr(context).regexpMatch("\\bgithub\\.event\\.review\\.body\\b")
 }
 
 bindingset[context]
 private predicate isExternalUserControlledComment(string context) {
-  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*comment\\s*\\.\\s*body\\b")
+  Utils::normalizeExpr(context).regexpMatch("\\bgithub\\.event\\.comment\\.body\\b")
 }
 
 bindingset[context]
 private predicate isExternalUserControlledGollum(string context) {
-  context
-      .regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages\\[[0-9]+\\]\\s*\\.\\s*page_name\\b") or
-  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages\\[[0-9]+\\]\\s*\\.\\s*title\\b")
+  exists(string reg |
+    reg =
+      [
+        "\\bgithub\\.event\\.pages\\[[0-9]+\\]\\.page_name\\b",
+        "\\bgithub\\.event\\.pages\\[[0-9]+\\]\\.title\\b"
+      ]
+  |
+    Utils::normalizeExpr(context).regexpMatch(reg)
+  )
 }
 
 bindingset[context]
@@ -69,26 +77,29 @@ private predicate isExternalUserControlledCommit(string context) {
   exists(string reg |
     reg =
       [
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*message\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*message\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*author\\s*\\.\\s*email\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*author\\s*\\.\\s*name\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*committer\\s*\\.\\s*email\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*committer\\s*\\.\\s*name\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*author\\s*\\.\\s*email\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*author\\s*\\.\\s*name\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*committer\\s*\\.\\s*email\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*committer\\s*\\.\\s*name\\b",
+        "\\bgithub\\.event\\.commits\\[[0-9]+\\]\\.message\\b",
+        "\\bgithub\\.event\\.head_commit\\.message\\b",
+        "\\bgithub\\.event\\.head_commit\\.author\\.email\\b",
+        "\\bgithub\\.event\\.head_commit\\.author\\.name\\b",
+        "\\bgithub\\.event\\.head_commit\\.committer\\.email\\b",
+        "\\bgithub\\.event\\.head_commit\\.committer\\.name\\b",
+        "\\bgithub\\.event\\.commits\\[[0-9]+\\]\\.author\\.email\\b",
+        "\\bgithub\\.event\\.commits\\[[0-9]+\\]\\.author\\.name\\b",
+        "\\bgithub\\.event\\.commits\\[[0-9]+\\]\\.committer\\.email\\b",
+        "\\bgithub\\.event\\.commits\\[[0-9]+\\]\\.committer\\.name\\b",
       ]
   |
-    context.regexpMatch(reg)
+    Utils::normalizeExpr(context).regexpMatch(reg)
   )
 }
 
 bindingset[context]
 private predicate isExternalUserControlledDiscussion(string context) {
-  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*discussion\\s*\\.\\s*title\\b") or
-  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*discussion\\s*\\.\\s*body\\b")
+  exists(string reg |
+    reg = ["\\bgithub\\.event\\.discussion\\.title\\b", "\\bgithub\\.event\\.discussion\\.body\\b"]
+  |
+    Utils::normalizeExpr(context).regexpMatch(reg)
+  )
 }
 
 bindingset[context]
@@ -96,18 +107,18 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
   exists(string reg |
     reg =
       [
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow\\s*\\.\\s*path\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_branch\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*display_title\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_repository\\b\\s*\\.\\s*description\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*message\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*author\\b\\s*\\.\\s*email\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*author\\b\\s*\\.\\s*name\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*committer\\b\\s*\\.\\s*email\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*committer\\b\\s*\\.\\s*name\\b",
+        "\\bgithub\\.event\\.workflow\\.path\\b",
+        "\\bgithub\\.event\\.workflow_run\\.head_branch\\b",
+        "\\bgithub\\.event\\.workflow_run\\.display_title\\b",
+        "\\bgithub\\.event\\.workflow_run\\.head_repository\\.description\\b",
+        "\\bgithub\\.event\\.workflow_run\\.head_commit\\.message\\b",
+        "\\bgithub\\.event\\.workflow_run\\.head_commit\\.author\\.email\\b",
+        "\\bgithub\\.event\\.workflow_run\\.head_commit\\.author\\.name\\b",
+        "\\bgithub\\.event\\.workflow_run\\.head_commit\\.committer\\.email\\b",
+        "\\bgithub\\.event\\.workflow_run\\.head_commit\\.committer\\.name\\b",
       ]
   |
-    context.regexpMatch(reg)
+    Utils::normalizeExpr(context).regexpMatch(reg)
   )
 }
 

From 0e50204672f05fdd33b115ff36a2c9f5c2c10bef Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Wed, 13 Mar 2024 22:19:55 +0100
Subject: [PATCH 04/17] More regexp improvements

---
 ql/lib/codeql/actions/ast/internal/Ast.qll    | 31 ++++++------
 .../codeql/actions/dataflow/ExternalFlow.qll  |  4 +-
 .../codeql/actions/dataflow/FlowSources.qll   |  8 +--
 ql/lib/codeql/actions/dataflow/FlowSteps.qll  |  4 +-
 .../Security/CWE-020/CompositeActionsSinks.ql |  1 +
 .../CWE-020/CompositeActionsSources.ql        |  1 +
 .../CWE-020/CompositeActionsSummaries.ql      |  1 +
 .../CWE-020/ReusableWorkflowsSinks.ql         |  1 +
 .../CWE-020/ReusableWorkflowsSources.ql       |  1 +
 .../CWE-020/ReusableWorkflowsSummaries.ql     |  1 +
 .../CWE-094/CriticalExpressionInjection.ql    |  1 +
 .../Security/CWE-094/ExpressionInjection.ql   |  1 +
 ql/src/Security/CWE-829/UntrustedCheckout.ql  | 49 ++++++++++---------
 13 files changed, 59 insertions(+), 45 deletions(-)

diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
index 14f3cd2ecd9..7ebed407c0f 100644
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -1,5 +1,6 @@
 private import codeql.actions.ast.internal.Yaml
 private import codeql.Locations
+private import codeql.actions.Ast::Utils as Utils
 
 /**
  * Gets the length of each line in the StringValue .
@@ -833,9 +834,9 @@ class StepsExpressionImpl extends ContextExpressionImpl {
   string fieldName;
 
   StepsExpressionImpl() {
-    expression.regexpMatch(stepsCtxRegex()) and
-    stepId = expression.regexpCapture(stepsCtxRegex(), 1) and
-    fieldName = expression.regexpCapture(stepsCtxRegex(), 2)
+    Utils::normalizeExpr(expression).regexpMatch(stepsCtxRegex()) and
+    stepId = Utils::normalizeExpr(expression).regexpCapture(stepsCtxRegex(), 1) and
+    fieldName = Utils::normalizeExpr(expression).regexpCapture(stepsCtxRegex(), 2)
   }
 
   override string getFieldName() { result = fieldName }
@@ -856,9 +857,9 @@ class NeedsExpressionImpl extends ContextExpressionImpl {
   string fieldName;
 
   NeedsExpressionImpl() {
-    expression.regexpMatch(needsCtxRegex()) and
-    fieldName = expression.regexpCapture(needsCtxRegex(), 2) and
-    neededJob.getId() = expression.regexpCapture(needsCtxRegex(), 1) and
+    Utils::normalizeExpr(expression).regexpMatch(needsCtxRegex()) and
+    fieldName = Utils::normalizeExpr(expression).regexpCapture(needsCtxRegex(), 2) and
+    neededJob.getId() = Utils::normalizeExpr(expression).regexpCapture(needsCtxRegex(), 1) and
     neededJob.getLocation().getFile() = this.getLocation().getFile()
   }
 
@@ -886,9 +887,9 @@ class JobsExpressionImpl extends ContextExpressionImpl {
   string fieldName;
 
   JobsExpressionImpl() {
-    expression.regexpMatch(jobsCtxRegex()) and
-    jobId = expression.regexpCapture(jobsCtxRegex(), 1) and
-    fieldName = expression.regexpCapture(jobsCtxRegex(), 2)
+    Utils::normalizeExpr(expression).regexpMatch(jobsCtxRegex()) and
+    jobId = Utils::normalizeExpr(expression).regexpCapture(jobsCtxRegex(), 1) and
+    fieldName = Utils::normalizeExpr(expression).regexpCapture(jobsCtxRegex(), 2)
   }
 
   override string getFieldName() { result = fieldName }
@@ -911,8 +912,8 @@ class InputsExpressionImpl extends ContextExpressionImpl {
   string fieldName;
 
   InputsExpressionImpl() {
-    expression.regexpMatch(inputsCtxRegex()) and
-    fieldName = expression.regexpCapture(inputsCtxRegex(), 1)
+    Utils::normalizeExpr(expression).regexpMatch(inputsCtxRegex()) and
+    fieldName = Utils::normalizeExpr(expression).regexpCapture(inputsCtxRegex(), 1)
   }
 
   override string getFieldName() { result = fieldName }
@@ -936,8 +937,8 @@ class EnvExpressionImpl extends ContextExpressionImpl {
   string fieldName;
 
   EnvExpressionImpl() {
-    expression.regexpMatch(envCtxRegex()) and
-    fieldName = expression.regexpCapture(envCtxRegex(), 1)
+    Utils::normalizeExpr(expression).regexpMatch(envCtxRegex()) and
+    fieldName = Utils::normalizeExpr(expression).regexpCapture(envCtxRegex(), 1)
   }
 
   override string getFieldName() { result = fieldName }
@@ -959,8 +960,8 @@ class MatrixExpressionImpl extends ContextExpressionImpl {
   string fieldName;
 
   MatrixExpressionImpl() {
-    expression.regexpMatch(matrixCtxRegex()) and
-    fieldName = expression.regexpCapture(matrixCtxRegex(), 1)
+    Utils::normalizeExpr(expression).regexpMatch(matrixCtxRegex()) and
+    fieldName = Utils::normalizeExpr(expression).regexpCapture(matrixCtxRegex(), 1)
   }
 
   override string getFieldName() { result = fieldName }
diff --git a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll
index 008b5a19ce6..7e265fb2570 100644
--- a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll
+++ b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll
@@ -1,6 +1,6 @@
 private import internal.ExternalFlowExtensions as Extensions
-import codeql.actions.DataFlow
-import actions
+private import codeql.actions.DataFlow
+private import actions
 
 /**
  * MaD sources
diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll
index d3a96e1a2c7..a586cab4a32 100644
--- a/ql/lib/codeql/actions/dataflow/FlowSources.qll
+++ b/ql/lib/codeql/actions/dataflow/FlowSources.qll
@@ -1,7 +1,7 @@
-import actions
-import codeql.actions.DataFlow
-import codeql.actions.dataflow.ExternalFlow
-import codeql.actions.Ast::Utils as Utils
+private import actions
+private import codeql.actions.DataFlow
+private import codeql.actions.dataflow.ExternalFlow
+private import codeql.actions.Ast::Utils as Utils
 
 /**
  * A data flow source.
diff --git a/ql/lib/codeql/actions/dataflow/FlowSteps.qll b/ql/lib/codeql/actions/dataflow/FlowSteps.qll
index fddf537ed1d..c10334436aa 100644
--- a/ql/lib/codeql/actions/dataflow/FlowSteps.qll
+++ b/ql/lib/codeql/actions/dataflow/FlowSteps.qll
@@ -2,10 +2,10 @@
  * Provides classes representing various flow steps for taint tracking.
  */
 
-import actions
+private import actions
 private import codeql.util.Unit
 private import codeql.actions.DataFlow
-import codeql.actions.dataflow.ExternalFlow
+private import codeql.actions.dataflow.ExternalFlow
 
 /**
  * A unit class for adding additional taint steps.
diff --git a/ql/src/Security/CWE-020/CompositeActionsSinks.ql b/ql/src/Security/CWE-020/CompositeActionsSinks.ql
index 1f90efa5bcc..0ea0713983d 100644
--- a/ql/src/Security/CWE-020/CompositeActionsSinks.ql
+++ b/ql/src/Security/CWE-020/CompositeActionsSinks.ql
@@ -12,6 +12,7 @@
  */
 
 import actions
+import codeql.actions.DataFlow
 import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.dataflow.ExternalFlow
diff --git a/ql/src/Security/CWE-020/CompositeActionsSources.ql b/ql/src/Security/CWE-020/CompositeActionsSources.ql
index 0edeb0a7ec8..8e4275f27c7 100644
--- a/ql/src/Security/CWE-020/CompositeActionsSources.ql
+++ b/ql/src/Security/CWE-020/CompositeActionsSources.ql
@@ -12,6 +12,7 @@
  */
 
 import actions
+import codeql.actions.DataFlow
 import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.dataflow.ExternalFlow
diff --git a/ql/src/Security/CWE-020/CompositeActionsSummaries.ql b/ql/src/Security/CWE-020/CompositeActionsSummaries.ql
index 59a05f64b6c..8b8b5af3c45 100644
--- a/ql/src/Security/CWE-020/CompositeActionsSummaries.ql
+++ b/ql/src/Security/CWE-020/CompositeActionsSummaries.ql
@@ -12,6 +12,7 @@
  */
 
 import actions
+import codeql.actions.DataFlow
 import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.dataflow.ExternalFlow
diff --git a/ql/src/Security/CWE-020/ReusableWorkflowsSinks.ql b/ql/src/Security/CWE-020/ReusableWorkflowsSinks.ql
index d84566dab04..31fbc1eaae2 100644
--- a/ql/src/Security/CWE-020/ReusableWorkflowsSinks.ql
+++ b/ql/src/Security/CWE-020/ReusableWorkflowsSinks.ql
@@ -12,6 +12,7 @@
  */
 
 import actions
+import codeql.actions.DataFlow
 import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.dataflow.ExternalFlow
diff --git a/ql/src/Security/CWE-020/ReusableWorkflowsSources.ql b/ql/src/Security/CWE-020/ReusableWorkflowsSources.ql
index 6e88f36fece..e5612d06343 100644
--- a/ql/src/Security/CWE-020/ReusableWorkflowsSources.ql
+++ b/ql/src/Security/CWE-020/ReusableWorkflowsSources.ql
@@ -12,6 +12,7 @@
  */
 
 import actions
+import codeql.actions.DataFlow
 import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.dataflow.ExternalFlow
diff --git a/ql/src/Security/CWE-020/ReusableWorkflowsSummaries.ql b/ql/src/Security/CWE-020/ReusableWorkflowsSummaries.ql
index 4f710a16e8f..444ce028954 100644
--- a/ql/src/Security/CWE-020/ReusableWorkflowsSummaries.ql
+++ b/ql/src/Security/CWE-020/ReusableWorkflowsSummaries.ql
@@ -12,6 +12,7 @@
  */
 
 import actions
+import codeql.actions.DataFlow
 import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.dataflow.ExternalFlow
diff --git a/ql/src/Security/CWE-094/CriticalExpressionInjection.ql b/ql/src/Security/CWE-094/CriticalExpressionInjection.ql
index 66a055634c7..e24b1ab9ddc 100644
--- a/ql/src/Security/CWE-094/CriticalExpressionInjection.ql
+++ b/ql/src/Security/CWE-094/CriticalExpressionInjection.ql
@@ -13,6 +13,7 @@
  */
 
 import actions
+import codeql.actions.DataFlow
 import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.dataflow.ExternalFlow
diff --git a/ql/src/Security/CWE-094/ExpressionInjection.ql b/ql/src/Security/CWE-094/ExpressionInjection.ql
index d59cc07cad2..1e7414e5ce6 100644
--- a/ql/src/Security/CWE-094/ExpressionInjection.ql
+++ b/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -13,6 +13,7 @@
  */
 
 import actions
+import codeql.actions.DataFlow
 import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.dataflow.ExternalFlow
diff --git a/ql/src/Security/CWE-829/UntrustedCheckout.ql b/ql/src/Security/CWE-829/UntrustedCheckout.ql
index 438e3dfe7fc..c9ad93d18b2 100644
--- a/ql/src/Security/CWE-829/UntrustedCheckout.ql
+++ b/ql/src/Security/CWE-829/UntrustedCheckout.ql
@@ -18,32 +18,37 @@ import actions
 /** An If node that contains an actor, user or label check */
 class ControlCheck extends If {
   ControlCheck() {
-    Utils::normalizeExpr(this.getCondition())
-        .regexpMatch([
-            ".*github\\.actor.*", ".*github\\.triggering_actor.*",
-            ".*github\\.event\\.pull_request\\.user\\.login.*",
-            ".*github\\.event\\.pull_request\\.labels.*", ".*github\\.event\\.label\\.name.*"
-          ])
+    exists(
+      Utils::normalizeExpr(this.getCondition())
+          .regexpFind([
+              "\\bgithub\\.actor\\b", // actor
+              "\\bgithub\\.triggering_actor\\b", // actor
+              "\\bgithub\\.event\\.pull_request\\.user\\.login\\b", //user
+              "\\bgithub\\.event\\.pull_request\\.labels\\b", // label
+              "\\bgithub\\.event\\.label\\.name\\b" // label
+            ], _, _)
+    )
   }
 }
 
 bindingset[s]
 predicate containsHeadRef(string s) {
-  Utils::normalizeExpr(s)
-      .matches("%" +
-          [
-            "github.event.number", // The pull request number.
-            "github.event.pull_request.head.ref", // The ref name of head.
-            "github.event.pull_request.head.sha", //  The commit SHA of head.
-            "github.event.pull_request.id", // The pull request ID.
-            "github.event.pull_request.number", // The pull request number.
-            "github.event.pull_request.merge_commit_sha", // The SHA of the merge commit.
-            "github.head_ref", // The head_ref or source branch of the pull request in a workflow run.
-            "github.event.workflow_run.head_branch", // The branch of the head commit.
-            "github.event.workflow_run.head_commit.id", // The SHA of the head commit.
-            "github.event.workflow_run.head_sha", // The SHA of the head commit.
-            "env.GITHUB_HEAD_REF",
-          ] + "%")
+  exists(
+    Utils::normalizeExpr(s)
+        .regexpFind([
+            "\\bgithub\\.event\\.number\\b", // The pull request number.
+            "\\bgithub\\.event\\.pull_request\\.head\\.ref\\b", // The ref name of head.
+            "\\bgithub\\.event\\.pull_request\\.head\\.sha\\b", //  The commit SHA of head.
+            "\\bgithub\\.event\\.pull_request\\.id\\b", // The pull request ID.
+            "\\bgithub\\.event\\.pull_request\\.number\\b", // The pull request number.
+            "\\bgithub\\.event\\.pull_request\\.merge_commit_sha\\b", // The SHA of the merge commit.
+            "\\bgithub\\.head_ref\\b", // The head_ref or source branch of the pull request in a workflow run.
+            "\\bgithub\\.event\\.workflow_run\\.head_branch\\b", // The branch of the head commit.
+            "\\bgithub\\.event\\.workflow_run\\.head_commit\\.id\\b", // The SHA of the head commit.
+            "\\bgithub\\.event\\.workflow_run\\.head_sha\\b", // The SHA of the head commit.
+            "\\benv\\.GITHUB_HEAD_REF\\b",
+          ], _, _)
+  )
 }
 
 /** Checkout of a Pull Request HEAD ref */
@@ -68,7 +73,7 @@ class GitCheckout extends PRHeadCheckoutStep instanceof Run {
         or
         exists(string varname |
           containsHeadRef(this.getInScopeEnvVarExpr(varname).getExpression()) and
-          line.matches("%" + varname + "%")
+          exists(line.regexpFind(varname, _, _))
         )
       )
     )

From 872b1f88f053dbb29a422f5fa0b33b3e0933a907 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Wed, 13 Mar 2024 22:47:19 +0100
Subject: [PATCH 05/17] More regexp improvements

---
 ql/lib/codeql/actions/Ast.qll                          |  9 +++++----
 ql/lib/codeql/actions/ast/internal/Ast.qll             |  4 ++--
 ql/src/Debug/partial.ql                                |  1 +
 .../Security/CWE-094/.github/workflows/test.yml        |  4 ++--
 .../CWE-094/CriticalExpressionInjection.expected       | 10 +++++-----
 .../Security/CWE-094/ExpressionInjection.expected      | 10 +++++-----
 6 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll
index 3d675bebce0..143e89512fe 100644
--- a/ql/lib/codeql/actions/Ast.qll
+++ b/ql/lib/codeql/actions/Ast.qll
@@ -5,10 +5,9 @@ module Utils {
   bindingset[expr]
   string normalizeExpr(string expr) {
     result =
-      expr.replaceAll("['", ".")
-          .replaceAll("']", "")
-          .replaceAll("[\"", ".")
-          .replaceAll("\"]", "")
+      //[A-Za-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]
+      expr.regexpReplaceAll("\\['([a-zA-Z0-9_\\*\\-]+)'\\]", ".$1")
+          .regexpReplaceAll("\\[\"([a-zA-Z0-9_\\*\\-]+)\"\\]", ".$1")
           .regexpReplaceAll("\\s*\\.\\s*", ".")
   }
 }
@@ -45,6 +44,8 @@ class Expression extends AstNode instanceof ExpressionImpl {
   string getExpression() { result = expression }
 
   string getRawExpression() { result = rawExpression }
+
+  string getNormalizedExpression() { result = Utils::normalizeExpr(expression) }
 }
 
 /** A common class for `env` in workflow, job or step. */
diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
index 7ebed407c0f..b05dd852dbf 100644
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -30,8 +30,8 @@ string getASimpleReferenceExpression(YamlString s, int offset) {
   // not just the last (greedy match) or first (reluctant match).
   result =
     s.getValue()
-        .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9_\\[\\]\\*\\(\\)\\.\\-]+\\s*\\}\\}", _, offset)
-        .regexpCapture("(\\$\\{\\{\\s*[A-Za-z0-9_\\[\\]\\*\\((\\)\\.\\-]+\\s*\\}\\})", 1)
+        .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]+\\s*\\}\\}", _, offset)
+        .regexpCapture("(\\$\\{\\{\\s*[A-Za-z0-9'\"_\\[\\]\\*\\((\\)\\.\\-]+\\s*\\}\\})", 1)
 }
 
 private newtype TAstNode =
diff --git a/ql/src/Debug/partial.ql b/ql/src/Debug/partial.ql
index fb31fe20990..702a454645c 100644
--- a/ql/src/Debug/partial.ql
+++ b/ql/src/Debug/partial.ql
@@ -8,6 +8,7 @@
  */
 
 import actions
+import codeql.actions.DataFlow
 import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import PartialFlow::PartialPathGraph
diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/test.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/test.yml
index 628b6e6f1bf..b9fa152e49a 100644
--- a/ql/test/query-tests/Security/CWE-094/.github/workflows/test.yml
+++ b/ql/test/query-tests/Security/CWE-094/.github/workflows/test.yml
@@ -12,7 +12,7 @@ jobs:
       - id: step0 
         uses: mad9000/actions-find-and-replace-string@3
         with:
-          source: ${{ github.event.head_commit.message }}
+          source: ${{ github.event['head_commit']['message'] }}
           find: 'foo'
           replace: ''
       - id: step1
@@ -34,4 +34,4 @@ jobs:
     needs: job1
 
     steps:
-      - run: echo ${{needs.job1.outputs.job_output}}
+      - run: echo ${{needs.job1.outputs['job_output']}}
diff --git a/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected b/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected
index 38884b3eaef..dfed1edb40a 100644
--- a/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected
+++ b/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected
@@ -44,10 +44,10 @@ edges
 | .github/workflows/simple2.yml:14:9:18:6 | Uses Step: source | .github/workflows/simple2.yml:22:20:22:64 | steps.source.outputs.all_changed_files |
 | .github/workflows/simple2.yml:18:9:26:6 | Uses Step: step [value] | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value |
 | .github/workflows/simple2.yml:22:20:22:64 | steps.source.outputs.all_changed_files | .github/workflows/simple2.yml:18:9:26:6 | Uses Step: step [value] |
-| .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | .github/workflows/test.yml:37:20:37:53 | needs.job1.outputs.job_output |
+| .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] |
 | .github/workflows/test.yml:8:20:8:50 | steps.step2.outputs.test | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] |
 | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | .github/workflows/test.yml:20:18:20:48 | steps.step0.outputs.value |
-| .github/workflows/test.yml:15:20:15:58 | github.event.head_commit.message | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] |
+| .github/workflows/test.yml:15:20:15:64 | github.event['head_commit']['message'] | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] |
 | .github/workflows/test.yml:18:9:24:6 | Run Step: step1 [MSG] | .github/workflows/test.yml:26:19:26:46 | steps.step1.outputs.MSG |
 | .github/workflows/test.yml:20:18:20:48 | steps.step0.outputs.value | .github/workflows/test.yml:18:9:24:6 | Run Step: step1 [MSG] |
 | .github/workflows/test.yml:24:9:29:2 | Run Step: step2 [test] | .github/workflows/test.yml:8:20:8:50 | steps.step2.outputs.test |
@@ -172,12 +172,12 @@ nodes
 | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] |
 | .github/workflows/test.yml:8:20:8:50 | steps.step2.outputs.test | semmle.label | steps.step2.outputs.test |
 | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | semmle.label | Uses Step: step0 [value] |
-| .github/workflows/test.yml:15:20:15:58 | github.event.head_commit.message | semmle.label | github.event.head_commit.message |
+| .github/workflows/test.yml:15:20:15:64 | github.event['head_commit']['message'] | semmle.label | github.event['head_commit']['message'] |
 | .github/workflows/test.yml:18:9:24:6 | Run Step: step1 [MSG] | semmle.label | Run Step: step1 [MSG] |
 | .github/workflows/test.yml:20:18:20:48 | steps.step0.outputs.value | semmle.label | steps.step0.outputs.value |
 | .github/workflows/test.yml:24:9:29:2 | Run Step: step2 [test] | semmle.label | Run Step: step2 [test] |
 | .github/workflows/test.yml:26:19:26:46 | steps.step1.outputs.MSG | semmle.label | steps.step1.outputs.MSG |
-| .github/workflows/test.yml:37:20:37:53 | needs.job1.outputs.job_output | semmle.label | needs.job1.outputs.job_output |
+| .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | semmle.label | needs.job1.outputs['job_output'] |
 | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | semmle.label | github.event.workflow_run.display_title |
 | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | semmle.label | github.event.workflow_run.head_commit.message |
 | .github/workflows/workflow_run.yml:11:19:11:75 | github.event.workflow_run.head_commit.author.email | semmle.label | github.event.workflow_run.head_commit.author.email |
@@ -254,7 +254,7 @@ subpaths
 | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | github.event.commits[11].committer.name |
 | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | .github/workflows/simple1.yml:11:20:11:58 | github.event.head_commit.message | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | steps.summary.outputs.value |
 | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | .github/workflows/simple2.yml:14:9:18:6 | Uses Step: source | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | steps.step.outputs.value |
-| .github/workflows/test.yml:37:20:37:53 | needs.job1.outputs.job_output | .github/workflows/test.yml:15:20:15:58 | github.event.head_commit.message | .github/workflows/test.yml:37:20:37:53 | needs.job1.outputs.job_output | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/test.yml:37:20:37:53 | needs.job1.outputs.job_output | needs.job1.outputs.job_output |
+| .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | .github/workflows/test.yml:15:20:15:64 | github.event['head_commit']['message'] | .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | needs.job1.outputs['job_output'] |
 | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | github.event.workflow_run.display_title |
 | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | github.event.workflow_run.head_commit.message |
 | .github/workflows/workflow_run.yml:11:19:11:75 | github.event.workflow_run.head_commit.author.email | .github/workflows/workflow_run.yml:11:19:11:75 | github.event.workflow_run.head_commit.author.email | .github/workflows/workflow_run.yml:11:19:11:75 | github.event.workflow_run.head_commit.author.email | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:11:19:11:75 | github.event.workflow_run.head_commit.author.email | github.event.workflow_run.head_commit.author.email |
diff --git a/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected b/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected
index 21a9978c54f..d22e9833f52 100644
--- a/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected
+++ b/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected
@@ -44,10 +44,10 @@ edges
 | .github/workflows/simple2.yml:14:9:18:6 | Uses Step: source | .github/workflows/simple2.yml:22:20:22:64 | steps.source.outputs.all_changed_files |
 | .github/workflows/simple2.yml:18:9:26:6 | Uses Step: step [value] | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value |
 | .github/workflows/simple2.yml:22:20:22:64 | steps.source.outputs.all_changed_files | .github/workflows/simple2.yml:18:9:26:6 | Uses Step: step [value] |
-| .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | .github/workflows/test.yml:37:20:37:53 | needs.job1.outputs.job_output |
+| .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] |
 | .github/workflows/test.yml:8:20:8:50 | steps.step2.outputs.test | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] |
 | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | .github/workflows/test.yml:20:18:20:48 | steps.step0.outputs.value |
-| .github/workflows/test.yml:15:20:15:58 | github.event.head_commit.message | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] |
+| .github/workflows/test.yml:15:20:15:64 | github.event['head_commit']['message'] | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] |
 | .github/workflows/test.yml:18:9:24:6 | Run Step: step1 [MSG] | .github/workflows/test.yml:26:19:26:46 | steps.step1.outputs.MSG |
 | .github/workflows/test.yml:20:18:20:48 | steps.step0.outputs.value | .github/workflows/test.yml:18:9:24:6 | Run Step: step1 [MSG] |
 | .github/workflows/test.yml:24:9:29:2 | Run Step: step2 [test] | .github/workflows/test.yml:8:20:8:50 | steps.step2.outputs.test |
@@ -172,12 +172,12 @@ nodes
 | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] |
 | .github/workflows/test.yml:8:20:8:50 | steps.step2.outputs.test | semmle.label | steps.step2.outputs.test |
 | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | semmle.label | Uses Step: step0 [value] |
-| .github/workflows/test.yml:15:20:15:58 | github.event.head_commit.message | semmle.label | github.event.head_commit.message |
+| .github/workflows/test.yml:15:20:15:64 | github.event['head_commit']['message'] | semmle.label | github.event['head_commit']['message'] |
 | .github/workflows/test.yml:18:9:24:6 | Run Step: step1 [MSG] | semmle.label | Run Step: step1 [MSG] |
 | .github/workflows/test.yml:20:18:20:48 | steps.step0.outputs.value | semmle.label | steps.step0.outputs.value |
 | .github/workflows/test.yml:24:9:29:2 | Run Step: step2 [test] | semmle.label | Run Step: step2 [test] |
 | .github/workflows/test.yml:26:19:26:46 | steps.step1.outputs.MSG | semmle.label | steps.step1.outputs.MSG |
-| .github/workflows/test.yml:37:20:37:53 | needs.job1.outputs.job_output | semmle.label | needs.job1.outputs.job_output |
+| .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | semmle.label | needs.job1.outputs['job_output'] |
 | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | semmle.label | github.event.workflow_run.display_title |
 | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | semmle.label | github.event.workflow_run.head_commit.message |
 | .github/workflows/workflow_run.yml:11:19:11:75 | github.event.workflow_run.head_commit.author.email | semmle.label | github.event.workflow_run.head_commit.author.email |
@@ -261,7 +261,7 @@ subpaths
 | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | ${{ github.event.commits[11].committer.name }} |
 | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | .github/workflows/simple1.yml:11:20:11:58 | github.event.head_commit.message | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | ${{steps.summary.outputs.value}} |
 | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | .github/workflows/simple2.yml:14:9:18:6 | Uses Step: source | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | ${{ steps.step.outputs.value }} |
-| .github/workflows/test.yml:37:20:37:53 | needs.job1.outputs.job_output | .github/workflows/test.yml:15:20:15:58 | github.event.head_commit.message | .github/workflows/test.yml:37:20:37:53 | needs.job1.outputs.job_output | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/test.yml:37:20:37:53 | needs.job1.outputs.job_output | ${{needs.job1.outputs.job_output}} |
+| .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | .github/workflows/test.yml:15:20:15:64 | github.event['head_commit']['message'] | .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | ${{needs.job1.outputs['job_output']}} |
 | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | ${{ github.event.workflow_run.display_title }} |
 | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | ${{ github.event.workflow_run.head_commit.message }} |
 | .github/workflows/workflow_run.yml:11:19:11:75 | github.event.workflow_run.head_commit.author.email | .github/workflows/workflow_run.yml:11:19:11:75 | github.event.workflow_run.head_commit.author.email | .github/workflows/workflow_run.yml:11:19:11:75 | github.event.workflow_run.head_commit.author.email | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:11:19:11:75 | github.event.workflow_run.head_commit.author.email | ${{ github.event.workflow_run.head_commit.author.email }} |

From fe1bf58ae537271980678fc34b9237e5a27fc4a9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 09:22:05 +0100
Subject: [PATCH 06/17] Apply suggestions from code review
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Jaroslav Lobačevski <jarlob@github.com>
---
 ql/src/Security/CWE-829/UntrustedCheckout.ql | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ql/src/Security/CWE-829/UntrustedCheckout.ql b/ql/src/Security/CWE-829/UntrustedCheckout.ql
index c9ad93d18b2..9ea69477675 100644
--- a/ql/src/Security/CWE-829/UntrustedCheckout.ql
+++ b/ql/src/Security/CWE-829/UntrustedCheckout.ql
@@ -37,6 +37,7 @@ predicate containsHeadRef(string s) {
     Utils::normalizeExpr(s)
         .regexpFind([
             "\\bgithub\\.event\\.number\\b", // The pull request number.
+            "\\bgithub\\.event\\.issue\\.number\\b", // The pull request number on issue_comment.
             "\\bgithub\\.event\\.pull_request\\.head\\.ref\\b", // The ref name of head.
             "\\bgithub\\.event\\.pull_request\\.head\\.sha\\b", //  The commit SHA of head.
             "\\bgithub\\.event\\.pull_request\\.id\\b", // The pull request ID.
@@ -82,7 +83,7 @@ class GitCheckout extends PRHeadCheckoutStep instanceof Run {
 
 from Workflow w, PRHeadCheckoutStep checkout
 where
-  w.hasTriggerEvent(["pull_request_target", "issue_comment", "workflow_run"]) and
+  w.hasTriggerEvent(["pull_request_target", "issue_comment", "pull_request_review_comment", "pull_request_review", "workflow_run", "check_run", "check_suite", "workflow_call"]) and
   w.getAJob().(LocalJob).getAStep() = checkout and
   not exists(ControlCheck check |
     checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check

From aa37339deb22e8ac1d24e7305659342be3bac1a8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 09:22:40 +0100
Subject: [PATCH 07/17] Apply suggestions from code review

---
 ql/lib/codeql/actions/Ast.qll              | 1 -
 ql/lib/codeql/actions/ast/internal/Ast.qll | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll
index 143e89512fe..19d1924731a 100644
--- a/ql/lib/codeql/actions/Ast.qll
+++ b/ql/lib/codeql/actions/Ast.qll
@@ -5,7 +5,6 @@ module Utils {
   bindingset[expr]
   string normalizeExpr(string expr) {
     result =
-      //[A-Za-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]
       expr.regexpReplaceAll("\\['([a-zA-Z0-9_\\*\\-]+)'\\]", ".$1")
           .regexpReplaceAll("\\[\"([a-zA-Z0-9_\\*\\-]+)\"\\]", ".$1")
           .regexpReplaceAll("\\s*\\.\\s*", ".")
diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
index b05dd852dbf..9a97a1c45b4 100644
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -47,7 +47,7 @@ private newtype TAstNode =
       )
     )
     or
-    // if's conditions do not need to be delimted with ${{}}
+    // `if`'s conditions do not need to be delimted with ${{}}
     exists(YamlMapping m |
       m.maps(key, value) and
       key.(YamlScalar).getValue() = ["if"] and

From e726f9fff12fb35b5f0b1287aa87e0e1e5fb7136 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 09:24:32 +0100
Subject: [PATCH 08/17] Apply suggestions from code review
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Jaroslav Lobačevski <jarlob@github.com>
---
 ql/src/Security/CWE-829/UntrustedCheckout.ql | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/ql/src/Security/CWE-829/UntrustedCheckout.ql b/ql/src/Security/CWE-829/UntrustedCheckout.ql
index 9ea69477675..4a0a4b6ade6 100644
--- a/ql/src/Security/CWE-829/UntrustedCheckout.ql
+++ b/ql/src/Security/CWE-829/UntrustedCheckout.ql
@@ -48,6 +48,26 @@ predicate containsHeadRef(string s) {
             "\\bgithub\\.event\\.workflow_run\\.head_commit\\.id\\b", // The SHA of the head commit.
             "\\bgithub\\.event\\.workflow_run\\.head_sha\\b", // The SHA of the head commit.
             "\\benv\\.GITHUB_HEAD_REF\\b",
+            
+            "\\bgithub\\.event\\.check_suite\\.after\\b",
+            "\\bgithub\\.event\\.check_suite\\.head_sha\\b",
+            "\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.ref\\b",
+            "\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.sha\\b",
+            "\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.id\\b",
+            "\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.number\\b",
+            
+            "\\bgithub\\.event\\.check_run\\.check_suite\\.after\\b",
+            "\\bgithub\\.event\\.check_run\\.check_suite\\.head_sha\\b",
+            "\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.ref\\b",
+            "\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.sha\\b",
+            "\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.id\\b",
+            "\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.number\\b",
+            
+            "\\bgithub\\.event\\.check_run\\.head_sha\\b",
+            "\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.head\\.ref\\b",
+            "\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.head\\.sha\\b",
+            "\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.id\\b",
+            "\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.number\\b",
           ], _, _)
   )
 }

From 3e2dffce8be696ae9a22b6605192aca3f85c728b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 11:57:43 +0100
Subject: [PATCH 09/17] Rename ContextExpression to SimpleReferenceExpression

---
 ql/lib/codeql/actions/Ast.qll                 | 14 ++---
 ql/lib/codeql/actions/ast/internal/Ast.qll    | 53 ++++++++++++-------
 .../dataflow/internal/DataFlowPrivate.qll     |  2 +-
 3 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll
index 19d1924731a..70424a46f95 100644
--- a/ql/lib/codeql/actions/Ast.qll
+++ b/ql/lib/codeql/actions/Ast.qll
@@ -226,20 +226,20 @@ class Run extends Step instanceof RunImpl {
   Expression getAnScriptExpr() { result = super.getAnScriptExpr() }
 }
 
-abstract class ContextExpression extends AstNode instanceof ContextExpressionImpl {
+abstract class SimpleReferenceExpression extends AstNode instanceof SimpleReferenceExpressionImpl {
   string getFieldName() { result = super.getFieldName() }
 
   AstNode getTarget() { result = super.getTarget() }
 }
 
-class StepsExpression extends ContextExpression instanceof StepsExpressionImpl { }
+class StepsExpression extends SimpleReferenceExpression instanceof StepsExpressionImpl { }
 
-class NeedsExpression extends ContextExpression instanceof NeedsExpressionImpl { }
+class NeedsExpression extends SimpleReferenceExpression instanceof NeedsExpressionImpl { }
 
-class JobsExpression extends ContextExpression instanceof JobsExpressionImpl { }
+class JobsExpression extends SimpleReferenceExpression instanceof JobsExpressionImpl { }
 
-class InputsExpression extends ContextExpression instanceof InputsExpressionImpl { }
+class InputsExpression extends SimpleReferenceExpression instanceof InputsExpressionImpl { }
 
-class EnvExpression extends ContextExpression instanceof EnvExpressionImpl { }
+class EnvExpression extends SimpleReferenceExpression instanceof EnvExpressionImpl { }
 
-class MatrixExpression extends ContextExpression instanceof MatrixExpressionImpl { }
+class MatrixExpression extends SimpleReferenceExpression instanceof MatrixExpressionImpl { }
diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
index 9a97a1c45b4..1f206c964eb 100644
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -19,24 +19,18 @@ int partialLineLengthSum(string text, int i) {
   result = sum(int j, int length | j in [0 .. i] and length = lineLength(text, j) | length)
 }
 
-/**
- * Holds if `${{ e }}` is a GitHub Actions expression evaluated within this YAML string.
- * See https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions.
- * Only finds simple expressions like `${{ github.event.comment.body }}`, where the expression contains only alphanumeric characters, underscores, dots, or dashes.
- * Does not identify more complicated expressions like `${{ fromJSON(env.time) }}`, or ${{ format('{{Hello {0}!}}', github.event.head_commit.author.name) }}
- */
-string getASimpleReferenceExpression(YamlString s, int offset) {
+string getADelimitedExpression(YamlString s, int offset) {
   // We use `regexpFind` to obtain *all* matches of `${{...}}`,
   // not just the last (greedy match) or first (reluctant match).
   result =
     s.getValue()
-        .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]+\\s*\\}\\}", _, offset)
-        .regexpCapture("(\\$\\{\\{\\s*[A-Za-z0-9'\"_\\[\\]\\*\\((\\)\\.\\-]+\\s*\\}\\})", 1)
+        .regexpFind("\\$\\{\\{\\s*.*\\s*\\}\\}", _, offset)
+        .regexpCapture("(\\$\\{\\{\\s*.*\\s*\\}\\})", 1)
 }
 
 private newtype TAstNode =
   TExpressionNode(YamlNode key, YamlScalar value, string raw, int exprOffset) {
-    raw = getASimpleReferenceExpression(value, exprOffset) and
+    raw = getADelimitedExpression(value, exprOffset) and
     exists(YamlMapping m |
       (
         exists(int i | value = m.getValueNode(i) and key = m.getKeyNode(i))
@@ -789,11 +783,29 @@ class RunImpl extends StepImpl {
   }
 }
 
+/**
+ * Holds if `${{ e }}` is a GitHub Actions expression evaluated within this YAML string.
+ * See https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions.
+ * Only finds simple expressions like `${{ github.event.comment.body }}`, where the expression contains only alphanumeric characters, underscores, dots, or dashes.
+ * Does not identify more complicated expressions like `${{ fromJSON(env.time) }}`, or ${{ format('{{Hello {0}!}}', github.event.head_commit.author.name) }}
+ */
+bindingset[s]
+string getASimpleReferenceExpression(string s, int offset) {
+  // We use `regexpFind` to obtain *all* matches of `${{...}}`,
+  // not just the last (greedy match) or first (reluctant match).
+  result =
+    s.trim()
+        .regexpFind("[A-Za-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]+", _, offset)
+        .regexpCapture("([A-Za-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]+)", 1)
+}
+
 /**
  * A ${{}} expression accessing a context variable such as steps, needs, jobs, env, inputs, or matrix.
  * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
  */
-abstract class ContextExpressionImpl extends ExpressionImpl {
+abstract class SimpleReferenceExpressionImpl extends ExpressionImpl {
+  SimpleReferenceExpressionImpl() { exists(getASimpleReferenceExpression(expression, _)) }
+
   abstract string getFieldName();
 
   abstract AstNodeImpl getTarget();
@@ -829,7 +841,7 @@ private string wrapRegexp(string regex) {
  * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
  * e.g. `${{ steps.changed-files.outputs.all_changed_files }}`
  */
-class StepsExpressionImpl extends ContextExpressionImpl {
+class StepsExpressionImpl extends SimpleReferenceExpressionImpl {
   string stepId;
   string fieldName;
 
@@ -842,7 +854,7 @@ class StepsExpressionImpl extends ContextExpressionImpl {
   override string getFieldName() { result = fieldName }
 
   override AstNodeImpl getTarget() {
-    this.getLocation().getFile() = result.getLocation().getFile() and
+    this.getEnclosingJob() = result.getEnclosingJob() and
     result.(StepImpl).getId() = stepId
   }
 }
@@ -852,7 +864,7 @@ class StepsExpressionImpl extends ContextExpressionImpl {
  * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
  * e.g. `${{ needs.job1.outputs.foo}}`
  */
-class NeedsExpressionImpl extends ContextExpressionImpl {
+class NeedsExpressionImpl extends SimpleReferenceExpressionImpl {
   JobImpl neededJob;
   string fieldName;
 
@@ -866,7 +878,10 @@ class NeedsExpressionImpl extends ContextExpressionImpl {
   override string getFieldName() { result = fieldName }
 
   override AstNodeImpl getTarget() {
-    this.getEnclosingJob().getANeededJob() = neededJob and
+    (
+      this.getEnclosingJob().getANeededJob() = neededJob or
+      this.getEnclosingJob() = neededJob
+    ) and
     (
       // regular jobs
       neededJob.getOutputs() = result
@@ -882,7 +897,7 @@ class NeedsExpressionImpl extends ContextExpressionImpl {
  * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
  * e.g. `${{ jobs.job1.outputs.foo}}` (within reusable workflows)
  */
-class JobsExpressionImpl extends ContextExpressionImpl {
+class JobsExpressionImpl extends SimpleReferenceExpressionImpl {
   string jobId;
   string fieldName;
 
@@ -908,7 +923,7 @@ class JobsExpressionImpl extends ContextExpressionImpl {
  * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
  * e.g. `${{ inputs.foo }}`
  */
-class InputsExpressionImpl extends ContextExpressionImpl {
+class InputsExpressionImpl extends SimpleReferenceExpressionImpl {
   string fieldName;
 
   InputsExpressionImpl() {
@@ -933,7 +948,7 @@ class InputsExpressionImpl extends ContextExpressionImpl {
  * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
  * e.g. `${{ env.foo }}`
  */
-class EnvExpressionImpl extends ContextExpressionImpl {
+class EnvExpressionImpl extends SimpleReferenceExpressionImpl {
   string fieldName;
 
   EnvExpressionImpl() {
@@ -956,7 +971,7 @@ class EnvExpressionImpl extends ContextExpressionImpl {
  * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
  * e.g. `${{ matrix.foo }}`
  */
-class MatrixExpressionImpl extends ContextExpressionImpl {
+class MatrixExpressionImpl extends SimpleReferenceExpressionImpl {
   string fieldName;
 
   MatrixExpressionImpl() {
diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll
index bda55da5c82..f1657717e04 100644
--- a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll
+++ b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll
@@ -271,7 +271,7 @@ predicate jumpStep(Node nodeFrom, Node nodeTo) { none() }
  * Holds if a Expression reads a field from a job (needs/jobs), step (steps) output via a read of `c` (fieldname)
  */
 predicate ctxFieldReadStep(Node node1, Node node2, ContentSet c) {
-  exists(ContextExpression access |
+  exists(SimpleReferenceExpression access |
     (
       access instanceof NeedsExpression or
       access instanceof StepsExpression or

From 8e2c1a4f4ed140e5aae014936a19039234f81dc1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 11:58:07 +0100
Subject: [PATCH 10/17] Expose predicates to check local flow

---
 .../actions/dataflow/internal/DataFlowPublic.qll    | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll
index dbae273151b..8e8ed5d9280 100644
--- a/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll
+++ b/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll
@@ -174,3 +174,16 @@ class FieldContent extends Content, TFieldContent {
 
   override string toString() { result = name }
 }
+
+predicate hasLocalFlow(Node n1, Node n2) {
+  simpleLocalFlowStep(n1, n2) or
+  exists(ContentSet c | ctxFieldReadStep(n1, n2, c))
+}
+
+predicate hasLocalFlowExpr(AstNode n1, AstNode n2) {
+  exists(Node dn1, Node dn2 |
+    dn1.asExpr() = n1 and
+    dn2.asExpr() = n2 and
+    hasLocalFlow(dn1, dn2)
+  )
+}

From 03277cc24bfa901ad253f2893cc4dbb00e9ad16d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 11:58:44 +0100
Subject: [PATCH 11/17] Add test for self-referencing jobs

---
 .../CWE-094/.github/workflows/self_needs.yml  | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 ql/test/query-tests/Security/CWE-094/.github/workflows/self_needs.yml

diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/self_needs.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/self_needs.yml
new file mode 100644
index 00000000000..afd39605bb3
--- /dev/null
+++ b/ql/test/query-tests/Security/CWE-094/.github/workflows/self_needs.yml
@@ -0,0 +1,20 @@
+name: Test
+
+on:
+  issue_comment:
+    types: [created]
+
+jobs:
+  test1:
+    runs-on: ubuntu-22.04
+    outputs:
+      job_output: ${{ steps.source.outputs.value }}
+    steps:
+      - id: source
+        uses: mad9000/actions-find-and-replace-string@3
+        with:
+          source: ${{ github.event['head_commit']['message'] }}
+          find: 'foo'
+          replace: ''
+      - run: ${{ steps.source.outputs.value }}
+      - run: ${{ needs.test1.outputs.job_output }}

From 7160f08222691a9182870200a6451b353604d0d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 12:03:40 +0100
Subject: [PATCH 12/17] Update
 ql/test/query-tests/Security/CWE-829/.github/workflows/auto_ci.yml
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Jaroslav Lobačevski <jarlob@github.com>
---
 .../query-tests/Security/CWE-829/.github/workflows/auto_ci.yml  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/auto_ci.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/auto_ci.yml
index cb20cfe629b..28ffab637f0 100644
--- a/ql/test/query-tests/Security/CWE-829/.github/workflows/auto_ci.yml
+++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/auto_ci.yml
@@ -68,7 +68,7 @@ jobs:
         uses: actions/checkout@v3
         with:
           fetch-depth: 0
-          ref: ${{ github.event.pull_request.head.ref }}
+          ref: ${{ github.event.pull_request.head.ref || github.event.pull_request.base.ref }}
           repository: ${{ github.event.pull_request.head.repo.full_name }}
 
       - name: Set up Python ${{ matrix.python-version }}

From 3150f24d3fc3c283df4c372925edc97800092b2d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 12:21:16 +0100
Subject: [PATCH 13/17] Update tests and fix regexp

---
 ql/lib/codeql/actions/ast/internal/Ast.qll          |  4 ++--
 .../CWE-094/CriticalExpressionInjection.expected    | 11 +++++++++++
 .../Security/CWE-094/ExpressionInjection.expected   | 13 +++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
index 1f206c964eb..ffe85b16f93 100644
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -24,8 +24,8 @@ string getADelimitedExpression(YamlString s, int offset) {
   // not just the last (greedy match) or first (reluctant match).
   result =
     s.getValue()
-        .regexpFind("\\$\\{\\{\\s*.*\\s*\\}\\}", _, offset)
-        .regexpCapture("(\\$\\{\\{\\s*.*\\s*\\}\\})", 1)
+        .regexpFind("\\$\\{\\{\\s*[^\\}]+\\s*\\}\\}", _, offset)
+        .regexpCapture("(\\$\\{\\{\\s*[^\\}]+\\s*\\}\\})", 1)
 }
 
 private newtype TAstNode =
diff --git a/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected b/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected
index dfed1edb40a..aa9d9ae2fc4 100644
--- a/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected
+++ b/ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected
@@ -39,6 +39,11 @@ edges
 | .github/workflows/issues.yaml:4:16:4:46 | github.event.issue.title | .github/workflows/issues.yaml:15:19:15:39 | env.global_env |
 | .github/workflows/issues.yaml:10:17:10:47 | github.event.issue.title | .github/workflows/issues.yaml:17:19:17:36 | env.job_env |
 | .github/workflows/issues.yaml:20:20:20:50 | github.event.issue.title | .github/workflows/issues.yaml:18:19:18:37 | env.step_env |
+| .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] | .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output |
+| .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value | .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] |
+| .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value |
+| .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value |
+| .github/workflows/self_needs.yml:16:20:16:64 | github.event['head_commit']['message'] | .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] |
 | .github/workflows/simple1.yml:8:9:14:6 | Uses Step: summary [value] | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value |
 | .github/workflows/simple1.yml:11:20:11:58 | github.event.head_commit.message | .github/workflows/simple1.yml:8:9:14:6 | Uses Step: summary [value] |
 | .github/workflows/simple2.yml:14:9:18:6 | Uses Step: source | .github/workflows/simple2.yml:22:20:22:64 | steps.source.outputs.all_changed_files |
@@ -162,6 +167,12 @@ nodes
 | .github/workflows/push.yml:14:19:14:64 | github.event.head_commit.committer.name | semmle.label | github.event.head_commit.committer.name |
 | .github/workflows/push.yml:15:19:15:65 | github.event.commits[11].committer.email | semmle.label | github.event.commits[11].committer.email |
 | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | semmle.label | github.event.commits[11].committer.name |
+| .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] |
+| .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value | semmle.label | steps.source.outputs.value |
+| .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | semmle.label | Uses Step: source [value] |
+| .github/workflows/self_needs.yml:16:20:16:64 | github.event['head_commit']['message'] | semmle.label | github.event['head_commit']['message'] |
+| .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | semmle.label | steps.source.outputs.value |
+| .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | semmle.label | needs.test1.outputs.job_output |
 | .github/workflows/simple1.yml:8:9:14:6 | Uses Step: summary [value] | semmle.label | Uses Step: summary [value] |
 | .github/workflows/simple1.yml:11:20:11:58 | github.event.head_commit.message | semmle.label | github.event.head_commit.message |
 | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | semmle.label | steps.summary.outputs.value |
diff --git a/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected b/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected
index d22e9833f52..d4fd27b18d4 100644
--- a/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected
+++ b/ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected
@@ -39,6 +39,11 @@ edges
 | .github/workflows/issues.yaml:4:16:4:46 | github.event.issue.title | .github/workflows/issues.yaml:15:19:15:39 | env.global_env |
 | .github/workflows/issues.yaml:10:17:10:47 | github.event.issue.title | .github/workflows/issues.yaml:17:19:17:36 | env.job_env |
 | .github/workflows/issues.yaml:20:20:20:50 | github.event.issue.title | .github/workflows/issues.yaml:18:19:18:37 | env.step_env |
+| .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] | .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output |
+| .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value | .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] |
+| .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value |
+| .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value |
+| .github/workflows/self_needs.yml:16:20:16:64 | github.event['head_commit']['message'] | .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] |
 | .github/workflows/simple1.yml:8:9:14:6 | Uses Step: summary [value] | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value |
 | .github/workflows/simple1.yml:11:20:11:58 | github.event.head_commit.message | .github/workflows/simple1.yml:8:9:14:6 | Uses Step: summary [value] |
 | .github/workflows/simple2.yml:14:9:18:6 | Uses Step: source | .github/workflows/simple2.yml:22:20:22:64 | steps.source.outputs.all_changed_files |
@@ -162,6 +167,12 @@ nodes
 | .github/workflows/push.yml:14:19:14:64 | github.event.head_commit.committer.name | semmle.label | github.event.head_commit.committer.name |
 | .github/workflows/push.yml:15:19:15:65 | github.event.commits[11].committer.email | semmle.label | github.event.commits[11].committer.email |
 | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | semmle.label | github.event.commits[11].committer.name |
+| .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] |
+| .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value | semmle.label | steps.source.outputs.value |
+| .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | semmle.label | Uses Step: source [value] |
+| .github/workflows/self_needs.yml:16:20:16:64 | github.event['head_commit']['message'] | semmle.label | github.event['head_commit']['message'] |
+| .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | semmle.label | steps.source.outputs.value |
+| .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | semmle.label | needs.test1.outputs.job_output |
 | .github/workflows/simple1.yml:8:9:14:6 | Uses Step: summary [value] | semmle.label | Uses Step: summary [value] |
 | .github/workflows/simple1.yml:11:20:11:58 | github.event.head_commit.message | semmle.label | github.event.head_commit.message |
 | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | semmle.label | steps.summary.outputs.value |
@@ -259,6 +270,8 @@ subpaths
 | .github/workflows/push.yml:14:19:14:64 | github.event.head_commit.committer.name | .github/workflows/push.yml:14:19:14:64 | github.event.head_commit.committer.name | .github/workflows/push.yml:14:19:14:64 | github.event.head_commit.committer.name | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/push.yml:14:19:14:64 | github.event.head_commit.committer.name | ${{ github.event.head_commit.committer.name }} |
 | .github/workflows/push.yml:15:19:15:65 | github.event.commits[11].committer.email | .github/workflows/push.yml:15:19:15:65 | github.event.commits[11].committer.email | .github/workflows/push.yml:15:19:15:65 | github.event.commits[11].committer.email | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/push.yml:15:19:15:65 | github.event.commits[11].committer.email | ${{ github.event.commits[11].committer.email }} |
 | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | ${{ github.event.commits[11].committer.name }} |
+| .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | .github/workflows/self_needs.yml:16:20:16:64 | github.event['head_commit']['message'] | .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | ${{ steps.source.outputs.value }} |
+| .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | .github/workflows/self_needs.yml:16:20:16:64 | github.event['head_commit']['message'] | .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | ${{ needs.test1.outputs.job_output }} |
 | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | .github/workflows/simple1.yml:11:20:11:58 | github.event.head_commit.message | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | ${{steps.summary.outputs.value}} |
 | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | .github/workflows/simple2.yml:14:9:18:6 | Uses Step: source | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | ${{ steps.step.outputs.value }} |
 | .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | .github/workflows/test.yml:15:20:15:64 | github.event['head_commit']['message'] | .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/test.yml:37:20:37:56 | needs.job1.outputs['job_output'] | ${{needs.job1.outputs['job_output']}} |

From 9ca1ac5bb9283d413eefc3bbb686949f69d90a4c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 12:58:02 +0100
Subject: [PATCH 14/17] Fix expression regexp

---
 ql/lib/codeql/actions/ast/internal/Ast.qll | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
index ffe85b16f93..084474b4020 100644
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -24,8 +24,9 @@ string getADelimitedExpression(YamlString s, int offset) {
   // not just the last (greedy match) or first (reluctant match).
   result =
     s.getValue()
-        .regexpFind("\\$\\{\\{\\s*[^\\}]+\\s*\\}\\}", _, offset)
-        .regexpCapture("(\\$\\{\\{\\s*[^\\}]+\\s*\\}\\})", 1)
+        .regexpFind("\\$\\{\\{(?:[^}]|}(?!}))*\\}\\}", _, offset)
+        .regexpCapture("(\\$\\{\\{(?:[^}]|}(?!}))*\\}\\})", 1)
+        .trim()
 }
 
 private newtype TAstNode =

From 35df9519e14a43946cca042b4286477f95c96b3f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 12:58:47 +0100
Subject: [PATCH 15/17] Support more untrusted checkout cases

---
 ql/src/Security/CWE-829/UntrustedCheckout.ql  |  22 ++--
 .../issue_comment_3rd_party_action.yml        |  53 ++++++++
 .../workflows/issue_comment_direct.yml        |  46 +++++++
 .../workflows/issue_comment_heuristic.yml     |  50 ++++++++
 .../workflows/issue_comment_octokit.yml       | 114 ++++++++++++++++++
 .../CWE-829/UnpinnedActionsTag.expected       |   6 +
 .../CWE-829/UntrustedCheckout.expected        |   9 ++
 7 files changed, 293 insertions(+), 7 deletions(-)
 create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_3rd_party_action.yml
 create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_direct.yml
 create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_heuristic.yml
 create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_octokit.yml

diff --git a/ql/src/Security/CWE-829/UntrustedCheckout.ql b/ql/src/Security/CWE-829/UntrustedCheckout.ql
index 4a0a4b6ade6..a24c80a2f60 100644
--- a/ql/src/Security/CWE-829/UntrustedCheckout.ql
+++ b/ql/src/Security/CWE-829/UntrustedCheckout.ql
@@ -14,6 +14,7 @@
  */
 
 import actions
+import codeql.actions.DataFlow
 
 /** An If node that contains an actor, user or label check */
 class ControlCheck extends If {
@@ -23,6 +24,7 @@ class ControlCheck extends If {
           .regexpFind([
               "\\bgithub\\.actor\\b", // actor
               "\\bgithub\\.triggering_actor\\b", // actor
+              "\\bgithub\\.event\\.comment\\.user\\.login\\b", //user
               "\\bgithub\\.event\\.pull_request\\.user\\.login\\b", //user
               "\\bgithub\\.event\\.pull_request\\.labels\\b", // label
               "\\bgithub\\.event\\.label\\.name\\b" // label
@@ -47,22 +49,18 @@ predicate containsHeadRef(string s) {
             "\\bgithub\\.event\\.workflow_run\\.head_branch\\b", // The branch of the head commit.
             "\\bgithub\\.event\\.workflow_run\\.head_commit\\.id\\b", // The SHA of the head commit.
             "\\bgithub\\.event\\.workflow_run\\.head_sha\\b", // The SHA of the head commit.
-            "\\benv\\.GITHUB_HEAD_REF\\b",
-            
-            "\\bgithub\\.event\\.check_suite\\.after\\b",
+            "\\benv\\.GITHUB_HEAD_REF\\b", "\\bgithub\\.event\\.check_suite\\.after\\b",
             "\\bgithub\\.event\\.check_suite\\.head_sha\\b",
             "\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.ref\\b",
             "\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.sha\\b",
             "\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.id\\b",
             "\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.number\\b",
-            
             "\\bgithub\\.event\\.check_run\\.check_suite\\.after\\b",
             "\\bgithub\\.event\\.check_run\\.check_suite\\.head_sha\\b",
             "\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.ref\\b",
             "\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.sha\\b",
             "\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.id\\b",
             "\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.number\\b",
-            
             "\\bgithub\\.event\\.check_run\\.head_sha\\b",
             "\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.head\\.ref\\b",
             "\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.head\\.sha\\b",
@@ -79,7 +77,14 @@ abstract class PRHeadCheckoutStep extends Step { }
 class ActionsCheckout extends PRHeadCheckoutStep instanceof UsesStep {
   ActionsCheckout() {
     this.getCallee() = "actions/checkout" and
-    containsHeadRef(this.getArgumentExpr("ref").getExpression())
+    (
+      containsHeadRef(this.getArgumentExpr("ref").getExpression())
+      or
+      exists(UsesStep head |
+        head.getCallee() = ["eficode/resolve-pr-refs", "xt0rted/pull-request-comment-branch"] and
+        DataFlow::hasLocalFlowExpr(head, this.getArgumentExpr("ref"))
+      )
+    )
   }
 }
 
@@ -103,7 +108,10 @@ class GitCheckout extends PRHeadCheckoutStep instanceof Run {
 
 from Workflow w, PRHeadCheckoutStep checkout
 where
-  w.hasTriggerEvent(["pull_request_target", "issue_comment", "pull_request_review_comment", "pull_request_review", "workflow_run", "check_run", "check_suite", "workflow_call"]) and
+  w.hasTriggerEvent([
+      "pull_request_target", "issue_comment", "pull_request_review_comment", "pull_request_review",
+      "workflow_run", "check_run", "check_suite", "workflow_call"
+    ]) and
   w.getAJob().(LocalJob).getAStep() = checkout and
   not exists(ControlCheck check |
     checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_3rd_party_action.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_3rd_party_action.yml
new file mode 100644
index 00000000000..4de47d6f17a
--- /dev/null
+++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_3rd_party_action.yml
@@ -0,0 +1,53 @@
+name: PR head from 3rd party action
+
+on:
+  workflow_call:
+  workflow_dispatch:
+
+jobs:
+
+  test1:
+    runs-on: ubuntu-20.04
+    steps:
+      - name: (PR comment) Get PR branch
+        if: ${{ github.event_name == 'issue_comment' }}
+        uses: xt0rted/pull-request-comment-branch@v2
+        id: comment-branch
+
+      - name: (PR comment) Checkout PR branch
+        if: ${{ github.event_name == 'issue_comment' }}
+        uses: actions/checkout@v3
+        with:
+          ref: ${{ steps.comment-branch.outputs.head_sha }}
+
+  test2:
+    runs-on: ubuntu-20.04
+    steps:
+      - name: (PR comment) Get PR branch
+        if: ${{ github.event_name == 'issue_comment' }}
+        uses: xt0rted/pull-request-comment-branch@v2
+        id: comment-branch
+
+      - name: (PR comment) Checkout PR branch
+        if: ${{ github.event_name == 'issue_comment' }}
+        uses: actions/checkout@v3
+        with:
+          ref: ${{ steps.comment-branch.outputs.head_ref }}
+
+  test3:
+    runs-on: ubuntu-20.04
+    steps:
+      - name: resolve pr refs
+        id: refs
+        uses: eficode/resolve-pr-refs@main
+        with:
+          token: ${{ secrets.GITHUB_TOKEN }}
+
+      - uses: actions/checkout@v4
+        with:
+          ref: ${{ steps.refs.outputs.head_ref }}
+          fetch-depth: 0
+      - uses: actions/checkout@v4
+        with:
+          ref: ${{ steps.refs.outputs.head_sha }}
+          fetch-depth: 0
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_direct.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_direct.yml
new file mode 100644
index 00000000000..ece4c02c356
--- /dev/null
+++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_direct.yml
@@ -0,0 +1,46 @@
+name: Direct access
+
+on:
+  issue_comment:
+    types: [created]
+
+jobs:
+  test1:
+    runs-on: ubuntu-latest
+    if: github.event_name == 'issue_comment' && github.event.issue.pull_request
+    steps:
+      - name: Unsafe Code Checkout
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ github.event.pull_request.head.ref || github.head_ref }} # Checkout the branch that made the PR or the comment's PR branch
+  test2:
+    runs-on: ubuntu-latest
+    if: github.event.issue.pull_request && github.event.comment.body == '/trigger release'
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          ref: refs/pull/${{ github.event.issue.number }}/merge
+
+  test3:
+    runs-on: ubuntu-latest
+    if: github.event.issue.pull_request && github.event.comment.body == '/trigger release'
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          ref: ${{ format('refs/pull/{0}/merge', github.event.issue.number) }}
+
+  test4:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout Branch
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ (github.event_name == 'pull_request_review_comment') && format('refs/pull/{0}/merge',  github.event.pull_request.number) || '' }}
+
+  test5:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout Branch
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ github.event_name == 'issue_comment' && format('refs/pull/{0}/merge',  github.event.issue.number) || '' }}
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_heuristic.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_heuristic.yml
new file mode 100644
index 00000000000..8c0865f598c
--- /dev/null
+++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_heuristic.yml
@@ -0,0 +1,50 @@
+name: Heuristic based
+
+on:
+  issue_comment:
+    types: [created]
+
+jobs:
+  test1:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Get Info from comment
+        uses: actions/github-script@v7
+        id: get-pr-info
+        with:
+          script: |
+            const request = {
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                pull_number: ${{ github.event.issue.number }},
+            }
+            core.info(`Getting PR #${request.pull_number} from ${request.owner}/${request.repo}`)
+            const pr = await github.rest.pulls.get(request);
+            return pr.data;
+      - name: Debug
+        id: get-sha
+        run: |
+          echo "sha=${{ fromJSON(steps.get-pr-info.outputs.result).head.sha }}" >> $GITHUB_OUTPUT
+      - name: "Checkout ${{ github.ref }} ( ${{ github.sha }} : ${{steps.get-sha.outputs.sha}} )"
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ steps.get-sha.outputs.sha }}
+
+  test2:
+    runs-on: ubuntu-latest
+
+    steps:
+    - name: Detect branch for PR
+      id: vars
+      run: |
+        PR=$( echo "${{ github.event.comment.issue_url }}" | grep -oE 'issues/([0-9]+)$' | cut -d'/' -f 2 )
+        PR_INFO=$( curl \
+                 --request GET \
+                 --header 'authorization: Bearer ${{ secrets.GITHUB_TOKEN }}' \
+                 --header 'content-type: application/json' \
+                 --url https://api.github.com/repos/$GITHUB_REPOSITORY/pulls/$PR )
+        REF=$(echo "${PR_INFO}" | jq -r .head.ref)
+        echo "branch=$REF" >> $GITHUB_OUTPUT
+    - uses: actions/checkout@v4
+      with:
+        ref: ${{ steps.vars.outputs.branch }}
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_octokit.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_octokit.yml
new file mode 100644
index 00000000000..1245d0302fb
--- /dev/null
+++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_octokit.yml
@@ -0,0 +1,114 @@
+name: Octokit (heuristics)
+
+on:
+  issue_comment:
+    types: [created]
+
+jobs:
+  test1: 
+    if: github.event.comment.body == '@metabase-bot run visual tests'
+    runs-on: ubuntu-22.04
+    steps:
+      - name: Fetch issue
+        uses: octokit/request-action@v2.x
+        id: fetch_issue
+        with:
+          route: GET ${{ github.event.issue.url }}
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+      - name: Fetch PR
+        uses: octokit/request-action@v2.x
+        id: fetch_pr
+        with:
+          route: GET ${{ fromJson(steps.fetch_issue.outputs.data).pull_request.url }}
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+      - uses: actions/checkout@v4
+        with:
+          ref: ${{ fromJson(steps.fetch_pr.outputs.data).head.ref }}
+          token: ${{ secrets.GITHUB_TOKEN }}
+      - uses: actions/checkout@v4
+        with:
+          ref: ${{ fromJson(steps.fetch_pr.outputs.data).head.sha }}
+          token: ${{ secrets.GITHUB_TOKEN }}
+
+  test2:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Get Info from comment
+        uses: actions/github-script@v7
+        id: get-pr-info
+        with:
+          script: |
+            const request = {
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                pull_number: ${{ github.event.issue.number }},
+            }
+            core.info(`Getting PR #${request.pull_number} from ${request.owner}/${request.repo}`)
+            const pr = await github.rest.pulls.get(request);
+            return pr.data;
+
+      - name: Debug
+        id: get-sha
+        run: |
+          echo "sha=${{ fromJSON(steps.get-pr-info.outputs.result).head.sha }}" >> $GITHUB_OUTPUT
+
+      - name: "Checkout ${{ github.ref }} ( ${{ github.sha }} : ${{steps.get-sha.outputs.sha}} )"
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ steps.get-sha.outputs.sha }}
+
+  test3:
+    if: github.event.comment.body == '@excalibot trigger release' && github.event.issue.pull_request
+    runs-on: ubuntu-latest
+    steps:
+      - name: Get PR SHA
+        id: sha
+        uses: actions/github-script@v4
+        with:
+          result-encoding: string
+          script: |
+            const { owner, repo, number } = context.issue;
+            const pr = await github.pulls.get({
+              owner,
+              repo,
+              pull_number: number,
+            });
+            return pr.data.head.sha
+      - uses: actions/checkout@v2
+        with:
+          ref: ${{ steps.sha.outputs.result }}
+
+  test4:
+    if: github.event.issue.pull_request && contains(github.event.comment.body, '!bench_parser')
+    runs-on: ubuntu-latest
+    steps:
+      - name: Get PR SHA
+        id: sha
+        uses: actions/github-script@v6
+        with:
+          result-encoding: string
+          script: |
+            const response = await github.request(context.payload.issue.pull_request.url);
+            return response.data.head.sha;
+      - name: Checkout PR Branch
+        uses: actions/checkout@v3
+        with:
+          ref: ${{ steps.sha.outputs.result }}
+
+  test5:
+    runs-on: ubuntu-20.04
+    steps:
+      - id: request
+        uses: octokit/request-action@v2.0.2
+        with:
+          route: ${{ github.event.issue.pull_request.url }}
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+      - name: Checkout PR Branch
+        uses: actions/checkout@v3
+        with:
+          token: ${{ secrets.GITHUB_TOKEN }}
+          repository: ${{fromJson(steps.request.outputs.data).head.repo.full_name}}
+          ref: ${{fromJson(steps.request.outputs.data).head.ref}}
diff --git a/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected b/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected
index 67fcc5555d1..48b7b762605 100644
--- a/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected
+++ b/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected
@@ -3,6 +3,12 @@
 | .github/workflows/auto_ci.yml:93:9:96:6 | Uses Step | Unpinned 3rd party Action 'Python CI' step $@ uses 'codecov/codecov-action' with ref 'v3', not a pinned commit hash | .github/workflows/auto_ci.yml:93:9:96:6 | Uses Step | Uses Step |
 | .github/workflows/auto_ci.yml:108:9:119:6 | Uses Step: create_pr | Unpinned 3rd party Action 'Python CI' step $@ uses 'peter-evans/create-pull-request' with ref 'v5', not a pinned commit hash | .github/workflows/auto_ci.yml:108:9:119:6 | Uses Step: create_pr | Uses Step: create_pr |
 | .github/workflows/auto_ci.yml:125:9:133:6 | Uses Step | Unpinned 3rd party Action 'Python CI' step $@ uses 'thollander/actions-comment-pull-request' with ref 'v2', not a pinned commit hash | .github/workflows/auto_ci.yml:125:9:133:6 | Uses Step | Uses Step |
+| .github/workflows/issue_comment_3rd_party_action.yml:12:9:17:6 | Uses Step: comment-branch | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'xt0rted/pull-request-comment-branch' with ref 'v2', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:12:9:17:6 | Uses Step: comment-branch | Uses Step: comment-branch |
+| .github/workflows/issue_comment_3rd_party_action.yml:26:9:31:6 | Uses Step: comment-branch | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'xt0rted/pull-request-comment-branch' with ref 'v2', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:26:9:31:6 | Uses Step: comment-branch | Uses Step: comment-branch |
+| .github/workflows/issue_comment_3rd_party_action.yml:40:9:46:6 | Uses Step: refs | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'eficode/resolve-pr-refs' with ref 'main', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:40:9:46:6 | Uses Step: refs | Uses Step: refs |
+| .github/workflows/issue_comment_octokit.yml:12:9:19:6 | Uses Step: fetch_issue | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref 'v2.x', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:12:9:19:6 | Uses Step: fetch_issue | Uses Step: fetch_issue |
+| .github/workflows/issue_comment_octokit.yml:19:9:26:6 | Uses Step: fetch_pr | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref 'v2.x', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:19:9:26:6 | Uses Step: fetch_pr | Uses Step: fetch_pr |
+| .github/workflows/issue_comment_octokit.yml:103:9:109:6 | Uses Step: request | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref 'v2.0.2', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:103:9:109:6 | Uses Step: request | Uses Step: request |
 | .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'completely/fakeaction' with ref 'v2', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Uses Step |
 | .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'fakerepo/comment-on-pr' with ref 'v1', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Uses Step |
 | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Unpinned 3rd party Action 'unpinned_tags.yml' step $@ uses 'foo/bar' with ref 'v1', not a pinned commit hash | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Uses Step |
diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
index be1c7cbfebd..c7f4e4ad1c2 100644
--- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
+++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
@@ -1,4 +1,13 @@
 | .github/workflows/auto_ci.yml:20:9:27:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_3rd_party_action.yml:17:9:23:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_3rd_party_action.yml:31:9:37:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_3rd_party_action.yml:46:9:50:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_3rd_party_action.yml:50:9:53:25 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_direct.yml:12:9:16:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_direct.yml:20:9:24:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_direct.yml:28:9:32:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_direct.yml:35:9:40:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_direct.yml:43:9:46:126 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/untrusted_checkout.yml:9:7:13:4 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |

From 22d0600da8d8a58fba75ca358b7e439111af5441 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 13:28:39 +0100
Subject: [PATCH 16/17] Support more PR head checkouts

---
 ql/lib/codeql/actions/Ast.qll                            | 4 +++-
 ql/lib/codeql/actions/ast/internal/Ast.qll               | 2 ++
 ql/src/Security/CWE-829/UntrustedCheckout.ql             | 9 +++++++++
 .../Security/CWE-829/UntrustedCheckout.expected          | 8 ++++++++
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll
index 70424a46f95..4a7ff12b4f9 100644
--- a/ql/lib/codeql/actions/Ast.qll
+++ b/ql/lib/codeql/actions/Ast.qll
@@ -232,7 +232,9 @@ abstract class SimpleReferenceExpression extends AstNode instanceof SimpleRefere
   AstNode getTarget() { result = super.getTarget() }
 }
 
-class StepsExpression extends SimpleReferenceExpression instanceof StepsExpressionImpl { }
+class StepsExpression extends SimpleReferenceExpression instanceof StepsExpressionImpl {
+  string getStepId() { result = super.getStepId() }
+}
 
 class NeedsExpression extends SimpleReferenceExpression instanceof NeedsExpressionImpl { }
 
diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
index 084474b4020..5c6ce37fa92 100644
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -858,6 +858,8 @@ class StepsExpressionImpl extends SimpleReferenceExpressionImpl {
     this.getEnclosingJob() = result.getEnclosingJob() and
     result.(StepImpl).getId() = stepId
   }
+
+  string getStepId() { result = stepId }
 }
 
 /**
diff --git a/ql/src/Security/CWE-829/UntrustedCheckout.ql b/ql/src/Security/CWE-829/UntrustedCheckout.ql
index a24c80a2f60..f12f1102087 100644
--- a/ql/src/Security/CWE-829/UntrustedCheckout.ql
+++ b/ql/src/Security/CWE-829/UntrustedCheckout.ql
@@ -66,6 +66,7 @@ predicate containsHeadRef(string s) {
             "\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.head\\.sha\\b",
             "\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.id\\b",
             "\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.number\\b",
+            "\\bhead\\.sha\\b", "\\bhead\\.ref\\b"
           ], _, _)
   )
 }
@@ -80,6 +81,14 @@ class ActionsCheckout extends PRHeadCheckoutStep instanceof UsesStep {
     (
       containsHeadRef(this.getArgumentExpr("ref").getExpression())
       or
+      exists(StepsExpression e |
+        this.getArgumentExpr("ref") = e and
+        (
+          e.getStepId().matches(["%sha%", "%head%", "branch"]) or
+          e.getFieldName().matches(["%sha%", "%head%", "branch"])
+        )
+      )
+      or
       exists(UsesStep head |
         head.getCallee() = ["eficode/resolve-pr-refs", "xt0rted/pull-request-comment-branch"] and
         DataFlow::hasLocalFlowExpr(head, this.getArgumentExpr("ref"))
diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
index c7f4e4ad1c2..a6f02e7752a 100644
--- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
+++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
@@ -10,4 +10,12 @@
 | .github/workflows/issue_comment_direct.yml:28:9:32:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/issue_comment_direct.yml:35:9:40:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/issue_comment_direct.yml:43:9:46:126 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_heuristic.yml:28:9:33:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_heuristic.yml:48:7:50:46 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_octokit.yml:26:9:30:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_octokit.yml:30:9:35:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_octokit.yml:57:9:62:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_octokit.yml:79:9:83:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_octokit.yml:95:9:100:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_octokit.yml:109:9:114:66 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/untrusted_checkout.yml:9:7:13:4 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |

From 778d8978b05267a6b1eed1df7d5e03cb8ea9f8a7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 14 Mar 2024 13:55:10 +0100
Subject: [PATCH 17/17] DF support for untrusted checkout query

---
 .../dataflow/internal/DataFlowPublic.qll      |  1 +
 ql/src/Security/CWE-829/UntrustedCheckout.ql  | 18 ++++++++-----
 .../issue_comment_3rd_party_action.yml        |  1 -
 .../.github/workflows/untrusted_checkout.yml  | 26 ++++++-------------
 .../CWE-829/UnpinnedActionsTag.expected       |  8 +++---
 .../CWE-829/UntrustedCheckout.expected        | 11 ++++----
 6 files changed, 30 insertions(+), 35 deletions(-)

diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll
index 8e8ed5d9280..681d6f1cfc3 100644
--- a/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll
+++ b/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll
@@ -176,6 +176,7 @@ class FieldContent extends Content, TFieldContent {
 }
 
 predicate hasLocalFlow(Node n1, Node n2) {
+  n1 = n2 or
   simpleLocalFlowStep(n1, n2) or
   exists(ContentSet c | ctxFieldReadStep(n1, n2, c))
 }
diff --git a/ql/src/Security/CWE-829/UntrustedCheckout.ql b/ql/src/Security/CWE-829/UntrustedCheckout.ql
index f12f1102087..1be8a6ea0f5 100644
--- a/ql/src/Security/CWE-829/UntrustedCheckout.ql
+++ b/ql/src/Security/CWE-829/UntrustedCheckout.ql
@@ -79,8 +79,19 @@ class ActionsCheckout extends PRHeadCheckoutStep instanceof UsesStep {
   ActionsCheckout() {
     this.getCallee() = "actions/checkout" and
     (
-      containsHeadRef(this.getArgumentExpr("ref").getExpression())
+      // ref argument contains the head ref
+      exists(Expression e |
+        containsHeadRef(e.getExpression()) and
+        DataFlow::hasLocalFlowExpr(e, this.getArgumentExpr("ref"))
+      )
       or
+      // 3rd party actions returning the PR head sha/ref
+      exists(UsesStep head |
+        head.getCallee() = ["eficode/resolve-pr-refs", "xt0rted/pull-request-comment-branch"] and
+        DataFlow::hasLocalFlowExpr(head, this.getArgumentExpr("ref"))
+      )
+      or
+      // heuristic base on the step id and field name
       exists(StepsExpression e |
         this.getArgumentExpr("ref") = e and
         (
@@ -88,11 +99,6 @@ class ActionsCheckout extends PRHeadCheckoutStep instanceof UsesStep {
           e.getFieldName().matches(["%sha%", "%head%", "branch"])
         )
       )
-      or
-      exists(UsesStep head |
-        head.getCallee() = ["eficode/resolve-pr-refs", "xt0rted/pull-request-comment-branch"] and
-        DataFlow::hasLocalFlowExpr(head, this.getArgumentExpr("ref"))
-      )
     )
   }
 }
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_3rd_party_action.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_3rd_party_action.yml
index 4de47d6f17a..221854ec204 100644
--- a/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_3rd_party_action.yml
+++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/issue_comment_3rd_party_action.yml
@@ -13,7 +13,6 @@ jobs:
         if: ${{ github.event_name == 'issue_comment' }}
         uses: xt0rted/pull-request-comment-branch@v2
         id: comment-branch
-
       - name: (PR comment) Checkout PR branch
         if: ${{ github.event_name == 'issue_comment' }}
         uses: actions/checkout@v3
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout.yml
index a37ceb8f9f6..6bcdcbb4291 100644
--- a/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout.yml
+++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout.yml
@@ -3,23 +3,13 @@ on:
 
 jobs:
   build:
-    name: Build and test
     runs-on: ubuntu-latest
+    env:
+      HEAD: ${{ github.event.pull_request.head.sha }}
     steps:
-    - uses: actions/checkout@v2
-      with:
-        ref: ${{ github.event.pull_request.head.sha }}
-
-    - uses: actions/setup-node@v1
-    - run: |
-        npm install
-        npm build
-
-    - uses: completely/fakeaction@v2
-      with:
-        arg1: ${{ secrets.supersecret }}
-
-    - uses: fakerepo/comment-on-pr@v1
-      with:
-        message: |
-          Thank you!
+      - uses: actions/checkout@v2
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+      - uses: actions/checkout@v2
+        with:
+          ref: ${{ env.HEAD }}
diff --git a/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected b/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected
index 48b7b762605..c3a3ec2f988 100644
--- a/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected
+++ b/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected
@@ -3,14 +3,12 @@
 | .github/workflows/auto_ci.yml:93:9:96:6 | Uses Step | Unpinned 3rd party Action 'Python CI' step $@ uses 'codecov/codecov-action' with ref 'v3', not a pinned commit hash | .github/workflows/auto_ci.yml:93:9:96:6 | Uses Step | Uses Step |
 | .github/workflows/auto_ci.yml:108:9:119:6 | Uses Step: create_pr | Unpinned 3rd party Action 'Python CI' step $@ uses 'peter-evans/create-pull-request' with ref 'v5', not a pinned commit hash | .github/workflows/auto_ci.yml:108:9:119:6 | Uses Step: create_pr | Uses Step: create_pr |
 | .github/workflows/auto_ci.yml:125:9:133:6 | Uses Step | Unpinned 3rd party Action 'Python CI' step $@ uses 'thollander/actions-comment-pull-request' with ref 'v2', not a pinned commit hash | .github/workflows/auto_ci.yml:125:9:133:6 | Uses Step | Uses Step |
-| .github/workflows/issue_comment_3rd_party_action.yml:12:9:17:6 | Uses Step: comment-branch | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'xt0rted/pull-request-comment-branch' with ref 'v2', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:12:9:17:6 | Uses Step: comment-branch | Uses Step: comment-branch |
-| .github/workflows/issue_comment_3rd_party_action.yml:26:9:31:6 | Uses Step: comment-branch | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'xt0rted/pull-request-comment-branch' with ref 'v2', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:26:9:31:6 | Uses Step: comment-branch | Uses Step: comment-branch |
-| .github/workflows/issue_comment_3rd_party_action.yml:40:9:46:6 | Uses Step: refs | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'eficode/resolve-pr-refs' with ref 'main', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:40:9:46:6 | Uses Step: refs | Uses Step: refs |
+| .github/workflows/issue_comment_3rd_party_action.yml:12:9:16:6 | Uses Step: comment-branch | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'xt0rted/pull-request-comment-branch' with ref 'v2', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:12:9:16:6 | Uses Step: comment-branch | Uses Step: comment-branch |
+| .github/workflows/issue_comment_3rd_party_action.yml:25:9:30:6 | Uses Step: comment-branch | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'xt0rted/pull-request-comment-branch' with ref 'v2', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:25:9:30:6 | Uses Step: comment-branch | Uses Step: comment-branch |
+| .github/workflows/issue_comment_3rd_party_action.yml:39:9:45:6 | Uses Step: refs | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'eficode/resolve-pr-refs' with ref 'main', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:39:9:45:6 | Uses Step: refs | Uses Step: refs |
 | .github/workflows/issue_comment_octokit.yml:12:9:19:6 | Uses Step: fetch_issue | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref 'v2.x', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:12:9:19:6 | Uses Step: fetch_issue | Uses Step: fetch_issue |
 | .github/workflows/issue_comment_octokit.yml:19:9:26:6 | Uses Step: fetch_pr | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref 'v2.x', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:19:9:26:6 | Uses Step: fetch_pr | Uses Step: fetch_pr |
 | .github/workflows/issue_comment_octokit.yml:103:9:109:6 | Uses Step: request | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref 'v2.0.2', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:103:9:109:6 | Uses Step: request | Uses Step: request |
 | .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'completely/fakeaction' with ref 'v2', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Uses Step |
 | .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'fakerepo/comment-on-pr' with ref 'v1', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Uses Step |
 | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Unpinned 3rd party Action 'unpinned_tags.yml' step $@ uses 'foo/bar' with ref 'v1', not a pinned commit hash | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Uses Step |
-| .github/workflows/untrusted_checkout.yml:18:7:22:4 | Uses Step | Unpinned 3rd party Action 'untrusted_checkout.yml' step $@ uses 'completely/fakeaction' with ref 'v2', not a pinned commit hash | .github/workflows/untrusted_checkout.yml:18:7:22:4 | Uses Step | Uses Step |
-| .github/workflows/untrusted_checkout.yml:22:7:25:21 | Uses Step | Unpinned 3rd party Action 'untrusted_checkout.yml' step $@ uses 'fakerepo/comment-on-pr' with ref 'v1', not a pinned commit hash | .github/workflows/untrusted_checkout.yml:22:7:25:21 | Uses Step | Uses Step |
diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
index a6f02e7752a..cf9d6c01d49 100644
--- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
+++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckout.expected
@@ -1,10 +1,10 @@
 | .github/workflows/auto_ci.yml:20:9:27:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
-| .github/workflows/issue_comment_3rd_party_action.yml:17:9:23:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
-| .github/workflows/issue_comment_3rd_party_action.yml:31:9:37:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
-| .github/workflows/issue_comment_3rd_party_action.yml:46:9:50:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
-| .github/workflows/issue_comment_3rd_party_action.yml:50:9:53:25 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_3rd_party_action.yml:16:9:22:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_3rd_party_action.yml:30:9:36:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_3rd_party_action.yml:45:9:49:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/issue_comment_3rd_party_action.yml:49:9:52:25 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/issue_comment_direct.yml:12:9:16:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/issue_comment_direct.yml:20:9:24:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/issue_comment_direct.yml:28:9:32:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
@@ -18,4 +18,5 @@
 | .github/workflows/issue_comment_octokit.yml:79:9:83:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/issue_comment_octokit.yml:95:9:100:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/issue_comment_octokit.yml:109:9:114:66 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
-| .github/workflows/untrusted_checkout.yml:9:7:13:4 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/untrusted_checkout.yml:10:9:13:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/untrusted_checkout.yml:13:9:15:31 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |