Skip to content

Merge-queue validator can be poisoned by an earlier step in the shared changes job #2950

Description

@devantler

🤖 Generated by the Agentic Engineer

Evidence

#2949 brings the merge-queue publication-contract gate up to the cd.yaml gate's rules with one deliberate exception: gateJobRunsOnlyItsValidator is not applied.

On the direct-push route that rule constrains what else may run in the gate job, because the runner's $GITHUB_ENV and $GITHUB_PATH bridges carry across steps. A step earlier in the job can therefore export a variable or prepend a PATH entry that shadows go for a later step, while that later step's own run block still contains exactly the two permitted command lines and passes every check aimed at it.

The merge-queue validator lives in changes, a shared job that also runs:

📑 Checkout · ⚙️ Setup Go · 🩹 Validate merge-group heal contract
🖋️ Validate DR signing contract · 🚦 Validate concurrency queue values · 🔍 Filter paths

gateJobRunsOnlyItsValidator cannot be applied as-is, because it exists to assert the job runs nothing but its validator — true of the dedicated validate-publication-contract job, false of changes by design.

Not currently exploited: no step in changes writes $GITHUB_ENV or $GITHUB_PATH (verified on origin/main). ⚙️ Setup Go does place GOTOOLCHAIN into the step environment through the action, which shows the mechanism is live and benign today.

Why it matters

#2949 closes the workflow-, job- and step-scope env and shell bypasses, and the run-block bypass. This is the one remaining member of that family on the merge-queue route, so the route is close to parity with cd.yaml but not at it — and a reader of #2949 could reasonably assume otherwise. It needs a workflow edit, exactly like every other bypass this contract refuses; it is not reachable by an outside contributor.

Expected behaviour

The validator step in changes is protected against environment poisoning by steps that precede it in the same job, without requiring changes to become a dedicated gate job.

Options (a spike may be the right first move)

  1. Move the validator into its own job. Cleanest — it inherits gateJobRunsOnlyItsValidator unchanged and the needs rule from fix(ci): enforce the publication-contract gate on the merge-queue route #2949 already requires production to depend on whichever job carries it. Costs one extra runner and a checkout/setup-go per run.
  2. Refuse $GITHUB_ENV / $GITHUB_PATH writes in the steps preceding the validator within changes. No new job, but it is a denylist over shell syntax — the pattern fix(ci): enforce the publication-contract gate on the merge-queue route #2949's own comments record as an unbounded guessing game that an allowlist replaced.
  3. Accept and document, on the grounds that it needs a workflow edit. Weakest, and inconsistent with refusing the six sibling bypasses.

Option 1 is the recommendation: it reuses an existing, already-tested rule instead of writing a second weaker one, which is the principle the rest of this validator is built on.

Acceptance criteria

  • A step preceding the validator that writes $GITHUB_ENV or $GITHUB_PATH fails the contract, or the validator no longer shares a job with steps that could.
  • The shipped ci.yaml passes unchanged.
  • An ablation per new rule, each failing a disjoint arm and asserting its own error reason.
  • cd.yaml's existing coverage is unchanged.

Size

Small–medium, depending on the option chosen.

Provenance

Raised in my own local review round on #2949 (all three external lanes were quota-limited at that head) and stated in that PR rather than left implicit. Kept out of #2949 to keep it one concern.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Projects

Status
✅ Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions