diff --git a/.changeset/filesystem-anchor-reads.md b/.changeset/filesystem-anchor-reads.md new file mode 100644 index 0000000000..150430718b --- /dev/null +++ b/.changeset/filesystem-anchor-reads.md @@ -0,0 +1,5 @@ +--- +"@modelcontextprotocol/server-filesystem": patch +--- + +Anchor read tools to the validated file, as write tools are, and pin the source of a same-device `move_file` to the entry that was validated. A FIFO, socket or device is refused rather than read. diff --git a/src/filesystem/README.md b/src/filesystem/README.md index 61cc4bdfa5..829cc9d03c 100644 --- a/src/filesystem/README.md +++ b/src/filesystem/README.md @@ -153,12 +153,14 @@ Files inside the allowed directories are also [resources](https://modelcontextpr - `tail` (non-negative integer, optional): Last N lines; `0` returns no lines - Always treats the file as UTF-8 text regardless of extension - Cannot specify both `head` and `tail` simultaneously + - Reads only a regular file: a FIFO, socket or device is refused rather + than read, as it is by every tool and by `resources/read` - **read_media_file** - Read a file and return it as a base64-encoded content block with its MIME type - Inputs: - `path` (string) - - Streams the file and returns base64 data with the corresponding MIME type. Image and + - Reads the file and returns base64 data with the corresponding MIME type. Image and audio files are returned as `image`/`audio` content; any other file type is returned as an embedded `resource` (a valid MCP content block for arbitrary binary data) diff --git a/src/filesystem/__tests__/file-resources.test.ts b/src/filesystem/__tests__/file-resources.test.ts index 76bf103dcc..589ce761a1 100644 --- a/src/filesystem/__tests__/file-resources.test.ts +++ b/src/filesystem/__tests__/file-resources.test.ts @@ -5,6 +5,7 @@ // the roots barrier on 2025-11-25, the no-directories error on 2026-07-28). import fs from "fs/promises"; +import fsSync from "fs"; import path from "path"; import { pathToFileURL } from "url"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -216,9 +217,14 @@ describe("resources/read", () => { const file = path.join(allowed, "locked.txt"); await fs.writeFile(file, "x"); const client = await connectTo([allowed]); - vi.spyOn(fs, "readFile").mockRejectedValueOnce( - Object.assign(new Error("permission denied"), { code: "EACCES" }), - ); + // The read goes through the descriptor the anchored open returned. + vi.spyOn(fsSync, "readFile").mockImplementationOnce((( + _fd: number, + callback: (error: Error) => void, + ) => + callback( + Object.assign(new Error("permission denied"), { code: "EACCES" }), + )) as typeof fsSync.readFile); const error = await readError(client, uriOf(file)); expect(error.code).toBe(-32603); expect(error.message).toContain("permission denied"); diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index ccc484b0fa..0f8317dd85 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -7,7 +7,7 @@ import { vi, type Mock, } from "vitest"; -import fs, { type FileHandle } from "fs/promises"; +import fs from "fs/promises"; import type { BigIntStats, Dirent, PathLike, Stats } from "fs"; import path from "path"; import { @@ -32,6 +32,7 @@ import { headFile, } from "../lib.js"; import * as pinned from "../pinned-directory.js"; +import type { ReadableFile } from "../pinned-directory.js"; // Mock fs module vi.mock("fs/promises"); @@ -39,6 +40,9 @@ vi.mock("fs/promises"); // covers them, so here only what lib.ts hands them is checked. vi.mock("../pinned-directory.js"); const mockPinned = vi.mocked(pinned); +// What the anchored read (readFileInDirectory) returns for the file's text. +const mockReadFile = (text: string) => + mockPinned.readFileInDirectory.mockResolvedValue(Buffer.from(text)); const mockFs = vi.mocked(fs); // fs.readdir is overloaded and vi.mocked types the mock against its last // (Buffer) overload; the code under test calls the withFileTypes overload. @@ -68,12 +72,10 @@ interface MockFileHandle { close: Mock; } -function asFileHandle(handle: MockFileHandle): FileHandle { - // Double cast is safe here: tailFile/headFile only ever call read() and - // close() on the handle, which this test double provides; FileHandle's - // overloaded read() signatures cannot be matched by a vi.fn() mock, so no - // single cast or structural type relates the two. - return handle as unknown as FileHandle; +function asFileHandle(handle: MockFileHandle, size: number = 0): ReadableFile { + // What openFileForReading hands tailFile/headFile: the opened file's size, + // and read() and close() on its descriptor. + return { size, read: handle.read, close: handle.close }; } function createMockFileHandle(content: Buffer) { @@ -566,7 +568,7 @@ describe("Lib Functions", () => { nlink: 2, }; - mockFs.stat.mockResolvedValueOnce(mockStats as Stats); + mockPinned.statInDirectory.mockReturnValueOnce(mockStats as Stats); const result = await getFileStats("/test/file.txt"); @@ -593,7 +595,7 @@ describe("Lib Functions", () => { mode: 0o755, }; - mockFs.stat.mockResolvedValueOnce(mockStats as Stats); + mockPinned.statInDirectory.mockReturnValueOnce(mockStats as Stats); const result = await getFileStats("/test/dir"); @@ -605,21 +607,26 @@ describe("Lib Functions", () => { describe("readFileContent", () => { it("reads file with default encoding", async () => { - mockFs.readFile.mockResolvedValueOnce("file content"); + mockPinned.readFileInDirectory.mockResolvedValueOnce( + Buffer.from("file content"), + ); const result = await readFileContent("/test/file.txt"); expect(result).toBe("file content"); - expect(mockFs.readFile).toHaveBeenCalledWith("/test/file.txt", "utf-8"); + expect(mockPinned.readFileInDirectory).toHaveBeenCalledWith( + "/test/file.txt", + ); }); it("reads file with custom encoding", async () => { - mockFs.readFile.mockResolvedValueOnce("file content"); + mockPinned.readFileInDirectory.mockResolvedValueOnce( + Buffer.from("file content"), + ); - const result = await readFileContent("/test/file.txt", "ascii"); + const result = await readFileContent("/test/file.txt", "base64"); - expect(result).toBe("file content"); - expect(mockFs.readFile).toHaveBeenCalledWith("/test/file.txt", "ascii"); + expect(result).toBe(Buffer.from("file content").toString("base64")); }); }); @@ -873,7 +880,7 @@ describe("Lib Functions", () => { describe("File Editing Functions", () => { describe("applyFileEdits", () => { beforeEach(() => { - mockFs.readFile.mockResolvedValue("line1\nline2\nline3\n"); + mockReadFile("line1\nline2\nline3\n"); mockFs.lstat.mockResolvedValue(REGULAR_FILE); }); @@ -937,7 +944,7 @@ describe("Lib Functions", () => { }); it("handles whitespace-flexible matching", async () => { - mockFs.readFile.mockResolvedValue(" line1\n line2\n line3\n"); + mockReadFile(" line1\n line2\n line3\n"); const edits = [{ oldText: "line2", newText: "modified line2" }]; @@ -982,7 +989,7 @@ describe("Lib Functions", () => { }); it("handles complex multi-line edits with indentation", async () => { - mockFs.readFile.mockResolvedValue( + mockReadFile( 'function test() {\n console.log("hello");\n return true;\n}', ); @@ -1004,9 +1011,7 @@ describe("Lib Functions", () => { }); it("handles edits with different indentation patterns", async () => { - mockFs.readFile.mockResolvedValue( - " if (condition) {\n doSomething();\n }", - ); + mockReadFile(" if (condition) {\n doSomething();\n }"); const edits = [ { @@ -1025,7 +1030,7 @@ describe("Lib Functions", () => { }); it("handles CRLF line endings in file content", async () => { - mockFs.readFile.mockResolvedValue("line1\r\nline2\r\nline3\r\n"); + mockReadFile("line1\r\nline2\r\nline3\r\n"); const edits = [{ oldText: "line2", newText: "modified line2" }]; @@ -1041,17 +1046,19 @@ describe("Lib Functions", () => { describe("tailFile", () => { it("handles empty files", async () => { - mockFs.stat.mockResolvedValue({ size: 0 } as Stats); + const mockFileHandle = createMockFileHandle(Buffer.alloc(0)); + mockPinned.openFileForReading.mockResolvedValue( + asFileHandle(mockFileHandle, 0), + ); const result = await tailFile("/test/empty.txt", 5); expect(result).toBe(""); - expect(mockFs.open).not.toHaveBeenCalled(); + expect(mockFileHandle.read).not.toHaveBeenCalled(); + expect(mockFileHandle.close).toHaveBeenCalled(); }); - it("calls stat to check file size", async () => { - mockFs.stat.mockResolvedValue({ size: 100 } as Stats); - + it("opens the file through its pinned directory", async () => { // Mock file handle with proper typing const mockFileHandle = { read: vi.fn(), @@ -1061,17 +1068,18 @@ describe("Lib Functions", () => { mockFileHandle.read.mockResolvedValue({ bytesRead: 0 }); mockFileHandle.close.mockResolvedValue(undefined); - mockFs.open.mockResolvedValue(asFileHandle(mockFileHandle)); + mockPinned.openFileForReading.mockResolvedValue( + asFileHandle(mockFileHandle, 100), + ); await tailFile("/test/file.txt", 2); - expect(mockFs.stat).toHaveBeenCalledWith("/test/file.txt"); - expect(mockFs.open).toHaveBeenCalledWith("/test/file.txt", "r"); + expect(mockPinned.openFileForReading).toHaveBeenCalledWith( + "/test/file.txt", + ); }); it("handles files with content and returns last lines", async () => { - mockFs.stat.mockResolvedValue({ size: 50 } as Stats); - const mockFileHandle = { read: vi.fn(), close: vi.fn(), @@ -1086,7 +1094,9 @@ describe("Lib Functions", () => { .mockResolvedValueOnce({ bytesRead: 0 }); mockFileHandle.close.mockResolvedValue(undefined); - mockFs.open.mockResolvedValue(asFileHandle(mockFileHandle)); + mockPinned.openFileForReading.mockResolvedValue( + asFileHandle(mockFileHandle, 50), + ); await tailFile("/test/file.txt", 2); @@ -1101,9 +1111,9 @@ describe("Lib Functions", () => { Buffer.from("\nlast"), ]); const mockFileHandle = createMockFileHandle(content); - - mockFs.stat.mockResolvedValue({ size: content.length } as Stats); - mockFs.open.mockResolvedValue(asFileHandle(mockFileHandle)); + mockPinned.openFileForReading.mockResolvedValue( + asFileHandle(mockFileHandle, content.length), + ); const result = await tailFile("/test/file.txt", 2); @@ -1111,8 +1121,6 @@ describe("Lib Functions", () => { }); it("handles read errors gracefully", async () => { - mockFs.stat.mockResolvedValue({ size: 100 } as Stats); - const mockFileHandle = { read: vi.fn(), close: vi.fn(), @@ -1121,7 +1129,9 @@ describe("Lib Functions", () => { mockFileHandle.read.mockResolvedValue({ bytesRead: 0 }); mockFileHandle.close.mockResolvedValue(undefined); - mockFs.open.mockResolvedValue(asFileHandle(mockFileHandle)); + mockPinned.openFileForReading.mockResolvedValue( + asFileHandle(mockFileHandle, 100), + ); await tailFile("/test/file.txt", 5); @@ -1140,11 +1150,15 @@ describe("Lib Functions", () => { mockFileHandle.read.mockResolvedValue({ bytesRead: 0 }); mockFileHandle.close.mockResolvedValue(undefined); - mockFs.open.mockResolvedValue(asFileHandle(mockFileHandle)); + mockPinned.openFileForReading.mockResolvedValue( + asFileHandle(mockFileHandle), + ); await headFile("/test/file.txt", 2); - expect(mockFs.open).toHaveBeenCalledWith("/test/file.txt", "r"); + expect(mockPinned.openFileForReading).toHaveBeenCalledWith( + "/test/file.txt", + ); }); it("handles files with content and returns first lines", async () => { @@ -1162,7 +1176,9 @@ describe("Lib Functions", () => { .mockResolvedValueOnce({ bytesRead: 0 }); mockFileHandle.close.mockResolvedValue(undefined); - mockFs.open.mockResolvedValue(asFileHandle(mockFileHandle)); + mockPinned.openFileForReading.mockResolvedValue( + asFileHandle(mockFileHandle), + ); await headFile("/test/file.txt", 2); @@ -1176,7 +1192,9 @@ describe("Lib Functions", () => { ]); const mockFileHandle = createMockFileHandle(content); - mockFs.open.mockResolvedValue(asFileHandle(mockFileHandle)); + mockPinned.openFileForReading.mockResolvedValue( + asFileHandle(mockFileHandle), + ); const result = await headFile("/test/file.txt", 1); @@ -1198,7 +1216,9 @@ describe("Lib Functions", () => { .mockResolvedValueOnce({ bytesRead: 0 }); mockFileHandle.close.mockResolvedValue(undefined); - mockFs.open.mockResolvedValue(asFileHandle(mockFileHandle)); + mockPinned.openFileForReading.mockResolvedValue( + asFileHandle(mockFileHandle), + ); await headFile("/test/file.txt", 5); @@ -1220,7 +1240,9 @@ describe("Lib Functions", () => { .mockResolvedValueOnce({ bytesRead: 0 }); mockFileHandle.close.mockResolvedValue(undefined); - mockFs.open.mockResolvedValue(asFileHandle(mockFileHandle)); + mockPinned.openFileForReading.mockResolvedValue( + asFileHandle(mockFileHandle), + ); await headFile("/test/file.txt", 2); diff --git a/src/filesystem/__tests__/path-validation.test.ts b/src/filesystem/__tests__/path-validation.test.ts index a8b704e86d..51a1910145 100644 --- a/src/filesystem/__tests__/path-validation.test.ts +++ b/src/filesystem/__tests__/path-validation.test.ts @@ -2,7 +2,10 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest"; import * as path from "path"; import * as fs from "fs/promises"; import * as os from "os"; -import { isPathWithinAllowedDirectories } from "../path-validation.js"; +import { + isPathWithinAllowedDirectories, + isWindowsUncRoot, +} from "../path-validation.js"; /** * Check if the current environment supports symlink creation @@ -1381,3 +1384,25 @@ describe("Path Validation", () => { }); }); }); + +describe("isWindowsUncRoot", () => { + it.each([ + "\\\\server\\share", + "\\\\server\\share\\", + "\\\\?\\UNC\\server\\share", + "\\\\.\\UNC\\server\\share\\", + ])("is true for the share root %s", (root) => { + expect(isWindowsUncRoot(root)).toBe(true); + }); + + it.each([ + "\\\\server\\share\\f.txt", + "\\\\server", + "\\\\?\\UNC\\server", + "\\\\?\\C:\\", + "C:\\", + "/srv/share", + ])("is false for %s", (notRoot) => { + expect(isWindowsUncRoot(notRoot)).toBe(false); + }); +}); diff --git a/src/filesystem/__tests__/pinned-directory.test.ts b/src/filesystem/__tests__/pinned-directory.test.ts index 38c0e80b34..98928951ca 100644 --- a/src/filesystem/__tests__/pinned-directory.test.ts +++ b/src/filesystem/__tests__/pinned-directory.test.ts @@ -14,8 +14,11 @@ import { isChildDirectory, isSameDirectory, moveFileAcrossDevices, + openFileForReading, overwriteFileInDirectory, + readFileInDirectory, renameIntoDirectory, + statInDirectory, } from "../pinned-directory.js"; import { makeTempDir } from "./helpers.js"; @@ -428,6 +431,329 @@ describe("renameIntoDirectory", () => { expect(fs.existsSync(path.join(dir, "a.txt"))).toBe(true); }, ); + + const movingNames = (directory: string) => + fs.readdirSync(directory).filter((n) => n.endsWith(".moving")); + + // Stand in for renameSync, calling `around(call, from, to, real)` on each + // call; the first call is step 1 (inside the source's directory), the + // second step 2 (across), the third step 3 (to the destination's name). + const onRename = ( + around: ( + call: number, + from: fs.PathLike, + to: fs.PathLike, + real: typeof fs.renameSync, + ) => void, + ) => { + const realRename = fs.renameSync; + let call = 0; + vi.spyOn(fs, "renameSync").mockImplementation((from, to) => + around(++call, from, to, realRename), + ); + }; + + // The source's directory swapped for a link to an outside one between + // step 1 and step 2: step 2's path now leads outside, where nothing has + // the fresh name, so nothing comes in from there. + const swapSourceBeforeStep2 = (before?: (held: string) => void) => + onRename((call, from, to, real) => { + if (call === 2) { + real(path.join(dir, "sub"), path.join(dir, "sub-real")); + fs.symlinkSync(outside, path.join(dir, "sub")); + before?.(path.basename(String(from))); + } + real(from, to); + }); + + it.skipIf(!symlinks)( + "never brings in an outside entry when the source directory is swapped", + () => { + fs.mkdirSync(path.join(dir, "sub")); + fs.writeFileSync(path.join(dir, "sub", "x.conf"), "inside"); + fs.writeFileSync(path.join(outside, "x.conf"), "secret"); + swapSourceBeforeStep2(); + expect(() => + renameIntoDirectory( + path.join(dir, "sub", "x.conf"), + path.join(dir, "moved.conf"), + ), + ).toThrow("Access denied"); + // Nothing came in, and nothing left the directory that was moved away: + // the source stays there under its fresh name, since the swapped path + // is never entered to put it back. + expect(fs.readdirSync(outside)).toEqual(["x.conf"]); + expect(fs.readdirSync(dir).sort()).toEqual(["sub", "sub-real"]); + const [held] = movingNames(path.join(dir, "sub-real")); + expect(fs.readFileSync(path.join(dir, "sub-real", held), "utf-8")).toBe( + "inside", + ); + expect(process.cwd()).toBe(cwd); + }, + ); + + it("puts back a directory source, which cannot be linked, by renaming it", () => { + fs.mkdirSync(path.join(dir, "d")); + fs.mkdirSync(path.join(dir, "sub")); + onRename((call, from, to, real) => { + if (call === 2) { + throw errno("EXDEV"); + } + real(from, to); + }); + expect(() => + renameIntoDirectory(path.join(dir, "d"), path.join(dir, "sub", "e")), + ).toThrow(expect.objectContaining({ code: "EXDEV" })); + expect(fs.readdirSync(dir).sort()).toEqual(["d", "sub"]); + }); + + it.skipIf(!symlinks)( + "sends back an entry planted outside under the fresh name", + () => { + fs.mkdirSync(path.join(dir, "sub")); + fs.writeFileSync(path.join(dir, "sub", "x.conf"), "inside"); + let planted = ""; + swapSourceBeforeStep2((held) => { + planted = held; + fs.writeFileSync(path.join(outside, held), "secret"); + }); + expect(() => + renameIntoDirectory( + path.join(dir, "sub", "x.conf"), + path.join(dir, "moved.conf"), + ), + ).toThrow("Access denied"); + expect(fs.readFileSync(path.join(outside, planted), "utf-8")).toBe( + "secret", + ); + expect(movingNames(dir)).toEqual([]); + expect(movingNames(path.join(dir, "sub-real"))).toHaveLength(1); + }, + ); + + it("puts the source back, keeping the error's code, when step 2 fails", () => { + fs.mkdirSync(path.join(dir, "sub")); + fs.writeFileSync(path.join(dir, "a.txt"), "a"); + onRename((call, from, to, real) => { + if (call === 2) { + throw errno("EXDEV"); + } + real(from, to); + }); + expect(() => + renameIntoDirectory( + path.join(dir, "a.txt"), + path.join(dir, "sub", "b.txt"), + ), + ).toThrow(expect.objectContaining({ code: "EXDEV" })); + expect(fs.readFileSync(path.join(dir, "a.txt"), "utf-8")).toBe("a"); + expect(movingNames(dir)).toEqual([]); + }); + + it("puts the source back when the destination's name cannot be taken", () => { + fs.mkdirSync(path.join(dir, "sub")); + fs.writeFileSync(path.join(dir, "a.txt"), "a"); + onRename((call, from, to, real) => { + if (call === 3) { + throw errno("EISDIR"); + } + real(from, to); + }); + expect(() => + renameIntoDirectory( + path.join(dir, "a.txt"), + path.join(dir, "sub", "b.txt"), + ), + ).toThrow(expect.objectContaining({ code: "EISDIR" })); + expect(fs.readFileSync(path.join(dir, "a.txt"), "utf-8")).toBe("a"); + expect(movingNames(dir)).toEqual([]); + expect(fs.readdirSync(path.join(dir, "sub"))).toEqual([]); + }); + + it("says where the source was left when its name was taken meanwhile", () => { + fs.mkdirSync(path.join(dir, "sub")); + fs.writeFileSync(path.join(dir, "a.txt"), "a"); + onRename((call, from, to, real) => { + if (call === 3) { + fs.writeFileSync(path.join(dir, "a.txt"), "someone else's"); + throw errno("EISDIR"); + } + real(from, to); + }); + let error: unknown; + try { + renameIntoDirectory( + path.join(dir, "a.txt"), + path.join(dir, "sub", "b.txt"), + ); + } catch (caught) { + error = caught; + } + // Brought back into the source's directory, and checked, but its name + // was taken, so it stays there under the fresh name. + const [left] = movingNames(dir); + expect(error).toMatchObject({ + code: "EISDIR", + message: expect.stringContaining(`was left as ${left} in ${dir}`), + }); + expect(fs.readFileSync(path.join(dir, left), "utf-8")).toBe("a"); + expect(movingNames(path.join(dir, "sub"))).toEqual([]); + }); + + it.skipIf(!symlinks)( + "never restores an entry planted where the destination's directory was swapped", + () => { + fs.mkdirSync(path.join(dir, "sub")); + fs.writeFileSync(path.join(dir, "a.txt"), "a"); + let landing = ""; + onRename((call, from, to, real) => { + if (call === 3) { + landing = String(from); + real(path.join(dir, "sub"), path.join(dir, "sub-real")); + fs.symlinkSync(outside, path.join(dir, "sub")); + fs.writeFileSync(path.join(outside, landing), "secret"); + throw errno("EISDIR"); + } + real(from, to); + }); + let error: unknown; + try { + renameIntoDirectory( + path.join(dir, "a.txt"), + path.join(dir, "sub", "b.txt"), + ); + } catch (caught) { + error = caught; + } + expect(String(error)).toContain( + `was left as ${landing} in ${path.join(dir, "sub")}`, + ); + expect(fs.existsSync(path.join(dir, "a.txt"))).toBe(false); + expect(movingNames(dir)).toEqual([]); + expect(fs.readFileSync(path.join(outside, landing), "utf-8")).toBe( + "secret", + ); + expect( + fs.readFileSync(path.join(dir, "sub-real", landing), "utf-8"), + ).toBe("a"); + }, + ); + + it("reports the source left when the destination's entry cannot be brought back", () => { + fs.mkdirSync(path.join(dir, "sub")); + fs.writeFileSync(path.join(dir, "a.txt"), "a"); + onRename((call, from, to, real) => { + if (call === 3 || call === 4) { + throw errno(call === 3 ? "EISDIR" : "EIO"); + } + real(from, to); + }); + expect(() => + renameIntoDirectory( + path.join(dir, "a.txt"), + path.join(dir, "sub", "b.txt"), + ), + ).toThrow(/was left as \.mcp-[0-9a-f]{16}\.moving in /); + expect(movingNames(path.join(dir, "sub"))).toHaveLength(1); + }); + + it("drops EXDEV when the source cannot be put back, so no copy follows", () => { + fs.mkdirSync(path.join(dir, "sub")); + fs.writeFileSync(path.join(dir, "a.txt"), "a"); + onRename((call, from, to, real) => { + if (call === 2) { + fs.writeFileSync(path.join(dir, "a.txt"), "someone else's"); + throw errno("EXDEV"); + } + real(from, to); + }); + let error: unknown; + try { + renameIntoDirectory( + path.join(dir, "a.txt"), + path.join(dir, "sub", "b.txt"), + ); + } catch (caught) { + error = caught; + } + expect((error as NodeJS.ErrnoException).code).toBeUndefined(); + expect((error as Error).cause).toMatchObject({ code: "EXDEV" }); + expect((error as Error).message).toContain("was left as"); + }); + + it("says where a directory source was left when its name was taken", () => { + fs.mkdirSync(path.join(dir, "d")); + fs.mkdirSync(path.join(dir, "sub")); + onRename((call, from, to, real) => { + if (call === 2) { + fs.writeFileSync(path.join(dir, "d"), "someone else's"); + throw errno("EIO"); + } + real(from, to); + }); + expect(() => + renameIntoDirectory(path.join(dir, "d"), path.join(dir, "sub", "e")), + ).toThrow(/was left as \.mcp-[0-9a-f]{16}\.moving in /); + expect(movingNames(dir)).toHaveLength(1); + }); + + it("leaves the source staged when its directory cannot be checked", () => { + fs.mkdirSync(path.join(dir, "d")); + fs.mkdirSync(path.join(dir, "sub")); + const realLstat = fs.lstatSync; + onRename((call, from, to, real) => { + if (call === 2) { + vi.spyOn(fs, "lstatSync").mockImplementation((( + p: fs.PathLike, + options?: fs.StatSyncOptions, + ) => { + if (p === "d") { + throw errno("EACCES"); + } + return realLstat(p, options); + }) as never); + throw errno("EIO"); + } + real(from, to); + }); + expect(() => + renameIntoDirectory(path.join(dir, "d"), path.join(dir, "sub", "e")), + ).toThrow("was left as"); + expect(movingNames(dir)).toHaveLength(1); + }); + + it("reports the source left when its directory can no longer be entered", () => { + fs.mkdirSync(path.join(dir, "sub")); + fs.mkdirSync(path.join(dir, "src")); + fs.writeFileSync(path.join(dir, "src", "a.txt"), "a"); + onRename((call, from, to, real) => { + if (call === 2) { + fs.renameSync(path.join(dir, "src"), path.join(dir, "src-gone")); + throw errno("EIO"); + } + real(from, to); + }); + expect(() => + renameIntoDirectory( + path.join(dir, "src", "a.txt"), + path.join(dir, "sub", "b.txt"), + ), + ).toThrow("could not be put back"); + }); + + it("moves to a name as long as the filesystem allows", () => { + fs.mkdirSync(path.join(dir, "sub")); + fs.writeFileSync(path.join(dir, "a.txt"), "a"); + const long = `${"n".repeat(251)}.txt`; + renameIntoDirectory(path.join(dir, "a.txt"), path.join(dir, "sub", long)); + expect(fs.readFileSync(path.join(dir, "sub", long), "utf-8")).toBe("a"); + }); + + it("names the source when it is missing", () => { + expect(() => + renameIntoDirectory(path.join(dir, "gone.txt"), path.join(dir, "b.txt")), + ).toThrow(path.join(dir, "gone.txt")); + }); }); // Each test here runs on one device: the helper is what moveFile falls back @@ -964,3 +1290,108 @@ describe("the borrowed working directory", () => { expect(fs.realpathSync(".")).toBe(fs.realpathSync(path.parse(dir).root)); }); }); + +describe("the anchored reads", () => { + it("reads a whole file and restores the working directory", async () => { + const file = path.join(dir, "r.txt"); + fs.writeFileSync(file, "content"); + expect((await readFileInDirectory(file)).toString()).toBe("content"); + expect(process.cwd()).toBe(cwd); + }); + + it("reads at a position through the opened file, which knows its size", async () => { + const file = path.join(dir, "r.txt"); + fs.writeFileSync(file, "0123456789"); + const handle = await openFileForReading(file); + try { + expect(handle.size).toBe(10); + const buffer = Buffer.alloc(3); + expect(await handle.read(buffer, 0, 3, 4)).toEqual({ bytesRead: 3 }); + expect(buffer.toString()).toBe("456"); + } finally { + await handle.close(); + } + }); + + it("reports a read failure, and closes the file", async () => { + const file = path.join(dir, "r.txt"); + fs.writeFileSync(file, "content"); + vi.spyOn(fs, "readFile").mockImplementationOnce((( + _fd: number, + callback: (error: Error) => void, + ) => callback(errno("EIO"))) as typeof fs.readFile); + const close = vi.spyOn(fs, "close"); + await expect(readFileInDirectory(file)).rejects.toThrow("EIO"); + expect(close).toHaveBeenCalled(); + }); + + it("reports a positioned read failure", async () => { + const file = path.join(dir, "r.txt"); + fs.writeFileSync(file, "content"); + const handle = await openFileForReading(file); + vi.spyOn(fs, "read").mockImplementationOnce(((...args: unknown[]) => { + (args[args.length - 1] as (error: Error) => void)(errno("EIO")); + }) as typeof fs.read); + try { + await expect(handle.read(Buffer.alloc(1), 0, 1, 0)).rejects.toThrow( + "EIO", + ); + } finally { + await handle.close(); + } + }); + + it("refuses a file other than the one lstat saw, and closes it", async () => { + const file = path.join(dir, "r.txt"); + fs.writeFileSync(file, "content"); + const realLstat = fs.lstatSync; + vi.spyOn(fs, "lstatSync").mockImplementationOnce((p) => ({ + ...realLstat(p, { bigint: true }), + isDirectory: () => false, + isFile: () => true, + ino: -1n, + })); + const closeSync = vi.spyOn(fs, "closeSync"); + await expect(readFileInDirectory(file)).rejects.toThrow( + `Refusing to read: ${file} was replaced while it was being opened`, + ); + expect(closeSync).toHaveBeenCalled(); + }); + + it.skipIf(!symlinks)( + "refuses a directory that resolves somewhere else", + async () => { + fs.writeFileSync(path.join(outside, "s.txt"), "SECRET"); + fs.symlinkSync(outside, path.join(dir, "link")); + await expect( + readFileInDirectory(path.join(dir, "link", "s.txt")), + ).rejects.toThrow( + "Access denied - a directory on the path changed during the operation", + ); + expect(() => statInDirectory(path.join(dir, "link", "s.txt"))).toThrow( + "Access denied - a directory on the path changed during the operation", + ); + }, + ); + + it("stats the entry in its directory, and a filesystem root as is", () => { + const file = path.join(dir, "r.txt"); + fs.writeFileSync(file, "content"); + expect(statInDirectory(file).size).toBe(7); + const root = path.parse(dir).root; + expect(statInDirectory(root).isDirectory()).toBe(true); + expect(process.cwd()).toBe(cwd); + }); + + it("stats a UNC share root directly on Windows, as it does any root", () => { + Object.defineProperty(process, "platform", { value: "win32" }); + const stats = fs.lstatSync(dir); + const lstat = vi.spyOn(fs, "lstatSync").mockReturnValue(stats); + const chdir = vi.spyOn(process, "chdir"); + for (const root of ["\\\\server\\share", "\\\\?\\UNC\\server\\share\\"]) { + expect(statInDirectory(root)).toBe(stats); + expect(lstat).toHaveBeenLastCalledWith(root); + } + expect(chdir).not.toHaveBeenCalled(); + }); +}); diff --git a/src/filesystem/__tests__/read-anchored.test.ts b/src/filesystem/__tests__/read-anchored.test.ts new file mode 100644 index 0000000000..cc4dde14bc --- /dev/null +++ b/src/filesystem/__tests__/read-anchored.test.ts @@ -0,0 +1,226 @@ +// Protocol-level tests that every read is anchored to the file validation +// resolved, as every write is (pinned-directory.ts). The swap an attacker +// would race is staged deterministically: right after validation realpath's +// the requested path, a directory on it is swapped for a symlink to a +// directory outside the allowed one (or the file itself is swapped for a +// symlink or a FIFO). Each read must then refuse rather than return, or block +// on, what the swap put there. Before reads were anchored, each of these +// returned the outside file's content or size. + +import fs from "fs/promises"; +import fsSync from "fs"; +import { execFileSync } from "child_process"; +import path from "path"; +import { pathToFileURL } from "url"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ProtocolError, type Client } from "@modelcontextprotocol/client"; +import { + call, + connect, + makeTempDir, + quietStderr, + textOf, + type Connected, +} from "./helpers.js"; + +// Distinct content and sizes inside and outside, so a leak of either the +// content or the size shows. +const INSIDE = "inside\n"; +const SECRET = "SECRET-outside-the-allowed-directory-0123456789\n"; + +let dir: string; +let outside: string; +let flip: string; +let flipReal: string; +let target: string; +let conn: Connected; +let client: Client; + +beforeEach(async () => { + quietStderr(); + dir = await makeTempDir("mcp-fs-read-anchor-"); + outside = await makeTempDir("mcp-fs-read-anchor-outside-"); + flip = path.join(dir, "flip"); + flipReal = path.join(dir, "flip-real"); + target = path.join(flip, "note.txt"); + await fs.mkdir(flip); + await fs.writeFile(target, INSIDE); + await fs.writeFile(path.join(outside, "note.txt"), SECRET); + conn = await connect([dir]); + client = conn.client; +}); + +afterEach(async () => { + vi.restoreAllMocks(); + await conn.close(); + await fs.rm(dir, { recursive: true, force: true }); + await fs.rm(outside, { recursive: true, force: true }); +}); + +/** Run `swap` once, right after validation realpath's `requested`. */ +function swapAfterRealpath(requested: string, swap: () => void): void { + const realRealpath = fs.realpath.bind(fs); + let swapped = false; + vi.spyOn(fs, "realpath").mockImplementation(async (p, opts) => { + const resolved = await realRealpath(p, opts); + if (!swapped && p === requested) { + swapped = true; + swap(); + } + return resolved; + }); +} + +/** Swap `flip` for a symlink to `outside`. */ +function swapDirectory(): void { + fsSync.renameSync(flip, flipReal); + fsSync.symlinkSync(outside, flip); +} + +const DIRECTORY_CHANGED = /^Access denied - a directory on the path changed/; + +describe.skipIf(process.platform === "win32")( + "a directory on the path swapped for a symlink after validation", + () => { + it.each([ + ["read_text_file", {}], + ["read_text_file", { head: 5 }], + ["read_text_file", { tail: 5 }], + ["read_file", {}], + ["read_media_file", {}], + ])("%s %j does not read through it", async (tool, extra) => { + swapAfterRealpath(target, swapDirectory); + const result = await call(client, tool, { path: target, ...extra }); + expect(result.isError).toBe(true); + expect(textOf(result)).toMatch(DIRECTORY_CHANGED); + expect(JSON.stringify(result)).not.toContain("SECRET"); + expect(JSON.stringify(result)).not.toContain( + Buffer.from(SECRET).toString("base64"), + ); + }); + + it("read_multiple_files does not read through it", async () => { + swapAfterRealpath(target, swapDirectory); + const result = await call(client, "read_multiple_files", { + paths: [target], + }); + expect(textOf(result)).toMatch( + new RegExp(`^${target}: Error - Access denied - a directory`), + ); + expect(textOf(result)).not.toContain("SECRET"); + }); + + it("edit_file with dryRun does not preview through it", async () => { + swapAfterRealpath(target, swapDirectory); + const result = await call(client, "edit_file", { + path: target, + edits: [{ oldText: "SECRET", newText: "x" }], + dryRun: true, + }); + expect(result.isError).toBe(true); + expect(textOf(result)).toMatch(DIRECTORY_CHANGED); + expect(textOf(result)).not.toContain("outside-the-allowed"); + }); + + it("get_file_info does not describe the file through it", async () => { + swapAfterRealpath(target, swapDirectory); + const result = await call(client, "get_file_info", { path: target }); + expect(result.isError).toBe(true); + expect(textOf(result)).toMatch(DIRECTORY_CHANGED); + expect(textOf(result)).not.toContain(`size: ${SECRET.length}`); + }); + + it("list_directory_with_sizes does not size an entry through it", async () => { + swapAfterRealpath(target, swapDirectory); + const result = await call(client, "list_directory_with_sizes", { + path: flip, + }); + expect(result.isError).toBeFalsy(); + // Listed, but with its size unknown rather than the outside file's. + expect(textOf(result)).toMatch(/\[FILE\] note\.txt\s+unknown/); + expect(textOf(result)).not.toContain(`${SECRET.length} B`); + }); + + it("resources/read does not read through it", async () => { + swapAfterRealpath(target, swapDirectory); + const error = await client + .readResource({ uri: pathToFileURL(target).href }) + .then( + () => undefined, + (e: unknown) => e, + ); + expect(error).toBeInstanceOf(ProtocolError); + expect((error as ProtocolError).message).toMatch( + /Access denied - a directory on the path changed/, + ); + expect((error as ProtocolError).message).not.toContain("SECRET"); + }); + }, +); + +describe.skipIf(process.platform === "win32")( + "the file itself swapped after validation", + () => { + it("is not read through a symlink swapped in", async () => { + swapAfterRealpath(target, () => { + fsSync.unlinkSync(target); + fsSync.symlinkSync(path.join(outside, "note.txt"), target); + }); + const result = await call(client, "read_text_file", { path: target }); + expect(result.isError).toBe(true); + expect(textOf(result)).toBe(`Cannot read: not a regular file: ${target}`); + }); + + it("is not read, and does not block, when a FIFO is swapped in", async () => { + swapAfterRealpath(target, () => { + fsSync.unlinkSync(target); + execFileSync("mkfifo", [target]); + }); + const result = await call(client, "read_text_file", { path: target }); + expect(result.isError).toBe(true); + expect(textOf(result)).toBe(`Cannot read: not a regular file: ${target}`); + }); + }, +); + +describe("reads that are not swapped", () => { + it("read every tool's way, from the file validation resolved", async () => { + expect(textOf(await call(client, "read_text_file", { path: target }))).toBe( + INSIDE, + ); + expect( + textOf(await call(client, "read_text_file", { path: target, tail: 1 })), + ).toBe("inside"); + expect( + textOf(await call(client, "read_text_file", { path: target, head: 1 })), + ).toBe("inside"); + const info = textOf(await call(client, "get_file_info", { path: target })); + expect(info).toContain(`size: ${INSIDE.length}`); + }); + + it("reports a missing file by its full path", async () => { + const missing = path.join(dir, "missing.txt"); + const result = await call(client, "read_text_file", { path: missing }); + expect(result.isError).toBe(true); + expect(textOf(result)).toBe( + `ENOENT: no such file or directory, lstat '${missing}'`, + ); + }); + + it("reports a directory as EISDIR, naming it", async () => { + const result = await call(client, "read_text_file", { path: flip }); + expect(result.isError).toBe(true); + expect(textOf(result)).toBe( + `EISDIR: illegal operation on a directory, read '${flip}'`, + ); + }); + + it("reports a missing file to get_file_info by its full path", async () => { + const missing = path.join(dir, "missing.txt"); + const result = await call(client, "get_file_info", { path: missing }); + expect(result.isError).toBe(true); + expect(textOf(result)).toBe( + `ENOENT: no such file or directory, lstat '${missing}'`, + ); + }); +}); diff --git a/src/filesystem/__tests__/server-tools.test.ts b/src/filesystem/__tests__/server-tools.test.ts index 1bdb34fd0a..fb7152c54b 100644 --- a/src/filesystem/__tests__/server-tools.test.ts +++ b/src/filesystem/__tests__/server-tools.test.ts @@ -660,7 +660,11 @@ describe("read_multiple_files", () => { it("reports a non-Error read failure by its string form", async () => { const a = path.join(dir, "a.txt"); await fs.writeFile(a, "alpha"); - vi.spyOn(fs, "readFile").mockRejectedValueOnce("disk on fire"); + // The read goes through the descriptor the anchored open returned. + vi.spyOn(fsSync, "readFile").mockImplementationOnce((( + _fd: number, + callback: (error: unknown) => void, + ) => callback("disk on fire")) as typeof fsSync.readFile); const result = await call(client, "read_multiple_files", { paths: [a] }); expect(textOf(result)).toBe(`${a}: Error - disk on fire`); }); diff --git a/src/filesystem/file-resources.ts b/src/filesystem/file-resources.ts index df1fd333e8..5ff8ef76f7 100644 --- a/src/filesystem/file-resources.ts +++ b/src/filesystem/file-resources.ts @@ -17,7 +17,6 @@ // the live allow-list, the roots barrier and the reason a 2026-07-28 // connection with no directories cannot operate, so a read sees the same // directories, at the same moment, as a tool call would. -import fs from "fs/promises"; import path from "path"; import { fileURLToPath } from "url"; import { @@ -29,6 +28,7 @@ import { } from "@modelcontextprotocol/server"; import type { ReadResourceResult } from "@modelcontextprotocol/server"; import { FsTimeoutError, validatePath } from "./lib.js"; +import { readFileInDirectory } from "./pinned-directory.js"; /** The URI template every file resource matches (RFC 6570 reserved expansion). */ export const FILE_RESOURCE_TEMPLATE = "file://{+path}"; @@ -226,7 +226,9 @@ export function registerFileResources( let bytes: Buffer; try { - bytes = await fs.readFile(validPath); + // Anchored to the validated file's directory, as the tools' reads + // are (pinned-directory.ts). + bytes = await readFileInDirectory(validPath); } catch (error) { if (isMissingPathError(error)) { throw new ResourceNotFoundError(uri.href); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 1ff5fe67ea..346f5e6c99 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -13,8 +13,11 @@ import { createFileInDirectory, moveFileAcrossDevices, copyFileInDirectory, + openFileForReading, overwriteFileInDirectory, + readFileInDirectory, renameIntoDirectory, + statInDirectory, } from "./pinned-directory.js"; // Type definitions @@ -593,8 +596,13 @@ async function resolveAllowedPath( } // File Operations +// +// Every read below, of content or of metadata, goes through +// pinned-directory.ts, as every write does: the path was validated before it +// runs, and a directory on it swapped for a symlink since then must not +// redirect the read outside the allowed directories (TOCTOU). export async function getFileStats(filePath: string): Promise { - const stats = await fs.stat(filePath); + const stats = statInDirectory(filePath); return { size: stats.size, created: stats.birthtime, @@ -613,7 +621,9 @@ export async function readFileContent( filePath: string, encoding: string = "utf-8", ): Promise { - return await fs.readFile(filePath, encoding as BufferEncoding); + return (await readFileInDirectory(filePath)).toString( + encoding as BufferEncoding, + ); } export async function writeFileContent( @@ -1021,7 +1031,7 @@ export async function applyFileEdits( ): Promise { // Read file content and normalize line endings, remembering which ending // the file uses so the edited file is written back with it (#4991). - const rawContent = await fs.readFile(filePath, "utf-8"); + const rawContent = await readFileContent(filePath); const eol = detectLineEnding(rawContent); const content = normalizeLineEndings(rawContent); @@ -1145,15 +1155,13 @@ export async function tailFile( numLines: number, ): Promise { const CHUNK_SIZE = 1024; // Read 1KB at a time - const stats = await fs.stat(filePath); - const fileSize = stats.size; - - // Zero lines is an empty result; slice(-0) below would return every line. - if (fileSize === 0 || numLines === 0) return ""; - - // Open file for reading - const fileHandle = await fs.open(filePath, "r"); + // Open file for reading; its size is the opened file's. + const fileHandle = await openFileForReading(filePath); try { + const fileSize = fileHandle.size; + // Zero lines is an empty result; slice(-0) below would return every line. + if (fileSize === 0 || numLines === 0) return ""; + const chunks: Buffer[] = []; let position = fileSize; const chunk = Buffer.alloc(CHUNK_SIZE); @@ -1197,7 +1205,7 @@ export async function headFile( filePath: string, numLines: number, ): Promise { - const fileHandle = await fs.open(filePath, "r"); + const fileHandle = await openFileForReading(filePath); try { const lines: string[] = []; let buffer = ""; diff --git a/src/filesystem/path-validation.ts b/src/filesystem/path-validation.ts index 6092970d98..47e4afb25c 100644 --- a/src/filesystem/path-validation.ts +++ b/src/filesystem/path-validation.ts @@ -43,6 +43,18 @@ export function namesAlternateDataStream(normalizedPath: string): boolean { const WINDOWS_UNC_ROOT = /^(?:\\\\[?.]\\UNC\\|\\\\(?![?.]\\))[^\\/]+[\\/][^\\/]+/i; +/** + * True when `windowsPath` is a UNC root itself (`\\server\share`, + * `\\?\UNC\server\share`, with or without a trailing separator): a root, + * like `C:\`, with no parent directory above it. Node's `path.win32` takes + * the share for a name inside the server, or, for the `\\?\UNC\` form, + * the server for a directory. + */ +export function isWindowsUncRoot(windowsPath: string): boolean { + const match = WINDOWS_UNC_ROOT.exec(windowsPath); + return match !== null && /^[\\/]?$/.test(windowsPath.slice(match[0].length)); +} + const WINDOWS_DEVICE_NAME = /^(?:CON|PRN|AUX|NUL|COM[1-9¹²³]|LPT[1-9¹²³]) *(?:\..*)?$/is; diff --git a/src/filesystem/pinned-directory.ts b/src/filesystem/pinned-directory.ts index 69a1af5f5b..7805b4ecee 100644 --- a/src/filesystem/pinned-directory.ts +++ b/src/filesystem/pinned-directory.ts @@ -1,9 +1,10 @@ -// Write operations anchored to their directory, so a directory swapped out -// from under them cannot redirect them. +// Filesystem operations anchored to their directory, so a directory swapped +// out from under them cannot redirect them: every write, and every read that +// returns a file's content or metadata. // // validatePath() checks a path and returns its canonical form, but the write -// that follows resolves that path again. A directory on it swapped for a -// symlink in between would redirect the write outside the allowed directories, +// or read that follows resolves that path again. A directory on it swapped +// for a symlink in between would redirect it outside the allowed directories, // because the kernel follows the symlink at use. Checking again afterwards // cannot close that (the racer swaps back in time), and openat() is not in // Node. The substitute here: chdir() into the directory, read back where the @@ -11,17 +12,25 @@ // the string it was given), refuse unless that is the validated directory, // and only then act on the final component relative to it. The working // directory holds the directory's inode, so swapping the path afterwards -// cannot move the operation. Every check happens before a byte is written, -// so nothing ever needs rolling back, except in the cross-device move, which -// copies and so removes its copy if a later step fails. +// cannot move the operation. A read opens the file that way and then reads +// only through the descriptor, which stays bound to the file it opened, so +// the read is anchored however long it takes. Every check happens before a +// byte is written or read, so nothing ever needs rolling back, except in two +// moves: the cross-device move copies, and so removes its copy if a later +// step fails; the same-device move can anchor only one side of each rename +// (rename takes two paths), so it first moves its source to a fresh random +// name inside the anchored source directory, renames only that name across, +// checks it arrived by identity, and puts the source back if anything fails. // // On Windows the working directory is a string and relative paths resolve // against it, so reading it back proves nothing; there the leaf is lstat'd // relative to the anchor and must be the file the descriptor opened, which // narrows the race without closing it. +import { randomBytes } from "crypto"; import fs from "fs"; import path from "path"; +import { isWindowsUncRoot } from "./path-validation.js"; // Descriptor I/O, looked up on fs at call time so tests can stand in for it. function ftruncate(fd: number): Promise { @@ -346,16 +355,151 @@ export async function appendFileInDirectory( } /** - * Rename `sourcePath` (absolute, outside this fix's scope) to the final - * component of `destinationPath`, relative to its anchored directory. + * A fresh name to stage a rename under. It is random, so nothing at it can + * have been prepared in advance, and its length is fixed (30 bytes), so it + * fits a filesystem's name limit whatever the entry's own name is. + */ +function stagingName(): string { + return `.mcp-${randomBytes(8).toString("hex")}.moving`; +} + +/** + * Rename `staged` to `name` without replacing anything at `name`, and say + * whether it did. A hard link refuses an existing name; a directory, which + * cannot be linked, is renamed only while nothing is at `name`. + */ +function returnStaged(staged: string, name: string): boolean { + try { + fs.linkSync(staged, name); + fs.unlinkSync(staged); + return true; + } catch { + // Not linkable (a directory), or something is at `name` now. + } + try { + fs.lstatSync(name); + return false; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + return false; + } + } + try { + fs.renameSync(staged, name); + return true; + } catch { + return false; + } +} + +/** + * Rename `sourcePath` to `destinationPath`, keeping both inside their + * validated directories. rename() takes two paths and Node has no + * renameat(), so each rename can resolve only one side relative to an + * anchor. The other side always names a fresh random name (`stagingName`), + * so a directory swapped on that side's path leads to a name nothing holds, + * and the rename fails, rather than to an entry that was waiting there: + * + * 1. Anchored at the source's directory, the source moves to a fresh name, + * and its identity is read. + * 2. Anchored at the destination's directory, that name moves to another + * fresh name there, which must be the entry read in step 1. + * 3. Still anchored, it takes the destination's name. + * + * If a step fails, the source goes back under its own name, and the error is + * the step's own (EXDEV still sends moveFile to the cross-device copy). If it + * cannot go back (something took its name in the meantime, or its directory + * no longer resolves to the one validated, which is never entered), the error + * says where it was left. */ export function renameIntoDirectory( sourcePath: string, destinationPath: string, ): void { - inDirectory(path.dirname(destinationPath), () => - fs.renameSync(sourcePath, path.basename(destinationPath)), - ); + const sourceDirectory = path.dirname(sourcePath); + const name = path.basename(sourcePath); + const destinationDirectory = path.dirname(destinationPath); + const held = stagingName(); + const expected = inDirectory(sourceDirectory, () => { + try { + fs.renameSync(name, held); + } catch (error) { + throw namingPath(error, name, sourcePath); + } + return fs.lstatSync(held, { bigint: true }); + }); + const landing = stagingName(); + let landed = false; + try { + inDirectory(destinationDirectory, () => { + try { + fs.renameSync(path.join(sourceDirectory, held), landing); + } catch (error) { + // The name was made in step 1, so a path that no longer reaches it + // leads somewhere else: the source's directory was swapped. + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + throw directoryChangedError(sourceDirectory); + } + throw error; + } + if (!isSameEntry(fs.lstatSync(landing, { bigint: true }), expected)) { + // An entry another process put under the fresh name, after seeing + // it, at the end of a swapped path: send it back the way it came. + // The source is still where step 1 put it. + returnStaged(landing, path.join(sourceDirectory, held)); + throw directoryChangedError(sourceDirectory); + } + landed = true; + fs.renameSync(landing, path.basename(destinationPath)); + }); + } catch (error) { + // Named by its directory as validated: if that directory was swapped, + // the entry is in the directory that was moved away, under this name. + let left = landed + ? `${landing} in ${destinationDirectory}` + : `${held} in ${sourceDirectory}`; + let restored = false; + try { + restored = inDirectory(sourceDirectory, () => { + let staged = held; + if (landed) { + // The destination's directory is reached by path here, and could + // have been swapped since step 3 failed, with an entry planted + // under the landing name it showed. So the entry comes back under + // a fresh name and must be the source before it takes the source's + // name; anything else goes back the way it came. + staged = stagingName(); + try { + fs.renameSync(path.join(destinationDirectory, landing), staged); + } catch { + return false; + } + if (!isSameEntry(fs.lstatSync(staged, { bigint: true }), expected)) { + returnStaged(staged, path.join(destinationDirectory, landing)); + return false; + } + left = `${staged} in ${sourceDirectory}`; + } + return returnStaged(staged, name); + }); + } catch { + // The source's directory cannot be entered as validated. + } + if (restored) { + throw error; + } + // EXDEV is dropped: it sends moveFile to copy whatever is at sourcePath + // now, which is not the source. The step's error stays as the cause. + const code = (error as NodeJS.ErrnoException).code; + throw Object.assign( + new Error( + `${(error as Error).message}. The entry being moved could not be ` + + `put back at ${sourcePath} and was left as ${left}`, + { cause: error }, + ), + { code: code === "EXDEV" ? undefined : code }, + ); + } } // The cross-device move below (#5188). rename() cannot cross a device @@ -408,40 +552,93 @@ function keepModeAndTimes(fd: number, source: fs.BigIntStats): Promise { ); } +/** What opening a regular file for reading is for, named in its refusals. */ +type ReadOperation = "move" | "copy" | "read"; + +function directoryRefusal(sourcePath: string, operation: ReadOperation): Error { + switch (operation) { + case "move": + return new Error( + `Cannot move a directory across filesystems: ${sourcePath}. ` + + "Create the directory at the destination and move its files one at a time.", + ); + case "copy": + return new Error( + `Cannot copy a directory: ${sourcePath}. copy_file copies one regular file; ` + + "create the directory at the destination and copy its files one at a time.", + ); + case "read": + // The code a read of a directory has always failed with, which callers + // (resources/read) branch on. + return Object.assign( + new Error( + `EISDIR: illegal operation on a directory, read '${sourcePath}'`, + ), + { code: "EISDIR" }, + ); + } +} + +function notRegularRefusal( + sourcePath: string, + operation: ReadOperation, +): Error { + switch (operation) { + case "move": + return new Error( + `Cannot move across filesystems: not a regular file: ${sourcePath}`, + ); + case "copy": + return new Error(`Cannot copy: not a regular file: ${sourcePath}`); + case "read": + return new Error(`Cannot read: not a regular file: ${sourcePath}`); + } +} + +/** + * An errno error from a call on the anchor-relative `name` names that bare + * name; name `filePath` instead, as the call on the absolute path would have. + */ +function namingPath(error: unknown, name: string, filePath: string): unknown { + const errno = error as NodeJS.ErrnoException; + if (error instanceof Error && errno.path === name) { + error.message = error.message.replace(`'${name}'`, `'${filePath}'`); + errno.path = filePath; + } + return error; +} + /** * Open the regular file at `sourcePath` for reading, relative to its anchored * directory. A directory, a symlink or anything else that is not a regular * file is refused: a symlink is not followed or recreated, since its target - * could name anything, and a directory is not copied recursively. `operation` - * names the caller in those refusals: a cross-device move, or copy_file. + * could name anything, a directory is not copied recursively, and a FIFO or + * device is never read, so a read cannot block on one. `operation` names the + * caller in those refusals: a cross-device move, copy_file, or a read. */ function openSourceInDirectory( sourcePath: string, - operation: "move" | "copy", + operation: ReadOperation, ): { fd: number; opened: fs.BigIntStats; } { const name = path.basename(sourcePath); return inDirectory(path.dirname(sourcePath), () => { - const found = fs.lstatSync(name, { bigint: true }); - if (found.isDirectory()) { - throw new Error( - operation === "move" - ? `Cannot move a directory across filesystems: ${sourcePath}. ` + - "Create the directory at the destination and move its files one at a time." - : `Cannot copy a directory: ${sourcePath}. copy_file copies one regular file; ` + - "create the directory at the destination and copy its files one at a time.", - ); - } - if (!found.isFile()) { - throw new Error( - operation === "move" - ? `Cannot move across filesystems: not a regular file: ${sourcePath}` - : `Cannot copy: not a regular file: ${sourcePath}`, - ); + let found: fs.BigIntStats; + let fd: number; + try { + found = fs.lstatSync(name, { bigint: true }); + if (found.isDirectory()) { + throw directoryRefusal(sourcePath, operation); + } + if (!found.isFile()) { + throw notRegularRefusal(sourcePath, operation); + } + fd = fs.openSync(name, readFlags()); + } catch (error) { + throw namingPath(error, name, sourcePath); } - const fd = fs.openSync(name, readFlags()); try { const opened = fs.fstatSync(fd, { bigint: true }); if (!opened.isFile() || !isSameEntry(found, opened)) { @@ -640,6 +837,95 @@ export async function copyFileInDirectory( } } +// Reads. The file is opened as the copy source above +// is, and everything read from it is read through that one descriptor. + +function readWhole(fd: number): Promise { + return new Promise((resolve, reject) => + fs.readFile(fd, (error, data) => (error ? reject(error) : resolve(data))), + ); +} + +function readAt( + fd: number, + buffer: Buffer, + offset: number, + length: number, + position: number, +): Promise<{ bytesRead: number }> { + return new Promise((resolve, reject) => + fs.read(fd, buffer, offset, length, position, (error, bytesRead) => + error ? reject(error) : resolve({ bytesRead }), + ), + ); +} + +/** A regular file opened for reading by openFileForReading. */ +export interface ReadableFile { + /** The size of the file opened, from its descriptor. */ + readonly size: number; + /** Read up to `length` bytes at `position` into `buffer` at `offset`. */ + read( + buffer: Buffer, + offset: number, + length: number, + position: number, + ): Promise<{ bytesRead: number }>; + close(): Promise; +} + +/** + * Open the regular file at `filePath` for reading, relative to its anchored + * directory, refusing anything else (see openSourceInDirectory). The caller + * reads through the result and closes it. + */ +export async function openFileForReading( + filePath: string, +): Promise { + const { fd, opened } = openSourceInDirectory(filePath, "read"); + return { + size: Number(opened.size), + read: (buffer, offset, length, position) => + readAt(fd, buffer, offset, length, position), + close: () => close(fd), + }; +} + +/** + * Read the whole of the regular file at `filePath`, opened relative to its + * anchored directory. + */ +export async function readFileInDirectory(filePath: string): Promise { + const { fd } = openSourceInDirectory(filePath, "read"); + try { + return await readWhole(fd); + } finally { + await close(fd); + } +} + +/** + * The lstat of `filePath`, taken relative to its anchored directory, so it + * describes the entry validation resolved rather than whatever a swapped + * directory now leads to. validatePath returns a resolved path, so its last + * component is not a symlink, and this reports what stat() would. A + * filesystem root has no directory above it to swap, and is lstat'd as is. + */ +export function statInDirectory(filePath: string): fs.Stats { + const name = path.basename(filePath); + // A root (`/`, `C:\`, a UNC share) has no directory above it to anchor at. + if (name === "" || (onWindows() && isWindowsUncRoot(filePath))) { + return fs.lstatSync(filePath); + } + return inDirectory(path.dirname(filePath), () => { + try { + return fs.lstatSync(name); + } catch (error) { + throw namingPath(error, name, filePath); + } + }); +} + /** * Create `directory` and any missing parents, like mkdir -p: anchor at the * deepest ancestor that exists, then step down one component at a time, diff --git a/src/filesystem/server.ts b/src/filesystem/server.ts index 01535d3b6c..2517bd7972 100644 --- a/src/filesystem/server.ts +++ b/src/filesystem/server.ts @@ -7,7 +7,6 @@ import { McpServer } from "@modelcontextprotocol/server"; import type { Root } from "@modelcontextprotocol/server"; import fs from "fs/promises"; -import { createReadStream } from "fs"; import path from "path"; import { pathToFileURL } from "url"; import { z } from "zod"; @@ -209,24 +208,6 @@ const GetFileInfoArgsSchema = z.object({ path: pathArg("The file or directory to describe"), }); -// Reads a file as a stream of buffers, concatenates them, and then encodes -// the result to a Base64 string. This is a memory-efficient way to handle -// binary data from a stream before the final encoding. -async function readFileAsBase64Stream(filePath: string): Promise { - return new Promise((resolve, reject) => { - const stream = createReadStream(filePath); - const chunks: Buffer[] = []; - stream.on("data", (chunk) => { - chunks.push(chunk as Buffer); - }); - stream.on("end", () => { - const finalBuffer = Buffer.concat(chunks); - resolve(finalBuffer.toString("base64")); - }); - stream.on("error", (err) => reject(err)); - }); -} - /** * Why the server cannot operate when it has no allowed directories from either * the command line or the client's Roots (#4992). @@ -503,7 +484,8 @@ export function createServer( ".flac": "audio/flac", }; const mimeType = mimeTypes[extension] || "application/octet-stream"; - const data = await readFileAsBase64Stream(validPath); + // Read through the anchored descriptor, as every read is (lib.ts). + const data = await readFileContent(validPath, "base64"); // Map the MIME type to a valid MCP content block. The spec only allows // text, image, audio, resource_link, and resource — so non-image/audio @@ -790,15 +772,16 @@ export function createServer( entries.map(async (entry): Promise => { const entryPath = path.join(validPath, entry.name); try { - // fs.stat follows symlinks, so validate each entry first: a - // symlink whose target is outside the allowed directories is - // listed with no size rather than the target's. + // Validate each entry, so a symlink whose target is outside the + // allowed directories is listed with no size rather than the + // target's; the size is then read from the resolved entry, + // anchored to its directory (lib.ts). const validEntryPath = await validatePath( entryPath, allowedDirectories, "read", ); - const stats = await fs.stat(validEntryPath); + const stats = await getFileStats(validEntryPath); return { name: entry.name, isDirectory: entry.isDirectory(),