From 5eee991913bc8410775168b647aab9f87e45a1d3 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 26 Jan 2026 20:05:03 +0000 Subject: [PATCH] fix: harden file serving --- src/canvas-host/server.ts | 50 ++++++++++-------- src/infra/fs-safe.ts | 103 ++++++++++++++++++++++++++++++++++++++ src/media/server.test.ts | 36 +++++++++++-- src/media/server.ts | 52 +++++++++++++------ src/media/store.ts | 22 ++++---- 5 files changed, 213 insertions(+), 50 deletions(-) create mode 100644 src/infra/fs-safe.ts diff --git a/src/canvas-host/server.ts b/src/canvas-host/server.ts index b04d0d5ff..7cde546b8 100644 --- a/src/canvas-host/server.ts +++ b/src/canvas-host/server.ts @@ -8,6 +8,7 @@ import type { Duplex } from "node:stream"; import chokidar from "chokidar"; import { type WebSocket, WebSocketServer } from "ws"; import { isTruthyEnvValue } from "../infra/env.js"; +import { SafeOpenError, openFileWithinRoot } from "../infra/fs-safe.js"; import { detectMime } from "../media/mime.js"; import type { RuntimeEnv } from "../runtime.js"; import { ensureDir, resolveUserPath } from "../utils.js"; @@ -145,30 +146,31 @@ async function resolveFilePath(rootReal: string, urlPath: string) { const rel = normalized.replace(/^\/+/, ""); if (rel.split("/").some((p) => p === "..")) return null; - let candidate = path.join(rootReal, rel); + const tryOpen = async (relative: string) => { + try { + return await openFileWithinRoot({ rootDir: rootReal, relativePath: relative }); + } catch (err) { + if (err instanceof SafeOpenError) return null; + throw err; + } + }; + if (normalized.endsWith("/")) { - candidate = path.join(candidate, "index.html"); + return await tryOpen(path.posix.join(rel, "index.html")); } + const candidate = path.join(rootReal, rel); try { - const st = await fs.stat(candidate); + const st = await fs.lstat(candidate); + if (st.isSymbolicLink()) return null; if (st.isDirectory()) { - candidate = path.join(candidate, "index.html"); + return await tryOpen(path.posix.join(rel, "index.html")); } } catch { // ignore } - const rootPrefix = rootReal.endsWith(path.sep) ? rootReal : `${rootReal}${path.sep}`; - try { - const lstat = await fs.lstat(candidate); - if (lstat.isSymbolicLink()) return null; - const real = await fs.realpath(candidate); - if (!real.startsWith(rootPrefix)) return null; - return real; - } catch { - return null; - } + return await tryOpen(rel); } function isDisabledByEnv() { @@ -311,8 +313,8 @@ export async function createCanvasHostHandler( return true; } - const filePath = await resolveFilePath(rootReal, urlPath); - if (!filePath) { + const opened = await resolveFilePath(rootReal, urlPath); + if (!opened) { if (urlPath === "/" || urlPath.endsWith("/")) { res.statusCode = 404; res.setHeader("Content-Type", "text/html; charset=utf-8"); @@ -327,22 +329,30 @@ export async function createCanvasHostHandler( return true; } - const lower = filePath.toLowerCase(); + const { handle, realPath } = opened; + let data: Buffer; + try { + data = await handle.readFile(); + } finally { + await handle.close().catch(() => {}); + } + + const lower = realPath.toLowerCase(); const mime = lower.endsWith(".html") || lower.endsWith(".htm") ? "text/html" - : ((await detectMime({ filePath })) ?? "application/octet-stream"); + : ((await detectMime({ filePath: realPath })) ?? "application/octet-stream"); res.setHeader("Cache-Control", "no-store"); if (mime === "text/html") { - const html = await fs.readFile(filePath, "utf8"); + const html = data.toString("utf8"); res.setHeader("Content-Type", "text/html; charset=utf-8"); res.end(liveReload ? injectCanvasLiveReload(html) : html); return true; } res.setHeader("Content-Type", mime); - res.end(await fs.readFile(filePath)); + res.end(data); return true; } catch (err) { opts.runtime.error(`canvasHost request failed: ${String(err)}`); diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts new file mode 100644 index 000000000..52a94b46f --- /dev/null +++ b/src/infra/fs-safe.ts @@ -0,0 +1,103 @@ +import { constants as fsConstants } from "node:fs"; +import type { Stats } from "node:fs"; +import type { FileHandle } from "node:fs/promises"; +import fs from "node:fs/promises"; +import path from "node:path"; + +export type SafeOpenErrorCode = "invalid-path" | "not-found"; + +export class SafeOpenError extends Error { + code: SafeOpenErrorCode; + + constructor(code: SafeOpenErrorCode, message: string) { + super(message); + this.code = code; + this.name = "SafeOpenError"; + } +} + +export type SafeOpenResult = { + handle: FileHandle; + realPath: string; + stat: Stats; +}; + +const NOT_FOUND_CODES = new Set(["ENOENT", "ENOTDIR"]); + +const ensureTrailingSep = (value: string) => (value.endsWith(path.sep) ? value : value + path.sep); + +const isNodeError = (err: unknown): err is NodeJS.ErrnoException => + Boolean(err && typeof err === "object" && "code" in (err as Record)); + +const isNotFoundError = (err: unknown) => + isNodeError(err) && typeof err.code === "string" && NOT_FOUND_CODES.has(err.code); + +const isSymlinkOpenError = (err: unknown) => + isNodeError(err) && (err.code === "ELOOP" || err.code === "EINVAL" || err.code === "ENOTSUP"); + +export async function openFileWithinRoot(params: { + rootDir: string; + relativePath: string; +}): Promise { + let rootReal: string; + try { + rootReal = await fs.realpath(params.rootDir); + } catch (err) { + if (isNotFoundError(err)) { + throw new SafeOpenError("not-found", "root dir not found"); + } + throw err; + } + const rootWithSep = ensureTrailingSep(rootReal); + const resolved = path.resolve(rootWithSep, params.relativePath); + if (!resolved.startsWith(rootWithSep)) { + throw new SafeOpenError("invalid-path", "path escapes root"); + } + + const supportsNoFollow = process.platform !== "win32" && "O_NOFOLLOW" in fsConstants; + const flags = fsConstants.O_RDONLY | (supportsNoFollow ? (fsConstants.O_NOFOLLOW as number) : 0); + + let handle: FileHandle; + try { + handle = await fs.open(resolved, flags); + } catch (err) { + if (isNotFoundError(err)) { + throw new SafeOpenError("not-found", "file not found"); + } + if (isSymlinkOpenError(err)) { + throw new SafeOpenError("invalid-path", "symlink open blocked"); + } + throw err; + } + + try { + const lstat = await fs.lstat(resolved).catch(() => null); + if (lstat?.isSymbolicLink()) { + throw new SafeOpenError("invalid-path", "symlink not allowed"); + } + + const realPath = await fs.realpath(resolved); + if (!realPath.startsWith(rootWithSep)) { + throw new SafeOpenError("invalid-path", "path escapes root"); + } + + const stat = await handle.stat(); + if (!stat.isFile()) { + throw new SafeOpenError("invalid-path", "not a file"); + } + + const realStat = await fs.stat(realPath); + if (stat.ino !== realStat.ino || stat.dev !== realStat.dev) { + throw new SafeOpenError("invalid-path", "path mismatch"); + } + + return { handle, realPath, stat }; + } catch (err) { + await handle.close().catch(() => {}); + if (err instanceof SafeOpenError) throw err; + if (isNotFoundError(err)) { + throw new SafeOpenError("not-found", "file not found"); + } + throw err; + } +} diff --git a/src/media/server.test.ts b/src/media/server.test.ts index 693ba5940..34182c5f2 100644 --- a/src/media/server.test.ts +++ b/src/media/server.test.ts @@ -7,12 +7,17 @@ import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; const MEDIA_DIR = path.join(process.cwd(), "tmp-media-test"); const cleanOldMedia = vi.fn().mockResolvedValue(undefined); -vi.mock("./store.js", () => ({ - getMediaDir: () => MEDIA_DIR, - cleanOldMedia, -})); +vi.mock("./store.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getMediaDir: () => MEDIA_DIR, + cleanOldMedia, + }; +}); const { startMediaServer } = await import("./server.js"); +const { MEDIA_MAX_BYTES } = await import("./store.js"); const waitForFileRemoval = async (file: string, timeoutMs = 200) => { const start = Date.now(); @@ -84,4 +89,27 @@ describe("media server", () => { expect(await res.text()).toBe("invalid path"); await new Promise((r) => server.close(r)); }); + + it("rejects invalid media ids", async () => { + const file = path.join(MEDIA_DIR, "file2"); + await fs.writeFile(file, "hello"); + const server = await startMediaServer(0, 5_000); + const port = (server.address() as AddressInfo).port; + const res = await fetch(`http://localhost:${port}/media/invalid%20id`); + expect(res.status).toBe(400); + expect(await res.text()).toBe("invalid path"); + await new Promise((r) => server.close(r)); + }); + + it("rejects oversized media files", async () => { + const file = path.join(MEDIA_DIR, "big"); + await fs.writeFile(file, ""); + await fs.truncate(file, MEDIA_MAX_BYTES + 1); + const server = await startMediaServer(0, 5_000); + const port = (server.address() as AddressInfo).port; + const res = await fetch(`http://localhost:${port}/media/big`); + expect(res.status).toBe(413); + expect(await res.text()).toBe("too large"); + await new Promise((r) => server.close(r)); + }); }); diff --git a/src/media/server.ts b/src/media/server.ts index e0af8b908..4791352d8 100644 --- a/src/media/server.ts +++ b/src/media/server.ts @@ -1,13 +1,23 @@ import fs from "node:fs/promises"; import type { Server } from "node:http"; -import path from "node:path"; import express, { type Express } from "express"; import { danger } from "../globals.js"; import { defaultRuntime, type RuntimeEnv } from "../runtime.js"; +import { SafeOpenError, openFileWithinRoot } from "../infra/fs-safe.js"; import { detectMime } from "./mime.js"; -import { cleanOldMedia, getMediaDir } from "./store.js"; +import { cleanOldMedia, getMediaDir, MEDIA_MAX_BYTES } from "./store.js"; const DEFAULT_TTL_MS = 2 * 60 * 1000; +const MAX_MEDIA_ID_CHARS = 200; +const MEDIA_ID_PATTERN = /^[\p{L}\p{N}._-]+$/u; +const MAX_MEDIA_BYTES = MEDIA_MAX_BYTES; + +const isValidMediaId = (id: string) => { + if (!id) return false; + if (id.length > MAX_MEDIA_ID_CHARS) return false; + if (id === "." || id === "..") return false; + return MEDIA_ID_PATTERN.test(id); +}; export function attachMediaRoutes( app: Express, @@ -18,26 +28,28 @@ export function attachMediaRoutes( app.get("/media/:id", async (req, res) => { const id = req.params.id; - const mediaRoot = (await fs.realpath(mediaDir)) + path.sep; - const file = path.resolve(mediaRoot, id); + if (!isValidMediaId(id)) { + res.status(400).send("invalid path"); + return; + } try { - const lstat = await fs.lstat(file); - if (lstat.isSymbolicLink()) { - res.status(400).send("invalid path"); + const { handle, realPath, stat } = await openFileWithinRoot({ + rootDir: mediaDir, + relativePath: id, + }); + if (stat.size > MAX_MEDIA_BYTES) { + await handle.close().catch(() => {}); + res.status(413).send("too large"); return; } - const realPath = await fs.realpath(file); - if (!realPath.startsWith(mediaRoot)) { - res.status(400).send("invalid path"); - return; - } - const stat = await fs.stat(realPath); if (Date.now() - stat.mtimeMs > ttlMs) { + await handle.close().catch(() => {}); await fs.rm(realPath).catch(() => {}); res.status(410).send("expired"); return; } - const data = await fs.readFile(realPath); + const data = await handle.readFile(); + await handle.close().catch(() => {}); const mime = await detectMime({ buffer: data, filePath: realPath }); if (mime) res.type(mime); res.send(data); @@ -47,7 +59,17 @@ export function attachMediaRoutes( fs.rm(realPath).catch(() => {}); }, 50); }); - } catch { + } catch (err) { + if (err instanceof SafeOpenError) { + if (err.code === "invalid-path") { + res.status(400).send("invalid path"); + return; + } + if (err.code === "not-found") { + res.status(404).send("not found"); + return; + } + } res.status(404).send("not found"); } }); diff --git a/src/media/store.ts b/src/media/store.ts index c24614016..268937084 100644 --- a/src/media/store.ts +++ b/src/media/store.ts @@ -10,7 +10,8 @@ import { resolvePinnedHostname } from "../infra/net/ssrf.js"; import { detectMime, extensionForMime } from "./mime.js"; const resolveMediaDir = () => path.join(resolveConfigDir(), "media"); -const MAX_BYTES = 5 * 1024 * 1024; // 5MB default +export const MEDIA_MAX_BYTES = 5 * 1024 * 1024; // 5MB default +const MAX_BYTES = MEDIA_MAX_BYTES; const DEFAULT_TTL_MS = 2 * 60 * 1000; // 2 minutes /** @@ -19,10 +20,9 @@ const DEFAULT_TTL_MS = 2 * 60 * 1000; // 2 minutes * Keeps: alphanumeric, dots, hyphens, underscores, Unicode letters/numbers. */ function sanitizeFilename(name: string): string { - // Remove: < > : " / \ | ? * and control chars (U+0000-U+001F) - // oxlint-disable-next-line no-control-regex -- Intentionally matching control chars - const unsafe = /[<>:"/\\|?*\x00-\x1f]/g; - const sanitized = name.trim().replace(unsafe, "_").replace(/\s+/g, "_"); // Replace whitespace runs with underscore + const trimmed = name.trim(); + if (!trimmed) return ""; + const sanitized = trimmed.replace(/[^\p{L}\p{N}._-]+/gu, "_"); // Collapse multiple underscores, trim leading/trailing, limit length return sanitized.replace(/_+/g, "_").replace(/^_|_$/g, "").slice(0, 60); } @@ -56,7 +56,7 @@ export function getMediaDir() { export async function ensureMediaDir() { const mediaDir = resolveMediaDir(); - await fs.mkdir(mediaDir, { recursive: true }); + await fs.mkdir(mediaDir, { recursive: true, mode: 0o700 }); return mediaDir; } @@ -123,7 +123,7 @@ async function downloadToFile( let total = 0; const sniffChunks: Buffer[] = []; let sniffLen = 0; - const out = createWriteStream(dest); + const out = createWriteStream(dest, { mode: 0o600 }); res.on("data", (chunk) => { total += chunk.length; if (sniffLen < 16384) { @@ -168,7 +168,7 @@ export async function saveMediaSource( ): Promise { const baseDir = resolveMediaDir(); const dir = subdir ? path.join(baseDir, subdir) : baseDir; - await fs.mkdir(dir, { recursive: true }); + await fs.mkdir(dir, { recursive: true, mode: 0o700 }); await cleanOldMedia(); const baseId = crypto.randomUUID(); if (looksLikeUrl(source)) { @@ -198,7 +198,7 @@ export async function saveMediaSource( const ext = extensionForMime(mime) ?? path.extname(source); const id = ext ? `${baseId}${ext}` : baseId; const dest = path.join(dir, id); - await fs.writeFile(dest, buffer); + await fs.writeFile(dest, buffer, { mode: 0o600 }); return { id, path: dest, size: stat.size, contentType: mime }; } @@ -213,7 +213,7 @@ export async function saveMediaBuffer( throw new Error(`Media exceeds ${(maxBytes / (1024 * 1024)).toFixed(0)}MB limit`); } const dir = path.join(resolveMediaDir(), subdir); - await fs.mkdir(dir, { recursive: true }); + await fs.mkdir(dir, { recursive: true, mode: 0o700 }); const uuid = crypto.randomUUID(); const headerExt = extensionForMime(contentType?.split(";")[0]?.trim() ?? undefined); const mime = await detectMime({ buffer, headerMime: contentType }); @@ -231,6 +231,6 @@ export async function saveMediaBuffer( } const dest = path.join(dir, id); - await fs.writeFile(dest, buffer); + await fs.writeFile(dest, buffer, { mode: 0o600 }); return { id, path: dest, size: buffer.byteLength, contentType: mime }; }