Harden CI: bind matrix.source before it reaches the smoke-test shell - #5363
Conversation
The "Install into isolated venv" step interpolated ${{ matrix.source }}
directly into its run: block. GitHub substitutes ${{ }} expressions
textually, before bash parses the line, so the value arrives as script
source rather than as data and the surrounding quotes give no protection.
zizmor reports this as template-injection (Medium) -- the last
non-informational finding of that family in the repository.
Nothing is reachable through it today, and this is defence in depth rather
than a fix for an exploitable path: the matrix is closed, because both
fromJSON arrays feeding `source` are literals, so the value can only ever
be the string "wheel" or "pypi". Binding it keeps that true if the matrix
later starts deriving its value from a workflow input or an event payload,
which is the change that would otherwise turn this line into a live
injection site without anyone editing it.
The value now travels through a step-level env: var and is read as a
quoted shell variable. The other two references (the job `name:` and the
step `if:`) are expression contexts, not shell, and are left as they are.
Verified: all 35 workflow and composite-action files parse; zizmor on this
file goes 1 -> 0 findings; and the run block was executed under `bash -e`
with a stubbed pip for both matrix values -- resolved argv is byte-identical
to the old form in each case (wheel -> local dist wheel, pypi -> --no-cache-dir
from PyPI).
No-PRD: CI-only change under .github/, exempt from the product-record gate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FcrEBdhyYY8etBZMtAL6UK
✅ Drift Bot (ClawMetry): no drift detectedDrift Bot analyzed the changed files against this project's blueprints and requirements and found no drift. |
|
| Spec | Result |
|---|---|
| Drift Bot | passed (no drift detected) |
| OSS golden path | passed [92s] |
| Cross-repo handoff | passed [183s] |
| Syntax & Lint | passed [183s] |
| Wheel install & assets | passed [1660s] |
| Store invariants | passed [1660s] |
| API Tests (3 OS) | passed, 3/3 legs [1721s] |
As of now 30+ checks on 05fb567 are green and zero are failing.
Why this is not this PR's failure
The diff is 10 lines in .github/workflows/api-latency-smoke.yml. That file defines the API Latency Smoke workflow, which is not among the twelve specs the gate aggregates — so this change cannot influence any of the checks that timed out. The cause is queue depth, which is repo-wide tonight: the CI run for main itself (run #9114) sat queued from 21:12, and #5364 records "100+ queued, 19 running, 40 open PRs." This PR's own jobs were queued 21:43 → 22:48 before a runner picked them up.
The fix that exists, and why I am not porting it here
#5332 raises MAX_WAIT 1800 → 3600 and timeout-minutes 35 → 65, and its own comment names this exact scenario: "This ceiling exists to handle runner starvation on days with many concurrent PRs." It is the right fix and it is already in flight.
I am deliberately not porting it into this PR, because here it would be counterproductive rather than merely redundant. scripts/e2e_gate.py polls the Check Runs API for results on the head SHA — it does not execute the other jobs. So:
- Pushing a commit invalidates the 30+ checks already green on
05fb567and re-queues all of them behind a backlog that is currently the binding constraint. That makes this PR strictly slower to green, not faster. - Re-running only the gate job re-polls the results already on this SHA, changing nothing about the diff and consuming no runner capacity beyond one short job.
So the cheap, deterministic path is a single re-run of the gate once the five stragglers land — not a push. E2E Browser Tests began at 23:20:06, twenty-eight seconds after the gate gave up; the rest are still queued.
What happens next
I am holding the one re-run until those specs have actually completed, so it is spent on a run that can pass rather than on a second timeout. If any of them fails on its merits once it runs, that is a real signal and I will treat it as this PR's to fix.
Nothing here needs an author decision — flagging only so a reader doesn't mistake a red required check for a broken change.
Generated by Claude Code
✅ Drift Bot (ClawMetry): no drift detectedDrift Bot analyzed the changed files against this project's blueprints and requirements and found no drift. |
|
✨ auto-fixed: merged main into branch (was BEHIND at 4165e76) Generated by Claude Code |
What
api-latency-smoke.yml's "Install into isolated venv" step interpolated${{ matrix.source }}directly into itsrun:block. The value now travels through a step-levelenv:var and is read as a quoted shell variable.GitHub substitutes
${{ }}expressions textually, before bash parses the line, so an expression in arun:block arrives as script source rather than as data — the surrounding quotes give no protection. zizmor reports this astemplate-injection(Medium), and it was the last non-informational finding of that family in the repository.Scope, stated plainly
Nothing is reachable through this today. The matrix is closed:
Both
fromJSONarrays are literals, somatrix.sourcecan only ever be the stringwheelorpypi. This is defence in depth, not a fix for an exploitable path — zizmor flags it because it cannot prove the matrix is closed, and I am not claiming more than that.What the binding buys is durability. If the matrix ever starts deriving its value from a workflow input or an event payload, this line becomes a live injection site with nobody editing it, and the change would not look like a security change. Binding now means that edit stays safe.
The other two references to
matrix.source— the jobname:and the stepif:— are expression contexts rather than shell, and are correct as they are. They are left alone.Verification
yaml.safe_loadover all workflow + composite-action filesapi-latency-smoke.ymlrun:blocksBehaviour is unchanged, and that was measured rather than assumed. The run block was extracted and executed under
bash -ewith a stubbedpip, for both matrix values, comparing the old (pre-substituted) form against the new (env-bound) form:matrix.sourcewheelpip install dist/clawmetry-*.whl flask waitress cryptography duckdb requestspypipip install --no-cache-dir clawmetry flask waitress cryptography duckdb requestsNotes for review
One file, one step,
.github/only — exempt from the product-record gate (No-PRDrecorded in the commit message).This is the last item I could find in this hardening batch that is both unclaimed and safe to fix without a judgement call. For the record, the remaining zizmor findings on this repo are deliberately not in this PR:
artipacked× 13 — 10 are covered by open PRs Harden CI: stop the build and publish jobs persisting the job credential #5331, Harden CI: stop the screenshot job persisting the job credential #5338 and Harden CI: stop the i18n autotranslate jobs persisting the job credential #5343. The other 3 (auto-deploy-cloud.yml:82,release-on-merge.yml,auto-quarantine.yml) all push from the working copy, so their credential has a real consumer and removing it would break the release path.dangerous-triggers× 2 (workflow_runinauto-deploy-cloud.yml,release-canary.yml) andcache-poisoning× 1 (publish.yml) — load-bearing for the release and publish pipelines. These want a deliberate design decision, not a sweep.github-env× 2 in.github/actions/setup-openclaw/action.yml— legitimateGITHUB_PATH/GITHUB_ENVwrites with internal values; restructuring them is a bigger change than a hardening tick should make unasked.Generated by Claude Code