fix: align posix/windows path handling and enable windows ci#14
Merged
Conversation
ae55e57 to
513b3ee
Compare
Run the check job on windows-latest in addition to ubuntu so the windows code paths (no O_NOFOLLOW, node fallbacks for fd-relative ops, ACL inspection) are exercised on every PR rather than only documented. Make the test suite pass on the new windows runner by addressing the platform-specific failures: - Long happy-path tests that mix supported (mkdir, write, read) and unsupported (stat, list, move, exists) operations are guarded with skipIf(process.platform === "win32") since the pinned filesystem helper throws "unsupported-platform" on win32 by design (src/pinned-python.ts). - Short focused tests where the unsupported operation is the whole point (pinned-python, pinned-write-fallback-coverage, write-boundary-bypass symlink-move) split into runIf(non-win32) and runIf(win32) tests, with the windows variant asserting unsupported-platform. - The expectFsSafeCode helper accepts unsupported-platform on windows; new expectedFsSafeCode helper substitutes for per-rejects.toMatchObject sites where the windows code differs from posix (e.g. path-alias / not-found returning unsupported-platform via the helper layer). - secure-file-reads test split into a posix happy-path runIf and a windows runIf that asserts permission-unverified, since ACL inspection has no portable equivalent on windows (src/secure-file.ts:177). - safeFileURLToPath test uses hardcoded platform-specific input/ output instead of building the URL via pathToFileURL+fileURLToPath so the assertion verifies the function directly. - Fix expandHomePrefix to normalize path separators by splitting via path.normalize + path.sep and rejoining via path.join. Apply the same segment-based check to resolveHomeRelativePath and resolveOsHomeRelativePath. Drop input.trim() — whitespace is a valid filename character on both platforms and env-var inputs are already trimmed upstream via normalizeOptionalString. - coverage-more's "normalizes empty temp names" decomposes the result with path.dirname/path.basename instead of regex-matching a path-separator literal. - extracted-helpers' path-helpers test builds its root with path.resolve so the drive letter is present on windows. - additional-boundary-bypass guards its "..\evil.txt" sanitizer assertion behind a non-win32 check (windows reserves "\" as a path separator and cannot have it in a filename). - coverage-more's sibling temp test guards just the posix file-mode assertion (stat.mode & 0o777 === 0o600), which has no analog on windows. The syncing behaviour the test actually targets still runs on both platforms. - Raise test/new-primitives.test.ts size budget to 1500 to accommodate the secure-file-reads test split. After: 253 passed, 1 failed, 66 skipped on windows-11-arm64. The single remaining failure is a separate library-side gap (a SAFE_REJECTED_SUSPICIOUS_WRITE_PAYLOADS payload resolves on windows instead of rejecting) and will be tracked in a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c102978 to
d73dc91
Compare
The posix write helper rejected any path whose computed
path.relative(root, resolved) string-began with "..", even when the
resolved path was fully inside the root. That matched literal
filenames whose first segment merely starts with the two characters
".." (e.g. "..%2fpwned.txt", "..%00/pwned.txt") and produced an
"outside-workspace" error for paths that do not actually escape
the root. The real boundary is already enforced upstream by
resolvePathInRoot's path.resolve + isPathInside check, so the
extra startsWith("..") guard added no security value while
introducing platform divergence (windows did not have it). Drop the
guard, keep the path.isAbsolute check.
Recategorize the three former SAFE_REJECTED_SUSPICIOUS_WRITE_PAYLOADS
test inputs so the test reflects what each platform actually does:
- "..%2fpwned.txt" and "..%00/pwned.txt" are literal names that
resolve fully inside root on both platforms; move them to
LITERAL_SUSPICIOUS_WRITE_PAYLOADS (accepted everywhere).
- "..\pwned.txt" is a real traversal on windows where "\\" is a
separator, but a literal filename on posix where "\\" is a regular
name character; move it to POSIX_LITERAL_SUSPICIOUS_WRITE_PAYLOADS
(accepted on posix, rejected on windows).
The literal-directory check in the same test uses fsp.stat on
windows since safeRoot.list goes through the pinned helper that is
unavailable on win32, matching the pattern already used a few lines
up. Bump the per-test timeout to 15s for slow windows fs under
parallel test load.
Drop a stale explanatory comment in expandHomePrefix.
After: 254 passed, 0 failed, 66 skipped on windows.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
On windows fs.realpathSync and fs.realpath (async) can disagree on 8.3 short-name canonicalization. The github actions windows runner exposes this: fs.realpathSync returns "C:\Users\RUNNER~1\..." while fs.realpath returns "C:\Users\runneradmin\...". Tests that compare a sync helper's output against await fs.realpath fail with the same path printed in two forms. Compare against fs.realpathSync (imported as realpathSync from node:fs) on both sides so the test exercises the same canonical form regardless of which short-name configuration the runner has. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
This PR makes four logically separate changes that travel together because the CI matrix change is what surfaced the rest:
..-prefix rejection in the POSIX write helper that produced false positives for legitimate filenames and diverged from the Windows write path.expandHomePrefixso it produces native paths instead of mixed-separator strings, and stops silently dropping leading/trailing whitespace from real filenames.unsupported-platformrejections.checkjob onwindows-latestin addition toubuntu-latest.1. Drop spurious
..-prefix rejection in the POSIX write helperresolvePinnedWriteTargetInRoot(the POSIX write path) rejected any input whosepath.relative(root, resolved)happened to start with the two characters.., even when the resolved absolute path was fully inside the root. That matched legitimate filenames such as..%2fpwned.txt(a single literal segment) and..%00/pwned.txt(two literal segments) — neither of which actually escapes the root — and produced a misleadingoutside-workspaceerror. The Windows write path never had this guard, so the same payloads were already accepted there; the result was a platform divergence around literal..-prefixed names.The real escape boundary is
path.resolve+isPathInside, applied upstream byresolvePathInRooton every code path. The extrastartsWith("..")check added no security value: any input that genuinely escapes the root is already rejected byisPathInside. The check is removed;path.isAbsolute(relativeResolved)is kept since absolute relative-paths still need to fail closed.2. Bug fixes to
expandHomePrefixTwo real defects in
src/home-dir.ts:Mixed separators on Windows. The previous implementation used a regex plus
input.replaceto swap the leading~with the home directory, leaving the rest of the input untouched:On Windows that produced outputs like
C:\Users\xxx/filefrom~/file— technically usable, but inconsistent with every other path the library returns. The function now splits the input into segments viapath.normalize+split(path.sep)and rejoins viapath.join, so the result always uses native separators. As a side effect, on POSIX the function correctly leaves~\fooalone —\is a literal name character on POSIX, not a separator, so it never indicated a home-prefixed path.Silent whitespace stripping.
resolveHomeRelativePathandresolveOsHomeRelativePathcalledinput.trim()before processing, which silently corrupted any path that legitimately contained leading or trailing whitespace. Whitespace is a valid filename character on both POSIX (NTFS, ext4, etc.) and Windows. The trim was unjustified: env-var inputs are already trimmed upstream vianormalizeOptionalStringbefore reaching the resolver, so the only callers actually affected by the trim were direct path callers — and for them the trim was wrong. Both functions now process the input verbatim. The corresponding test assertions that codified the trim have been removed.The same segment-based check (
segments[0] !== "~") is applied to bothresolveHomeRelativePathandresolveOsHomeRelativePathfor consistency withexpandHomePrefix.3. Windows-aware test assertions
Several tests embedded POSIX path assumptions in their assertions. The library code was already correct on Windows; only the tests needed updating to express the right expectation per platform.
The categories of fix:
path.join(path.sep, "tmp", "root"), which produces\tmp\rooton Windows. Functions under test (resolveSafeBaseDir,path.resolve, etc.) prepend the cwd's drive letter, yieldingC:\tmp\root. The fix is to construct the test root viapath.resolveso the drive letter is present from the start; comparisons then succeed on both platforms./or compared against strings built withpath.join("/...", ...). On Windows the actual separator is\. The fix is either to decompose results withpath.dirname/path.basename(avoiding the separator entirely), or to compare against expected values built with the same path utilities the implementation uses.path.basenametreats\as a separator on Windows and as a literal name character on POSIX. One sanitizer test asserted the POSIX outcome unconditionally; that assertion is now POSIX-only with a comment explaining that\cannot appear in a Windows filename anyway.(stat.mode & 0o777) === 0o600. POSIX modes do not fully apply on Windows, where Node returns0o666/0o444based on the read-only bit. The mode assertions are guarded behindprocess.platform !== "win32"so the surrounding behaviour the tests actually target still runs on both platforms.unsupported-platformrejections. The pinned filesystem helper throwscode: "unsupported-platform"on Windows by design (src/pinned-python.ts). Tests that exercise it through long happy-paths are nowskipIf(win32); tests where the unsupported operation is the entire point are split into matchedrunIf(!win32)/runIf(win32)pairs, with the Windows variant asserting the documented error code. The sharedexpectFsSafeCodehelper acceptsunsupported-platformas an additional valid code on Windows; a newexpectedFsSafeCode(code)helper substitutes for.rejects.toMatchObject({ code })sites.readSecureFilefails closed on Windows withpermission-unverifiedbecause there is no portable ACL equivalent (src/secure-file.ts:177). The "reads from a validated file handle" test is now split: a POSIX happy-path test and a Windows test asserting the documented error code.file://URLs. The local file URL test built its input by string-concatenating a POSIX-style path; on Windows that produced URLs without drive letters which Node rejects. The assertion now uses hardcoded platform-specific input/output pairs so it verifiessafeFileURLToPathdirectly without depending on Node URL helpers...%2fpwned.txtand..%00/pwned.txtmove fromSAFE_REJECTED_SUSPICIOUS_WRITE_PAYLOADStoLITERAL_SUSPICIOUS_WRITE_PAYLOADS(accepted on both platforms now that §1 lands)...\pwned.txtmoves toPOSIX_LITERAL_SUSPICIOUS_WRITE_PAYLOADS(literal on POSIX, real traversal on Windows). The emptySAFE_REJECTED_SUSPICIOUS_WRITE_PAYLOADSarray is removed.No POSIX behaviour is changed by these test edits.
4. CI: enable
windows-latest.github/workflows/ci.ymlchanges:lint-workflowsjob.checkjob becomes an OS matrix (ubuntu-latest,windows-latest) withfail-fast: false, so a Windows failure does not cancel the Linux run.checktimeout is raised to 20 minutes to accommodate the slower Windows runner (pnpm install+tsc+vitest).The repository's security model documents Windows-specific behaviour — no
O_NOFOLLOW, Node-only fallbacks for fd-relative POSIX hardening, Windows ACL inspection insecure-fileandpermissions— but until now CI only ran on Linux. With this change, the documented fallback paths are exercised on every PR.Result
Test plan
pnpm checkon Windows 11 (arm64), Node 24.15.0, Developer Mode enabled — fully green