Skip to content

Commit 22ce3e5

Browse files
cliffhallclaude
andcommitted
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 <[email protected]> Signed-off-by: cliffhall <[email protected]>
1 parent 446be48 commit 22ce3e5

5 files changed

Lines changed: 129 additions & 11 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@modelcontextprotocol/server-filesystem": patch
3+
---
4+
5+
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).

‎src/filesystem/__tests__/roots-utils.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
mkdirSync,
1414
writeFileSync,
1515
realpathSync,
16+
symlinkSync,
1617
} from "fs";
1718
import { homedir, tmpdir } from "os";
1819
import { join } from "path";
@@ -139,3 +140,35 @@ describe("getValidRootDirectories", () => {
139140
});
140141
});
141142
});
143+
144+
// #5320: a Root's realpath is kept as the file system spells it. Trimming a
145+
// trailing space or quote from it names the sibling without the suffix.
146+
// Windows refuses or strips these characters in names, so POSIX only.
147+
describe.skipIf(process.platform === "win32")(
148+
"getValidRootDirectories: a realpath ending in a space or quote (#5320)",
149+
() => {
150+
let base: string;
151+
152+
beforeEach(() => {
153+
base = realpathSync.native(mkdtempSync(join(tmpdir(), "mcp-fs-5320-")));
154+
});
155+
156+
afterEach(() => {
157+
rmSync(base, { recursive: true, force: true });
158+
});
159+
160+
it.each([" ", "'", '"'])(
161+
"keeps the resolved form untrimmed when it ends in %j",
162+
async (suffix) => {
163+
const target = join(base, `real${suffix}`);
164+
mkdirSync(target);
165+
mkdirSync(join(base, "real"));
166+
const link = join(base, "link");
167+
symlinkSync(target, link);
168+
expect(
169+
await getValidRootDirectories([{ uri: pathToFileURL(link).href }]),
170+
).toEqual([link, target]);
171+
},
172+
);
173+
},
174+
);

‎src/filesystem/__tests__/stdio.test.ts‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,3 +275,76 @@ describe("isEntryPoint", () => {
275275
expect(isEntryPoint(undefined, moduleUrl)).toBe(false);
276276
});
277277
});
278+
279+
// #5320: a command-line directory whose name ends in a space or quote is
280+
// allowed under that exact name. Trimming it, in its given form or its
281+
// realpath, names the sibling without the suffix and allows that instead.
282+
// Windows refuses or strips these characters in names, so POSIX only.
283+
describe.skipIf(process.platform === "win32").each([
284+
["a space", " "],
285+
["a single quote", "'"],
286+
["a double quote", '"'],
287+
])("an allowed directory whose name ends in %s (#5320)", (_, suffix) => {
288+
let allowed: string;
289+
let sibling: string;
290+
291+
beforeEach(async () => {
292+
allowed = path.join(dir, `project${suffix}`);
293+
sibling = path.join(dir, "project");
294+
await fs.mkdir(allowed);
295+
await fs.mkdir(sibling);
296+
await fs.writeFile(path.join(allowed, "mine.txt"), "allowed");
297+
await fs.writeFile(path.join(sibling, "secret.txt"), "not allowed");
298+
});
299+
300+
async function readOver(args: string[], file: string) {
301+
await session.start(args);
302+
await session.initialize();
303+
session.send(2, "tools/call", {
304+
name: "list_allowed_directories",
305+
arguments: {},
306+
});
307+
session.send(3, "tools/call", {
308+
name: "read_text_file",
309+
arguments: { path: file },
310+
});
311+
return {
312+
listed: (await session.response(2)).result?.structuredContent,
313+
read: (await session.response(3)).result,
314+
};
315+
}
316+
317+
it("lists it untrimmed and refuses the sibling, given directly", async () => {
318+
const { listed, read } = await readOver(
319+
[allowed],
320+
path.join(sibling, "secret.txt"),
321+
);
322+
expect(listed).toEqual({ content: `Allowed directories:\n${allowed}` });
323+
expect(read?.isError).toBe(true);
324+
expect(JSON.stringify(read?.content)).toMatch(
325+
/Access denied - path outside allowed directories/,
326+
);
327+
});
328+
329+
it("keeps the realpath untrimmed and refuses the sibling, given through a symlink", async () => {
330+
const link = path.join(dir, "link");
331+
await fs.symlink(allowed, link);
332+
const { listed, read } = await readOver(
333+
[link],
334+
path.join(sibling, "secret.txt"),
335+
);
336+
expect(listed).toEqual({
337+
content: `Allowed directories:\n${link}\n${allowed}`,
338+
});
339+
expect(read?.isError).toBe(true);
340+
expect(JSON.stringify(read?.content)).toMatch(
341+
/Access denied - path outside allowed directories/,
342+
);
343+
});
344+
345+
it("still reads the allowed directory's own file", async () => {
346+
const { read } = await readOver([allowed], path.join(allowed, "mine.txt"));
347+
expect(read?.isError).toBeFalsy();
348+
expect(read?.structuredContent).toEqual({ content: "allowed" });
349+
});
350+
});

‎src/filesystem/index.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,11 @@ import {
1919
StdioServerTransport,
2020
type StdioServerHandle,
2121
} from "@modelcontextprotocol/server/stdio";
22-
import { normalizePath, expandHome, realpathWithRetry } from "./path-utils.js";
22+
import {
23+
normalizePathFormat,
24+
expandHome,
25+
realpathWithRetry,
26+
} from "./path-utils.js";
2327
import { createServer } from "./server.js";
2428

2529
/**
@@ -39,12 +43,18 @@ export async function resolveAllowedDirectories(
3943
args.map(async (dir) => {
4044
const expanded = expandHome(dir);
4145
const absolute = path.resolve(expanded);
42-
const normalizedOriginal = normalizePath(absolute);
46+
// normalizePathFormat, not normalizePath, for both forms: a trailing
47+
// space or quote is part of a POSIX filename, so trimming it would
48+
// allow the sibling that lacks it (#5320). Trimming after
49+
// path.resolve could only ever remove a trailing character, never
50+
// shell-quoting leftovers at the start, so the argument is taken
51+
// exactly as the shell passed it.
52+
const normalizedOriginal = normalizePathFormat(absolute);
4353
try {
4454
// Security: Resolve symlinks in allowed directories during startup
4555
// This ensures we know the real paths and can validate against them later
4656
const resolved = await realpathWithRetry(absolute);
47-
const normalizedResolved = normalizePath(resolved);
57+
const normalizedResolved = normalizePathFormat(resolved);
4858
// Return both original and resolved paths if they differ
4959
// This allows matching against either /tmp or /private/tmp on macOS
5060
if (normalizedOriginal !== normalizedResolved) {

‎src/filesystem/roots-utils.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,7 @@
11
import { promises as fs, type Stats } from "fs";
22
import path from "path";
33
import os from "os";
4-
import {
5-
normalizePath,
6-
normalizePathFormat,
7-
realpathWithRetry,
8-
} from "./path-utils.js";
4+
import { normalizePathFormat, realpathWithRetry } from "./path-utils.js";
95
import type { Root } from "@modelcontextprotocol/server";
106
import { fileURLToPath } from "url";
117

@@ -36,10 +32,11 @@ async function parseRootUri(rootUri: string): Promise<RootPaths | null> {
3632
const absolutePath = path.resolve(expandedPath);
3733
const resolvedPath = await realpathWithRetry(absolutePath);
3834
return {
39-
// normalizePathFormat, not normalizePath: trimming whitespace or quotes
40-
// from the client's spelling would name a different directory.
35+
// normalizePathFormat, not normalizePath, for both: trimming whitespace
36+
// or quotes from the client's spelling or from the realpath would name
37+
// a different directory, and allow it (#5183, #5320).
4138
advertised: normalizePathFormat(absolutePath),
42-
resolved: normalizePath(resolvedPath),
39+
resolved: normalizePathFormat(resolvedPath),
4340
};
4441
} catch {
4542
return null; // Path doesn't exist or other error

0 commit comments

Comments
 (0)