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

.github/workflows/review-pr.yml

@@ -80,6 +80,20 @@ permissions:
   id-token: write
   actions: read   # download-artifact across workflow_run boundary
 
+# Rate-anomaly safeguard: serialize same-trigger bursts on a PR (e.g. rapid
+# force-pushes firing repeated auto-reviews) instead of running N in parallel.
+# The group is keyed per PR AND per trigger intent: the comment id (or event
+# name) suffix keeps distinct comments/replies in distinct groups, so a quick
+# conversational reply is never queued behind a 45-minute review. cancel-in-
+# progress is false so an in-flight review/reply is never killed mid-post.
+# Per-PR request *frequency* is enforced by the rate-limit check below, and the
+# in-action cache lock (review-pr/action.yml) prevents concurrent reviews; the
+# workflow_run/fork path (PR number only in the artifact) falls back to a per-run
+# group, where those two mechanisms still bound abuse.
+concurrency:
+  group: pr-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.pr-number || github.run_id }}-${{ github.event.comment.id || github.event_name }}
+  cancel-in-progress: false
+
 jobs:
   resolve-context:
     if: inputs.trigger-run-id != ''
@@ -305,12 +319,55 @@ jobs:
           PR_SOURCE: ${{ steps.pr.outputs.source }}
           ORG: docker
           COMMENT_AUTHOR: ${{ github.event.comment.user.login }}
+          # PR author for the direct pull_request auto-review path, where there is
+          # no comment author. Without this the membership check ran against an
+          # empty login and the PR author was never actually verified.
+          PR_AUTHOR: ${{ github.event.pull_request.user.login }}
         run: node "$DOCKER_AGENT_ACTION_ROOT/dist/check-org-membership.js"
 
+      # Rate-anomaly safeguard: count how many docker-agent review/reply comments
+      # were posted on this PR in the recent window. An authorized account can
+      # still drive the bot at high frequency (each request costs an LLM run), so
+      # a burst above the threshold is flagged and the expensive review is skipped.
+      # Runs BEFORE "Create check run" so a throttled request creates no check run
+      # (a skipped review must not surface as a green "PR Review" check).
+      - name: Check rate anomaly
+        id: rate
+        if: |
+          steps.membership.outputs.is_member == 'true' &&
+          steps.command.outputs.is_review != 'false' &&
+          steps.draft.outputs.skip != 'true'
+        continue-on-error: true   # fail-open: a rate-check error must not block reviews
+        shell: bash
+        env:
+          GITHUB_TOKEN: ${{ env.GITHUB_APP_TOKEN || github.token }}
+          RATE_PR_NUMBER: ${{ steps.pr.outputs.number }}
+        run: node "$DOCKER_AGENT_ACTION_ROOT/dist/rate-limit.js"
+
+      # Force-push safeguard: compare the SHA the review was requested for against
+      # the current PR head. The review still runs against the latest commit
+      # (refs/pull/N/head); this records the SHA actually reviewed and posts a
+      # notice when the branch was force-pushed/rebased after the request.
+      - name: Check force-push staleness
+        id: staleness
+        if: |
+          steps.membership.outputs.is_member == 'true' &&
+          steps.command.outputs.is_review != 'false' &&
+          steps.draft.outputs.skip != 'true' &&
+          steps.rate.outputs.anomalous != 'true'
+        continue-on-error: true   # fail-open: a staleness-check error must not block reviews
+        shell: bash
+        env:
+          GITHUB_TOKEN: ${{ env.GITHUB_APP_TOKEN || github.token }}
+          STALE_PR_NUMBER: ${{ steps.pr.outputs.number }}
+          STALE_REQUESTED_SHA: ${{ needs.resolve-context.outputs.pr-head-sha || github.event.pull_request.head.sha }}
+        run: node "$DOCKER_AGENT_ACTION_ROOT/dist/check-staleness.js"
+
       - name: Create check run
         if: |
           (steps.pr.outputs.source == 'event' || steps.pr.outputs.source == 'trigger') &&
-          steps.membership.outputs.is_member == 'true'
+          steps.membership.outputs.is_member == 'true' &&
+          steps.rate.outputs.anomalous != 'true'
         id: create-check
         continue-on-error: true
         uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
@@ -337,11 +394,94 @@ jobs:
             });
             core.setOutput('check-id', check.id);
 
+      # Audit safeguard: emit one structured record per review request that joins
+      # the requester, time, trigger, PR, reviewed SHA, and the authorize/deny/
+      # throttle decision. Runs on the allow AND deny paths so denied, throttled,
+      # and stale requests are all recorded, not just successful ones. If
+      # setup-credentials failed (no dist bundle / DOCKER_AGENT_ACTION_ROOT), the
+      # record is still emitted inline so even infra-failure denials are logged.
+      # Placed BEFORE "Checkout PR head" on purpose: the decision is fully known by
+      # now, and keeping this run-step ahead of the untrusted checkout avoids an
+      # execute-after-untrusted-checkout (TOCTOU) pattern.
+      - name: Audit review request
+        if: |
+          always() &&
+          steps.command.outputs.is_review != 'false' &&
+          steps.draft.outputs.skip != 'true'
+        continue-on-error: true
+        shell: bash
+        env:
+          IS_MEMBER: ${{ steps.membership.outputs.is_member }}
+          RATE_ANOMALOUS: ${{ steps.rate.outputs.anomalous }}
+          STALE: ${{ steps.staleness.outputs.stale }}
+          CURRENT_SHA: ${{ steps.staleness.outputs.current-sha }}
+          REQUESTED_SHA: ${{ needs.resolve-context.outputs.pr-head-sha || github.event.pull_request.head.sha }}
+          USER_REQUESTED: ${{ steps.trigger-type.outputs.user_requested }}
+          AUDIT_ACTOR: ${{ github.event.comment.user.login || github.event.sender.login || github.actor }}
+          AUDIT_PR_NUMBER: ${{ steps.pr.outputs.number }}
+          AUDIT_EVENT: ${{ github.event_name }}
+          REVIEW_AUDIT_FILE: ${{ runner.temp }}/review-audit/review-audit.jsonl
+        run: |
+          if [ "$IS_MEMBER" != "true" ]; then
+            DECISION=denied
+            REASON="requester is not a docker org member"
+          elif [ "$RATE_ANOMALOUS" = "true" ]; then
+            DECISION=throttled
+            REASON="rate anomaly: too many recent review requests on this PR"
+          else
+            DECISION=authorized
+            if [ "$STALE" = "true" ]; then
+              REASON="docker org member (PR head moved after request — reviewing latest commit)"
+            else
+              REASON="docker org member"
+            fi
+          fi
+          export AUDIT_DECISION="$DECISION"
+          export AUDIT_REASON="$REASON"
+          export AUDIT_TRIGGER="$([ "$USER_REQUESTED" = "true" ] && echo user-requested || echo automatic)"
+          export AUDIT_HEAD_SHA="${CURRENT_SHA:-$REQUESTED_SHA}"
+          export AUDIT_REQUESTED_SHA="$REQUESTED_SHA"
+
+          # If setup-credentials failed (OIDC/Secrets Manager misconfig), the dist
+          # bundle and DOCKER_AGENT_ACTION_ROOT are never set. Still record the
+          # denial inline so the "log every request, including denials" guarantee
+          # holds even on infra-failure paths.
+          if [ -z "$DOCKER_AGENT_ACTION_ROOT" ] || [ ! -f "$DOCKER_AGENT_ACTION_ROOT/dist/audit-log.js" ]; then
+            TS=$(date -u +%Y-%m-%dT%H:%M:%SZ)
+            REC=$(jq -nc \
+              --arg ts "$TS" --arg event "$AUDIT_EVENT" --arg trigger "$AUDIT_TRIGGER" \
+              --arg actor "${AUDIT_ACTOR:-unknown}" --arg repo "$GITHUB_REPOSITORY" \
+              --arg pr "$AUDIT_PR_NUMBER" --arg head "$AUDIT_HEAD_SHA" \
+              --arg requested "$AUDIT_REQUESTED_SHA" --arg decision "$AUDIT_DECISION" \
+              --arg reason "$AUDIT_REASON (audit-log bundle unavailable — emitted inline)" \
+              '{timestamp:$ts,event:$event,trigger:$trigger,actor:$actor,repository:$repo,prNumber:$pr,headSha:$head,requestedSha:$requested,decision:$decision,reason:$reason}')
+            echo "::notice title=Review request audit::[review-request-audit] $REC"
+            mkdir -p "$(dirname "$REVIEW_AUDIT_FILE")"
+            printf '%s\n' "$REC" >> "$REVIEW_AUDIT_FILE" || true
+            exit 0
+          fi
+
+          node "$DOCKER_AGENT_ACTION_ROOT/dist/audit-log.js" || echo "::warning::audit-log failed"
+
+      - name: Upload review audit log
+        if: |
+          always() &&
+          steps.command.outputs.is_review != 'false' &&
+          steps.draft.outputs.skip != 'true'
+        continue-on-error: true
+        uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
+        with:
+          name: pr-review-audit-${{ github.run_id }}-${{ github.run_attempt }}
+          path: ${{ runner.temp }}/review-audit/
+          retention-days: 90
+          if-no-files-found: ignore
+
       - name: Checkout PR head
         if: |
           steps.membership.outputs.is_member == 'true' &&
           steps.command.outputs.is_review != 'false' &&
-          steps.draft.outputs.skip != 'true'
+          steps.draft.outputs.skip != 'true' &&
+          steps.rate.outputs.anomalous != 'true'
         uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
         with:
           fetch-depth: 0
@@ -351,7 +491,8 @@ jobs:
         if: |
           steps.membership.outputs.is_member == 'true' &&
           steps.command.outputs.is_review != 'false' &&
-          steps.draft.outputs.skip != 'true'
+          steps.draft.outputs.skip != 'true' &&
+          steps.rate.outputs.anomalous != 'true'
         id: run-review
         continue-on-error: true
         uses: docker/docker-agent-action/review-pr@e96a4bb40cac114f64358621e1d08346c8eadc8c # v2.0.1

.github/workflows/self-review-pr-trigger.yml

@@ -8,6 +8,15 @@ on:
   pull_request_review_comment:
     types: [ created ]
 
+# Rate-anomaly safeguard: collapse a burst of review_requested events on the same
+# PR at the source — re-requesting a review is redundant, so cancel-in-progress
+# drops superseded context-capture runs and only the latest fans out to a review.
+# Distinct review comments stay independent (the comment id keys them into their
+# own group) so a real reply is never dropped by another comment on the same PR.
+concurrency:
+  group: pr-review-trigger-${{ github.event.pull_request.number }}-${{ github.event.comment.id || 'review-request' }}
+  cancel-in-progress: true
+
 permissions: {}
 
 jobs:

SECURITY.md

@@ -26,6 +26,8 @@ This action includes **built-in security features for all agent executions**:
    - **Suspicious patterns** (strip + warn): Behavioral/natural-language injection attempts ("ignore previous instructions", "system mode", "reveal the token", base64/hex obfuscation, etc.) — matching lines are removed from the prompt before it reaches the agent
    - **Medium-risk patterns** (warn only): API key variable names in configuration (`ANTHROPIC_API_KEY`, `GITHUB_TOKEN`, etc.)
 
+4. **Review-Request Abuse Safeguards** — the comment/event-triggered PR review pipeline is hardened against abuse on every trigger path: org-membership validation, a structured audit log of every request (including denials), rate-anomaly throttling, and force-push staleness handling. See [Review-Request Abuse Safeguards](#review-request-abuse-safeguards).
+
 ## Security Architecture
 
 The action implements a defense-in-depth approach:
@@ -69,6 +71,74 @@ The action implements a defense-in-depth approach:
 └────────────────────────────────────────────────────────────────┘
 ```
 
+## Review-Request Abuse Safeguards
+
+The PR review pipeline (`.github/workflows/review-pr.yml` and the `review-pr/`
+composite actions) is comment- and event-triggered, which makes it the main
+abuse vector for cost/spam. Four safeguards harden the review-request flow on
+every trigger path (`review_requested`, the deprecated `/review` comment,
+`@docker-agent` mentions, automatic `opened`/`synchronize`, and the
+`workflow_run` fork path).
+
+### 1. Review requests come from org members
+
+Membership is verified before any review work runs, and which actor is checked
+depends on the trigger:
+
+| Trigger | Actor verified | Mechanism |
+|---------|----------------|-----------|
+| `/review` comment, `@docker-agent` mention, reply-to-feedback | The commenter | `check-org-membership` against the `docker` org (OIDC `org-membership-token`) |
+| Automatic review (`opened`/`synchronize`/`ready_for_review`) and `review_requested` | The PR author | `check-org-membership` (`PR_AUTHOR` env, with a live PR-author API lookup as fallback) |
+| `review_requested` via the PR sidebar | The requester | **GitHub-native enforcement** — only users with triage/write access can request a reviewer; the workflow additionally gates on `requested_reviewer.login == 'docker-agent'` |
+
+The membership check **fails closed**: if no actor login can be resolved, or the
+actor is not an org member, the review is skipped. `src/check-org-membership`
+resolves the actor via `resolveUsername` so the directly-wired `pull_request`
+auto-review path verifies the PR author instead of an empty comment author.
+
+### 2. All review requests are logged
+
+`src/audit-log` emits one structured audit record per review request — on the
+allow path **and** on every denial/throttle path — joining the fields needed to
+investigate abuse after the fact: requester login, ISO-8601 timestamp, trigger
+type, repository, PR number, reviewed head SHA (and requested SHA when they
+differ), and the `authorized | denied | throttled | skipped | stale` decision
+with a reason. Each record is emitted three ways so it survives even when raw
+run logs are gone:
+
+- a `core.notice` annotation prefixed `[review-request-audit]` (queryable via the
+  annotations API and greppable in logs),
+- a line appended to the job summary (`$GITHUB_STEP_SUMMARY`), and
+- an append-only JSONL line uploaded as the `pr-review-audit-*` artifact
+  (90-day retention).
+
+### 3. Rate anomalies are detected and throttled
+
+Authorization gates *who* may trigger a review, not *how often*. Three layers
+bound request frequency:
+
+- **Per-PR `concurrency:` groups** on `review-pr.yml` and the trigger workflow
+  collapse same-trigger bursts (e.g. rapid force-pushes, repeated review
+  requests) instead of running them in parallel. Groups are scoped per trigger
+  intent so a quick conversational reply is never queued behind a long review.
+- **`src/rate-limit`** counts docker-agent review/reply comments posted on the PR
+  within a sliding window (default 600 s) and, when the count crosses a threshold
+  (default 8), flags a rate anomaly. The review job skips the expensive review and
+  records a `throttled` audit entry. The check **fails open** so an API error
+  never blocks a legitimate review.
+- The existing in-action **cache lock** (`pr-review-lock-<repo>-<pr>-*`, 600 s
+  TTL) prevents concurrent reviews from racing on the same PR.
+
+### 4. Force-pushed / rebased commits are handled
+
+A review always checks out `refs/pull/N/head` (the current head), but the head
+SHA captured when the review was requested may differ after a force-push/rebase.
+`src/check-staleness` re-fetches the current head and compares it to the
+requested SHA. The review proceeds against the latest commit (the freshest code
+is what should be reviewed); the SHA actually reviewed is recorded in the audit
+log and a `core.notice` is posted when the branch moved, so a force-push during
+the request window is visible rather than silent.
+
 ## Security Modules
 
 All security logic lives under `src/security/` and is compiled into `dist/security.js` by

review-pr/README.md

@@ -175,9 +175,12 @@ with:
 
 > **Built-in defense-in-depth:**
 >
-> 1. **Verifies org membership** before every review (auto-review checks the PR author; `/review` checks the commenter)
+> 1. **Verifies org membership** before every review. Comment/mention/reply triggers check the commenter; auto-review and `review_requested` check the PR author. Requesting a review from `docker-agent` in the sidebar additionally relies on GitHub-native enforcement (only users with triage/write access can request reviewers).
 > 2. **Prevents bot cascades** — replies from bots (except `docker-agent`) are ignored
-> 3. **Fork PRs work automatically** with the two-workflow setup — the trigger → `workflow_run` pattern provides OIDC/secret access regardless of fork status
+> 3. **Logs every review request** — one structured audit record per request (requester, time, trigger, PR, reviewed SHA, allow/deny/throttle decision) is emitted as a notice, a job-summary line, and a 90-day audit artifact, including on denial paths
+> 4. **Throttles rate anomalies** — per-PR `concurrency:` groups collapse same-trigger bursts, and a rate-limit check skips the review when too many requests land on one PR in a short window
+> 5. **Handles force-pushes** — when a PR is rebased/force-pushed after a review is requested, the review runs against the latest commit and the actually-reviewed SHA is recorded with a notice
+> 6. **Fork PRs work automatically** with the two-workflow setup — the trigger → `workflow_run` pattern provides OIDC/secret access regardless of fork status
 
 ### What you don't need to add
 
@@ -186,10 +189,13 @@ The workflow YAML examples above are the complete, recommended setup. The reusab
 | Protection | How it's handled |
 | ---------- | ---------------- |
 | **Bot comment filtering** | All jobs in the reusable workflow filter out `docker-agent`, `docker-agent[bot]`, any `Bot`-type user, and comments with `<!-- docker-agent-review -->`/`<!-- docker-agent-review-reply -->` markers. No caller-side filtering needed. |
-| **Org membership / authorization** | A `check-org-membership` step runs before any review work. PR authors and comment authors are verified as org members or collaborators via OIDC. Callers never need `author_association` checks. |
+| **Org membership / authorization** | A `check-org-membership` step runs before any review work. On comment/mention/reply triggers it verifies the commenter; on auto-review and `review_requested` it verifies the PR author (falling back to a live PR-author lookup when no author login is in the event). Callers never need `author_association` checks. |
 | **PR vs issue comment** | The reusable workflow checks `github.event.issue.pull_request` internally. Plain issue comments on non-PR issues are silently ignored. |
 | **Draft PR skipping** | Draft PRs are skipped internally — no caller condition needed. |
 | **Concurrent review guard** | A cache-based lock (`pr-review-lock-<repo>-<pr>-*`) prevents duplicate reviews from racing on the same PR. |
+| **Rate-anomaly throttling** | Per-PR `concurrency:` groups serialize same-trigger bursts, and a rate-limit check skips the review when too many docker-agent review/reply comments land on one PR within the window. No caller configuration needed. |
+| **Review-request audit log** | Every request is recorded (notice + job summary + 90-day `pr-review-audit-*` artifact) with requester, trigger, PR, reviewed SHA, and the allow/deny/throttle decision — including denied and throttled requests. |
+| **Force-push handling** | The SHA a review was requested for is compared to the current head; the review runs against the latest commit and the reviewed SHA is recorded so a force-push mid-flight is visible rather than silent. |
 
 **The only decision callers make** is which setup pattern to use: 1-workflow for same-repo PRs, 2-workflow for repos that accept fork PRs. That distinction is the caller's responsibility because it controls which event path delivers OIDC credentials to the reusable workflow.