From 206756e13c018dfe08b7aa804d87742f1535b655 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 26 Sep 2026 00:52:43 +0200 Subject: [PATCH] fix: stop treating narrow refusals as total ones in the app's sends and exports Two places took a narrow refusal from the server for a complete one. Port of the webmail's 7977a6ed. - Sending at a full submission quota (Stalwart 1.0): when making room found nothing, no search ran for five minutes, for any send. Scheduled mail and undo-send holds can go once their message is out, and until the next search every send failed with "ask your administrator". A send the user starts, a read receipt included, now always looks again; the pause stays for automatic read receipts. - vCard export: Stalwart answers Blob/get only in accounts the user is a member of, so for an address book shared with the user it came back `forbidden` ("You are not an owner of account ...") and the export fell back to one photo download after the other. It checks the access to each blob by itself, whichever account the request names, so `getBlobsAsBase64` now asks again in the user's own account, on 0.16 and 1.0 alike. --- .../email-submission-cleanup.test.ts | 72 ++++++++++++++----- src/api/blob.ts | 17 ++++- src/api/email.ts | 2 +- src/api/submissions.ts | 17 +++-- src/lib/__tests__/contact-media.test.ts | 59 +++++++++++++++ 5 files changed, 140 insertions(+), 27 deletions(-) diff --git a/src/api/__tests__/email-submission-cleanup.test.ts b/src/api/__tests__/email-submission-cleanup.test.ts index ce533119..b28f8df9 100644 --- a/src/api/__tests__/email-submission-cleanup.test.ts +++ b/src/api/__tests__/email-submission-cleanup.test.ts @@ -645,7 +645,7 @@ describe('making room where nothing can go (Stalwart 1.0)', () => { const searches = (server: FakeSubmissionServer) => server.methods().flat().filter((m) => m === 'EmailSubmission/query' || m === 'EmailSubmission/changes').length; - it('does not search the account again for five minutes', async () => { + it('searches again for every send the user starts', async () => { let clock = Date.parse('2026-09-25T12:00:00Z'); vi.spyOn(Date, 'now').mockImplementation(() => clock); const server = setup('1.0', { limit: 2 }); @@ -655,24 +655,64 @@ describe('making room where nothing can go (Stalwart 1.0)', () => { ]); await expect(sendEmail(OUTGOING, 'id-1', 'sent', undefined, VIA_DRAFTS)).rejects.toBeInstanceOf(SubmissionQuotaError); - const first = searches(server); // The query, and the change-log walk once it found nothing. - expect(first).toBe(2); + expect(searches(server)).toBe(2); - // A send and an automatic read receipt right after: one retry each, no search. clock += MINUTE; await expect(sendEmail(OUTGOING, 'id-1', 'sent', undefined, VIA_DRAFTS)).rejects.toBeInstanceOf(SubmissionQuotaError); - await expect(sendReadReceipt(RECEIPT)).rejects.toBeInstanceOf(SubmissionQuotaError); - expect(searches(server)).toBe(first); - expect(argsOf(server, 'EmailSubmission/set').filter((a) => a.create)).toHaveLength(6); - - clock += 5 * MINUTE; - await expect(sendEmail(OUTGOING, 'id-1', 'sent', undefined, VIA_DRAFTS)).rejects.toBeInstanceOf(SubmissionQuotaError); - expect(searches(server)).toBe(2 * first); + expect(searches(server)).toBe(4); expect(server.submissions().map((s) => s.id)).toEqual(pending); }); - it('searches again at once when a search failed', async () => { + // A search that found nothing used to hold off the next one for five + // minutes, whoever sent: a held send that went out within them could not + // make room, and every send failed with "ask your administrator". + it('makes room once a held send went out, minutes after a search found nothing', async () => { + let clock = Date.parse('2026-09-25T12:00:00Z'); + vi.spyOn(Date, 'now').mockImplementation(() => clock); + const server = setup('1.0', { limit: 2 }); + const [undoHold, scheduled] = server.seed('acc-1', [ + { sendAt: clock + MINUTE, status: 'pending' }, + { sendAt: clock + DAY, status: 'pending' }, + ]); + + await expect(sendEmail(OUTGOING, 'id-1', 'sent', undefined, VIA_DRAFTS)).rejects.toBeInstanceOf(SubmissionQuotaError); + + // Two minutes later the undo hold has run out and its message left. + clock += 2 * MINUTE; + server.deliver('acc-1', undoHold); + const result = await sendEmail(OUTGOING, 'id-1', 'sent', undefined, VIA_DRAFTS); + + expect(result.emailSubmissionId).toBeDefined(); + expect(server.submissions().map((s) => s.id)).toEqual([scheduled]); + }); + + it('does not search again for automatic read receipts for five minutes', async () => { + let clock = Date.parse('2026-09-25T12:00:00Z'); + vi.spyOn(Date, 'now').mockImplementation(() => clock); + const server = setup('1.0', { limit: 1 }); + server.seed('acc-1', [{ sendAt: clock + DAY, status: 'pending' }]); + + await expect(sendReadReceipt(RECEIPT)).rejects.toBeInstanceOf(SubmissionQuotaError); + expect(searches(server)).toBe(2); + + // Every further automatic receipt only tries the create once more ... + clock += MINUTE; + await expect(sendReadReceipt(RECEIPT)).rejects.toBeInstanceOf(SubmissionQuotaError); + expect(searches(server)).toBe(2); + expect(argsOf(server, 'EmailSubmission/set').filter((a) => a.create)).toHaveLength(4); + + // ... while one the user sends looks again, + await expect(sendReadReceipt({ ...RECEIPT, automatic: false })).rejects.toBeInstanceOf(SubmissionQuotaError); + expect(searches(server)).toBe(4); + + // and so do automatic ones after five minutes. + clock += 5 * MINUTE; + await expect(sendReadReceipt(RECEIPT)).rejects.toBeInstanceOf(SubmissionQuotaError); + expect(searches(server)).toBe(6); + }); + + it('searches again at once for an automatic receipt when a search failed', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); const server = setup('1.0', { limit: 1 }); server.seed('acc-1', [{ sendAt: Date.now() + DAY, status: 'pending' }]); @@ -684,23 +724,23 @@ describe('making room where nothing can go (Stalwart 1.0)', () => { } }); - await expect(sendEmail(OUTGOING, 'id-1', 'sent', undefined, VIA_DRAFTS)).rejects.toBeInstanceOf(SubmissionQuotaError); + await expect(sendReadReceipt(RECEIPT)).rejects.toBeInstanceOf(SubmissionQuotaError); expect(warn).toHaveBeenCalled(); const first = argsOf(server, 'EmailSubmission/query').length; - await expect(sendEmail(OUTGOING, 'id-1', 'sent', undefined, VIA_DRAFTS)).rejects.toBeInstanceOf(SubmissionQuotaError); + await expect(sendReadReceipt(RECEIPT)).rejects.toBeInstanceOf(SubmissionQuotaError); // A failed look-up proved nothing, so the second attempt looked again. expect(argsOf(server, 'EmailSubmission/query').length).toBe(first + 1); }); - it('keeps no pause once a search found something to remove', async () => { + it('keeps no pause for automatic receipts once a search found something to remove', async () => { const server = setup('1.0', { limit: 2 }); const now = Date.now(); server.seed('acc-1', [{ sendAt: now + DAY, status: 'pending' }, { sendAt: now - HOUR, status: 'final' }]); await sendEmail(OUTGOING, 'id-1', 'sent', 10, VIA_DRAFTS); // Full again: a scheduled send holds the freed place. - await expect(sendEmail(OUTGOING, 'id-1', 'sent', 10, VIA_DRAFTS)).rejects.toBeInstanceOf(SubmissionQuotaError); + await expect(sendReadReceipt(RECEIPT)).rejects.toBeInstanceOf(SubmissionQuotaError); expect(argsOf(server, 'EmailSubmission/query')).toHaveLength(2); }); diff --git a/src/api/blob.ts b/src/api/blob.ts index b3eb667a..9f87e2d1 100644 --- a/src/api/blob.ts +++ b/src/api/blob.ts @@ -325,18 +325,29 @@ function batchedBySize(items: T[], sizeOf: (item: T) => number | undefined, b * Null when the session doesn't advertise the blob capability, which then * must stay out of `using`. Blobs the server doesn't find are left out; once a * request fails, the blobs not fetched yet are listed in `failed`. + * + * Stalwart answers Blob/get only in accounts the user is a member of: an + * address book shared with the user is in an account that isn't one, and the + * call comes back `forbidden` ("You are not an owner of account ..."). It + * checks the access to each blob by itself, though, whichever account the + * request names, so the rest are asked for in the user's own account + * (webmail 7977a6ed). */ export async function getBlobsAsBase64(blobIds: string[], accountId?: string): Promise { if (!jmapClient.hasCapability(CAPABILITIES.BLOB)) return null; - const account = accountId || jmapClient.accountId; + let account = accountId || jmapClient.accountId; const ids = [...new Set(blobIds)]; const data = new Map(); const blobGet = async (chunk: string[], property: 'size' | 'data:asBase64') => { - const res = await jmapClient.request( + const call = () => jmapClient.request( [['Blob/get', { accountId: account, ids: chunk, properties: [property] }, '0']], [CAPABILITIES.CORE, CAPABILITIES.BLOB], ); - const [name, result] = res.methodResponses?.[0] ?? []; + let [name, result] = (await call()).methodResponses?.[0] ?? []; + if (name === 'error' && result?.type === 'forbidden' && account !== jmapClient.accountId) { + account = jmapClient.accountId; + [name, result] = (await call()).methodResponses?.[0] ?? []; + } if (name !== 'Blob/get') { throw new Error(`Blob/get failed: ${result?.description || result?.type || 'unknown'}`); } diff --git a/src/api/email.ts b/src/api/email.ts index 0b7593f1..3a5a4742 100644 --- a/src/api/email.ts +++ b/src/api/email.ts @@ -2283,7 +2283,7 @@ export async function sendReadReceipt(opts: SendReadReceiptOptions): Promise { +export async function makeRoomForSubmission(accountId: string, options: { automatic?: boolean } = {}): Promise { const key = pruneKey(accountId); const recheckAt = roomRecheckAt.get(key); - if (recheckAt !== undefined && Date.now() < recheckAt) return; + if (options.automatic && recheckAt !== undefined && Date.now() < recheckAt) return; const now = Date.now(); prunedAt.set(key, now); let found = 0; diff --git a/src/lib/__tests__/contact-media.test.ts b/src/lib/__tests__/contact-media.test.ts index 3c36a291..54eda320 100644 --- a/src/lib/__tests__/contact-media.test.ts +++ b/src/lib/__tests__/contact-media.test.ts @@ -446,6 +446,65 @@ describe('inlineContactMediaForExport', () => { expect(mockRequest.mock.calls[0][0][0][1].accountId).toBe('team'); }); + // Stalwart answers Blob/get only in accounts the user is a member of - the + // own account and groups, not one that merely shares an address book - and + // then serves every blob the user may read, whichever account the request + // names. Live: userc's Blob/get naming userb's account `e` (a book shared + // with userc) is refused on 1.0 and 0.16.23 alike, and on 1.0 the same + // blobId asked for in userc's own account comes back. 0.16 keeps contact + // photos as data: URIs, so an export never gets here there. + function memberServer(sizes: Record, memberOf = ['f', 'team']) { + mockRequest.mockImplementation(async (calls: Array<[string, { accountId: string; ids: string[]; properties: string[] }, string]>) => { + const [, args, callId] = calls[0]; + if (!memberOf.includes(args.accountId)) { + return { methodResponses: [['error', { type: 'forbidden', description: `You are not an owner of account ${args.accountId}` }, callId]] }; + } + const found = args.ids.filter((id) => id in sizes); + const list = found.map((id) => (args.properties[0] === 'size' ? { id, size: sizes[id] } : { id, 'data:asBase64': 'AAAA' })); + return { methodResponses: [['Blob/get', { accountId: args.accountId, list, notFound: args.ids.filter((id) => !(id in sizes)) }, callId]] }; + }); + } + const sharedCard = (blobId: string) => + card(`e:${blobId}`, { photo: { blobId, kind: 'photo', mediaType: 'image/jpeg' } }, { isShared: true, accountId: 'e', originalId: blobId }); + + it('reads the photos of an address book shared with the user through the own account (Stalwart 1.0)', async () => { + memberServer({ 'b-1': 40_000, 'b-2': 2_000 }); + + const out = await inlineContactMediaForExport([sharedCard('b-1'), sharedCard('b-2')]); + + expect(mockRequest.mock.calls.map(([calls]) => calls[0][1].accountId)).toEqual(['e', 'f']); + expect(mockRequest.mock.calls[1][0][0]).toEqual(['Blob/get', { accountId: 'f', ids: ['b-1', 'b-2'], properties: ['data:asBase64'] }, '0']); + expect(out.map((c) => c.media?.photo?.uri)).toEqual(['data:image/jpeg;base64,AAAA', 'data:image/jpeg;base64,AAAA']); + expect(mockFetchBlob).not.toHaveBeenCalled(); + }); + + it('switches to the own account once when a shared address book has many photos (Stalwart 1.0)', async () => { + const MiB = 1024 * 1024; + memberServer({ 'b-1': 3 * MiB, 'b-2': 2 * MiB, 'b-3': 2 * MiB }); + + await inlineContactMediaForExport([sharedCard('b-1'), sharedCard('b-2'), sharedCard('b-3')]); + + expect(mockRequest.mock.calls.map(([calls]) => [calls[0][1].accountId, calls[0][1].properties[0]])).toEqual([ + ['e', 'size'], + ['f', 'size'], + ['f', 'data:asBase64'], + ['f', 'data:asBase64'], + ]); + expect(mockFetchBlob).not.toHaveBeenCalled(); + }); + + it('does not ask the own account again when it refuses too', async () => { + memberServer({ 'b-1': 10 }, []); + mockFetchBlob.mockResolvedValue(new Uint8Array(JPEG_BYTES).buffer); + + const out = await inlineContactMediaForExport([card('One', { photo: { blobId: 'b-1', kind: 'photo', mediaType: 'image/jpeg' } })]); + + expect(mockRequest).toHaveBeenCalledTimes(1); + // What the batch could not deliver is downloaded one by one. + expect(mockFetchBlob).toHaveBeenCalledTimes(1); + expect(out[0].media?.photo?.uri).toBe(JPEG_URI); + }); + it('leaves 0.16 cards, whose photos are data: URIs, alone', async () => { const cards = [card('One', { photo: { uri: JPEG_URI, kind: 'photo', mediaType: 'image/jpeg' } })]; const out = await inlineContactMediaForExport(cards);