diff --git a/.changeset/5154-filesystem-bigint-identity.md b/.changeset/5154-filesystem-bigint-identity.md new file mode 100644 index 0000000000..62fc36217a --- /dev/null +++ b/.changeset/5154-filesystem-bigint-identity.md @@ -0,0 +1,5 @@ +--- +"@modelcontextprotocol/server-filesystem": patch +--- + +`write_file` and `edit_file` compare the file they opened against the one they checked as BigInt stats, so a Windows file id above 2^53 is compared exactly rather than rounded. Where a path-based stat on Windows reports no device (libuv < 1.51.0, the Node 23 line), the overwrite no longer refuses every existing file: the file must instead match on inode, size and modification time, and any difference still refuses. diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index 793c98e59c..886bc25af5 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -98,9 +98,15 @@ function createMockFileHandle(content: Buffer) { }; } -// What lstat and the opened handle's stat report for the file an in-place -// write replaces: a regular file, the same device and inode both times. -const REGULAR_FILE = { isFile: () => true, dev: 1, ino: 42 } as Stats; +// What lstat reports for the file an in-place write replaces: a regular file, +// as a BigInt stat, the form the identity check compares exactly (#5154). +const REGULAR_FILE = { + isFile: () => true, + dev: 1n, + ino: 42n, + size: 8n, + mtimeNs: 1_700_000_000_000_000_000n, +} as BigIntStats; // The allow-list validatePath checks against; each server instance passes its // own (#4854), so the unit tests pass this one explicitly. @@ -618,7 +624,9 @@ describe("Lib Functions", () => { await writeFileContent("/test/script.sh", "new content"); - expect(mockFs.lstat).toHaveBeenCalledWith("/test/script.sh"); + expect(mockFs.lstat).toHaveBeenCalledWith("/test/script.sh", { + bigint: true, + }); expect(mockPinned.overwriteFileInDirectory).toHaveBeenCalledWith( "/test/script.sh", "new content", @@ -638,7 +646,7 @@ describe("Lib Functions", () => { mockFs.lstat.mockResolvedValueOnce({ ...REGULAR_FILE, isFile: () => false, - } as Stats); + } as BigIntStats); await expect( writeFileContent("/test/link.txt", "new content"), diff --git a/src/filesystem/__tests__/pinned-directory.test.ts b/src/filesystem/__tests__/pinned-directory.test.ts index e84cbb0675..43f83df7d0 100644 --- a/src/filesystem/__tests__/pinned-directory.test.ts +++ b/src/filesystem/__tests__/pinned-directory.test.ts @@ -5,7 +5,7 @@ // working-directory bookkeeping a borrowed chdir needs. import { execFileSync } from "child_process"; -import fs from "fs"; +import fs, { type BigIntStats } from "fs"; import path from "path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { @@ -20,6 +20,9 @@ import { makeTempDir } from "./helpers.js"; const symlinks = process.platform !== "win32"; const realPlatform = process.platform; +// The identity check compares BigInt stats (#5154), so every expected stat a +// test hands it is one. +const lstatBig = (p: string) => fs.lstatSync(p, { bigint: true }); const errno = (code: string) => Object.assign(new Error(`${code}: stand-in`), { code }); @@ -144,8 +147,8 @@ describe("createFileInDirectory", () => { it("refuses, and closes the file, when the path no longer holds what was opened", async () => { const realLstat = fs.lstatSync; vi.spyOn(fs, "lstatSync").mockImplementationOnce((p) => ({ - ...realLstat(p), - ino: -1, + ...realLstat(p, { bigint: true }), + ino: -1n, })); const closeSync = vi.spyOn(fs, "closeSync"); await expect( @@ -183,8 +186,8 @@ describe("createFileInDirectory", () => { fs.symlinkSync(outside, path.join(dir, "link")); const realLstat = fs.lstatSync; vi.spyOn(fs, "lstatSync").mockImplementationOnce((p) => ({ - ...realLstat(p), - ino: -1, + ...realLstat(p, { bigint: true }), + ino: -1n, })); await expect( createFileInDirectory(path.join(dir, "link", "x.txt"), "x"), @@ -197,16 +200,16 @@ describe("overwriteFileInDirectory", () => { it("replaces the content in place, keeping the inode", async () => { const file = path.join(dir, "f.txt"); fs.writeFileSync(file, "a much longer original"); - const expected = fs.lstatSync(file); + const expected = lstatBig(file); await overwriteFileInDirectory(file, "short", expected); expect(fs.readFileSync(file, "utf-8")).toBe("short"); - expect(fs.lstatSync(file).ino).toBe(expected.ino); + expect(lstatBig(file).ino).toBe(expected.ino); }); it("refuses a file other than the one expected", async () => { const file = path.join(dir, "f.txt"); fs.writeFileSync(file, "original"); - const expected = { ...fs.lstatSync(file), ino: -1 }; + const expected = { ...lstatBig(file), ino: -1n }; await expect( overwriteFileInDirectory(file, "new", expected), ).rejects.toThrow( @@ -218,10 +221,10 @@ describe("overwriteFileInDirectory", () => { it("refuses something that is not a regular file", async () => { const file = path.join(dir, "f.txt"); fs.writeFileSync(file, "original"); - const expected = fs.lstatSync(file); + const expected = lstatBig(file); const realFstat = fs.fstatSync; vi.spyOn(fs, "fstatSync").mockImplementationOnce((fd) => { - const stats = realFstat(fd); + const stats = realFstat(fd, { bigint: true }); return Object.assign(stats, { isFile: () => false }); }); await expect( @@ -237,7 +240,7 @@ describe("overwriteFileInDirectory", () => { }) as typeof fs.ftruncate); const close = vi.spyOn(fs, "close"); await expect( - overwriteFileInDirectory(file, "new", fs.lstatSync(file)), + overwriteFileInDirectory(file, "new", lstatBig(file)), ).rejects.toThrow("EIO"); expect(close).toHaveBeenCalled(); expect(fs.readFileSync(file, "utf-8")).toBe("original"); @@ -249,7 +252,11 @@ describe("overwriteFileInDirectory", () => { const link = path.join(dir, "l.txt"); fs.symlinkSync(target, link); await expect( - overwriteFileInDirectory(link, "pwned", fs.statSync(target)), + overwriteFileInDirectory( + link, + "pwned", + fs.statSync(target, { bigint: true }), + ), ).rejects.toMatchObject({ code: "ELOOP" }); expect(fs.readFileSync(target, "utf-8")).toBe("SECRET"); }); @@ -265,7 +272,11 @@ describe("on Windows, where O_NOFOLLOW does not exist", () => { const link = path.join(dir, "l.txt"); fs.symlinkSync(target, link); await expect( - overwriteFileInDirectory(link, "pwned", fs.statSync(target)), + overwriteFileInDirectory( + link, + "pwned", + fs.statSync(target, { bigint: true }), + ), ).rejects.toThrow(`Refusing to write: not a regular file: ${link}`); expect(fs.readFileSync(target, "utf-8")).toBe("SECRET"); }, @@ -275,11 +286,120 @@ describe("on Windows, where O_NOFOLLOW does not exist", () => { Object.defineProperty(process, "platform", { value: "win32" }); const file = path.join(dir, "f.txt"); fs.writeFileSync(file, "original"); - await overwriteFileInDirectory(file, "new", fs.lstatSync(file)); + await overwriteFileInDirectory(file, "new", lstatBig(file)); expect(fs.readFileSync(file, "utf-8")).toBe("new"); }); }); +describe("the identity check (#5154)", () => { + it("compares inodes exactly, where a number stat would round a 64-bit file id", async () => { + const file = path.join(dir, "f.txt"); + fs.writeFileSync(file, "original"); + const high = 2n ** 53n; + // The premise: as doubles, these two file ids are one number. + expect(Number(high + 1n)).toBe(Number(high)); + const expected = { ...lstatBig(file), ino: high }; + const realFstat = fs.fstatSync; + const realLstat = fs.lstatSync; + // Persistent and prototype-preserving: on Windows openInDirectory lstats + // the name once for its symlink check before the identity check. + vi.spyOn(fs, "fstatSync").mockImplementation((fd) => + Object.assign(realFstat(fd, { bigint: true }), { ino: high + 1n }), + ); + vi.spyOn(fs, "lstatSync").mockImplementation((p) => + Object.assign(realLstat(p, { bigint: true }), { ino: high + 1n }), + ); + await expect( + overwriteFileInDirectory(file, "new", expected), + ).rejects.toThrow("was replaced while it was being opened"); + expect(fs.readFileSync(file, "utf-8")).toBe("original"); + }); + + describe("on Windows, where a path stat can report no device (libuv < 1.51.0)", () => { + beforeEach(() => { + Object.defineProperty(process, "platform", { value: "win32" }); + }); + + it("overwrites when inode, size and mtime all match", async () => { + const file = path.join(dir, "f.txt"); + fs.writeFileSync(file, "original"); + const expected = { ...lstatBig(file), dev: 0n }; + await overwriteFileInDirectory(file, "new", expected); + expect(fs.readFileSync(file, "utf-8")).toBe("new"); + }); + + it("creates a file whose path stat reports no device", async () => { + const realLstat = fs.lstatSync; + vi.spyOn(fs, "lstatSync").mockImplementationOnce((p) => ({ + ...realLstat(p, { bigint: true }), + dev: 0n, + })); + const file = path.join(dir, "new.txt"); + await createFileInDirectory(file, "hello"); + expect(fs.readFileSync(file, "utf-8")).toBe("hello"); + }); + + it.each([ + ["inode", (s: BigIntStats) => ({ ino: s.ino + 1n })], + ["size", (s: BigIntStats) => ({ size: s.size + 1n })], + ["mtime", (s: BigIntStats) => ({ mtimeNs: s.mtimeNs - 1n })], + ])("refuses when the %s differs", async (_field, change) => { + const file = path.join(dir, "f.txt"); + fs.writeFileSync(file, "original"); + const stats = lstatBig(file); + const expected = { ...stats, dev: 0n, ...change(stats) }; + await expect( + overwriteFileInDirectory(file, "new", expected), + ).rejects.toThrow("was replaced while it was being opened"); + expect(fs.readFileSync(file, "utf-8")).toBe("original"); + }); + + it("refuses a known device that differs", async () => { + const file = path.join(dir, "f.txt"); + fs.writeFileSync(file, "original"); + const stats = lstatBig(file); + const expected = { ...stats, dev: stats.dev + 1n }; + await expect( + overwriteFileInDirectory(file, "new", expected), + ).rejects.toThrow("was replaced while it was being opened"); + }); + }); + + it.skipIf(realPlatform === "win32")( + "off Windows, refuses a path stat that reports no device", + async () => { + const file = path.join(dir, "f.txt"); + fs.writeFileSync(file, "original"); + const expected = { ...lstatBig(file), dev: 0n }; + await expect( + overwriteFileInDirectory(file, "new", expected), + ).rejects.toThrow("was replaced while it was being opened"); + }, + ); + + // Checked on the windows-latest leg: the device fallback above exists for + // a Node line this package does not support. If a supported line ever + // reports no device for a path, the fallback becomes the normal path, and + // this fails to say so. + it.skipIf(realPlatform !== "win32")( + "on a real Windows volume, the path stat and the handle agree on a nonzero device and the inode", + () => { + const file = path.join(dir, "f.txt"); + fs.writeFileSync(file, "original"); + const fd = fs.openSync(file, "r"); + try { + const handle = fs.fstatSync(fd, { bigint: true }); + const byPath = lstatBig(file); + expect(byPath.dev).not.toBe(0n); + expect(byPath.dev).toBe(handle.dev); + expect(byPath.ino).toBe(handle.ino); + } finally { + fs.closeSync(fd); + } + }, + ); +}); + describe("renameIntoDirectory", () => { it("moves the entry into the destination directory", () => { fs.mkdirSync(path.join(dir, "sub")); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index f1ad736b99..b148618e35 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -470,8 +470,11 @@ export async function writeFileContent( * directory on the path swapped for a symlink cannot redirect it; * - O_NOFOLLOW makes the open itself fail with ELOOP on a symlink (POSIX * only), and the opened handle must be the very file lstat saw (same device - * and inode), which catches a swap between the lstat and the open. Once - * the handle is verified, every write goes through it. + * and inode, compared as BigInts so a 64-bit file id cannot round; where + * Windows reports no device for the path, same inode, size and mtime: see + * isSameEntry in pinned-directory.ts, #5154), which catches a swap between + * the lstat and the open. Once the handle is verified, every write goes + * through it. * * The trade-off is crash-atomicity: a crash mid-write leaves a partly written * file, where the rename left either the old content or the new. @@ -480,7 +483,7 @@ async function overwriteInPlace( filePath: string, content: string, ): Promise { - const expected = await fs.lstat(filePath); + const expected = await fs.lstat(filePath, { bigint: true }); if (!expected.isFile()) { throw new Error(`Refusing to write: not a regular file: ${filePath}`); } diff --git a/src/filesystem/pinned-directory.ts b/src/filesystem/pinned-directory.ts index 97d20de48e..9cb15ec9f6 100644 --- a/src/filesystem/pinned-directory.ts +++ b/src/filesystem/pinned-directory.ts @@ -59,11 +59,39 @@ function directoryChangedError(directory: string): Error { ); } +/** What the identity check reads from a stat (always a BigInt stat). */ +type EntryIdentity = Pick; + +/** + * Whether `pathStat` (an lstat of the path) and `handleStat` (an fstat of the + * opened descriptor) describe the same file (#5154). + * + * Both are BigInt stats, so `ino` compares exactly: a number stat's `ino` is + * a double, and NTFS file ids above 2^53 round in it, so two different files + * could compare equal. + * + * Normally the file is the same when device and inode both match. On Windows + * with libuv < 1.51.0 (the Node 23 line), a path-based stat reports `dev` 0 + * while the handle reports the volume serial, so comparing devices would + * refuse every overwrite. There a 0n device on the path stat is treated as + * unknown rather than as a match: the file must then agree on inode, size and + * modification time (in nanoseconds), and any difference refuses. It never + * skips the check; a stale or changed size or time fails closed. + */ function isSameEntry( - left: { dev: number; ino: number }, - right: { dev: number; ino: number }, + pathStat: EntryIdentity, + handleStat: EntryIdentity, ): boolean { - return left.dev === right.dev && left.ino === right.ino; + if (pathStat.ino !== handleStat.ino) { + return false; + } + if (onWindows() && pathStat.dev === 0n) { + return ( + pathStat.size === handleStat.size && + pathStat.mtimeNs === handleStat.mtimeNs + ); + } + return pathStat.dev === handleStat.dev; } // The macOS Data volume, which firmlinks /Users, /private and the rest into @@ -179,7 +207,7 @@ function openInDirectory( filePath: string, flags: number, refuseLink: boolean, -): { fd: number; opened: fs.Stats } { +): { fd: number; opened: fs.BigIntStats } { const name = path.basename(filePath); return inDirectory(path.dirname(filePath), () => { if (refuseLink && fs.lstatSync(name).isSymbolicLink()) { @@ -187,8 +215,8 @@ function openInDirectory( } const fd = fs.openSync(name, flags, 0o666); try { - const opened = fs.fstatSync(fd); - if (!isSameEntry(fs.lstatSync(name), opened)) { + const opened = fs.fstatSync(fd, { bigint: true }); + if (!isSameEntry(fs.lstatSync(name, { bigint: true }), opened)) { throw directoryChangedError(path.dirname(filePath)); } return { fd, opened }; @@ -222,12 +250,12 @@ export async function createFileInDirectory( /** * Replace the content of the regular file at `filePath`, which must be the - * file `expected` describes, through a single descriptor. + * file `expected` (its BigInt lstat) describes, through a single descriptor. */ export async function overwriteFileInDirectory( filePath: string, content: string, - expected: { dev: number; ino: number }, + expected: EntryIdentity, ): Promise { // O_RDWR rather than O_WRONLY: opening a FIFO swapped in for writing only // would block until a reader arrives. O_NOFOLLOW fails the open with ELOOP @@ -238,7 +266,7 @@ export async function overwriteFileInDirectory( onWindows(), ); try { - if (!opened.isFile() || !isSameEntry(opened, expected)) { + if (!opened.isFile() || !isSameEntry(expected, opened)) { throw new Error( `Refusing to write: ${filePath} was replaced while it was being opened`, );