Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/brave-pianos-listen.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@modelcontextprotocol/server-filesystem": patch
---

Name a broken symlink in the error message instead of reporting its parent directory as missing.
57 changes: 57 additions & 0 deletions src/filesystem/__tests__/broken-symlink.test.ts
Original file line number Diff line number Diff line change
@@ -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"),
);
});
});
4 changes: 2 additions & 2 deletions src/filesystem/__tests__/server-paths.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});

Expand Down
16 changes: 13 additions & 3 deletions src/filesystem/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Loading