fix: scope confused deputy check to bot actors on pull_request:synchronize#44494
Conversation
…onize The isConfusedDeputyAttack function was comparing github.actor against pull_request.user.login (PR author) for all synchronize events, causing false positives when a human team member pushes commits to a PR they did not author (legitimate collaboration). The confused deputy attack (@dependabot recreate) always involves a bot actor: actor=dependabot[bot], prAuthor=original human. Restricting the synchronize check to actors ending in [bot] eliminates the false positive while maintaining full protection against the actual attack. Also: - Add regression test for human-collaborates-on-others-PR scenario - Update ADR-29450 normative spec and narrative to reflect bot-scoping Closes #39871" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR narrows the Dependabot “confused deputy” detection logic for pull_request:synchronize events so it only triggers when the GitHub Actions actor is a bot (login ending with [bot]). This prevents legitimate multi-author collaboration (a human pushing commits to someone else’s PR) from being incorrectly blocked, while preserving the intended protection against the documented bot-driven attack vector.
Changes:
- Restrict
isConfusedDeputyAttack’spull_request:synchronizecheck to bot actors (actor.endsWith("[bot]")). - Add a regression test ensuring human-vs-human actor/PR-author mismatches on
synchronizeare not flagged. - Update ADR-29450 to make the bot-scoping requirement explicit in both narrative and normative spec.
Show a summary per file
| File | Description |
|---|---|
| docs/adr/29450-guard-bot-filter-against-dependabot-confused-deputy-injection.md | Updates the ADR decision and normative requirements to specify bot-only scoping for the pull_request:synchronize guard. |
| actions/setup/js/check_permissions_utils.test.cjs | Adds regression coverage for legitimate human collaboration on pull_request:synchronize. |
| actions/setup/js/check_permissions_utils.cjs | Implements bot-only scoping for pull_request:synchronize confused-deputy detection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
|
|
🤖 PR Triage — Run §29020837798
Score breakdown: Impact 38 + Urgency 25 + Quality 18 Rationale: Tiny, targeted security fix (16 +/ 5 −) eliminating a false positive that blocked legitimate multi-author collaboration. Patch is trivially correct — adds
|
|
@copilot please run the
|
…39871-fix-access-denied-error Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
|
|
Ran the PR finisher pass on |
|
@copilot please run the
|
Ran the PR finisher pass on Local full validation is still blocked by a pre-existing clean-checkout issue on the current base: |
|
@copilot please run the
|
Ran the PR-finisher pass on No code changes were needed. The PR-side |
|
Run: https://github.com/github/gh-aw/actions/runs/29038065496
|
|
🎉 This pull request is included in a new release. Release: |
isConfusedDeputyAttackwas flagging anypull_request:synchronizeevent wheregithub.actor≠pull_request.user.login— blocking legitimate multi-author collaboration where a human team member pushes commits to a PR they didn't open.The actual attack vector (
@dependabot recreate) always produces a bot actor:actor=dependabot[bot],prAuthor=<human>. Human-vs-human mismatches are never confused deputy — the pusher's own repository permissions are checked normally immediately after.Changes
check_permissions_utils.cjs— Addactor.endsWith("[bot]")guard to thepull_request:synchronizebranch:check_permissions_utils.test.cjs— Add regression test:actor="bob",prAuthor="alice"onsynchronize→false.ADR-29450 — Update normative spec (Part 2 §2) and narrative (Decision + Negative Consequences) to require bot-scoping on the synchronize guard.