diff --git a/CLAUDE.md b/CLAUDE.md index 5379a12..23969c0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,22 +4,29 @@ Guidance for working in this repo. The [README](README.md) covers what each building block does and the consumer contract; this file captures the rules a change must not violate. -## Security ground rules — `claude-review` reusable workflow +## Security ground rules — the Claude lane reusable workflows -The PR-review workflow runs an AI agent with an org credential on a **public** -repo. These rules are load-bearing; changing any of them needs explicit review. +`claude-review.yml`, `claude-security-review.yml`, and `claude-e2e-verify.yml` +each run an AI agent with an org credential on a **public** repo, and each +workflow's `SECURITY MODEL` header block cites this file for the rationale. +These rules are load-bearing; changing any of them needs explicit review. - **`pull_request` only; never `pull_request_target` or `workflow_run` with secrets.** Those triggers run in a privileged context (base-repo secrets + write token) over potentially untrusted fork code — the "pwn request" class, - i.e. token exfiltration. The workflow keeps a tripwire step that hard-fails on - those two events. Do not whitelist a single event (that would block a + i.e. token exfiltration. Each workflow keeps a tripwire step that hard-fails + on those two events. Do not whitelist a single event (that would block a consumer's legitimate `workflow_dispatch` / `schedule`); reject only the dangerous two. - **Fork PRs are intentionally not reviewed.** GitHub passes no secrets and a - read-only token to fork-triggered `pull_request` runs, so the review degrades - to a warning. That is the safety guarantee, not a bug — never "fix" it by - reaching for `pull_request_target`. + read-only token to fork-triggered `pull_request` runs, so the agent cannot + run. `claude-review` and `claude-e2e-verify` let it degrade to a warning; + `claude-security-review` skips the job at job level instead, because its + fail-closed mapping would otherwise pin every fork PR red for a cause no push + can fix — and because that skip reads as success, its required check does not + prove a fork PR was reviewed, so fork changes to security-sensitive surfaces + need a human. The no-secrets guarantee is the safety property, not a bug — + never "fix" it by reaching for `pull_request_target`. - **SHA-pin both layers.** Consumers pin this reusable workflow `@<40-char-sha>`; this workflow pins `anthropics/claude-code-action@` (mutable tags were the tj-actions/CVE-2025-30066 vector). Dependabot bumps the inner pin. @@ -27,7 +34,7 @@ repo. These rules are load-bearing; changing any of them needs explicit review. the caller's `GITHUB_TOKEN` grant, so the consumer's caller job must grant exactly `contents: read` + `pull-requests: write` + `id-token: write` (`id-token` mints the Claude GitHub App token and is required even with an - OAuth/API-key credential). The review job re-declares that minimal set so an + OAuth/API-key credential). Each lane's job re-declares that minimal set so an over-granting caller is still narrowed here. - **Never touch the org secret.** `CLAUDE_CODE_OAUTH_TOKEN` is an org secret with visibility *all repositories* — the live, deliberate value. Never @@ -35,15 +42,33 @@ repo. These rules are load-bearing; changing any of them needs explicit review. selected-repositories. Forced-failure testing uses a repo-level same-name override in a sandbox repo, never a change to the org secret. - **Public-repo log hygiene.** Keep `display_report` and `show_full_output` off - (defaults) — both can surface model-authored content or secrets in publicly - visible logs. Never enable Actions debug (`ACTIONS_STEP_DEBUG`) here. Never - echo the token. -- **No untrusted checkout before the action.** Do not check out a PR head ref - into the workspace root ahead of the action step. + — both can surface model-authored content or secrets in publicly visible + logs. No lane sets `show_full_output`, so it stays at the action's off + default. `display_report` is enforced unevenly, and deliberately so: + `claude-security-review` hardcodes it `false` so a consumer cannot turn it + on, `claude-e2e-verify` never sets it, and `claude-review` exposes it as an + input for private consumers — **a public caller must never set + `display-report: true`**. Never enable Actions debug (`ACTIONS_STEP_DEBUG`) + here. Never echo the token. +- **Never execute PR-authored code in a review job.** All three lanes check out + before the action step, and on `pull_request` the default checkout resolves to + the PR merge ref, so PR-authored file content is already in the workspace root + — presence is not the hazard, execution is. Never point a checkout at the PR + head ref, and never run a build, install script, or test command against that + tree in a review lane. `claude-e2e-verify` is the + deliberate exception, because building, serving, and browser-driving the PR + head *is* that lane: it runs PR-authored code on the runner alongside the + persisted `GITHUB_TOKEN` and the agent's credential. That residual is + accepted, not mitigated in-job; the reasoning, its bound (the + `pull_request`-only tripwire plus the fork-PR no-secrets guarantee), and the + hardening direction if the lane's scope widens are recorded in that + workflow's own header. Do not widen it, and do not carry the pattern into a + review lane. The `secrets:`/`vars:` context is unavailable in composite actions; that, plus -the need for job-level `permissions` and the `secrets:` interface, is why review -is a reusable workflow and the tool-runner lanes are composite actions. +the need for job-level `permissions` and the `secrets:` interface, is why each +Claude lane is a reusable workflow and the tool-runner lanes are composite +actions. ## Pin everything by SHA diff --git a/README.md b/README.md index 0c3ee60..37b88c7 100644 --- a/README.md +++ b/README.md @@ -636,27 +636,38 @@ GitHub continues the normal weekly patching of each hosted image generation. repo, so the lane scopes itself to security-sensitive surfaces — but the caller must NOT express that scope with a workflow-level `on.pull_request.paths` filter, because a path miss leaves a required check Pending forever and wedges - every prose PR. Instead the caller triggers on all PR events and passes the - `paths` input (security-sensitive surfaces in Actions `paths:` syntax: - workflow files, permission/settings configs, hook and shell scripts, - auth/token-touching code, network-call sites); the workflow's `changes` job - evaluates it and a not-applicable PR yields a name-stable skipped - `security-review` check. A consumer's ruleset may make that EXECUTION check - required (check context ` / security-review`); the VERDICT stays - advisory. All inputs have public-safe defaults documented inline in the - workflow header (the authoritative list). Consume it per the [Claude + every prose PR. Instead the caller triggers on all PR events and supplies that + scope as a pattern list of root-anchored globs (workflow files, + permission/settings configs, hook and shell scripts, auth/token-touching code, + network-call sites); the workflow's `changes` job evaluates it and a + not-applicable PR yields a name-stable skipped `security-review` check. A + consumer's ruleset may make that EXECUTION check required (check context + ` / security-review`); the VERDICT stays advisory. + + **Where that pattern list lives** is the caller's choice between two inputs. + The conventional shape is `paths-file`, pointing at a repo-owned file + (`.github/claude-security-paths`) so each repo keeps its own + security-sensitive-surface list in its own tree; the inline `paths` input + takes the same content directly and, when non-empty, wins over the file. The + file is read from the PR's **base** branch, never the head, so a PR cannot + edit its content to skip its own security review — repointing the input is + still possible, but only as a visible caller diff, which is the pre-existing + trust boundary. An absent or unreadable file **fails open** (every PR + reviewed, with a warning), matching the `changes` job's fail-open discipline + throughout; both inputs empty means no filtering, so a consumer that passes + nothing is unaffected. Patterns are matched as root-anchored **gitignore** + patterns rather than Actions `paths:` patterns — identical for the ordinary + `*` / `**` globs worth writing here, but `!` negation cannot be honored as + Actions defines it and is rejected outright, so express an exclusion by + narrowing the positive patterns. All inputs have public-safe defaults + documented inline + in the workflow header (the authoritative list). Consume it per the [Claude lanes — shared consumption contract](#claude-lanes--shared-consumption-contract) - below, triggering on all PR events (no workflow-level `paths:`) and passing - the `paths` input in the job: + below, triggering on all PR events (no workflow-level `paths:`): ```yaml with: - paths: | - .github/workflows/** - .github/actions/** - **/*.sh - **/*.ps1 - # plus the caller's own auth/token and network-call source paths + paths-file: .github/claude-security-paths ``` - `.github/workflows/claude-e2e-verify.yml` — Claude-powered end-to-end @@ -878,26 +889,19 @@ GitHub continues the normal weekly patching of each hosted image generation. `claude-review.yml`, `claude-security-review.yml`, and `claude-e2e-verify.yml` share one consumption shape. Each is **advisory**: it posts PR comments and never gates `ci-status`. (The advisory verdict is separate from execution -evidence: `claude-security-review.yml` additionally takes a `paths` input and -its name-stable `security-review` check may be made a required status check — -see its entry above.) Each is a whole-job concern (job `permissions:` plus a -`secrets:` interface), which is why each is a reusable workflow rather than a -composite action — the caller owns the triggers and the permission grant, and -the workflow owns the SHA-pinned `anthropics/claude-code-action` and the safe -handling. Security rules live in [CLAUDE.md](CLAUDE.md). +evidence: `claude-security-review.yml` scopes itself to security-sensitive +paths, and its name-stable `security-review` check may be made a required +status check — see its entry above.) Each is a whole-job concern (job +`permissions:` plus a `secrets:` interface), which is why each is a reusable +workflow rather than a composite action — the caller owns the triggers and the +permission grant, and the workflow owns the SHA-pinned +`anthropics/claude-code-action` and the safe handling. Security rules live in +[CLAUDE.md](CLAUDE.md). ```yaml on: pull_request: - # Trigger types are PER LANE — copy the canonical caller from the lane's - # own workflow header, not this line: - # claude-review: [opened, ready_for_review, reopened] - # (no `synchronize` — pushes do not re-trigger the code review; - # re-run the job for a fresh pass) - # claude-security-review: [opened, synchronize, ready_for_review, reopened] - # (its check certifies execution at the merge head) - # claude-e2e-verify: [opened, synchronize, ready_for_review, reopened] - types: [opened, synchronize, ready_for_review, reopened] + types: [] jobs: : permissions: @@ -913,7 +917,79 @@ The caller's job must grant those three permissions (a called workflow can only downgrade, not elevate); the `CLAUDE_CODE_OAUTH_TOKEN` org secret has visibility "all repositories", so every org repo receives it. Pass that one named secret explicitly rather than `secrets: inherit`, which forwards every -parent secret. Fork PRs receive no secrets by design and are not reviewed. +parent secret. Fork PRs receive no secrets by design and are not reviewed. On +both review lanes the caller can name actors whose comments are withheld from +the agent's context — prompt-injection hygiene, not a trigger gate. + +**What the security lane's required check proves — and does not.** Only +`claude-security-review`'s check is designed to be required: it reports under +its own context (` / security-review`), never through `ci-status`. +Its claim is narrow: a security pass RAN at this head, or the PR was judged not +applicable. The verdict stays advisory — findings never fail the job. Execution +is what **fails closed**: when the PR is in scope and the review could not run +at all (usage limit, dead credential, SDK crash), the check reports FAILURE, +because `success`, `neutral` and `skipped` all satisfy a required check, so +failure is the only conclusion that does not silently authorize a merge on +absent evidence. That mapping is scoped to `pull_request` runs; a non-PR event +cannot run the review at all and keeps the historical pass-through, and a fork +PR skips the job outright — neither green is execution evidence. + +The corollary is what the check does not prove. Every legitimate non-run reads +as success to a ruleset. Four are name-stable job-level skips: a fork PR, an +out-of-scope PR, a skip-listed actor, a kill-switched lane. A fifth is not a +skip at all — a run whose head was superseded while it queued retires itself +step by step and reports a **green** job having reviewed nothing, on the +premise that the newer run for the current head reports the same context. So a +green required check is not by itself proof that this head was reviewed, and it +never establishes that a fork PR was: review fork changes to security-sensitive +surfaces by hand. Availability is bought back by the bounded retry below, and a +sustained outage is meant to be handled by an explicit, logged, attributable +break-glass on the consumer's ruleset — never by weakening the check. + +**Trigger cadence is per lane, deliberately.** `claude-review` runs on +`opened` / `ready_for_review` / `reopened` and **not** on `synchronize`: a push +does not re-trigger the code review, so re-run the job for a fresh pass. That +caps per-PR spend on active branches, and it is safe precisely because the +lane's verdict gates nothing. It also skips draft PRs at job level, so an +`opened` event on a draft costs nothing and `ready_for_review` is what buys the +review. `claude-security-review` keeps `synchronize`, because its check +certifies that a security pass ran at the head being merged — a review of an +earlier head is not that evidence, and it reviews drafts. `claude-e2e-verify` +keeps `synchronize` too, and gates on nothing but its kill-switches — no draft +skip, no `skip-actors` input — so the most expensive lane has the loosest gate. +Scope it with the caller's own trigger types. Take each lane's canonical caller +from its own workflow header. + +**Bounded retry.** Every lane makes at most **two** agent attempts — one +automatic retry, never a loop. The retry is deliberately narrow, because a +second attempt after the agent has already spoken duplicates its comments: it +fires only on **zero assistant turns** in the first attempt's execution file. +Nor does an **auth-class** failure retry — HTTP 401/402/403, or an +`authentication_error` / `billing_error` / `permission_error` in the error +payload — because the credential needs an operator and no retry can clear it. +The gate also honors the same guards the first attempt does, so a superseded +run (and, on the code-review lane, a capped one) never spends a retry. Between +the attempts the lane backs off `retry-delay-seconds` plus a 0–29 second +jitter, so lanes retrying against the same contended seat do not re-collide in +lockstep. What this buys is availability against the sporadic-429 class without +weakening the security lane's fail-closed execution claim. + +One divergence is worth knowing. The two review lanes demand **proof** of zero +turns: a missing or unparsable execution file is not proof — a hard kill can +lose the file after turns were already spent — so they do not retry on one. +`claude-e2e-verify` reads an unreadable file as recording no assistant turn and +does retry. It also sets no `track_progress` tracking comment, so it has no +orphan comment to clean up between attempts, which the review lanes do. + +**Review-count cap (code-review lane only).** `claude-review.yml` stops +reviewing a PR after `max-reviews-per-pr` successful reviews, capping spend on +long-lived PRs. The counter is a **visible** per-PR status comment upserted +after each successful review — failed and skipped runs never inflate it — which +doubles as the human "was this reviewed" signal; +deleting it resets the count, which is fail-open by design. A capped run is a +name-stable skip, not a red check. Treat it as a soft cap: concurrent runs for +different heads read the counter before either writes it, so a burst can +briefly exceed it by the number of concurrent heads. **Kill-switches.** Every lane honors two Actions variables at job level: `CLAUDE_LANES_DISABLED` (all lanes) and a per-lane switch @@ -928,6 +1004,22 @@ disabled NO lane reports security findings — REVIEW.md's code-review exclusion keys on the security workflow file existing, and the file remains — so re-enable promptly and treat the outage window as security-unreviewed. +All four organization variables carry **all-repositories** visibility. That is +a deliberate deviation from the org's selected-visibility convention for +Actions variables, not an oversight: a switch scoped to a selection is invisible +to every repo outside it, so flipping it during an incident would silently +no-op exactly where nobody is looking. A kill-switch is only worth having if it +reaches the whole fleet. Do not "correct" the visibility to selected. + +**Adoption.** Each lane's own workflow header carries its canonical caller; +copy it from there. Callers are additionally being brought under the org's +sync-managed component distribution in +[`melodic-software/standards`](https://github.com/melodic-software/standards), +which materializes a canonical per-lane caller into every target repo and keeps +it current through the ordinary sync PR. Where a repo's caller is +sync-managed, change it at the component source and let the sync carry it to +every target — never edit the materialized caller in the target repo. + ## Standalone gate checks — shared adoption contract `semantic-pr.yml`, `pr-issue-linkage.yml`, and `do-not-merge-gate.yml` are