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,