Uh oh!
There was an error while loading. Please reload this page.
Extend confused deputy protection to pull_request_target - #52976
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Extends confused-deputy protection to pull_request_target synchronization events.
Changes:
- Applies bot actor-to-PR-author validation to both PR event types.
- Adds coverage for malicious mismatches and legitimate actors.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/check_permissions_utils.cjs | Extends event validation to pull_request_target. |
actions/setup/js/check_permissions_utils.test.cjs | Tests matching, mismatched, and human actors. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52976 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #52976 locally; no actionable review comments or GitHub writes were needed because the patch is a narrow correctness fix with targeted tests and I found no blocking issues.
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
The change is correct and well-tested. Extending the confused deputy guard to pull_request_target is the right call — that trigger runs in the base-branch context with write permissions, making it more susceptible to the attack, not less. The three new test cases cover all relevant scenarios (bot-mismatch → true, bot-match → false, human → false). No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.3 AIC · ⌖ 7 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one minor gap in test coverage, otherwise approving.
📋 Key Themes & Highlights
Key Themes
- Security fix is correct: The one-line condition change (
|| eventName === "pull_request_target") is minimal and precisely closes the bypass. - Test coverage is good but not complete: Three new tests cover the attack case, the safe case, and the human-collaborator case. The only gap is a non-
synchronizeaction forpull_request_target, which thepull_requestsuite already covers.
Positive Highlights
- ✅ Root cause properly addressed — the condition now mirrors
pull_requestexactly. - ✅ Comment updated to reflect both event types.
- ✅ Tests read as specifications (clear arrange/act/assert, descriptive names).
- ✅ No collateral changes — the fix is surgical.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 23.6 AIC · ⌖ 8.95 AIC · ⊞ 7.7K
Comment /matt to run again
| it("should return false for pull_request_review when actor matches review author (genuine review)", () => { | ||
| const payload = { | ||
| pull_request: { user: { login: "pr-author" } }, |
There was a problem hiding this comment.
[/tdd] Missing edge case: no test covers pull_request_target with a non-synchronize action (e.g. labeled). The existing pull_request suite has an equivalent case — adding one here fully mirrors coverage and guards against future regressions if the condition logic changes.
💡 Suggested test
it("should return false for pull_request_target:labeled (non-synchronize action)",()=>{constpayload={action: "labeled",pull_request: {user: {login: "alice"}}};expect(isConfusedDeputyAttack("dependabot[bot]","pull_request_target",payload)).toBe(false);});@copilot please address this.
There was a problem hiding this comment.
Added the non-synchronizepull_request_target:labeled coverage in 4866db1. The focused setup JS test and make agent-report-progress both pass locally.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (3 tests)
AnalysisThis PR extends confused deputy attack detection to
All tests are behavioral contracts enforcing the security design, not implementation details. The 5:1 test-to-production ratio is justified: expanding event-type coverage from Verdict
|
pelikhan
commented
Aug 15, 2026
gh-aw-bot
commented
Aug 15, 2026
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, fix the remaining failed check, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed pr-finisher pass in
CI on the agent-pushed HEAD is stale and needs a maintainer re-trigger before merge. |
🎉 This pull request is included in a new release. Release: |
pull_request_targetsynchronize events bypassed the confused-deputy check applied to equivalentpull_requestevents, allowing a bot actor to differ from the PR author.Detection
synchronize-only safeguards.Coverage