fix(windows): handle file paths correctly on Windows - #172
Merged
Conversation
freema
marked this pull request as ready for review
August 25, 2026 20:26
juliandescottes
requested changes
Aug 25, 2026
juliandescottes
left a comment
juliandescottes
left a comment
Collaborator
There was a problem hiding this comment.
Thanks, LGTM, one misplaced comment to fix.
Comment on lines
32
to
37
| @@ -35,13 +35,27 @@ async function isDirectory(path: string): Promise<boolean> { | |||
| * current working directory; absolute paths must stay within | |||
| * ~/.firefox-devtools-mcp. | |||
| */ | |||
Collaborator
There was a problem hiding this comment.
This comment should stay close to assertAllowedPath
Collaborator
Author
There was a problem hiding this comment.
Moved isWithinRoot above the docblock and squashed it into the first commit. Thanks.
The saveTo boundary check compared the resolved path against its allowed root with a case-sensitive startsWith. Windows paths are case-insensitive, so `c:\Users\me\.firefox-devtools-mcp\out.json` was rejected as "outside the allowed location" while the identical path with a capital drive letter was accepted. Agents produce either spelling. Ignoring case on Windows only removes false rejections: the comparison now matches what the filesystem considers the same directory, so it cannot let through a path that previously escaped.
Both log paths opened their file without ensuring the directory existed, while the auto-generated path right beside one of them already called mkdirSync. A --output-file whose directory was missing therefore threw ENOENT from deep inside connect(), and a --log-file in the same state silently disabled logging through a stream error. Windows makes this easy to hit, since paths like /tmp/foo.log do not exist there at all, but the gap is not platform-specific. saveOutput already creates parents for the paths it is given; this brings both log paths in line.
freema
force-pushed
the
fix/windows-path-handling
branch
from
August 26, 2026 14:10
9c56de2 to
55fbccb
Compare
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.
Split out of #169, which @juliandescottes asked me to break into smaller pieces. This part is independent of everything else in that PR.
Two fixes around file paths:
The
saveToboundary check compared the resolved path against its allowed root with a case-sensitivestartsWith. Windows paths are case-insensitive, soc:\Users\me\.firefox-devtools-mcp\out.jsonwas rejected as "outside the allowed location" while the same path with a capital drive letter was accepted. Agents produce either spelling. Ignoring case on Windows only removes false rejections, it cannot let through a path that previously escaped.Both log paths opened their file without ensuring the directory existed, while the auto-generated path right beside one of them already called
mkdirSync. An--output-filewhose directory was missing threw ENOENT from deep insideconnect(), and a--log-filein the same state silently disabled logging through a stream error. Not platform-specific, just easy to hit on Windows where/tmp/foo.logdoes not exist at all.Verified on Windows 11, Node 22.22.0, Firefox 154.0. Every case was reproduced against
mainfirst:mainsaveTowith a lowercase drive letter (c:\Users\...)saveTowith an uppercased segment (\USERS\)--log-fileinto a missing directory--output-fileinto a missing directoryconnect()The escapes still fail closed: a path outside the root, the
~/.firefox-devtools-mcp-evilprefix look-alike, a..traversal, and the same traversal spelled with a lowercase drive letter are all still rejected. Unit suite, typecheck, lint andformat:checkare green on Windows.