Harden file explorer symlink handling (#847)

2026-05-10 13:14:50 +00:00
parent 3f5acfff31
commit 25d4c5023a
3 changed files with 162 additions and 76 deletions
@@ -1,7 +1,7 @@
 import express from "express";
 import { createServer as createHTTPServer, type IncomingMessage, type ServerResponse } from "http";
-import { createReadStream, unlinkSync, existsSync } from "fs";
-import { stat } from "fs/promises";
+import { constants, existsSync, unlinkSync } from "fs";
+import { open } from "fs/promises";
 import { randomUUID } from "node:crypto";
 import { hostname as getHostname } from "node:os";
 import path from "node:path";
@@ -143,6 +143,8 @@ type AgentMcpTransportMap = Map<string, StreamableHTTPServerTransport>;

 const MAX_MCP_DEBUG_BATCH_ITEMS = 10;
 const REDACTED_LOG_VALUE = "[redacted]";
+const DOWNLOAD_OPEN_FLAGS =
+  process.platform === "win32" ? constants.O_RDONLY : constants.O_RDONLY | constants.O_NOFOLLOW;

 function formatHostForHttpUrl(host: string): string {
  return host.includes(":") && !host.startsWith("[") ? `[${host}]` : host;
@@ -415,8 +417,10 @@ export async function createPaseoDaemon(
      return;
    }

+    let fileHandle: Awaited<ReturnType<typeof open>> | null = null;
    try {
-      const fileStats = await stat(entry.absolutePath);
+      fileHandle = await open(entry.absolutePath, DOWNLOAD_OPEN_FLAGS);
+      const fileStats = await fileHandle.stat();
      if (!fileStats.isFile()) {
        res.status(404).json({ error: "File not found" });
        return;
@@ -425,9 +429,10 @@ export async function createPaseoDaemon(
      const safeFileName = entry.fileName.replace(/["\r\n]/g, "_");
      res.setHeader("Content-Type", entry.mimeType);
      res.setHeader("Content-Disposition", `attachment; filename="${safeFileName}"`);
-      res.setHeader("Content-Length", entry.size.toString());
+      res.setHeader("Content-Length", fileStats.size.toString());

-      const stream = createReadStream(entry.absolutePath);
+      const stream = fileHandle.createReadStream();
+      fileHandle = null;
      stream.on("error", (err) => {
        logger.error({ err }, "Failed to stream download");
        if (!res.headersSent) {
@@ -442,6 +447,8 @@ export async function createPaseoDaemon(
      if (!res.headersSent) {
        res.status(404).json({ error: "File not found" });
      }
+    } finally {
+      await fileHandle?.close().catch(() => undefined);
    }
  };

@@ -1,10 +1,10 @@
 // POSIX-only: symlink fixtures
 /* eslint-disable max-nested-callbacks */
-import { mkdtemp, mkdir, rm, symlink, writeFile } from "node:fs/promises";
+import { mkdtemp, mkdir, realpath, rm, symlink, writeFile } from "node:fs/promises";
 import os from "node:os";
 import path from "node:path";
 import { describe, expect, it } from "vitest";
-import { listDirectoryEntries, readExplorerFile } from "./service.js";
+import { getDownloadableFileInfo, listDirectoryEntries, readExplorerFile } from "./service.js";
 import { isPlatform } from "../../test-utils/platform.js";

 async function createTempDir(prefix: string): Promise<string> {
@@ -55,4 +55,53 @@ describe.skipIf(isPlatform("win32"))("service POSIX-only", () => {
      await rm(outsideRoot, { recursive: true, force: true });
    }
  });
+
+  it("skips listed symlink entries that resolve outside the workspace", async () => {
+    const root = await createTempDir("paseo-file-explorer-");
+    const outsideRoot = await createTempDir("paseo-file-explorer-outside-");
+
+    try {
+      await writeFile(path.join(root, "visible.txt"), "visible\n", "utf-8");
+      const externalFile = path.join(outsideRoot, "secret.txt");
+      await writeFile(externalFile, "top secret\n", "utf-8");
+      await symlink(externalFile, path.join(root, "secret-link.txt"));
+
+      const result = await listDirectoryEntries({ root });
+
+      const names = result.entries.map((entry) => entry.name);
+      expect(names).toContain("visible.txt");
+      expect(names).not.toContain("secret-link.txt");
+    } finally {
+      await rm(root, { recursive: true, force: true });
+      await rm(outsideRoot, { recursive: true, force: true });
+    }
+  });
+
+  it("uses canonical paths for downloadable symlink targets inside the workspace", async () => {
+    const root = await createTempDir("paseo-file-explorer-");
+
+    try {
+      const target = path.join(root, "safe.txt");
+      const link = path.join(root, "safe-link.txt");
+      await writeFile(target, "safe\n", "utf-8");
+      await symlink("safe.txt", link);
+
+      const file = await readExplorerFile({
+        root,
+        relativePath: "safe-link.txt",
+      });
+      const info = await getDownloadableFileInfo({
+        root,
+        relativePath: "safe-link.txt",
+      });
+
+      expect(file.path).toBe("safe-link.txt");
+      expect(file.content).toBe("safe\n");
+      expect(info.path).toBe("safe-link.txt");
+      expect(info.fileName).toBe("safe-link.txt");
+      expect(info.absolutePath).toBe(await realpath(target));
+    } finally {
+      await rm(root, { recursive: true, force: true });
+    }
+  });
 });
@@ -1,4 +1,5 @@
-import { promises as fs } from "fs";
+import { constants, promises as fs } from "fs";
+import type { FileHandle } from "fs/promises";
 import path from "path";
 import { resolvePathFromBase } from "../path-utils.js";

@@ -54,6 +55,10 @@ const TEXT_MIME_TYPES: Record<string, string> = {
 };

 const DEFAULT_TEXT_MIME_TYPE = "text/plain";
+const FILE_TYPE_SAMPLE_BYTES = 8192;
+const READ_FILE_OPEN_FLAGS =
+  process.platform === "win32" ? constants.O_RDONLY : constants.O_RDONLY | constants.O_NOFOLLOW;
+const ACCESS_OUTSIDE_WORKSPACE_MESSAGE = "Access outside of workspace is not allowed";

 const IMAGE_MIME_TYPES: Record<string, string> = {
  ".png": "image/png",
@@ -69,6 +74,11 @@ interface ScopedPathParams {
  relativePath?: string;
 }

+interface ScopedPath {
+  requestedPath: string;
+  resolvedPath: string;
+}
+
 interface EntryPayloadParams {
  root: string;
  targetPath: string;
@@ -81,17 +91,17 @@ export async function listDirectoryEntries({
  relativePath = ".",
 }: ListDirectoryParams): Promise<FileExplorerDirectory> {
  const directoryPath = await resolveScopedPath({ root, relativePath });
-  const stats = await fs.stat(directoryPath);
+  const stats = await fs.stat(directoryPath.resolvedPath);

  if (!stats.isDirectory()) {
    throw new Error("Requested path is not a directory");
  }

-  const dirents = await fs.readdir(directoryPath, { withFileTypes: true });
+  const dirents = await fs.readdir(directoryPath.resolvedPath, { withFileTypes: true });

  const entriesWithNulls = await Promise.all(
    dirents.map(async (dirent) => {
-      const targetPath = path.join(directoryPath, dirent.name);
+      const targetPath = path.join(directoryPath.requestedPath, dirent.name);
      const kind: ExplorerEntryKind = dirent.isDirectory() ? "directory" : "file";
      try {
        return await buildEntryPayload({
@@ -103,7 +113,7 @@ export async function listDirectoryEntries({
      } catch (error) {
        // Directories can contain dangling links (e.g. AGENTS.md -> CLAUDE.md).
        // Skip entries whose targets disappeared instead of failing the whole listing.
-        if (isMissingEntryError(error)) {
+        if (isMissingEntryError(error) || isOutsideWorkspaceError(error)) {
          return null;
        }
        throw error;
@@ -121,7 +131,7 @@ export async function listDirectoryEntries({
  });

  return {
-    path: normalizeRelativePath({ root, targetPath: directoryPath }),
+    path: normalizeRelativePath({ root, targetPath: directoryPath.requestedPath }),
    entries,
  };
 }
@@ -171,48 +181,53 @@ export async function readExplorerFileBytes({
  relativePath,
 }: ReadFileParams): Promise<FileExplorerFileBytes> {
  const filePath = await resolveScopedPath({ root, relativePath });
-  const stats = await fs.stat(filePath);
+  const handle = await openFileForRead(filePath.resolvedPath);

-  if (!stats.isFile()) {
-    throw new Error("Requested path is not a file");
-  }
+  try {
+    const stats = await handle.stat();

-  const ext = path.extname(filePath).toLowerCase();
-  const basePayload = {
-    path: normalizeRelativePath({ root, targetPath: filePath }),
-    size: stats.size,
-    modifiedAt: stats.mtime.toISOString(),
-  };
+    if (!stats.isFile()) {
+      throw new Error("Requested path is not a file");
+    }
+
+    const ext = path.extname(filePath.resolvedPath).toLowerCase();
+    const basePayload = {
+      path: normalizeRelativePath({ root, targetPath: filePath.requestedPath }),
+      size: stats.size,
+      modifiedAt: stats.mtime.toISOString(),
+    };
+
+    const buffer = await handle.readFile();
+    if (ext in IMAGE_MIME_TYPES) {
+      return {
+        ...basePayload,
+        kind: "image",
+        encoding: "binary",
+        bytes: buffer,
+        mimeType: IMAGE_MIME_TYPES[ext],
+      };
+    }
+
+    if (isLikelyBinary(buffer)) {
+      return {
+        ...basePayload,
+        kind: "binary",
+        encoding: "binary",
+        bytes: buffer,
+        mimeType: "application/octet-stream",
+      };
+    }

-  if (ext in IMAGE_MIME_TYPES) {
-    const buffer = await fs.readFile(filePath);
    return {
      ...basePayload,
-      kind: "image",
-      encoding: "binary",
+      kind: "text",
+      encoding: "utf-8",
      bytes: buffer,
-      mimeType: IMAGE_MIME_TYPES[ext],
+      mimeType: textMimeTypeForExtension(ext),
    };
+  } finally {
+    await handle.close();
  }
-
-  const buffer = await fs.readFile(filePath);
-  if (isLikelyBinary(buffer)) {
-    return {
-      ...basePayload,
-      kind: "binary",
-      encoding: "binary",
-      bytes: buffer,
-      mimeType: "application/octet-stream",
-    };
-  }
-
-  return {
-    ...basePayload,
-    kind: "text",
-    encoding: "utf-8",
-    bytes: buffer,
-    mimeType: textMimeTypeForExtension(ext),
-  };
 }

 export async function getDownloadableFileInfo({ root, relativePath }: ReadFileParams): Promise<{
@@ -223,47 +238,50 @@ export async function getDownloadableFileInfo({ root, relativePath }: ReadFilePa
  size: number;
 }> {
  const filePath = await resolveScopedPath({ root, relativePath });
-  const stats = await fs.stat(filePath);
+  const handle = await openFileForRead(filePath.resolvedPath);

-  if (!stats.isFile()) {
-    throw new Error("Requested path is not a file");
-  }
+  try {
+    const stats = await handle.stat();

-  const ext = path.extname(filePath).toLowerCase();
-  let mimeType = "application/octet-stream";
-  if (ext in IMAGE_MIME_TYPES) {
-    mimeType = IMAGE_MIME_TYPES[ext];
-  } else {
-    // Read only a small prefix to classify likely text vs binary.
-    const handle = await fs.open(filePath, "r");
-    const sample = Buffer.alloc(8192);
-    try {
+    if (!stats.isFile()) {
+      throw new Error("Requested path is not a file");
+    }
+
+    const ext = path.extname(filePath.resolvedPath).toLowerCase();
+    let mimeType = "application/octet-stream";
+    if (ext in IMAGE_MIME_TYPES) {
+      mimeType = IMAGE_MIME_TYPES[ext];
+    } else {
+      const sample = Buffer.alloc(FILE_TYPE_SAMPLE_BYTES);
      const { bytesRead } = await handle.read(sample, 0, sample.length, 0);
      const chunk = bytesRead < sample.length ? sample.subarray(0, bytesRead) : sample;
      if (!isLikelyBinary(chunk)) {
        mimeType = textMimeTypeForExtension(ext);
      }
-    } finally {
-      await handle.close();
    }
-  }

-  return {
-    path: normalizeRelativePath({ root, targetPath: filePath }),
-    absolutePath: filePath,
-    fileName: path.basename(filePath),
-    mimeType,
-    size: stats.size,
-  };
+    return {
+      path: normalizeRelativePath({ root, targetPath: filePath.requestedPath }),
+      absolutePath: filePath.resolvedPath,
+      fileName: path.basename(filePath.requestedPath),
+      mimeType,
+      size: stats.size,
+    };
+  } finally {
+    await handle.close();
+  }
 }

-async function resolveScopedPath({ root, relativePath = "." }: ScopedPathParams): Promise<string> {
+async function resolveScopedPath({
+  root,
+  relativePath = ".",
+}: ScopedPathParams): Promise<ScopedPath> {
  const normalizedRoot = path.resolve(root);
  const requestedPath = resolvePathFromBase(normalizedRoot, relativePath);
  const relative = path.relative(normalizedRoot, requestedPath);

  if (relative !== "" && (relative.startsWith("..") || path.isAbsolute(relative))) {
-    throw new Error("Access outside of workspace is not allowed");
+    throw new Error(ACCESS_OUTSIDE_WORKSPACE_MESSAGE);
  }

  const realRoot = await fs.realpath(normalizedRoot);
@@ -272,24 +290,32 @@ async function resolveScopedPath({ root, relativePath = "." }: ScopedPathParams)
    const realPath = await fs.realpath(requestedPath);
    const realRelative = path.relative(realRoot, realPath);
    if (realRelative !== "" && (realRelative.startsWith("..") || path.isAbsolute(realRelative))) {
-      throw new Error("Access outside of workspace is not allowed");
+      throw new Error(ACCESS_OUTSIDE_WORKSPACE_MESSAGE);
    }
-    return requestedPath;
+    return { requestedPath, resolvedPath: realPath };
  } catch (error) {
    if (isMissingEntryError(error)) {
-      return requestedPath;
+      return { requestedPath, resolvedPath: requestedPath };
    }
    throw error;
  }
 }

+async function openFileForRead(filePath: string): Promise<FileHandle> {
+  return fs.open(filePath, READ_FILE_OPEN_FLAGS);
+}
+
 async function buildEntryPayload({
  root,
  targetPath,
  name,
  kind,
 }: EntryPayloadParams): Promise<FileExplorerEntry> {
-  const stats = await fs.stat(targetPath);
+  const entryPath = await resolveScopedPath({
+    root,
+    relativePath: normalizeRelativePath({ root, targetPath }),
+  });
+  const stats = await fs.stat(entryPath.resolvedPath);
  return {
    name,
    path: normalizeRelativePath({ root, targetPath }),
@@ -304,6 +330,10 @@ function isMissingEntryError(error: unknown): boolean {
  return code === "ENOENT" || code === "ENOTDIR" || code === "ELOOP";
 }

+function isOutsideWorkspaceError(error: unknown): boolean {
+  return error instanceof Error && error.message === ACCESS_OUTSIDE_WORKSPACE_MESSAGE;
+}
+
 function normalizeRelativePath({ root, targetPath }: { root: string; targetPath: string }): string {
  const normalizedRoot = path.resolve(root);
  const normalizedTarget = path.resolve(targetPath);