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::$(