Skip to content

Commit 548d95f

Browse files
committed
fix(security): eliminate pwn-request checkout in review-bot privileged job
CodeQL flagged review-bot.yml (pull_request_target, App token minted) for checking out untrusted code in a privileged context: the diff step did `git fetch origin pull/<n>/head:pr-head` then diffed against it, pulling attacker-controlled ref content into the runner's git state before the critic gate ran. Replace with an API-only diff: `gh pr diff <n>` piped directly into critic_cli over stdin (--diff -). No git fetch/checkout of the PR head occurs anywhere in the job; the only actions/checkout calls left are the job's own base-ref self-checkout and a pinned, trusted autogen commit for critic_cli. Also add fail-closed handling if `gh pr diff` itself errors, so a failed API call can no longer be misread as "zero findings". Ref: T-38-SEC-PWN
1 parent 24b9832 commit 548d95f

2 files changed

Lines changed: 80 additions & 13 deletions

File tree

.github/workflows/review-bot.yml

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,16 @@ jobs:
5656
echo "PR is OUT-OF-CLASS. review-bot will not approve or merge. Falling back to human review."
5757
5858
# --- Gate 2: deterministic critic (autogen critic_cli, pinned) ----
59-
- name: Compute PR diff (base...head)
60-
id: diff
61-
if: steps.classify.outputs.eligible == 'true'
62-
run: |
63-
set -euo pipefail
64-
git fetch origin "${{ github.event.pull_request.base.ref }}" --depth=100
65-
git fetch origin "pull/${{ github.event.pull_request.number }}/head:pr-head" --depth=100
66-
git diff "origin/${{ github.event.pull_request.base.ref }}...pr-head" > "${{ github.workspace }}/pr.diff"
67-
59+
# SECURITY (T-38-SEC-PWN / CodeQL actions/checkout-of-untrusted-code,
60+
# PR #17 review-bot.yml lines 59-68): this job runs under
61+
# pull_request_target with a minted App token, so the PR head is
62+
# untrusted content in a privileged context. It MUST NOT be fetched,
63+
# checked out, or merged into the local git object database here --
64+
# not even via a bare `git fetch`. The diff is obtained exclusively
65+
# through the GitHub API (`gh pr diff`, an authenticated HTTPS call
66+
# that returns text, not a git ref) and streamed straight into
67+
# critic_cli over stdin in the "Run critic_cli" step below. No PR-head
68+
# commit ever touches this runner's filesystem or git state.
6869
- name: Checkout autogen (pinned commit, for critic_cli)
6970
if: steps.classify.outputs.eligible == 'true'
7071
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
@@ -86,19 +87,30 @@ jobs:
8687
echo "autogen checkout at the pinned critic_cli SHA failed -- treating as a hard stop (fail-closed). No approval will be issued." >&2
8788
exit 1
8889
89-
- name: Run critic_cli against PR diff
90+
- name: Run critic_cli against PR diff (diff via API, no checkout of PR head)
9091
id: critic
9192
if: steps.classify.outputs.eligible == 'true'
9293
working-directory: autogen-critic
9394
env:
9495
PYTHONPATH: ${{ github.workspace }}/autogen-critic
96+
# Read-only default token (permissions: contents: read, pull-requests:
97+
# read) is sufficient for `gh pr diff`; the App token is intentionally
98+
# NOT used here (least privilege -- this step never approves/merges).
99+
GH_TOKEN: ${{ github.token }}
95100
run: |
96101
set -o pipefail
97102
python -m pip install --quiet -r requirements.txt
98-
python -m maf_starter.critic_cli --diff "${{ github.workspace }}/pr.diff" --severity-gate blocking \
103+
gh pr diff "${{ github.event.pull_request.number }}" --repo "${{ github.repository }}" \
104+
| python -m maf_starter.critic_cli --diff - --severity-gate blocking \
99105
| tee "${{ github.workspace }}/critic-output.txt"
100-
exit_code=${PIPESTATUS[0]}
101-
echo "critic_exit=${exit_code}" >> "$GITHUB_OUTPUT"
106+
diff_exit=${PIPESTATUS[0]}
107+
critic_exit=${PIPESTATUS[1]}
108+
if [ "${diff_exit}" -ne 0 ]; then
109+
echo "gh pr diff failed (exit ${diff_exit}) -- treating as a hard stop (fail-closed). No approval will be issued." >&2
110+
echo "critic_exit=1" >> "$GITHUB_OUTPUT"
111+
exit 1
112+
fi
113+
echo "critic_exit=${critic_exit}" >> "$GITHUB_OUTPUT"
102114
continue-on-error: true
103115

104116
- name: Request changes - critic findings block auto-merge
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
# T-38-SEC-PWN: review-bot pwn-request mitigation
2+
3+
## Finding
4+
5+
CodeQL (`actions/checkout-of-untrusted-code`, "Checkout of untrusted code in a
6+
privileged context") flagged `.github/workflows/review-bot.yml` on PR #17.
7+
The `review` job runs on `pull_request_target`, which mints a GitHub App
8+
installation token with `pull-requests: write` scope before the diff step
9+
ran. The prior "Compute PR diff (base...head)" step fetched the PR head ref
10+
directly (`git fetch origin pull/<n>/head:pr-head`) into the runner's local
11+
git object database and then diffed against it. Fetching attacker-controlled
12+
ref content into a privileged job's git state is the classic GitHub Actions
13+
pwn-request pattern (see GitHub's own docs on "Preventing pwn requests"),
14+
even though the original step never wrote that content to the working tree
15+
or executed it directly.
16+
17+
## Mitigation
18+
19+
The privileged `review` job is now checkout-free of any untrusted ref:
20+
21+
- The eligibility classifier (`.github/scripts/classify-automerge-eligibility.ps1`)
22+
already sourced its input exclusively from `gh pr view --json files,author`
23+
(API text), not from any checked-out ref.
24+
- The diff step now calls `gh pr diff <n> --repo <repo>` (GitHub API over
25+
HTTPS, returning diff text) and streams that text directly into
26+
`critic_cli` over stdin (`--diff -`). No `git fetch`/`git checkout` of the
27+
PR head, and no PR-head commit ever enters the runner's git object
28+
database or filesystem.
29+
- The only `actions/checkout` calls remaining in the job are: (1) the job's
30+
own repository at its default/base ref (never the PR head — no `ref:`
31+
override is supplied, so under `pull_request_target` this resolves to the
32+
base branch), and (2) `Coding-Autopilot-System/autogen` pinned to a fixed,
33+
trusted commit SHA for `critic_cli`. Neither checks out attacker-controlled
34+
content.
35+
- Added fail-closed handling: if `gh pr diff` itself fails (non-zero exit),
36+
the job now hard-stops rather than silently treating an empty/partial diff
37+
as "no findings" and proceeding toward approval.
38+
39+
## Verification
40+
41+
- Structural check: `grep` confirms zero occurrences of `git fetch .*pull/`,
42+
`pr-head`, or `actions/checkout` with a PR-head `ref:` in
43+
`.github/workflows/review-bot.yml`. The remaining `actions/checkout` uses
44+
are the base-ref self-checkout and the pinned trusted `autogen` checkout.
45+
- The change was pushed to PR #17 (`feat/merge-flow-review-bot`); GitHub's
46+
CodeQL check re-runs automatically on `synchronize` and is expected to
47+
clear the "Checkout of untrusted code in a privileged context" alert on
48+
the updated lines.
49+
50+
## Note on scope
51+
52+
This note was added directly to PR #17 rather than to PR #18, because PR #18
53+
("fix(release): resolve reusable workflow reference") is an unrelated
54+
release-engineering fix and does not contain a merge-flow threat table to
55+
update.

0 commit comments

Comments
 (0)