From 9b7b641a6af842038e134ad7af6691548358cef0 Mon Sep 17 00:00:00 2001 From: Dmitry Voropaev Date: Mon, 12 Oct 2026 13:21:43 +0300 Subject: [PATCH] fix(filesystem): name a broken symlink instead of blaming its parent validatePath on a symlink with nothing at the other end reported "Parent directory does not exist: " for a directory that is plainly there. resolveUnicodeEquivalentPath lets the ENOENT from fs.realpath escape bare, and validatePath's catch reads any ENOENT as a missing parent. Naming the symlink needs the catch inside the equivalent-path walk, where the failing entry is known. Only ENOENT is renamed; the realpath error is attached as the cause, the way the Parent directory error below it already does. The characterization test for this case asserted the old message. Its name and assertion now follow the fix; the security assertion in it, that nothing is created outside the allowed directory, is unchanged. Added a guard that a symlink cycle still reports ELOOP, so a later, broader catch cannot rename every realpath failure into a broken symlink. Signed-off-by: Dmitry Voropaev --- .changeset/brave-pianos-listen.md | 5 ++ .../__tests__/broken-symlink.test.ts | 57 +++++++++++++++++++ src/filesystem/__tests__/server-paths.test.ts | 4 +- src/filesystem/lib.ts | 16 +++++- 4 files changed, 77 insertions(+), 5 deletions(-) create mode 100644 .changeset/brave-pianos-listen.md create mode 100644 src/filesystem/__tests__/broken-symlink.test.ts diff --git a/.changeset/brave-pianos-listen.md b/.changeset/brave-pianos-listen.md new file mode 100644 index 0000000000..5e44958427 --- /dev/null +++ b/.changeset/brave-pianos-listen.md @@ -0,0 +1,5 @@ +--- +"@modelcontextprotocol/server-filesystem": patch +--- + +Name a broken symlink in the error message instead of reporting its parent directory as missing. diff --git a/src/filesystem/__tests__/broken-symlink.test.ts b/src/filesystem/__tests__/broken-symlink.test.ts new file mode 100644 index 0000000000..2084aaf988 --- /dev/null +++ b/src/filesystem/__tests__/broken-symlink.test.ts @@ -0,0 +1,57 @@ +import fs from "fs/promises"; +import os from "os"; +import path from "path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { validatePath } from "../lib.js"; + +describe("a symlink with no target", () => { + let testDirectory: string; + let allowedDirs: string[]; + + beforeEach(async () => { + testDirectory = await fs.mkdtemp( + path.join(os.tmpdir(), "mcp-broken-symlink-"), + ); + allowedDirs = [testDirectory]; + }); + + afterEach(async () => { + await fs.rm(testDirectory, { recursive: true, force: true }); + }); + + it("is named as such, not as a missing parent directory", async () => { + const link = path.join(testDirectory, "link.txt"); + await fs.symlink(path.join(testDirectory, "gone.txt"), link); + + await expect(validatePath(link, allowedDirs)).rejects.toThrow( + /Broken symlink/, + ); + // the parent is right there, so the old message pointed at the wrong thing + await expect(fs.stat(testDirectory)).resolves.toBeDefined(); + }); + + // Windows needs a privilege to create a symlink at all, and a self-referencing + // one is not reachable there the same way. + it.skipIf(process.platform === "win32")( + "leaves a realpath failure other than ENOENT alone", + async () => { + const loop = path.join(testDirectory, "loop"); + await fs.symlink(loop, loop); + + // A cycle fails realpath with ELOOP, not ENOENT, so validatePath throws + // before the equivalent-path walk runs at all. The guard is against a + // broader catch there that would rename every realpath failure. + await expect(validatePath(loop, allowedDirs)).rejects.toThrow(/^ELOOP/); + }, + ); + + it("does not change what a genuinely missing path reports", async () => { + const missing = path.join(testDirectory, "nope", "file.txt"); + + // resolveUnicodeEquivalentPath returns the tail so create_directory can + // mkdir -p it; that is unrelated to the broken-symlink case. + await expect(validatePath(missing, allowedDirs)).resolves.toBe( + path.join(await fs.realpath(testDirectory), "nope", "file.txt"), + ); + }); +}); diff --git a/src/filesystem/__tests__/server-paths.test.ts b/src/filesystem/__tests__/server-paths.test.ts index d6a8995317..70970f518c 100644 --- a/src/filesystem/__tests__/server-paths.test.ts +++ b/src/filesystem/__tests__/server-paths.test.ts @@ -199,14 +199,14 @@ describe("symlinks", () => { expect(await fs.readdir(outside)).toEqual(["secret.txt"]); }); - it("refuses to write through a dangling symlink, reporting a missing parent", async () => { + it("refuses to write through a dangling symlink, naming the symlink", async () => { const link = path.join(dir, "dangling"); await fs.symlink(path.join(outside, "not-yet"), link); const result = await call(client, "write_file", { path: link, content: "x", }); - expect(textOf(result)).toBe(`Parent directory does not exist: ${dir}`); + expect(textOf(result)).toBe(`Broken symlink: ${link}`); await expect(fs.stat(path.join(outside, "not-yet"))).rejects.toThrow(); }); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index abb6b09cd4..3140206eae 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -175,9 +175,19 @@ async function resolveUnicodeEquivalentPath( return path.join(currentPath, ...relativeParts.slice(index)); } - currentPath = await fs.realpath( - path.join(currentPath, equivalentMatches[0]), - ); + const matchedPath = path.join(currentPath, equivalentMatches[0]); + try { + currentPath = await fs.realpath(matchedPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + // The entry is in the directory listing but realpath cannot follow it, + // so it is a symlink with nothing at the other end. Name that. Left as + // a bare ENOENT it reaches validatePath's catch, which reports a + // missing parent directory for a directory that is plainly there. + throw new Error(`Broken symlink: ${matchedPath}`, { cause: error }); + } + throw error; + } if ( !isPathWithinAllowedDirectories( normalizePath(currentPath),