Skip to content

Commit e9d9d3a

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

1 file changed

Lines changed: 25 additions & 36 deletions

File tree

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

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

315+
class ActorIfCheck extends ActorCheck instanceof If {
316+
ActorIfCheck() {
317+
// eg: github.actor == 'admin'
318+
// eg: github.triggering_actor == 'admin'
319+
exists(
320+
normalizeExpr(this.getCondition())
321+
.regexpFind(["\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b",], _, _)
322+
) and
323+
not normalizeExpr(this.getCondition()).matches("%[bot]%")
324+
}
325+
}
326+
315327
/**
316328
* Gets a regular expression matching a condition on an actor field that is
317329
* only populated for events whose payload contains the `context_prefix` context.
@@ -325,57 +337,34 @@ private string eventPayloadActorFieldRegex(string context_prefix) {
325337
or
326338
context_prefix = "github.event.commits" and
327339
result = "\\bgithub\\.event\\.commits.*\\.author\\.name\\b"
328-
}
329-
330-
class ActorIfCheck extends ActorCheck instanceof If {
331-
ActorIfCheck() {
332-
// eg: github.actor == 'admin'
333-
// eg: github.triggering_actor == 'admin'
334-
exists(
335-
normalizeExpr(this.getCondition())
336-
.regexpFind(["\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b",], _, _)
337-
) and
338-
not normalizeExpr(this.getCondition()).matches("%[bot]%")
339-
}
340+
or
341+
context_prefix = "github.event.sender" and
342+
result = "\\bgithub\\.event\\.sender\\.login\\b"
340343
}
341344

342345
/** An If node that checks an actor field from the event payload */
343346
class EventActorIfCheck extends ActorCheck instanceof If {
347+
string context_prefix;
348+
344349
EventActorIfCheck() {
345350
// eg: github.event.pull_request.user.login == 'admin'
346351
exists(
347352
normalizeExpr(this.getCondition())
348-
.regexpFind([eventPayloadActorFieldRegex(_), "\\bgithub\\.event\\.sender\\.login\\b"], _,
349-
_)
353+
.regexpFind(eventPayloadActorFieldRegex(context_prefix), _, _)
350354
)
351355
}
352356

353357
override predicate protectsCategoryAndEvent(string category, string event) {
354358
ActorCheck.super.protectsCategoryAndEvent(category, event) and
355359
(
356-
// the `sender` object is part of every webhook event payload,
357-
// so `github.event.sender.login` is populated for every event
358-
exists(
359-
normalizeExpr(this.(If).getCondition())
360-
.regexpFind("\\bgithub\\.event\\.sender\\.login\\b", _, _)
361-
)
360+
// the `sender` object is part of every webhook event payload
361+
context_prefix = "github.event.sender"
362362
or
363-
// other actor fields are only populated for events whose payload contains
364-
// the corresponding context. eg: a check on
365-
// `github.event.pull_request.user.login` cannot restrict the actor of an
366-
// `issues` event since `github.event.pull_request` is not populated there,
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
372-
exists(string context_prefix |
373-
contextTriggerDataModel(event, context_prefix) and
374-
exists(
375-
normalizeExpr(this.(If).getCondition())
376-
.regexpFind(eventPayloadActorFieldRegex(context_prefix), _, _)
377-
)
378-
)
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)
379368
)
380369
}
381370
}

0 commit comments

Comments
 (0)