Fix isPathWithinAllowedDirectories for UNC paths on Windows#3921
Fix isPathWithinAllowedDirectories for UNC paths on Windows#3921Christian-Sidak wants to merge 2 commits intomodelcontextprotocol:mainfrom
Conversation
On Windows, path.normalize can strip a leading backslash from UNC paths (\\server\share becomes \server\share). When path.resolve is then called on this single-backslash path, it gets interpreted as drive-relative (e.g. C:\server\share), causing the allowed-directory check to always fail for UNC network paths. This introduces normalizePossiblyUNCPath which detects UNC paths and preserves the \\\\ prefix through normalization, rather than passing them through path.resolve which corrupts them. Fixes modelcontextprotocol#3756
olaservo
left a comment
olaservo
left a comment
There was a problem hiding this comment.
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.
|
Added the three UNC path test cases (within allowed directory, outside allowed directory, exact match) all gated behind |
PR modelcontextprotocol#3921 fixes isPathWithinAllowedDirectories for UNC paths, but three other sites in the filesystem server also corrupt UNC paths via path.resolve or preserve them incorrectly through path.normalize: 1. path-validation.ts::normalizePossiblyUNCPath path.normalize preserves trailing separators on UNC paths on Windows (unlike path.resolve for drive paths), so 'normalizedDir + path.sep' produces a double separator that startsWith() never matches. Strip the trailing separator to harmonize. 2. lib.ts::validatePath Calls path.resolve(expandedPath) before the allowed-directory check, which on Windows corrupts UNC paths into drive-relative paths (\\\\server\\share -> C:\\server\\share). Skip path.resolve for UNC paths and let normalizePath handle them. 3. index.ts bootstrap Same path.resolve corruption when normalizing allowed directories at server startup, before they are stored via setAllowedDirectories. Same guard applied. All non-UNC paths retain existing behavior (path.resolve + normalize). Tests added: - 4 tests in path-validation.test.ts for the trailing-separator case - 6 tests in lib.test.ts for validatePath end-to-end on UNC, gated behind process.platform === 'win32' and using the existing fs mock pattern - Bug modelcontextprotocol#3 (bootstrap) has no direct unit test since UNC access in CI is not available, but the fix applies the same principle as validatePath and is logically covered. Fixes modelcontextprotocol#3756 Supersedes modelcontextprotocol#3921
|
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. |
3 participants
Summary
Fixes #3756
On Windows,
isPathWithinAllowedDirectoriesfails for UNC paths (e.g.\\192.168.1.1\share\) becausepath.normalizestrips a leading backslash from UNC paths, and thenpath.resolveinterprets 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
normalizePossiblyUNCPathhelper that detects UNC paths and preserves the\\prefix through normalization, rather than passing them throughpath.resolvewhich corrupts them. Non-UNC paths continue to use the existingpath.resolve(path.normalize(...))logic.Changes
isUNCPath()helper to detect UNC paths by their\\prefixnormalizePossiblyUNCPath()that preserves the UNC prefix throughpath.normalize, restoring the stripped backslash when needednormalizePossiblyUNCPath()to both the input path and allowed directory normalization inisPathWithinAllowedDirectoriesTest plan
path-validation.test.tscovers the core scenario: matching\\server\share\projectagainst allowed directories, subdirectory access, and cross-server rejection\\192.168.x.x\share\as an allowed directory on Windows and attempt file access -- previously always denied, now correctly allowed