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),