Skip to content

feat(review-pr): add abuse safeguards for review requests#13

Open
Sayt-0 wants to merge 3 commits into
mainfrom
feat/review-request-abuse-safeguards
Open

feat(review-pr): add abuse safeguards for review requests#13
Sayt-0 wants to merge 3 commits into
mainfrom
feat/review-request-abuse-safeguards

Conversation

@Sayt-0

@Sayt-0 Sayt-0 commented Jun 24, 2026

Copy link
Copy Markdown
Member

Summary

Adds safeguards against abuse of review requests on every trigger path
(review_requested, /review, @docker-agent mention, automatic opened/synchronize,
and the workflow_run fork path).

Requirement Status How
Review requests come from org members done PR author verified on the direct pull_request path (was an empty comment author); resolveUsername with a live PR-author fallback; fails closed. review_requested relies on GitHub-native reviewer-request enforcement, now documented.
Log all review requests done src/audit-log writes one structured record per request (requester, time, trigger, PR, reviewed SHA, decision) as a notice, a job-summary line, and a 90 day JSONL artifact, on allow and deny paths.
Detect rate anomalies done src/rate-limit skips the review above a per-PR frequency threshold; per-PR concurrency: groups collapse same-trigger bursts; existing cache lock prevents concurrent reviews.
Handle force-pushed commits after review done src/check-staleness compares the requested SHA to current head, records the reviewed SHA, and posts a notice on rebase.

Validation

Check Result
build (tsup) pass
typecheck (tsc) pass
biome ci pass (88 files)
actionlint 1.7.12 pass
unit tests 500 pass (30 new)

An adversarial review pass found and fixed two defects: a throttled request could
create a green "PR Review" check (rate check now precedes and gates check
creation), and an audit record could be lost on a setup-credentials failure (an
inline fallback now records it).

Residual design notes

  • The rate-limit signal counts the bot's own recent review/reply comments, so it
    catches sustained driving rather than the very first instantaneous burst; that
    first burst is bounded by the concurrency groups and the 600s cache lock.
  • The review_requested path verifies the PR author and relies on GitHub-native
    enforcement (only triage/write can request a reviewer) for the requester.
  • Not yet exercised by a live end-to-end run; the e2e workflows run on this PR.

Files

New modules: src/audit-log, src/rate-limit, src/check-staleness (each with tests).
Changed: src/check-org-membership, src/post-mention-reply, tsup.config.ts,
.github/workflows/review-pr.yml, .github/workflows/self-review-pr-trigger.yml,
SECURITY.md, review-pr/README.md.

Harden the comment and event triggered PR review pipeline against abuse on
every trigger path (review_requested, /review, @docker-agent mention, automatic
opened/synchronize, and the workflow_run fork path).

- Org membership: verify the PR author on the direct pull_request auto-review
  path (previously an empty comment author was checked) via a new resolveUsername
  helper with a live PR-author lookup fallback; fail closed when no user resolves.
- Audit log: new src/audit-log module emits one structured record per request
  (requester, time, trigger, PR, reviewed SHA, allow/deny/throttle decision) as a
  notice, a job-summary line, and a 90 day JSONL artifact, on allow and deny
  paths, with an inline fallback when setup-credentials is unavailable.
- Rate anomaly: new src/rate-limit module skips the review when too many agent
  review/reply comments land on a PR within a window; per-PR concurrency groups
  collapse same-trigger bursts; the existing cache lock still prevents concurrent
  reviews.
- Force push: new src/check-staleness module compares the requested SHA against
  current head, records the actually reviewed SHA, and posts a notice on rebase.

Documented in SECURITY.md and review-pr/README.md. Unit tests added for all new
modules.
Comment thread .github/workflows/review-pr.yml Fixed
CodeQL (actions/untrusted-checkout-toctou/critical) flagged the new
"Audit review request" run-step because it executed after "Checkout PR head"
checks out untrusted PR code in an issue_comment-privileged workflow. The audit
decision is fully determined before checkout (membership, rate, staleness), so
move the audit and audit-upload steps ahead of the checkout. No post-checkout
run-step is introduced by this change; behavior and recorded fields are
unchanged.
@Sayt-0 Sayt-0 requested a review from derekmisler June 24, 2026 12:10

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

The PR introduces three new modules (src/audit-log, src/rate-limit, src/check-staleness) and wires them into the review workflow with a clear security-first design. The changes are well-structured and the logic holds up under scrutiny.

What was reviewed:

  • src/audit-log/index.ts — JSONL artifact, job summary, audit record emission: no data-loss paths in the CLI call chain
  • src/rate-limit/index.ts — comment counting with since/updated_at vs. created_at: the explicit client-side created_at filter correctly compensates for GitHub's updated_at-based since parameter; no under-count scenario is possible since updated_at ≥ created_at always holds on GitHub
  • src/check-staleness/index.ts — SHA comparison and stale-post logic: correct
  • src/check-org-membership/index.tsresolveUsername fallback on non-event paths: the PR-author fallback on trigger/input paths is intentional design; both paths are gated by trusted callers who control the PR number, so there is no bypass
  • .github/workflows/review-pr.yml — rate check ordering, concurrency groups, needs: dependencies: correctly ordered; rate check precedes check creation as documented
  • The fail-open behavior for the rate-limit step and the audit decision logging are consistent with the documented design

No confirmed or likely bugs were found in the changed code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants