fix: compute stream package integrity from stream content, not from the destination path by mmaietta · Pull Request #447 · electron/asar

mmaietta · 2026-06-12T18:33:46Z

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/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 — should compute stream package integrity from stream content, not from a same-named file in the CWD:

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).
Packs via createPackageFromStreams while chdir'd into the decoy directory (emulating building a project from its root).
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.

…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.

mmaietta · 2026-06-12T18:38:48Z

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 (console? debug?) logging if desired.

claude

Working with @mmaietta

Change looks right: p is the archive destination path in the streams API, so the sync fast path could hash an unrelated CWD file. Skipping the fast path via fromStream is the minimal fix, and after main's content dedupe (#465) a wrong hash would also have aliased contentOffsets, so this matters more now.

Merged main into this branch locally: lint clean, 204/204 vitest passing. The head lives on your fork, which I can't push to, so the conflict resolution is below.

A few non-blocking notes:

test/api-spec.ts:229 — fs.mkdtemp(os.tmpdir(), ...) is never removed, so each run leaks a directory. The other tests write under TEST_APPS_DIR, which beforeEach wipes; building work there (or removing it in the finally) keeps that consistent.
test/api-spec.ts:241 — process.chdir only works because vitest runs with pool: 'forks'; under threads it throws. A one-line comment would save someone a confusing failure later.
src/filesystem.ts:160 — the fileBuffer.length === size guard is fine as belt-and-braces, but for the files API a mismatch means the file changed during packing, and we still record the stale stat size and stream whatever is on disk, so the archive is corrupt either way. Either throw a clear error on mismatch or narrow the comment to "don't trust a buffer that doesn't match the header size".
Nothing exercises the length !== size fall-through; a small filesystem-spec case calling insertFile with a mismatched stat.size would cover it.

Conflict resolution against main (two hunks)

src/asar.ts, createPackageFromStreams case 'file' — keep main's ordering (insertFile before files.push, capture duplicate) and add the PR's fromStream option:

      case 'file': {
        const duplicate = await filesystem.insertFile(
          filename,
          stream.streamGenerator,
          stream.unpacked,
          { type: 'file', stat: stream.stat },
          // `filename` is the destination path inside the archive, not a path
          // on disk, so integrity must be computed from the stream.
          { fromStream: true },
        );
        files.push({
          filename,
          streamGenerator: stream.streamGenerator,
          link: undefined,
          mode: stream.stat.mode,
          unpack: stream.unpacked,
          duplicate,
        });
        break;
      }

src/filesystem.ts, insertFile fast path — keep the PR's size guard around main's storeFileEntry body:

    if (!options.fromStream && size <= BUFFER_HASH_THRESHOLD) {
      // Fully synchronous fast path — no Promise, no stream, no microtask yield
      try {
        const fileBuffer = fs.readFileSync(p);
        // Only trust the buffer if it matches the size recorded in the header;
        // otherwise the file at `p` is not the content being archived.
        if (fileBuffer.length === size) {
          const integrity = getFileIntegrityFromBuffer(fileBuffer);
          const duplicate = this.storeFileEntry(node, size, executable, integrity);
          if (!duplicate) {
            file.cachedBuffer = fileBuffer;
          }
          return Promise.resolve(duplicate);
        }
      } catch {
        // Fall through to stream path
      }
    }

    return getFileIntegrity(streamGenerator()).then((integrity) =>
      this.storeFileEntry(node, size, executable, integrity),
    );

Everything else (the fromStream?: boolean option, yarn.lock) merges cleanly.

Generated by Claude Code

…am-integrity-fast-path # Conflicts: # src/asar.ts # src/filesystem.ts

mmaietta · 2026-09-12T19:01:35Z

Thanks for the review. Merged main in f24d0ea (same conflict resolution as suggested), and addressed the notes in 07eb0c6:

Temp dir leak: the regression test now builds its working dir under TEST_APPS_DIR, which beforeEach wipes, instead of os.tmpdir().
process.chdir: added a comment noting it throws in worker threads and relies on vitest's pool: 'forks'.
Size guard: narrowed the comment to "don't trust a buffer that doesn't match the size recorded in the header; hash the stream instead" rather than throwing. A throw in the fast path would only detect mid-pack modification for files at or below BUFFER_HASH_THRESHOLD (2MB), since larger files never read the buffer, so detection would be inconsistent across sizes. Catching files modified during packing for all sizes seems better as a separate change.
Fall-through coverage: added filesystem-spec > insertFile > should hash the stream when the file at the path does not match the header size. It calls insertFile with a stat.size that differs from the file on disk and asserts the integrity hash comes from the stream and no cachedBuffer is set. Confirmed it fails with the fileBuffer.length === size guard removed.

yarn lint clean, test typecheck clean, yarn vitest run 205/205 passing.

MarshallOfSound · 2026-09-12T23:32:14Z

@claude review

claude

Code review found no issues

No high-confidence issues detected in this change.

mmaietta marked this pull request as ready for review June 12, 2026 19:07

mmaietta requested a review from a team as a code owner June 12, 2026 19:07

mmaietta linked an issue Jun 12, 2026 that may be closed by this pull request

@electron/asar ≥ 4.1.2: createPackageFromStreams computes per-file integrity from the **destination path / CWD** instead of the stream content #446

Open

MarshallOfSound approved these changes Jun 28, 2026

View reviewed changes

claude Bot reviewed Sep 12, 2026

View reviewed changes

mmaietta added 2 commits September 12, 2026 11:47

Merge commit '00dd71672aeac88ba228bca17b0992f561b4465d' into fix/stre…

f24d0ea

…am-integrity-fast-path # Conflicts: # src/asar.ts # src/filesystem.ts

Address PR comments

07eb0c6

claude Bot reviewed Sep 12, 2026

View reviewed changes

MarshallOfSound approved these changes Sep 12, 2026

View reviewed changes

MarshallOfSound enabled auto-merge (squash) September 12, 2026 23:44

Navigation Menu

fix: compute stream package integrity from stream content, not from the destination path - #447

fix: compute stream package integrity from stream content, not from the destination path#447
mmaietta wants to merge 3 commits into
electron:mainfrom
mmaietta:fix/stream-integrity-fast-path

mmaietta commented Jun 12, 2026

Uh oh!

mmaietta commented Jun 12, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

mmaietta commented Sep 12, 2026

Uh oh!

MarshallOfSound commented Sep 12, 2026

Uh oh!

claude Bot left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mmaietta commented Jun 12, 2026

Summary

Regression test

Uh oh!

mmaietta commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mmaietta commented Sep 12, 2026

Uh oh!

MarshallOfSound commented Sep 12, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mmaietta commented Jun 12, 2026 •

edited

Loading