fix: compute stream package integrity from stream content, not from the destination path#447
Open
mmaietta wants to merge 1 commit into
Open
fix: compute stream package integrity from stream content, not from the destination path#447mmaietta wants to merge 1 commit into
mmaietta wants to merge 1 commit into
Conversation
…he destination path
## Summary
- Fixes a bug introduced in `4.1.2` where `createPackageFromStreams` could embed the **wrong per-file integrity hash** in the archive header. The `insertFile` synchronous fast path computes integrity via `fs.readFileSync(p)`, but for the streams API `p` is the file's *destination path inside the archive* (relative), so it resolves against the process CWD. If the CWD happens to contain a file at the same relative path (e.g. `package.json` when building a project from its root), the header records the hash of that unrelated file while the archive stores the correct bytes from the stream.
- Apps packed this way and built with the `EnableEmbeddedAsarIntegrityValidation` fuse fail Electron's load-time integrity check and exit immediately at launch. The corruption is silent at pack time and environment-dependent: when no colliding file exists, the `readFileSync` throws `ENOENT` and the code silently falls back to (correct) stream hashing.
- Fix ([src/filesystem.ts](src/filesystem.ts), [src/asar.ts](src/asar.ts)):
- `insertFile` accepts a new internal option `fromStream`; when set, the `readFileSync` fast path is skipped entirely and integrity is always computed from `streamGenerator()` — the only authoritative source for stream content.
- `createPackageFromStreams` passes `{ fromStream: true }`.
- Safety guard in the remaining fast path (files API): the buffer returned by `fs.readFileSync(p)` is only trusted when `fileBuffer.length === size` (the size recorded in the header); otherwise it falls through to stream hashing. This protects against stat/read races and any future caller passing a `p` that isn't the real source file.
- No performance impact: the stream write path (`streamFilesystem`) never consumed `file.cachedBuffer`, so the fast path provided no write-batching benefit for streams — it only produced the (potentially wrong) hash. `createPackageFromFiles` behavior is unchanged.
## Regression test
[test/api-spec.ts](test/api-spec.ts) — `should compute stream package integrity from stream content, not from a same-named file in the CWD`:
1. Creates a source file with known content and a **decoy** file with different content located in the CWD at the same relative path as the archive destination (`index.js`).
2. Packs via `createPackageFromStreams` while `chdir`'d into the decoy directory (emulating building a project from its root).
3. Asserts the archived bytes are the stream content **and** the header integrity hash equals `sha256(stream content)`.
Verified the test fails on unpatched code (header hash matched the decoy's sha256) and passes with the fix. Full suite: `yarn lint` clean, `yarn vitest run` 188/188 passing.
Contributor
Author
|
Note: I didn't see any previous usage of logging, so I'm curious about the approach/policy there, as I think some of the swallowed errors and/or if-statements could use some ( |
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
4.1.2wherecreatePackageFromStreamscould embed the wrong per-file integrity hash in the archive header. TheinsertFilesynchronous fast path computes integrity viafs.readFileSync(p), but for the streams APIpis the file's destination path inside the archive (relative), so it resolves against the process CWD. If the CWD happens to contain a file at the same relative path (e.g.package.jsonwhen building a project from its root), the header records the hash of that unrelated file while the archive stores the correct bytes from the stream.EnableEmbeddedAsarIntegrityValidationfuse fail Electron's load-time integrity check and exit immediately at launch. The corruption is silent at pack time and environment-dependent: when no colliding file exists, thereadFileSyncthrowsENOENTand the code silently falls back to (correct) stream hashing.insertFileaccepts a new internal optionfromStream; when set, thereadFileSyncfast path is skipped entirely and integrity is always computed fromstreamGenerator()— the only authoritative source for stream content.createPackageFromStreamspasses{ fromStream: true }.fs.readFileSync(p)is only trusted whenfileBuffer.length === size(the size recorded in the header); otherwise it falls through to stream hashing. This protects against stat/read races and any future caller passing apthat isn't the real source file.streamFilesystem) never consumedfile.cachedBuffer, so the fast path provided no write-batching benefit for streams — it only produced the (potentially wrong) hash.createPackageFromFilesbehavior is unchanged.Regression test
test/api-spec.ts —
should compute stream package integrity from stream content, not from a same-named file in the CWD:index.js).createPackageFromStreamswhilechdir'd into the decoy directory (emulating building a project from its root).sha256(stream content).Verified the test fails on unpatched code (header hash matched the decoy's sha256) and passes with the fix. Full suite:
yarn lintclean,yarn vitest run188/188 passing.