Merge pull request #22391 from computersarebad/association-check-event-validity

Actions: only count association checks for events that populate the checked field

Author: jketema

actions/ql/lib/change-notes/2026-08-19-association-if-check-event-validity.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Checks on author association fields read from the event payload (e.g. `github.event.pull_request.author_association`) now only count as protection for events whose payload actually populates that field. Previously, a condition such as `github.event.pull_request.author_association != 'NONE'` on a workflow triggered by `issues` events was treated as a protective check even though `github.event.pull_request` is not populated for `issues` events, which makes the condition vacuous. This change may result in more alerts for queries using the `ControlCheck` class.

actions/ql/lib/codeql/actions/security/ControlChecks.qll

@@ -408,16 +408,37 @@ class WorkflowRunRepositoryIfCheck extends RepositoryCheck instanceof If {
   }
 }
 
+/**
+ * Gets a regular expression matching a condition on an author association field
+ * that is only populated for events whose payload contains the `context_prefix`
+ * context.
+ */
+private string eventPayloadAssociationFieldRegex(string context_prefix) {
+  context_prefix = "github.event.comment" and
+  result = "\\bgithub\\.event\\.comment\\.author_association\\b"
+  or
+  context_prefix = "github.event.issue" and
+  result = "\\bgithub\\.event\\.issue\\.author_association\\b"
+  or
+  context_prefix = "github.event.pull_request" and
+  result = "\\bgithub\\.event\\.pull_request\\.author_association\\b"
+}
+
 class AssociationIfCheck extends AssociationCheck instanceof If {
+  string context_prefix;
+
   AssociationIfCheck() {
     // eg: contains(fromJson('["MEMBER", "OWNER"]'), github.event.comment.author_association)
-    normalizeExpr(this.getCondition())
-        .splitAt("\n")
-        .regexpMatch([
-            ".*\\bgithub\\.event\\.comment\\.author_association\\b.*",
-            ".*\\bgithub\\.event\\.issue\\.author_association\\b.*",
-            ".*\\bgithub\\.event\\.pull_request\\.author_association\\b.*",
-          ])
+    exists(
+      normalizeExpr(this.getCondition())
+          .regexpFind(eventPayloadAssociationFieldRegex(context_prefix), _, _)
+    )
+  }
+
+  override predicate protectsCategoryAndEvent(string category, string event) {
+    AssociationCheck.super.protectsCategoryAndEvent(category, event) and
+    // association fields only restrict events whose payload populates them
+    contextTriggerDataModel(event, context_prefix)
   }
 }
 

actions/ql/test/query-tests/Security/CWE-094/.github/workflows/association_check_wrong_event.yml

@@ -0,0 +1,21 @@
+on:
+  issues:
+    types: [opened]
+
+jobs:
+  # The `if:` condition compares an association field that is never populated
+  # for `issues` events, so it is always true and does not protect the
+  # injectable step.
+  vacuous-association-check:
+    runs-on: ubuntu-latest
+    if: github.event.pull_request.author_association != 'NONE'
+    steps:
+      - run: echo '${{ github.event.issue.title }}'
+
+  # `github.event.issue` is populated for `issues` events, so this check is
+  # effective and the injectable step is protected.
+  valid-association-check:
+    runs-on: ubuntu-latest
+    if: github.event.issue.author_association == 'MEMBER'
+    steps:
+      - run: echo '${{ github.event.issue.title }}'

actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected

@@ -312,6 +312,8 @@ nodes
 | .github/workflows/artifactpoisoning8.yml:17:9:21:6 | Run Step: artifact [id] | semmle.label | Run Step: artifact [id] |
 | .github/workflows/artifactpoisoning8.yml:19:14:19:58 | echo "::set-output name=id::$(<artifact.txt)" | semmle.label | echo "::set-output name=id::$(<artifact.txt)" |
 | .github/workflows/artifactpoisoning8.yml:22:20:22:51 | steps.artifact.outputs.id | semmle.label | steps.artifact.outputs.id |
+| .github/workflows/association_check_wrong_event.yml:13:21:13:51 | github.event.issue.title | semmle.label | github.event.issue.title |
+| .github/workflows/association_check_wrong_event.yml:21:21:21:51 | github.event.issue.title | semmle.label | github.event.issue.title |
 | .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | semmle.label | Uses Step: changed-files1 |
 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | semmle.label | steps.changed-files1.outputs.all_changed_files |
 | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | semmle.label | Uses Step: changed-files3 |
@@ -729,6 +731,7 @@ subpaths
 | .github/workflows/artifactpoisoning6.yml:29:20:29:59 | steps.artifact2.outputs.pr_number | .github/workflows/artifactpoisoning6.yml:8:9:15:6 | Uses Step | .github/workflows/artifactpoisoning6.yml:29:20:29:59 | steps.artifact2.outputs.pr_number | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning6.yml:29:20:29:59 | steps.artifact2.outputs.pr_number | ${{ steps.artifact2.outputs.pr_number }} | .github/workflows/artifactpoisoning6.yml:3:5:3:16 | workflow_run | workflow_run |
 | .github/workflows/artifactpoisoning7.yml:30:20:30:58 | steps.artifact.outputs.pr_number | .github/workflows/artifactpoisoning7.yml:8:9:15:6 | Uses Step | .github/workflows/artifactpoisoning7.yml:30:20:30:58 | steps.artifact.outputs.pr_number | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning7.yml:30:20:30:58 | steps.artifact.outputs.pr_number | ${{ steps.artifact.outputs.pr_number }} | .github/workflows/artifactpoisoning7.yml:3:5:3:16 | workflow_run | workflow_run |
 | .github/workflows/artifactpoisoning8.yml:22:20:22:51 | steps.artifact.outputs.id | .github/workflows/artifactpoisoning8.yml:9:9:17:6 | Uses Step | .github/workflows/artifactpoisoning8.yml:22:20:22:51 | steps.artifact.outputs.id | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning8.yml:22:20:22:51 | steps.artifact.outputs.id | ${{ steps.artifact.outputs.id }} | .github/workflows/artifactpoisoning8.yml:4:5:4:16 | workflow_run | workflow_run |
+| .github/workflows/association_check_wrong_event.yml:13:21:13:51 | github.event.issue.title | .github/workflows/association_check_wrong_event.yml:13:21:13:51 | github.event.issue.title | .github/workflows/association_check_wrong_event.yml:13:21:13:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/association_check_wrong_event.yml:13:21:13:51 | github.event.issue.title | ${{ github.event.issue.title }} | .github/workflows/association_check_wrong_event.yml:2:3:2:8 | issues | issues |
 | .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 code 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:1:5:1:17 | issue_comment | issue_comment |
 | .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 code 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:1:5:1:17 | issue_comment | issue_comment |
 | .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 code 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 }} | .github/workflows/comment_issue.yml:1:5:1:17 | issue_comment | issue_comment |

actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected

@@ -312,6 +312,8 @@ nodes
 | .github/workflows/artifactpoisoning8.yml:17:9:21:6 | Run Step: artifact [id] | semmle.label | Run Step: artifact [id] |
 | .github/workflows/artifactpoisoning8.yml:19:14:19:58 | echo "::set-output name=id::$(<artifact.txt)" | semmle.label | echo "::set-output name=id::$(<artifact.txt)" |
 | .github/workflows/artifactpoisoning8.yml:22:20:22:51 | steps.artifact.outputs.id | semmle.label | steps.artifact.outputs.id |
+| .github/workflows/association_check_wrong_event.yml:13:21:13:51 | github.event.issue.title | semmle.label | github.event.issue.title |
+| .github/workflows/association_check_wrong_event.yml:21:21:21:51 | github.event.issue.title | semmle.label | github.event.issue.title |
 | .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | semmle.label | Uses Step: changed-files1 |
 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | semmle.label | steps.changed-files1.outputs.all_changed_files |
 | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | semmle.label | Uses Step: changed-files3 |
@@ -718,6 +720,7 @@ subpaths
 | .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | ${{ github.event.pull_request.title }} |
 | .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | ${{ github.event.issue.title }} |
 | .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | ${{ github.event.issue.title }} |
+| .github/workflows/association_check_wrong_event.yml:21:21:21:51 | github.event.issue.title | .github/workflows/association_check_wrong_event.yml:21:21:21:51 | github.event.issue.title | .github/workflows/association_check_wrong_event.yml:21:21:21:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/association_check_wrong_event.yml:21:21:21:51 | github.event.issue.title | ${{ github.event.issue.title }} |
 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | ${{ steps.changed-files1.outputs.all_changed_files }} |
 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | ${{ steps.changed-files3.outputs.all_changed_files }} |
 | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | .github/workflows/changed-files.yml:53:9:56:6 | Uses Step: changed-files5 | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | ${{ steps.changed-files5.outputs.all_changed_files }} |

actions/ql/test/query-tests/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.expected

@@ -104,8 +104,3 @@ edges
 | .github/workflows/test0.yml:58:9:60:2 | Run Step | .github/workflows/test0.yml:54:9:58:6 | Uses Step | .github/workflows/test0.yml:58:9:60:2 | Run Step | Insufficient protection against execution of untrusted code on a privileged workflow ($@). | .github/workflows/test0.yml:4:3:4:15 | issue_comment | issue_comment |
 | .github/workflows/test0.yml:68:9:68:43 | Run Step | .github/workflows/test0.yml:64:9:68:6 | Uses Step | .github/workflows/test0.yml:68:9:68:43 | Run Step | Insufficient protection against execution of untrusted code on a privileged workflow ($@). | .github/workflows/test0.yml:4:3:4:15 | issue_comment | issue_comment |
 | .github/workflows/test4.yml:85:7:88:54 | Uses Step | .github/workflows/test4.yml:79:7:85:4 | Uses Step | .github/workflows/test4.yml:85:7:88:54 | Uses Step | Insufficient protection against execution of untrusted code on a privileged workflow ($@). | .github/workflows/test4.yml:5:3:5:15 | issue_comment | issue_comment |
-| .github/workflows/test5.yml:151:7:156:4 | Uses Step | .github/workflows/test5.yml:87:7:93:4 | Uses Step | .github/workflows/test5.yml:151:7:156:4 | Uses Step | Insufficient protection against execution of untrusted code on a privileged workflow ($@). | .github/workflows/test5.yml:5:3:5:15 | issue_comment | issue_comment |
-| .github/workflows/test5.yml:156:7:169:4 | Run Step | .github/workflows/test5.yml:87:7:93:4 | Uses Step | .github/workflows/test5.yml:156:7:169:4 | Run Step | Insufficient protection against execution of untrusted code on a privileged workflow ($@). | .github/workflows/test5.yml:5:3:5:15 | issue_comment | issue_comment |
-| .github/workflows/test5.yml:169:7:180:4 | Run Step | .github/workflows/test5.yml:87:7:93:4 | Uses Step | .github/workflows/test5.yml:169:7:180:4 | Run Step | Insufficient protection against execution of untrusted code on a privileged workflow ($@). | .github/workflows/test5.yml:5:3:5:15 | issue_comment | issue_comment |
-| .github/workflows/test6.yml:213:7:218:4 | Uses Step | .github/workflows/test6.yml:162:7:167:4 | Uses Step | .github/workflows/test6.yml:213:7:218:4 | Uses Step | Insufficient protection against execution of untrusted code on a privileged workflow ($@). | .github/workflows/test6.yml:5:3:5:15 | issue_comment | issue_comment |
-| .github/workflows/test6.yml:218:7:224:4 | Run Step | .github/workflows/test6.yml:162:7:167:4 | Uses Step | .github/workflows/test6.yml:218:7:224:4 | Run Step | Insufficient protection against execution of untrusted code on a privileged workflow ($@). | .github/workflows/test6.yml:5:3:5:15 | issue_comment | issue_comment |

actions/ql/test/query-tests/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.expected

@@ -1,2 +0,0 @@
-| .github/workflows/test6.yml:42:7:47:4 | Uses Step | Insufficient protection against execution of untrusted code on a privileged workflow ($@). | .github/workflows/test6.yml:5:3:5:15 | issue_comment | issue_comment |
-| .github/workflows/test6.yml:92:7:97:4 | Uses Step | Insufficient protection against execution of untrusted code on a privileged workflow ($@). | .github/workflows/test6.yml:5:3:5:15 | issue_comment | issue_comment |