fix(filesystem): reject Windows-style paths on POSIX hosts instead of writing literal filenames - #4689
Open
Parker-Fawcett wants to merge 1 commit into
Conversation
… writing literal filenames On POSIX, a path like C:\Users\me\notes\file.md is not absolute, so validatePath() sent it down the relative-path branch and path.resolve() placed it inside the allowed root as a single entry literally named 'C:\Users\me\notes\file.md' (or created a C:/ directory tree). Callers saw success and only discovered the mistake when inspecting disk - reported in modelcontextprotocol#4686. validatePath() now rejects drive-letter forms (^[A-Za-z]:(?:[\\/]|$)) up front when process.platform is not win32, with an explicit access-denied error. The check is one-directional and platform-guarded so Windows behavior is untouched and POSIX paths are never rewritten (no modelcontextprotocol#3628 regression). Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
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
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.
Description
On a POSIX host,
validatePath()accepts Windows drive-letter paths likeC:\Users\me\notes\file.mdas if they were relative paths.path.resolve(allowedDir, requested)then places them inside the allowed root as a single entry literally namedC:\Users\me\notes\file.md— or, forC:/Users/me/..., creates an actualC:/Users/...directory tree. The tool answers "Successfully wrote to ...", so the mistake is only discovered when inspecting disk.validatePath()now rejects strings matching a Windows drive-letter form (^[A-Za-z]:(?:[\\/]|$)) up front wheneverprocess.platform !== 'win32', with an explicit error:Fixes #4686.
Server Details
validatePathinsrc/filesystem/lib.ts) + testsMotivation and Context
Reported in #4686 with a full root-cause analysis: on POSIX
path.isAbsolute("C:\\...")is false, so the string falls intoresolveRelativePathAgainstAllowedDirectories()and resolves inside the sandbox. Nothing escapes containment —isPathWithinAllowedDirectories()is working correctly — but the caller's intent is silently misinterpreted, and the resulting file is easy to overlook (the reporter's sat unnoticed for three weeks).The guard is deliberately one-directional and platform-guarded:
process.platform !== 'win32', so Windows behavior is untouched.expandHome, failing fast with an actionable message.How Has This Been Tested?
src/filesystem/__tests__/lib.test.ts(POSIX-only viadescribe.skipIf(process.platform === 'win32'), following the file's existing platform-conditional style):C:\Users\me\notes\file.md,C:/Users/me/file.md,Z:\, and bareC:with the explicit error.realpathnever called).notes/file:C.md) still resolve normally on all platforms.Breaking Changes
Behavioral change only for inputs that were already broken in intent: passing a Windows path to a server running on macOS/Linux previously produced a misleading success plus a junk file; it now produces an immediate, descriptive error. No valid workflow is affected, and no client configuration changes are needed.
Types of changes
Checklist
validatePathinvocation over the built server module plus unit suites; N/A granularity for a validation-layer fix)Additional context
The regex intentionally follows the form proposed in #4686 (
^[A-Za-z]:[\\/]plus bare^[A-Za-z]:$, combined here as^[A-Za-z]:(?:[\\/]|$)). Known trade-off, accepted per that issue's spec: a POSIX filename that genuinely begins with a single letter followed by a colon (e.g.a:b/c) will now be rejected on POSIX hosts; such names are vanishingly rare in practice and were previously indistinguishable from this bug class anyway.Happy to follow up with the same treatment for UNC forms (
\\server\share) on POSIX if maintainers want them rejected too (#3527 covers the Windows side of UNC handling).