feat(review-pr): authorize review_requested via the requester for external contributors

[Sayt-0]

Summary

Adds the second authorization path the issue describes, so the review workflow handles both triggers feeding the one review pipeline.

Path	Trigger	Authorized on
Auto-run (existing)	pull_request opened/ready/synchronize	PR author org membership (org members only)
Review-requested (new)	docker-agent requested as reviewer	Requester org membership (a maintainer pulls an external contributor PR into review)

Routing already converged both onto the single review job; the missing piece was the authorization, now handled in check-org-membership.

Issue expectations mapped

Expectation	Status	Where
Auto-run for org member PRs (existing behavior)	kept	evaluateMembership author path
Review-requested trigger for external contributors	added	evaluateMembership requester path
Both feed the same review pipeline	yes (pre-existing routing)	review-pr.yml review job (if lines 161-164)

Security

The requester is never trusted from the trigger artifact (a fork PR controls its own trigger workflow and could otherwise forge a member login):

• Direct same-repo path: requester is github.event.sender.login (populated by GitHub).
• Fork / workflow_run path: requester is re-derived server-side from the PR timeline (listEventsForTimeline) using the repo-scoped token.

Safeguards:

• The timeline lookup is bound to the same pr_number that drives the checkout, so forging pr_number only redirects to a real PR's own code, never attacker-controlled code.
• The derived requester is re-validated against real org membership.
• The timeline call runs only when the PR author is not already a member, so member-authored PRs incur no extra API call.

Side fix

Same-repo direct pull_request auto-run previously resolved an empty username (COMMENT_AUTHOR is empty on a pull_request event), so it now resolves the PR author.

Validation

Check	Result
typecheck	pass
build (tsup)	pass
unit tests	484 pass (+15 new)
biome ci	pass
actionlint (CI mode)	pass

Rollout note

The self-review workflow runs check-org-membership from the pinned released action (DOCKER_AGENT_ACTION_ROOT) and dist/ is gitignored, so this takes effect on the next release plus setup-credentials pin bump. No trigger-workflow or consumer template change is required, because the requester is derived from the timeline rather than the artifact.

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

One LIKELY finding in the new / resolveTrustedRequester authorization logic. The core security model is sound — the workflow routing enforces the fork/same-repo separation — but the TypeScript code relies on that workflow-layer invariant without any in-code assertion, creating a fragile defense-in-depth posture.

[docker-agent]

[LIKELY / MEDIUM] resolveTrustedRequester trusts REQUESTER env var without an in-code same-repo guard

On the prSource === 'event' + eventName === 'pull_request' + eventAction === 'review_requested' branch, the function returns inputs.trustedRequester — which comes directly from process.env.REQUESTER (set to github.event.sender.login in the workflow). There is no in-code assertion that this event path is exclusive to same-repo (non-fork) triggers; the trust guarantee is enforced only at the workflow layer (source=trigger is set for fork workflow_run paths, source=event for direct events).

This is safe as long as the workflow routing invariant holds. However, the TypeScript code itself has no internal check, so any future misconfiguration of the workflow (e.g., a consumer that passes PR_SOURCE=event on a fork path) would silently trust an attacker-controlled REQUESTER value and could grant review authorization to a non-member.

Suggested hardening: add an explicit guard in resolveTrustedRequester to make the safety assumption explicit and self-documenting — e.g., throw (or return '') if trustedRequester is empty, or add a comment tying the invariant to the workflow routing. A stronger fix would be to always use resolveReviewRequester (the timeline-based, server-side method) even on the prSource==='event' path, removing any reliance on the env-var value entirely.

[derekmisler]

one real security issue to fix before merging: resolveReviewRequester doesn't track review_request_removed events, so a retracted review request can still authorize a fork PR. fix is small (see inline). one minor suggestion about denial logging subject. the overall approach (trusted triple gate, timeline re-derivation on fork path, fail-closed on API errors) is solid.

[derekmisler]

issue: resolveReviewRequester only accumulates review_requested events but never checks for a later review_request_removed event on the same reviewer. if a maintainer requests docker-agent then removes that request, requester still holds the stale login and path 2 in evaluateMembership will grant authorization anyway, on the fork/workflow_run path especially, that's a real access control bypass.

fix: handle both event types in the same pass (timeline is ascending-chronological, so this handles re-request-after-removal correctly too):

let requester = '';
for (const ev of events) {
  if (ev.event === 'review_requested' && ev.requested_reviewer?.login === reviewerLogin) {
    requester = ev.review_requester?.login ?? ev.actor?.login ?? '';
  } else if (ev.event === 'review_request_removed' && ev.requested_reviewer?.login === reviewerLogin) {
    requester = ''; // revoked
  }
}

also worth adding review_request_removed as a possible event value in TimelineReviewRequestedEvent so future contributors see this case at the type level.

[Sayt-0]

Fixed in 6949610.

resolveReviewRequester now replays both event types for the reviewer in one pass: review_requested sets the requester, review_request_removed clears it, so a retracted request no longer authorizes the PR. review_request_removed is added to the (renamed) TimelineReviewEvent type.

One addition beyond the suggested fix: the replay now sorts by created_at rather than relying on the timeline's array order. That order is ascending in practice but not contractually guaranteed, and an out-of-order request/removal pair would otherwise leave a retraction stale, which the org-membership re-check cannot catch because the login is a real member.

New tests:

• removal clears a prior request
• re-request after removal wins
• removal targeting a different reviewer is ignored
• out-of-order replay still lets the retraction win
• fork/trigger integration: a retracted request denies an external PR

[derekmisler]

suggestion (non-blocking): when path 2 fires and the requester check fails, this always returns subject: author. but the party that actually failed the membership check was requester, not author (who correctly failed path 1 first). the operator log says ⏭️ Not authorized to review (subject: @ext-author) which points at the wrong person.

return { isMember: false, subject: requester || author, via: 'none' };

small change, but audit logs for auth failures should identify who actually failed.

[Sayt-0]

Applied in 6949610.

evaluateMembership now returns subject: requester || author on denial, so an authorization failure on path 2 names the requester that actually failed rather than the author who already failed path 1.

Tests updated to assert the corrected subject:

• direct review_requested denial reports the requester (outsider)
• fork/trigger denial reports the timeline-derived requester (not-a-member)

[derekmisler]

fix is correct. the review_request_removed handling is clean, the created_at sort to avoid relying on undocumented array ordering is a nice proactive addition, and the denial subject now points at the right party. good work addressing this quickly.