Repository navigation
Add federated Claude PR review - #974
Conversation
🦋 Changeset detectedLatest commit: c02a492 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approving. The security posture is right and the guards are real, not decorative. Three follow-ups below, none blocking.
What I checked and agree with
- Fork boundary.
pull_requestrather thanpull_request_target, plus an explicithead.repo.full_name == github.repositoryjob condition, and the job is registered inEXPECTED_FORK_GUARDED_JOBS. The trigger set is pinned by an equality (Object.keys(triggers)), so the deadgithub.event_name != 'pull_request'disjunct cannot quietly become a bypass when somebody adds a second trigger — that test fails first. Worth keeping the equality in mind if the disjunct ever tempts anyone to delete it. - No static credentials.
anthropic_api_keyandclaude_code_oauth_tokenare asserted absent, not merely omitted. The action is SHA-pinned and the SHA is pinned again in the test.persist-credentials: falseon the checkout. - Least privilege.
contents: readat workflow level, the three scopes granted per job, andpull-requests: writejustified by the sticky comment and the inline-comment MCP tool. - Prompt construction. No user-controlled string is interpolated into the prompt — only repository, PR number and head SHA. That closes the script-injection path that usually sinks these workflows, and it is the part most PRs of this kind get wrong. The tool denylist covers the exfiltration and mutation routes.
- Housekeeping. Changeset present and correctly scoped to
stashand@cipherstash/wizard, sinceskills/ships in both tarballs.stash-supply-chain-securityis the right skill to have touched.
Follow-up 1 — the check is green and the review did not run
On this PR the action skipped:
Skipping action due to workflow validation: Workflow validation failed. The workflow file must exist and have identical content to the version on the repository's default branch.
The step then reported outcome=success and the review check passed. The vendor says this is expected when the workflow file itself is in the diff, and it is — nothing to fix here, and nothing gates on it today, since main carries no branch protection.
The durable problem is the shape. Any future PR that edits claude-review.yml gets a green review check that reviewed nothing, and nothing says so. This repository argues against precisely that elsewhere — workflow-publish-permissions.test.mjs pins the OIDC holder set because "a scan that finds none of them passes having verified nothing", and integration-workflow-paths.test.mjs carries a required floor for the same reason.
I do not think it blocks the merge, because the behaviour is the vendor's and the first real run only happens after this lands. Worth a follow-up that either fails the job on the validation skip, or records the behaviour so the next person does not read a green tick as a review.
Follow-up 2 — the classification exempts the file, not just the job
NON_PUBLISH_OIDC_JOBS correctly keeps the Claude job out of the publisher equality. But it also drops the whole file out of the "leaves the non-publishing jobs of a publishing workflow read-only" scan, because that filter now requires an OIDC holder that is not classified.
Nothing is lost today: the workflow has one job, and claude-review-workflow.test.mjs pins its permissions exactly. The gap is a second job added later with, say, contents: write and no id-token — the equality on OIDC holders would not see it, the sibling scan no longer covers the file, and no test pins the job set.
Cheapest fix is an equality on Object.keys(workflow.jobs) in the dedicated test. Alternatively keep the file inside the sibling scan and exempt only the publisher-specific assertions. Either way the reasoning in the new doc comment stays correct.
Follow-up 3 (documentation — for a separate PR)
SECURITY.md now says the test holds publishers and named non-publishing exchanges as "separate equalities". There is one equality, and it is over the union: holders.sort() against OIDC_JOBS.sort(). The two lists are separate lists feeding separate predicates, and REPO_WRITE_JOBS is a spread of the union rather than an independent equality. The distinction matters in this file specifically, because it is what a security reporter reads to work out which guard would have caught a given mistake.
Everything else in the SECURITY.md and skill wording reads accurately, including "keep its static credential inputs absent", which the test does enforce.
Note, not a finding
The four federation identifiers are echoed into a public build log by the preflight step. That is inherent to holding them in vars rather than secrets, and it is the right call — the trust sits in the OIDC token's repository and workflow claims, not in the identifiers. Flagging only so the choice is on the record as deliberate.
A prompt on a pull_request event puts claude-code-action in agent mode, which injects no PR context and creates no tracking comment. With Bash disallowed and a depth-1 checkout, the reviewer could not see what changed, and its summary was discarded while the job still reported success. Allow scoped `gh pr diff`, `gh pr view` and `gh pr comment` (edit-last, so the summary stays a single comment), drop the inert `use_sticky_comment`, and raise max-turns to fit the extra calls. Tag mode via `track_progress` was rejected because it grants git commit and push and auto-accepts edits. Also name the likely cause when the action skips on workflow validation, and correct SECURITY.md: OIDC holders are asserted against one equality over the union of the publishing and non-publishing lists.
|
Follow-up spec for running the review as parallel, skill-defined lenses (including the official |
tobyhede
changed the title
|
@freshtonic thanks. Status of the three follow-ups:
While auditing I found a bigger problem the green checks had been hiding. A prompt on a Follow-up spec for parallel, skill-defined review lenses: #997. |
claude-code-action restores CLAUDE.md and .claude/ from the base branch before Claude starts, but not the files CLAUDE.md imports. This repo's CLAUDE.md is a single `@AGENTS.md` import, so the reviewer's project instructions came from the pull request under review, which could rewrite them. Sparse-check out the base commit's AGENTS.md and copy it over the PR's copy before the action runs, removing the PR's file first so a planted symlink is replaced rather than written through. The test derives the restored set from CLAUDE.md's @imports, so a new import fails CI until it is restored too, and now requires every checkout to drop its credential.
With no `github_token` input, claude-code-action exchanges the job's OIDC token for a Claude GitHub App installation token with write access to contents, pull requests and issues. The job's `permissions:` block does not limit that token, and the action exposes it to Claude in GH_TOKEN and in the checkout's git remote URL. A prompt-injected review could leak it in a PR comment, and a leaked token could push a branch and open a same-repository PR whose workflows run with repository secrets. Pass the job's GITHUB_TOKEN instead, so contents: read and pull-requests: write are what a review can do on GitHub, and deny Claude's file tools access to .git/. The deny is partial (read-only git commands still run), so the token's scope is the real bound. The action's "workflow must match main" check is only made on the App token path, so a PR that edits this workflow now runs its own version. The fail-closed message no longer blames that skip. Correct SECURITY.md and the supply-chain skill, which described the job's permissions as the limit on this exchange. Refs #974, #997
Review summaryOne finding, posted inline:
Everything else — the fork/bot/draft guard, the OIDC federation wiring, the base-branch AGENTS.md restore to prevent instruction rewriting, the |
This reverts commit 2df0f36.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| claude_args: | | ||
| --model sonnet | ||
| --max-turns 25 | ||
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr comment:*)" |
There was a problem hiding this comment.
The Bash(gh pr diff:*), Bash(gh pr view:*), and Bash(gh pr comment:*) allowlist patterns are not scoped to the PR number under review — they match gh pr diff <any-number>, gh pr view <any-number>, and gh pr comment <any-number> ... for any PR in this repository. The prompt tells Claude to treat PR content as untrusted data and only act on the current PR, but that's an instruction, not an enforced boundary: a successful prompt injection embedded in a reviewed diff could have Claude read an unrelated PR or post/spam a comment on it, using the job's pull-requests: write token. Since the PR number is already available as ${{ github.event.pull_request.number }} (used elsewhere in this same prompt), the allowlist could be scoped to it, e.g. Bash(gh pr diff ${{ github.event.pull_request.number }}:*), to make the tool boundary match the intended scope instead of relying solely on prompt instructions.
2 participants
Summary
gh pr diff/gh pr view) and post its summary (gh pr comment --edit-last): in agent mode the action gives Claude no PR context and creates no commentValidation
pnpm test:scripts— 922 passedpnpm --dir e2e exec vitest run tests/supply-chain.e2e.test.ts— 26 passedpnpm exec turbo run typecheck— 16 tasks passedactionlint .github/workflows/claude-review.ymlpnpm exec changeset statusThe complete
pnpm testrun reached the package suites but the CLI live-testcollector could not resolve the locally unavailable
@cipherstash/eql-upgrade-baseline/sql; 90 other CLI test files passed.Follow-up: #997 (parallel review lenses defined as repo skills).