Fix isPathWithinAllowedDirectories for UNC paths on Windows

[Christian-Sidak]

Summary

Fixes #3756

On Windows, isPathWithinAllowedDirectories fails for UNC paths (e.g. \\192.168.1.1\share\) because path.normalize strips a leading backslash from UNC paths, and then path.resolve interprets the result as a drive-relative path (e.g. C:\server\share). This causes the allowed-directory check to always fail for network shares.

This PR introduces a normalizePossiblyUNCPath helper that detects UNC paths and preserves the \\ prefix through normalization, rather than passing them through path.resolve which corrupts them. Non-UNC paths continue to use the existing path.resolve(path.normalize(...)) logic.

Changes

• Added isUNCPath() helper to detect UNC paths by their \\ prefix
• Added normalizePossiblyUNCPath() that preserves the UNC prefix through path.normalize, restoring the stripped backslash when needed
• Applied normalizePossiblyUNCPath() to both the input path and allowed directory normalization in isPathWithinAllowedDirectories

Test plan

• All 53 existing tests pass (including the existing UNC path test case)
• The existing UNC test at line 432 of path-validation.test.ts covers the core scenario: matching \\server\share\project against allowed directories, subdirectory access, and cross-server rejection
• To reproduce the original bug: configure \\192.168.x.x\share\ as an allowed directory on Windows and attempt file access -- previously always denied, now correctly allowed

[olaservo]

Good approach — skipping path.resolve entirely for UNC paths is the right call since that's what causes the corruption. The normalizePossiblyUNCPath helper is clean and well-documented. Non-UNC paths are unaffected.

One request before we can merge: could you add test cases for UNC paths? PR #3920 has some good ones you could reference. They should be gated behind path.sep === '\' (like the existing UNC test at line 432) since UNC paths are meaningless on Linux. Something like:

• UNC path within allowed UNC directory → true
• UNC path outside allowed UNC directory → false
• Exact UNC path match → true

---

This review was assisted by Claude Code.

[Christian-Sidak]

Added the three UNC path test cases (within allowed directory, outside allowed directory, exact match) all gated behind path.sep === '\\'. CI is re-running now.

[quocnam1]

FYI I opened #3981 which addresses the same root issue plus three other corruption sites (validatePath, bootstrap init, trailing-separator normalization) that also cause UNC failures on Windows. Happy to close this one if #3981 is preferred, or to rebase on top of it if you'd rather land yours first.

[Christian-Sidak]

Thanks for the heads up, @quocnam1! Happy to have this landed alongside or after #3981 -- whatever the maintainers prefer. If #3981's broader scope is more useful, I'm fine closing this one. Either way works for me.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 24s —— View job

---

Code Review

• Gather context and understand the request
• Read changed files
• Analyze the implementation
• Review test coverage
• Post review feedback

---

Overall: The fix is correct and the approach is sound. The core bug — path.resolve converting \\server\share into a drive-relative path on Windows — is cleanly avoided by detecting UNC paths and skipping path.resolve. No security regressions. A few notes below.

---

src/filesystem/path-validation.ts

The fix is correct. On Windows, path.normalize('\\\\server\\share') strips one leading backslash, and path.resolve('\\server\\share') then makes it drive-relative (C:\server\share). Skipping path.resolve for UNC paths and restoring the stripped backslash is the right call.

Minor: inline comment restates the JSDoc (path-validation.ts:22). The comment inside the body of normalizePossiblyUNCPath duplicates what the function-level JSDoc above already says. One or the other is enough.

// path.normalize may strip a leading backslash from UNC paths  ← redundant with JSDoc
if (!normalized.startsWith('\\\\')) {

Consideration: no platform guard in isUNCPath. On Linux, path.normalize doesn't treat \ as a separator, so a path like \\server\share would pass isUNCPath, be returned as-is from normalizePossiblyUNCPath, fail path.isAbsolute, and throw. This is probably acceptable (the caller throws on non-absolute paths anyway), but it's a subtle behavior difference from the pre-existing non-UNC code path which returns false on bad input inside the try/catch. Not a blocker since UNC paths won't appear on Linux in practice, but worth documenting.

Trailing separator coverage. Tracing \\server\share\project\ through the new code:

• path.normalize on Windows → \server\share\project (strips trailing sep and leading backslash)
• The prefix-restoration restores \\ → \\server\share\project

This matches the non-trailing form correctly. ✓

---

src/filesystem/__tests__/path-validation.test.ts

The three new test cases (allows UNC path within allowed UNC directory, blocks UNC path outside allowed UNC directory, allows exact UNC path match) are all covered by the pre-existing handles UNC paths on Windows test at line 432, which already tests exact match, subdirectory, same-share rejection, and cross-server rejection. The new tests don't add new logic — only the subdir\file.txt deeper nesting in the first case is marginally new.

This isn't harmful and the reviewer asked for them explicitly, but consolidating into the existing test block would reduce duplication. The path.sep === '\\' guard is correctly applied. ✓

---

Scope note (not a blocker)

Comment [quocnam1] is worth flagging: PR #3981 addresses the same root cause in additional locations (validatePath, bootstrap directory initialization, trailing-separator normalization). This PR only protects isPathWithinAllowedDirectories. Maintainers may want to decide whether to merge this PR as-is or wait for / redirect to #3981.

---

Summary: No correctness issues, no security issues. The inline comment on line 22 is minor redundancy. The broader scope gap (other normalization sites) is noted but is a separate concern. Approved with the above as suggestions.