Fix isPathWithinAllowedDirectories for UNC paths on Windows#3921
Open
Christian-Sidak wants to merge 1 commit intomodelcontextprotocol:mainfrom
Open
Fix isPathWithinAllowedDirectories for UNC paths on Windows#3921Christian-Sidak wants to merge 1 commit intomodelcontextprotocol:mainfrom
Christian-Sidak wants to merge 1 commit 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
reviewed
Apr 12, 2026
Member
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.
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
2 participants
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
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