fix(registry): verify staged asset content before publishing manifests - #3832
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed at head 61757bee04445fa066410ac2747bac6a799995c3 (3 files, +112/−24). Verified locally in an isolated checkout built the way the Test job builds (parsers/lint/studio-server → core → core runtime), then node --import tsx --test scripts/host-registry-assets.test.ts, typecheck:scripts, oxlint/oxfmt on the two changed files, and a read-only --all --dry-run against the real registry. No upload, no manifest write to the tree.
Strengths
scripts/host-registry-assets.ts:132-141— validation and hashing go through one descriptor (openSync→fstatSync(fd)→readFileSync(fd)),O_NONBLOCKkeeps a FIFO from hanging the run, and the fd is closed infinallyon both the throw and success paths. This is the structural fixjs/file-system-raceasks for, not a check-reorder.scripts/host-registry-assets.ts:177-183— the staged copy is re-hashed against the full planned digest (not the 16-hex key prefix), the mismatched object is unlinked, and the error text ("No upload was attempted") is accurate for every reachable path.scripts/host-registry-assets.ts:253-254— manifests are only rewritten afterstage()returns. Confirmed with a two-item probe where the second item's source is swapped mid-copy: bothregistry-item.jsonfiles stay byte-identical to their originals and both sources remain on disk, so a plain re-run is the retry.scripts/host-registry-assets.test.ts:53-70— the regression is real: with themainversion of the script swapped in, this test fails withMissing expected rejection; at head both tests pass. Isolating via a temp-root copy of the script (sorepoRoot/STAGING_DIRresolve undertmpdir) is the right shape for a script that otherwise writes intoregistry/.
Claims checked
- "Behavior unchanged" for the third commit:
main():246filters empty-plan items beforeflatMap, which is exactly the set the removed inline loop wrote; theplans.length === 0early-return is still reached from the same state. Equivalent. - Deduplication: two entries with identical bytes stage one object under one key and both manifest entries carry the same URL (probed).
- Dry-run: no longer write-then-restore; nothing on disk changes and no staging dir is created (probed on the real registry: 4 items / 70 files reported, tree clean).
- No-upload source retention: covered by the first test; also holds on the abort path.
- CodeQL: the PR merge ref has 0 open alerts across 103 rules; the
js/file-system-raceinstance onmain(hostFile, line 130) has no counterpart at head. Themainalert itself will only flip oncemainis re-scanned post-merge.
Nits
scripts/host-registry-assets.ts:132— a missing declared file now surfaces as the rawENOENT: no such file or directory, open '/abs/path'instead of the previous<item>: files[] declares "<path>", which is not on disk.(directories still get the friendly message viafstat). CatchingENOENTaroundopenSyncand rethrowing the item-scoped message would keep the operator context without reintroducing a path pre-check.scripts/host-registry-assets.ts:175—seendedups on the 16-hex key prefix. Now thatplan.digestis in hand, aMap<key, digest>could throw on a prefix collision with differing full digests instead of silently pointing the second manifest at the first file's bytes. Astronomically unlikely for honest content; cheap to make explicit.
Notes (no action)
- On abort, objects staged for earlier plans remain in
.registry-assets-staging(only the mismatched one is unlinked). Harmless:stage()starts withrmSync, and neither--no-uploadnorupload()is reached. - Trust model: the stated maintainer-owned checkout/staging assumption is appropriate for this path. The script runs on a maintainer workstation with that maintainer's AWS profile; a same-user adversary already holds the credentials, so the residual windows (
discardStagedSources:226check-then-unlink, source modified betweenstage()and post-upload discard) don't buy an attacker anything they lack. What this PR closes is the realistic failure — an editor save, asset regeneration, orgit checkoutmid-run — which now aborts loudly before any manifest or CDN write instead of publishing mismatched bytes under an immutable key. - Cost: each asset is now read twice (plan + staged verification). Inherent to verifying; fine for this tool.
Companion PR
#3827 touches packages/producer only; there is no code dependency between the two. Sequencing #3827's alert closure first is a process preference, not something this PR's correctness relies on.
Verdict: APPROVE
Reasoning: The race is closed structurally (single-descriptor hash, full-digest verification of the staged bytes, manifests written last), the regression test genuinely fails on the baseline, and every behavior-preservation claim in the description held up under local probing. Remaining items are nits.
— Miga (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Re-check of head 19a30a85 after the main-branch conflict refresh (merged=null; native CI green at this head: Test, CLI smoke, Fallow audit, CodeQL, Windows shards all passed; the two skipped jobs — one Preflight variant and regression-shards — were skipped identically at 61757bee).
Verified
- The three PR commits (
49cdfa58,8a147f0f,61757bee) are unchanged;19a30a85is a merge of currentmain(merge base is now8bf5b442=origin/main). Interdiff of the PR delta against the base before and after the refresh is exactly one line:package.jsontest:scriptsnow lists bothscripts/host-registry-assets.test.ts(yours) andpackages/core/scripts/writeGeneratedFile.test.ts(main's). Nothing else moved. scripts/host-registry-assets.tsand its test are byte-identical to the approved head, so the single-descriptor hash, full-digest verification of the staged copy, and manifests-written-last ordering all stand as reviewed.- Local at
19a30a85:host-registry-assets.test.ts2/2 (staging match + dry-run/no-upload retention; source replacement between plan and stage aborts before publication), and the fulltest:scriptschain (node test runner +vitest run scripts/catalog/) exits 0.
The two nits from the first review (raw ENOENT path in the missing-source error; key-prefix dedup could assert the full digest) are unchanged and remain non-blocking, as agreed.
Verdict: APPROVE
Reasoning: The refresh is a pure merge with a one-line, correct test-list resolution; the security change is unchanged and re-verified locally with CI green at this head.
— Miga (pr-review)
Registry asset hosting previously checked a path, reopened it to hash its contents, and later copied it again. A source replacement between those operations could publish different bytes under an immutable CDN key derived from the original file (CodeQL #902).
Read and validate the source through one descriptor, then verify the full SHA-256 of the staged copy against the planned digest. If the bytes changed, remove that staged object and stop before uploading or rewriting manifests. Manifest updates now happen after all staging succeeds, so a failed verification leaves the original manifests available for retry. Existing CDN keys, deduplication, upload arguments, dry-run output, and no-upload source retention are preserved.
Validation: the actual script runs against isolated fixture registries. A deterministic replacement between hashing and copying fails on the original code and is rejected by this change; normal staging, byte/key identity, unchanged dry-run manifests, and source retention pass. The regression is wired into
test:scripts: 190 Node tests and 86 catalog tests pass. Full workspace build, lint/format, Fallow, typecheck hooks and signed commits pass.This addresses the checked-source/staged-content race. The maintainer-owned checkout and staging directory retain their existing local trust assumption; this does not claim atomic protection from another process with the same user's filesystem permissions. No CDN upload was performed during validation.