fix(filesystem): GHSA-p9cv-8g75-4xcg

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

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)
 

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

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,13 +32,17 @@ 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");
 // The pinned-directory writes act on the real filesystem; their own suite
 // 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,18 +1111,16 @@ 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);
 
         expect(result).toBe(`界${"a".repeat(1017)}\nlast`);
       });
 
       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);
 

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);
+  });
+});