From 4acb368c872acca53ba12c015d167f3538c19644 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Fri, 30 Oct 2026 19:26:16 +0100 Subject: [PATCH] fix(core): serialise attachment file mutations, atomic writes, reject svg Co-Authored-By: Claude Sonnet 5.5 --- packages/core/src/attachments.ts | 118 ++++++++++++++++--------- packages/core/src/crypto.ts | 4 +- packages/core/test/attachments.test.ts | 65 ++++++++++++++ packages/core/test/crypto.test.ts | 13 +++ 4 files changed, 157 insertions(+), 43 deletions(-) diff --git a/packages/core/src/attachments.ts b/packages/core/src/attachments.ts index 1c53f00..02d3019 100644 --- a/packages/core/src/attachments.ts +++ b/packages/core/src/attachments.ts @@ -1,4 +1,4 @@ -import { mkdir, rm } from "node:fs/promises"; +import { mkdir, rename, rm } from "node:fs/promises"; import { dirname, join } from "node:path"; import type { Db } from "@sttracker/db/db"; import { newId, nowIso } from "@sttracker/db/ids"; @@ -9,10 +9,34 @@ export type Attachment = AttachmentsTable; const MAX_BYTES = 25 * 1024 * 1024; +/** Lowercase and strip parameters (`; charset=...`). */ +function normalizeMime(mime: string): string { + return (mime.split(";")[0] ?? "").trim().toLowerCase(); +} + function mimeAllowed(mime: string): boolean { + if (mime === "image/svg+xml") return false; // scriptable; never serve user SVG return mime.startsWith("image/") || mime === "application/pdf" || mime === "text/plain"; } +const locks = new Map>(); + +/** Serialise file mutations per sha within this process. */ +async function withLock(key: string, fn: () => Promise): Promise { + const prev = locks.get(key) ?? Promise.resolve(); + const run = prev.then(fn, fn); + const tail = run.then( + () => undefined, + () => undefined, + ); + locks.set(key, tail); + try { + return await run; + } finally { + if (locks.get(key) === tail) locks.delete(key); + } +} + function pathFor(dataDir: string, sha: string): string { return join(dataDir, "attachments", sha.slice(0, 2), sha.slice(2, 4), sha); } @@ -28,21 +52,18 @@ export async function storeAttachment( file: { bytes: Uint8Array; mime: string; filename: string; caption?: string; takenAt?: string }, link: { entity: AttachmentEntity; entityId: string }, ): Promise { - if (!mimeAllowed(file.mime)) throw new ValidationError("mime", "unsupported file type"); + const mime = normalizeMime(file.mime); + if (!mimeAllowed(mime)) throw new ValidationError("mime", "unsupported file type"); if (file.bytes.byteLength > MAX_BYTES) throw new ValidationError("file", "larger than 25 MB"); const sha256 = new Bun.CryptoHasher("sha256").update(file.bytes).digest("hex"); const path = pathFor(dataDir, sha256); - if (!(await Bun.file(path).exists())) { - await mkdir(dirname(path), { recursive: true }); - await Bun.write(path, file.bytes); - } const row: Attachment = { id: newId(), user_id: userId, sha256, - mime: file.mime, + mime, size: file.bytes.byteLength, filename: file.filename, caption: file.caption ?? null, @@ -50,12 +71,25 @@ export async function storeAttachment( visibility: null, created_at: nowIso(), }; - await db.k.transaction().execute(async (tx) => { - await tx.insertInto("attachments").values(row).execute(); - await tx - .insertInto("attachment_links") - .values({ id: newId(), attachment_id: row.id, entity: link.entity, entity_id: link.entityId }) - .execute(); + await withLock(sha256, async () => { + if (!(await Bun.file(path).exists())) { + await mkdir(dirname(path), { recursive: true }); + const tmp = `${path}.tmp-${newId()}`; + try { + await Bun.write(tmp, file.bytes); + await rename(tmp, path); + } catch (e) { + await rm(tmp, { force: true }); + throw e; + } + } + await db.k.transaction().execute(async (tx) => { + await tx.insertInto("attachments").values(row).execute(); + await tx + .insertInto("attachment_links") + .values({ id: newId(), attachment_id: row.id, entity: link.entity, entity_id: link.entityId }) + .execute(); + }); }); return row; } @@ -108,33 +142,35 @@ export async function unlinkAttachment( entity: AttachmentEntity, entityId: string, ): Promise { - const orphanSha = await db.k.transaction().execute(async (tx) => { - const att = await tx - .selectFrom("attachments") - .select("sha256") - .where("id", "=", attachmentId) - .where("user_id", "=", userId) - .executeTakeFirst(); - if (!att) return null; - await tx - .deleteFrom("attachment_links") - .where("attachment_id", "=", attachmentId) - .where("entity", "=", entity) - .where("entity_id", "=", entityId) - .execute(); - const left = await tx - .selectFrom("attachment_links") - .select("id") - .where("attachment_id", "=", attachmentId) - .executeTakeFirst(); - if (left) return null; - await tx.deleteFrom("attachments").where("id", "=", attachmentId).execute(); - const shared = await tx - .selectFrom("attachments") - .select("id") - .where("sha256", "=", att.sha256) - .executeTakeFirst(); - return shared ? null : att.sha256; + const att = await db.k + .selectFrom("attachments") + .select("sha256") + .where("id", "=", attachmentId) + .where("user_id", "=", userId) + .executeTakeFirst(); + if (!att) return; + await withLock(att.sha256, async () => { + const orphan = await db.k.transaction().execute(async (tx) => { + await tx + .deleteFrom("attachment_links") + .where("attachment_id", "=", attachmentId) + .where("entity", "=", entity) + .where("entity_id", "=", entityId) + .execute(); + const left = await tx + .selectFrom("attachment_links") + .select("id") + .where("attachment_id", "=", attachmentId) + .executeTakeFirst(); + if (left) return false; + await tx.deleteFrom("attachments").where("id", "=", attachmentId).execute(); + const shared = await tx + .selectFrom("attachments") + .select("id") + .where("sha256", "=", att.sha256) + .executeTakeFirst(); + return !shared; + }); + if (orphan) await rm(pathFor(dataDir, att.sha256), { force: true }); }); - if (orphanSha) await rm(pathFor(dataDir, orphanSha), { force: true }); } diff --git a/packages/core/src/crypto.ts b/packages/core/src/crypto.ts index 604bc46..3669ae8 100644 --- a/packages/core/src/crypto.ts +++ b/packages/core/src/crypto.ts @@ -39,7 +39,7 @@ export async function decryptJson(secret: string, blob: string): Promise { const key = await deriveKey(secret); const plain = await crypto.subtle.decrypt({ name: "AES-GCM", iv }, key, ct); return JSON.parse(new TextDecoder().decode(plain)) as T; - } catch { - throw new CryptoError("decryption failed"); + } catch (cause) { + throw new CryptoError("decryption failed", { cause }); } } diff --git a/packages/core/test/attachments.test.ts b/packages/core/test/attachments.test.ts index 3acf6ac..1f7d36d 100644 --- a/packages/core/test/attachments.test.ts +++ b/packages/core/test/attachments.test.ts @@ -105,4 +105,69 @@ forEachDialect("attachments", (getDb) => { expect(await filesUnder(join(dataDir, "attachments"))).toHaveLength(0); expect(await db.k.selectFrom("attachments").selectAll().execute()).toHaveLength(0); }); + + test("rejects svg and normalizes mime", async () => { + const db = getDb(); + const u = await addUser(db, "u1"); + const link = { entity: "work", entityId: "w1" } as const; + for (const mime of ["image/svg+xml", "IMAGE/SVG+XML; charset=utf-8"]) { + await expect(storeAttachment(db, dataDir, u, { ...png("x"), mime }, link)).rejects.toBeInstanceOf( + ValidationError, + ); + } + const a = await storeAttachment(db, dataDir, u, { ...png("x"), mime: "Image/PNG; foo=bar" }, link); + expect(a.mime).toBe("image/png"); + expect((await listAttachments(db, u, "work", "w1"))[0]?.mime).toBe("image/png"); + }); + + test("shared bytes across users survive the other's unlink", async () => { + const db = getDb(); + const ua = await addUser(db, "a"); + const ub = await addUser(db, "b"); + const a = await storeAttachment(db, dataDir, ua, png("same"), { entity: "work", entityId: "w1" }); + const b = await storeAttachment(db, dataDir, ub, png("same"), { entity: "work", entityId: "w1" }); + await unlinkAttachment(db, dataDir, ua, a.id, "work", "w1"); + expect(await filesUnder(join(dataDir, "attachments"))).toHaveLength(1); + const r = await readAttachment(db, dataDir, ub, b.id); + expect(await r?.file.text()).toBe("same"); + }); + + test("another user's unlink is a no-op", async () => { + const db = getDb(); + const ua = await addUser(db, "a"); + const ub = await addUser(db, "b"); + const a = await storeAttachment(db, dataDir, ua, png("mine"), { entity: "work", entityId: "w1" }); + await unlinkAttachment(db, dataDir, ub, a.id, "work", "w1"); + expect(await listAttachments(db, ua, "work", "w1")).toHaveLength(1); + expect(await filesUnder(join(dataDir, "attachments"))).toHaveLength(1); + }); + + test("listAttachments is scoped to the user", async () => { + const db = getDb(); + const ua = await addUser(db, "a"); + const ub = await addUser(db, "b"); + await storeAttachment(db, dataDir, ua, png("one"), { entity: "work", entityId: "w1" }); + expect(await listAttachments(db, ub, "work", "w1")).toHaveLength(0); + expect(await listAttachments(db, ua, "work", "w1")).toHaveLength(1); + }); + + test("concurrent store and unlink never leave a row without its file", async () => { + const db = getDb(); + const u = await addUser(db, "u1"); + const first = await storeAttachment(db, dataDir, u, png("race"), { entity: "work", entityId: "w1" }); + const [, second] = await Promise.all([ + unlinkAttachment(db, dataDir, u, first.id, "work", "w1"), + storeAttachment(db, dataDir, u, png("race"), { entity: "work", entityId: "w2" }), + ]); + const r = await readAttachment(db, dataDir, u, second.id); + expect(await r?.file.text()).toBe("race"); + }); + + test("leaves no temp files behind", async () => { + const db = getDb(); + const u = await addUser(db, "u1"); + await storeAttachment(db, dataDir, u, png("clean"), { entity: "work", entityId: "w1" }); + const files = await filesUnder(join(dataDir, "attachments")); + expect(files.some((f) => f.includes(".tmp-"))).toBe(false); + }); }); diff --git a/packages/core/test/crypto.test.ts b/packages/core/test/crypto.test.ts index 6102f7f..e6b79aa 100644 --- a/packages/core/test/crypto.test.ts +++ b/packages/core/test/crypto.test.ts @@ -23,4 +23,17 @@ describe("crypto", () => { await expect(decryptJson("k", "garbage")).rejects.toBeInstanceOf(CryptoError); await expect(decryptJson("k", "v2.AA==.AA==")).rejects.toBeInstanceOf(CryptoError); }); + + test("IV decodes to 12 bytes", async () => { + const blob = await encryptJson("k", { a: 1 }); + expect(Buffer.from(blob.split(".")[1]!, "base64")).toHaveLength(12); + }); + + test("flipping a ciphertext byte throws CryptoError", async () => { + const [v, iv, ct] = (await encryptJson("k", { a: 1 })).split("."); + const bytes = Buffer.from(ct!, "base64"); + bytes[0] = bytes[0]! ^ 0xff; + const tampered = `${v}.${iv}.${bytes.toString("base64")}`; + await expect(decryptJson("k", tampered)).rejects.toBeInstanceOf(CryptoError); + }); });