Skip to content

Commit 3048f47

Browse files
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
1 parent 9251d21 commit 3048f47

2 files changed

Lines changed: 22 additions & 19 deletions

File tree

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
---
22
category: minorAnalysis
33
---
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`.
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. 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`.

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

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -329,13 +329,6 @@ private string eventPayloadActorFieldRegex(string context_prefix) {
329329

330330
class ActorIfCheck extends ActorCheck instanceof If {
331331
ActorIfCheck() {
332-
// eg: github.event.pull_request.user.login == 'admin'
333-
exists(
334-
normalizeExpr(this.getCondition())
335-
.regexpFind([eventPayloadActorFieldRegex(_), "\\bgithub\\.event\\.sender\\.login\\b"], _,
336-
_)
337-
)
338-
or
339332
// eg: github.actor == 'admin'
340333
// eg: github.triggering_actor == 'admin'
341334
exists(
@@ -344,28 +337,38 @@ class ActorIfCheck extends ActorCheck instanceof If {
344337
) and
345338
not normalizeExpr(this.getCondition()).matches("%[bot]%")
346339
}
340+
}
341+
342+
/** An If node that checks an actor field from the event payload */
343+
class EventActorIfCheck extends ActorCheck instanceof If {
344+
EventActorIfCheck() {
345+
// eg: github.event.pull_request.user.login == 'admin'
346+
exists(
347+
normalizeExpr(this.getCondition())
348+
.regexpFind([eventPayloadActorFieldRegex(_), "\\bgithub\\.event\\.sender\\.login\\b"], _,
349+
_)
350+
)
351+
}
347352

348353
override predicate protectsCategoryAndEvent(string category, string event) {
349354
ActorCheck.super.protectsCategoryAndEvent(category, event) and
350355
(
351-
// `github.actor`, `github.triggering_actor` and `github.event.sender.login`
352-
// are populated for every event
356+
// the `sender` object is part of every webhook event payload,
357+
// so `github.event.sender.login` is populated for every event
353358
exists(
354359
normalizeExpr(this.(If).getCondition())
355360
.regexpFind("\\bgithub\\.event\\.sender\\.login\\b", _, _)
356361
)
357362
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
363+
// other actor fields are only populated for events whose payload contains
364+
// the corresponding context. eg: a check on
366365
// `github.event.pull_request.user.login` cannot restrict the actor of an
367366
// `issues` event since `github.event.pull_request` is not populated there,
368-
// which makes the condition vacuous
367+
// which makes the condition vacuous.
368+
// note that `github.event.head_commit` and `github.event.commits` are only
369+
// populated for `push` events, which are not protectable by ActorCheck, so
370+
// checks on those fields never count as protection here. they are still
371+
// matched so that these Ifs keep being classified as actor checks
369372
exists(string context_prefix |
370373
contextTriggerDataModel(event, context_prefix) and
371374
exists(

0 commit comments

Comments
 (0)