From b4c834f3cd80213e45d0f88dbef797a1d9cc3459 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 14 Jul 2026 12:16:49 +0200 Subject: [PATCH] ci: run Codex/Pi review on fork PRs when a maintainer triggers it (#10069) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ci: run Codex review on fork PRs when a maintainer triggers it The fork skip in codex-pr-review.yml unconditionally bailed on cross-repository PRs, so even a maintainer's /codex or /review comment (routed through pr-review-commands.yml via workflow_call, gated by check-write-access) skipped external PRs. Gate the skip on the automatic pull_request trigger only, detected via an empty INPUT_PR_NUMBER (the metadata step already branches on this at the same step). The workflow_call path now reviews fork PRs; the auto pull_request trigger still skips them. Co-Authored-By: Claude Opus 4.8 (1M context) * ci: run Pi review on fork PRs when a maintainer triggers it Apply the same fork-skip gating as the Codex review: skip fork PRs only on the automatic pull_request trigger (empty INPUT_PR_NUMBER), so a maintainer's /pi or /review comment (workflow_call, gated by check-write-access) reviews external PRs. Claude's pr-ready-review.yml needs no change: it has no fork skip, checks out main (not the fork ref), and reviews via gh pr diff/view with a restricted tool allowlist, so it already handles fork PRs on the command path. Co-Authored-By: Claude Opus 4.8 (1M context) * ci: harden fork-review path against secret exfiltration Addresses the CI review of the fork-review enablement. On the fork path (maintainer-triggered workflow_call for a cross-repository PR), the reviewer ran an autonomous agent over the attacker-controlled merge checkout with the EE token present, full-access sandbox, and the review prompt itself read from that untrusted checkout — so a malicious fork could rewrite the reviewer's own instructions to exfiltrate secrets. For fork PRs only (detected via the is_fork step output): - withhold WINDMILL_EE_PRIVATE_ACCESS: skip the EE access/checkout/ substitution steps, so the private-repo token is never in the env. - read REVIEW.md and the prompt file from the trusted base ref (git show origin/:...) instead of the merge checkout. - restrict the agent: Codex runs with -s workspace-write (network off) instead of danger-full-access; Pi drops the bash tool. Non-fork PRs are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) * ci: redact provider credentials from fork review comments The model call needs the provider credential in its environment/config, so a network-disabled sandbox alone can't stop a prompt-injected fork review from reading the key (Codex: $HOME/.codex/auth.json; Pi: /proc/self/environ) and emitting it in the final message, which both workflows post verbatim. GitHub Actions log masking does not cover comments posted via the API. Strip the known credential values (OpenAI key + raw Codex auth JSON and its nested tokens; DeepSeek key) from the review body before posting, closing the comment as an exfiltration channel. Applied unconditionally since a credential should never appear in a review comment regardless of trigger. Co-Authored-By: Claude Opus 4.8 (1M context) * ci: don't persist github.token in fork review checkout actions/checkout writes github.token into .git/config (http.extraheader) by default. The review agent can read the checked-out tree, so on the fork path a prompt injection could exfiltrate that token (issue/PR write) via .git/config — the provider-credential redaction added earlier didn't cover it. Set persist-credentials: false on the merge-ref checkout so the token is never written to disk. Safe on both paths: the only later git op is an unauthenticated fetch from the public origin, EE checkout uses its own token, and gh uses GH_TOKEN. Also redact github.token from the posted comment as defense-in-depth. Co-Authored-By: Claude Opus 4.8 (1M context) * ci: disable Pi project-local discovery on fork reviews Pi auto-discovers and executes project-local .pi extensions (.ts/.js) at startup with DEEPSEEK_API_KEY in its environment — before the --tools allowlist applies — so a fork could add an extension that exfiltrates the key over the network, which output redaction can't catch. On the fork path (cwd is the fork checkout), pass --no-extensions to disable extension discovery, plus --no-skills/--no-prompt-templates/--no-themes/ --no-context-files so fork-controlled skills, templates, themes, and AGENTS.md/CLAUDE.md aren't auto-loaded into the reviewer's prompt as an injection vector. Non-fork behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) * ci: use unguessable delimiter for untrusted PR metadata outputs The PR title/body were written to $GITHUB_OUTPUT with a fixed heredoc terminator (PR_BODY_EOF). A fork author could embed that terminator in their PR body to close the heredoc early and append their own output lines — e.g. is_fork=false, which (last-write-wins) overrides the real is_fork=true and puts fork code back on the trusted path (EE checkout + substitute_ee_code.sh with the private token, full-access agent). Generate a per-run random delimiter (128 bits from /dev/urandom) for the title and body heredocs so the terminator can't be predicted or embedded. Everything else in the block is single-line and newline-free, so this closes the injection. Co-Authored-By: Claude Opus 4.8 (1M context) * ci: set PI_OFFLINE=1 on fork Pi reviews to block package resolution --no-extensions only filters which resources are *loaded*; Pi still resolves packages declared in a fork's .pi/settings.json first, running `npm install` / the configured npmCommand and lifecycle scripts with DEEPSEEK_API_KEY in env and network available — before the extension filter applies. Set PI_OFFLINE=1 on the fork path so the resolver's installMissing() short- circuits (returns false) for every missing package, skipping all install/clone/ lifecycle execution. It gates only startup network ops (installs, helper-binary downloads), not the provider inference call, so the review still runs. Verified: a fork .pi/settings.json with a malicious npmCommand does not execute under the flag. Non-fork path unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) * ci: run fork Pi review from an isolated dir to cut off project config Root cause of the recurring fork-review exposure: Pi resolves every project config from /.pi — settings/packages, extensions, skills, themes, prompts, SYSTEM.md, APPEND_SYSTEM.md — so running inside the fork checkout let a fork inject any of them to execute code or rewrite the reviewer's system prompt with DEEPSEEK_API_KEY in env. Per-flag opt-outs (--no-extensions, PI_OFFLINE, ...) only covered discovered vectors one at a time (SYSTEM.md wasn't covered). Discovery is cwd-based (single level, no walk-up; global fallback is the trusted runner home), so run Pi from a fresh mktemp dir where no fork .pi/* is on the path. The fork agent has no shell, so pre-compute the diff (base...head SHAs are trusted) into the context file it reads; it may still read fork files by absolute path for extra context — reads are safe, only config discovery and code execution were the risk. Outputs now use absolute workspace paths since cwd moved. The --no-* flags and PI_OFFLINE stay as belt-and-suspenders. Non-fork path unchanged. Verified: a fork .pi/SYSTEM.md sentinel is not discovered from the isolated cwd. Co-Authored-By: Claude Opus 4.8 (1M context) * ci: keep review artifacts outside the checkout to defeat symlink writes Both workflows wrote generated files (final message, event stream, review context, prior-comments) into $GITHUB_WORKSPACE. On the fork path the merge tree is attacker-controlled, so a fork could commit any of those paths as a symlink (e.g. codex-final-message.md -> ../../_actions/actions/github-script/v7/dist/ index.js). Our write would follow it and overwrite the next action's code, which then executes with the provider credential and the write-capable GitHub token — no prompt injection required. Route every generated file through $RUNNER_TEMP, which is runner-created and outside the checkout, so no fork-committed symlink is on the path: - prior-comments.json and pr-review-context.md are written to RUNNER_TEMP; the context step reads prior-comments from there. - The agent is given the context file's absolute RUNNER_TEMP path (appended to the prompt); prompt files updated to reference it instead of a checkout- relative path. Pi (no shell on forks) gets the diff pre-computed into that context file; the isolated-cwd hardening is retained. - Codex writes -o to RUNNER_TEMP; Pi writes its events/final message there; both post steps read from RUNNER_TEMP. Non-fork behavior is functionally unchanged (trusted checkout; same review inputs, now sourced from RUNNER_TEMP). Co-Authored-By: Claude Opus 4.8 (1M context) * ci: condense fork-review comments to the 4-line limit AGENTS.md requires each invariant stated in <=4 lines. Trim the security comments added in this branch (fork-skip rationale, output delimiter, isolated cwd, RUNNER_TEMP artifacts, credential redaction) to comply without dropping the constraint each one records. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .github/codex/pr-review.prompt.md | 2 +- .github/pi/pr-review.prompt.md | 2 +- .github/workflows/codex-pr-review.yml | 109 ++++++++++++++++++---- .github/workflows/pi-pr-review.yml | 128 ++++++++++++++++++++++---- 4 files changed, 204 insertions(+), 37 deletions(-) diff --git a/.github/codex/pr-review.prompt.md b/.github/codex/pr-review.prompt.md index fef52dba85..97a6217062 100644 --- a/.github/codex/pr-review.prompt.md +++ b/.github/codex/pr-review.prompt.md @@ -1,5 +1,5 @@ # Codex output format -- Read `./.github/codex/pr-review-context.md` for PR metadata and the diff commands. +- Read the review context file whose absolute path is given at the end of these instructions; it holds the PR metadata and the diff commands. - Return a markdown PR comment starting with `## Codex Review`. - Tag each finding with a severity (P0 / P1 / P2), file path, and line number when known confidently. diff --git a/.github/pi/pr-review.prompt.md b/.github/pi/pr-review.prompt.md index 92f128c6b1..952ba4c428 100644 --- a/.github/pi/pr-review.prompt.md +++ b/.github/pi/pr-review.prompt.md @@ -1,6 +1,6 @@ # Pi output format -- Read `./.github/pi/pr-review-context.md` for PR metadata and the diff commands. +- Read the review context file whose absolute path is given at the end of these instructions; it holds the PR metadata and the diff (or the git commands to produce it). - Return a markdown PR comment starting with `## Pi Review`. - Tag each finding with a severity (P0 / P1 / P2), file path, and line number when known confidently. - Output ONLY the final review markdown — no preamble, no thinking, no tool transcripts. diff --git a/.github/workflows/codex-pr-review.yml b/.github/workflows/codex-pr-review.yml index b395db98be..68bea1d715 100644 --- a/.github/workflows/codex-pr-review.yml +++ b/.github/workflows/codex-pr-review.yml @@ -104,24 +104,36 @@ jobs: IS_FORK="$EVENT_FORK" PR_AUTHOR="$EVENT_AUTHOR" fi - if [ "$IS_FORK" = "true" ]; then - echo "Skipping Codex review for fork PR." + # Fork PRs run untrusted code with secrets present, so the automatic + # pull_request trigger never reviews them. A non-empty INPUT_PR_NUMBER + # means we arrived via workflow_call (a maintainer /codex comment gated + # by check-write-access), so allow forks only on that path. + if [ "$IS_FORK" = "true" ] && [ -z "$INPUT_PR_NUMBER" ]; then + echo "Skipping Codex review for fork PR (automatic trigger)." echo "skip=true" >> "$GITHUB_OUTPUT" exit 0 fi + # PR title/body are attacker-controlled free text. Use an unguessable + # per-run delimiter so a fork can't embed a fixed heredoc terminator to + # inject extra outputs — e.g. is_fork=false (last-write-wins), which + # would re-enable the EE checkout and trusted-path settings for forks. + RAND=$(head -c 16 /dev/urandom | od -An -tx1 | tr -d ' \n') + TITLE_EOF="TITLE_EOF_${RAND}" + BODY_EOF="BODY_EOF_${RAND}" { echo "skip=false" + echo "is_fork=$IS_FORK" echo "pr_number=$PR_NUMBER" echo "base_ref=$BASE_REF" echo "base_sha=$BASE_SHA" echo "head_sha=$HEAD_SHA" echo "pr_author=$PR_AUTHOR" - echo 'title<> "$GITHUB_OUTPUT" - name: Checkout repository @@ -130,9 +142,17 @@ jobs: with: ref: refs/pull/${{ steps.pr.outputs.pr_number }}/merge fetch-depth: 1 + # Don't persist github.token in .git/config: the review agent can read + # the checkout, and on the fork path that token (issue/PR write) would + # otherwise be exfiltratable. All later git ops target the public origin + # and need no auth; EE checkout and gh use their own explicit tokens. + persist-credentials: false + # Never expose the EE private-repo token to untrusted fork code. Skipping + # this step leaves steps.ee.outputs.available empty, so the EE checkout and + # substitution steps below are skipped too. - name: Check EE access - if: steps.codex_config.outputs.enabled == 'true' && steps.pr.outputs.skip != 'true' + if: steps.codex_config.outputs.enabled == 'true' && steps.pr.outputs.skip != 'true' && steps.pr.outputs.is_fork != 'true' id: ee env: EE_TOKEN: ${{ secrets.WINDMILL_EE_PRIVATE_ACCESS }} @@ -206,9 +226,12 @@ jobs: REPO: ${{ github.repository }} PR_NUMBER: ${{ steps.pr.outputs.pr_number }} run: | + # Write outside the checkout: on the fork path the merge tree is + # attacker-controlled, and a committed symlink at this path would + # redirect the write. gh api "repos/$REPO/issues/$PR_NUMBER/comments?per_page=100" \ --jq '[.[] | {user: .user.login, created_at: .created_at, body: (.body | .[:4000])}] | sort_by(.created_at) | .[-20:]' \ - > prior-comments.json || echo "[]" > prior-comments.json + > "$RUNNER_TEMP/prior-comments.json" || echo "[]" > "$RUNNER_TEMP/prior-comments.json" - name: Write Codex review context if: steps.codex_config.outputs.enabled == 'true' && steps.pr.outputs.skip != 'true' @@ -222,9 +245,9 @@ jobs: PR_AUTHOR: ${{ steps.pr.outputs.pr_author }} EXTRA_PROMPT: ${{ inputs.extra_prompt }} run: | - mkdir -p .github/codex node <<'NODE' const fs = require('fs'); + const tmp = process.env.RUNNER_TEMP; const lines = [ `Repository: ${process.env.PR_REPOSITORY}`, `PR number: ${process.env.PR_NUMBER}`, @@ -254,9 +277,9 @@ jobs: if (process.env.EXTRA_PROMPT && process.env.EXTRA_PROMPT.trim()) { lines.push('', 'Additional reviewer instructions:', process.env.EXTRA_PROMPT.trim()); } - if (fs.existsSync('prior-comments.json')) { + if (fs.existsSync(`${tmp}/prior-comments.json`)) { try { - const comments = JSON.parse(fs.readFileSync('prior-comments.json', 'utf8')); + const comments = JSON.parse(fs.readFileSync(`${tmp}/prior-comments.json`, 'utf8')); if (Array.isArray(comments) && comments.length > 0) { lines.push( '', @@ -271,19 +294,39 @@ jobs: } } catch (_) {} } - fs.writeFileSync('.github/codex/pr-review-context.md', `${lines.join('\n')}\n`); + fs.writeFileSync(`${tmp}/pr-review-context.md`, `${lines.join('\n')}\n`); NODE - name: Run Codex review if: steps.codex_config.outputs.enabled == 'true' && steps.pr.outputs.skip != 'true' + env: + PR_IS_FORK: ${{ steps.pr.outputs.is_fork }} + PR_BASE_REF: ${{ steps.pr.outputs.base_ref }} run: | - cat REVIEW.md .github/codex/pr-review.prompt.md > /tmp/codex-prompt.md + if [ "$PR_IS_FORK" = "true" ]; then + # Fork code is untrusted. Read the review policy/prompt from the base + # ref (git show) rather than the attacker-controlled merge checkout, + # so a malicious fork can't rewrite the reviewer's own instructions, + # and run in a network-disabled sandbox to block secret exfiltration. + git show "origin/$PR_BASE_REF:REVIEW.md" > /tmp/codex-prompt.md + git show "origin/$PR_BASE_REF:.github/codex/pr-review.prompt.md" >> /tmp/codex-prompt.md + SANDBOX_MODE=workspace-write + else + cat REVIEW.md .github/codex/pr-review.prompt.md > /tmp/codex-prompt.md + SANDBOX_MODE=danger-full-access + fi + # The context file lives in RUNNER_TEMP (outside the attacker-controlled + # checkout); tell the agent its absolute path. + printf '\nReview context file (absolute path): %s\n' "$RUNNER_TEMP/pr-review-context.md" >> /tmp/codex-prompt.md + # Write the final message outside the checkout too: a fork could commit + # codex-final-message.md as a symlink and redirect this write to overwrite + # e.g. a GitHub Action's index.js, which then runs with our credentials. codex exec \ -C "$GITHUB_WORKSPACE" \ -m gpt-5.6-sol \ -c 'model_reasoning_effort="xhigh"' \ - -s danger-full-access \ - -o codex-final-message.md \ + -s "$SANDBOX_MODE" \ + -o "$RUNNER_TEMP/codex-final-message.md" \ - < /tmp/codex-prompt.md - name: Post Codex review comment @@ -291,20 +334,52 @@ jobs: uses: actions/github-script@v7 env: PR_NUMBER: ${{ steps.pr.outputs.pr_number }} + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + CODEX_AUTH_JSON: ${{ secrets.CODEX_AUTH_JSON }} + GH_JOB_TOKEN: ${{ github.token }} with: github-token: ${{ github.token }} script: | const fs = require('fs'); - const path = `${process.env.GITHUB_WORKSPACE}/codex-final-message.md`; + const path = `${process.env.RUNNER_TEMP}/codex-final-message.md`; if (!fs.existsSync(path)) { core.info('Codex did not produce a final message; skipping PR comment.'); return; } - const body = fs.readFileSync(path, 'utf8').trim(); + let body = fs.readFileSync(path, 'utf8').trim(); if (!body) { core.info('Codex final message was empty; skipping PR comment.'); return; } + // Defense-in-depth for fork reviews: the model call needs the provider + // credential in the env, and the posted comment bypasses Actions log + // masking. Strip any credential (API key, raw auth JSON, nested + // tokens) that leaked into the review text before posting. + const secrets = []; + const addSecret = (v, min) => { + if (typeof v === 'string' && v.length >= min) secrets.push(v); + }; + addSecret(process.env.OPENAI_API_KEY, 8); + addSecret(process.env.CODEX_AUTH_JSON, 8); + addSecret(process.env.GH_JOB_TOKEN, 8); + if (process.env.CODEX_AUTH_JSON) { + try { + const collect = (o) => { + if (typeof o === 'string') addSecret(o, 20); + else if (Array.isArray(o)) o.forEach(collect); + else if (o && typeof o === 'object') Object.values(o).forEach(collect); + }; + collect(JSON.parse(process.env.CODEX_AUTH_JSON)); + } catch (_) {} + } + for (const s of [...new Set(secrets)].sort((a, b) => b.length - a.length)) { + body = body.split(s).join('[REDACTED]'); + } + body = body.trim(); + if (!body) { + core.info('Codex final message was empty after redaction; skipping PR comment.'); + return; + } await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, diff --git a/.github/workflows/pi-pr-review.yml b/.github/workflows/pi-pr-review.yml index 72553b0d83..03c9599480 100644 --- a/.github/workflows/pi-pr-review.yml +++ b/.github/workflows/pi-pr-review.yml @@ -97,24 +97,36 @@ jobs: IS_FORK="$EVENT_FORK" PR_AUTHOR="$EVENT_AUTHOR" fi - if [ "$IS_FORK" = "true" ]; then - echo "Skipping Pi review for fork PR." + # Fork PRs run untrusted code with secrets present, so the automatic + # pull_request trigger never reviews them. A non-empty INPUT_PR_NUMBER + # means we arrived via workflow_call (a maintainer /pi comment gated by + # check-write-access), so allow forks only on that path. + if [ "$IS_FORK" = "true" ] && [ -z "$INPUT_PR_NUMBER" ]; then + echo "Skipping Pi review for fork PR (automatic trigger)." echo "skip=true" >> "$GITHUB_OUTPUT" exit 0 fi + # PR title/body are attacker-controlled free text. Use an unguessable + # per-run delimiter so a fork can't embed a fixed heredoc terminator to + # inject extra outputs — e.g. is_fork=false (last-write-wins), which + # would re-enable the EE checkout and trusted-path settings for forks. + RAND=$(head -c 16 /dev/urandom | od -An -tx1 | tr -d ' \n') + TITLE_EOF="TITLE_EOF_${RAND}" + BODY_EOF="BODY_EOF_${RAND}" { echo "skip=false" + echo "is_fork=$IS_FORK" echo "pr_number=$PR_NUMBER" echo "base_ref=$BASE_REF" echo "base_sha=$BASE_SHA" echo "head_sha=$HEAD_SHA" echo "pr_author=$PR_AUTHOR" - echo 'title<> "$GITHUB_OUTPUT" - name: Checkout repository @@ -123,9 +135,17 @@ jobs: with: ref: refs/pull/${{ steps.pr.outputs.pr_number }}/merge fetch-depth: 1 + # Don't persist github.token in .git/config: the review agent can read + # the checkout, and on the fork path that token (issue/PR write) would + # otherwise be exfiltratable. All later git ops target the public origin + # and need no auth; EE checkout and gh use their own explicit tokens. + persist-credentials: false + # Never expose the EE private-repo token to untrusted fork code. Skipping + # this step leaves steps.ee.outputs.available empty, so the EE checkout and + # substitution steps below are skipped too. - name: Check EE access - if: steps.pi_config.outputs.enabled == 'true' && steps.pr.outputs.skip != 'true' + if: steps.pi_config.outputs.enabled == 'true' && steps.pr.outputs.skip != 'true' && steps.pr.outputs.is_fork != 'true' id: ee env: EE_TOKEN: ${{ secrets.WINDMILL_EE_PRIVATE_ACCESS }} @@ -178,9 +198,12 @@ jobs: REPO: ${{ github.repository }} PR_NUMBER: ${{ steps.pr.outputs.pr_number }} run: | + # Write outside the checkout: on the fork path the merge tree is + # attacker-controlled, and a committed symlink at this path would + # redirect the write. gh api "repos/$REPO/issues/$PR_NUMBER/comments?per_page=100" \ --jq '[.[] | {user: .user.login, created_at: .created_at, body: (.body | .[:4000])}] | sort_by(.created_at) | .[-20:]' \ - > prior-comments.json || echo "[]" > prior-comments.json + > "$RUNNER_TEMP/prior-comments.json" || echo "[]" > "$RUNNER_TEMP/prior-comments.json" - name: Write Pi review context if: steps.pi_config.outputs.enabled == 'true' && steps.pr.outputs.skip != 'true' @@ -194,9 +217,9 @@ jobs: PR_AUTHOR: ${{ steps.pr.outputs.pr_author }} EXTRA_PROMPT: ${{ inputs.extra_prompt }} run: | - mkdir -p .github/pi node <<'NODE' const fs = require('fs'); + const tmp = process.env.RUNNER_TEMP; const lines = [ `Repository: ${process.env.PR_REPOSITORY}`, `PR number: ${process.env.PR_NUMBER}`, @@ -226,9 +249,9 @@ jobs: if (process.env.EXTRA_PROMPT && process.env.EXTRA_PROMPT.trim()) { lines.push('', 'Additional reviewer instructions:', process.env.EXTRA_PROMPT.trim()); } - if (fs.existsSync('prior-comments.json')) { + if (fs.existsSync(`${tmp}/prior-comments.json`)) { try { - const comments = JSON.parse(fs.readFileSync('prior-comments.json', 'utf8')); + const comments = JSON.parse(fs.readFileSync(`${tmp}/prior-comments.json`, 'utf8')); if (Array.isArray(comments) && comments.length > 0) { lines.push( '', @@ -243,7 +266,7 @@ jobs: } } catch (_) {} } - fs.writeFileSync('.github/pi/pr-review-context.md', `${lines.join('\n')}\n`); + fs.writeFileSync(`${tmp}/pr-review-context.md`, `${lines.join('\n')}\n`); NODE - name: Run Pi review @@ -251,16 +274,69 @@ jobs: env: DEEPSEEK_API_KEY: ${{ secrets.DEEPSEEK_API_KEY }} PI_SKIP_VERSION_CHECK: '1' + PR_IS_FORK: ${{ steps.pr.outputs.is_fork }} + PR_BASE_REF: ${{ steps.pr.outputs.base_ref }} + PR_BASE_SHA: ${{ steps.pr.outputs.base_sha }} + PR_HEAD_SHA: ${{ steps.pr.outputs.head_sha }} run: | set -o pipefail - cat REVIEW.md .github/pi/pr-review.prompt.md > /tmp/pi-prompt.md + PI_HARDEN_FLAGS=() + # Keep generated files (final message, events, context) outside the + # checkout: on the fork path a committed symlink at any of these paths + # would redirect our write and could overwrite an action's code that + # then runs with our credentials. RUNNER_TEMP is outside the checkout. + OUT_DIR="$RUNNER_TEMP" + CTX="$RUNNER_TEMP/pr-review-context.md" + if [ "$PR_IS_FORK" = "true" ]; then + # Fork code is untrusted. Read the review policy/prompt from the base + # ref (git show) rather than the attacker-controlled merge checkout, + # so a malicious fork can't rewrite the reviewer's own instructions, + # and drop the bash tool so the agent has no shell to exfiltrate with. + git show "origin/$PR_BASE_REF:REVIEW.md" > /tmp/pi-prompt.md + git show "origin/$PR_BASE_REF:.github/pi/pr-review.prompt.md" >> /tmp/pi-prompt.md + PI_TOOLS=read,grep,find,ls + + # The agent has no shell, so pre-compute the diff (base...head SHAs are + # trusted) into the context file it reads. It may still read fork files + # by absolute path for extra context — reads are safe. + { + echo "" + echo "## Pre-computed review diff (base...head)" + echo "You have no shell. The full diff is below. The repository checkout" + echo "is at $GITHUB_WORKSPACE — you may read files there by absolute path." + echo '```diff' + git -C "$GITHUB_WORKSPACE" diff --unified=0 "$PR_BASE_SHA...$PR_HEAD_SHA" + echo '```' + } >> "$CTX" + + # Pi resolves ALL project config from /.pi (settings/packages, + # extensions, skills, themes, prompts, SYSTEM.md); inside the fork + # checkout a fork could inject any to run code or rewrite our system + # prompt. Discovery is cwd-based, so run from a fresh empty dir. + PI_WORKDIR=$(mktemp -d) + cd "$PI_WORKDIR" + + # Belt-and-suspenders on top of the isolated cwd: refuse discovery of + # extensions/skills/templates/themes/context-files, and PI_OFFLINE=1 to + # block any startup network op or package install. PI_OFFLINE gates only + # startup network ops, not the provider inference call. + PI_HARDEN_FLAGS=(--no-extensions --no-skills --no-prompt-templates --no-themes --no-context-files) + export PI_OFFLINE=1 + else + cat REVIEW.md .github/pi/pr-review.prompt.md > /tmp/pi-prompt.md + PI_TOOLS=read,grep,find,ls,bash + fi + # The context file lives in RUNNER_TEMP (outside the checkout); tell the + # agent its absolute path. + printf '\nReview context file (absolute path): %s\n' "$CTX" >> /tmp/pi-prompt.md pi -p \ --provider deepseek \ --model deepseek-v4-pro \ - --tools read,grep,find,ls,bash \ + --tools "$PI_TOOLS" \ + "${PI_HARDEN_FLAGS[@]}" \ --mode json \ < /tmp/pi-prompt.md \ - | tee pi-events.jsonl \ + | tee "$OUT_DIR/pi-events.jsonl" \ | jq -rc --unbuffered ' if .type == "agent_start" then "🤖 pi agent started" elif .type == "turn_start" then "── turn ──" @@ -288,27 +364,43 @@ jobs: | map(select(.role == "assistant")) | last | (.content[]? | select(.type == "text") | .text) - ' pi-events.jsonl > pi-final-message.md + ' "$OUT_DIR/pi-events.jsonl" > "$OUT_DIR/pi-final-message.md" - name: Post Pi review comment if: steps.pi_config.outputs.enabled == 'true' && steps.pr.outputs.skip != 'true' uses: actions/github-script@v7 env: PR_NUMBER: ${{ steps.pr.outputs.pr_number }} + DEEPSEEK_API_KEY: ${{ secrets.DEEPSEEK_API_KEY }} + GH_JOB_TOKEN: ${{ github.token }} with: github-token: ${{ github.token }} script: | const fs = require('fs'); - const path = `${process.env.GITHUB_WORKSPACE}/pi-final-message.md`; + const path = `${process.env.RUNNER_TEMP}/pi-final-message.md`; if (!fs.existsSync(path)) { core.info('Pi did not produce a final message; skipping PR comment.'); return; } - const body = fs.readFileSync(path, 'utf8').trim(); + let body = fs.readFileSync(path, 'utf8').trim(); if (!body) { core.info('Pi final message was empty; skipping PR comment.'); return; } + // Defense-in-depth for fork reviews: the model call needs the provider + // credential in the environment (readable via /proc/self/environ), and + // the posted comment is an exfiltration channel that bypasses GitHub + // Actions log masking. Strip the credential if it leaked into the text. + for (const s of [process.env.DEEPSEEK_API_KEY, process.env.GH_JOB_TOKEN]) { + if (typeof s === 'string' && s.length >= 8) { + body = body.split(s).join('[REDACTED]'); + } + } + body = body.trim(); + if (!body) { + core.info('Pi final message was empty after redaction; skipping PR comment.'); + return; + } await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo,