Skip to content

Conversation

@tomonacci
Copy link
Contributor

Description

When normalizing the paths for allowed directories, use negative lookbehind to ensure that the slashes we are removing are trailing, i.e., not at the beginning of the path.

Server Details

  • Server: filesystem
  • Changes to: roots

Motivation and Context

This becomes relevant when you specify the root directory (/) as the allowed directory. Before the fix, / is normalized to the empty string and you get ENOENT:

vagrant@debian-13:~$  npx @modelcontextprotocol/server-filesystem /
Need to install the following packages:
@modelcontextprotocol/[email protected]
Ok to proceed? (y) y

Error accessing directory : Error: ENOENT: no such file or directory, stat ''
    at async Object.stat (node:internal/fs/promises:1040:18)
    at async file:///home/vagrant/.npm/_npx/a3241bba59c344f5/node_modules/@modelcontextprotocol/server-filesystem/dist/index.js:43:23
    at async Promise.all (index 0)
    at async file:///home/vagrant/.npm/_npx/a3241bba59c344f5/node_modules/@modelcontextprotocol/server-filesystem/dist/index.js:41:1 {
  errno: -2,
  code: 'ENOENT',
  syscall: 'stat',
  path: ''
}

How Has This Been Tested?

By running tests locally. I added a couple of test cases that fail before the change and pass after. Also, using / like the above works after the change.

Breaking Changes

None.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • 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

Copy link
Member

@domdomegg domdomegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the regex change - no ReDoS concerns here.

The lookbehind (?<!^) is safe: ReDoS comes from nested quantifiers or overlapping alternatives that cause exponential backtracking. This regex just does a constant-time position check + single character match + anchor. No backtracking possible.

The fix is correct - after the first replace collapses multiple slashes, there's at most one trailing slash, and the lookbehind ensures we preserve root /.

Lookbehind requires ES2018/Node 8.10+, but the server requires Node 18+ anyway.

Tests cover the edge cases well. LGTM!

@domdomegg domdomegg merged commit 1ae217d into modelcontextprotocol:main Jan 5, 2026
19 checks passed
@tomonacci
Copy link
Contributor Author

Thanks for the quick review and merge!

@tomonacci tomonacci deleted the remove-only-trailing-slashes branch January 6, 2026 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants