From 2bfbe0589c5956d0e9660f9d82d985a90ca2737d Mon Sep 17 00:00:00 2001 From: Travis Cook Date: Mon, 6 Jul 2026 17:27:59 -0400 Subject: [PATCH] fix(ci): persist-credentials:false on the auto path too; document matcher semantics Follow-ups from PR #4's own automated review: - Add persist-credentials:false to claude-review.yml's checkout for consistency with the mention path (low risk on the trusted in-repo path, but keeps the invariant). - Document in the spec why find -exec is a bypass (single command word-list) while command substitution / chaining is NOT (the matcher parses those and requires each segment to be allowlisted). Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/claude-review.yml | 6 ++++++ .../specs/2026-07-06-automated-pr-review-design.md | 14 ++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 9293bb3..b7e9d02 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -30,6 +30,12 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: 0 # full history so the reviewer can `git diff` against the base + # Don't persist GITHUB_TOKEN into .git/config. This path is trusted + # (in-repo PRs), so risk is low, but it keeps the "reviewed content must not + # reach tokens" invariant consistent with the mention path. Safe here: the + # fetch already happened at checkout; git diff/log/show are local and + # gh pr comment authenticates via GH_TOKEN, not the git credential helper. + persist-credentials: false - uses: actions/setup-node@v4 with: diff --git a/docs/superpowers/specs/2026-07-06-automated-pr-review-design.md b/docs/superpowers/specs/2026-07-06-automated-pr-review-design.md index 051eb91..60f7006 100644 --- a/docs/superpowers/specs/2026-07-06-automated-pr-review-design.md +++ b/docs/superpowers/specs/2026-07-06-automated-pr-review-design.md @@ -244,8 +244,18 @@ allowlist" is only as good as the *specific* commands allowed. Confirmed fixes: read-only `git diff/log/show`. The general file-read utils (`cat`/`grep`/`jq`/`ls`) were dropped there because prefix rules can't pin them to the repo tree, so they could read `/proc/self/environ` or `.git/config`. -- **`persist-credentials: false`** on the mention checkout, so the `GITHUB_TOKEN` is - not written into `.git/config` where a read primitive could recover it. +- **`persist-credentials: false`** on **both** checkouts, so the `GITHUB_TOKEN` is + not written into `.git/config` where a read primitive could recover it. (Critical + on the mention path; added to the auto path too for consistency — low risk there + since it only runs for trusted in-repo PRs.) +- **Why `find` is special but command-substitution isn't a bypass:** `find -exec` + is a *single* command whose child process is an internal `find` argument, so the + allowlist never sees a second command to check. By contrast the matcher **does** + parse shell operators and command substitutions — `gh pr comment --body "$(cat …)"` + makes the matcher require `cat` to *also* be allowlisted, and chaining (`a; b`, + `a && b`, `a | b`) requires every segment to be allowed. So substitution/chaining + is not a `find -exec`-style hole; the fix was to drop the commands (like `find`) + that smuggle execution *inside a single command word-list*. - **Residual risk (documented, accepted):** the mention path still co-locates a subscription token with an agent reading untrusted content; the real defenses are the human-gated trigger, the trusted-actor gate, and the minimized (non-exec,