From 8028ec7eada51eb26b9c678cace21c1191f74115 Mon Sep 17 00:00:00 2001 From: Sean Silvius Date: Sat, 11 Apr 2026 14:21:39 -0700 Subject: [PATCH] fix(imap): use destination UIDVALIDITY in APPENDUID / COPYUID response codes Fixes issue #83 plus two related bugs of the same shape in COPY and MOVE. The original issue only named APPEND; while re-auditing the extension command handlers I found the same pattern in handleCopy and handleMove. ## The bugs RFC 4315 Section 3: APPENDUID / COPYUID response codes carry the DESTINATION mailbox's UIDVALIDITY as their first argument. RFC 6851 Section 4 inherits the same COPYUID semantics for MOVE. Three handlers got this wrong: 1. **handleAppend** hardcoded `APPENDUID 1 ${uid}`. The literal `1` was used regardless of the actual destination mailbox's UIDVALIDITY. 2. **handleCopy** used `uidMap.uidValidity` which is the currently selected (source) folder's UIDVALIDITY, not the destination's. For cross-mailbox copies and for copies between folders that have different UIDVALIDITY values, this returned the wrong value. 3. **handleMove** had the same bug as handleCopy. Any IMAP client that caches by (uidValidity, uid) tuples (which is standard) would either think its cache is still valid when it is not, or invalidate on every COPY/MOVE/APPEND because the response UIDVALIDITY matched the source. ## The fix Extended the `ExtensionAdapter` interface. All three return types now include `uidValidity: number`: - `copyMessage(messageId, targetFolderId) -> { newUid, uidValidity }` - `moveMessage(messageId, targetFolderId) -> { newUid, uidValidity }` - `appendMessage(folderId, content, flags, date?) -> { uid, uidValidity, messageId }` The adapter knows the destination folder's UIDVALIDITY because it just wrote to that folder. No extra query is needed; this is a pure return-type extension. Handlers updated to use the returned validity: - `handleAppend`: `[APPENDUID ${result.uidValidity} ${result.uid}]` - `handleCopy`: captures `destUidValidity` on the first adapter call (all copied messages share one destination) and uses it in the COPYUID response. - `handleMove`: same pattern, captures `destUidValidity` on first moveMessage call. Added a "no messages copied/moved" guard: if the sequence set resolves to zero messages, the handler omits the COPYUID response code entirely and just returns tagged OK. Previously the code emitted `[COPYUID ]` with trailing empty strings. ## Breaking change `ExtensionAdapter` is a public interface consumers implement in their adapters. The return-type extension is a breaking change: existing adapters that return `{ newUid }` without `uidValidity` will fail typecheck on upgrade. The upgrade is mechanical: add `uidValidity` to the return from each method's adapter implementation. Pre-1.0 is the right time for this. The framework is shipping at 0.1.0 and no consumers have written adapters yet against the published packages. ## Tests Three new tests added to `packages/imap/tests/commands/extensions.test.ts`, each citing the RFC section it verifies: 1. `returns destination UIDVALIDITY in APPENDUID response (RFC 4315 Section 3)`: calls handleAppend with a mock adapter that returns `uidValidity: 42, uid: 300`. Asserts the response contains `[APPENDUID 42 300]`. Before the fix, this failed with `[APPENDUID 1 300]`. 2. `returns destination UIDVALIDITY in COPYUID response (RFC 4315 Section 3)`: source session has `uidValidity: 1`, mock copyMessage returns `uidValidity: 42`. Asserts the response contains `[COPYUID 42 `. Before the fix, this failed with `[COPYUID 1 ...]` (source validity). 3. `returns destination UIDVALIDITY in MOVE's COPYUID response (RFC 6851 Section 4)`: same setup for MOVE. Same failure mode before the fix. The existing substring-match tests (`toContain("APPENDUID")` and `toContain("COPYUID")`) were kept -- they still pass because the literal prefix appears in both old and new responses. The new tests exercise the actual validity value, which the old tests never checked. ## Verification - pnpm --filter @rafters/mail-imap test: 316 passing (313 existing + 3 new) - pnpm test (workspace): 617 passing across 37 files (was 614 + 3 new) - pnpm -r build: all 9 packages build ESM + dts - pnpm typecheck: clean across workspace - pnpm lint: 0 warnings, 0 errors - pnpm format:check: clean Closes #83. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/imap/src/commands/extensions.ts | 70 ++++++++++++++++--- .../imap/tests/commands/extensions.test.ts | 65 ++++++++++++++++- 2 files changed, 121 insertions(+), 14 deletions(-) diff --git a/packages/imap/src/commands/extensions.ts b/packages/imap/src/commands/extensions.ts index 96b7b9a..0b8b1ae 100644 --- a/packages/imap/src/commands/extensions.ts +++ b/packages/imap/src/commands/extensions.ts @@ -16,14 +16,39 @@ import type { UidMap } from "../uid-map.ts"; * Adapter interface for extension operations. */ export interface ExtensionAdapter { - copyMessage(messageId: string, targetFolderId: string): Promise<{ newUid: number }>; - moveMessage(messageId: string, targetFolderId: string): Promise<{ newUid: number }>; + /** + * RFC 4315 Section 3: COPY returns COPYUID with the DESTINATION + * mailbox's UIDVALIDITY. The adapter must return both the new UID + * assigned in the destination and the destination's uidValidity so + * the handler can emit the correct response code. + */ + copyMessage( + messageId: string, + targetFolderId: string, + ): Promise<{ newUid: number; uidValidity: number }>; + + /** + * RFC 6851 Section 4: MOVE inherits COPYUID semantics from RFC 4315. + * The COPYUID response code must carry the DESTINATION mailbox's + * UIDVALIDITY, not the source's. + */ + moveMessage( + messageId: string, + targetFolderId: string, + ): Promise<{ newUid: number; uidValidity: number }>; + + /** + * RFC 3501 Section 6.3.6 + RFC 4315: APPEND returns APPENDUID with + * the destination mailbox's UIDVALIDITY and the UID assigned to the + * newly appended message. Both come from the adapter. + */ appendMessage( folderId: string, content: string, flags: string[], internalDate?: Date, - ): Promise<{ uid: number; messageId: string }>; + ): Promise<{ uid: number; uidValidity: number; messageId: string }>; + getFolderIdByName(mailboxId: string, name: string): Promise; } @@ -61,6 +86,7 @@ export async function handleCopy( const sourceUids: number[] = []; const destUids: number[] = []; + let destUidValidity: number | null = null; for (const uid of uids) { const msgId = uidMap.uidToMessageId(uid); @@ -68,11 +94,19 @@ export async function handleCopy( const result = await adapter.copyMessage(msgId, targetFolderId); sourceUids.push(uid); destUids.push(result.newUid); + // All copied messages share one destination, so uidValidity is constant + // across the loop. Capture it on first hit. + if (destUidValidity === null) destUidValidity = result.uidValidity; + } + + if (sourceUids.length === 0 || destUidValidity === null) { + // RFC 4315: no messages copied, no COPYUID response code + return [formatTagged(tag, "OK", "COPY completed")]; } - // RFC 4315: COPYUID response code - const uidValidity = uidMap.uidValidity; - const copyUid = `[COPYUID ${uidValidity} ${sourceUids.join(",")} ${destUids.join(",")}]`; + // RFC 4315 Section 3: COPYUID response code uses the DESTINATION + // mailbox's UIDVALIDITY. The adapter returns it alongside the new UID. + const copyUid = `[COPYUID ${destUidValidity} ${sourceUids.join(",")} ${destUids.join(",")}]`; return [formatTagged(tag, "OK", `${copyUid} COPY completed`)]; } @@ -116,6 +150,7 @@ export async function handleMove( const responses: string[] = []; const sourceUids: number[] = []; const destUids: number[] = []; + let destUidValidity: number | null = null; // Move in reverse order to keep sequence numbers valid for EXPUNGE responses for (let i = uids.length - 1; i >= 0; i--) { @@ -126,13 +161,23 @@ export async function handleMove( const result = await adapter.moveMessage(msgId, targetFolderId); sourceUids.unshift(uid); destUids.unshift(result.newUid); + // All moved messages share one destination, so uidValidity is constant + // across the loop. Capture it on first hit. + if (destUidValidity === null) destUidValidity = result.uidValidity; const formerSeq = uidMap.expungeUid(uid); responses.push(formatExpungeResponse(formerSeq)); } - // RFC 6851 Section 4: COPYUID response code - const uidValidity = uidMap.uidValidity; - const copyUid = `[COPYUID ${uidValidity} ${sourceUids.join(",")} ${destUids.join(",")}]`; + if (sourceUids.length === 0 || destUidValidity === null) { + // No messages moved -- omit the COPYUID response code + responses.push(formatTagged(tag, "OK", "MOVE completed")); + return responses; + } + + // RFC 6851 Section 4: COPYUID response code inherits RFC 4315 semantics. + // First argument is the DESTINATION mailbox's UIDVALIDITY, not the + // source's. The adapter returns it alongside each new UID. + const copyUid = `[COPYUID ${destUidValidity} ${sourceUids.join(",")} ${destUids.join(",")}]`; responses.push(formatTagged(tag, "OK", `${copyUid} MOVE completed`)); return responses; } @@ -167,8 +212,11 @@ export async function handleAppend( const result = await adapter.appendMessage(folderId, parsed.content, parsed.flags, parsed.date); - // RFC 4315: APPENDUID response code - return [formatTagged(tag, "OK", `[APPENDUID 1 ${result.uid}] APPEND completed`)]; + // RFC 4315 Section 3: APPENDUID response code uses the destination + // mailbox's UIDVALIDITY, returned by the adapter alongside the new UID. + return [ + formatTagged(tag, "OK", `[APPENDUID ${result.uidValidity} ${result.uid}] APPEND completed`), + ]; } /** diff --git a/packages/imap/tests/commands/extensions.test.ts b/packages/imap/tests/commands/extensions.test.ts index e226fe5..17b6029 100644 --- a/packages/imap/tests/commands/extensions.test.ts +++ b/packages/imap/tests/commands/extensions.test.ts @@ -11,9 +11,13 @@ import { UidMap } from "../../src/uid-map.ts"; function mockAdapter(): ExtensionAdapter { return { - copyMessage: vi.fn(async () => ({ newUid: 200 })), - moveMessage: vi.fn(async () => ({ newUid: 201 })), - appendMessage: vi.fn(async () => ({ uid: 300, messageId: "new-msg" })), + copyMessage: vi.fn(async () => ({ newUid: 200, uidValidity: 42 })), + moveMessage: vi.fn(async () => ({ newUid: 201, uidValidity: 42 })), + appendMessage: vi.fn(async () => ({ + uid: 300, + uidValidity: 42, + messageId: "new-msg", + })), getFolderIdByName: vi.fn(async (_mbx: string, name: string) => name.toUpperCase() === "SENT" ? "folder-sent" : undefined, ), @@ -73,6 +77,25 @@ describe("RFC 3501 Section 6.4.7: COPY Command", () => { expect(responses[0]).toContain("COPYUID"); }); + // RFC 4315 Section 3: COPYUID response code returns the DESTINATION + // mailbox's UIDVALIDITY as its first argument, not the source's. The + // source session has uidValidity: 1 (selectedSession) and the mock + // adapter returns copies from a folder with uidValidity: 42. The + // response must carry 42 -- not 1 (source) and not any other constant. + it("returns destination UIDVALIDITY in COPYUID response (RFC 4315 Section 3)", async () => { + const adapter = mockAdapter(); + const responses = await handleCopy( + "a001", + "1:3 Sent", + selectedSession(), + loadedUidMap(), + "mbx-1", + adapter, + false, + ); + expect(responses[0]).toContain("[COPYUID 42 "); + }); + it("supports UID COPY", async () => { const adapter = mockAdapter(); await handleCopy("a001", "10 Sent", selectedSession(), loadedUidMap(), "mbx-1", adapter, true); @@ -141,6 +164,25 @@ describe("RFC 6851: MOVE Command", () => { expect(uidMap.totalMessages()).toBe(2); }); + // RFC 6851 Section 4 inherits the RFC 4315 COPYUID semantics for MOVE. + // The first argument is the DESTINATION mailbox's UIDVALIDITY. The mock + // moveMessage returns uidValidity: 42; the response must carry 42, not + // the source validity of 1. + it("returns destination UIDVALIDITY in MOVE's COPYUID response (RFC 6851 Section 4)", async () => { + const adapter = mockAdapter(); + const responses = await handleMove( + "a001", + "1 Sent", + selectedSession(), + loadedUidMap(), + "mbx-1", + adapter, + false, + ); + const tagged = responses[responses.length - 1]; + expect(tagged).toContain("[COPYUID 42 "); + }); + it("rejects MOVE on read-only folder", async () => { const responses = await handleMove( "a001", @@ -198,6 +240,23 @@ describe("RFC 3501 Section 6.3.6: APPEND Command", () => { expect(responses[0]).toContain("APPENDUID"); }); + // RFC 4315 Section 3: APPENDUID response code returns the destination + // mailbox's UIDVALIDITY as its first argument. The mock adapter returns + // uidValidity: 42; the response must carry that value, not a constant. + it("returns destination UIDVALIDITY in APPENDUID response (RFC 4315 Section 3)", async () => { + const adapter = mockAdapter(); + const session = new ImapSession(); + session.authenticate(); + const responses = await handleAppend( + "a001", + "Sent Message content here", + session, + "mbx-1", + adapter, + ); + expect(responses[0]).toContain("[APPENDUID 42 300]"); + }); + it("appends with flags", async () => { const adapter = mockAdapter(); const session = new ImapSession();