ci(migrations): re-check open PRs when a new head lands on main (CHOO-2689) - #441
Conversation
![semgrep-code-sandbox-quantum[bot]](https://avatars.githubusercontent.com/u/96544495?s=60&v=4){kind=small}
| *args: str, cwd: Path = REPO_ROOT, check: bool = True | ||
| ) -> subprocess.CompletedProcess[str]: | ||
| return subprocess.run( | ||
| list(args), cwd=cwd, check=check, capture_output=True, text=True |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
Why this might be safe to ignore:
The tainted repository value is passed as one element in a list to subprocess.run, with shell execution not enabled, so shell metacharacters cannot cause command injection. GITHUB_REPOSITORY is also a GitHub-provided repository identifier rather than arbitrary user input in this script.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>scripts/check_migration_heads_for_open_prs.py</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/sandbox-quantum/switch/blob/06598b33a7ffea479fa039d41c2f3b4450097604/scripts/check_migration_heads_for_open_prs.py#L245 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 245] os.environ</a>"]
end
%% Intermediate
subgraph Traces0[Traces]
direction TB
v2["<a href=https://github.com/sandbox-quantum/switch/blob/06598b33a7ffea479fa039d41c2f3b4450097604/scripts/check_migration_heads_for_open_prs.py#L245 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 245] repo</a>"]
v3["<a href=https://github.com/sandbox-quantum/switch/blob/06598b33a7ffea479fa039d41c2f3b4450097604/scripts/check_migration_heads_for_open_prs.py#L247 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 247] open_prs_touching_migrations</a>"]
v4["<a href=https://github.com/sandbox-quantum/switch/blob/06598b33a7ffea479fa039d41c2f3b4450097604/scripts/check_migration_heads_for_open_prs.py#L131 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 131] repo</a>"]
v5["<a href=https://github.com/sandbox-quantum/switch/blob/06598b33a7ffea479fa039d41c2f3b4450097604/scripts/check_migration_heads_for_open_prs.py#L132 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 132] run</a>"]
v6["<a href=https://github.com/sandbox-quantum/switch/blob/06598b33a7ffea479fa039d41c2f3b4450097604/scripts/check_migration_heads_for_open_prs.py#L124 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 124] args</a>"]
end
v2 --> v3
v3 --> v4
v4 --> v5
v5 --> v6
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/sandbox-quantum/switch/blob/06598b33a7ffea479fa039d41c2f3b4450097604/scripts/check_migration_heads_for_open_prs.py#L127 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 127] list(args)</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
Traces0:::invis
File0:::invis
%% Connections
Source --> Traces0
Traces0 --> Sink
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by dangerous-subprocess-use-tainted-env-args.
You can view more details about this finding in the Semgrep AppSec Platform.
|
A few comments:
fetch_pr_revisions does git ls-tree -r FETCH_HEAD -- versions/, which returns the PR head's whole versions directory, not just the files it adds. check_pr then does base_revisions + fetch_pr_revisions(...), so all 65 migrations the two sides share appear twice. So every PR that touches migrations gets state: failure with "Merging this PR now would break the migration chain — rebase onto the latest main", unconditionally, and the job exits 1 on every main push. Worse, describe_chain_problem returns on duplicates first, so the two-head case it exists to catch is never reached. The docstring's reasoning is right ("the versions directory only ever gains files, so a plain union ... is the merge result") — it's a set union that was implemented as list concatenation. Build the union keyed by filename, PR side winning: merged = {r.filename: r for r in base_revisions} With that, against the real tree: a PR chaining off the current head reports clean, and a sibling off the head's parent reports expected exactly one migration head, got ['a3f61c02d5be', 'sibrev00001']. That's the intended behaviour, and it's what the unchecked end-to-end box in the test plan would have caught.
No --limit. The repo has 33 open PRs today: Three PRs get no check and nothing says so. Pass --limit 1000, or better --search to filter server-side.
On push, github.ref is refs/heads/main for every merge. Merge a migration PR, then merge anything else a minute later: the second run cancels the first, and because paths-filter on push only sees the second commit's files, migrations is false and the job is skipped. No statuses are posted, silently — on precisely the busy day this is meant to cover. Either give push runs their own group (pr-ci-${{ github.ref }}-${{ github.event_name == 'push' && github.sha || '' }}) or set cancel-in-progress: ${{ github.event_name == 'pull_request' }}.
run() is check=True everywhere and check_pr has no error handling, so a single gh/git failure — a transient API error, a fetch that can't resolve — raises out of the list comprehension in main(). PRs already checked keep their status; the rest get nothing, and the only signal is a red job on main that looks identical to a genuine finding. Wrap each PR, post an error status for the one that failed, and keep going. That matters because two inputs can raise on ordinary content:
Both should be reported as a problem with that file, not a stack trace. |
|
All four addressed in 0f564fa, and each verified against the real 65-revision tree the way the review was. 1. The union — blocker. Keyed by filename now, PR side winning, in a You were right that the docstring's reasoning was sound and the implementation wasn't; the reasoning now lives on the helper that actually does the union. 2. 3. Concurrency. Push runs now carry the commit in their group: group: pr-ci-${{ github.ref }}-${{ github.event_name == 'push' && github.sha || '' }}Your first suggestion rather than the second: with 4. The sweep, and the two parse inputs. Each PR now runs inside
On the Semgrep finding — the taint it drew ( Not addressed, and I don't think it should be here: the branch ruleset gap you found in the description ( |
|
Parking this until Phase 2 (CHOO-2624) is done. Not because of the review — all four points are fixed in 0f564fa and CI is green apart from the stale Semgrep finding. The reason is scope: this touches Nothing in the Phase 2 stack (#444 → #445 → #446 → #447, plus #442) depends on this. Until it lands, a second head that slips past a stale green check is still caught the way it has been: by hand, with a merge revision. Two things to pick up when it comes off the shelf:
|
…-2689) actions/checkout already checks out refs/pull/<n>/merge on pull_request events, so test_migration_chain.py already runs against the merge preview, not the branch tip. The actual gap is that GitHub keeps that preview current as the base moves but never re-runs the workflow to notice, and merging isn't gated on being up to date -- so a PR whose last CI run was green can still merge after main gains a colliding migration. That's exactly what happened twice in #404/#426, hand-fixed with merge revisions b47e0c39a1f5 and c81f4a06d2b7. Add a job that runs on push to main when the migrations directory changes: it re-checks every other open PR touching migrations by reading that PR's added revision files as plain text alongside the base as it now stands, then posts the result as a commit status on the PR's own head commit. A PR that would create a second head goes red the moment the collision exists, without needing a new push. The check reads revision/down_revision with a regex and ast.literal_eval instead of going through Alembic's ScriptDirectory, which builds its graph by importing every revision file. Importing a file from someone else's open PR on a public repo, from a job that runs on push to main with a token that can write commit statuses, would be running unreviewed code with that token's privileges. The per-PR pull_request run keeps using the real test_migration_chain.py unchanged, since there the code under test is the code already under review.
…2689) The repository slug and the tracked path both reach a git or gh argument list. Nothing runs through a shell, so the risk was never a metavariable — it is a leading dash, which git reads as an option rather than a path. Both are now matched against an explicit pattern first, which also keeps the reachable surface to files that could actually be migrations.
…689)
The scanner flagged the repository slug reaching a subprocess argument
list, and its own note said the flow was safe: list form, no shell, and a
value GitHub supplies. Both true, and still the wrong thing to argue
about — the value did not need to be there at all.
gh resolves the repository from its own environment, and fills its
`{owner}/{repo}` placeholders in an API path, so `--repo` and the
interpolated status URL both go away. What remains from outside the
script is a tracked path and a commit id, each matched against a pattern
before it is passed. Removing the flow beats annotating it.
The remaining scanner finding was the run URL, which comes from the environment and was interpolated into a `-f` argument. The rule is about environment values reaching an argument list, and it is right that this is the wrong shape even though no shell is involved. The whole request body now goes to `gh api --input -` as JSON on stdin, which takes the description text off the command line too. Nothing variable-length is an argument any more.
…aborted sweep (CHOO-2689) Four defects found in review, each of which would have made the check useless in a different way. The base and the PR's head both carry the whole versions directory, so concatenating them counts every shared migration twice. Every PR that touched migrations would have gone red on a duplicate-id error, and because duplicates are reported first, the two-head case the job exists to catch was unreachable. The union is now keyed by filename. `gh pr list` stops at 30 and says nothing about the rest; the repository has more open than that, so the PRs it skipped got no check and no signal. It now asks for a limit it will not reach, and fails loudly if it ever does. Every push to main shares one `github.ref`, so the concurrency group let a later merge cancel the run for the migration that had just landed — and the replacement run skips this job, because its own commit touched no migration. Push runs now carry the commit in their group. A single `gh` or `git` failure aborted the whole sweep from inside a list comprehension, leaving the PRs not yet reached with nothing on them and a red job on main indistinguishable from a genuine finding. Each PR is now checked in isolation and gets an error status of its own. Two inputs could raise on ordinary content: a `down_revision` tuple spread over several lines, which a line-anchored regex hands to `literal_eval` half-finished, and a non-migration `.py` under `versions/`, which came out as a bare `KeyError`. Reading the assignments off `ast.parse`'s tree removes the first outright, and both are now reported as a problem naming the file. Nothing is imported or executed, which was the point of not using Alembic here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0f564fa to
9ddfbc4
Compare
2 participants
Summary
Two migrations chained off the same parent are each green in isolation and produce a second head only once both merge — at which point the server crash-loops on boot, because it self-migrates with
alembic upgrade headand that is ambiguous with more than one. It happened twice in 48 hours (#404 with #397, then the templates migration) and was hand-fixed with merge revisions both times.The gap is not where the ticket says. A manual
alembic stampcures nothing permanently, and the per-PR check is not testing the wrong tree:actions/checkoutalready checks out GitHub's merge preview on apull_requestevent, verified against a real run's logs. The gap is staleness — GitHub recomputes that preview as the base moves but nothing re-runs the workflow, and merging is not gated on the branch being current, so a day-old green check still satisfies the merge button.So: when a migration lands on
main, re-check every other open PR that also adds one, and post a failing status on it immediately rather than letting the next deploy find out.It reads the chain statically, and that is the security-relevant part of the design. The obvious implementation overlays the other PR's files and runs the existing test — but that test builds an Alembic
ScriptDirectory, and Alembic imports every revision module to construct the graph. On amainworkflow run holding a token that can write statuses, in a public repository, that is executing unreviewed Python from any pull request anyone cares to open. This parsesrevisionanddown_revisionout of the file text with a regex andast.literal_evalinstead — the same way the existingtest_revision_ids_are_uniquealready reads revision ids, and for the same reason — and derives heads as "any revision that is nobody's parent". Nothing is imported, nothing is checked out, and the job needs neither a database nor the project's dependencies.The per-PR run keeps using the real test unchanged: there, the code being imported is the code under review.
Test plan
expected exactly one migration head, got ['a3f61c02d5be', 'fake000000aa'].down_revision(checked against the realb47e0c39a1f5).just check,just typecheckandmypyon the script directly all pass; workflow YAML validated.Note for whoever has repository admin
Out of scope here, but found while investigating: the branch ruleset's required status checks are only "Detect changed trees" and "Secret scan".
Backend (core)is not required, so a red test suite does not block a merge today.