Skip to content

Commit 8365ea3

Browse files
cliffhallclaude
andcommitted
ci(claude): apply inspector PR review findings to fork-review job
Cross-applied from the review on the parallel inspector PR (modelcontextprotocol/inspector#1339). Findings 1, 3, 4, 6 from modelcontextprotocol/inspector#1339 (comment) Inspector finding 1 (defense-in-depth): expand strip step beyond .mcp.json. Renamed "Prepare trusted MCP config" → "Strip fork-supplied CLI config + write trusted MCP config" and expanded the find sweep to cover the full set of files that Claude Code (and the action's restoreConfigFromBase) auto-discover from the working directory: files: .mcp.json .claude.json .gitmodules .ripgreprc CLAUDE.md CLAUDE.local.md dirs: .claude .husky The highest-impact vector in that set is `.claude/settings.json` — its `SessionStart` / `PreToolUse` hooks execute arbitrary shell BEFORE any --allowedTools allowlist applies. A fork shipping such hooks could exfiltrate ANTHROPIC_API_KEY (still in process env at hook time) or anything else on the runner. The action's restoreConfigFromBase currently neutralizes this in v1.0.99, but that's an internal implementation detail; if a future bump narrows or removes that behavior — the same regression class we SHA-pin against — the hole silently reopens. Stripping here is defense-in-depth that holds regardless of what the action does internally. The comment block on the step now leads with the hooks threat and explains the defense-in-depth framing. Inspector finding 3: add concurrency group to the fork-review job. concurrency: group: claude-fork-review-${{ github.event.pull_request.number }} cancel-in-progress: false Prevents label→unlabel→relabel races from spawning parallel reviews on the same PR (duplicate comments, doubled API spend, label-removal race). `cancel-in-progress: false` so an in-flight review finishes and removes its own label cleanly. Inspector finding 4: drop `issues: read` from the fork-review permissions block. Label removal on a PR is covered by `pull-requests: write` alone — issues: read was dead. Inspector finding 6 (phrasing): reworded "the only outbound HTTP this job can make is to modelcontextprotocol.io" → "the only outbound HTTP Claude can direct". The runner itself still talks to api.anthropic.com, api.github.com, the Actions cache, etc.; the claim is about Claude's tool surface, not the runner. Same fix applied to the job-header threat-model comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8107aeb commit 8365ea3

1 file changed

Lines changed: 73 additions & 31 deletions

File tree

.github/workflows/claude.yml

Lines changed: 73 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -175,13 +175,15 @@ jobs:
175175
# - Mitigations: (a) no Bash glob, no Edit, no WebFetch — Claude can
176176
# only post inline comments, read PR metadata via narrow `gh`
177177
# commands, and query the MCP docs server at modelcontextprotocol.io
178-
# (the only network egress; fork's `.mcp.json` is removed and
179-
# replaced with a base-repo-controlled config — see the "Prepare
180-
# trusted MCP config" step); (b) no fork code is ever executed (no
181-
# install, build, or test steps); (c) the GITHUB_TOKEN is scoped to
182-
# pull-requests:write only; (d) the ANTHROPIC_API_KEY exists in the
183-
# runner env but isn't reachable through Claude's allowed tool
184-
# surface.
178+
# (the only outbound HTTP Claude can direct; the fork's
179+
# auto-discovered CLI config — `.claude/`, `.mcp.json`, `CLAUDE.md`,
180+
# etc., with `.claude/settings.json` hooks as the highest-impact
181+
# vector — is stripped and replaced with a base-repo-controlled MCP
182+
# config; see the "Strip fork-supplied CLI config" step); (b) no
183+
# fork code is ever executed (no install, build, or test steps);
184+
# (c) the GITHUB_TOKEN is scoped to pull-requests:write only;
185+
# (d) the ANTHROPIC_API_KEY exists in the runner env but isn't
186+
# reachable through Claude's allowed tool surface.
185187
#
186188
# NOTE: this repo (modelcontextprotocol/servers) hosts many independent
187189
# MCP server implementations as subdirectories. The system prompt asks
@@ -193,10 +195,18 @@ jobs:
193195
github.event_name == 'pull_request_target' &&
194196
github.event.label.name == 'claude-review'
195197
runs-on: ubuntu-latest
198+
# Prevent label→unlabel→relabel races (or rapid label flips by a
199+
# maintainer) from spawning parallel reviews on the same PR. Each
200+
# parallel run would consume API budget, post duplicate comments,
201+
# and race the label-removal step. `cancel-in-progress: false` so an
202+
# in-flight review finishes and removes its own label rather than
203+
# being aborted partway through.
204+
concurrency:
205+
group: claude-fork-review-${{ github.event.pull_request.number }}
206+
cancel-in-progress: false
196207
permissions:
197208
contents: read
198209
pull-requests: write
199-
issues: read
200210
steps:
201211
# Check out the FORK head explicitly. We use a separate, least-privileged
202212
# token here and disable credential persistence so nothing fork-side can
@@ -209,31 +219,61 @@ jobs:
209219
fetch-depth: 1
210220
persist-credentials: false
211221

212-
# Prepare a trusted, base-repo-controlled MCP config for the review run.
222+
# Strip fork-supplied CLI config, then write a trusted MCP config.
223+
#
224+
# PRIMARY THREAT: `.claude/settings.json` hooks.
225+
# Claude Code reads `.claude/settings.json` from cwd at startup, and
226+
# its `SessionStart` / `PreToolUse` hooks run arbitrary shell BEFORE
227+
# any `--allowedTools` allowlist applies. That's a direct allowlist
228+
# bypass — a fork could ship `.claude/settings.json` with a
229+
# `SessionStart` hook that exfiltrates ANTHROPIC_API_KEY (still in
230+
# process env at that point) or anything else on the runner. Hooks
231+
# are the highest-impact config-discovery vector on this path.
232+
#
233+
# SECONDARY VECTORS: other auto-discovered config files.
234+
# `.mcp.json` — points Claude at attacker-controlled MCP servers,
235+
# which (if connected) become an exfiltration channel via tool calls.
236+
# `CLAUDE.md` / `CLAUDE.local.md` — auto-loaded as project memory,
237+
# become trusted-feeling prompt input. `.claude.json`, `.gitmodules`,
238+
# `.ripgreprc`, `.husky` are restored by the action's own base-revert
239+
# logic; we include them so this strip is the single source of truth.
240+
#
241+
# WHY WE STRIP RATHER THAN RELY ON THE ACTION:
242+
# anthropics/claude-code-action v1.0.99 calls `restoreConfigFromBase`
243+
# which reverts this same set of paths from the base branch before
244+
# the CLI starts (see src/github/operations/restore-config.ts in the
245+
# action source). So the fork-review job is safe today on this axis.
246+
# But that's an internal implementation detail of one action version.
247+
# If a future bump narrows or removes `restoreConfigFromBase` — the
248+
# same regression class we SHA-pin against (#1021) — the hole
249+
# silently reopens. Stripping here is defense-in-depth: the security
250+
# model holds regardless of what the action does internally.
213251
#
214-
# Why we delete every `.mcp.json` in the fork checkout:
215-
# Claude Code auto-discovers a project-level `.mcp.json` in addition
216-
# to anything passed via `--mcp-config`. The fork's checkout above
217-
# may ship a `.mcp.json` (at the repo root OR in any subdirectory —
218-
# this is a monorepo, so subdirectory configs are plausible) that has
219-
# been modified to point at attacker-controlled MCP servers, which
220-
# would become an exfiltration channel the moment Claude connected
221-
# to it (an injection in the diff could coerce tool calls that leak
222-
# review context). The `find ... -delete` sweep guarantees the only
223-
# MCP servers in scope for this run are the ones in our trusted,
224-
# inline config below — regardless of which directory Claude `cd`s
225-
# into during the review.
252+
# We use `find` rather than root-only `rm` because this is a monorepo
253+
# — a fork could plant configs under `src/<server>/.claude/` or
254+
# similar in case Claude's discovery walks subdirectories. The
255+
# `-print` makes any hits visible in run logs so we'd notice if a
256+
# fork ever ships one.
226257
#
227-
# The trusted config is written under $RUNNER_TEMP (outside the fork
228-
# checkout, so the fork cannot shadow it) and exposes only the
229-
# read-only MCP docs server at https://modelcontextprotocol.io/mcp.
258+
# The trusted MCP config is written under $RUNNER_TEMP (outside the
259+
# fork checkout, so the fork cannot shadow it) and exposes only the
260+
# read-only docs server at https://modelcontextprotocol.io/mcp.
230261
# Combined with no WebFetch and no unrestricted Bash in --allowedTools,
231-
# this means the only outbound HTTP this job can make is to
232-
# modelcontextprotocol.io — useful for protocol lookups while
233-
# reviewing, with no other network egress.
234-
- name: Prepare trusted MCP config
262+
# the only outbound HTTP Claude can direct is to modelcontextprotocol.io
263+
# — useful for protocol lookups while reviewing. (The runner itself
264+
# still talks to api.anthropic.com, api.github.com, the Actions cache,
265+
# etc.; the claim is about Claude's tool surface, not the runner.)
266+
- name: Strip fork-supplied CLI config + write trusted MCP config
235267
run: |
236-
find . -type f -name '.mcp.json' -print -delete
268+
find . -type f \( \
269+
-name '.mcp.json' \
270+
-o -name '.claude.json' \
271+
-o -name '.gitmodules' \
272+
-o -name '.ripgreprc' \
273+
-o -name 'CLAUDE.md' \
274+
-o -name 'CLAUDE.local.md' \
275+
\) -print -delete
276+
find . -type d \( -name '.claude' -o -name '.husky' \) -print -exec rm -rf {} +
237277
mkdir -p "$RUNNER_TEMP/claude-fork-review"
238278
printf '%s\n' '{"mcpServers":{"mcp-docs":{"type":"http","url":"https://modelcontextprotocol.io/mcp"}}}' > "$RUNNER_TEMP/claude-fork-review/mcp.json"
239279
echo "FORK_REVIEW_MCP_CONFIG=$RUNNER_TEMP/claude-fork-review/mcp.json" >> "$GITHUB_ENV"
@@ -261,8 +301,10 @@ jobs:
261301
github_token: ${{ github.token }}
262302
# Hardened tool surface: inline comments, read-only `gh`, and the
263303
# MCP docs server (only). Notably absent: Bash glob, Edit, Write,
264-
# WebFetch, and the fork's `.mcp.json` (which the prior step
265-
# deleted and replaced with a base-repo-controlled config).
304+
# WebFetch, and any fork-supplied auto-discovered CLI config
305+
# (`.claude/`, `.mcp.json`, `CLAUDE.md`, etc. — all stripped by
306+
# the prior step and replaced with a base-repo-controlled MCP
307+
# config under $RUNNER_TEMP).
266308
claude_args: |
267309
--max-turns 8
268310
--mcp-config ${{ env.FORK_REVIEW_MCP_CONFIG }}

0 commit comments

Comments
 (0)