Sanitize file names taken from server metadata (closes #9)

A file title or album name decrypted from server data could name a path
outside the chosen directory (`../../.ssh/authorized_keys`). One module,
src/filename.ts, now makes such names safe for `quak get`/`get-thumb`
without `--out`, downloadFile/downloadThumbnail without outPath, and the
backup and metadata backup trees. Originals-cache extensions are limited to
letters and digits. A user-supplied path is still used as is. decryptFile
reads a missing or non-string title as "" and rejects metadata that is not
a JSON object.

Model: opus-5-5

test/cli/backup.test.ts

@@ -36,6 +36,7 @@ import {
     lstatSync,
     mkdirSync,
     mkdtempSync,
+    readdirSync,
     readFileSync,
     readlinkSync,
     rmSync,
@@ -107,6 +108,29 @@ class MockClient {
     }
 }
 
+// A server that names an album and a file so as to climb out of the backup
+// directory.
+class HostileClient extends MockClient {
+    override async collectionsSince(): Promise<CollectionsPage> {
+        const page = await super.collectionsSince();
+        return {
+            ...page,
+            collections: page.collections.length
+                ? [collection(3, "../escape")]
+                : [],
+        };
+    }
+    override async filesSince(args: {
+        collectionID: number;
+    }): Promise<FilesPage> {
+        const files =
+            args.collectionID === 3
+                ? [file(300, 3, "../../.ssh/authorized_keys")]
+                : [];
+        return { files, deleted: [], cursor: 1 };
+    }
+}
+
 // A content source that writes byte buffers of the expected length and can be
 // told to fail one fileID's original, to exercise per-file resilience.
 interface StubSource extends ContentSource {
@@ -136,9 +160,12 @@ const stubSource = (): StubSource => {
 
 let root: string;
 
-const openLibrary = (source: ContentSource): Promise<Library> =>
+const openLibrary = (
+    source: ContentSource,
+    client: MockClient = new MockClient(),
+): Promise<Library> =>
     Library.open({
-        client: new MockClient(),
+        client,
         cacheDirectory: join(root, "cache"),
         contentSource: source,
         refreshIntervalSeconds: 3600,
@@ -243,6 +270,30 @@ describe("lib.backup", () => {
         lib.close();
     });
 
+    it("keeps server-supplied album and file names inside the backup", async () => {
+        const lib = await openLibrary(stubSource(), new HostileClient());
+        const outDir = join(root, "backup");
+
+        const result = await lib.backup({ downloadDirectory: outDir });
+
+        expect(result.failed).toBe(0);
+        // The title has no usable extension, so the original is `.bin`.
+        expect(existsSync(join(outDir, "originals", "300.bin"))).toBe(true);
+        const link = join(
+            outDir,
+            "collections",
+            "__escape",
+            "__.._.ssh_authorized_keys",
+        );
+        expect(lstatSync(link).isSymbolicLink()).toBe(true);
+        expect(existsSync(join(outDir, "collections", "__escape.json"))).toBe(
+            true,
+        );
+        // Nothing landed beside or above the backup directory.
+        expect(readdirSync(root).sort()).toEqual(["backup", "cache"]);
+        lib.close();
+    });
+
     it("is an idempotent no-op when every original is already present", async () => {
         const source = stubSource();
         const lib = await openLibrary(source);

test/cli/metadata-backup.test.ts

@@ -165,14 +165,15 @@ const buildMetaMock = async (): Promise<MetaMockState> => {
         },
     };
 
-    // Collection 2: "Work" with no magic metadata
+    // Collection 2: "../Work" with no magic metadata. The server chose a name
+    // that tries to climb out of the backup directory.
     const ck2 = sodium.crypto_secretbox_keygen();
     const { ciphertext: encCK2, nonce: ck2N } = encryptSecretbox(
         ck2,
         masterKey,
     );
     const { ciphertext: encCN2, nonce: cn2N } = encryptSecretbox(
-        new TextEncoder().encode("Work"),
+        new TextEncoder().encode("../Work"),
         ck2,
     );
     const rawColl2 = {
@@ -496,7 +497,8 @@ describe("quak backup-metadata", () => {
         await runBackup(outDir);
 
         const collDirs = readdirSync(join(outDir, "collections"));
-        expect(collDirs.length).toBe(2);
+        // "../Work" is sanitized into one directory name.
+        expect(collDirs.sort()).toEqual(["10-Vacation", "20-__Work"]);
 
         // Find the Vacation collection dir (prefixed with ID)
         const vacDir = collDirs.find((d) => d.includes("Vacation"))!;

test/cli/output.test.ts

@@ -64,6 +64,24 @@ describe("CLI file output (issue #52)", () => {
         expect(thumbnailName(renamedFile)).toBe(`thumb_${RAW_TITLE}`);
     });
 
+    it("sanitizes the title when naming `quak get` downloads", () => {
+        // Without `--out`, the server-supplied title names the file, so it must
+        // not be able to point outside the working directory.
+        const hostile = {
+            ...renamedFile,
+            metadata: { ...renamedFile.metadata, title: "../../.bashrc" },
+        };
+        expect(originalName(hostile)).toBe("__.._.bashrc");
+        expect(thumbnailName(hostile)).toBe("thumb___.._.bashrc");
+
+        const untitled = {
+            ...renamedFile,
+            metadata: { ...renamedFile.metadata, title: "" },
+        };
+        expect(originalName(untitled)).toBe("file-100");
+        expect(thumbnailName(untitled)).toBe("thumb_file-100");
+    });
+
     it("does not use the editedName/editedTime projection", () => {
         const record = deriveRecords([], [renamedFile]).photos.get(100);
         // The projection prefers the edits and reports milliseconds; the CLI

test/download/download.test.ts

@@ -49,6 +49,7 @@
 
 import {
     existsSync,
+    mkdirSync,
     readdirSync,
     readFileSync,
     rmSync,
@@ -501,6 +502,22 @@ const entryPoints = [
     { name: "downloadThumbnail", download: downloadThumbnail },
 ];
 
+// With no `outPath`, the destination is named after `metadata.title`, relative
+// to the working directory. Such tests run inside a temporary directory:
+// `make check` must not create files in the repo root.
+const inDirectory = async <T>(
+    dir: string,
+    run: () => Promise<T>,
+): Promise<T> => {
+    const previous = process.cwd();
+    process.chdir(dir);
+    try {
+        return await run();
+    } finally {
+        process.chdir(previous);
+    }
+};
+
 // ---------------------------------------------------------------------------
 // Tests
 // ---------------------------------------------------------------------------
@@ -536,26 +553,59 @@ describe("downloadFile", () => {
     });
 
     it("uses metadata.title as filename when outPath is omitted", async () => {
-        // With no `outPath`, the destination is `metadata.title`, used
-        // verbatim as a path. The title here is therefore given inside the
-        // test's temporary directory: a bare relative name would resolve
-        // against the process working directory, i.e. the repo root, and
-        // `make check` must not create files in the repo — a failure between
-        // the write and any cleanup would leave one behind.
         const plaintext = new Uint8Array([1, 2, 3]);
         const key = sodium.crypto_secretstream_xchacha20poly1305_keygen();
         const { header, ciphertext } = encryptFileBody(plaintext, key);
         const thumbPush =
             sodium.crypto_secretstream_xchacha20poly1305_init_push(key);
         const file = buildMockEnteFile(key, header, thumbPush.header);
-        const titlePath = join(testDir, "fallback-name.png");
-        file.metadata.title = titlePath;
+        file.metadata.title = "fallback-name.png";
+        const dir = mkdtempSync(join(testDir, "title-"));
 
         const api = new ApiClient({ fetch: mockFetchForBody(ciphertext) });
-        const result = await downloadFile(api, file);
+        const result = await inDirectory(dir, () => downloadFile(api, file));
 
-        expect(result.path).toBe(titlePath);
-        expect(readFileSync(result.path)).toEqual(Buffer.from(plaintext));
+        expect(result.path).toBe("fallback-name.png");
+        expect(readFileSync(join(dir, "fallback-name.png"))).toEqual(
+            Buffer.from(plaintext),
+        );
+    });
+
+    it("keeps a hostile title inside the working directory", async () => {
+        // The server controls the title. `../escaped.png` must not write to
+        // the parent directory; it becomes one file name in the current one.
+        const { api, file } = fixtureFor(
+            multiChunkKey,
+            multiChunk.header,
+            multiChunk.body,
+        );
+        file.metadata.title = "../escaped.png";
+        const parent = mkdtempSync(join(testDir, "hostile-"));
+        const dir = join(parent, "cwd");
+        mkdirSync(dir);
+
+        const result = await inDirectory(dir, () => downloadFile(api, file));
+
+        expect(result.path).toBe("__escaped.png");
+        expect(readdirSync(dir)).toEqual(["__escaped.png"]);
+        expect(readdirSync(parent)).toEqual(["cwd"]);
+    });
+
+    it("uses an explicit outPath verbatim, even one with ..", async () => {
+        // The caller is trusted: its path is not sanitized.
+        const { api, file } = fixtureFor(
+            multiChunkKey,
+            multiChunk.header,
+            multiChunk.body,
+        );
+        const dir = mkdtempSync(join(testDir, "explicit-"));
+        mkdirSync(join(dir, "sub"));
+        const outPath = join(dir, "sub", "..", "explicit.bin");
+
+        const result = await downloadFile(api, file, outPath);
+
+        expect(result.path).toBe(outPath);
+        expect(existsSync(join(dir, "explicit.bin"))).toBe(true);
     });
 
     it("handles a larger single-chunk file (random binary payload)", async () => {
@@ -617,6 +667,23 @@ describe("downloadThumbnail", () => {
         expect(result).toEqual({ path: outPath, bytesWritten: 4 });
         expect(readFileSync(outPath)).toEqual(Buffer.from(plaintext));
     });
+
+    it("names the thumbnail thumb_ plus the sanitized title", async () => {
+        const { api, file } = fixtureFor(
+            multiChunkKey,
+            multiChunk.header,
+            multiChunk.body,
+        );
+        file.metadata.title = "/etc/passwd";
+        const dir = mkdtempSync(join(testDir, "thumb-title-"));
+
+        const result = await inDirectory(dir, () =>
+            downloadThumbnail(api, file),
+        );
+
+        expect(result.path).toBe("thumb__etc_passwd");
+        expect(readdirSync(dir)).toEqual(["thumb__etc_passwd"]);
+    });
 });
 
 // ---------------------------------------------------------------------------

test/filename/filename.test.ts

@@ -0,0 +1,93 @@
+// File names built from server-supplied metadata.
+//
+// quak does not trust the server. A file's title and a collection's name are
+// decrypted from data the server hands us, and a hostile server (or a
+// compromised account) can set them to anything. quak uses them to name files
+// on disk: `quak get` without `--out`, `downloadFile` without `outPath`, the
+// backup's symlink and collection directories, and the extension of every file
+// in the originals cache. Each of those goes through `sanitizeFileName` or
+// `safeExtension`, so a title can only ever name one file inside the directory
+// the caller chose.
+//
+// A path the user supplies (`--out`, `outPath`) is never sanitized: the caller
+// is trusted, the server is not.
+
+import { describe, expect, it } from "vitest";
+
+import { safeExtension, sanitizeFileName } from "../../src/filename.js";
+
+const FALLBACK = "file-42";
+
+describe("sanitizeFileName", () => {
+    it("passes a normal title through unchanged", () => {
+        expect(sanitizeFileName("IMG_0001.HEIC", FALLBACK)).toBe(
+            "IMG_0001.HEIC",
+        );
+        expect(sanitizeFileName("Holiday 2024 (1).jpg", FALLBACK)).toBe(
+            "Holiday 2024 (1).jpg",
+        );
+        expect(sanitizeFileName("café.jpg", FALLBACK)).toBe("café.jpg");
+    });
+
+    it("cannot climb out of the directory with ../", () => {
+        // Without sanitizing, this would overwrite the user's SSH keys.
+        expect(sanitizeFileName("../../.ssh/authorized_keys", FALLBACK)).toBe(
+            "__.._.ssh_authorized_keys",
+        );
+        expect(sanitizeFileName("..", FALLBACK)).toBe("_");
+        expect(sanitizeFileName("..\\..\\x", FALLBACK)).toBe("__.._x");
+    });
+
+    it("cannot name an absolute path", () => {
+        expect(sanitizeFileName("/etc/passwd", FALLBACK)).toBe("_etc_passwd");
+        expect(sanitizeFileName("C:\\Windows\\x.dll", FALLBACK)).toBe(
+            "C__Windows_x.dll",
+        );
+    });
+
+    it("replaces embedded separators, so the name stays one file", () => {
+        expect(sanitizeFileName("a/b\\c.jpg", FALLBACK)).toBe("a_b_c.jpg");
+    });
+
+    it("replaces NUL and other control characters", () => {
+        // A NUL truncates the path in C code and makes Node's fs throw.
+        expect(sanitizeFileName("evil\0.jpg", FALLBACK)).toBe("evil_.jpg");
+        expect(sanitizeFileName("line\nbreak\x7f.jpg", FALLBACK)).toBe(
+            "line_break_.jpg",
+        );
+    });
+
+    it("does not produce a hidden file", () => {
+        expect(sanitizeFileName(".bashrc", FALLBACK)).toBe("_bashrc");
+    });
+
+    it("does not produce a Windows device name", () => {
+        expect(sanitizeFileName("CON", FALLBACK)).toBe("_CON");
+        expect(sanitizeFileName("nul.txt", FALLBACK)).toBe("_nul.txt");
+        expect(sanitizeFileName("LPT1", FALLBACK)).toBe("_LPT1");
+        // Only the exact names are reserved.
+        expect(sanitizeFileName("console.jpg", FALLBACK)).toBe("console.jpg");
+    });
+
+    it("falls back to the given name for an empty title", () => {
+        expect(sanitizeFileName("", FALLBACK)).toBe(FALLBACK);
+    });
+});
+
+describe("safeExtension", () => {
+    it("keeps a normal extension", () => {
+        expect(safeExtension("IMG_0001.HEIC")).toBe(".HEIC");
+        expect(safeExtension("clip.mp4")).toBe(".mp4");
+    });
+
+    it("uses .bin when there is no extension", () => {
+        expect(safeExtension("")).toBe(".bin");
+        expect(safeExtension("README")).toBe(".bin");
+    });
+
+    it("uses .bin when the extension holds anything but letters and digits", () => {
+        expect(safeExtension("x.j\\..\\pg")).toBe(".bin");
+        expect(safeExtension("x.jp g")).toBe(".bin");
+        expect(safeExtension("x.jpg\0")).toBe(".bin");
+    });
+});

test/library/content.test.ts

@@ -226,6 +226,25 @@ describe("ContentCache.original / thumbnail", () => {
         expect(skips).toEqual(["skipped"]);
     });
 
+    it("takes only a letters-and-digits extension from the title", async () => {
+        // The title comes from the server; an extension such as `.\..\x`
+        // must not reach the cache file name, so it becomes `.bin`.
+        const { cache } = buildCache({
+            files: [file(1, "a.jpg"), file(2, "b.\\..\\x"), file(3, "")],
+        });
+        await cache.open();
+
+        expect((await cache.original(1)).path).toBe(
+            join(cacheDir, "originals", "1.jpg"),
+        );
+        expect((await cache.original(2)).path).toBe(
+            join(cacheDir, "originals", "2.bin"),
+        );
+        expect((await cache.original(3)).path).toBe(
+            join(cacheDir, "originals", "3.bin"),
+        );
+    });
+
     it("serves a file already present in the download directory without fetching", async () => {
         const downloadDirectory = join(root, "backup");
         mkdirSync(join(downloadDirectory, "originals"), { recursive: true });

test/model/decrypt.test.ts

@@ -146,10 +146,13 @@ const buildSharedRawCollection = (
 const buildRawFile = (
     collectionKey: Uint8Array,
     opts?: {
-        title?: string;
+        // Any JSON value; `undefined` leaves the title out of the metadata.
+        title?: unknown;
         fileType?: number;
         creationTime?: number;
         info?: { fileSize?: number; thumbSize?: number };
+        // Replaces the whole metadata JSON value.
+        metadata?: unknown;
     },
 ): RawEnteFile => {
     const fileKey = sodium.crypto_secretbox_keygen();
@@ -158,8 +161,8 @@ const buildRawFile = (
         collectionKey,
     );
 
-    const metadata = {
-        title: opts?.title ?? "IMG_0001.jpg",
+    const defaultMetadata = {
+        title: opts && "title" in opts ? opts.title : "IMG_0001.jpg",
         fileType: opts?.fileType ?? 0,
         creationTime: opts?.creationTime ?? 1700000000000000,
         modificationTime: 1700000000000000,
@@ -167,6 +170,8 @@ const buildRawFile = (
         longitude: 2.3522,
         hash: "abcdef1234567890",
     };
+    const metadata =
+        opts && "metadata" in opts ? opts.metadata : defaultMetadata;
     // File metadata is encrypted as a single-chunk secretstream blob
     // (not secretbox). The decryptionHeader is the secretstream init header.
     const metadataBytes = new TextEncoder().encode(JSON.stringify(metadata));
@@ -321,6 +326,31 @@ describe("model.decryptFile", () => {
         expect(file.metadata.longitude).toBeCloseTo(2.3522);
     });
 
+    it("reads a missing or non-string title as an empty string", () => {
+        // The server controls the metadata JSON. A title that is not a
+        // string must not reach code that builds file names from it.
+        const masterKey = sodium.crypto_secretbox_keygen();
+        const { collectionKey } = buildRawCollection(masterKey);
+        for (const title of [undefined, null, 42, ["a"], { x: "../y" }]) {
+            const file = decryptFile(
+                buildRawFile(collectionKey, { title }),
+                collectionKey,
+            );
+            expect(file.metadata.title).toBe("");
+        }
+    });
+
+    it("rejects metadata that is not a JSON object", () => {
+        const masterKey = sodium.crypto_secretbox_keygen();
+        const { collectionKey } = buildRawCollection(masterKey);
+        for (const metadata of [null, "IMG_0001.jpg", 7, []]) {
+            const raw = buildRawFile(collectionKey, { metadata });
+            expect(() => decryptFile(raw, collectionKey)).toThrow(
+                "file 200: metadata is not a JSON object",
+            );
+        }
+    });
+
     it("maps fileType numbers to FileType strings", () => {
         // Ente uses: 0=image, 1=video, 2=livePhoto
         const masterKey = sodium.crypto_secretbox_keygen();