Advisoriesby IbisGitHub: improper authorization
Summary
isConfusedDeputyAttack exists to catch the case where the actor who triggered a
workflow is not the author of the content the agent will read. It handles four
event names. pull_request_target is not one of them, and the string does not
appear anywhere in the file.
On pull_request_target: types: [labeled] the actor is the maintainer who
applied the label. The membership gate validates that maintainer, the
confused-deputy check returns false because it has no branch for the event, and
the agent then runs in the base-repo context reading a fork PR written by someone
else entirely.
The gate answers "may this person start the agent". Nobody asks "who wrote the
content the agent is about to read".
I want to put the argument plainly, because the easy reply is that the adopter
chose an untrusted trigger.
I am not arguing that actor != author is always suspicious. For pull_request
your own comment gives a good reason to tolerate it, and I agree with it. The
argument is narrower: this mechanism exists specifically to detect the
confused-deputy condition, and it omits the event where the actor/content
principal mismatch intersects with the highest-trust execution context. The check
covers four sibling events and skips that one. An adopter using a documented
trigger with a documented forks: key gets no coverage from a function written
for exactly their...
Affected versions
| Package | Affected | Fixed in |
|---|---|---|
| github/gh-aw Product | < v0.87.0 | v0.87.0 |
Details and references
### Summary `isConfusedDeputyAttack` exists to catch the case where the actor who triggered a workflow is not the author of the content the agent will read. It handles four event names. `pull_request_target` is not one of them, and the string does not appear anywhere in the file. On `pull_request_target: types: [labeled]` the actor is the maintainer who applied the label. The membership gate validates that maintainer, the confused-deputy check returns false because it has no branch for the event, and the agent then runs in the base-repo context reading a fork PR written by someone else entirely. The gate answers "may this person start the agent". Nobody asks "who wrote the content the agent is about to read". I want to put the argument plainly, because the easy reply is that the adopter chose an untrusted trigger. I am not arguing that actor != author is always suspicious. For `pull_request` your own comment gives a good reason to tolerate it, and I agree with it. The argument is narrower: this mechanism exists specifically to detect the confused-deputy condition, and it omits the event where the actor/content principal mismatch intersects with the highest-trust execution context. The check covers four sibling events and skips that one. An adopter using a documented trigger with a documented `forks:` key gets no coverage from a function written for exactly their situation. The distinction that matters for a fix is fork versus internal, not labeled versus not. A PR where `pull_request.head.repo.full_name != pull_request.base.repo.full_name` is authored by someone outside the repository's trust boundary; a same-repo PR is not, and its author already holds repository permissions. Keying on that boundary lets you close this without disturbing the ordinary case of a maintainer labelling an internal PR, which I expect is the false-positive you would worry about first. ### Details `actions/setup/js/check_permissions_utils.cjs`, `isConfusedDeputyAttack` at `:107`. The branches are: - `:120` `pull_request` — only when `action === "synchronize"` and the actor ends with `[bot]` - `:130` `pull_request_review` — reviewer must match the actor - `:140` `pull_request_review_comment` — comment author must match the actor - `:161` `issue_comment` — comment author must match the actor Then `return false`. `grep -c pull_request_target` on that file returns **0** at v0.85.4 and **0** on main at `b2ef1f3`. The comment at `:114-116` explains the design for `pull_request`: > Other pull_request actions (labeled, unlabeled, assigned, review_requested, > etc.) legitimately have actor != pr_author — the actor is whoever performed the > action, not the PR author — so checking those would cause false positives. That reasoning is sound for `pull_request`, which does not get repository secrets on a fork PR. `pull_request_target` does. The same "actor != author is normal" assumption is carried into an event where the content is untrusted and the token is not, and the file never distinguishes them because it never sees the event name at all. `check_membership.cjs:195` calls the function and, when it returns false, falls through to `checkRepositoryPermission` on `context.actor` — the labeler. ### PoC 7 assertions driving the unmodified shipped `check_permissions_utils.cjs`. No network, no third-party system. Attached, runs in one command. Environment: node 18.19.1, WSL2 Ubuntu 24.04. The payload is a `labeled` event where the PR author is `attacker` and the actor is `maintainer`: - **T1** At v0.85.4, `isConfusedDeputyAttack("maintainer", "pull_request_target", …)` returns **false**. The attacker-authored PR is not flagged. - **T2** Positive controls: the same harness gets **true** for `issue_comment` and `pull_request_review` with the same author/actor mismatch. The branches that exist do fire, so a false negative in T1 is a real gap and not a broken t