diff --git a/.changeset/filesystem-dangling-symlink-message.md b/.changeset/filesystem-dangling-symlink-message.md new file mode 100644 index 0000000000..4a2439496e --- /dev/null +++ b/.changeset/filesystem-dangling-symlink-message.md @@ -0,0 +1,5 @@ +--- +"@modelcontextprotocol/server-filesystem": patch +--- + +Report a dangling symlink as a broken symlink. A request through a symlink whose target does not exist is still refused, but the error now names the symlink instead of saying its parent directory, which exists, does not. diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index 15ee98abf1..b1e720ec86 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -402,6 +402,24 @@ describe("Lib Functions", () => { ).rejects.toThrow("EACCES lstat"); }); + // #5191: only an ENOENT from an existing entry's realpath means a + // broken symlink; any other failure is reported as it is. + it("reports a realpath failure other than ENOENT on an existing component as is", async () => { + const base = + process.platform === "win32" ? "C:\\Users\\test" : "/home/user"; + const enoentError = new Error("ENOENT") as NodeJS.ErrnoException; + enoentError.code = "ENOENT"; + mockFs.realpath + .mockRejectedValueOnce(enoentError) + .mockResolvedValueOnce(base) + .mockRejectedValueOnce(new Error("EACCES realpath")); + mockReaddirNames.mockResolvedValueOnce(["link"]); + + await expect( + validatePath(path.join(base, "link", "a"), allowedDirs), + ).rejects.toThrow("EACCES realpath"); + }); + it("rejects when no ancestor directory exists at all", async () => { const newFilePath = process.platform === "win32" diff --git a/src/filesystem/__tests__/server-paths.test.ts b/src/filesystem/__tests__/server-paths.test.ts index 803cfb41a6..d24ee98076 100644 --- a/src/filesystem/__tests__/server-paths.test.ts +++ b/src/filesystem/__tests__/server-paths.test.ts @@ -314,7 +314,7 @@ describe("symlinks", () => { ).toBe("x"); }); - it("refuses to write under a respelled dangling symlink, reporting a missing parent", async () => { + it("refuses to write under a respelled dangling symlink, naming it as broken (#5191)", async () => { await fs.symlink( path.join(outside, "not-yet"), path.join(dir, "dangling"), @@ -324,24 +324,51 @@ describe("symlinks", () => { content: "x", }); expect(textOf(result)).toBe( - `Parent directory does not exist: ${path.join(dir, "DANGLING")}`, + `Broken symlink: ${path.join(dir, "DANGLING")} points to a target that does not exist`, ); await expect(fs.stat(path.join(outside, "not-yet"))).rejects.toThrow(); }); }, ); - it("refuses to write through a dangling symlink, reporting a missing parent", async () => { + // #5191: the refusal is right, and the message names the symlink rather + // than reporting its parent, which exists, as missing. + it("refuses to write through a dangling symlink, naming it as broken (#5191)", 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} points to a target that does not exist`, + ); await expect(fs.stat(path.join(outside, "not-yet"))).rejects.toThrow(); }); + it("refuses to read a dangling symlink, naming it as broken (#5191)", async () => { + const link = path.join(dir, "dangling"); + await fs.symlink(path.join(dir, "gone.txt"), link); + const result = await read(link); + expect(result.isError).toBe(true); + expect(textOf(result)).toBe( + `Broken symlink: ${link} points to a target that does not exist`, + ); + }); + + it("names a dangling symlink in the middle of a path as broken (#5191)", async () => { + const link = path.join(dir, "dangling-dir"); + await fs.symlink(path.join(dir, "gone"), link); + const result = await call(client, "write_file", { + path: path.join(link, "f.txt"), + content: "x", + }); + expect(textOf(result)).toBe( + `Broken symlink: ${link} points to a target that does not exist`, + ); + await expect(fs.stat(path.join(dir, "gone"))).rejects.toThrow(); + }); + it("reports a realpath failure other than ENOENT as is", async () => { await fs.writeFile(path.join(dir, "file.txt"), ""); const result = await read(path.join(dir, "file.txt", "child")); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 775145e708..87f4aeca57 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -429,7 +429,21 @@ async function resolveUnicodeEquivalentPath( return path.join(currentPath, ...relativeParts.slice(index)); } - currentPath = await realpathWithTimeout(candidate); + try { + currentPath = await realpathWithTimeout(candidate); + } catch (error) { + // The entry exists (lstat found it just above) but realpath cannot + // follow it, so it is a symlink whose target is missing. Name it: a bare + // ENOENT reaches validatePath's catch, which would report its parent, + // a directory that plainly exists, as missing (#5191). + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + throw new Error( + `Broken symlink: ${candidate} points to a target that does not exist`, + { cause: error }, + ); + } + throw error; + } if ( !isPathWithinAllowedDirectories( normalizePathFormat(currentPath),