fix: preserve UNC path prefix in isPathWithinAllowedDirectories

[hashwnath]

Summary

Fixes #3756

On Windows, isPathWithinAllowedDirectories fails for UNC paths (e.g. \\server\share\project) because path.resolve(path.normalize(...)) can corrupt the UNC prefix — stripping a leading backslash and turning \\server\share into \server\share, which path.resolve then interprets as drive-relative (e.g. C:\server\share). This causes all UNC path access to fail with "Access denied - path outside allowed directories".

Fix

Extract path normalization into a normalizePath() helper that detects UNC paths and normalizes them without path.resolve:

• UNC paths (\\server\share\...) are always absolute, so path.normalize alone is sufficient
• After normalization, verify the \\ prefix is preserved; restore it if stripped
• Non-UNC paths continue using path.resolve(path.normalize(...)) as before

Test plan

• All 53 existing path-validation tests pass
• On Windows with a UNC allowed directory (\\192.168.x.x\share), files within the share are now accessible
• Non-UNC paths (drive letters, Unix paths) behave identically to before

[cliffhall]

@claude review

[claude]

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

---

PR Review: fix: preserve UNC path prefix in isPathWithinAllowedDirectories

Tasks:

• Gather context and read PR
• Read changed files and git diff
• Analyze the fix for correctness, edge cases, and security
• Post review feedback

---

Summary

This is a well-targeted, minimal fix for a real Windows bug. The approach is sound and the existing test suite provides good coverage for non-UNC paths. A few concerns worth addressing below.

---

What the PR Does

Two private helpers are added to src/filesystem/path-validation.ts:

function isUNCPath(p: string): boolean {
  return p.startsWith('\\\\') && !p.startsWith('\\\\?\\');
}

function normalizePath(p: string): string {
  if (isUNCPath(p)) {
    const normalized = path.normalize(p);
    if (!normalized.startsWith('\\\\')) {
      return '\\' + normalized;  // restore stripped leading \
    }
    return normalized;
  }
  return path.resolve(path.normalize(p));
}

And the two inline path.resolve(path.normalize(...)) calls in isPathWithinAllowedDirectories are replaced with normalizePath(...).

---

Issues

1. No dedicated unit tests for isUNCPath / normalizePath

The only UNC test is at line 432 of path-validation.test.ts, and it is gated behind if (path.sep === '\\'). This means CI running on Linux will never exercise the new code paths — not even the isUNCPath string-matching logic. Consider adding a small, platform-unconditional test block that covers the helpers directly:

• isUNCPath('\\\\server\\share') → true
• isUNCPath('\\\\?\\C:\\long\\path') → false (extended-length, excluded)
• isUNCPath('/unix/path') → false
• normalizePath('\\\\server\\share\\sub') preserves \\ prefix

2. The prefix-restoration branch is untested

The recovery branch:

if (!normalized.startsWith('\\\\')) {
  return '\\' + normalized;
}

has no test that exercises it. If path.normalize on the CI platform never triggers this branch, a regression there would be silent. Even a unit test with a mock or a comment documenting when this branch fires would help.

3. Correctness of the restoration logic

The logic looks correct for the documented bug (Node.js stripping one \ from \\server\share, turning it into \server\share). Re-adding '\\' (one backslash character) restores \\server\share. That said, the comment could be clearer — '\\' in JavaScript is a single \ character, which might confuse future readers. A named constant like UNC_PREFIX = '\\\\' (two backslashes) paired with a .slice(1) approach might be more legible.

4. isUNCPath does not validate the full UNC structure

Paths like \\ or \\server (missing the \share component) would pass isUNCPath and be sent through path.normalize without path.resolve. These are structurally invalid UNC paths, but the code won't reject them — it will just normalize and continue. path.isAbsolute will still catch the case where the result is non-absolute, but it's a minor ambiguity worth noting.

5. Case sensitivity for UNC paths

This is a pre-existing issue, not introduced by this PR: UNC hostnames on Windows are case-insensitive (\\SERVER\share and \\server\share are the same), but the startsWith comparison is case-sensitive. This means \\SERVER\share\file would fail validation against an allowed dir of \\server\share. Outside scope for this PR, but worth filing a follow-up issue.

---

Positives

• Correctly excludes \\?\ (extended-length paths) from the UNC branch — these require different handling and should continue through path.resolve.
• Non-UNC code path is entirely unchanged, so no regression risk for existing users.
• The fix is in the right layer: path normalization is centralized rather than scattered across callers.
• The approach (skip path.resolve for already-absolute UNC paths) is architecturally clean and matches Node.js semantics.

---

Minor

The whitespace change on line 7 of the diff (- * \n+ *\n) is a trivial trailing-space cleanup — fine to keep.

---

Verdict

Approve with suggestions. The core fix is correct and the security model is unchanged. The main gap is test coverage — the new helpers should have at least a few platform-unconditional tests so CI catches any future regression. The restoration branch (return '\\' + normalized) in particular is a silent path today.

Fix missing tests →

[LuuOW]

Technical audit: Verified MCP server implementation for consistency with current SDK patterns.