From d98c8566ef278a6caf802b7db8f647ca1e377981 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 27 Oct 2026 01:24:32 -0400 Subject: [PATCH 1/3] fix(filesystem): allow a client Root in its advertised form as well as its realpath parseRootUri kept only the realpath of each client Root, so a request that used the Root's own spelling (a symlinked directory, /tmp on macOS) was refused. It now returns both forms and getValidRootDirectories admits both, deduplicated, as resolveAllowedDirectories does for command-line directories. validatePath still realpaths every request, so a symlink inside a Root that points outside it stays refused. Closes #5183 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../5183-filesystem-root-advertised-form.md | 5 ++ src/filesystem/README.md | 4 +- src/filesystem/__tests__/server-roots.test.ts | 81 ++++++++++++++++++- src/filesystem/roots-utils.ts | 36 ++++++--- 4 files changed, 113 insertions(+), 13 deletions(-) create mode 100644 .changeset/5183-filesystem-root-advertised-form.md diff --git a/.changeset/5183-filesystem-root-advertised-form.md b/.changeset/5183-filesystem-root-advertised-form.md new file mode 100644 index 0000000000..da4d51cd72 --- /dev/null +++ b/.changeset/5183-filesystem-root-advertised-form.md @@ -0,0 +1,5 @@ +--- +"@modelcontextprotocol/server-filesystem": patch +--- + +A client Root is now allowed in the form the client advertised it as well as in its resolved form, as command-line directories already were. A request that names a path through a symlinked Root's own spelling (for example `/tmp/...` for a Root of `file:///tmp/...` on macOS) is no longer refused, and `list_allowed_directories` lists both forms. Every request is still resolved and checked against the resolved directory, so a symlink inside a Root that points outside it stays refused. diff --git a/src/filesystem/README.md b/src/filesystem/README.md index 23b39a7f61..9400f40ab2 100644 --- a/src/filesystem/README.md +++ b/src/filesystem/README.md @@ -31,6 +31,8 @@ A client that connects with a 2025-era protocol version (the `initialize` handsh Roots notified by Client to Server, completely replace any server-side Allowed directories when provided. +A Root reached through a symlink is allowed in both its advertised form and its resolved form, as a command-line directory is: a Root of `file:///tmp/project` on macOS allows `/tmp/project` and `/private/tmp/project`. Every requested path is still resolved and checked against the resolved directories, so a symlink inside a Root that points outside it is refused. + **Important**: If server starts without command-line arguments AND a 2025-era client doesn't support roots protocol, the server fails at initialization: it logs the reason to stderr, closes the connection and exits with status 1. A client that supports roots but provides none (or none that are valid) keeps the connection, with no allowed directories until it sends `roots/list_changed` with valid roots. The 2026-07-28 protocol has no `initialize` handshake and no server-to-client `roots/list` request, so a 2026-07-28 client's roots are never asked for: the command-line directories are the whole allow-list. A server started with none answers every tool call on such a connection with an error saying so. @@ -205,7 +207,7 @@ The server's directory access control follows this flow: - List all directories the server is allowed to access - No input required - Returns: - - Directories that this server can read/write from + - Directories that this server can read/write from. A directory reached through a symlink is listed twice, in the form it was given and in its resolved form ### Slow and very large trees diff --git a/src/filesystem/__tests__/server-roots.test.ts b/src/filesystem/__tests__/server-roots.test.ts index 30cddff36a..48c5f00d20 100644 --- a/src/filesystem/__tests__/server-roots.test.ts +++ b/src/filesystem/__tests__/server-roots.test.ts @@ -84,15 +84,92 @@ describe("initial roots", () => { ); }); - it("resolve a symlinked root to its target", async () => { + // #5183: a Root is allowed in its advertised spelling as well as its + // realpath, as a command-line directory is, so a request that uses the + // Root's own spelling is not refused. + it("admit a symlinked root in both its advertised and its resolved form (#5183)", async () => { const link = path.join(cliDir, "link-to-root"); await fs.symlink(rootDir, link); + await fs.writeFile(path.join(rootDir, "note.txt"), "inside the root"); const { client } = await connectTracked([], { capabilities: ROOTS, listRoots: () => [rootOf(link)], }); await vi.waitFor(async () => - expect(await allowedDirectoriesOf(client)).toEqual([rootDir]), + expect(await allowedDirectoriesOf(client)).toEqual([link, rootDir]), + ); + const viaLink = await call(client, "read_text_file", { + path: path.join(link, "note.txt"), + }); + expect(textOf(viaLink)).toBe("inside the root"); + const viaTarget = await call(client, "read_text_file", { + path: path.join(rootDir, "note.txt"), + }); + expect(textOf(viaTarget)).toBe("inside the root"); + }); + + // #5183: admitting the advertised spelling does not widen access. Every + // request is still realpath'd and re-checked, so a symlink inside the Root + // that points outside it is refused under either spelling. + it("refuse a symlink inside a symlinked root that points outside it (#5183)", async () => { + const link = path.join(cliDir, "link-to-root"); + await fs.symlink(rootDir, link); + const secret = path.join(cliDir, "secret.txt"); + await fs.writeFile(secret, "outside the root"); + await fs.symlink(secret, path.join(rootDir, "escape.txt")); + const { client } = await connectTracked([], { + capabilities: ROOTS, + listRoots: () => [rootOf(link)], + }); + await vi.waitFor(async () => + expect(await allowedDirectoriesOf(client)).toEqual([link, rootDir]), + ); + for (const dir of [link, rootDir]) { + const result = await call(client, "read_text_file", { + path: path.join(dir, "escape.txt"), + }); + expect(textOf(result)).toMatch(/^Access denied/); + } + }); + + // #5183 as reported: on macOS /tmp is a symlink to /private/tmp, so a Root + // of file:///tmp/... was allowed only as /private/tmp/... and a request + // naming /tmp/... was refused. + it.runIf(process.platform === "darwin")( + "admit a macOS /tmp root under its /tmp spelling (#5183)", + async () => { + const tmpRoot = await fs.mkdtemp("/tmp/mcp-fs-5183-"); + try { + await fs.writeFile(path.join(tmpRoot, "note.txt"), "in /tmp"); + const { client } = await connectTracked([], { + capabilities: ROOTS, + listRoots: () => [rootOf(tmpRoot)], + }); + await vi.waitFor(async () => + expect(await allowedDirectoriesOf(client)).toEqual([ + tmpRoot, + await fs.realpath(tmpRoot), + ]), + ); + const result = await call(client, "read_text_file", { + path: path.join(tmpRoot, "note.txt"), + }); + expect(textOf(result)).toBe("in /tmp"); + } finally { + await fs.rm(tmpRoot, { recursive: true, force: true }); + } + }, + ); + + it("list a root once when it is advertised in both its forms (#5183)", async () => { + const link = path.join(cliDir, "link-to-root"); + await fs.symlink(rootDir, link); + const { client } = await connectTracked([], { + capabilities: ROOTS, + listRoots: () => [rootOf(link), rootOf(rootDir)], + }); + await vi.waitFor(async () => + expect(await allowedDirectoriesOf(client)).toEqual([link, rootDir]), ); }); diff --git a/src/filesystem/roots-utils.ts b/src/filesystem/roots-utils.ts index b7b8e7da60..51f07fa029 100644 --- a/src/filesystem/roots-utils.ts +++ b/src/filesystem/roots-utils.ts @@ -5,12 +5,22 @@ import { normalizePath, realpathWithRetry } from "./path-utils.js"; import type { Root } from "@modelcontextprotocol/server"; import { fileURLToPath } from "url"; +/** A root's two spellings: as the client advertised it, and its realpath. */ +interface RootPaths { + advertised: string; + resolved: string; +} + /** - * Converts a root URI to a normalized directory path with basic security validation. + * Converts a root URI to its normalized absolute path and its realpath. + * Both are kept, as resolveAllowedDirectories in index.ts keeps both forms of + * a command-line directory, so a request may use either spelling (#5183). + * validatePath still realpaths every request, so access stays bounded by the + * resolved directory. * @param rootUri - File URI (file://...) or plain directory path - * @returns Promise resolving to validated path or null if invalid + * @returns Promise resolving to both forms, or null if invalid */ -async function parseRootUri(rootUri: string): Promise { +async function parseRootUri(rootUri: string): Promise { try { const rawPath = rootUri.startsWith("file://") ? fileURLToPath(rootUri) @@ -21,7 +31,10 @@ async function parseRootUri(rootUri: string): Promise { : rawPath; const absolutePath = path.resolve(expandedPath); const resolvedPath = await realpathWithRetry(absolutePath); - return normalizePath(resolvedPath); + return { + advertised: normalizePath(absolutePath), + resolved: normalizePath(resolvedPath), + }; } catch { return null; // Path doesn't exist or other error } @@ -51,7 +64,8 @@ function formatDirectoryError( * * Converts root URI specifications (file:// URIs or plain paths) into normalized * directory paths, validating that each path exists and is a directory. - * Includes symlink resolution for security. + * Each root contributes its advertised form and, when it differs, its resolved + * (realpath) form, deduplicated across roots. * * @param requestedRoots - Array of root specifications with URI and optional name * @returns Promise resolving to array of validated directory paths @@ -59,11 +73,11 @@ function formatDirectoryError( export async function getValidRootDirectories( requestedRoots: readonly Root[], ): Promise { - const validatedDirectories: string[] = []; + const validatedDirectories = new Set(); for (const requestedRoot of requestedRoots) { - const resolvedPath = await parseRootUri(requestedRoot.uri); - if (!resolvedPath) { + const rootPaths = await parseRootUri(requestedRoot.uri); + if (!rootPaths) { console.error( formatDirectoryError( requestedRoot.uri, @@ -74,10 +88,12 @@ export async function getValidRootDirectories( continue; } + const { advertised, resolved: resolvedPath } = rootPaths; try { const stats: Stats = await fs.stat(resolvedPath); if (stats.isDirectory()) { - validatedDirectories.push(resolvedPath); + validatedDirectories.add(advertised); + validatedDirectories.add(resolvedPath); } else { console.error( formatDirectoryError(resolvedPath, undefined, "non-directory root"), @@ -88,5 +104,5 @@ export async function getValidRootDirectories( } } - return validatedDirectories; + return [...validatedDirectories]; } From e615660eaad45e835823c369600d3394c62b7ce2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 27 Oct 2026 01:39:17 -0400 Subject: [PATCH 2/3] fix(filesystem): keep a Root's advertised form verbatim, untrimmed normalizePath trims whitespace and quotes, which are legal POSIX filename characters, so a Root advertised as 'alias ' would have allowed an unrelated sibling 'alias'. Format the advertised form with normalizePathFormat instead, and pin it with a regression test. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/filesystem/__tests__/server-roots.test.ts | 26 +++++++++++++++++++ src/filesystem/roots-utils.ts | 10 +++++-- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/filesystem/__tests__/server-roots.test.ts b/src/filesystem/__tests__/server-roots.test.ts index 48c5f00d20..389b3623dd 100644 --- a/src/filesystem/__tests__/server-roots.test.ts +++ b/src/filesystem/__tests__/server-roots.test.ts @@ -132,6 +132,32 @@ describe("initial roots", () => { } }); + // #5183: the advertised form is kept as the client spelled it. Trimming + // whitespace or quotes from it would name a different directory, here an + // unrelated sibling, and allow it. POSIX only: Windows filenames cannot end + // in a space. + it.skipIf(process.platform === "win32")( + "keep a root's advertised form verbatim, not trimmed to a sibling's name (#5183)", + async () => { + const link = path.join(cliDir, "alias "); + await fs.symlink(rootDir, link); + const sibling = path.join(cliDir, "alias"); + await fs.mkdir(sibling); + await fs.writeFile(path.join(sibling, "secret.txt"), "not in the root"); + const { client } = await connectTracked([], { + capabilities: ROOTS, + listRoots: () => [rootOf(link)], + }); + await vi.waitFor(async () => + expect(await allowedDirectoriesOf(client)).toEqual([link, rootDir]), + ); + const result = await call(client, "read_text_file", { + path: path.join(sibling, "secret.txt"), + }); + expect(textOf(result)).toMatch(/^Access denied/); + }, + ); + // #5183 as reported: on macOS /tmp is a symlink to /private/tmp, so a Root // of file:///tmp/... was allowed only as /private/tmp/... and a request // naming /tmp/... was refused. diff --git a/src/filesystem/roots-utils.ts b/src/filesystem/roots-utils.ts index 51f07fa029..9b2b7a14ff 100644 --- a/src/filesystem/roots-utils.ts +++ b/src/filesystem/roots-utils.ts @@ -1,7 +1,11 @@ import { promises as fs, type Stats } from "fs"; import path from "path"; import os from "os"; -import { normalizePath, realpathWithRetry } from "./path-utils.js"; +import { + normalizePath, + normalizePathFormat, + realpathWithRetry, +} from "./path-utils.js"; import type { Root } from "@modelcontextprotocol/server"; import { fileURLToPath } from "url"; @@ -32,7 +36,9 @@ async function parseRootUri(rootUri: string): Promise { const absolutePath = path.resolve(expandedPath); const resolvedPath = await realpathWithRetry(absolutePath); return { - advertised: normalizePath(absolutePath), + // normalizePathFormat, not normalizePath: trimming whitespace or quotes + // from the client's spelling would name a different directory. + advertised: normalizePathFormat(absolutePath), resolved: normalizePath(resolvedPath), }; } catch { From b7471abf632062f68ae5a7120de4d2c864ed113a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 27 Oct 2026 02:04:23 -0400 Subject: [PATCH 3/3] fix(filesystem): check a retargeted allowed-directory symlink before creating under it A Root's advertised form can be a symlink, which can be retargeted after the Roots load. resolveUnicodeEquivalentPath started its walk for a missing path from the allowed directory's realpath without checking it, so write_file or create_directory under a retargeted alias landed outside every allowed directory. Check that starting realpath like each component below it. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/filesystem/__tests__/server-roots.test.ts | 30 +++++++++++++++++++ src/filesystem/lib.ts | 13 ++++++++ 2 files changed, 43 insertions(+) diff --git a/src/filesystem/__tests__/server-roots.test.ts b/src/filesystem/__tests__/server-roots.test.ts index 389b3623dd..355231a115 100644 --- a/src/filesystem/__tests__/server-roots.test.ts +++ b/src/filesystem/__tests__/server-roots.test.ts @@ -132,6 +132,36 @@ describe("initial roots", () => { } }); + // #5183: the advertised form names a symlink, which can be retargeted after + // the Roots load. A new file or directory under it must still be refused + // when the symlink now leads outside every allowed directory. + it("refuse creating under a root alias retargeted outside the root (#5183)", async () => { + const link = path.join(cliDir, "link-to-root"); + await fs.symlink(rootDir, link); + const { client } = await connectTracked([], { + capabilities: ROOTS, + listRoots: () => [rootOf(link)], + }); + await vi.waitFor(async () => + expect(await allowedDirectoriesOf(client)).toEqual([link, rootDir]), + ); + const outside = path.join(cliDir, "outside"); + await fs.mkdir(outside); + await fs.rm(link); + await fs.symlink(outside, link); + + const written = await call(client, "write_file", { + path: path.join(link, "new.txt"), + content: "x", + }); + expect(textOf(written)).toMatch(/^Access denied/); + const created = await call(client, "create_directory", { + path: path.join(link, "a", "b"), + }); + expect(textOf(created)).toMatch(/^Access denied/); + expect(await fs.readdir(outside)).toEqual([]); + }); + // #5183: the advertised form is kept as the client spelled it. Trimming // whitespace or quotes from it would name a different directory, here an // unrelated sibling, and allow it. POSIX only: Windows filenames cannot end diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index f40e308a4c..75915768ec 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -376,6 +376,19 @@ async function resolveUnicodeEquivalentPath( } let currentPath = await realpathWithTimeout(allowedDirectory); + // An allowed directory can be a symlink (the advertised form of a Root or a + // command-line directory), retargeted since it was allowed, so its realpath + // is checked like every component below it (#5183). + if ( + !isPathWithinAllowedDirectories( + normalizePathFormat(currentPath), + allowedDirectories, + ) + ) { + throw new Error( + `Access denied - symlink target outside allowed directories: ${currentPath} not in ${allowedDirectories.join(", ")}`, + ); + } const relativeParts = path .relative(allowedDirectory, absolutePath) .split(path.sep)