Skip to content

Commit 9251d21

Browse files
computersarebadCopilot
computersarebad
and
Copilot
committed
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
1 parent 97e3d36 commit 9251d21

6 files changed

Lines changed: 93 additions & 6 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+
* 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`.

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

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -312,17 +312,28 @@ class LabelIfCheck extends LabelCheck instanceof If {
312312
}
313313
}
314314

315+
/**
316+
* Gets a regular expression matching a condition on an actor field that is
317+
* only populated for events whose payload contains the `context_prefix` context.
318+
*/
319+
private string eventPayloadActorFieldRegex(string context_prefix) {
320+
context_prefix = "github.event.pull_request" and
321+
result = "\\bgithub\\.event\\.pull_request\\.user\\.login\\b"
322+
or
323+
context_prefix = "github.event.head_commit" and
324+
result = "\\bgithub\\.event\\.head_commit\\.author\\.name\\b"
325+
or
326+
context_prefix = "github.event.commits" and
327+
result = "\\bgithub\\.event\\.commits.*\\.author\\.name\\b"
328+
}
329+
315330
class ActorIfCheck extends ActorCheck instanceof If {
316331
ActorIfCheck() {
317332
// eg: github.event.pull_request.user.login == 'admin'
318333
exists(
319334
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-
], _, _)
335+
.regexpFind([eventPayloadActorFieldRegex(_), "\\bgithub\\.event\\.sender\\.login\\b"], _,
336+
_)
326337
)
327338
or
328339
// eg: github.actor == 'admin'
@@ -333,6 +344,37 @@ class ActorIfCheck extends ActorCheck instanceof If {
333344
) and
334345
not normalizeExpr(this.getCondition()).matches("%[bot]%")
335346
}
347+
348+
override predicate protectsCategoryAndEvent(string category, string event) {
349+
ActorCheck.super.protectsCategoryAndEvent(category, event) and
350+
(
351+
// `github.actor`, `github.triggering_actor` and `github.event.sender.login`
352+
// are populated for every event
353+
exists(
354+
normalizeExpr(this.(If).getCondition())
355+
.regexpFind("\\bgithub\\.event\\.sender\\.login\\b", _, _)
356+
)
357+
or
358+
exists(
359+
normalizeExpr(this.(If).getCondition())
360+
.regexpFind(["\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b",], _, _)
361+
) and
362+
not normalizeExpr(this.(If).getCondition()).matches("%[bot]%")
363+
or
364+
// actor fields read from the event payload are only populated for events
365+
// whose payload contains the corresponding context. eg: a check on
366+
// `github.event.pull_request.user.login` cannot restrict the actor of an
367+
// `issues` event since `github.event.pull_request` is not populated there,
368+
// which makes the condition vacuous
369+
exists(string context_prefix |
370+
contextTriggerDataModel(event, context_prefix) and
371+
exists(
372+
normalizeExpr(this.(If).getCondition())
373+
.regexpFind(eventPayloadActorFieldRegex(context_prefix), _, _)
374+
)
375+
)
376+
)
377+
}
336378
}
337379

338380
class PullRequestTargetRepositoryIfCheck extends RepositoryCheck instanceof If {

‎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: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
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 }}'

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,9 @@ 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 |
273276
| .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] |
274277
| .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title |
275278
| .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE |
@@ -710,6 +713,7 @@ subpaths
710713
| .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 |
711714
| .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 |
712715
| .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 |
716+
| .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 |
713717
| .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 |
714718
| .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 |
715719
| .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: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,9 @@ 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 |
273276
| .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] |
274277
| .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title |
275278
| .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE |
@@ -709,6 +712,8 @@ subpaths
709712
| .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 }} |
710713
| .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 }} |
711714
| .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 }} |
715+
| .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 }} |
716+
| .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 }} |
712717
| .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 }} |
713718
| .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 }} |
714719
| .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)