fix(tar-xz): close Win32 symlink-swap TOCTOU with JS-pure 'wx'+retry fail-closed#114
Merged
Conversation
- docs/plans/TAR-XZ-STREAMING-2026-04-28.md: draft → canonical (PR #113 squash 06a9937 shipped 2026-04-29; all 5 blocks delivered, opus adversarial + LLM consensus + 4 Copilot rounds passed) - docs/plans/native-binding-migration.md: draft → canonical (research conclusion: no migration; document is the deliverable) - TODO.md: remove stale duplicate of #113 streaming entry from Pending-MEDIUM (was a copy of the now-shipped item), refresh Quick Reference table, move Win32 follow-up from Pending to In Progress (story WIN32-TOCTOU-2026-04-29 starts here)
…fail-closed
Replace the by-path Win32 fallback in extractFile with a handle-based
path using O_EXCL ('wx') atomic create plus an unlink-then-retry
pattern for legitimate overwrite. fd-based chmod/utimes follow the
file descriptor, not the path. If a symlink is injected during the
unlink+retry race, the second 'wx' open fails with EEXIST and a
security error is thrown citing the entry name — no bytes are written
through the symlink.
Recon invalidated the original "match node-tar with CreateFileW"
framing: node-tar is pure JS and explicitly does NOT protect Windows
either (their PR #456 is Unix-only, get-write-flag.ts gates O_NOFOLLOW
on !isWindows). Adopting JS-pure 'wx'+retry exceeds the ecosystem
norm without expanding the native addon surface.
Adversarial pass folded one M finding (reparse-point coverage gap)
into SECURITY.md with full IO_REPARSE_TAG_SYMLINK / MOUNT_POINT /
CLOUD_FILES coverage table. Vectors 2-5 (hardlinks, case-insensitive
NTFS, ADS, vi.stubGlobal reachability) confirmed handled or
non-issues; vector 5 simplified the implementation by cutting the
platform-detection seam.
Closes the TOCTOU window widened by PR #113 streaming refactor
(was ~ms post-Buffer.concat, became seconds-minutes per entry size
during streaming write — realistic exploit window for co-tenants).
- packages/tar-xz/src/node/file.ts: Win32 branch rewritten
('wx' + unlink+retry, fd-based chmod/utimes, threat-model JSDoc
citing W1-W4); unused chmod/utimes named imports trimmed
- packages/tar-xz/test/security-win32.spec.ts: 4 BDD scenarios
(atomic-create, legit-overwrite, race-detected fail-closed,
pre-existing-symlink-rejected-upstream regression lock);
vi.mock('node:fs/promises') + vi.spyOn for race injection
- packages/tar-xz/SECURITY.md: §"Windows symlink-swap TOCTOU"
with reparse-tag coverage table, residual race documentation,
user mitigations
- packages/tar-xz/README.md: link to SECURITY.md§Win32
- .changeset: patch-level entry
- docs/plans/WIN32-TOCTOU-2026-04-29.md: spec (12 sections,
threat model, BDD, adversarial ledger; will be promoted to
canonical at /finalize)
All tar-xz tests: 155 pass / 0 fail / 3 pre-existing platform skips.
Lint: 0 errors. Type-check: 0 errors.
There was a problem hiding this comment.
Pull request overview
Hardens tar-xz’s Windows (win32) extractFile() path against symlink-swap TOCTOU by switching from by-path writes to an atomic exclusive-create ('wx') + retry flow and moving post-open operations to fd-based APIs, with accompanying regression tests and documentation.
Changes:
- Replace Win32 by-path extraction with
open(target, 'wx')+EEXIST → unlink → retryand fd-basedwrite/chmod/utimes. - Add a dedicated Win32 TOCTOU regression test suite plus new security documentation and README updates.
- Add a Changesets entry for a patch release reflecting the Win32 hardening.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tar-xz/src/node/file.ts | Implements Win32 'wx' + retry fail-closed extraction with fd-based writes/metadata ops. |
| packages/tar-xz/test/security-win32.spec.ts | Adds BDD-style regression tests for Win32 symlink-swap race and overwrite behavior. |
| packages/tar-xz/SECURITY.md | Documents Windows symlink-swap TOCTOU threat model, residual risks, and mitigations. |
| packages/tar-xz/README.md | Updates security model section and links to SECURITY.md for Win32 details. |
| .changeset/win32-toctou-hardening.md | Adds patch changeset entry describing the Win32 hardening. |
| docs/plans/WIN32-TOCTOU-2026-04-29.md | Adds an internal hardening spec/plan document for the Win32 TOCTOU work. |
| docs/plans/native-binding-migration.md | Updates doc metadata to “canonical/finalized” state. |
| docs/plans/TAR-XZ-STREAMING-2026-04-28.md | Updates doc metadata to “canonical/finalized” state. |
| TODO.md | Updates project tracking entries related to the Win32 TOCTOU work and prior streaming story. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+17
to
+22
| ### What changed in v6.1.1 | ||
|
|
||
| The Windows extraction path now uses `open(target, 'wx', mode)` — the `'wx'` | ||
| flag maps to `O_CREAT | O_EXCL` in libuv, an atomic "create or fail" syscall. | ||
| Writes, `chmod`, and `utimes` are all performed through the resulting | ||
| `FileHandle`, immune to any by-path swap after the open. |
There was a problem hiding this comment.
This doc section says the change landed in v6.1.1, but the release version is determined by the set of pending Changesets when publishing. To avoid shipping incorrect version claims, consider removing the specific patch number or aligning it with the actual release version that will include this hardening.
Comment on lines
+150
to
+169
| vi.spyOn(fsp, 'unlink').mockImplementationOnce(async (p) => { | ||
| await origUnlink(p as string); | ||
| // Attacker injects a symlink pointing to /dev/null (safe for CI). | ||
| // On a real attack this would be /etc/shadow or a co-tenant file. | ||
| await fsp.symlink('/dev/null', target); | ||
| }); | ||
|
|
||
| await expect(extractFile(archivePath, { cwd: dest })).rejects.toThrow( | ||
| /symlink-swap race detected/ | ||
| ); | ||
|
|
||
| // Critical: target must still be the attacker's symlink (we did NOT | ||
| // overwrite it and did NOT unlink it a second time). | ||
| const stat = await fsp.lstat(target); | ||
| expect(stat.isSymbolicLink()).toBe(true); | ||
|
|
||
| // Critical: the symlink still points to /dev/null — we did not follow it. | ||
| const linkTarget = await fsp.readlink(target); | ||
| expect(linkTarget).toBe('/dev/null'); | ||
| }); |
There was a problem hiding this comment.
The test creates symlinks to '/dev/null' and asserts readlink() returns '/dev/null'. This is POSIX-specific and may behave differently (or fail) on Windows runners. Use a temp-path target under the test directory (or platform-appropriate null target) and compare using path normalization to keep this scenario cross-platform.
Comment on lines
+365
to
+366
| `Security error: symlink-swap race detected for '${entry.name}' — ` + | ||
| `a symlink was injected at the target path between unlink and open` |
There was a problem hiding this comment.
The thrown message on second EEXIST states "a symlink was injected", but the code comment above says this can also be a junction/other reparse point (or even a benign concurrent creator). Consider wording the error message more precisely (e.g., "target still exists on retry; possible symlink/junction injection") so it doesn't assert a specific object type that wasn't verified.
Suggested change
| `Security error: symlink-swap race detected for '${entry.name}' — ` + | |
| `a symlink was injected at the target path between unlink and open` | |
| `Security error: target still exists on retry for '${entry.name}' — ` + | |
| `possible symlink/junction injection or concurrent creation at the target path between unlink and open` |
Comment on lines
+5
to
+12
| Win32 extractFile fail-closed under symlink-swap race (JS-pure). | ||
|
|
||
| Replaces the by-path `createWriteStream` Windows fallback with an fd-based | ||
| `open('wx')` + unlink-then-retry pattern. If a symlink is injected between | ||
| the unlink and the retry-open (symlink-swap race), extraction rejects with a | ||
| security error instead of writing through the symlink. | ||
|
|
||
| ```ts |
There was a problem hiding this comment.
PR metadata/description discusses WASM decoder memlimit wiring, but this changeset (and the code changes in this PR) are about tar-xz Win32 TOCTOU hardening. Please update the PR description to match the actual change set (or split the unrelated memlimit work into a separate PR) so reviewers and release notes are aligned.
Comment on lines
357
to
358
| await unlink(target); | ||
| try { |
There was a problem hiding this comment.
In the Win32 EEXIST path, unlink(target) is awaited without handling ENOENT. Since the filesystem can change between the first open() failure and unlink(), ENOENT here will currently abort extraction even though a retry open('wx') would likely succeed. Consider ignoring ENOENT (and still retrying open) to make the overwrite path robust to concurrent deletions while preserving the fail-closed behavior for reparse-point injection.
Suggested change
| await unlink(target); | |
| try { | |
| // If the target disappears between the failed open() and unlink(), ignore | |
| // ENOENT and still retry the atomic exclusive create. | |
| try { | |
| await unlink(target); | |
| } catch (unlinkErr) { | |
| if ((unlinkErr as NodeJS.ErrnoException).code !== 'ENOENT') throw unlinkErr; | |
| } | |
| try { |
Comment on lines
+204
to
+208
| As of v6.1.1 this gap is closed: the Windows path uses `open(target, 'wx')` (atomic | ||
| exclusive create) with an unlink+retry pattern for legitimate overwrites. If a symlink | ||
| is injected between the unlink and the retry-open, extraction fails with a security error. | ||
| All writes and metadata operations are fd-based. See [SECURITY.md](./SECURITY.md#windows-symlink-swap-toctou) | ||
| for the full reparse-tag coverage table and user mitigations. |
There was a problem hiding this comment.
This section hard-codes the fix landing "As of v6.1.1". With Changesets, the actual version that first contains this change depends on what other unreleased changesets ship together (e.g., there is also a pending minor bump for tar-xz). Consider phrasing this without a specific patch version (or ensure the stated version matches the release plan) to avoid documentation drift.
Three M-class findings + two L-class fixes from Copilot review;
one finding (C-4) dismissed as a hallucination after content
verification (changeset is on-topic, not WASM-related).
- file.ts:358 — wrap unlink() in try/catch ENOENT (M-3): when
target disappears between failed first open() and our unlink,
ignore ENOENT so the retry open('wx') can succeed. Preserves
fail-closed for reparse-point injection (retry still rejects
any non-empty path).
- file.ts:364 — reword security error (M-2): replace
"symlink-swap race detected … a symlink was injected" with
"target still exists on retry … possible symlink/junction
injection or concurrent creation at the target path between
unlink and open". The new wording does not overclaim a
specific reparse-point type and accepts benign concurrent
creators as a possibility, while still failing closed.
- security-win32.spec.ts — cross-platform symlink target (M-1):
pre-create attacker-target.txt under tmp; symlink points there
instead of /dev/null. Add `it.skipIf(!canCreateSymlinks)` gate
with a beforeAll probe so Win32 runners without Developer Mode
skip symlink-creation paths instead of failing with EPERM.
Also fixes Scenario 4's /dev/null target (same portability
issue, same fix).
- SECURITY.md / README.md — version-agnostic phrasing (L-1, L-2):
replace three "v6.1.1" mentions with "this hardening" / "now
closed". Removes documentation drift risk if the release ends
up as 6.2.0 instead of 6.1.1.
- WIN32-TOCTOU-2026-04-29.md — spec drift fix: §7 Observable
Success block now cites the new error wording and the
cross-platform attacker target.
C-4 dismissed: Copilot claimed the changeset references "WASM
decoder memlimit"; verified false — `.changeset/win32-toctou-
hardening.md` only discusses Win32 TOCTOU.
Tests: 155 pass / 0 fail / 3 pre-existing skips.
Lint: 0 errors. Type-check: 0 errors.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+90
to
+91
| // on all CI platforms (Linux, macOS, Windows). | ||
| vi.stubGlobal('process', { ...process, platform: 'win32' }); |
There was a problem hiding this comment.
vi.stubGlobal('process', { ...process, platform: 'win32' }) replaces the global process object with a plain object that only includes enumerable properties. This can drop non-enumerable methods (e.g. EventEmitter APIs) and can make tests flaky or break the runner. Prefer stubbing via a Proxy or Object.create(process) and shadowing only platform so the rest of the process API stays intact.
Suggested change
| // on all CI platforms (Linux, macOS, Windows). | |
| vi.stubGlobal('process', { ...process, platform: 'win32' }); | |
| // on all CI platforms (Linux, macOS, Windows), while preserving the | |
| // original process object's non-enumerable properties and methods. | |
| const processStub = Object.create(process) as NodeJS.Process; | |
| Object.defineProperty(processStub, 'platform', { | |
| value: 'win32', | |
| configurable: true, | |
| }); | |
| vi.stubGlobal('process', processStub); |
Comment on lines
+13
to
+15
| // Race-injected symlink now throws instead of writing through it: | ||
| // Error: Security error: symlink-swap race detected for 'victim.txt' — | ||
| // a symlink was injected at the target path between unlink and open |
There was a problem hiding this comment.
The changeset’s example error text ("symlink-swap race detected" ... "a symlink was injected") doesn’t match the actual error message thrown in extractFile (which includes "target still exists on retry"). Please update the snippet to match the real thrown message (or avoid hardcoding exact wording in the example) so release notes don’t drift from behavior.
Suggested change
| // Race-injected symlink now throws instead of writing through it: | |
| // Error: Security error: symlink-swap race detected for 'victim.txt' — | |
| // a symlink was injected at the target path between unlink and open | |
| // Race-injected symlink now throws a security error instead of writing | |
| // through the target path. |
Comment on lines
+162
to
+170
| * **Windows:** `O_NOFOLLOW` is not available. The Windows path uses | ||
| * `open(target, 'wx', mode)` (atomic exclusive create — `O_CREAT | O_EXCL`). | ||
| * If the target exists (`EEXIST`), it is unlinked and the open is retried. | ||
| * If the retry also fails with `EEXIST`, a symlink was injected between the | ||
| * unlink and the retry-open (symlink-swap race) and extraction fails closed | ||
| * with a security error. All write/chmod/utimes ops are fd-based (via | ||
| * `FileHandle`) so no by-path symlink follow can occur after the open. | ||
| * The residual race is limited to the `open()` syscall itself (sub-microsecond). | ||
| * See `SECURITY.md§"Windows symlink-swap TOCTOU"` for the full reparse-tag |
There was a problem hiding this comment.
The PR description discusses wiring memlimit into the WASM decoder, but the actual diff is Win32 extractFile TOCTOU hardening (open('wx') + unlink+retry, new SECURITY.md, new Win32 regression tests). Please update the PR description to reflect the Win32 security change set so reviewers and release tooling aren’t misled.
One M finding + one L finding from Copilot round 2; the third (PR description WASM mismatch) was an orchestrator slip that has already been corrected via gh pr edit (stale /tmp/pr-body.md from PR #111 was used at PR creation time — apologies to Copilot, which had correctly flagged it both rounds). - security-win32.spec.ts (C2-1, M): vi.stubGlobal('process', spread) drops non-enumerable Node.js APIs (EventEmitter, etc), risking test runner instability. Switch to Object.create(process) + Object.defineProperty('platform', 'win32') so only platform is shadowed and the rest of the process API stays intact. Helper stubWin32Platform() extracted with JSDoc rationale. - .changeset/win32-toctou-hardening.md (C2-2, L): example error text drifted from the new wording (round 2 changed the source but the changeset still cited the old "symlink-swap race detected" message). Replaced with a generic 2-line snippet decoupled from exact error wording. Tests: 155 pass / 0 fail / 3 pre-existing skips (identical to pre-fix). Lint: 0 errors. Type-check: 0 errors. Trend: round 1 = 6 findings (3 M + 2 L + 1 misclassified), round 2 = 3 findings (1 M + 2 L), round 3 expected = 0 (healthy down-trend).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+167
to
+173
| // On Windows without Developer Mode, symlink creation requires admin rights. | ||
| // The throw still comes from our mock (injected EEXIST path) even when | ||
| // symlink creation would fail; only the post-throw verification assertions | ||
| // (isSymbolicLink / readlink) require the symlink to exist. | ||
| it.skipIf(process.platform === 'win32' && !canCreateSymlinks)( | ||
| 'throws security error when symlink-swap race is detected (Win32)', | ||
| async () => { |
There was a problem hiding this comment.
it.skipIf(process.platform === 'win32' && !canCreateSymlinks) is evaluated when the test is defined, but canCreateSymlinks is only updated later in beforeAll. On Windows without Developer Mode/admin rights this will not skip and the test can fail with EPERM when creating the injected symlink. Consider removing skipIf and instead conditionally skip just the post-throw symlink assertions at runtime (or compute canCreateSymlinks before registering tests via top-level await).
One M finding from Copilot round 3 — broken skipIf gate. The `it.skipIf(process.platform === 'win32' && !canCreateSymlinks)` gate added in round 2 was subtly broken: Vitest evaluates `skipIf` conditions at test-registration time, but `canCreateSymlinks` is only updated later inside `beforeAll`. Result: the gate always sees `true` on Win32, the test runs even without Developer Mode, and `fsp.symlink` throws EPERM during the race-injection mock, making the test fail with the wrong error. Replaced with a static `it.skipIf(process.platform === 'win32')` on the 2 tests that need symlink creation (race-detected and pre-existing-symlink-rejected scenarios). Removed the orphaned `canCreateSymlinks` variable and `beforeAll` probe block. Coverage tradeoff: - Linux/macOS (~90% CI): all 4 tests run via process.platform stub. Security contract (extract → 'wx' → unlink+retry → security error) fully validated. - Windows: skips the 2 symlink-injection tests cleanly. The other 2 tests (atomic-create-success, legit-overwrite-via-unlink-retry) exercise the real-NTFS Win32 path without needing symlink privilege. Tests: 155 pass / 0 fail / 3 pre-existing skips. Lint: 0 errors. Type-check: 0 errors. Trend: round 1 = 6 findings, round 2 = 3, round 3 = 1, round 4 expected = 0. Convergence.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+384
to
+387
| await handle.chmod(fileMode); | ||
| if (entry.mtime > 0) { | ||
| const mt = new Date(entry.mtime * 1000); | ||
| await handle.utimes(mt, mt); |
There was a problem hiding this comment.
Windows branch now treats handle.chmod()/handle.utimes() failures as fatal. Previously the Windows path explicitly treated chmod/utimes as best-effort (swallowing errors), so this is a behavior change that can cause extractFile() to fail on Windows for archives that otherwise extract fine (e.g., EPERM from chmod/utimes on some filesystems). Consider restoring best-effort semantics here (wrap chmod/utimes in narrow try/catch similar to the previous code), or document the intentional behavior change in the changeset/README.
Suggested change
| await handle.chmod(fileMode); | |
| if (entry.mtime > 0) { | |
| const mt = new Date(entry.mtime * 1000); | |
| await handle.utimes(mt, mt); | |
| // On Windows these metadata updates are best-effort: some filesystems can | |
| // reject them (for example with EPERM) even when the file contents were | |
| // written successfully. | |
| try { | |
| await handle.chmod(fileMode); | |
| } catch { | |
| // Best-effort on Windows. | |
| } | |
| if (entry.mtime > 0) { | |
| const mt = new Date(entry.mtime * 1000); | |
| try { | |
| await handle.utimes(mt, mt); | |
| } catch { | |
| // Best-effort on Windows. | |
| } |
| // Critical: the symlink still points to the attacker's file — we did | ||
| // not follow it. Use path.normalize for cross-platform path comparison. | ||
| const linkTarget = await fsp.readlink(target); | ||
| expect(path.normalize(linkTarget)).toBe(path.normalize(attackerTarget)); |
There was a problem hiding this comment.
The “symlink-swap race detected” scenario verifies the target path is a symlink pointing at attackerTarget, but it never asserts attackerTarget’s contents remain unchanged. If the extractor accidentally follows the symlink and writes through it, the symlink would still point to attackerTarget and this test could still pass. Add an assertion that attackerTarget was not modified (e.g., still contains the original content) to make the test an observable-proof regression gate.
Suggested change
| expect(path.normalize(linkTarget)).toBe(path.normalize(attackerTarget)); | |
| expect(path.normalize(linkTarget)).toBe(path.normalize(attackerTarget)); | |
| // Critical: the attacker-controlled target file must remain unchanged. | |
| // If extraction ever follows the swapped symlink and writes through it, | |
| // this assertion will fail and make the regression observable. | |
| const attackerContent = await fsp.readFile(attackerTarget, 'utf8'); | |
| expect(attackerContent).toBe('attacker content'); |
Comment on lines
+13
to
+14
| // Race-injected symlink now throws a security error instead of writing | ||
| // through the target path. |
There was a problem hiding this comment.
The changeset includes an empty TypeScript code fence (no snippet inside). This renders as an empty block in release notes. Consider either adding a minimal illustrative snippet (or removing the code fence entirely) to keep the changeset output clean.
Suggested change
| // Race-injected symlink now throws a security error instead of writing | |
| // through the target path. | |
| await expect(extractFile(archive, target)).rejects.toThrow( | |
| /security error/i | |
| ) |
Three findings: 1 regression we introduced + 1 test gap + 1 doc. - file.ts (C4-1, M, regression): wrap handle.chmod/utimes on Win32 branch in narrow try/catch with /* best-effort */ comment. The master pre-PR Win32 path treated chmod/utimes as best-effort (some Win32 filesystems — FAT32, cloud-mounted shares — reject these with EPERM even when the write succeeded). Our PR inadvertently made them fatal, breaking those workflows. Now matches master semantics. handle.write errors still propagate (data integrity). - security-win32.spec.ts (C4-2, M, observable proof): the race- detected test verified the symlink remained the attacker's, but did NOT assert attackerTarget content was unchanged. If the extractor accidentally followed the symlink and wrote through it we wouldn't have caught it. Added the byte-equality assert on attackerTarget after the throw — true observable-proof regression gate. attackerTarget was already pre-populated with 'attacker content' from round 2. - .changeset/win32-toctou-hardening.md (C4-3, L): empty typescript code fence (round 3 emptied the block but left the fence) replaced with a 3-line minimal example using extractFile + rejects.toThrow. Tests: 155 pass / 0 fail / 3 pre-existing skips. Lint: 0 errors. Type-check: 0 errors. Trend: round 1=6, round 2=3, round 3=1, round 4=3 (2 real Ms in our own prior code), round 5=3 fixed → expected 0 remaining.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scope: native-bindings | ||
| type: research | ||
| created: 2026-03-01 | ||
| updated: 2026-03-01 |
There was a problem hiding this comment.
doc-meta.updated still reads 2026-03-01, but this PR changes the document’s status/finalization metadata. If updated is meant to reflect the last modification date (as it typically does in frontmatter), it should be bumped to match this edit (e.g. 2026-04-29) for consistency.
Suggested change
| updated: 2026-03-01 | |
| updated: 2026-04-29 |
|
|
||
| # WIN32-TOCTOU — tar-xz Win32 extractFile symlink-swap hardening | ||
|
|
||
| > **Project:** /mnt/wsl/shared/dev/node-liblzma |
There was a problem hiding this comment.
This spec includes a machine-specific local path (/mnt/wsl/...) in the “Project” line. Since this lives in-repo, it’s likely to confuse readers and becomes stale quickly; consider replacing it with a repo-relative reference (or removing the line) so the doc is portable.
Suggested change
| > **Project:** /mnt/wsl/shared/dev/node-liblzma | |
| > **Project:** `.` |
Comment on lines
+42
to
+43
| /** Build a minimal TAR buffer with a single regular-file entry. */ | ||
|
|
There was a problem hiding this comment.
There’s a stray JSDoc block here that isn’t attached to any declaration (it precedes another JSDoc and the next function starts at line 46). This reads like it was meant to document buildSingleFileTar; consider moving it immediately above that function or removing it to avoid dead/comment-only blocks.
Comment on lines
+30
to
+36
| // Replace the module with a proxy so vi.spyOn intercepts named imports in file.ts. | ||
| // Without this, ESM named bindings in file.ts are separate live references that | ||
| // vi.spyOn on the namespace object cannot intercept. | ||
| vi.mock('node:fs/promises', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('node:fs/promises')>(); | ||
| return { ...actual }; | ||
| }); |
There was a problem hiding this comment.
This comment says the module is replaced with a “proxy”, but the mock factory returns a shallow spread ({ ...actual }), not a Proxy. Consider rewording to describe the actual mechanism (returning a mutable copy of the exports) so future readers don’t assume Proxy semantics.
| // retry-'wx' open by injecting a symlink in the mock. | ||
| // | ||
| // BEFORE fix: extract() resolves and writes through the symlink. | ||
| // AFTER fix: extract() rejects with /symlink-swap race detected/ |
There was a problem hiding this comment.
The scenario description says the post-fix behavior is to reject with /symlink-swap race detected/, but the test actually asserts /target still exists on retry/. Please align the comment with the real expected error text to avoid documentation drift in the regression gate.
Suggested change
| // AFTER fix: extract() rejects with /symlink-swap race detected/ | |
| // AFTER fix: extract() rejects with /target still exists on retry/ |
Five cosmetic findings — no behavior change. - docs/plans/native-binding-migration.md: bump doc-meta updated date 2026-03-01 → 2026-04-29 to match this PR's housekeeping commit (status was promoted to canonical but updated stayed stale). - docs/plans/WIN32-TOCTOU-2026-04-29.md: remove machine-specific '/mnt/wsl/...' Project line — was non-portable. - packages/tar-xz/test/security-win32.spec.ts: remove orphan JSDoc block detached from any declaration; correct "proxy" comment near vi.mock (the factory does shallow spread, not Proxy); align scenario comment regex with the actual assertion text (/symlink-swap race detected/ → /target still exists on retry/, drift from round-2 wording change). Tests: 155 pass / 0 fail / 3 pre-existing skips. Lint: 0 errors. Type-check: 0 errors. Pushed with NO-COPILOT-REVIEW marker — premium quota saved per workflow §2.10 termination rule (a) for L-only round on a ready-to-merge PR. Trend: round 1=6, round 2=3, round 3=1, round 4=3 (2 real Ms in our prior code), round 5=3 fixed, round 6=5 L addressed. Total findings folded across 6 rounds: 21. PR ready for merge.
oorabona
added a commit
that referenced
this pull request
Apr 29, 2026
Post-merge finalize (PR #114 squash `b24040d` landed on master): - docs/plans/WIN32-TOCTOU-2026-04-29.md: doc-meta promoted draft → canonical with merged_pr=114, merged_sha=b24040d, finalized=2026-04-29, copilot_review_rounds=6, copilot_findings_folded=21 - TODO.md: Win32 hardening entry moved from In Progress to Completed with squash SHA + 1-line story summary; Quick Reference table refreshed (MEDIUM cleared); Last merge / Last story stats updated to PR #114
oorabona
added a commit
that referenced
this pull request
Apr 29, 2026
Pre-publish verification of `tar-xz@6.1.0` revealed: - README.md links to SECURITY.md§"Windows symlink-swap TOCTOU" (added in PR #114) but SECURITY.md was excluded from the npm tarball — broken link for all npm consumers. - CHANGELOG.md was also excluded — users couldn't read release notes via the npm package. Root cause: `files: ['lib/']` whitelist only included compiled output. README.md was auto-included by npm; CHANGELOG.md and SECURITY.md were not. Fix: add both to the `files` array. npm pack --dry-run now shows 84 files (was 82) including CHANGELOG.md (11.7 kB) and SECURITY.md (5.7 kB). Echoes the pre-existing gotcha about prebuilds/ being missing from files since v3.0.0 — same class of mistake.
oorabona
added a commit
that referenced
this pull request
Apr 29, 2026
The Changesets - Version PR workflow has been failing on `node-liblzma` because the root package isn't included in pnpm-workspace.yaml's `packages: ['packages/*']` glob. The `native-memlimit.md` changeset referenced `node-liblzma` and triggered "Found changeset for package not in the workspace" errors on every master push. Architectural reality: this monorepo uses release.yml (release-it + conventional commits) as the canonical release path. Changesets artifacts are decorative — never consumed by the actual release flow. Their content is already captured in CHANGELOG.md via release-it's conventional-commit scan. Cleanup: - .changeset/native-memlimit.md (PR #112, node-liblzma not in workspace, blocked the workflow) - .changeset/wasm-memlimit-option.md (PR #111, same reason) - .changeset/validate-changesets-flow.md (test file from changesets adoption — no longer needed) - .changeset/tar-xz-streaming.md (PR #113, shipped via tar-xz@6.1.0 release-it 2026-04-29) - .changeset/win32-toctou-hardening.md (PR #114, shipped via same tar-xz@6.1.0 release) Keep .changeset/config.json + README.md for the directory's identity. Future PRs can still add changesets if the changesets workflow is later re-enabled as a primary path. Unblocks: changesets.yml workflow on master pushes.
6 tasks
oorabona
added a commit
that referenced
this pull request
Apr 29, 2026
…(63→1) (#115) * chore: gitignore Claude Code transient state files Adds `.claude-recovery.md` and `.workflow-state.json` to .gitignore. Both are per-session orchestration artifacts — should never be committed. The `/finalize` step deletes `.workflow-state.json` on successful workflow completion; without gitignore, an accidental `git add -A` (anti-pattern per CLAUDE.md) leaks them. * refactor: biome auto-fix sweep — useTemplate, optional chains, FIXABLE Phase 2 of REFACTOR-BIOME-2026-04-29: applied `biome check --write` across the workspace. Strictly behavior-preserving auto-fixes only. Categories addressed: - useTemplate: 2 string-concat → template literal sites - noNonNullAssertion (FIXABLE): 5 `gc!()` calls → optional chain `gc?.()` - noUnusedImports: removed orphan import in security.spec.ts - noUnusedVariables: removed orphan local in xz-helpers.spec.ts Files: 7 changed, +24/-29. Verification (post-fix): - pnpm test: 671 pass / 0 fail / 3 skip (identical to baseline) - pnpm type-check: 0 errors x3 packages (root + tar-xz + nxz-cli) - rtk proxy biome lint: 63 → 40 warnings (-23, -36.5%) Remaining 40 warnings (Phases 3-5): - 23 noNonNullAssertion (manual replacements via type narrowing) - 12 noExcessiveCognitiveComplexity (extract-method refactors) - 2 noImportCycles (intentional lzma↔pool, will be biome-ignore'd) - 1 useForOf (manual: index-aware loop) * refactor: kill noNonNullAssertion + noImportCycles + useForOf manually Phase 3+4 of REFACTOR-BIOME-2026-04-29: manual replacements for warnings biome --write cannot fix automatically. Categories addressed: - 23 noNonNullAssertion sites — manual narrowing per site: - errors.ts:176 — undefined-check + early fallback (no `messages[errno]!`) - pool.ts:166 — undefined-check on shift() with invariant comment - tar/checksum.ts:32,92 — `?? 0` for fixed-size header reads + early-break narrows byte type - tar/format.ts:100 — undefined-break in parseOctal loop - tar/format.ts:168 — `?? 0` for header[OFFSETS.typeflag] - nxz.ts:114 — destructure presetMatch[1] with explicit fallback - nxz.ts:710,713,717,755,756,759 — explicit undefined guards in findCommonParent / resolveArchiveCwd; preserves existing process.cwd() fallback semantics - nxz.ts:736,995 — `?? ''` for files[0] indexing - test/coverage.spec.ts:917,965,1045 — `?? 0` in checksum loops - test/security.spec.ts:456,492,541 — `?? 0` in checksum loops - test/wasm/decompress-memlimit.test.ts:141 — explicit `if (result === undefined) throw` (Vitest toBeDefined doesn't narrow TS types per js-typescript GOTCHAS) - examples/browser/main.ts:24 — getElementById guarded - 2 noImportCycles (lzma↔pool re-export): biome-ignore comments citing intentional ESM-resolved-at-runtime pattern from MEMORY.md. - 1 useForOf (format.ts:125 isEmptyBlock): for-of over Uint8Array yields number directly, simultaneously kills the warning AND the block[i]! bypass it covered up. Files: 10 changed, +53/-28. Verification: - pnpm test: 671 pass / 0 fail / 3 skip (identical to baseline) - pnpm type-check: 0 errors x3 packages - rtk proxy biome lint: 40 → 14 warnings (-26) Remaining 14 (Phase 5 + out of scope): - 12 noExcessiveCognitiveComplexity (extract-method refactors next) - 2 suppressions/unused (pre-existing, out of refactor scope) * refactor: extract-method on test cognitive-complexity sites Phase 5a of REFACTOR-BIOME-2026-04-29: kill 7 noExcessiveCognitiveComplexity warnings in test files. Pure helper extraction; test names + ordering unchanged. Helpers extracted (kept inside the spec file, not promoted to a shared util): - `buildEntryBlocks(entry, blocks)` — per-entry tar block construction loop body. Used by `buildTar` in all 3 spec files. - `buildGlobalPaxBlock(attrs, blocks)` — global PAX construction branch (tar-parser-stream.spec.ts only). - `recalculateChecksum(header)` — inline checksum loop pattern shared across multiple `V6b`/`V12` it blocks. After extraction, top-level `buildTar` reduces to a 3-line dispatch (global PAX vs regular entries). Cognitive complexity scores drop below biome threshold. The 7 it() bodies that previously inlined the checksum recalculation now call `recalculateChecksum(header)` — identical math, less noise. Bonus cleanup: tar-parser-stream.spec.ts had a `// biome-ignore lint/complexity/noExcessiveCognitiveComplexity` suppression on the old `buildTar`; refactor makes it unnecessary, suppression removed. Files: 3 changed, +192/-175. Verification: - pnpm test: 671 pass / 0 fail / 3 skip (identical to baseline) - pnpm --filter tar-xz type-check: 0 errors - biome warnings (3 test files): 7 → 0 - biome warnings (full project): 14 → 6 Remaining 6 warnings (Phase 5b + out of scope): - 5 noExcessiveCognitiveComplexity in src files: nxz.ts:777, browser/extract.ts:28, node/extract.ts:198, node/file.ts:180, tar/format.ts:227 (Phase 5b — high-risk territory: just-hardened Win32 TOCTOU code + fresh streaming refactor + core tar format). - 1 pre-existing noEmptyBlockStatements biome-ignore in a test file unrelated to this sweep (out of refactor scope). * refactor: extract-method on lower-risk src complexity sites Phase 5b-1 of REFACTOR-BIOME-2026-04-29: drops 3 noExcessiveCognitiveComplexity warnings via pure extract-method on the lower-risk source files. Helpers extracted (private, same-file scope): - `packages/tar-xz/src/browser/extract.ts` parseTar: - `parsePaxHeaderBlock(data, offset)` → `{offset, paxAttrs}` for the PAX_HEADER branch (parse + advance offset) - `extractEntryContent(data, entry, offset)` → `{offset, contentData}` for the content-slice + 512-byte padding advance Pure helpers, no captured mutable state. Loop semantics preserved. - `packages/tar-xz/src/tar/format.ts` createHeader: - `splitLongName(fullName)` → `{prefix, name}` for the legacy 100-byte field validation + 155/100 prefix/name split. Error message "File name too long for TAR format: ${fullName}" preserved byte-for-byte (consumers may pattern-match). - `packages/nxz/src/nxz.ts` collectArchiveFiles: - `buildEntryPath(entry, dirAbsPath, dirRelative)` → relative path construction for archive entries. Lazy `import('node:fs')` retained inline. Files: 3 changed, +74/-25. Verification: - pnpm test: 671 pass / 0 fail / 3 skip (identical to baseline) - pnpm type-check: 0 errors x3 packages - rtk proxy biome lint: 6 → 3 warnings (-3) Remaining 3 warnings: - 2 noExcessiveCognitiveComplexity in node/file.ts (extractFile, PR #114 hardened) and node/extract.ts (streaming, PR #113) → Phase 5b-2 with pre-push opus senior-review - 1 noNonNullAssertion suppression in test/node-api.spec.ts:249 (pre-existing, unrelated to this sweep) * refactor: extract-method on security-sensitive src complexity sites Phase 5b-2 of REFACTOR-BIOME-2026-04-29: dropped the last 2 noExcessiveCognitiveComplexity warnings via pure extract-method on the highest-risk src files (PR #114 Win32 TOCTOU + PR #113 streaming). ZERO behavior change; all security/streaming contracts preserved verbatim. `packages/tar-xz/src/node/file.ts` extractFile (HIGH RISK — recently hardened by PR #114): 7 private helpers extracted, original extractFile becomes a thin dispatcher: - `ensureSafeLinkname(linkname, opts, name)` — leaf+ancestor symlink validation for hardlink/symlink targets - `extractSymlinkEntry(target, entry, opts)` — SYMLINK branch - `extractHardlinkEntry(target, entry, opts)` — HARDLINK branch - `openFileExclusive(target, fileMode, entryName)` — Win32 'wx' + EEXIST→unlink+retry→security-throw fail-closed gate. Security error message preserved BYTE-IDENTICAL: "Security error: target still exists on retry for '${entryName}' — possible symlink/junction injection or concurrent creation at the target path between unlink and open". The unlink ENOENT pass-through and second-EEXIST throw remain in the same try/catch nesting. - `writeFileEntryPosix(target, entry, fileMode, mtime)` — POSIX path with O_NOFOLLOW + strict chmod/utimes propagation - `writeFileEntryWin32(handle, entry, fileMode, mtime)` — Win32 fd ops with best-effort chmod/utimes wrap (FAT32/cloud share semantics) - `writeFileEntry(target, entry, fileMode, mtime)` — platform dispatcher (POSIX vs Win32) `packages/tar-xz/src/node/extract.ts` extract async generator: 4 top-level helpers extracted; lookaheadRef ref-object pattern replaces inline closure mutation: - `nextParseEvent(parser, lookaheadRef)` — pulls next event with lookahead consumption - `drainEntryChunks(parser, lookaheadRef)` — drains chunk events until next entry boundary; preserves D-2 swallow policy - `drainSkippedEntry(parser, lookaheadRef)` — drains chunks for a filtered-out entry without yielding - `createEntryDataPull(parser, lookaheadRef)` — replaces makeDataGen + dataGenInFlight closure; preserves D-5 TAR_PARSER_INVARIANT detection on concurrent iteration Removed dead biome-ignore comment from extract.ts (no longer needed post-extraction). Replaced `filter && !filter(strippedEntry)` with `filter?.(strippedEntry) === false` — equivalent semantics, lower nesting cost. Files: 2 changed, +417/-250 net (mostly helper extraction). Verification (BLOCKING — all green): - pnpm test: 671 pass / 0 fail / 3 skip (identical to baseline) - pnpm type-check: 0 errors x3 packages - packages/tar-xz/test/security-win32.spec.ts: 4/4 ✅ - packages/tar-xz/test/security.spec.ts: 22/22 ✅ - rtk proxy biome lint: 3 → 1 warnings (-2 cognitive-complexity) Final state across the full REFACTOR-BIOME-2026-04-29 (Phases 1-5b-2): - 63 → 1 biome warnings (-62 = -98.4%) - 1 remaining = pre-existing `suppressions/unused` in test/node-api.spec.ts:249 (unrelated to this sweep, not addressed) - 671 tests pass throughout, 0 regression - 0 TS errors x3 packages throughout - 8 commits on branch fix/biome-warnings-sweep * chore(todo): update biome warnings entry — 63→1 after refactor * docs: refresh WIN32-TOCTOU spec post-refactor + log F-002 follow-up Pre-push opus senior-review on REFACTOR-BIOME-2026-04-29 surfaced two LOW-severity informational findings: - F-001: spec § 1 cited file.ts:328-346 (line refs valid at PR #114 merge); post-refactor the fail-closed path lives in private helpers openFileExclusive + writeFileEntryWin32 + writeFileEntry. Spec amended to keep both the historical reference (audit trail) AND the current helper names. Contract semantics unchanged. - F-002: pre-existing latent HARDLINK validation gap. resolve(cwd, strippedLinkname) throws TypeError on undefined linkname when strip=0. Filed in TODO as defensive-hardening item; not introduced by this refactor. Both findings non-blocking for the refactor PR. * refactor: fix filter check semantics — preserve all-falsy short-circuit Round-1 Copilot review on PR #115 surfaced one M-class finding: Phase 5b-2 replaced the original `filter && !filter(strippedEntry)` with `filter?.(strippedEntry) === false`. The strict `=== false` check skipped entries ONLY when the predicate returned literal `false`; the original `&&` short-circuit skipped on ANY falsy return (false / 0 / '' / null / undefined). Real behavior drift, not equivalent semantics — and divergent from the browser-side `extract()` which kept `filter && !filter(...)`. Fix: revert to `filter && !filter(...)` semantics, but extract the conditional into a private helper `isExcludedByFilter(entry, filter)` so the inline `&&` short-circuit doesn't push extract()'s cognitive complexity score back over biome's 15 threshold. The helper preserves the exact branching of the original idiom: - filter undefined → returns false (don't skip) - filter returns false → returns true (skip) - filter returns 0 / '' / null / undefined → returns true (skip) - filter returns truthy → returns false (don't skip) Verification: - pnpm --filter tar-xz test: 155 pass / 0 fail / 3 skip - pnpm --filter tar-xz type-check: 0 errors - rtk proxy biome lint extract.ts: 0 warnings (cognitive complexity back under threshold thanks to the helper) * refactor: address Copilot round-2 findings on PR #115 Three findings folded: - **C2-1 (M)**: `createEntryDataPull` threw the D-5 invariant error ("concurrent entry.data iteration is not supported") as a plain Error without `code = 'TAR_PARSER_INVARIANT'`. The stray-chunk invariant elsewhere in the module DOES set `code`, and `drainSkippedEntry` filters errors by that exact code to decide swallow vs re-throw. Without the code attribute, a concurrent- iteration error inside a skipped entry would be silently swallowed, breaking the D-5 contract. Pre-existing latent bug surfaced by the Phase 5b-2 extraction. Fixed by setting `err.code = 'TAR_PARSER_INVARIANT'` on the dataGenInFlight error. - **C2-2 (L)**: `ensureSafeLinkname` JSDoc said "empty or undefined linkname fields" but `TarEntry.linkname` is typed as required `string` (parser returns `''` for empty fields). Removed the dead `entry.linkname !== undefined` check (always true given the type) and clarified the docstring. Behavior preserved: empty linkname on SYMLINK/HARDLINK still rejected by `ensureSafeName`'s empty-string guard. - **C2-3 (L)**: F-002 TODO entry I added in the previous round claimed `extractHardlinkEntry` could pass `undefined` to `resolve`. Mischaracterized — `linkname` is required string, parser sets `''` for empty fields, and `ensureSafeLinkname` rejects '' upstream. Replaced the entry with an HTML comment explaining why it was dropped (preserves audit trail). Verification: - pnpm --filter tar-xz test: 155 pass / 0 fail / 3 skip - pnpm --filter tar-xz type-check: 0 errors - rtk proxy biome lint: 1 warning (pre-existing unrelated suppressions/unused — not introduced by this PR) * refactor: fail-fast on empty files in resolveTarOutput Round-3 Copilot finding (M): Phase 3+4's `files[0] ?? ''` replacement in `resolveTarOutput()` swallowed the original `basename(undefined)` TypeError that signaled an invariant violation (empty input files array when no output path provided). With `?? ''`, an empty array silently produces an output named `.tar.xz` (just the suffix) — worse than a clear error. Fix: explicit `firstFile === undefined` check + descriptive throw, preserving the invariant-violation fail-fast semantics. The narrowing also avoids re-introducing the `noNonNullAssertion` warning that Phase 3+4 fought to eliminate. The other `files[0] ?? ''` site at line 1020 (in the dispatcher) is gated by an upstream `if (options.files.length === 0) process.exit` guard at line 1013 — the `??` there is unreachable defensive code, documented in the comment. Left unchanged. Verification: - pnpm --filter nxz-cli test: 27/27 pass - pnpm --filter nxz-cli type-check: 0 errors - rtk proxy biome lint nxz.ts: 0 warnings * refactor: null-safe filter check in isExcludedByFilter Round-4 Copilot finding (M): the helper used `filter !== undefined` to gate the predicate call. The original `filter && !filter(entry)` short-circuited on ANY falsy filter (false / 0 / '' / null / undefined); `!== undefined` lets `null` through, which would crash at `filter(null)` runtime. Browser-side `extract()` correctly uses `filter && ...`. Fix: `typeof filter === 'function' && !filter(entry)` — null-safe, matches browser-side semantics, preserves the documented contract. Note on macOS CI failure on the previous push: the smoke test crashed with `Error: async hook stack has become corrupted (actual: 6939, expected: 6939)` — a known Node.js v22 internal AsyncHooks race during pnpm install postinstall, NOT a refactor regression. Stack trace is 100% inside Node native code (uv__work_done → AfterStat → InternalCallbackScope::Close → AsyncHooks::pop). This push will re-trigger CI; if macOS passes on retry, confirmed flaky. Verification: - pnpm --filter tar-xz test: 155/0/3 - pnpm --filter tar-xz type-check: 0 errors - rtk proxy biome lint extract.ts: 0 warnings * refactor: fail-fast on invariant breach in pool + nxz dispatcher Round-5 Copilot findings (2 M, same fail-fast vs fail-silent class as round-3 C3-1): - **C5-1 (M)** `src/pool.ts:168` — `processQueue()` guards `this.queue.length === 0` immediately above the `shift()` call, so `shift()` returning undefined indicates an INVARIANT VIOLATION (re-entrancy, future refactor, etc.). The previous Phase 3+4 replacement of `!` with `if (item === undefined) return` silenced this breach, leading to queued tasks stalling silently. Fixed: throw `Invariant violation: queue was non-empty but shift() returned undefined`. - **C5-2 (M)** `packages/nxz/src/nxz.ts:1029` — same pattern. After the stdin guard at line 1013 exits on `files.length === 0`, the next reference `options.files[0] ?? ''` (added in Phase 3+4) silenced an invariant breach. Fixed: explicit `if (firstFile === undefined) throw new Error('Internal error: expected at least one input file after stdin handling')`. Mirrors the C3-1 fix in resolveTarOutput from round-3. Both findings predate this refactor in spirit (master had `!` non-null assertions that hid the same invariant assumption); Phase 3+4 replaced the `!` with silent fallbacks. Round-3+5 walked the project back to fail-fast for those specific sites. Verification: - pnpm test: full suite green (155 tar-xz / 27 nxz / 489 root = 671/0/3) - pnpm type-check: 0 errors x3 packages - rtk proxy biome lint: 1 warning (pre-existing unrelated suppressions/unused) * refactor: round-6 comment-precision cleanups (L-only) Three L-class comment-accuracy findings from Copilot round-6: - pool.ts:169 — round-5 commit cited `processQueue.length > 0` but that's the function arity (=0), not the queue. Reworded to reference the actual `this.queue.length === 0` early-return guard above. - file.ts:158 — `extractSymlinkEntry` comment said "We do NOT validate entry.linkname here" but `ensureSafeLinkname()` runs upstream now (since Phase 5b-2). Reworded to clarify what's validated upstream (syntactic: non-empty + no NUL + not dot placeholder) vs what's intentionally NOT enforced here (cwd- containment of the symlink target). - extract.ts:193 — round-2 commit's rationale for setting `err.code = 'TAR_PARSER_INVARIANT'` cited `drainSkippedEntry` filtering, but the makeDataGen() branch is effectively unreachable in production (`makeTarEntryWithData` calls dataPull() exactly once and never exposes makeDataGen). Reworded to describe what the guard actually catches (double makeDataGen() invocation) and why the `code` attribute is set (consistency with other invariant errors in the module). Pure comment-precision; zero behavior change. Push uses `# NO-COPILOT-REVIEW` per workflow §2.10 termination rule (a) for L-only on a refactor PR — saves Copilot premium quota; opus SAFE-TO-PUSH already cleared the actual code. Verification: - pnpm test: 671/0/3 (full suite) - pnpm type-check: 0 errors x3 packages - rtk proxy biome lint: 1 warning (pre-existing unrelated)
2 tasks
oorabona
added a commit
that referenced
this pull request
Apr 30, 2026
…t/stop (#123) Six call-sites are guarded by checks that TypeScript's strict mode (noUncheckedIndexedAccess + typed errors) requires but cannot fire at runtime under current invariants. Suppress them from coverage so the metrics reflect only paths that real tests can exercise. - file.ts: ELOOP on POSIX writeFile (O_NOFOLLOW vs ensureSafeTarget), ENOENT on the unlink-after-failed-open race, EEXIST on the wx-retry TOCTOU re-open, ENOENT on the unlink-before-link race for hardlinks, and the Win32 best-effort chmod/utimes catch arms. All five mirror the PR #114 Win32 TOCTOU hardening contract — the contract is locked by the adversarial review and SECURITY.md scenarios; race windows are too flaky for unit tests. - tar-parser.ts: parseHeader === null after isEmptyBlock — the prior empty-block check guarantees the header parses successfully. - xz-helpers.ts: '!unxzStream.destroyed' early-return guard inside the AsyncIterable's finally block — the fully-iterated path always destroys before finally runs, so the falsy arm is only hit on early generator.return() (not exercised by current tests; stream is destroyed idempotently anyway). - checksum.ts: compound parseOctal condition split so only the noUncheckedIndexedAccess-required 'byte === undefined' arm is suppressed. The reachable 'byte === 0 || byte === 0x20' continues to count toward coverage. Coverage delta on tar-xz: statements 82.05 -> 86.96; xz-helpers.ts reaches 100. Real test cases for the still-uncovered EACCES, filter, and malformed-archive paths will land in a follow-up PR. 707 tests pass byte-identical.
3 tasks
oorabona
added a commit
that referenced
this pull request
Apr 30, 2026
Pair real test cases with v8 ignore wraps for the genuinely defensive branches that PR-γ missed and one Win32 TOCTOU rethrow that landed after PR-γ scope was closed. Tests added (13 cases in test/coverage-final.spec.ts): - create.ts AsyncIterable source — async generator and single-chunk variants ; ArrayBuffer source — locks the resolveSource contract. - extract.ts zero-byte entry — bytes() returns the cached empty Uint8Array ; second call hits the cache. - file.ts hardlink edges — forward-reference (lstat ENOENT swallowed, link() ENOENT propagates), symlink-as-source rejection, symlink-ancestor rejection, EACCES propagation on POSIX. - tar-parser.ts truncated archive — three phases covered (mid-header 256-byte XZ buffer, mid-body during list() SKIP, mid-padding with size=300 entry). All assert 'Unexpected end of archive'. v8 ignore wraps in extract.ts (4 sites): - drainSkippedEntry TAR_PARSER_INVARIANT rethrow — corrupt parser state unreachable via public API. - makeDataGen concurrent-iteration guard — makeTarEntryWithData calls dataPull() exactly once per entry. - extract() stray-chunk invariant — parseTar never emits chunk before entry. - bytes() overflow guard — parseTar clips chunks at bytesRemaining, so over-delivery cannot happen via extract(). v8 ignore wrap in file.ts (1 site): - openFileExclusive Win32 TOCTOU retry rethrow (line 343) — same race-window pattern as the other PR #114 hardening guards. Coverage: - tar-xz lines: 95.33% → 100%. - tar-xz branches: 89.45% → 92.53% (v8-ignored bodies leave the condition counter active; only /* v8 ignore next */ would suppress it, which the project convention forbids). - create.ts, extract.ts, tar-parser.ts, file.ts: all at 100% lines. tar-xz tests: 190 → 203 passing (+13), 3 skipped unchanged. Full workspace 707 → 720 passing, 0 failing.
2 tasks
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
tar-xzextractFilewith a JS-pure
'wx'+retry fail-closed pattern. No native addon expansion.refactor (entry-size-scaled exposure during write).
node-tar is pure JS and does NOT protect Windows either (their PR #456 is
Unix-only). This PR exceeds the ecosystem norm without addon expansion.
Implementation
packages/tar-xz/src/node/file.ts— Win32 branch rewritten:open(target, 'wx', mode)for atomic create; onEEXIST,unlink+retry
'wx'; if retry alsoEEXIST→ security error citing"target still exists on retry — possible symlink/junction injection
or concurrent creation at the target path between unlink and open".
fd-based
handle.chmod/handle.utimesfollow the descriptor.ENOENT on unlink is ignored (handles benign concurrent deletion).
packages/tar-xz/test/security-win32.spec.ts— 4 BDD scenarios fromthe spec (atomic-create, legit-overwrite, race-detected fail-closed,
upstream-symlink-rejected regression lock). Race injection via
vi.mock('node:fs/promises')+vi.spyOn(fsp, 'unlink').mockImplementationOnce.Cross-platform attacker target under
tmp(not/dev/null); Win32Developer-Mode-aware skip gate via
beforeAllsymlink probe.process.platformstub usesObject.create(process)to preservenon-enumerable methods.
packages/tar-xz/SECURITY.md— §"Windows symlink-swap TOCTOU" withfull reparse-tag coverage table (
IO_REPARSE_TAG_SYMLINK/MOUNT_POINT/CLOUD_FILES), residual race documentation, usermitigations.
.changeset/win32-toctou-hardening.md— patch-level entry.docs/plans/WIN32-TOCTOU-2026-04-29.md— full spec (threat model,BDD, adversarial ledger).
Threat model & adversarial findings
Adversarial pass on 5 Windows-specific vectors:
lstat().isSymbolicLink()detects
IO_REPARSE_TAG_SYMLINKonly. Junctions and cloud placeholdersare not detected. Leaf is protected by
'wx'EEXIST fail-closed; ancestorsdocumented as residual risk in SECURITY.md.
ADS: confirmed handled or non-issue by the
'wx'+retry contract.process.platformcheck is inlinein async fn body at
file.ts:292; stub reaches it directly.Review rounds
(3 M + 2 L + 1 misclassification originally dismissed in error)
error wording precision, ENOENT handling on unlink); 2 L
(version-pinning) replaced with version-agnostic phrasing
vi.stubGlobalnon-enumerable preservation viaObject.create) + 1 L (changeset example error text drift) foldedTest plan
tar-xztests pass: 155 / 0 (3 pre-existing platform skips)rtk proxy biome lint packages/tar-xz)pnpm type-check)extractFileto throw/target still exists on retry/; targetremains the attacker's symlink; no bytes written through it
Housekeeping
The first commit (
chore: lifecycle hygiene…) promotes two shipped plansto canonical and removes a stale duplicate from TODO.md. Standalone of the
fix; can be cherry-picked if needed.
Story
WIN32-TOCTOU-2026-04-29 — closed via this PR (will be promoted to canonical
at
/finalize).