Skip to content

Conversation

@mfcasazza
Copy link
Contributor

Description

Added support for Windows file paths with spaces in them. Also added support for linked directories.

Server Details

  • Server: filesystem
  • Changes to: server implementation and path resolution logic

Motivation and Context

I ran into this bug: #447 when trying to use the claude desktop app.

How Has This Been Tested?

Tested extensively with Claude desktop client on Windows using multiple scenarios:

  1. Basic directory access (Desktop folder)
  2. Access to program files (I used my World of Warcraft directory - C:\Program Files (x86)\World of Warcraft)
  3. Confirmed proper security boundaries (access denied for non-allowed paths)
  4. Verified directory/file type differentiation is maintained through symbolic links
  5. Added tests to address the scenarios in the open bug

Breaking Changes

No breaking changes. Existing configurations should continue to work as before - I tested locally with my desktop and downloads folders

Types of changes

  • New feature (non-breaking change which adds functionality)
  • [] Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Additional context

I don't have a mac - the code shouldn't affect mac paths - however was not able to test that locally

@m2self
Copy link

m2self commented Feb 5, 2025

You need to incorporate this to normalize path like "c:\Users" to "C:\Users".

diff --git a/src/filesystem/index.ts b/src/filesystem/index.ts
index b4d5c41..9de9005 100644
--- a/src/filesystem/index.ts
+++ b/src/filesystem/index.ts
@@ -23,8 +23,19 @@ if (args.length === 0) {
 }

 // Normalize all paths consistently
-function normalizePath(p: string): string {
-  return path.normalize(p);
+let normalizePath: (p: string) => string;
+if (process.platform === 'win32') {
+    normalizePath = (p) => {
+        let n = path.normalize(p);
+        if (/^[a-z]:/.test(n)) {
+            n = n.charAt(0).toUpperCase() + n.slice(1);
+        }
+        return n;
+    }
+} else {
+    normalizePath = (p) => {
+        return path.normalize(p);
+    }
 }

 function expandHome(filepath: string): string {

@mfcasazza
Copy link
Contributor Author

mfcasazza commented Feb 8, 2025

Incorporated logic to hand lowercase drive letters per @m2self suggestion - tested and working locally

@NemanSyed
Copy link

Can this handle "special" characters in the folder name? For example the ampersand in "C:\Sub&Folder"

In my 0.7.7.0 release of the Windows desktop app, including the "C:\NS\Sub&Folder" entry in claude_desktop_config.json

    "filesystem": {
      "command": "npx",
      "args": [
        "-y",
        "@modelcontextprotocol/server-filesystem",
                "C:\\NS",
		"C:\\NS\\Folder",
		"C:\\NS\\Sub&Folder",
                "C:\\NS\\MYKIND~1",
                "/Users/NEMANS~1/FOLDER~2/SUBFO~1/Public/P12PST~1"
        ]
    }

Throws the following error at Claude app load time:
image

@mfcasazza
Copy link
Contributor Author

@NemanSyed yes the normalize path function takes care of that, added some tests for ya and here is a SS of it working locally for me
image

@NemanSyed
Copy link

NemanSyed commented Mar 3, 2025

Great, thanks @mfcasazza! Looking forward to getting this working!

Edit: Just tried with Claude desktop app 0.8.8.0. It still breaks if the path has spaces in it or it it has any "special" characters (I tested with &). How do I confirm I'm using your enhancements?

@mfcasazza
Copy link
Contributor Author

@NemanSyed my pull request is still open and hasn't been merged - you'd need to clone my code, build locally and point your filesystem config to the local version - my config looks like this and is what was working in my screenshot 4 days ago

image

@NemanSyed
Copy link

Thanks @mfcasazza. That's beyond my skill level at this point - I'll wait for the merge. Thank you for the new code and explanation!

@olaservo
Copy link
Member

Hi @mfcasazza thanks for opening this, fyi since a lot has changed since your last push I went ahead and:

  • Did some refactoring to use the latest helper functions
  • Included the latest security patching
  • Made sure the tests will run as part of CI

I am getting more 👀 on this PR to get it merged in, so let me know if you'd like to pick this up again to get it through the finish line. Or I can just take care of any necessary adjustments before merging, for the sake of expediting.

@olaservo olaservo requested a review from jenn-newton July 10, 2025 13:55
@mfcasazza
Copy link
Contributor Author

@olaservo I'm on a climbing trip until next Wednesday - happy to pick it up then but if you'd like to merge it feel free - thanks

@olaservo olaservo self-requested a review July 22, 2025 14:45
olaservo
olaservo previously approved these changes Jul 22, 2025
@domdomegg domdomegg requested review from olaservo and removed request for cliffhall, evalstate, jenn-newton and tadasant August 15, 2025 15:55
@domdomegg domdomegg enabled auto-merge (squash) August 15, 2025 15:56
@domdomegg domdomegg merged commit 4636883 into modelcontextprotocol:main Aug 18, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants