fix(api): contain library stored-name path traversal (#600)

The library file-storage helpers joined FILES_STORAGE_PATH with a database stored_name and never checked containment, so a crafted name could read or delete files outside the storage root after a malicious 1.x SQLite import (which copies stored_name verbatim). Add assertSafeStoredName() and apply it in every helper that resolves a stored name to a path, matching the containment guard object-storage already uses.

Reported by Alpesh Bhagwatkar.

apps/api/src/lib/file-storage.ts

@@ -1,7 +1,7 @@
 import { randomUUID } from "node:crypto";
 import { createReadStream } from "node:fs";
 import { mkdir, readFile, statfs, unlink, writeFile } from "node:fs/promises";
-import { extname, join } from "node:path";
+import { basename, extname, isAbsolute, join } from "node:path";
 import type { Readable } from "node:stream";
 import type { S3StorageModule } from "@snapotter/enterprise";
 import { SafeError } from "@snapotter/shared";
@@ -90,6 +90,39 @@ function generateStoredName(originalName: string): string {
   return `${randomUUID()}${ext}`;
 }
 
+/**
+ * Reject any stored name that could escape FILES_STORAGE_PATH.
+ *
+ * Stored names are generated basenames (a UUID plus a safe extension), so a
+ * real value never contains a path separator, parent reference, NUL byte, or an
+ * absolute path. Checking that before every join keeps file access inside the
+ * storage root even when the name did not come from saveFile. The one way to
+ * plant a hostile name today is the 1.x SQLite import, which copies
+ * user_files.stored_name verbatim; without this guard a crafted name like
+ * "../../../../etc/passwd" would let the library read and delete helpers reach
+ * anything the API process can. The sibling object-storage module already
+ * applies the same containment idea to its keys.
+ */
+function assertSafeStoredName(storedName: string): void {
+  if (
+    typeof storedName !== "string" ||
+    storedName.length === 0 ||
+    storedName === "." ||
+    storedName.includes("\0") ||
+    storedName.includes("/") ||
+    storedName.includes("\\") ||
+    storedName.includes("..") ||
+    isAbsolute(storedName) ||
+    basename(storedName) !== storedName
+  ) {
+    throw new SafeError("Invalid stored file name", {
+      kind: "operational",
+      code: "INVALID_STORED_NAME",
+      statusCode: 400,
+    });
+  }
+}
+
 // ── Public API ──────────────────────────────────────────────────────
 
 let storageReady = false;
@@ -142,6 +175,7 @@ export async function saveFile(buffer: Buffer, originalName: string): Promise<st
 }
 
 export async function readStoredFile(storedName: string): Promise<Buffer> {
+  assertSafeStoredName(storedName);
   if (isS3Enabled()) {
     const s3 = await getS3();
     return s3.getObject(storedName);
@@ -150,6 +184,7 @@ export async function readStoredFile(storedName: string): Promise<Buffer> {
 }
 
 export async function streamStoredFile(storedName: string): Promise<Readable> {
+  assertSafeStoredName(storedName);
   if (isS3Enabled()) {
     const s3 = await getS3();
     return s3.getObjectStream(storedName);
@@ -158,6 +193,7 @@ export async function streamStoredFile(storedName: string): Promise<Readable> {
 }
 
 export async function deleteStoredFile(storedName: string): Promise<void> {
+  assertSafeStoredName(storedName);
   if (isS3Enabled()) {
     const s3 = await getS3();
     await s3.deleteObject(storedName);
@@ -171,6 +207,7 @@ export async function deleteStoredFile(storedName: string): Promise<void> {
 }
 
 export function getStoredFilePath(storedName: string): string {
+  assertSafeStoredName(storedName);
   return join(env.FILES_STORAGE_PATH, storedName);
 }
 
@@ -205,6 +242,7 @@ function thumbPath(storedName: string): string {
 }
 
 export async function getCachedThumbnail(storedName: string): Promise<Buffer | null> {
+  assertSafeStoredName(storedName);
   if (isS3Enabled()) {
     const s3 = await getS3();
     return s3.getThumbnail(storedName);
@@ -217,6 +255,7 @@ export async function getCachedThumbnail(storedName: string): Promise<Buffer | n
 }
 
 export async function saveThumbnail(storedName: string, buffer: Buffer): Promise<void> {
+  assertSafeStoredName(storedName);
   if (isS3Enabled()) {
     const s3 = await getS3();
     await s3.putThumbnail(storedName, buffer);
@@ -227,6 +266,7 @@ export async function saveThumbnail(storedName: string, buffer: Buffer): Promise
 }
 
 export async function deleteThumbnail(storedName: string): Promise<void> {
+  assertSafeStoredName(storedName);
   if (isS3Enabled()) {
     const s3 = await getS3();
     await s3.deleteThumbnail(storedName);

tests/unit/api/file-storage.test.ts

@@ -1,6 +1,6 @@
 import { randomUUID } from "node:crypto";
 import { existsSync } from "node:fs";
-import { chmod, mkdir, readFile, rm } from "node:fs/promises";
+import { chmod, mkdir, readFile, rm, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
@@ -145,6 +145,79 @@ describe("getStoredFilePath", () => {
   });
 });
 
+// Regression guard for the path-traversal report: stored names are generated
+// basenames, so any separator/parent-reference/absolute name must be rejected
+// before it is joined onto FILES_STORAGE_PATH. Without the guard a poisoned
+// user_files.stored_name (planted via the 1.x SQLite import, which copies it
+// verbatim) lets these helpers read or delete files anywhere the API user can.
+describe("path traversal containment", () => {
+  const TRAVERSAL_NAMES = [
+    "../escape.txt",
+    "../../../../etc/passwd",
+    "..\\escape.txt",
+    "subdir/escape.txt",
+    "nested/../../escape.txt",
+    "..",
+    ".",
+    "/etc/passwd",
+    "with\0nul.png",
+  ];
+
+  it("readStoredFile refuses to read a file outside the storage root", async () => {
+    const { readStoredFile } = await importModule();
+    // Plant a secret directly in tmpdir, one level above testDir.
+    const secretName = `snapotter-secret-${randomUUID().slice(0, 8)}.txt`;
+    const secretPath = join(tmpdir(), secretName);
+    await writeFile(secretPath, "TOP SECRET");
+    try {
+      await expect(readStoredFile(join("..", secretName))).rejects.toThrow();
+    } finally {
+      await rm(secretPath, { force: true });
+    }
+  });
+
+  it("deleteStoredFile refuses a traversal name and leaves the outside file intact", async () => {
+    const { deleteStoredFile } = await importModule();
+    const secretName = `snapotter-secret-${randomUUID().slice(0, 8)}.txt`;
+    const secretPath = join(tmpdir(), secretName);
+    await writeFile(secretPath, "TOP SECRET");
+    try {
+      await expect(deleteStoredFile(join("..", secretName))).rejects.toThrow();
+      expect(existsSync(secretPath)).toBe(true);
+    } finally {
+      await rm(secretPath, { force: true });
+    }
+  });
+
+  it.each(TRAVERSAL_NAMES)("streamStoredFile rejects %j", async (name) => {
+    const { streamStoredFile } = await importModule();
+    await expect(streamStoredFile(name)).rejects.toThrow();
+  });
+
+  it.each(TRAVERSAL_NAMES)("getStoredFilePath rejects %j", async (name) => {
+    const { getStoredFilePath } = await importModule();
+    expect(() => getStoredFilePath(name)).toThrow();
+  });
+
+  it.each(TRAVERSAL_NAMES)("getCachedThumbnail rejects %j", async (name) => {
+    const { getCachedThumbnail } = await importModule();
+    await expect(getCachedThumbnail(name)).rejects.toThrow();
+  });
+
+  it.each(TRAVERSAL_NAMES)("deleteThumbnail rejects %j", async (name) => {
+    const { deleteThumbnail } = await importModule();
+    await expect(deleteThumbnail(name)).rejects.toThrow();
+  });
+
+  it("still serves and deletes a normally-stored file", async () => {
+    const { saveFile, readStoredFile, deleteStoredFile } = await importModule();
+    const name = await saveFile(Buffer.from("hello"), "photo.png");
+    expect((await readStoredFile(name)).toString()).toBe("hello");
+    await expect(deleteStoredFile(name)).resolves.toBeUndefined();
+    expect(existsSync(join(testDir, name))).toBe(false);
+  });
+});
+
 describe("ensureStorageDir", () => {
   it("creates directory if not exists", async () => {
     await rm(testDir, { recursive: true, force: true });