Honor bot allowlists before confused-deputy checks - #60135
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
There was a problem hiding this comment.
🟡 Changes recommended
The reordering lets allowlisted bots bypass confused-deputy protection on unrelated event types.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates bot authorization ordering so trusted active bots can synchronize pull requests opened by other actors.
Changes:
- Checks bot allowlists before PR confused-deputy validation.
- Adds synchronization and inactive-bot regression tests.
File summaries
| File | Description |
|---|---|
actions/setup/js/check_membership.cjs |
Reorders bot authorization and confused-deputy checks. |
actions/setup/js/check_membership.test.cjs |
Covers both PR synchronization event types and inactive bots. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Test Quality Sentinel Report 🧪PR Context
Test Quality Score: 100/100 ✅ ExcellentTest Coverage Summary
Quality AnalysisTest 1: Active Allowlisted Bot Authorization
Test 2: Inactive Allowlisted Bot Denial
Coverage Highlights
No Red Flags
Scoring Breakdown
Total: 100/100 What This PR TestsThis PR adds critical test coverage for the bot allowlist authorization check ordering fix (Issue #59952). The tests verify that:
Approval ✅Implementation tests: 0% All tests are behavioral design tests covering critical security authorization logic. The test inflation ratio is within acceptable bounds for security-critical changes. No coding guideline violations. Recommendation: Approve — Test quality is excellent and properly validates the security fix. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Impeccable review — security/correctness pass (harden, audit modes)
This PR reorders checkBotAllowlistAuthorization to run before isConfusedDeputyAttack in check_membership.cjs, so that an allowlisted-bot match short-circuits the confused-deputy check on pull_request(_target):synchronize.
🔴 Blocking: the new ordering re-opens the exact Dependabot Confused Deputy Injection scenario ADR-29450 was written to close, for any repo whose on.bots: allowlist includes dependabot[bot] (which the project's own docs show as an example allowlist entry). See inline comment for the detailed attack trace and a suggested fix/regression test.
The added tests only cover a synthetic bot name (my-fixup-bot[bot]), so the existing suite does not catch this regression — the fix and this new test scenario should ship together.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 97.9 AIC · ⌖ 13.9 AIC · ⊞ 8.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs (regression risk analysis for a security-sensitive check reordering) and /tdd (test coverage gaps). Requesting changes: the fix reopens the specific attack ADR-29450 was written to close, for the actor it was designed to stop.
📋 Key Themes & Highlights
Key Themes
- Security regression: Moving
checkBotAllowlistAuthorizationahead ofisConfusedDeputyAttackremoves confused-deputy protection for any actor that is both allowlisted inon.bots:and installed/active on the repo — which per the project's own docs (triggers.md:291,495) commonly includesdependabot[bot]itself, the actor ADR-29450 explicitly targeted.checkBotAllowlistAuthorization/checkBotStatusonly verify allowlist membership + installation status; neither checks whether the current event's actor genuinely matches the PR/comment author, so it cannot distinguish a legitimate bot-initiated event from a confused-deputy replay. - Test coverage gap: All new tests use a fictitious
my-fixup-bot[bot]that is not simultaneously the historically-targeted actor. No test exercises the combination of "actor isdependabot[bot], is allowlisted viaon.bots, and PR author differs" — which is exactly the scenario the ADR's threat model addresses and where the new ordering silently authorizes instead of denying. - Scope beyond
synchronize: The same reordering also affectsissue_commentevents (the@dependabot showvector), sinceisConfusedDeputyAttackforissue_commentdoesn't require a[bot]-suffixed actor — any allowlisted bot whose comment differs from the actor field now bypasses that check too.
Suggested Direction
Either (a) keep the confused-deputy guard ahead of the bot-allowlist bypass for actors matching known slash-command-triggerable bots (or all bots, falling back to explicit allowlist-driven override only when the actor equality also holds), or (b) if bypassing confused-deputy checks for any allowlisted bot is the intended new behavior, add explicit regression tests proving dependabot[bot] (allowlisted + active + author mismatch) is still denied, and update ADR-29450 to document the narrowed threat model and residual risk.
Positive Highlights
- ✅ Good test hygiene:
it.eachused to cover bothpull_requestandpull_request_targetfor the new allowlist-first path. - ✅ The "inactive allowlisted bot still denied" test is a solid fail-closed check for the allowlist branch itself.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 113 AIC · ⌖ 15.2 AIC · ⊞ 10.4K
Comment /matt to run again
|
@copilot The compact report shows a failed check on this PR. Please inspect the failing automation, address any outstanding review feedback, refresh the branch if needed, and run the pr-finisher skill before handing back to maintainers. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in d68f0f3. I inspected the failed PR Code Quality Reviewer logs (tooling failure, not a finding), incorporated all review feedback, refreshed from |
|
@copilot Three unresolved review threads on this PR already have follow-up responses from the PR author or Copilot but remain unresolved:
Please resolve the remaining review state, refresh the branch if needed, and run the Run: https://github.com/github/gh-aw/actions/runs/34581583531 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed the pr-finisher pass. All three named review threads are now resolved. Run 34581583531 succeeded with all 10 jobs green and no failed logs; the branch is current with |
|
@copilot The obsolete blocking review still remains on this PR even though the author reports all three named threads are resolved. Please clean up the remaining review state, refresh the branch if needed, and run the Run: https://github.com/github/gh-aw/actions/runs/34589658410
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed the pr-finisher pass. Run 34589658410 succeeded with all 10 jobs green and no failed logs; all review threads are resolved, the branch is current with |
|
@copilot add logging |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot update compiler threat specification |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in be72029. Compiler threat specification v1.0.34 now defines CTR-027 for allowlisted bot synchronization provenance, maps the runtime implementation and tests, assigns compliance test T-CTR-042, and documents fail-closed behavior for forks, untrusted authors, inactive bots, missing provenance, and Dependabot mismatches. Formal conformance and final impacted validation pass. |
|
🎉 This pull request is included in a new release. Release: |
Allowlisted bots were rejected on pull request
synchronizeevents when pushing to PRs opened by another actor. The confused-deputy check ran before the expliciton.botsauthorization.Authorization
Coverage
pull_requestandpull_request_targetsynchronization.on.bots:allowlist is never consulted when the confused-deputy check fires onpull_request(_target):synchronize#59952Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.