Skip to content

ci: keep security-review code trusted and PR diff tool-free - #11076

Open
lmtlssss wants to merge 3 commits into
spesmilo:masterfrom
lmtlssss:ci-trusted-pr-security-review-20261024
Open

lmtlssss wants to merge 3 commits into
spesmilo:masterfrom
lmtlssss:ci-trusted-pr-security-review-20261024

Conversation

@lmtlssss

Copy link
Copy Markdown

Why this matters

The automated security-review job runs on pull_request_target with CLAUDE_CODE_OAUTH_TOKEN, and its manual dispatch allows maintainers to review external contributors' PRs. Previously it checked out refs/pull/<number>/head, installed Python dependencies from that checkout, and executed contrib/ci/claude_security_review.py from the same PR tree. A contributor could change the PR after a maintainer initiated a review; the workflow itself documented this TOCTOU risk (issue #10801). The reviewer additionally ran Claude Code in --dangerously-skip-permissions mode with the OAuth token present, while its input included contributor-controlled diff text.

This is a credential isolation / supply-chain hardening change. No credential exfiltration or compromise was attempted or observed.

Change

  • Always checkout trusted base source (pull_request.base.sha for pull_request_target, the trusted workflow commit for dispatch). Disable persisted checkout credentials. All installation scripts, dependency manifests, the Python reviewer, and its system prompt now come from that trusted checkout.
  • Retrieve refs/pull/<number>/head only as Git data. Validate the PR number, fetch once, capture the resolved 40-byte hex commit SHA, and compute the diff and commit-message range against that pinned SHA. Never checkout or execute PR code. A later force-push cannot alter the reviewed revision.
  • Run the model with --tools "", --strict-mcp-config, and --disable-slash-commands; remove --dangerously-skip-permissions. This keeps untrusted diff text as review data and does not grant the reviewer filesystem, shell, or inherited MCP tools.
  • Preserve full Git history when fetching the base branch; using --depth=1 would re-shallow the trusted checkout and could hide the merge base for older PR branches.

Observed qualification

A native local Git journey was executed using a disposable bare remote, a trusted base advanced after a contributor branch diverged, and a simulated refs/pull/42/head. The candidate fetched the original SHA and calculated the diff without moving the runner's trusted HEAD. After a force-push changed the PR ref, the originally selected diff remained byte-identical; malformed PR input was rejected; the three-dot diff continued to work across the divergent histories. All four assertions passed. The fixture and output are recorded in the task workspace.

Source was independently verified at the published branch commit. Exact GitHub Actions run and a real Claude Code invocation with a valid token have not been executed; keep this PR DRAFT until maintainers confirm CLI flag compatibility and run the workflow on a test PR. Existing contributors continue to be eligible for manual dispatch.

Ref: #10801 (the previously documented external-PR review / force-push race). This is a fix to a documented trust-boundary defect plus a separate model-tool exposure, rather than a claimed first disclosure of a successful attack.

@lmtlssss

Copy link
Copy Markdown
Author

Independent GitHub-hosted qualification — 2026-10-25 UTC

The patched exact repository Python reviewer was exercised on a GitHub-hosted ubuntu-24.04 runner in an isolated fork against this upstream PR. It fetched PR #11076 at the expected immutable SHA fa9488e4cb3eda7c0d1392bd78ae49e76d8e535c, calculated the two-file diff, invoked a deliberately fake Claude process which asserted --tools "", --strict-mcp-config, --disable-slash-commands, and absence of --dangerously-skip-permissions, parsed a VERDICT: PASS response, and left the trusted checkout's HEAD unchanged.

A second job step installed the actual current published Claude Code CLI and checked its help output for all three security flags. Both steps passed: GitHub Actions run #40153247866.

Separate native disposable Git regression covered a force-push after selecting the PR SHA, divergent branch histories, unchanged trusted checkout, and invalid PR numbers: 4/4 checks passed. This is no-credential qualification; it deliberately does not call the live model or access an upstream OAuth secret. Maintainers should review the trust boundary and may run the genuine security-review job on a controlled PR.

No external contributor's code was executed in the isolated test.

@lmtlssss
lmtlssss marked this pull request as ready for review October 25, 2026 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant