Skip to content

Commit 322dcf0

Browse files
cliffhallclaude
andcommitted
fix(filesystem): compare overwrite identity as BigInt stats, with a Windows no-device fallback
The identity check between the path lstat and the opened handle's fstat now takes both as BigInt stats, so NTFS file ids above 2^53 compare exactly. On win32, a path stat reporting dev 0n (libuv < 1.51.0) is treated as an unknown device: inode, size and mtimeNs must then all match, and any difference refuses. A windows-latest-only test pins the premise that a supported Node reports a nonzero device that agrees with the handle. Closes #5154 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: cliffhall <[email protected]>
1 parent 84b412b commit 322dcf0

5 files changed

Lines changed: 196 additions & 31 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@modelcontextprotocol/server-filesystem": patch
3+
---
4+
5+
`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‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -98,9 +98,15 @@ function createMockFileHandle(content: Buffer) {
9898
};
9999
}
100100

101-
// What lstat and the opened handle's stat report for the file an in-place
102-
// write replaces: a regular file, the same device and inode both times.
103-
const REGULAR_FILE = { isFile: () => true, dev: 1, ino: 42 } as Stats;
101+
// What lstat reports for the file an in-place write replaces: a regular file,
102+
// as a BigInt stat, the form the identity check compares exactly (#5154).
103+
const REGULAR_FILE = {
104+
isFile: () => true,
105+
dev: 1n,
106+
ino: 42n,
107+
size: 8n,
108+
mtimeNs: 1_700_000_000_000_000_000n,
109+
} as BigIntStats;
104110

105111
// The allow-list validatePath checks against; each server instance passes its
106112
// own (#4854), so the unit tests pass this one explicitly.
@@ -618,7 +624,9 @@ describe("Lib Functions", () => {
618624

619625
await writeFileContent("/test/script.sh", "new content");
620626

621-
expect(mockFs.lstat).toHaveBeenCalledWith("/test/script.sh");
627+
expect(mockFs.lstat).toHaveBeenCalledWith("/test/script.sh", {
628+
bigint: true,
629+
});
622630
expect(mockPinned.overwriteFileInDirectory).toHaveBeenCalledWith(
623631
"/test/script.sh",
624632
"new content",
@@ -638,7 +646,7 @@ describe("Lib Functions", () => {
638646
mockFs.lstat.mockResolvedValueOnce({
639647
...REGULAR_FILE,
640648
isFile: () => false,
641-
} as Stats);
649+
} as BigIntStats);
642650

643651
await expect(
644652
writeFileContent("/test/link.txt", "new content"),

‎src/filesystem/__tests__/pinned-directory.test.ts‎

Lines changed: 135 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// working-directory bookkeeping a borrowed chdir needs.
66

77
import { execFileSync } from "child_process";
8-
import fs from "fs";
8+
import fs, { type BigIntStats } from "fs";
99
import path from "path";
1010
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
1111
import {
@@ -20,6 +20,9 @@ import { makeTempDir } from "./helpers.js";
2020

2121
const symlinks = process.platform !== "win32";
2222
const realPlatform = process.platform;
23+
// The identity check compares BigInt stats (#5154), so every expected stat a
24+
// test hands it is one.
25+
const lstatBig = (p: string) => fs.lstatSync(p, { bigint: true });
2326
const errno = (code: string) =>
2427
Object.assign(new Error(`${code}: stand-in`), { code });
2528

@@ -144,8 +147,8 @@ describe("createFileInDirectory", () => {
144147
it("refuses, and closes the file, when the path no longer holds what was opened", async () => {
145148
const realLstat = fs.lstatSync;
146149
vi.spyOn(fs, "lstatSync").mockImplementationOnce((p) => ({
147-
...realLstat(p),
148-
ino: -1,
150+
...realLstat(p, { bigint: true }),
151+
ino: -1n,
149152
}));
150153
const closeSync = vi.spyOn(fs, "closeSync");
151154
await expect(
@@ -183,8 +186,8 @@ describe("createFileInDirectory", () => {
183186
fs.symlinkSync(outside, path.join(dir, "link"));
184187
const realLstat = fs.lstatSync;
185188
vi.spyOn(fs, "lstatSync").mockImplementationOnce((p) => ({
186-
...realLstat(p),
187-
ino: -1,
189+
...realLstat(p, { bigint: true }),
190+
ino: -1n,
188191
}));
189192
await expect(
190193
createFileInDirectory(path.join(dir, "link", "x.txt"), "x"),
@@ -197,16 +200,16 @@ describe("overwriteFileInDirectory", () => {
197200
it("replaces the content in place, keeping the inode", async () => {
198201
const file = path.join(dir, "f.txt");
199202
fs.writeFileSync(file, "a much longer original");
200-
const expected = fs.lstatSync(file);
203+
const expected = lstatBig(file);
201204
await overwriteFileInDirectory(file, "short", expected);
202205
expect(fs.readFileSync(file, "utf-8")).toBe("short");
203-
expect(fs.lstatSync(file).ino).toBe(expected.ino);
206+
expect(lstatBig(file).ino).toBe(expected.ino);
204207
});
205208

206209
it("refuses a file other than the one expected", async () => {
207210
const file = path.join(dir, "f.txt");
208211
fs.writeFileSync(file, "original");
209-
const expected = { ...fs.lstatSync(file), ino: -1 };
212+
const expected = { ...lstatBig(file), ino: -1n };
210213
await expect(
211214
overwriteFileInDirectory(file, "new", expected),
212215
).rejects.toThrow(
@@ -218,10 +221,10 @@ describe("overwriteFileInDirectory", () => {
218221
it("refuses something that is not a regular file", async () => {
219222
const file = path.join(dir, "f.txt");
220223
fs.writeFileSync(file, "original");
221-
const expected = fs.lstatSync(file);
224+
const expected = lstatBig(file);
222225
const realFstat = fs.fstatSync;
223226
vi.spyOn(fs, "fstatSync").mockImplementationOnce((fd) => {
224-
const stats = realFstat(fd);
227+
const stats = realFstat(fd, { bigint: true });
225228
return Object.assign(stats, { isFile: () => false });
226229
});
227230
await expect(
@@ -237,7 +240,7 @@ describe("overwriteFileInDirectory", () => {
237240
}) as typeof fs.ftruncate);
238241
const close = vi.spyOn(fs, "close");
239242
await expect(
240-
overwriteFileInDirectory(file, "new", fs.lstatSync(file)),
243+
overwriteFileInDirectory(file, "new", lstatBig(file)),
241244
).rejects.toThrow("EIO");
242245
expect(close).toHaveBeenCalled();
243246
expect(fs.readFileSync(file, "utf-8")).toBe("original");
@@ -249,7 +252,11 @@ describe("overwriteFileInDirectory", () => {
249252
const link = path.join(dir, "l.txt");
250253
fs.symlinkSync(target, link);
251254
await expect(
252-
overwriteFileInDirectory(link, "pwned", fs.statSync(target)),
255+
overwriteFileInDirectory(
256+
link,
257+
"pwned",
258+
fs.statSync(target, { bigint: true }),
259+
),
253260
).rejects.toMatchObject({ code: "ELOOP" });
254261
expect(fs.readFileSync(target, "utf-8")).toBe("SECRET");
255262
});
@@ -265,7 +272,11 @@ describe("on Windows, where O_NOFOLLOW does not exist", () => {
265272
const link = path.join(dir, "l.txt");
266273
fs.symlinkSync(target, link);
267274
await expect(
268-
overwriteFileInDirectory(link, "pwned", fs.statSync(target)),
275+
overwriteFileInDirectory(
276+
link,
277+
"pwned",
278+
fs.statSync(target, { bigint: true }),
279+
),
269280
).rejects.toThrow(`Refusing to write: not a regular file: ${link}`);
270281
expect(fs.readFileSync(target, "utf-8")).toBe("SECRET");
271282
},
@@ -275,11 +286,121 @@ describe("on Windows, where O_NOFOLLOW does not exist", () => {
275286
Object.defineProperty(process, "platform", { value: "win32" });
276287
const file = path.join(dir, "f.txt");
277288
fs.writeFileSync(file, "original");
278-
await overwriteFileInDirectory(file, "new", fs.lstatSync(file));
289+
await overwriteFileInDirectory(file, "new", lstatBig(file));
279290
expect(fs.readFileSync(file, "utf-8")).toBe("new");
280291
});
281292
});
282293

294+
describe("the identity check (#5154)", () => {
295+
it("compares inodes exactly, where a number stat would round a 64-bit file id", async () => {
296+
const file = path.join(dir, "f.txt");
297+
fs.writeFileSync(file, "original");
298+
const high = 2n ** 53n;
299+
// The premise: as doubles, these two file ids are one number.
300+
expect(Number(high + 1n)).toBe(Number(high));
301+
const expected = { ...lstatBig(file), ino: high };
302+
const realFstat = fs.fstatSync;
303+
const realLstat = fs.lstatSync;
304+
vi.spyOn(fs, "fstatSync").mockImplementationOnce((fd) => ({
305+
...realFstat(fd, { bigint: true }),
306+
isFile: () => true,
307+
ino: high + 1n,
308+
}));
309+
vi.spyOn(fs, "lstatSync").mockImplementationOnce((p) => ({
310+
...realLstat(p, { bigint: true }),
311+
ino: high + 1n,
312+
}));
313+
await expect(
314+
overwriteFileInDirectory(file, "new", expected),
315+
).rejects.toThrow("was replaced while it was being opened");
316+
expect(fs.readFileSync(file, "utf-8")).toBe("original");
317+
});
318+
319+
describe("on Windows, where a path stat can report no device (libuv < 1.51.0)", () => {
320+
beforeEach(() => {
321+
Object.defineProperty(process, "platform", { value: "win32" });
322+
});
323+
324+
it("overwrites when inode, size and mtime all match", async () => {
325+
const file = path.join(dir, "f.txt");
326+
fs.writeFileSync(file, "original");
327+
const expected = { ...lstatBig(file), dev: 0n };
328+
await overwriteFileInDirectory(file, "new", expected);
329+
expect(fs.readFileSync(file, "utf-8")).toBe("new");
330+
});
331+
332+
it("creates a file whose path stat reports no device", async () => {
333+
const realLstat = fs.lstatSync;
334+
vi.spyOn(fs, "lstatSync").mockImplementationOnce((p) => ({
335+
...realLstat(p, { bigint: true }),
336+
dev: 0n,
337+
}));
338+
const file = path.join(dir, "new.txt");
339+
await createFileInDirectory(file, "hello");
340+
expect(fs.readFileSync(file, "utf-8")).toBe("hello");
341+
});
342+
343+
it.each([
344+
["inode", (s: BigIntStats) => ({ ino: s.ino + 1n })],
345+
["size", (s: BigIntStats) => ({ size: s.size + 1n })],
346+
["mtime", (s: BigIntStats) => ({ mtimeNs: s.mtimeNs - 1n })],
347+
])("refuses when the %s differs", async (_field, change) => {
348+
const file = path.join(dir, "f.txt");
349+
fs.writeFileSync(file, "original");
350+
const stats = lstatBig(file);
351+
const expected = { ...stats, dev: 0n, ...change(stats) };
352+
await expect(
353+
overwriteFileInDirectory(file, "new", expected),
354+
).rejects.toThrow("was replaced while it was being opened");
355+
expect(fs.readFileSync(file, "utf-8")).toBe("original");
356+
});
357+
358+
it("refuses a known device that differs", async () => {
359+
const file = path.join(dir, "f.txt");
360+
fs.writeFileSync(file, "original");
361+
const stats = lstatBig(file);
362+
const expected = { ...stats, dev: stats.dev + 1n };
363+
await expect(
364+
overwriteFileInDirectory(file, "new", expected),
365+
).rejects.toThrow("was replaced while it was being opened");
366+
});
367+
});
368+
369+
it.skipIf(realPlatform === "win32")(
370+
"off Windows, refuses a path stat that reports no device",
371+
async () => {
372+
const file = path.join(dir, "f.txt");
373+
fs.writeFileSync(file, "original");
374+
const expected = { ...lstatBig(file), dev: 0n };
375+
await expect(
376+
overwriteFileInDirectory(file, "new", expected),
377+
).rejects.toThrow("was replaced while it was being opened");
378+
},
379+
);
380+
381+
// Checked on the windows-latest leg: the device fallback above exists for
382+
// a Node line this package does not support. If a supported line ever
383+
// reports no device for a path, the fallback becomes the normal path, and
384+
// this fails to say so.
385+
it.skipIf(realPlatform !== "win32")(
386+
"on a real Windows volume, the path stat and the handle agree on a nonzero device and the inode",
387+
() => {
388+
const file = path.join(dir, "f.txt");
389+
fs.writeFileSync(file, "original");
390+
const fd = fs.openSync(file, "r");
391+
try {
392+
const handle = fs.fstatSync(fd, { bigint: true });
393+
const byPath = lstatBig(file);
394+
expect(byPath.dev).not.toBe(0n);
395+
expect(byPath.dev).toBe(handle.dev);
396+
expect(byPath.ino).toBe(handle.ino);
397+
} finally {
398+
fs.closeSync(fd);
399+
}
400+
},
401+
);
402+
});
403+
283404
describe("renameIntoDirectory", () => {
284405
it("moves the entry into the destination directory", () => {
285406
fs.mkdirSync(path.join(dir, "sub"));

‎src/filesystem/lib.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -470,8 +470,11 @@ export async function writeFileContent(
470470
* directory on the path swapped for a symlink cannot redirect it;
471471
* - O_NOFOLLOW makes the open itself fail with ELOOP on a symlink (POSIX
472472
* only), and the opened handle must be the very file lstat saw (same device
473-
* and inode), which catches a swap between the lstat and the open. Once
474-
* the handle is verified, every write goes through it.
473+
* and inode, compared as BigInts so a 64-bit file id cannot round; where
474+
* Windows reports no device for the path, same inode, size and mtime: see
475+
* isSameEntry in pinned-directory.ts, #5154), which catches a swap between
476+
* the lstat and the open. Once the handle is verified, every write goes
477+
* through it.
475478
*
476479
* The trade-off is crash-atomicity: a crash mid-write leaves a partly written
477480
* file, where the rename left either the old content or the new.
@@ -480,7 +483,7 @@ async function overwriteInPlace(
480483
filePath: string,
481484
content: string,
482485
): Promise<void> {
483-
const expected = await fs.lstat(filePath);
486+
const expected = await fs.lstat(filePath, { bigint: true });
484487
if (!expected.isFile()) {
485488
throw new Error(`Refusing to write: not a regular file: ${filePath}`);
486489
}

‎src/filesystem/pinned-directory.ts‎

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,39 @@ function directoryChangedError(directory: string): Error {
5959
);
6060
}
6161

62+
/** What the identity check reads from a stat (always a BigInt stat). */
63+
type EntryIdentity = Pick<fs.BigIntStats, "dev" | "ino" | "size" | "mtimeNs">;
64+
65+
/**
66+
* Whether `pathStat` (an lstat of the path) and `handleStat` (an fstat of the
67+
* opened descriptor) describe the same file (#5154).
68+
*
69+
* Both are BigInt stats, so `ino` compares exactly: a number stat's `ino` is
70+
* a double, and NTFS file ids above 2^53 round in it, so two different files
71+
* could compare equal.
72+
*
73+
* Normally the file is the same when device and inode both match. On Windows
74+
* with libuv < 1.51.0 (the Node 23 line), a path-based stat reports `dev` 0
75+
* while the handle reports the volume serial, so comparing devices would
76+
* refuse every overwrite. There a 0n device on the path stat is treated as
77+
* unknown rather than as a match: the file must then agree on inode, size and
78+
* modification time (in nanoseconds), and any difference refuses. It never
79+
* skips the check; a stale or changed size or time fails closed.
80+
*/
6281
function isSameEntry(
63-
left: { dev: number; ino: number },
64-
right: { dev: number; ino: number },
82+
pathStat: EntryIdentity,
83+
handleStat: EntryIdentity,
6584
): boolean {
66-
return left.dev === right.dev && left.ino === right.ino;
85+
if (pathStat.ino !== handleStat.ino) {
86+
return false;
87+
}
88+
if (onWindows() && pathStat.dev === 0n) {
89+
return (
90+
pathStat.size === handleStat.size &&
91+
pathStat.mtimeNs === handleStat.mtimeNs
92+
);
93+
}
94+
return pathStat.dev === handleStat.dev;
6795
}
6896

6997
// The macOS Data volume, which firmlinks /Users, /private and the rest into
@@ -179,16 +207,16 @@ function openInDirectory(
179207
filePath: string,
180208
flags: number,
181209
refuseLink: boolean,
182-
): { fd: number; opened: fs.Stats } {
210+
): { fd: number; opened: fs.BigIntStats } {
183211
const name = path.basename(filePath);
184212
return inDirectory(path.dirname(filePath), () => {
185213
if (refuseLink && fs.lstatSync(name).isSymbolicLink()) {
186214
throw new Error(`Refusing to write: not a regular file: ${filePath}`);
187215
}
188216
const fd = fs.openSync(name, flags, 0o666);
189217
try {
190-
const opened = fs.fstatSync(fd);
191-
if (!isSameEntry(fs.lstatSync(name), opened)) {
218+
const opened = fs.fstatSync(fd, { bigint: true });
219+
if (!isSameEntry(fs.lstatSync(name, { bigint: true }), opened)) {
192220
throw directoryChangedError(path.dirname(filePath));
193221
}
194222
return { fd, opened };
@@ -222,12 +250,12 @@ export async function createFileInDirectory(
222250

223251
/**
224252
* Replace the content of the regular file at `filePath`, which must be the
225-
* file `expected` describes, through a single descriptor.
253+
* file `expected` (its BigInt lstat) describes, through a single descriptor.
226254
*/
227255
export async function overwriteFileInDirectory(
228256
filePath: string,
229257
content: string,
230-
expected: { dev: number; ino: number },
258+
expected: EntryIdentity,
231259
): Promise<void> {
232260
// O_RDWR rather than O_WRONLY: opening a FIFO swapped in for writing only
233261
// would block until a reader arrives. O_NOFOLLOW fails the open with ELOOP
@@ -238,7 +266,7 @@ export async function overwriteFileInDirectory(
238266
onWindows(),
239267
);
240268
try {
241-
if (!opened.isFile() || !isSameEntry(opened, expected)) {
269+
if (!opened.isFile() || !isSameEntry(expected, opened)) {
242270
throw new Error(
243271
`Refusing to write: ${filePath} was replaced while it was being opened`,
244272
);

0 commit comments

Comments
 (0)