Skip to content

Commit c07ea76

Browse files
committed
Dismiss CodeQL #14 with documented rationale + guard test
CodeQL alert #14 (untrusted-checkout-toctou/critical) is the third re-fire of the same rule family on this workflow (#11/#12#13#14). PR #427's fork-skip gate was load-bearing for #11/#12 but the rule re-classified as #14 at a shifted line range with escalated severity. A structural fix (checkout BASE_SHA only; `git show` for PR-head reads) would close the rule cleanly but degrade reviewer quality: pr_review.md explicitly instructs Codex to `grep -n "pattern" diff_diff/*.py` for pattern-consistency checks, and Codex's `sandbox: read-only` lets it inspect workspace files. Under restructure, those operations would search BASE content, missing patterns added in the PR. Workarounds that materialize PR-head content via non-`actions/checkout` mechanisms (git worktree, overlay) satisfy CodeQL's pattern matcher without changing runtime risk — security theater dressed up. The honest path: dismiss with documented rationale + guard test. The actual threat model accepted by the dismissal: - Codex runs with sandbox: read-only — can read PR-head files, cannot execute or write them - head_sha is API-pinned in the resolve-pr step before checkout; TOCTOU window is sub-second and bounded by the is_fork gate - Same-repo PR contributors who can push to PR head branches already have write access to push to main; the checkout doesn't expand attack surface - Fork PRs are skipped entirely (PR #427 fork-skip gate) The dismissal becomes invalid only if a future workflow edit adds a step that EXECUTES PR-head content. Guard test TestWorkflowDoesNotExecutePRHeadCode in tests/test_openai_review.py fails CI on any of: pip install, pytest, npm install, cargo run/test, make, ./configure, maturin develop/build, poetry install/run, pdm, uv sync/run, tox, setup.py, etc. The maturin entries are load-bearing for this repo (Rust build per CLAUDE.md is the most likely future regression vector). The dismissal API call (gh api PATCH .../code-scanning/alerts/14) will be run post-merge, separately from this PR.
1 parent 7c1e9d0 commit c07ea76

2 files changed

Lines changed: 131 additions & 0 deletions

File tree

.github/workflows/ai_pr_review.yml

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,40 @@ jobs:
5555
)
5656
5757
steps:
58+
# ─────────────────────────────────────────────────────────────────
59+
# CodeQL alert #14 (untrusted-checkout-toctou/critical) was
60+
# dismissed on 2026-05-14 as "won't fix" with the rationale
61+
# documented below. The dismissal is valid ONLY while ALL of
62+
# the following hold:
63+
# 1. The Codex action runs with sandbox: read-only (see the
64+
# Run Codex step below). No step EXECUTES PR-head FILE
65+
# BYTES — no `pip install -e .`, `pytest`, `npm install`,
66+
# `cargo run`, `make`, `./configure`, `maturin develop`,
67+
# or `python3 <pr-file>.py` against checked-out PR-head
68+
# files. Inline `python3 -c '...'` invocations operating
69+
# on sanitized environment variables (PR_TITLE, PR_BODY,
70+
# PREV_REVIEW) are SAFE — the script body comes from base,
71+
# not from PR head.
72+
# 2. The fork-skip gate (`is_fork == 'false'`) gates every
73+
# post-resolve step, including both actions/checkout
74+
# invocations.
75+
# 3. The author_association check on issue_comment /
76+
# review-comment events restricts triggers to
77+
# OWNER/MEMBER/COLLABORATOR.
78+
# 4. head_sha is API-pinned in this resolve-pr step before
79+
# checkout.
80+
#
81+
# If you are adding a step that VIOLATES any of these
82+
# invariants, this dismissal is invalid. Either remove the
83+
# offending step OR restructure the workflow to checkout
84+
# BASE_SHA only and use `git show` for PR-head reads (degrades
85+
# reviewer quality — see plan archive).
86+
#
87+
# Guard test: tests/test_openai_review.py
88+
# ::TestWorkflowDoesNotExecutePRHeadCode
89+
# CI fails closed if a forbidden execution pattern is
90+
# introduced.
91+
# ─────────────────────────────────────────────────────────────────
5892
- name: Resolve PR number + metadata
5993
id: pr
6094
uses: actions/github-script@v9

tests/test_openai_review.py

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2665,6 +2665,103 @@ def test_workflow_post_resolve_steps_gated_on_is_fork(self, workflow_text):
26652665
)
26662666

26672667

2668+
class TestWorkflowDoesNotExecutePRHeadCode:
2669+
"""Guards CodeQL #14 dismissal — see the workflow comment block above
2670+
the resolve-pr step in `.github/workflows/ai_pr_review.yml` for the
2671+
full rationale and invalidation conditions. The dismissal accepts
2672+
that the workflow CHECKS OUT PR-head content but is valid only
2673+
while the workflow does not EXECUTE that content."""
2674+
2675+
FORBIDDEN_EXECUTION_PATTERNS = (
2676+
"pip install",
2677+
"pytest",
2678+
"npm install",
2679+
"yarn install",
2680+
"cargo run",
2681+
"cargo test",
2682+
"make ",
2683+
"./configure",
2684+
"bundle exec",
2685+
"rake ",
2686+
"go test",
2687+
"go run",
2688+
"maturin develop",
2689+
"maturin build",
2690+
"poetry install",
2691+
"poetry run",
2692+
"pdm install",
2693+
"pdm run",
2694+
"uv sync",
2695+
"uv run",
2696+
"tox",
2697+
"setup.py",
2698+
)
2699+
2700+
@pytest.fixture
2701+
def workflow_text(self):
2702+
assert _SCRIPT_PATH is not None
2703+
repo_root = _SCRIPT_PATH.parent.parent.parent
2704+
wf = repo_root / ".github" / "workflows" / "ai_pr_review.yml"
2705+
if not wf.exists():
2706+
pytest.skip("workflow not found")
2707+
return wf.read_text()
2708+
2709+
def test_workflow_run_blocks_have_no_forbidden_execution_patterns(
2710+
self, workflow_text
2711+
):
2712+
"""If this fails, the CodeQL #14 dismissal is invalid. Either
2713+
remove the offending step or restructure per the dismissed plan
2714+
(checkout BASE_SHA only + git show for PR-head)."""
2715+
import re
2716+
2717+
# Match every `run: |` block's body. The body is the indented
2718+
# content following `run: |` until the next step (next ` - `
2719+
# at 6-space indent) or end of file. Body lines are at 10+ space
2720+
# indent (the `run: |` itself is at 8-space step-property indent;
2721+
# block scalars indent further).
2722+
run_block_re = re.compile(
2723+
r"^\s+run:\s*\|\s*\n((?:^(?:[ ]{8,}|\s*$).*\n?)*)",
2724+
re.MULTILINE,
2725+
)
2726+
run_blocks = run_block_re.findall(workflow_text)
2727+
assert run_blocks, "No `run:` blocks found — extraction regex broke"
2728+
2729+
violations = []
2730+
for i, block in enumerate(run_blocks):
2731+
for pattern in self.FORBIDDEN_EXECUTION_PATTERNS:
2732+
if pattern in block:
2733+
snippet = next(
2734+
(line for line in block.splitlines() if pattern in line),
2735+
"",
2736+
).strip()
2737+
violations.append(
2738+
f"run-block #{i}: forbidden pattern {pattern!r} in: {snippet}"
2739+
)
2740+
assert not violations, (
2741+
"CodeQL #14 dismissal invalidated by forbidden execution "
2742+
"patterns in workflow run blocks:\n" + "\n".join(violations)
2743+
+ "\nSee `.github/workflows/ai_pr_review.yml` comment block "
2744+
"above the resolve-pr step for context."
2745+
)
2746+
2747+
def test_workflow_dismissal_comment_block_present(self, workflow_text):
2748+
"""The comment block that documents the #14 dismissal must stay
2749+
attached to the workflow file. If a future edit removes it, the
2750+
rationale lives only in the GitHub Security UI's
2751+
dismissed_comment field — easy to lose track of."""
2752+
assert "CodeQL alert #14" in workflow_text, (
2753+
"Workflow must keep the #14 dismissal rationale comment "
2754+
"block above the resolve-pr step."
2755+
)
2756+
assert "won't fix" in workflow_text, (
2757+
"Comment block must cite the dismissal reason for grep-ability."
2758+
)
2759+
assert "TestWorkflowDoesNotExecutePRHeadCode" in workflow_text, (
2760+
"Workflow comment must reference this guard test by name so "
2761+
"future maintainers can find it."
2762+
)
2763+
2764+
26682765
class TestExtractResponseText:
26692766
def test_prefers_output_text_field(self, review_mod):
26702767
result = {"output_text": "Direct text.", "output": []}

0 commit comments

Comments
 (0)