diff --git a/lib/__tests__/jmap-send-threading.test.ts b/lib/__tests__/jmap-send-threading.test.ts index 10f5dc36..df6dc9bf 100644 --- a/lib/__tests__/jmap-send-threading.test.ts +++ b/lib/__tests__/jmap-send-threading.test.ts @@ -55,7 +55,7 @@ interface CapturedRequest { * Mailbox/get → Identity/get → Email/set + EmailSubmission/set. * Returns the captured request bodies for assertions. */ -function mockSendEmailFlow() { +function mockSendEmailFlow(draftsId = 'mb-drafts', sentId = 'mb-sent') { const captured: CapturedRequest[] = []; const fetchSpy = vi.spyOn(globalThis, 'fetch'); @@ -71,8 +71,8 @@ function mockSendEmailFlow() { 'Mailbox/get', { list: [ - { id: 'mb-drafts', name: 'Drafts', role: 'drafts' }, - { id: 'mb-sent', name: 'Sent', role: 'sent' }, + { id: draftsId, name: 'Drafts', role: 'drafts' }, + { id: sentId, name: 'Sent', role: 'sent' }, ], }, '0', @@ -264,3 +264,74 @@ describe('JMAPClient.sendEmail threading headers', () => { ])); }); }); + +describe('JMAPClient post-send mailbox filing', () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + function sentFilingPatch(captured: CapturedRequest[]): Record { + const submissionCall = captured[2].methodCalls.find(call => call[0] === 'EmailSubmission/set'); + expect(submissionCall).toBeDefined(); + const onSuccess = (submissionCall![1] as { + onSuccessUpdateEmail: Record>; + }).onSuccessUpdateEmail; + return Object.values(onSuccess)[0]; + } + + it('files the sent message via a full mailboxIds replacement, never mailboxIds/ pointers', async () => { + const client = createClient(); + const captured = mockSendEmailFlow(); + + await client.sendEmail( + ['recipient@example.com'], 'subject', 'body', + undefined, undefined, 'identity-1', 'user@example.com', + ); + + const patch = sentFilingPatch(captured); + // A `mailboxIds/` JSON-pointer whose token is purely numeric (e.g. a + // Drafts folder whose JMAP id is "0") is rejected by Stalwart, silently + // stranding already-delivered mail in Drafts. The move must use a full + // `mailboxIds` replacement, which has no per-id pointer token. + expect(Object.keys(patch).some(key => key.startsWith('mailboxIds/'))).toBe(false); + expect(patch.mailboxIds).toEqual({ 'mb-sent': true }); + expect(patch['keywords/$draft']).toBeNull(); + }); + + it('files correctly when the Drafts mailbox id is a purely numeric string (Stalwart numeric-id bug)', async () => { + const client = createClient(); + // Drafts id "0", Sent id "e": the old pointer form emitted `mailboxIds/0`, + // which Stalwart rejects with invalidProperties "Invalid patch value". + const captured = mockSendEmailFlow('0', 'e'); + + await client.sendEmail( + ['recipient@example.com'], 'subject', 'body', + undefined, undefined, 'identity-1', 'user@example.com', + ); + + const patch = sentFilingPatch(captured); + expect(Object.keys(patch).some(key => key.startsWith('mailboxIds/'))).toBe(false); + expect(patch.mailboxIds).toEqual({ e: true }); + }); + + it('restoreEmailToDraft places the message in Drafts only via a full mailboxIds replacement', async () => { + const client = createClient(); + let capturedUpdate: Record | undefined; + vi.spyOn(client as unknown as { request: JMAPClient['request'] }, 'request') + .mockImplementation(async (methodCalls) => { + const args = methodCalls[0][1] as { update?: Record> }; + capturedUpdate = args.update?.['email-1']; + return { methodResponses: [['Email/set', { updated: { 'email-1': null } }, '0']] }; + }); + + // Third arg (Sent mailbox id) is intentionally ignored — the message must + // end up in Drafts only, with no leftover Sent membership. Drafts id "0" + // also exercises the numeric-id path in the reverse direction. + await client.restoreEmailToDraft('email-1', '0', 'e'); + + expect(capturedUpdate).toBeDefined(); + expect(Object.keys(capturedUpdate!).some(key => key.startsWith('mailboxIds/'))).toBe(false); + expect(capturedUpdate!.mailboxIds).toEqual({ '0': true }); + expect(capturedUpdate!['keywords/$draft']).toBe(true); + }); +}); diff --git a/lib/demo/demo-client.ts b/lib/demo/demo-client.ts index 72436779..4086fbb5 100644 --- a/lib/demo/demo-client.ts +++ b/lib/demo/demo-client.ts @@ -1096,11 +1096,12 @@ export class DemoJMAPClient implements IJMAPClient { return { scheduled: true, emailId, emailSubmissionId: replacement, sendAt: delayedUntil }; } - async restoreEmailToDraft(emailId: string, draftMailboxId: string, sentMailboxId?: string): Promise { + // Mirrors JMAPClient.restoreEmailToDraft: the third parameter is ignored and + // the message ends up in Drafts only (full mailboxIds replacement). + async restoreEmailToDraft(emailId: string, draftMailboxId: string, _sentMailboxId?: string): Promise { const email = this.data.emails.find(e => e.id === emailId); if (!email) return; - email.mailboxIds[draftMailboxId] = true; - if (sentMailboxId) delete email.mailboxIds[sentMailboxId]; + email.mailboxIds = { [draftMailboxId]: true }; email.keywords.$draft = true; this.recalcMailboxCounts(); } diff --git a/lib/jmap/client-interface.ts b/lib/jmap/client-interface.ts index 729c55f6..2c7849bf 100644 --- a/lib/jmap/client-interface.ts +++ b/lib/jmap/client-interface.ts @@ -189,6 +189,7 @@ export interface IJMAPClient { getScheduledEmails(limit?: number, position?: number): Promise<{ emails: ScheduledEmail[]; hasMore: boolean; total: number; nextPosition: number }>; cancelEmailSubmission(submissionId: string): Promise; rescheduleEmailSubmission(submissionId: string, emailId: string, identityId: string, delayedUntil: string): Promise; + /** `sentMailboxId` is accepted for backwards compatibility but ignored: the message is placed in Drafts only. */ restoreEmailToDraft(emailId: string, draftMailboxId: string, sentMailboxId?: string): Promise; sendImipReply(opts: { diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 89ad0aff..40f9f933 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -15,6 +15,41 @@ function parseRecipientString(s: string): { name?: string; email: string } { return { email: trimmed }; } +/** + * Build the `mailboxIds` portion of an `Email/set` PatchObject as a full-property + * replacement — `{ mailboxIds: { : true, ... } }` — instead of per-id + * `mailboxIds/` JSON-Pointer patches. + * + * Two reasons: + * 1. It states the actual intent of a post-send / undo-send move: the message + * should belong to *exactly* the given mailbox(es). + * 2. It avoids per-id JSON-Pointer tokens entirely. Stalwart (observed on + * 0.15.5) rejects an `Email/set` PatchObject whose pointer token is a + * purely-numeric string — e.g. `mailboxIds/0` for a mailbox whose JMAP id is + * "0" — with `invalidProperties: "Invalid patch value"` (it treats the digits + * as a JSON-Pointer array index even though `mailboxIds` is a JSON object; + * cf. RFC 6901 §4, and RFC 8620 §1.2's warning against interop-hostile ids). + * That silently stranded already-delivered mail in Drafts for accounts whose + * Drafts/Sent mailbox id happened to be all digits (a full member of `0`, + * `1`, … `9`, `10`, … was verified rejected; ids containing a letter work). + * Stalwart fixed the parsing in 0.16.5 (stalwartlabs/stalwart@175f34ea, + * jmap-tools 0.1.5), but earlier deployments remain in the wild — and not + * emitting interop-hostile pointer tokens is the safer shape regardless. + * + * This is a *replacement*: it drops any other mailbox membership the message + * had, so callers must know the complete target set. Do NOT also place a + * `mailboxIds/` pointer key in the same PatchObject — a pointer whose prefix + * is another key in the object is illegal (RFC 8620 §5.3). + */ +function mailboxIdsReplacement( + mailboxId: string, + ...moreMailboxIds: string[] +): { mailboxIds: Record } { + const mailboxIds: Record = { [mailboxId]: true }; + for (const id of moreMailboxIds) mailboxIds[id] = true; + return { mailboxIds }; +} + export class RateLimitError extends Error { retryAfterMs: number; constructor(retryAfterMs: number) { @@ -2522,8 +2557,7 @@ export class JMAPClient implements IJMAPClient { // issues with servers that encrypt on append (e.g. Stalwart). See #188. const onSuccessUpdateEmail = { "#1": { - [`mailboxIds/${draftsMailbox.id}`]: null, - [`mailboxIds/${sentMailbox.id}`]: true, + ...mailboxIdsReplacement(sentMailbox.id), "keywords/$draft": null, }, }; @@ -2797,8 +2831,7 @@ export class JMAPClient implements IJMAPClient { create: { "sub-1": { emailId: `#${emailId}`, identityId: finalIdentityId } }, onSuccessUpdateEmail: { "#sub-1": { - [`mailboxIds/${draftsMailbox.id}`]: null, - [`mailboxIds/${sentMailbox.id}`]: true, + ...mailboxIdsReplacement(sentMailbox.id), "keywords/$draft": null, }, }, @@ -2983,8 +3016,7 @@ export class JMAPClient implements IJMAPClient { create: { "sub-1": { emailId: `#${emailId}`, identityId } }, onSuccessUpdateEmail: { "#sub-1": { - [`mailboxIds/${draftsMailbox.id}`]: null, - [`mailboxIds/${sentMailbox.id}`]: true, + ...mailboxIdsReplacement(sentMailbox.id), "keywords/$draft": null, }, }, @@ -3138,8 +3170,7 @@ export class JMAPClient implements IJMAPClient { create: { "sub-1": { emailId: `#${emailId}`, identityId } }, onSuccessUpdateEmail: { "#sub-1": { - [`mailboxIds/${draftsMailbox.id}`]: null, - [`mailboxIds/${sentMailbox.id}`]: true, + ...mailboxIdsReplacement(sentMailbox.id), "keywords/$draft": null, }, }, @@ -6249,8 +6280,7 @@ export class JMAPClient implements IJMAPClient { ...(draftMailboxId ? { onSuccessUpdateEmail: { '#raw-submit': { - [`mailboxIds/${draftMailboxId}`]: null, - [`mailboxIds/${sentMailboxId}`]: true, + ...mailboxIdsReplacement(sentMailboxId), 'keywords/$draft': null, }, }, @@ -6417,8 +6447,7 @@ export class JMAPClient implements IJMAPClient { ...(draftsMailbox && sentMailbox ? { onSuccessUpdateEmail: { '#replacement': { - [`mailboxIds/${draftsMailbox.id}`]: null, - [`mailboxIds/${sentMailbox.id}`]: true, + ...mailboxIdsReplacement(sentMailbox.id), 'keywords/$draft': null, }, }, @@ -6450,15 +6479,17 @@ export class JMAPClient implements IJMAPClient { return { scheduled: true, emailId, emailSubmissionId: replacementId, sendAt: finalSendAt }; } - async restoreEmailToDraft(emailId: string, draftMailboxId: string, sentMailboxId?: string): Promise { + // The third parameter is intentionally unused: this restores an undo-send / + // canceled-scheduled message to be a draft, so it should live in Drafts *only*. + // A full mailboxIds replacement (rather than mailboxIds/ pointer patches) + // both drops the Sent copy without needing its id and stays safe for numeric + // mailbox ids — see mailboxIdsReplacement(). + async restoreEmailToDraft(emailId: string, draftMailboxId: string, _sentMailboxId?: string): Promise { const update: Record = { - [`mailboxIds/${draftMailboxId}`]: true, + ...mailboxIdsReplacement(draftMailboxId), 'keywords/$draft': true, 'keywords/$seen': true, }; - if (sentMailboxId) { - update[`mailboxIds/${sentMailboxId}`] = null; - } const response = await this.request([ ['Email/set', { accountId: this.accountId,