From 43334324c446c53eb1dc2f8ef14cbecca4e2a676 Mon Sep 17 00:00:00 2001 From: SnapOtter Date: Tue, 21 Jul 2026 15:30:46 +0800 Subject: [PATCH] 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 | 42 +++++++++++++++- tests/unit/api/file-storage.test.ts | 75 ++++++++++++++++++++++++++++- 2 files changed, 115 insertions(+), 2 deletions(-) diff --git a/apps/api/src/lib/file-storage.ts b/apps/api/src/lib/file-storage.ts index 7be19369..5629ab00 100644 --- a/apps/api/src/lib/file-storage.ts +++ b/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 { + assertSafeStoredName(storedName); if (isS3Enabled()) { const s3 = await getS3(); return s3.getObject(storedName); @@ -150,6 +184,7 @@ export async function readStoredFile(storedName: string): Promise { } export async function streamStoredFile(storedName: string): Promise { + assertSafeStoredName(storedName); if (isS3Enabled()) { const s3 = await getS3(); return s3.getObjectStream(storedName); @@ -158,6 +193,7 @@ export async function streamStoredFile(storedName: string): Promise { } export async function deleteStoredFile(storedName: string): Promise { + assertSafeStoredName(storedName); if (isS3Enabled()) { const s3 = await getS3(); await s3.deleteObject(storedName); @@ -171,6 +207,7 @@ export async function deleteStoredFile(storedName: string): Promise { } 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 { + assertSafeStoredName(storedName); if (isS3Enabled()) { const s3 = await getS3(); return s3.getThumbnail(storedName); @@ -217,6 +255,7 @@ export async function getCachedThumbnail(storedName: string): Promise { + 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 { + assertSafeStoredName(storedName); if (isS3Enabled()) { const s3 = await getS3(); await s3.deleteThumbnail(storedName); diff --git a/tests/unit/api/file-storage.test.ts b/tests/unit/api/file-storage.test.ts index 2209cf63..1bef63e8 100644 --- a/tests/unit/api/file-storage.test.ts +++ b/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 });