fix(filesystem): compare overwrite identity as BigInt stats, with a Windows no-device fallback

.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.

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

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"));

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<void> {
-  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}`);
   }

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<fs.BigIntStats, "dev" | "ino" | "size" | "mtimeNs">;
+
+/**
+ * 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,16 +207,16 @@ 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()) {
       throw new Error(`Refusing to write: not a regular file: ${filePath}`);
     }
     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<void> {
   // 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`,
       );