fix(server): validate image and avatar names and serve them inert

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

apps/server/src/image-headers.ts

@@ -0,0 +1,18 @@
+import type { MiddlewareHandler } from "hono";
+
+/**
+ * Images and avatars are untrusted bytes (TMDB SVGs, user uploads): never let a browser run them
+ * as a document. `frame-ancestors` repeats the app-wide policy from request-guards.ts, which this
+ * header replaces on these routes.
+ */
+export const IMAGE_CONTENT_SECURITY_POLICY =
+  "default-src 'none'; style-src 'unsafe-inline'; sandbox; frame-ancestors 'self'";
+
+/**
+ * Must be registered before `secureHeaders` in index.ts: secureHeaders sets its CSP after the
+ * handler runs and would otherwise overwrite this one.
+ */
+export const imageSecurityHeaders: MiddlewareHandler = async (c, next) => {
+  await next();
+  c.res.headers.set("Content-Security-Policy", IMAGE_CONTENT_SECURITY_POLICY);
+};

apps/server/src/index.ts

@@ -17,6 +17,7 @@ import { createLogger } from "@sofa/logger";
 
 import { apiBodyLimit, UPLOAD_BODY_LIMIT } from "./body-limits";
 import { getJobSchedules, startJobs, stopJobs } from "./cron";
+import { imageSecurityHeaders } from "./image-headers";
 import { handler as rpcHandler } from "./orpc/handler";
 import { openApiHandler } from "./orpc/openapi-handler";
 import { rejectCrossSiteRequests, SECURITY_HEADERS_OPTIONS } from "./request-guards";
@@ -83,6 +84,9 @@ app.use(
   }),
 );
 
+// Image responses get a sandboxing CSP; registered first so secureHeaders' CSP doesn't replace it.
+app.use("/images/*", imageSecurityHeaders);
+app.use("/api/avatars/*", imageSecurityHeaders);
 app.use("*", secureHeaders(SECURITY_HEADERS_OPTIONS));
 
 app.use("*", async (c, next) => {

apps/server/src/routes/avatars.ts

@@ -1,3 +1,4 @@
+import { readFile } from "node:fs/promises";
 import path from "node:path";
 
 import { Hono } from "hono";
@@ -5,6 +6,15 @@ import { Hono } from "hono";
 import { auth } from "@sofa/auth/server";
 import { AVATAR_DIR } from "@sofa/config";
 
+const AVATAR_USER_ID = /^[A-Za-z0-9_-]{1,128}$/;
+// Keep in sync with MIME_TO_EXT in orpc/procedures/account.ts (the extensions uploads are saved with).
+const AVATAR_TYPES: Record<string, string> = {
+  jpg: "image/jpeg",
+  png: "image/png",
+  webp: "image/webp",
+  gif: "image/gif",
+};
+
 const app = new Hono();
 
 app.get("/:userId", async (c) => {
@@ -15,31 +25,23 @@ app.get("/:userId", async (c) => {
   }
 
   const userId = c.req.param("userId");
-
-  // Sanitize userId to prevent path traversal
-  const safeUserId = path.basename(userId);
-  if (!safeUserId || safeUserId !== userId || safeUserId.includes("..")) {
+  if (!AVATAR_USER_ID.test(userId)) {
     return c.json({ error: "Invalid user ID" }, 400);
   }
 
-  // Find the avatar file (could be .jpg, .png, .webp, .gif)
-  const glob = new Bun.Glob(`${safeUserId}.*`);
-  const matches = await Array.fromAsync(glob.scan(AVATAR_DIR));
-  if (matches.length === 0) {
-    return c.json({ error: "Not found" }, 404);
+  for (const [ext, contentType] of Object.entries(AVATAR_TYPES)) {
+    let data: Buffer;
+    try {
+      data = await readFile(path.join(AVATAR_DIR, `${userId}.${ext}`));
+    } catch {
+      continue;
+    }
+    return new Response(new Uint8Array(data), {
+      status: 200,
+      headers: { "Content-Type": contentType, "Cache-Control": "private, no-cache" },
+    });
   }
-
-  const file = Bun.file(path.join(AVATAR_DIR, matches[0]));
-  if (!(await file.exists())) {
-    return c.json({ error: "Not found" }, 404);
-  }
-
-  return new Response(await file.arrayBuffer(), {
-    status: 200,
-    headers: {
-      "Content-Type": file.type,
-    },
-  });
+  return c.json({ error: "Not found" }, 404);
 });
 
 export default app;

apps/server/src/routes/images.ts

@@ -1,5 +1,3 @@
-import path from "node:path";
-
 import { Hono } from "hono";
 import { z } from "zod";
 
@@ -7,6 +5,9 @@ import { fetchAndMaybeCache, imageCacheEnabled } from "@sofa/core/image-cache";
 
 const categorySchema = z.enum(["posters", "backdrops", "stills", "logos", "profiles"]);
 
+/** TMDB file names: letters, digits, `_`/`-`, and an image extension. Rejects `?`, `%`, etc. */
+const TMDB_IMAGE_FILENAME = /^[A-Za-z0-9_-]{1,100}\.(?:jpe?g|png|svg|webp)$/;
+
 const app = new Hono();
 
 app.get("/:category/:filename", async (c) => {
@@ -23,13 +24,11 @@ app.get("/:category/:filename", async (c) => {
   }
   const category = catResult.data;
 
-  // Sanitize filename — only allow basename to prevent path traversal
-  const filename = path.basename(rawFilename);
-  if (!filename || filename !== rawFilename || filename.includes("..")) {
+  if (!TMDB_IMAGE_FILENAME.test(rawFilename)) {
     return c.json({ error: "Invalid filename" }, 400);
   }
 
-  const tmdbPath = `/${filename}`;
+  const tmdbPath = `/${rawFilename}`;
   const result = await fetchAndMaybeCache(tmdbPath, category);
 
   if (!result) {
@@ -40,6 +39,7 @@ app.get("/:category/:filename", async (c) => {
     status: 200,
     headers: {
       "Content-Type": result.contentType,
+      "Cache-Control": "public, max-age=31536000, immutable",
     },
   });
 });

apps/server/test/media-routes.test.ts

@@ -0,0 +1,120 @@
+import { writeFileSync } from "node:fs";
+import path from "node:path";
+
+import { Hono } from "hono";
+import { secureHeaders } from "hono/secure-headers";
+import { beforeEach, describe, expect, test, vi } from "vitest";
+
+import { auth } from "@sofa/auth/server";
+import { AVATAR_DIR } from "@sofa/config";
+
+import { IMAGE_CONTENT_SECURITY_POLICY, imageSecurityHeaders } from "../src/image-headers";
+import { SECURITY_HEADERS_OPTIONS } from "../src/request-guards";
+import avatars from "../src/routes/avatars";
+import images from "../src/routes/images";
+
+vi.mock("@sofa/auth/server", () => ({
+  auth: {
+    api: { getSession: vi.fn<() => void>(), updateUser: vi.fn<() => void>() },
+    handler: vi.fn<() => void>(),
+  },
+}));
+vi.mock("@sofa/core/image-cache", () => ({
+  imageCacheEnabled: () => true,
+  fetchAndMaybeCache: vi.fn<() => Promise<{ buffer: Buffer; contentType: string }>>(async () => ({
+    buffer: Buffer.from([1, 2, 3]),
+    contentType: "image/jpeg",
+  })),
+}));
+// A throwaway avatar directory. The factory is hoisted above the imports, so it loads its own
+// Node modules; the test reads the same path back through the mocked `AVATAR_DIR` import.
+vi.mock("@sofa/config", async (importOriginal) => {
+  const { mkdtempSync } = await import("node:fs");
+  const { tmpdir } = await import("node:os");
+  const { join } = await import("node:path");
+  return {
+    ...(await importOriginal<typeof import("@sofa/config")>()),
+    AVATAR_DIR: mkdtempSync(join(tmpdir(), "sofa-avatars-")),
+  };
+});
+
+const getSession = vi.mocked(auth.api.getSession);
+
+const app = new Hono();
+app.use("/images/*", imageSecurityHeaders);
+app.use("/api/avatars/*", imageSecurityHeaders);
+app.use("*", secureHeaders(SECURITY_HEADERS_OPTIONS));
+app.route("/images", images);
+app.route("/api/avatars", avatars);
+app.get("/other", (c) => c.text("ok"));
+
+beforeEach(() => {
+  getSession.mockReset();
+});
+
+describe("image route", () => {
+  test("serves a valid name with the sandboxing CSP", async () => {
+    const res = await app.request("/images/posters/kqjL17yufvn9OVLyXYpvtyrFfak.jpg");
+    expect(res.status).toBe(200);
+    expect(res.headers.get("content-type")).toBe("image/jpeg");
+    expect(res.headers.get("content-security-policy")).toBe(IMAGE_CONTENT_SECURITY_POLICY);
+    expect(res.headers.get("x-content-type-options")).toBe("nosniff");
+  });
+
+  test("rejects a name with an encoded query string", async () => {
+    const res = await app.request("/images/posters/abc.jpg%3Fx%3D1");
+    expect(res.status).toBe(400);
+  });
+
+  test("rejects a name with a trailing extension", async () => {
+    const res = await app.request("/images/posters/abc.jpg.html");
+    expect(res.status).toBe(400);
+  });
+
+  test("rejects a name with wildcard characters", async () => {
+    const res = await app.request("/images/posters/a%2Ab.jpg");
+    expect(res.status).toBe(400);
+  });
+
+  test("rejects an unknown category", async () => {
+    const res = await app.request("/images/secrets/abc.jpg");
+    expect(res.status).toBe(400);
+  });
+});
+
+describe("avatar route", () => {
+  beforeEach(() => {
+    getSession.mockResolvedValue({
+      session: { id: "s1", userId: "user-1" },
+      user: { id: "user-1", role: "user", name: "U", email: "u@example.com" },
+    } as never);
+  });
+
+  test("serves an existing avatar with the sandboxing CSP", async () => {
+    writeFileSync(path.join(AVATAR_DIR, "user-1.png"), Buffer.from([1]));
+    const res = await app.request("/api/avatars/user-1");
+    expect(res.status).toBe(200);
+    expect(res.headers.get("content-type")).toBe("image/png");
+    expect(res.headers.get("content-security-policy")).toBe(IMAGE_CONTENT_SECURITY_POLICY);
+    expect(res.headers.get("x-content-type-options")).toBe("nosniff");
+  });
+
+  test("rejects wildcard user ids", async () => {
+    const res = await app.request("/api/avatars/user-%2A");
+    expect(res.status).toBe(400);
+  });
+
+  test("returns 404 for an unknown user", async () => {
+    const res = await app.request("/api/avatars/user-2");
+    expect(res.status).toBe(404);
+  });
+});
+
+describe("other routes", () => {
+  test("keep the app-wide CSP without the sandbox directive", async () => {
+    const res = await app.request("/other");
+    const csp = res.headers.get("content-security-policy") ?? "";
+    expect(csp).toContain("frame-ancestors 'self'");
+    expect(csp).not.toContain("sandbox");
+  });
+});

packages/core/src/image-cache.ts

@@ -75,6 +75,12 @@ async function fetchRemoteImage(
     return null;
   }
 
+  const contentType = res.headers.get("content-type") ?? "";
+  if (!contentType.startsWith("image/")) {
+    log.warn(`Not an image: ${url} -> ${contentType || "no content-type"}`);
+    return null;
+  }
+
   const contentLength = Number(res.headers.get("content-length") || "0");
   if (contentLength > MAX_IMAGE_BYTES) {
     log.warn(`Image too large: ${url} -> ${contentLength} bytes`);
@@ -87,10 +93,7 @@ async function fetchRemoteImage(
     return null;
   }
 
-  return {
-    buffer,
-    contentType: res.headers.get("content-type") || "image/jpeg",
-  };
+  return { buffer, contentType };
 }
 
 export async function downloadAndCacheImage(
@@ -117,6 +120,9 @@ export async function downloadAndCacheImage(
   return buffer;
 }
 
+/** Concurrent misses for the same image share one TMDB fetch and one disk write. */
+const inflightFetches = new Map<string, Promise<{ buffer: Buffer; contentType: string } | null>>();
+
 export async function fetchAndMaybeCache(
   tmdbPath: string,
   category: ImageCategory,
@@ -132,23 +138,32 @@ export async function fetchAndMaybeCache(
     return { buffer: cached, contentType };
   }
 
-  // Fetch from TMDB
-  const remote = await fetchRemoteImage(tmdbPath, category);
-  if (!remote) return null;
+  const key = `${category}/${filename}`;
+  const inflight = inflightFetches.get(key);
+  if (inflight) return inflight;
 
-  const { buffer, contentType } = remote;
+  const fetchPromise = (async () => {
+    // Fetch from TMDB
+    const remote = await fetchRemoteImage(tmdbPath, category);
+    if (!remote) return null;
 
-  // Fire-and-forget save to disk
-  const finalPath = getLocalImagePath(category, filename);
-  const tmpPath = `${finalPath}.tmp.${Date.now()}`;
-  Bun.write(tmpPath, buffer)
-    .then(() => rename(tmpPath, finalPath))
-    .catch((err) => {
-      log.warn(`Failed to cache ${category}/${filename}:`, err);
-      unlink(tmpPath).catch(() => {});
-    });
+    const { buffer, contentType } = remote;
 
-  return { buffer, contentType };
+    // Fire-and-forget save to disk
+    const finalPath = getLocalImagePath(category, filename);
+    const tmpPath = `${finalPath}.tmp.${Date.now()}`;
+    Bun.write(tmpPath, buffer)
+      .then(() => rename(tmpPath, finalPath))
+      .catch((err) => {
+        log.warn(`Failed to cache ${category}/${filename}:`, err);
+        unlink(tmpPath).catch(() => {});
+      });
+
+    return { buffer, contentType };
+  })().finally(() => inflightFetches.delete(key));
+
+  inflightFetches.set(key, fetchPromise);
+  return fetchPromise;
 }
 
 export async function loadImageBuffer(

packages/core/test/image-cache.test.ts

@@ -96,6 +96,18 @@ describe("loadImageBuffer", () => {
     expect(result).toHaveLength(4);
   });
 
+  test("returns null when the response is not an image", async () => {
+    mockFetch(async () => {
+      return new Response("<html></html>", {
+        status: 200,
+        headers: { "content-type": "text/html" },
+      });
+    });
+
+    const result = await loadImageBuffer("/x.jpg", "posters");
+    expect(result).toBeNull();
+  });
+
   test("rejects images exceeding size limit via content-length", async () => {
     mockFetch(async () => {
       return new Response(new Uint8Array([1]), {