Skip to content

feat(review-pr): add rate-anomaly safeguard for review requests #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Sayt-0 merged 11 commits into from
Jun 26, 2026
Merged

feat(review-pr): add rate-anomaly safeguard for review requests #13

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5836af5
feat(review-pr): add abuse safeguards for review requests
Sayt-0 Jun 24, 2026
b69a05c
fix(review-pr): run audit step before untrusted checkout (CodeQL TOCTOU)
Sayt-0 Jun 24, 2026
ab4a2a6
Merge branch 'main' into feat/review-request-abuse-safeguards
Sayt-0 Jun 24, 2026
633b8e9
Merge branch 'main' into feat/review-request-abuse-safeguards
Sayt-0 Jun 25, 2026
7f5e3c3
refactor(review-pr): drop audit-log and staleness from abuse safeguards
Sayt-0 Jun 26, 2026
8b63e17
Merge remote-tracking branch 'origin/main' into feat/review-request-a…
Sayt-0 Jun 26, 2026
8080ae7
Merge branch 'main' into feat/review-request-abuse-safeguards
Sayt-0 Jun 26, 2026
c6bbbd3
Merge branch 'main' into feat/review-request-abuse-safeguards
Sayt-0 Jun 26, 2026
9576ee0
fix(rate-limit): count full PR reviews in the rate-anomaly window
Sayt-0 Jun 26, 2026
bf2774f
fix(rate-limit): address review comments from derekmisler
Sayt-0 Jun 26, 2026
731b522
fix(rate-limit): satisfy lint and pin clock in main() tests
Sayt-0 Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 140 additions & 3 deletions .github/workflows/review-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +83 to +95

@derekmisler derekmisler Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This actually already exists, but doesn't use GH's concurrency, and only 1 review-per-PR can run within a window of time: https://github.com/docker/docker-agent-action/blob/main/review-pr/action.yml#L142-L146

@Sayt-0 Sayt-0 Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct that a per-PR collapse already exists via the cache lock. The new concurrency: group is additive rather than equivalent: it is keyed per (PR + trigger intent), queues instead of skipping, applies before a runner spins up, and has no race window (the lock has a documented one). The trigger workflow also uses cancel-in-progress to drop superseded context-capture runs. If relying on the lock plus the rate-limit check is preferred, the group can be dropped; it is left in place for now as a cheap, race-free pre-resource layer.

@derekmisler derekmisler Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

makes sense, thanks for the explanation!


jobs:
resolve-context:
if: inputs.trigger-run-id != ''
Expand Down Expand Up @@ -315,10 +329,49 @@ jobs:
REQUESTER: ${{ github.event.sender.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

@docker-agent docker-agent Jun 26, 2026

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.

[LOW] Check rate anomaly is scoped to review requests only — @docker-agent mention floods are unthrottled

The Check rate anomaly step has:

if: |
  steps.membership.outputs.is_member == 'true' &&
  steps.command.outputs.is_review != 'false' &&
  steps.draft.outputs.skip != 'true'

When an @docker-agent mention arrives (not a /review command), steps.command.outputs.is_review is 'false', so the rate check is skipped. The reply-to-mention job (line ~785) is a separate job with no rate-limit integration at all.

This means an org member can flood @docker-agent mentions on a PR and drive up LLM inference costs with no throttle. The PR title scopes itself to "review requests", so this may be an intentional deferral — if so, a comment in the workflow noting the known gap would be helpful. If unintentional, the reply-to-mention job should also consult dist/rate-limit.js with its own window/threshold (likely a looser one, since mention replies are cheaper than full reviews).

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
Expand All @@ -345,11 +398,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
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
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
Expand All @@ -359,7 +495,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
Expand Down
9 changes: 9 additions & 0 deletions .github/workflows/self-review-pr-trigger.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' }}

@derekmisler derekmisler Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nitpick (super non-blocking): if github.event.pull_request.number is somehow absent (low probability, but possible if the event shape changes), the group key collapses to pr-review-trigger--<comment.id>, which could let two different PRs land in the same group. a || github.run_id at the end is a cheap safety net:

group: pr-review-trigger-${{ github.event.pull_request.number || github.run_id }}-${{ github.event.comment.id || 'review-request' }}

cancel-in-progress: true

permissions: {}

jobs:
Expand Down
72 changes: 72 additions & 0 deletions SECURITY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -69,6 +71,76 @@ 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`) | The PR author | `check-org-membership` resolves the PR author live via the GitHub API |
| `review_requested` via the PR sidebar | The requester | The requesting org member is verified, so an external contributor's PR can be reviewed on request. The requester is taken only from a trusted source (`github.event.sender.login` on the direct same-repo path, re-derived from the PR timeline on the fork/`workflow_run` path), never from the trigger artifact. Also relies on **GitHub-native enforcement** (only users with triage/write access can request a reviewer) and 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`
evaluates the authorization paths in order (the PR author for automatic review,
then the trusted requester for `review_requested`), resolving the PR author live
via the API so the directly-wired `pull_request` path verifies the 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
Expand Down
8 changes: 7 additions & 1 deletion review-pr/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,10 @@ with:
>
> 1. **Verifies org membership** before every review. Auto-review checks the **PR author** (so only org members' PRs are reviewed automatically); a **requested review** checks the **requester**, so a maintainer can pull an external contributor's PR into review on demand; `/review` checks the commenter
> 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

Expand All @@ -204,6 +207,9 @@ The workflow YAML examples above are the complete, recommended setup. The reusab
| **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.

Expand Down
Loading