fix(filesystem): tolerate Windows realpath ENOENT

[krot4u]

Summary

• Fix Windows roots parsing/validation when s.realpath() returns ENOENT for existing paths (e.g. SUBST/mapped drives).
• In �alidatePath(), fall back to stat() checks when
  ealpath() fails with ENOENT to avoid false Parent directory does not exist.
• Add/adjust tests to avoid dependence on Windows 8.3 short-path differences and to cover the new fallback behavior.

Motivation

Cursor/VS Code clients may provide roots like ile:///d%3A/.... On some Windows setups, s.realpath() can fail with ENOENT for paths that exist, causing filesystem MCP to drop roots and reject valid operations.

Test plan

pm test in src/filesystem

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 5s —— View job

---

PR Review: fix(filesystem): tolerate Windows realpath ENOENT

Tasks:

• Gather context and understand request
• Read changed files
• Analyze diff
• Post review feedback

---

Overview

This PR addresses a real Windows pain point where fs.realpath() throws ENOENT for paths on SUBST/mapped drives that actually exist (e.g. Cursor/VS Code sending file:///d%3A/... URIs). The fix is applied in two places — roots-utils.ts (root discovery) and lib.ts (path validation) — and is a reasonable approach overall. A few issues deserve attention before merge.

---

Issues

1. Security: catch-all in roots-utils.ts swallows non-ENOENT errors

src/filesystem/roots-utils.ts:24–28

try {
  resolvedPath = await fs.realpath(absolutePath);
} catch {
  // keep `absolutePath`
}

The catch block is intentionally broad — it silences every error from realpath, not only ENOENT. That means:

• EACCES (permission denied to traverse a symlink) → silently uses the raw path, bypassing symlink target validation.
• ELOOP (too many symbolic links) → same: a cyclic-symlink attack is silently ignored and the unresolved path is accepted.

On the Windows SUBST scenario the error is always ENOENT, so this should be narrowed:

try {
  resolvedPath = await fs.realpath(absolutePath);
} catch (err) {
  if ((err as NodeJS.ErrnoException)?.code !== 'ENOENT') throw err;
  // keep absolutePath — realpath can fail with ENOENT on SUBST/mapped drives
}

Fix this →

---

2. Misleading test mock — "handles non-existent files" test

src/filesystem/__tests__/lib.test.ts:183–186

mockFs.realpath
  .mockRejectedValueOnce(enoentError)
  .mockResolvedValueOnce(parentPath);   // ← this mock is never consumed

The updated validatePath now calls fs.stat(absolute) (not fs.realpath) for the parent check. The second mockResolvedValueOnce(parentPath) on mockFs.realpath is dead setup — it's never called. The test passes only because mockFs.stat is not mocked here and auto-resolves to undefined (truthy enough), which happens to match the "path exists" branch.

The test should be updated to explicitly mock the new code path:

mockFs.realpath.mockRejectedValueOnce(enoentError);
mockFs.stat.mockResolvedValueOnce({ isDirectory: () => false }); // stat(absolute) succeeds

Fix this →

---

3. Misleading test mock — "rejects when parent directory does not exist" test

src/filesystem/__tests__/lib.test.ts:199–201

mockFs.realpath
  .mockRejectedValueOnce(enoentError1)
  .mockRejectedValueOnce(enoentError2);  // ← never consumed

Same issue: the second realpath rejection is never reached. The important mock is mockFs.stat (correctly added on lines 205–207), but the dead realpath setup is confusing.

---

4. Test coverage gap — no unit mock test for the SUBST scenario in roots-utils.test.ts

The roots-utils.test.ts tests use real tmpdir directories. There is no test that simulates the SUBST scenario itself (i.e., realpath throws ENOENT but stat succeeds). Without a mocked unit test, the specific regression being fixed is not covered.

Consider adding a test with vi.mock('fs/promises') that mocks realpath to throw ENOENT while stat succeeds, verifying the path is still included in the result.

---

5. Minor: lib.ts try/catch structure wraps more than realpath

src/filesystem/lib.ts:117–142

try {
  const realPath = await fs.realpath(absolute);
  // ...
  if (!isPathWithinAllowedDirectories(normalizedReal, allowedDirectories)) {
    throw new Error(`Access denied - symlink target outside allowed directories: ...`);
  }
  return realPath;
} catch (error) {
  if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') { ... }
  throw error;
}

The "Access denied" error thrown by the inner if is caught by the same catch block, then re-thrown (since it has no .code === 'ENOENT'). Behaviour is correct, but the structure is confusing. This predates the PR, so just a note rather than a blocker.

---

Summary

Fix correctness	✅ The two-phase fallback (stat absolute → stat parent) in lib.ts is logically sound and covers the real-world scenario.
Blocker	⚠️ The catch-all in roots-utils.ts is a security regression — only ENOENT should be suppressed.
Test quality	The dead realpath mocks in lib.test.ts should be cleaned up to reflect actual code paths.
Test coverage	A mocked unit test for the SUBST/mapped-drive scenario would strengthen confidence.

[krot4u]

Pushed fixes for review notes:

• Narrowed roots realpath fallback to only suppress ENOENT (rethrow other errors)
• Cleaned up validatePath tests to mock stat() fallback (removed dead realpath mocks)
• Added a mocked unit test covering the SUBST/mapped-drive scenario (realpath ENOENT + stat succeeds)

Tests: npm test -w @modelcontextprotocol/server-filesystem

[LuuOW]

Technical audit: Verified MCP server implementation for consistency with current SDK patterns.