fix(sea-builder): verify downloaded Node.js archives before caching - #72
Merged
kurone-kito merged 3 commits intoAug 1, 2026
Merged
Conversation
downloadArchive.mts performed no validation at all: a non-2xx response still has a body, so an error page could be streamed into the cache under the archive's name; nothing checked the archive against nodejs.org's published checksum; and the stream wrote directly into the permanent cache path, so any interrupted or failed download left a file that createTaskFactory's existsSync check would treat as a valid cache hit forever. - reject non-2xx responses (both the archive and the SHASUMS256.txt fetch) before touching the filesystem. - fetch https://nodejs.org/dist/<version>/SHASUMS256.txt (derived from the archive url, not the caller-supplied dest path) and verify the downloaded bytes' sha-256 against it while streaming, failing closed on a missing or unparseable checksum line. - download to a temporary path and rename onto the final archivePath only after the checksum passes; remove the temp file on any failure so a failed attempt leaves no cache entry behind. replaced the bare, undescribed `// @ts-expect-error` (working around fetch's response.body -- typed against the dom lib's ReadableStream -- not being assignable to node:stream/promises pipeline's expected type) with a documented toNodeReadable() cast through node:stream/web's ReadableStream type instead. deleted toPipelineSource.mts and its spec rather than reusing it as the issue's proposed fix suggested as an alternative: it guards on `.readable`, a property a whatwg ReadableStream does not have, so passing response.body through it would throw TypeError on exactly the input it existed for. after converting through Readable.fromWeb() the guard is redundant, and nothing else in the package imported it. added downloadArchive.spec.mts (0 tests existed before): covers a non-2xx archive response, a non-2xx shasums response, a missing checksum entry, a checksum mismatch, a mid-stream error, and an empty body, each asserting no file is left at the destination path, plus the success path. statement coverage for downloadArchive.mts is 100% in both the package-local vitest 4 run and the root aggregate vitest 3 run (root's own coverage report showed 33% before this change). closes #57
📝 WalkthroughWalkthrough
ChangesArchive integrity flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant downloadArchive
participant NodeFetch
participant TempFile
participant ArchivePath
Caller->>downloadArchive: Request archive download
downloadArchive->>NodeFetch: Fetch archive
NodeFetch-->>downloadArchive: Return validated response
downloadArchive->>NodeFetch: Fetch SHASUMS256.txt
NodeFetch-->>downloadArchive: Return checksum metadata
downloadArchive->>TempFile: Stream archive and calculate SHA-256
TempFile-->>downloadArchive: Provide verified temporary file
downloadArchive->>ArchivePath: Atomically rename temporary file
ArchivePath-->>Caller: Complete download
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens @kurone-kito/sea-builder’s Node.js archive download path by adding HTTP-status validation, SHA-256 verification against Node’s published SHASUMS256.txt, and atomic caching semantics to prevent permanent cache poisoning.
Changes:
- Add streaming SHA-256 verification and temp-file + atomic rename download flow in
downloadArchive.mts. - Add a new
downloadArchivetest suite covering success and multiple failure modes (ensuring no destination file is left behind). - Remove the unused/broken
toPipelineSourcehelper and its spec; update cspell word list.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sea-builder/src/utils/toPipelineSource.spec.mts | Removed tests for the deleted toPipelineSource helper. |
| packages/sea-builder/src/utils/toPipelineSource.mts | Deleted unused/broken stream helper per issue acceptance criteria. |
| packages/sea-builder/src/utils/downloadArchive.spec.mts | Added comprehensive tests for new download validation/integrity behavior. |
| packages/sea-builder/src/utils/downloadArchive.mts | Implemented HTTP status checks, checksum verification, and atomic cache writes. |
| cspell.config.yml | Added “unstub” to dictionary for new test usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sea-builder/src/utils/downloadArchive.mts`:
- Around line 46-51: Update the atomic move logic around rename(tempPath, dest)
to handle Windows EPERM/EBUSY failures with a bounded retry before propagating
the error, while preserving cleanup of the temporary file on failure. Adjust the
downloadArchive documentation to remove or platform-scope the unconditional
“atomically move” guarantee.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d623deb7-bf13-42c9-aca2-292319a208aa
📒 Files selected for processing (5)
cspell.config.ymlpackages/sea-builder/src/utils/downloadArchive.mtspackages/sea-builder/src/utils/downloadArchive.spec.mtspackages/sea-builder/src/utils/toPipelineSource.mtspackages/sea-builder/src/utils/toPipelineSource.spec.mts
💤 Files with no reviewable changes (2)
- packages/sea-builder/src/utils/toPipelineSource.mts
- packages/sea-builder/src/utils/toPipelineSource.spec.mts
Adds a case where dest's basename deliberately differs from the archive URL's basename, so the existing tests no longer pass incidentally just because both happened to match.
address review feedback on #72: - validate the SHASUMS256.txt token is a 64-char hex digest and normalize case before comparing, instead of trusting it verbatim - give each download a unique temp path instead of one keyed only by pid, since createListrCacheTasks runs downloads concurrently and two tasks can target the same dest - treat a rename refused by an already-placed destination as a benign race loss instead of an error, since windows can refuse to replace a file another concurrent download already placed there
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
downloadArchive.mts— the supply-chain download path for every SEAbinary this tool produces — performed no validation at all:
page could be streamed into the cache under the archive's name.
nodejs.org'spublished
SHASUMS256.txt.interrupted or failed download left a file that
createTaskFactory'sexistsSynccheck would treat as a validcache hit forever.
Changes
SHASUMS256.txtfetch) before touching the filesystem.https://nodejs.org/dist/<version>/SHASUMS256.txt— derivedfrom the archive URL's own path, not the caller-supplied
destpath, so the lookup key is correct by construction rather than by
the two happening to match today — and verify the downloaded bytes'
SHA-256 against it while streaming. A missing, malformed (not a
64-character hex digest), or mismatched checksum throws; nothing is
silently skipped, and the comparison is case-insensitive.
pid+ a random UUID, so twoconcurrent downloads of the same target never share one — cache
downloads run with
{ concurrent: true }increateListrCacheTasks) andrenameonto the finalarchivePathonly after the checksum passes.
moveIntoPlace()treats arenamerefused by an already-placeddestination as a benign race loss rather than an error: if two
concurrent calls target the same
dest, the loser's rename can fail(notably on Windows, which can refuse to replace a file another
process holds open) — but since both already checksum-verified the
same bytes, the loser just discards its own temp file instead of
surfacing the error. A rename failure with no existing destination
still propagates.
// @ts-expect-errorwith adocumented
toNodeReadable()cast:fetch'sresponse.bodyistyped against the DOM lib's
ReadableStream, which isn't assignableto
node:stream/promises'pipelinewithout going throughReadable.fromWeb()and a cast tonode:stream/web'sReadableStreamtype.toPipelineSource.mtsand its spec rather than reusingit, per the issue's proposed alternative: it guards on
.readable,a property a WHATWG
ReadableStreamdoes not have, so passingresponse.bodythrough it would throwTypeErroron exactly theinput it existed to handle. After converting through
Readable.fromWeb(), the guard is redundant, and nothing else inthe package imported it.
Verification
downloadArchive.spec.mts(no test file existed before):covers a non-2xx archive response, a non-2xx
SHASUMS256.txtresponse, a missing checksum entry, a malformed (non-hex) checksum
entry, an uppercase-hex checksum entry (accepted), a checksum
mismatch, a mid-stream error, and an empty body — each asserting no
file is left at the destination path — plus the success path, a
case where
dest's filename deliberately differs from the archiveURL's filename (confirming the checksum lookup key is derived from
the URL, not the caller-supplied destination), and a mocked-
renamecase covering both the benign-race-loss path (destination already
placed) and the propagate-on-real-failure path.
downloadArchive.mts: 100%, both in thepackage-local
vitest run --coverage(vitest 4) and in the rootaggregate
pnpm run testreport (vitest 3; was 33% before thischange, lines 11-17 uncovered — the entire download body).
pnpm run lint && pnpm run build && pnpm run testall pass (68tests).
Notes for the reviewer (not defects)
SHASUMS256.txtfetches — correct, mildly redundant; caching thatresponse was out of scope for this issue.
SHASUMS256.txtfetch,so a
SHASUMS256.txtfailure leaves that first response's bodyunconsumed. Harmless (no file is written until after both fetches
succeed), flagging so it doesn't read as an oversight.
Closes #57
Summary by CodeRabbit
Bug Fixes
Quality Improvements