From 9251d21e24fe3f201a71f975430b102946f6fe19 Mon Sep 17 00:00:00 2001 From: Jordan Pike Date: Mon, 17 Aug 2026 19:44:12 -0400 Subject: [PATCH 1/4] Actions: ActorIfCheck should not protect events that do not populate the checked field A condition like 'github.event.pull_request.user.login != ...' on a workflow triggered by issues events is always true since github.event.pull_request is not populated for issues events, but ActorIfCheck still treated it as a protective check, suppressing alerts such as actions/code-injection/critical. Override protectsCategoryAndEvent in ActorIfCheck so that checks on event payload fields only protect events whose payload contains the corresponding context, using contextTriggerDataModel as the mapping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4084febb-f9c7-44df-baf3-c8be8e9932a7 --- ...026-08-17-actor-if-check-event-validity.md | 4 ++ .../codeql/actions/security/ControlChecks.qll | 54 ++++++++++++++++--- .../workflows/actor_check_valid_event.yml | 12 +++++ .../workflows/actor_check_wrong_event.yml | 20 +++++++ .../CWE-094/CodeInjectionCritical.expected | 4 ++ .../CWE-094/CodeInjectionMedium.expected | 5 ++ 6 files changed, 93 insertions(+), 6 deletions(-) create mode 100644 actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md create mode 100644 actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_valid_event.yml create mode 100644 actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml diff --git a/actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md b/actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md new file mode 100644 index 000000000000..18d10595a756 --- /dev/null +++ b/actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Altered the logic of `ActorIfCheck` so that checks on actor fields read from the event payload (e.g. `github.event.pull_request.user.login`) only count as protection for events whose payload actually populates that field. Previously, a condition such as `github.event.pull_request.user.login != 'name'` 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 will result in more results being found by the queries that rely on control checks, such as `actions/code-injection/critical`. diff --git a/actions/ql/lib/codeql/actions/security/ControlChecks.qll b/actions/ql/lib/codeql/actions/security/ControlChecks.qll index 2228e9f96e21..a1058610871c 100644 --- a/actions/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/actions/ql/lib/codeql/actions/security/ControlChecks.qll @@ -312,17 +312,28 @@ class LabelIfCheck extends LabelCheck instanceof If { } } +/** + * Gets a regular expression matching a condition on an actor field that is + * only populated for events whose payload contains the `context_prefix` context. + */ +private string eventPayloadActorFieldRegex(string context_prefix) { + context_prefix = "github.event.pull_request" and + result = "\\bgithub\\.event\\.pull_request\\.user\\.login\\b" + or + context_prefix = "github.event.head_commit" and + result = "\\bgithub\\.event\\.head_commit\\.author\\.name\\b" + or + context_prefix = "github.event.commits" and + result = "\\bgithub\\.event\\.commits.*\\.author\\.name\\b" +} + class ActorIfCheck extends ActorCheck instanceof If { ActorIfCheck() { // eg: github.event.pull_request.user.login == 'admin' exists( normalizeExpr(this.getCondition()) - .regexpFind([ - "\\bgithub\\.event\\.pull_request\\.user\\.login\\b", - "\\bgithub\\.event\\.head_commit\\.author\\.name\\b", - "\\bgithub\\.event\\.commits.*\\.author\\.name\\b", - "\\bgithub\\.event\\.sender\\.login\\b" - ], _, _) + .regexpFind([eventPayloadActorFieldRegex(_), "\\bgithub\\.event\\.sender\\.login\\b"], _, + _) ) or // eg: github.actor == 'admin' @@ -333,6 +344,37 @@ class ActorIfCheck extends ActorCheck instanceof If { ) and not normalizeExpr(this.getCondition()).matches("%[bot]%") } + + override predicate protectsCategoryAndEvent(string category, string event) { + ActorCheck.super.protectsCategoryAndEvent(category, event) and + ( + // `github.actor`, `github.triggering_actor` and `github.event.sender.login` + // are populated for every event + exists( + normalizeExpr(this.(If).getCondition()) + .regexpFind("\\bgithub\\.event\\.sender\\.login\\b", _, _) + ) + or + exists( + normalizeExpr(this.(If).getCondition()) + .regexpFind(["\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b",], _, _) + ) and + not normalizeExpr(this.(If).getCondition()).matches("%[bot]%") + or + // actor fields read from the event payload are only populated for events + // whose payload contains the corresponding context. eg: a check on + // `github.event.pull_request.user.login` cannot restrict the actor of an + // `issues` event since `github.event.pull_request` is not populated there, + // which makes the condition vacuous + exists(string context_prefix | + contextTriggerDataModel(event, context_prefix) and + exists( + normalizeExpr(this.(If).getCondition()) + .regexpFind(eventPayloadActorFieldRegex(context_prefix), _, _) + ) + ) + ) + } } class PullRequestTargetRepositoryIfCheck extends RepositoryCheck instanceof If { diff --git a/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_valid_event.yml b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_valid_event.yml new file mode 100644 index 000000000000..cd139700f078 --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_valid_event.yml @@ -0,0 +1,12 @@ +on: + pull_request_target: + types: [opened] + +jobs: + # The `if:` condition checks an actor field that is populated for + # `pull_request_target` events, so the injectable step is protected. + valid-actor-check: + runs-on: ubuntu-latest + if: github.event.pull_request.user.login == 'trusted-user' + steps: + - run: echo '${{ github.event.pull_request.title }}' diff --git a/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml new file mode 100644 index 000000000000..fce829d4a9b4 --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml @@ -0,0 +1,20 @@ +on: + issues: + types: [opened] + +jobs: + # The `if:` condition compares an actor field that is never populated for + # `issues` events, so it is always true and does not protect the injectable step. + vacuous-actor-check: + runs-on: ubuntu-latest + if: github.event.pull_request.user.login != 'some-bot[bot]' + steps: + - run: echo '${{ github.event.issue.title }}' + + # The `if:` condition checks an actor context that is populated for every + # event, so the injectable step is protected. + valid-actor-check: + runs-on: ubuntu-latest + if: github.actor == 'trusted-user' + 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 02f7f68c05f4..d58f13b44d7f 100644 --- a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected +++ b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected @@ -270,6 +270,9 @@ nodes | .github/actions/external/ultralytics/actions/action.yaml:66:3:66:6 | input body | semmle.label | input body | | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | semmle.label | inputs.body | | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | semmle.label | github.head_ref \|\| github.ref | +| .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | +| .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] | | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE | @@ -710,6 +713,7 @@ subpaths | .github/actions/external/TestOrg/TestRepo/.github/actions/clone-repo/action.yaml:22:19:22:37 | inputs.title | .github/workflows/composite-action-caller-4.yml:14:19:14:56 | github.event.pull_request.title | .github/actions/external/TestOrg/TestRepo/.github/actions/clone-repo/action.yaml:22:19:22:37 | inputs.title | Potential code injection in $@, which may be controlled by an external user ($@). | .github/actions/external/TestOrg/TestRepo/.github/actions/clone-repo/action.yaml:22:19:22:37 | inputs.title | ${{ inputs.title }} | .github/workflows/composite-action-caller-4.yml:4:3:4:21 | pull_request_target | pull_request_target | | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | .github/workflows/test29.yml:35:18:35:54 | github.event.pull_request.body | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | Potential code injection in $@, which may be controlled by an external user ($@). | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | ${{ inputs.body }} | .github/workflows/test29.yml:12:3:12:21 | pull_request_target | pull_request_target | | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | Potential code injection in $@, which may be controlled by an external user ($@). | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | ${{ github.head_ref \|\| github.ref }} | .github/workflows/test29.yml:12:3:12:21 | pull_request_target | pull_request_target | +| .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | ${{ github.event.issue.title }} | .github/workflows/actor_check_wrong_event.yml:2:3:2:8 | issues | issues | | .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 code 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/argus_case_study.yml:4:3:4:8 | issues | issues | | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | .github/workflows/artifactpoisoning1.yml:14:9:20:6 | Uses Step | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | ${{ steps.pr.outputs.id }} | .github/workflows/artifactpoisoning1.yml:4:3:4:14 | workflow_run | workflow_run | | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | .github/workflows/artifactpoisoning2.yml:13:9:19:6 | Uses Step: pr | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | ${{ steps.pr.outputs.id }} | .github/workflows/artifactpoisoning2.yml:4:3:4:14 | workflow_run | workflow_run | diff --git a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected index 231d41bc2518..9833fd104e96 100644 --- a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected +++ b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected @@ -270,6 +270,9 @@ nodes | .github/actions/external/ultralytics/actions/action.yaml:66:3:66:6 | input body | semmle.label | input body | | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | semmle.label | inputs.body | | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | semmle.label | github.head_ref \|\| github.ref | +| .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | +| .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] | | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE | @@ -709,6 +712,8 @@ subpaths | .github/actions/action7/action.yml:214:41:214:69 | inputs.github_username | .github/actions/action7/action.yml:214:41:214:69 | inputs.github_username | .github/actions/action7/action.yml:214:41:214:69 | inputs.github_username | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action7/action.yml:214:41:214:69 | inputs.github_username | ${{ inputs.github_username }} | | .github/actions/action7/action.yml:215:41:215:66 | inputs.github_email | .github/actions/action7/action.yml:215:41:215:66 | inputs.github_email | .github/actions/action7/action.yml:215:41:215:66 | inputs.github_email | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action7/action.yml:215:41:215:66 | inputs.github_email | ${{ inputs.github_email }} | | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | ${{ github.head_ref \|\| github.ref }} | +| .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/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 }} | From 3048f4788ed8339e238669c2aa85ab2324100cb2 Mon Sep 17 00:00:00 2001 From: Jordan Pike Date: Tue, 18 Aug 2026 10:13:43 -0400 Subject: [PATCH 2/4] Split payload-field actor checks into EventActorIfCheck Per review, ActorIfCheck now only covers github.actor and github.triggering_actor, which are populated for every event and need no event-validity override. Checks on actor fields read from the event payload move to the new EventActorIfCheck class, which only protects events whose payload populates the checked field. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4084febb-f9c7-44df-baf3-c8be8e9932a7 --- ...026-08-17-actor-if-check-event-validity.md | 2 +- .../codeql/actions/security/ControlChecks.qll | 39 ++++++++++--------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md b/actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md index 18d10595a756..4397e9487415 100644 --- a/actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md +++ b/actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Altered the logic of `ActorIfCheck` so that checks on actor fields read from the event payload (e.g. `github.event.pull_request.user.login`) only count as protection for events whose payload actually populates that field. Previously, a condition such as `github.event.pull_request.user.login != 'name'` 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 will result in more results being found by the queries that rely on control checks, such as `actions/code-injection/critical`. +* Checks on actor fields read from the event payload (e.g. `github.event.pull_request.user.login`) now only count as protection for events whose payload actually populates that field. These checks were split out of `ActorIfCheck` into a new class `EventActorIfCheck`, and `ActorIfCheck` now only covers `github.actor` and `github.triggering_actor`. Previously, a condition such as `github.event.pull_request.user.login != 'name'` 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 will result in more results being found by the queries that rely on control checks, such as `actions/code-injection/critical`. diff --git a/actions/ql/lib/codeql/actions/security/ControlChecks.qll b/actions/ql/lib/codeql/actions/security/ControlChecks.qll index a1058610871c..a9fd20c12565 100644 --- a/actions/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/actions/ql/lib/codeql/actions/security/ControlChecks.qll @@ -329,13 +329,6 @@ private string eventPayloadActorFieldRegex(string context_prefix) { class ActorIfCheck extends ActorCheck instanceof If { ActorIfCheck() { - // eg: github.event.pull_request.user.login == 'admin' - exists( - normalizeExpr(this.getCondition()) - .regexpFind([eventPayloadActorFieldRegex(_), "\\bgithub\\.event\\.sender\\.login\\b"], _, - _) - ) - or // eg: github.actor == 'admin' // eg: github.triggering_actor == 'admin' exists( @@ -344,28 +337,38 @@ class ActorIfCheck extends ActorCheck instanceof If { ) and not normalizeExpr(this.getCondition()).matches("%[bot]%") } +} + +/** An If node that checks an actor field from the event payload */ +class EventActorIfCheck extends ActorCheck instanceof If { + EventActorIfCheck() { + // eg: github.event.pull_request.user.login == 'admin' + exists( + normalizeExpr(this.getCondition()) + .regexpFind([eventPayloadActorFieldRegex(_), "\\bgithub\\.event\\.sender\\.login\\b"], _, + _) + ) + } override predicate protectsCategoryAndEvent(string category, string event) { ActorCheck.super.protectsCategoryAndEvent(category, event) and ( - // `github.actor`, `github.triggering_actor` and `github.event.sender.login` - // are populated for every event + // the `sender` object is part of every webhook event payload, + // so `github.event.sender.login` is populated for every event exists( normalizeExpr(this.(If).getCondition()) .regexpFind("\\bgithub\\.event\\.sender\\.login\\b", _, _) ) or - exists( - normalizeExpr(this.(If).getCondition()) - .regexpFind(["\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b",], _, _) - ) and - not normalizeExpr(this.(If).getCondition()).matches("%[bot]%") - or - // actor fields read from the event payload are only populated for events - // whose payload contains the corresponding context. eg: a check on + // other actor fields are only populated for events whose payload contains + // the corresponding context. eg: a check on // `github.event.pull_request.user.login` cannot restrict the actor of an // `issues` event since `github.event.pull_request` is not populated there, - // which makes the condition vacuous + // which makes the condition vacuous. + // note that `github.event.head_commit` and `github.event.commits` are only + // populated for `push` events, which are not protectable by ActorCheck, so + // checks on those fields never count as protection here. they are still + // matched so that these Ifs keep being classified as actor checks exists(string context_prefix | contextTriggerDataModel(event, context_prefix) and exists( From e9d9d3a007b115258255d83210505a8a28dbcf4d Mon Sep 17 00:00:00 2001 From: Jordan Pike Date: Tue, 18 Aug 2026 15:00:09 -0400 Subject: [PATCH 3/4] Simplify EventActorIfCheck by binding context_prefix as a field Per review, fold the sender case into eventPayloadActorFieldRegex and keep the matched context_prefix on the class instead of re-matching in the override. Also move the helper next to the class that uses it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4084febb-f9c7-44df-baf3-c8be8e9932a7 --- .../codeql/actions/security/ControlChecks.qll | 61 ++++++++----------- 1 file changed, 25 insertions(+), 36 deletions(-) diff --git a/actions/ql/lib/codeql/actions/security/ControlChecks.qll b/actions/ql/lib/codeql/actions/security/ControlChecks.qll index a9fd20c12565..675c1d18852b 100644 --- a/actions/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/actions/ql/lib/codeql/actions/security/ControlChecks.qll @@ -312,6 +312,18 @@ class LabelIfCheck extends LabelCheck instanceof If { } } +class ActorIfCheck extends ActorCheck instanceof If { + ActorIfCheck() { + // eg: github.actor == 'admin' + // eg: github.triggering_actor == 'admin' + exists( + normalizeExpr(this.getCondition()) + .regexpFind(["\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b",], _, _) + ) and + not normalizeExpr(this.getCondition()).matches("%[bot]%") + } +} + /** * Gets a regular expression matching a condition on an actor field that is * only populated for events whose payload contains the `context_prefix` context. @@ -325,57 +337,34 @@ private string eventPayloadActorFieldRegex(string context_prefix) { or context_prefix = "github.event.commits" and result = "\\bgithub\\.event\\.commits.*\\.author\\.name\\b" -} - -class ActorIfCheck extends ActorCheck instanceof If { - ActorIfCheck() { - // eg: github.actor == 'admin' - // eg: github.triggering_actor == 'admin' - exists( - normalizeExpr(this.getCondition()) - .regexpFind(["\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b",], _, _) - ) and - not normalizeExpr(this.getCondition()).matches("%[bot]%") - } + or + context_prefix = "github.event.sender" and + result = "\\bgithub\\.event\\.sender\\.login\\b" } /** An If node that checks an actor field from the event payload */ class EventActorIfCheck extends ActorCheck instanceof If { + string context_prefix; + EventActorIfCheck() { // eg: github.event.pull_request.user.login == 'admin' exists( normalizeExpr(this.getCondition()) - .regexpFind([eventPayloadActorFieldRegex(_), "\\bgithub\\.event\\.sender\\.login\\b"], _, - _) + .regexpFind(eventPayloadActorFieldRegex(context_prefix), _, _) ) } override predicate protectsCategoryAndEvent(string category, string event) { ActorCheck.super.protectsCategoryAndEvent(category, event) and ( - // the `sender` object is part of every webhook event payload, - // so `github.event.sender.login` is populated for every event - exists( - normalizeExpr(this.(If).getCondition()) - .regexpFind("\\bgithub\\.event\\.sender\\.login\\b", _, _) - ) + // the `sender` object is part of every webhook event payload + context_prefix = "github.event.sender" or - // other actor fields are only populated for events whose payload contains - // the corresponding context. eg: a check on - // `github.event.pull_request.user.login` cannot restrict the actor of an - // `issues` event since `github.event.pull_request` is not populated there, - // which makes the condition vacuous. - // note that `github.event.head_commit` and `github.event.commits` are only - // populated for `push` events, which are not protectable by ActorCheck, so - // checks on those fields never count as protection here. they are still - // matched so that these Ifs keep being classified as actor checks - exists(string context_prefix | - contextTriggerDataModel(event, context_prefix) and - exists( - normalizeExpr(this.(If).getCondition()) - .regexpFind(eventPayloadActorFieldRegex(context_prefix), _, _) - ) - ) + // other actor fields only restrict events whose payload populates them. + // eg: `github.event.pull_request.user.login` cannot restrict the actor + // of an `issues` event since `github.event.pull_request` is not + // populated there, which makes the condition vacuous + contextTriggerDataModel(event, context_prefix) ) } } From dc6ee115443e58ff8f84ad2d2118aaafbf2b65b5 Mon Sep 17 00:00:00 2001 From: Jordan Pike Date: Tue, 18 Aug 2026 15:54:56 -0400 Subject: [PATCH 4/4] Add test coverage for sender, head_commit and commits checks Pins the sender.login special case (protects every event) and that head_commit/commits checks do not protect issues events, where those fields are not populated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4084febb-f9c7-44df-baf3-c8be8e9932a7 --- .../workflows/actor_check_wrong_event.yml | 26 +++++++++++++++++++ .../CWE-094/CodeInjectionCritical.expected | 5 ++++ .../CWE-094/CodeInjectionMedium.expected | 4 +++ 3 files changed, 35 insertions(+) diff --git a/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml index fce829d4a9b4..982956b3d953 100644 --- a/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml +++ b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml @@ -18,3 +18,29 @@ jobs: if: github.actor == 'trusted-user' steps: - run: echo '${{ github.event.issue.title }}' + + # The `sender` object is part of every webhook event payload, so this + # check is effective and the injectable step is protected. + valid-sender-check: + runs-on: ubuntu-latest + if: github.event.sender.login == 'trusted-user' + steps: + - run: echo '${{ github.event.issue.title }}' + + # `github.event.head_commit` is only populated for `push` events, so this + # condition is always true for `issues` events and does not protect the + # injectable step. + vacuous-head-commit-check: + runs-on: ubuntu-latest + if: github.event.head_commit.author.name != 'some-bot' + steps: + - run: echo '${{ github.event.issue.title }}' + + # `github.event.commits` is only populated for `push` events, so this + # condition is always true for `issues` events and does not protect the + # injectable step. + vacuous-commits-check: + runs-on: ubuntu-latest + if: github.event.commits[0].author.name != 'some-bot' + 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 d58f13b44d7f..14e50942d734 100644 --- a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected +++ b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected @@ -273,6 +273,9 @@ nodes | .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] | | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE | @@ -714,6 +717,8 @@ subpaths | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | .github/workflows/test29.yml:35:18:35:54 | github.event.pull_request.body | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | Potential code injection in $@, which may be controlled by an external user ($@). | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | ${{ inputs.body }} | .github/workflows/test29.yml:12:3:12:21 | pull_request_target | pull_request_target | | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | Potential code injection in $@, which may be controlled by an external user ($@). | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | ${{ github.head_ref \|\| github.ref }} | .github/workflows/test29.yml:12:3:12:21 | pull_request_target | pull_request_target | | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | ${{ github.event.issue.title }} | .github/workflows/actor_check_wrong_event.yml:2:3:2:8 | issues | issues | +| .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | ${{ github.event.issue.title }} | .github/workflows/actor_check_wrong_event.yml:2:3:2:8 | issues | issues | +| .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | ${{ github.event.issue.title }} | .github/workflows/actor_check_wrong_event.yml:2:3:2:8 | issues | issues | | .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 code 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/argus_case_study.yml:4:3:4:8 | issues | issues | | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | .github/workflows/artifactpoisoning1.yml:14:9:20:6 | Uses Step | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | ${{ steps.pr.outputs.id }} | .github/workflows/artifactpoisoning1.yml:4:3:4:14 | workflow_run | workflow_run | | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | .github/workflows/artifactpoisoning2.yml:13:9:19:6 | Uses Step: pr | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | ${{ steps.pr.outputs.id }} | .github/workflows/artifactpoisoning2.yml:4:3:4:14 | workflow_run | workflow_run | diff --git a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected index 9833fd104e96..f10a7a55c049 100644 --- a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected +++ b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected @@ -273,6 +273,9 @@ nodes | .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] | | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE | @@ -714,6 +717,7 @@ subpaths | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | ${{ github.head_ref \|\| github.ref }} | | .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/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 }} |