diff --git a/.github/workflows/ai_pr_review.yml b/.github/workflows/ai_pr_review.yml index 9b2a3765..060d2331 100644 --- a/.github/workflows/ai_pr_review.yml +++ b/.github/workflows/ai_pr_review.yml @@ -22,10 +22,18 @@ jobs: runs-on: ubuntu-latest # Run if: - # - PR opened, OR + # - PR opened (same-repo only — fork PRs are skipped here at the workflow + # level, since `head.repo.full_name` is available on `pull_request` + # events). For comment-triggered events, the same-repo check happens at + # the step level via `steps.pr.outputs.is_fork` (we have to API-fetch + # the PR there because `issue_comment` event payloads don't include + # head-repo info). Closes CodeQL alerts #11, #12 (untrusted checkout). # - Comment "/ai-review" on a PR by a collaborator/member/owner (issue or inline diff comment) if: | - (github.event_name == 'pull_request') || + ( + github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository + ) || ( github.event_name == 'issue_comment' && github.event.issue.pull_request != null && @@ -75,6 +83,27 @@ jobs: const headRepoFullName = pr.data.head.repo?.full_name || `${owner}/${repo}`; + // Fork detection for comment-triggered events. Workflow-level + // `if:` already excludes fork PRs for `pull_request` events + // (uses `github.event.pull_request.head.repo.full_name`), but + // `issue_comment` / `pull_request_review_comment` payloads + // don't include head-repo info — we have to API-fetch. + // Subsequent steps gate on `steps.pr.outputs.is_fork == 'false'`. + // Uses raw `pr.data.head.repo?.full_name` (NOT the fallback + // `headRepoFullName`): a deleted fork should still skip the + // workflow because the code IS untrusted from CodeQL's + // perspective. `undefined !== "owner/repo"` -> is_fork = true. + // Closes CodeQL alerts #11, #12 (untrusted checkout TOCTOU). + const isFork = + pr.data.head.repo?.full_name !== `${owner}/${repo}`; + if (isFork) { + core.notice( + `AI review skipped: PR head is from fork ` + + `${pr.data.head.repo?.full_name || "(deleted)"}. ` + + `See CodeQL alerts #11, #12 for the security rationale.` + ); + } + core.setOutput("number", prNumber); core.setOutput("title", pr.data.title || ""); core.setOutput("body", pr.data.body || ""); @@ -84,6 +113,7 @@ jobs: core.setOutput("head_ref", pr.data.head.ref); core.setOutput("head_repo_full_name", headRepoFullName); core.setOutput("state", pr.data.state); + core.setOutput("is_fork", isFork ? "true" : "false"); # Closed/merged PR (e.g. `/ai-review` rerun on a merged PR): # use the base-repo mirror of the PR head, which GitHub keeps @@ -91,7 +121,7 @@ jobs: # The previous workflow used `refs/pull//merge`, which is # garbage-collected on closed PRs — this path replaces that. - uses: actions/checkout@v6 - if: steps.pr.outputs.state != 'open' + if: steps.pr.outputs.state != 'open' && steps.pr.outputs.is_fork == 'false' with: ref: refs/pull/${{ steps.pr.outputs.number }}/head @@ -102,12 +132,13 @@ jobs: # (see .claude/commands/submit-pr.md:327-345). head_sha is # guaranteed to exist on the head repo for an open PR. - uses: actions/checkout@v6 - if: steps.pr.outputs.state == 'open' + if: steps.pr.outputs.state == 'open' && steps.pr.outputs.is_fork == 'false' with: repository: ${{ steps.pr.outputs.head_repo_full_name }} ref: ${{ steps.pr.outputs.head_sha }} - name: Pre-fetch base SHA + if: steps.pr.outputs.is_fork == 'false' run: | set -euo pipefail # base_sha lives on the base repo (github.repository), which differs @@ -119,6 +150,7 @@ jobs: - name: Fetch previous AI review (if any) id: prev_review + if: steps.pr.outputs.is_fork == 'false' uses: actions/github-script@v9 with: script: | @@ -136,6 +168,7 @@ jobs: core.setOutput("found", last ? "true" : "false"); - name: Build review prompt with PR context + diff + if: steps.pr.outputs.is_fork == 'false' env: PR_TITLE: ${{ steps.pr.outputs.title }} PR_BODY: ${{ steps.pr.outputs.body }} @@ -415,6 +448,7 @@ jobs: - name: Run Codex id: run_codex + if: steps.pr.outputs.is_fork == 'false' uses: openai/codex-action@v1 with: openai-api-key: ${{ secrets.OPENAI_API_KEY }} @@ -426,6 +460,7 @@ jobs: effort: xhigh - name: Post PR comment (new on every event except initial open) + if: steps.pr.outputs.is_fork == 'false' uses: actions/github-script@v9 env: CODEX_FINAL_MESSAGE: ${{ steps.run_codex.outputs.final-message }} diff --git a/tests/test_openai_review.py b/tests/test_openai_review.py index 66b89a70..2d641ab8 100644 --- a/tests/test_openai_review.py +++ b/tests/test_openai_review.py @@ -2557,6 +2557,114 @@ def test_notice_caps_output_at_10_files( assert "and 15 more" in err +class TestWorkflowForkSkip: + """The AI review workflow must skip PRs from forks to avoid the + untrusted-checkout pattern that CodeQL flagged as alerts #11 and #12. + Two-layer skip: + 1. Workflow-level `if:` gates `pull_request` events on + `head.repo.full_name == github.repository` + 2. The resolve-pr step sets `is_fork` output (via API fetch); + all 7 post-resolve steps gate on `is_fork == 'false'`. + + These contract tests pin both layers — without them, a future workflow + refactor could drop the gate and re-introduce the CodeQL alerts.""" + + @pytest.fixture + def workflow_text(self): + assert _SCRIPT_PATH is not None + repo_root = _SCRIPT_PATH.parent.parent.parent + wf = repo_root / ".github" / "workflows" / "ai_pr_review.yml" + if not wf.exists(): + pytest.skip("workflow not found") + return wf.read_text() + + def test_workflow_pull_request_if_block_excludes_fork_prs(self, workflow_text): + """Layer 1: the workflow `if:` block for `pull_request` events must + require head.repo.full_name == github.repository so fork PRs never + start a workflow run.""" + assert ( + "github.event.pull_request.head.repo.full_name == github.repository" + in workflow_text + ), ( + "workflow `if:` for pull_request events must check that the PR " + "head is from the same repo (not a fork) — required to clear " + "CodeQL alerts #11/#12 (untrusted checkout)." + ) + + def test_workflow_resolve_pr_step_sets_is_fork_output(self, workflow_text): + """Layer 2: the resolve-pr github-script step must set the `is_fork` + output that subsequent steps gate on. Comment-triggered events + (`issue_comment`, `pull_request_review_comment`) can't be gated at + the workflow `if:` level (event payload doesn't include head-repo + info), so the gate happens at the step level via this output.""" + assert 'core.setOutput("is_fork"' in workflow_text, ( + "resolve-pr step must set `is_fork` output so post-resolve steps " + "can gate on `steps.pr.outputs.is_fork == 'false'`." + ) + + def test_workflow_post_resolve_steps_gated_on_is_fork(self, workflow_text): + """Every step in the `review` job that runs AFTER the `resolve-pr` + step must include `steps.pr.outputs.is_fork == 'false'` in its + `if:` clause. + + Per CodeQL alerts #11/#12, no step that could touch untrusted PR + contents (or run while OPENAI_API_KEY is in scope) may execute + on a fork PR. The resolve-pr step itself only API-fetches PR + metadata via GITHUB_TOKEN — safe to run before the gate is + computed. Every step after must be gated. + + The earlier (PR #427 R0) version of this test counted the string + `is_fork == 'false'` globally with `>= 7`, which had two false- + negative modes: + (a) a real gate could be removed — string count drops 8→7, + still passes + (b) a new ungated post-resolve step could be added — gate + count stays at 7, total step count grows, passes + + This rewrite (R1, addressing the reviewer's P3) anchors on: + - `^ if:` at 8-space indent (the step-property indent + level for the review job's nested `if:` keys), excluding + the JS doc comment inside the resolve-pr step's `script: |` + block which would not match this anchor + - `^ - (name|uses):` at 6-space indent (step-list-item + indent), counting every step in the job + + Then asserts `gated_steps == total_steps - 1` (resolve-pr is the + only legitimately ungated step). Catches both failure modes + above.""" + import re + + # `if:` lines at step-property indent (8 spaces) containing the + # gate. Allows combined conditions like + # `if: steps.pr.outputs.state == 'open' && steps.pr.outputs.is_fork == 'false'`. + gate_re = re.compile( + r"^ if:.*is_fork == 'false'", re.MULTILINE + ) + gates = gate_re.findall(workflow_text) + + # All step starts in the review job (` - name:` or + # ` - uses:` at 6-space indent). + step_start_re = re.compile( + r"^ - (?:name|uses):", re.MULTILINE + ) + steps = step_start_re.findall(workflow_text) + + # The resolve-pr step is the only ungated step (it sets the + # output that all subsequent steps gate on). + expected_gates = len(steps) - 1 + assert len(gates) == expected_gates, ( + f"Fork-skip gate invariant violated: found {len(gates)} " + f"gated step(s) but {len(steps)} total step(s) in the " + f"`review` job — expected exactly {expected_gates} gates " + f"(every step except resolve-pr must include " + f"`is_fork == 'false'` in its `if:`). Either a gate was " + f"removed or a new post-resolve step was added without one. " + f"Per CodeQL alerts #11/#12, every post-resolve step must " + f"be gated to prevent untrusted-checkout execution on fork " + f"PRs." + ) + + class TestExtractResponseText: def test_prefers_output_text_field(self, review_mod): result = {"output_text": "Direct text.", "output": []}