From 22ce3e52148f95bfb991dbfd84d9bcaecb4678f7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 18:16:46 -0400 Subject: [PATCH] fix(filesystem): do not trim an allowed directory's name after realpath normalizePath trims whitespace and strips a leading or trailing quote, which suits a typed request but not a directory name the file system returned. resolveAllowedDirectories (both forms of a command-line directory) and parseRootUri (a Root's realpath) used it, so an allowed directory named `project ` was allowed as `project`, its sibling. Use normalizePathFormat, which formats without trimming, for all of them. The command-line form is not trimmed either: trimming after path.resolve could only ever drop a trailing character, which is exactly the sibling case, never a leading quote left over from shell quoting. Closes #5320 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .changeset/filesystem-realpath-no-trim.md | 5 ++ src/filesystem/__tests__/roots-utils.test.ts | 33 +++++++++ src/filesystem/__tests__/stdio.test.ts | 73 ++++++++++++++++++++ src/filesystem/index.ts | 16 ++++- src/filesystem/roots-utils.ts | 13 ++-- 5 files changed, 129 insertions(+), 11 deletions(-) create mode 100644 .changeset/filesystem-realpath-no-trim.md diff --git a/.changeset/filesystem-realpath-no-trim.md b/.changeset/filesystem-realpath-no-trim.md new file mode 100644 index 0000000000..d8b2df6887 --- /dev/null +++ b/.changeset/filesystem-realpath-no-trim.md @@ -0,0 +1,5 @@ +--- +"@modelcontextprotocol/server-filesystem": patch +--- + +Keep an allowed directory's name exactly as the file system spells it. A command-line directory, or a client Root's realpath, whose name ends in a space or a quote was trimmed, so the server allowed the sibling directory without that suffix (and refused the directory that was actually given). diff --git a/src/filesystem/__tests__/roots-utils.test.ts b/src/filesystem/__tests__/roots-utils.test.ts index 411aa84e48..862998173c 100644 --- a/src/filesystem/__tests__/roots-utils.test.ts +++ b/src/filesystem/__tests__/roots-utils.test.ts @@ -13,6 +13,7 @@ import { mkdirSync, writeFileSync, realpathSync, + symlinkSync, } from "fs"; import { homedir, tmpdir } from "os"; import { join } from "path"; @@ -139,3 +140,35 @@ describe("getValidRootDirectories", () => { }); }); }); + +// #5320: a Root's realpath is kept as the file system spells it. Trimming a +// trailing space or quote from it names the sibling without the suffix. +// Windows refuses or strips these characters in names, so POSIX only. +describe.skipIf(process.platform === "win32")( + "getValidRootDirectories: a realpath ending in a space or quote (#5320)", + () => { + let base: string; + + beforeEach(() => { + base = realpathSync.native(mkdtempSync(join(tmpdir(), "mcp-fs-5320-"))); + }); + + afterEach(() => { + rmSync(base, { recursive: true, force: true }); + }); + + it.each([" ", "'", '"'])( + "keeps the resolved form untrimmed when it ends in %j", + async (suffix) => { + const target = join(base, `real${suffix}`); + mkdirSync(target); + mkdirSync(join(base, "real")); + const link = join(base, "link"); + symlinkSync(target, link); + expect( + await getValidRootDirectories([{ uri: pathToFileURL(link).href }]), + ).toEqual([link, target]); + }, + ); + }, +); diff --git a/src/filesystem/__tests__/stdio.test.ts b/src/filesystem/__tests__/stdio.test.ts index 196fa30f59..cc7cf06571 100644 --- a/src/filesystem/__tests__/stdio.test.ts +++ b/src/filesystem/__tests__/stdio.test.ts @@ -275,3 +275,76 @@ describe("isEntryPoint", () => { expect(isEntryPoint(undefined, moduleUrl)).toBe(false); }); }); + +// #5320: a command-line directory whose name ends in a space or quote is +// allowed under that exact name. Trimming it, in its given form or its +// realpath, names the sibling without the suffix and allows that instead. +// Windows refuses or strips these characters in names, so POSIX only. +describe.skipIf(process.platform === "win32").each([ + ["a space", " "], + ["a single quote", "'"], + ["a double quote", '"'], +])("an allowed directory whose name ends in %s (#5320)", (_, suffix) => { + let allowed: string; + let sibling: string; + + beforeEach(async () => { + allowed = path.join(dir, `project${suffix}`); + sibling = path.join(dir, "project"); + await fs.mkdir(allowed); + await fs.mkdir(sibling); + await fs.writeFile(path.join(allowed, "mine.txt"), "allowed"); + await fs.writeFile(path.join(sibling, "secret.txt"), "not allowed"); + }); + + async function readOver(args: string[], file: string) { + await session.start(args); + await session.initialize(); + session.send(2, "tools/call", { + name: "list_allowed_directories", + arguments: {}, + }); + session.send(3, "tools/call", { + name: "read_text_file", + arguments: { path: file }, + }); + return { + listed: (await session.response(2)).result?.structuredContent, + read: (await session.response(3)).result, + }; + } + + it("lists it untrimmed and refuses the sibling, given directly", async () => { + const { listed, read } = await readOver( + [allowed], + path.join(sibling, "secret.txt"), + ); + expect(listed).toEqual({ content: `Allowed directories:\n${allowed}` }); + expect(read?.isError).toBe(true); + expect(JSON.stringify(read?.content)).toMatch( + /Access denied - path outside allowed directories/, + ); + }); + + it("keeps the realpath untrimmed and refuses the sibling, given through a symlink", async () => { + const link = path.join(dir, "link"); + await fs.symlink(allowed, link); + const { listed, read } = await readOver( + [link], + path.join(sibling, "secret.txt"), + ); + expect(listed).toEqual({ + content: `Allowed directories:\n${link}\n${allowed}`, + }); + expect(read?.isError).toBe(true); + expect(JSON.stringify(read?.content)).toMatch( + /Access denied - path outside allowed directories/, + ); + }); + + it("still reads the allowed directory's own file", async () => { + const { read } = await readOver([allowed], path.join(allowed, "mine.txt")); + expect(read?.isError).toBeFalsy(); + expect(read?.structuredContent).toEqual({ content: "allowed" }); + }); +}); diff --git a/src/filesystem/index.ts b/src/filesystem/index.ts index 160948c18b..d6bcc24a26 100644 --- a/src/filesystem/index.ts +++ b/src/filesystem/index.ts @@ -19,7 +19,11 @@ import { StdioServerTransport, type StdioServerHandle, } from "@modelcontextprotocol/server/stdio"; -import { normalizePath, expandHome, realpathWithRetry } from "./path-utils.js"; +import { + normalizePathFormat, + expandHome, + realpathWithRetry, +} from "./path-utils.js"; import { createServer } from "./server.js"; /** @@ -39,12 +43,18 @@ export async function resolveAllowedDirectories( args.map(async (dir) => { const expanded = expandHome(dir); const absolute = path.resolve(expanded); - const normalizedOriginal = normalizePath(absolute); + // normalizePathFormat, not normalizePath, for both forms: a trailing + // space or quote is part of a POSIX filename, so trimming it would + // allow the sibling that lacks it (#5320). Trimming after + // path.resolve could only ever remove a trailing character, never + // shell-quoting leftovers at the start, so the argument is taken + // exactly as the shell passed it. + const normalizedOriginal = normalizePathFormat(absolute); try { // Security: Resolve symlinks in allowed directories during startup // This ensures we know the real paths and can validate against them later const resolved = await realpathWithRetry(absolute); - const normalizedResolved = normalizePath(resolved); + const normalizedResolved = normalizePathFormat(resolved); // Return both original and resolved paths if they differ // This allows matching against either /tmp or /private/tmp on macOS if (normalizedOriginal !== normalizedResolved) { diff --git a/src/filesystem/roots-utils.ts b/src/filesystem/roots-utils.ts index 9b2b7a14ff..119f97ec1a 100644 --- a/src/filesystem/roots-utils.ts +++ b/src/filesystem/roots-utils.ts @@ -1,11 +1,7 @@ import { promises as fs, type Stats } from "fs"; import path from "path"; import os from "os"; -import { - normalizePath, - normalizePathFormat, - realpathWithRetry, -} from "./path-utils.js"; +import { normalizePathFormat, realpathWithRetry } from "./path-utils.js"; import type { Root } from "@modelcontextprotocol/server"; import { fileURLToPath } from "url"; @@ -36,10 +32,11 @@ async function parseRootUri(rootUri: string): Promise { const absolutePath = path.resolve(expandedPath); const resolvedPath = await realpathWithRetry(absolutePath); return { - // normalizePathFormat, not normalizePath: trimming whitespace or quotes - // from the client's spelling would name a different directory. + // normalizePathFormat, not normalizePath, for both: trimming whitespace + // or quotes from the client's spelling or from the realpath would name + // a different directory, and allow it (#5183, #5320). advertised: normalizePathFormat(absolutePath), - resolved: normalizePath(resolvedPath), + resolved: normalizePathFormat(resolvedPath), }; } catch { return null; // Path doesn't exist or other error