fix(core): serialise attachment file mutations, atomic writes, reject svg

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

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<string, Promise<void>>();
+
+/** Serialise file mutations per sha within this process. */
+async function withLock<T>(key: string, fn: () => Promise<T>): Promise<T> {
+  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<Attachment> {
-  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<void> {
-  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 });
 }

packages/core/src/crypto.ts

@@ -39,7 +39,7 @@ export async function decryptJson<T>(secret: string, blob: string): Promise<T> {
     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 });
   }
 }

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);
+  });
 });

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);
+  });
 });