From 1a430070f83f17353e929a9d383a8df4b3624344 Mon Sep 17 00:00:00 2001 From: Jordan Pike Date: Wed, 19 Aug 2026 14:53:30 -0400 Subject: [PATCH] Actions: AssociationIfCheck should not protect events that do not populate the checked field Same problem as EventActorIfCheck had. A condition like 'github.event.pull_request.author_association != NONE' on a workflow triggered by issues events is always true since github.event.pull_request is not populated there, but it still counted as a protective check for those events. Bind the matched context prefix in the characteristic predicate and require contextTriggerDataModel to populate it for the event, following the pattern from #22368. Existing TOCTOU alerts on vacuous gates migrate to the untrusted-checkout critical query, since a vacuous gate is no protection rather than raceable protection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4084febb-f9c7-44df-baf3-c8be8e9932a7 --- ...-19-association-if-check-event-validity.md | 4 +++ .../codeql/actions/security/ControlChecks.qll | 35 +++++++++++++++---- .../association_check_wrong_event.yml | 21 +++++++++++ .../CWE-094/CodeInjectionCritical.expected | 3 ++ .../CWE-094/CodeInjectionMedium.expected | 3 ++ .../UntrustedCheckoutTOCTOUCritical.expected | 5 --- .../UntrustedCheckoutTOCTOUHigh.expected | 2 -- .../UntrustedCheckoutCritical.expected | 5 +++ 8 files changed, 64 insertions(+), 14 deletions(-) create mode 100644 actions/ql/lib/change-notes/2026-08-19-association-if-check-event-validity.md create mode 100644 actions/ql/test/query-tests/Security/CWE-094/.github/workflows/association_check_wrong_event.yml diff --git a/actions/ql/lib/change-notes/2026-08-19-association-if-check-event-validity.md b/actions/ql/lib/change-notes/2026-08-19-association-if-check-event-validity.md new file mode 100644 index 000000000000..28ef240b2394 --- /dev/null +++ b/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. diff --git a/actions/ql/lib/codeql/actions/security/ControlChecks.qll b/actions/ql/lib/codeql/actions/security/ControlChecks.qll index 675c1d18852b..aea57fdc4b71 100644 --- a/actions/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/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) } } diff --git a/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/association_check_wrong_event.yml b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/association_check_wrong_event.yml new file mode 100644 index 000000000000..b1cef25955dd --- /dev/null +++ b/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 }}' diff --git a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected index 14e50942d734..e32155dffe93 100644 --- a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected +++ b/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::$(