diff --git a/FEATURES.md b/FEATURES.md index c36ca83..7ff6d4b 100644 --- a/FEATURES.md +++ b/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. diff --git a/web/src/store/__tests__/attach-size-limit.test.ts b/web/src/store/__tests__/attach-size-limit.test.ts new file mode 100644 index 0000000..f5dd4c0 --- /dev/null +++ b/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 => ({ + 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: "john@example.org", 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/); + }); +}); diff --git a/web/src/store/compose.ts b/web/src/store/compose.ts index 6041d71..cef0d19 100644 --- a/web/src/store/compose.ts +++ b/web/src/store/compose.ts @@ -487,15 +487,33 @@ export const useCompose = create((set, get) => ({ const accountId = useMail.getState().accountId; if (!accountId || !nodes.length) return; const max = client.maxSizeUpload; - const atts: ComposeAttachment[] = nodes.map((n) => ({ - id: uid("a"), - name: n.name, - type: n.type || "application/octet-stream", - size: n.size ?? 0, - blobId: n.accountId === accountId ? n.blobId : null, - progress: n.accountId === accountId ? 100 : 0, - error: (n.size ?? 0) > max ? translate("Larger than {size} MB limit", { size: Math.round(max / 1048576) }) : null, - })); + const atts: ComposeAttachment[] = nodes.map((n) => { + /* + * `maxSizeUpload` is what the server will accept for a single *upload* + * (RFC 8620), so it only bears on a file that is about to be uploaded. + * + * A blob already in this account is attached by reference and nothing is + * sent, however large it is -- which is the whole point of attaching from + * 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()) {