fix(filesystem): normalize UNC paths in access check

[nielskaspers]

Summary

UNC/network share paths (\\server\share\subdir) fail the access check in isPathWithinAllowedDirectories() despite the parent share being in the allowed directories list. The root listing works but any subdirectory access fails.

Root cause: path-validation.ts used path.resolve(path.normalize(...)) to normalize paths, which can mangle the \\ prefix on UNC paths on Windows. Meanwhile, path-utils.ts already has a normalizePath() function with explicit UNC path handling that preserves the prefix correctly.

Fix: Use normalizePath() from path-utils.ts in isPathWithinAllowedDirectories() instead of raw path.resolve(path.normalize(...)), ensuring both the requested path and allowed directories are normalized consistently for UNC paths.

All 145 existing tests pass.

Fixes #3527

[cliffhall]

@claude review

[claude]

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

---

PR Review

• Gather context and read changed files
• Review the implementation
• Check for correctness, security, and test coverage
• Post final review

---

Summary: The fix is correct and well-targeted. It resolves a real bug where UNC paths like \\server\share\subdir were being rejected by isPathWithinAllowedDirectories() because path.resolve() can mangle the \\ prefix. The change reuses the existing normalizePath() from path-utils.ts which already has explicit UNC handling — good reuse of existing infrastructure.

---

What Changed

src/filesystem/path-validation.ts — two normalization calls updated:

Before:

normalizedPath = path.resolve(path.normalize(absolutePath));
// ...
normalizedDir = path.resolve(path.normalize(dir));

After:

normalizedPath = normalizePath(path.resolve(normalizePath(absolutePath)));
// ...
normalizedDir = normalizePath(path.resolve(normalizePath(dir)));

The double-wrap (normalizePath → path.resolve → normalizePath) is intentional and correct:

1. Inner normalizePath() normalizes the UNC prefix into a consistent form before path.resolve()
2. path.resolve() handles .. segments — still critical for traversal protection
3. Outer normalizePath() repairs any UNC prefix mangling that path.resolve() may have introduced

---

Issues Found

Missing test for the exact bug scenario (minor, but worth adding)

The existing UNC test in path-validation.test.ts:432-441 uses \\\\server\\share\\project (a sub-path of the share) as the allowed directory. The bug in #3527 occurs when the share root itself (\\\\server\\share) is in the allowed list and a subdirectory is accessed. That specific case isn't covered:

// Missing coverage for issue #3527 specifically
const allowed = ['\\\\server\\share'];
expect(isPathWithinAllowedDirectories('\\\\server\\share\\subdir', allowed)).toBe(true);
expect(isPathWithinAllowedDirectories('\\\\server\\share\\subdir\\file.txt', allowed)).toBe(true);

Fix this →

Windows-only test coverage (expected limitation, but worth noting)

All UNC path tests are guarded by if (path.sep === '\\'), so they don't execute in Linux CI. This is unavoidable since UNC paths are a Windows-only concept, but it means the fix itself can't be verified by automated CI. Consider noting in the PR description whether this was manually tested on Windows.

---

Security Review

The security properties are fully preserved:

• Path traversal via .. is still neutralized by path.resolve() before the final check
• Null byte injection is rejected before normalization
• Prefix vulnerability protection (normalizedPath.startsWith(normalizedDir + path.sep)) is unchanged
• The normalizePath() function's quote-stripping (p.trim().replace(/^["']|["']$/g, '')) is pre-existing behavior, not introduced here

---

Verdict

This is a clean, focused fix. The double-wrap normalization is correct and the root cause analysis is accurate. The only gap is a test targeting the specific bug scenario from #3527 (share root as allowed dir). Everything else looks good.