Merge pull request #212 from Coffey-Labs/fix/attach-size-limits

Apply the upload limit only where something is uploaded

FEATURES.md

@@ -459,6 +459,13 @@ minimisable and maximisable; full-screen on mobile.
   upload at all**, however large. A file from someone else's shared folder is
   copied to your account first, because a message can only carry blobs from the
   account sending it; the picker says so before it does.
+
+  The upload limit applies to that copy and to nothing else. `maxSizeUpload` is
+  what the server will accept for a single *upload*, so it bears only on a file
+  that is about to be uploaded — a blob this account already holds is attached
+  by reference and never sent. Checking it in both cases refused a 60 MB message
+  the server was already storing, on the grounds that it could not have been
+  uploaded, which it was not being.
 - **Attachment reminder** when the text mentions an attachment and none is there.
 - **Spell check** toggle.
 - **Drafts** save as you type and on close, with the save state shown.

web/src/store/__tests__/attach-size-limit.test.ts

@@ -0,0 +1,100 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+import { CAP, client } from "@/jmap/client";
+import { useCompose, type AttachableFile } from "@/store/compose";
+import { useMail } from "@/store/mail";
+import type { JmapSession } from "@/jmap/types";
+
+/**
+ * `maxSizeUpload` is what the server will accept for a single *upload*
+ * (RFC 8620), so it bears only on a file that is about to be uploaded.
+ *
+ * FEATURES has always said attach-from-Files works "however large" because a
+ * blob the account already holds is attached by reference. The code applied
+ * the limit to those as well, which refused a message the server was already
+ * storing on the grounds that it could not have been uploaded — which it was
+ * not being.
+ */
+
+const MAX = 50_000_000;
+const OURS = "a1";
+const THEIRS = "a2";
+
+const file = (over: Partial<AttachableFile> = {}): AttachableFile => ({
+  accountId: OURS,
+  name: "big.bin",
+  type: "application/octet-stream",
+  size: MAX * 2,
+  blobId: "b-big",
+  ...over,
+});
+
+const attachments = (key: string) => useCompose.getState().drafts.find((d) => d.key === key)!.attachments;
+
+beforeEach(() => {
+  client.session = {
+    capabilities: { [CAP.core]: { maxSizeUpload: MAX }, [CAP.mail]: {} },
+    accounts: {},
+    primaryAccounts: {},
+    state: "s1",
+  } as unknown as JmapSession;
+  useCompose.setState({ drafts: [], activeKey: null, pendingSends: {} });
+  useMail.setState({
+    accountId: OURS,
+    identities: [{ id: "i1", name: "John", email: "[email protected]", replyTo: null }] as never,
+  });
+  // Nothing here should reach the network; a call would mean an upload was
+  // attempted for a file that is only being referenced.
+  vi.stubGlobal("fetch", vi.fn(async () => {
+    throw new Error("no upload should happen");
+  }));
+});
+
+describe("attaching a blob this account already holds", () => {
+  it("takes it however large, because nothing is uploaded", async () => {
+    const key = useCompose.getState().open();
+    await useCompose.getState().addFromFiles(key, [file()]);
+    const a = attachments(key)[0]!;
+    expect(a.error).toBeNull();
+    expect(a.blobId).toBe("b-big");
+    expect(a.progress).toBe(100);
+  });
+
+  it("is complete the moment it is added, with no request made", async () => {
+    const key = useCompose.getState().open();
+    await useCompose.getState().addFromFiles(key, [file({ size: MAX * 10 })]);
+    expect(attachments(key)[0]!.error).toBeNull();
+    expect(fetch).not.toHaveBeenCalled();
+  });
+});
+
+describe("attaching a blob from somebody else's account", () => {
+  it("refuses one larger than the server will accept, since it must be uploaded", async () => {
+    const key = useCompose.getState().open();
+    await useCompose.getState().addFromFiles(key, [file({ accountId: THEIRS, size: MAX + 1 })]);
+    const a = attachments(key)[0]!;
+    expect(a.error).toMatch(/Larger than/);
+    expect(a.blobId).toBeNull();
+  });
+
+  it("allows one within the limit, and marks it as still needing the upload", async () => {
+    const key = useCompose.getState().open();
+    await useCompose.getState().addFromFiles(key, [file({ accountId: THEIRS, size: 1000, blobId: "b-small" })]);
+    const a = attachments(key)[0]!;
+    // The upload itself fails here because fetch is stubbed to throw; what
+    // matters is that it was attempted rather than refused up front.
+    expect(a.progress).not.toBe(100);
+  });
+});
+
+describe("a mixture in one drop", () => {
+  it("judges each file by whether it will actually be uploaded", async () => {
+    const key = useCompose.getState().open();
+    await useCompose.getState().addFromFiles(key, [
+      file({ name: "ours.bin", size: MAX * 3 }),
+      file({ name: "theirs.bin", accountId: THEIRS, size: MAX * 3, blobId: "b-theirs" }),
+    ]);
+    const [ours, theirs] = attachments(key);
+    expect(ours!.error).toBeNull();
+    expect(theirs!.error).toMatch(/Larger than/);
+  });
+});

web/src/store/compose.ts

@@ -487,15 +487,33 @@ export const useCompose = create<ComposeState>((set, get) => ({
     const accountId = useMail.getState().accountId;
     if (!accountId || !nodes.length) return;
     const max = client.maxSizeUpload;
-    const atts: ComposeAttachment[] = nodes.map((n) => ({
+    const atts: ComposeAttachment[] = nodes.map((n) => {
-      id: uid("a"),
+      /*
-      name: n.name,
+       * `maxSizeUpload` is what the server will accept for a single *upload*
-      type: n.type || "application/octet-stream",
+       * (RFC 8620), so it only bears on a file that is about to be uploaded.
-      size: n.size ?? 0,
+       *
-      blobId: n.accountId === accountId ? n.blobId : null,
+       * A blob already in this account is attached by reference and nothing is
-      progress: n.accountId === accountId ? 100 : 0,
+       * sent, however large it is -- which is the whole point of attaching from
-      error: (n.size ?? 0) > max ? translate("Larger than {size} MB limit", { size: Math.round(max / 1048576) }) : null,
+       * Files, and of forwarding a message as an attachment. Applying the limit
-    }));
+       * to those refused a 60 MB message the server was already holding, on the
+       * grounds that it could not have been uploaded, which it was not being.
+       *
+       * A file from somebody else's account is fetched and re-uploaded into
+       * this one, because a message can only carry blobs from the account
+       * sending it. That upload is real, and the limit is real for it.
+       */
+      const byReference = n.accountId === accountId;
+      const tooLargeToUpload = !byReference && (n.size ?? 0) > max;
+      return {
+        id: uid("a"),
+        name: n.name,
+        type: n.type || "application/octet-stream",
+        size: n.size ?? 0,
+        blobId: byReference ? n.blobId : null,
+        progress: byReference ? 100 : 0,
+        error: tooLargeToUpload ? translate("Larger than {size} MB limit", { size: Math.round(max / 1048576) }) : null,
+      };
+    });
     get().update(key, { attachments: [...(get().drafts.find((d) => d.key === key)?.attachments ?? []), ...atts] });
 
     for (const [i, a] of atts.entries()) {