fix(jmap): file post-send message with a full mailboxIds replacement
Sending mail through Bulwark could leave the delivered message stuck in Drafts (keeping the $draft keyword) and never file a copy into Sent, with no error shown, for accounts whose Drafts/Sent mailbox JMAP id is a purely-numeric string (e.g. "0"). The post-send Drafts->Sent move is expressed as onSuccessUpdateEmail on EmailSubmission/set using `mailboxIds/<id>` JSON-Pointer patches. Stalwart up to 0.16.4 (observed on 0.15.5) rejects an Email/set PatchObject whose pointer token is all digits -- e.g. `mailboxIds/0` -- with invalidProperties "Invalid patch value", treating the token as a JSON-Pointer array index even though mailboxIds is a JSON object (cf. RFC 6901 section 4; RFC 8620 section 1.2 warns servers against such interop-hostile ids). Because the move runs only AFTER the EmailSubmission already succeeded, the message is delivered but the filing update is silently rejected: the send code inspects only `notCreated`, not the onSuccessUpdateEmail `notUpdated` result, so nothing surfaces to the user. Stalwart fixed the pointer parsing server-side in 0.16.5 (stalwartlabs/stalwart@175f34ea, jmap-tools 0.1.4 -> 0.1.5; a sibling symptom was stalwartlabs/stalwart#2985). The client-side change is still worthwhile: earlier Stalwart deployments remain in the wild, and a full-property replacement both states the actual intent of the move and emits no per-id pointer token that another server could mishandle. Replace the per-id pointer patches at every post-send / undo-send move site (send, scheduled send, raw-import send, reschedule, and restoreEmailToDraft) with a full `mailboxIds` property replacement via a new mailboxIdsReplacement() helper. This states the actual intent -- after the move the message should belong to exactly the target mailbox -- and is immune to the pointer-token bug. Every one of these sites moves a message that Bulwark itself placed solely in Drafts (or, for undo, in Sent), so the replacement is behaviour-equivalent. Note it is a replacement: a membership added to the message by another client between creation and send is not preserved. restoreEmailToDraft now always lands the message in Drafts only (previously, when no Sent mailbox id was passed, it left the Sent copy in place); the demo client is aligned with the same contract. Add regression tests for the full-replacement shape, a numeric ("0") Drafts id, and restoreEmailToDraft. Follow-up (not included here): the send paths still ignore the implicit Email/set `notUpdated` result of onSuccessUpdateEmail, so any other post-send filing failure would remain silent.
This commit is contained in:
KazNIISA IT
@@ -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<string, unknown> {
|
||||
const submissionCall = captured[2].methodCalls.find(call => call[0] === 'EmailSubmission/set');
|
||||
expect(submissionCall).toBeDefined();
|
||||
const onSuccess = (submissionCall![1] as {
|
||||
onSuccessUpdateEmail: Record<string, Record<string, unknown>>;
|
||||
}).onSuccessUpdateEmail;
|
||||
return Object.values(onSuccess)[0];
|
||||
}
|
||||
|
||||
it('files the sent message via a full mailboxIds replacement, never mailboxIds/<id> 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/<id>` 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<string, unknown> | undefined;
|
||||
vi.spyOn(client as unknown as { request: JMAPClient['request'] }, 'request')
|
||||
.mockImplementation(async (methodCalls) => {
|
||||
const args = methodCalls[0][1] as { update?: Record<string, Record<string, unknown>> };
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<void> {
|
||||
// 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<void> {
|
||||
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();
|
||||
}
|
||||
|
||||
@@ -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<void>;
|
||||
rescheduleEmailSubmission(submissionId: string, emailId: string, identityId: string, delayedUntil: string): Promise<SendEmailResult>;
|
||||
/** `sentMailboxId` is accepted for backwards compatibility but ignored: the message is placed in Drafts only. */
|
||||
restoreEmailToDraft(emailId: string, draftMailboxId: string, sentMailboxId?: string): Promise<void>;
|
||||
|
||||
sendImipReply(opts: {
|
||||
|
||||
+48
-17
@@ -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: { <id>: true, ... } }` — instead of per-id
|
||||
* `mailboxIds/<id>` 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/<id>` 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<string, true> } {
|
||||
const mailboxIds: Record<string, true> = { [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<void> {
|
||||
// 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/<id> 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<void> {
|
||||
const update: Record<string, unknown> = {
|
||||
[`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,
|
||||
|
||||
Reference in New Issue
Block a user