Skip to content

Commit 039c2b5

Browse files
jketemajketema
authored
Merge pull request #22368 from computersarebad/actor-if-check-event-validity
Actions: `ActorIfCheck` `ControlCheck` model fix for events that don't populate the checked field
2 parents 7c2b8b4 + 0a376e8 commit 039c2b5

7 files changed

Lines changed: 129 additions & 11 deletions

File tree

‎actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* 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. 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 may result in more alerts for queries using the `ControlCheck` class.

‎actions/ql/lib/change-notes/2026-08-18-split-event-actor-if-check.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: breaking
3+
---
4+
* Checks on actor fields read from the event payload (e.g. `github.event.pull_request.user.login`) were split out of `ActorIfCheck` into a new class `EventActorIfCheck`. The `ActorIfCheck` class now only covers `github.actor` and `github.triggering_actor`.

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

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -314,17 +314,6 @@ class LabelIfCheck extends LabelCheck instanceof If {
314314

315315
class ActorIfCheck extends ActorCheck instanceof If {
316316
ActorIfCheck() {
317-
// eg: github.event.pull_request.user.login == 'admin'
318-
exists(
319-
normalizeExpr(this.getCondition())
320-
.regexpFind([
321-
"\\bgithub\\.event\\.pull_request\\.user\\.login\\b",
322-
"\\bgithub\\.event\\.head_commit\\.author\\.name\\b",
323-
"\\bgithub\\.event\\.commits.*\\.author\\.name\\b",
324-
"\\bgithub\\.event\\.sender\\.login\\b"
325-
], _, _)
326-
)
327-
or
328317
// eg: github.actor == 'admin'
329318
// eg: github.triggering_actor == 'admin'
330319
exists(
@@ -335,6 +324,51 @@ class ActorIfCheck extends ActorCheck instanceof If {
335324
}
336325
}
337326

327+
/**
328+
* Gets a regular expression matching a condition on an actor field that is
329+
* only populated for events whose payload contains the `context_prefix` context.
330+
*/
331+
private string eventPayloadActorFieldRegex(string context_prefix) {
332+
context_prefix = "github.event.pull_request" and
333+
result = "\\bgithub\\.event\\.pull_request\\.user\\.login\\b"
334+
or
335+
context_prefix = "github.event.head_commit" and
336+
result = "\\bgithub\\.event\\.head_commit\\.author\\.name\\b"
337+
or
338+
context_prefix = "github.event.commits" and
339+
result = "\\bgithub\\.event\\.commits.*\\.author\\.name\\b"
340+
or
341+
context_prefix = "github.event.sender" and
342+
result = "\\bgithub\\.event\\.sender\\.login\\b"
343+
}
344+
345+
/** An If node that checks an actor field from the event payload */
346+
class EventActorIfCheck extends ActorCheck instanceof If {
347+
string context_prefix;
348+
349+
EventActorIfCheck() {
350+
// eg: github.event.pull_request.user.login == 'admin'
351+
exists(
352+
normalizeExpr(this.getCondition())
353+
.regexpFind(eventPayloadActorFieldRegex(context_prefix), _, _)
354+
)
355+
}
356+
357+
override predicate protectsCategoryAndEvent(string category, string event) {
358+
ActorCheck.super.protectsCategoryAndEvent(category, event) and
359+
(
360+
// the `sender` object is part of every webhook event payload
361+
context_prefix = "github.event.sender"
362+
or
363+
// other actor fields only restrict events whose payload populates them.
364+
// eg: `github.event.pull_request.user.login` cannot restrict the actor
365+
// of an `issues` event since `github.event.pull_request` is not
366+
// populated there, which makes the condition vacuous
367+
contextTriggerDataModel(event, context_prefix)
368+
)
369+
}
370+
}
371+
338372
class PullRequestTargetRepositoryIfCheck extends RepositoryCheck instanceof If {
339373
PullRequestTargetRepositoryIfCheck() {
340374
// eg: github.event.pull_request.head.repo.full_name == github.repository

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
on:
2+
pull_request_target:
3+
types: [opened]
4+
5+
jobs:
6+
# The `if:` condition checks an actor field that is populated for
7+
# `pull_request_target` events, so the injectable step is protected.
8+
valid-actor-check:
9+
runs-on: ubuntu-latest
10+
if: github.event.pull_request.user.login == 'trusted-user'
11+
steps:
12+
- run: echo '${{ github.event.pull_request.title }}'

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
on:
2+
issues:
3+
types: [opened]
4+
5+
jobs:
6+
# The `if:` condition compares an actor field that is never populated for
7+
# `issues` events, so it is always true and does not protect the injectable step.
8+
vacuous-actor-check:
9+
runs-on: ubuntu-latest
10+
if: github.event.pull_request.user.login != 'some-bot[bot]'
11+
steps:
12+
- run: echo '${{ github.event.issue.title }}'
13+
14+
# The `if:` condition checks an actor context that is populated for every
15+
# event, so the injectable step is protected.
16+
valid-actor-check:
17+
runs-on: ubuntu-latest
18+
if: github.actor == 'trusted-user'
19+
steps:
20+
- run: echo '${{ github.event.issue.title }}'
21+
22+
# The `sender` object is part of every webhook event payload, so this
23+
# check is effective and the injectable step is protected.
24+
valid-sender-check:
25+
runs-on: ubuntu-latest
26+
if: github.event.sender.login == 'trusted-user'
27+
steps:
28+
- run: echo '${{ github.event.issue.title }}'
29+
30+
# `github.event.head_commit` is only populated for `push` events, so this
31+
# condition is always true for `issues` events and does not protect the
32+
# injectable step.
33+
vacuous-head-commit-check:
34+
runs-on: ubuntu-latest
35+
if: github.event.head_commit.author.name != 'some-bot'
36+
steps:
37+
- run: echo '${{ github.event.issue.title }}'
38+
39+
# `github.event.commits` is only populated for `push` events, so this
40+
# condition is always true for `issues` events and does not protect the
41+
# injectable step.
42+
vacuous-commits-check:
43+
runs-on: ubuntu-latest
44+
if: github.event.commits[0].author.name != 'some-bot'
45+
steps:
46+
- run: echo '${{ github.event.issue.title }}'

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,12 @@ nodes
270270
| .github/actions/external/ultralytics/actions/action.yaml:66:3:66:6 | input body | semmle.label | input body |
271271
| .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | semmle.label | inputs.body |
272272
| .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | semmle.label | github.head_ref \|\| github.ref |
273+
| .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
274+
| .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | semmle.label | github.event.issue.title |
275+
| .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | semmle.label | github.event.issue.title |
276+
| .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | semmle.label | github.event.issue.title |
277+
| .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | semmle.label | github.event.issue.title |
278+
| .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | semmle.label | github.event.issue.title |
273279
| .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] |
274280
| .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title |
275281
| .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE |
@@ -710,6 +716,9 @@ subpaths
710716
| .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 |
711717
| .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 |
712718
| .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 |
719+
| .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 |
720+
| .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 |
721+
| .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 |
713722
| .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 |
714723
| .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 |
715724
| .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 |

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,12 @@ nodes
270270
| .github/actions/external/ultralytics/actions/action.yaml:66:3:66:6 | input body | semmle.label | input body |
271271
| .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | semmle.label | inputs.body |
272272
| .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | semmle.label | github.head_ref \|\| github.ref |
273+
| .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
274+
| .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | semmle.label | github.event.issue.title |
275+
| .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | semmle.label | github.event.issue.title |
276+
| .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | semmle.label | github.event.issue.title |
277+
| .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | semmle.label | github.event.issue.title |
278+
| .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | semmle.label | github.event.issue.title |
273279
| .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] |
274280
| .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title |
275281
| .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE |
@@ -709,6 +715,9 @@ subpaths
709715
| .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 }} |
710716
| .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 }} |
711717
| .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 }} |
718+
| .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 }} |
719+
| .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 }} |
720+
| .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 }} |
712721
| .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 }} |
713722
| .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 }} |
714723
| .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 }} |

0 commit comments

Comments
 (0)