From 706c6c23fd9535b1973e1334b31347716213393b Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 6 Aug 2026 18:55:08 +0200 Subject: [PATCH] fix: send via the identity's own account client so DKIM matches the From domain #461 --- components/email/email-composer.tsx | 5 +- components/mail/mail-app.tsx | 23 ++- .../cross-account-send-routing.test.tsx | 162 ++++++++++++++++++ components/pro/pro-compose-tab-body.tsx | 12 +- stores/email-store.ts | 24 ++- 5 files changed, 218 insertions(+), 8 deletions(-) create mode 100644 components/pro/__tests__/cross-account-send-routing.test.tsx diff --git a/components/email/email-composer.tsx b/components/email/email-composer.tsx index 8c2e93c72..113f00296 100644 --- a/components/email/email-composer.tsx +++ b/components/email/email-composer.tsx @@ -1979,7 +1979,10 @@ export function EmailComposer({ const sendHandledByPlugin = (await emailHooks.onComposeSend.intercept(composeSendRequest)) === false; if (sendHandledByPlugin) { if (finalDraftId) { - client?.deleteEmail(finalDraftId).catch((err) => { + // The draft was created through the identity's owning account + // (see saveDraft), so clean it up there too - not on the active + // account, where the id doesn't resolve (#461). + composerClient?.deleteEmail(finalDraftId).catch((err) => { debug.warn('email', 'Plugin handled the send, but draft cleanup failed:', err); }); } diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index 218aa4cc4..aac9eae24 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -1415,6 +1415,9 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { fromEmail?: string; fromName?: string; identityId?: string; + /** Local account owning the selected identity, when it came from the + * cross-account From dropdown (Pro / embedded multi-account). */ + localAccountId?: string; envelopeMailFrom?: string; attachments?: Array<{ blobId: string; name: string; type: string; size: number; disposition?: 'attachment' | 'inline'; cid?: string }>; inReplyTo?: string[]; @@ -1424,6 +1427,15 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { }) => { if (!client) return; + // Sending from an identity that belongs to another connected account has + // to go through that account's own client: the identity id is only valid + // on its own server, and an unknown id makes the server fall back to the + // active account's primary identity - which then signs the message with + // the wrong DKIM key (#461). The draft was saved against the same account. + const sendClient = data.localAccountId + ? (useAuthStore.getState().getClientForAccount(data.localAccountId) ?? client) + : client; + // Separates "the submission failed" from "a follow-up step failed". Only the // former may propagate: the composer treats a resolved onSend as a delivered // message and wipes its fields, so swallowing a real send failure here lost @@ -1434,7 +1446,7 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { const effectiveMode = pendingDraft?.mode ?? composerMode; const originalEmailId = selectedEmail?.id; - const result = await sendEmail(client, data.to, data.subject, data.body, data.cc, data.bcc, data.identityId, data.fromEmail, data.draftId, data.fromName, data.htmlBody, data.attachments, data.inReplyTo, data.references, data.delayedUntil, data.envelopeMailFrom, { requestReadReceipt: data.requestReadReceipt }); + const result = await sendEmail(sendClient, data.to, data.subject, data.body, data.cc, data.bcc, data.identityId, data.fromEmail, data.draftId, data.fromName, data.htmlBody, data.attachments, data.inReplyTo, data.references, data.delayedUntil, data.envelopeMailFrom, { requestReadReceipt: data.requestReadReceipt, localAccountId: data.localAccountId }); submitted = true; setShowComposer(false); if (result.filingError) { @@ -1664,6 +1676,11 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { lastUndoToastSubmissionRef.current = pendingUndoSend.submissionId; const pending = pendingUndoSend; + // The submission lives on the account that owns the sending identity, + // which is not always the active one in the multi-account shell (#461). + const undoClient = pending.localAccountId + ? (useAuthStore.getState().getClientForAccount(pending.localAccountId) ?? client) + : client; const undoDurationMs = Math.max(sendDelaySeconds, 8) * 1000; toast.success(t('email_viewer.scheduled_send_created'), { @@ -1674,7 +1691,7 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { onClick: () => { void (async () => { try { - await client.rescheduleEmailSubmission( + await undoClient.rescheduleEmailSubmission( pending.submissionId, pending.emailId!, pending.identityId!, @@ -1694,7 +1711,7 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { onClick: () => { void (async () => { try { - const restored = await cancelUndoSend(client, pending); + const restored = await cancelUndoSend(undoClient, pending); if (restored && !pending.isSmime) { await handleEditDraft(restored); } diff --git a/components/pro/__tests__/cross-account-send-routing.test.tsx b/components/pro/__tests__/cross-account-send-routing.test.tsx new file mode 100644 index 000000000..4561119ae --- /dev/null +++ b/components/pro/__tests__/cross-account-send-routing.test.tsx @@ -0,0 +1,162 @@ +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import React from 'react'; + +/** + * #461 - the Pro "From" dropdown lists identities from every connected + * account. Sending through the *active* account's client while using another + * account's identity makes the server fall back to its own primary identity, + * so the message goes out DKIM-signed with the wrong domain key. The compose + * tab must submit through the client of the account owning the identity. + */ + +// The composer itself is irrelevant here - stub it down to a button that +// fires the onSend payload the real composer builds for a cross-account +// identity (raw identity id + owning local account id). +const payloadRef = vi.hoisted(() => ({ current: {} as Record })); + +vi.mock('@/components/email/email-composer', () => ({ + EmailComposer: ({ onSend }: { onSend?: (d: Record) => Promise }) => + React.createElement( + 'button', + { onClick: () => { void onSend?.(payloadRef.current); } }, + 'send', + ), +})); + +vi.mock('@/components/error', () => ({ + ErrorBoundary: ({ children }: { children: React.ReactNode }) => React.createElement(React.Fragment, null, children), + ComposerErrorFallback: () => null, +})); + +vi.mock('next-intl', () => ({ + useTranslations: () => (key: string) => key, +})); + +vi.mock('@/lib/debug', () => ({ + debug: { log: () => {}, warn: () => {}, error: () => {} }, +})); + +vi.mock('@/stores/toast-store', () => ({ + toast: { error: () => {}, success: () => {}, warning: () => {} }, +})); + +// vi.mock factories are hoisted above module-level consts, so the clients +// have to be created in a hoisted block to be visible inside them. +const { activeClient, otherClient } = vi.hoisted(() => { + const makeClient = (label: string) => ({ + label, + getAccountId: () => `jmap-${label}`, + setKeyword: async () => {}, + deleteEmail: async () => {}, + getThreadEmails: async () => [], + }); + return { activeClient: makeClient('account-1'), otherClient: makeClient('account-2') }; +}); + +vi.mock('@/stores/auth-store', () => { + const state = { + client: activeClient, + activeAccountId: 'local-1', + getClientForAccount: (id: string) => (id === 'local-2' ? otherClient : undefined), + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + return { useAuthStore: hook }; +}); + +const { sendEmail } = vi.hoisted(() => ({ + sendEmail: vi.fn(async (...args: unknown[]) => { + void args; + return { scheduled: false }; + }), +})); + +vi.mock('@/stores/email-store', () => { + const state = { + sendEmail: (...args: unknown[]) => sendEmail(...args), + refreshCurrentMailbox: async () => {}, + fetchScheduledEmails: async () => {}, + refreshScheduledMetadata: async () => {}, + isScheduledView: false, + emails: [], + expandedThreadIds: new Set(), + threadEmailsCache: new Map(), + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = () => {}; + return { useEmailStore: hook }; +}); + +vi.mock('@/stores/pro-tab-store', () => { + const state = { + closeTab: () => {}, + updateTabTitle: () => {}, + updateComposeDraft: () => {}, + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + return { + useProTabStore: hook, + registerProTabCloseInterceptor: () => () => {}, + }; +}); + +import { ProComposeTabBody } from '../pro-compose-tab-body'; + +const TAB_DATA = { sessionId: 1, mode: 'compose' as const, title: 'New message' }; + +const basePayload = { + to: ['someone@example.org'], + cc: [], + bcc: [], + subject: 'Hi', + body: 'text', + identityId: 'identity-on-account-2', + fromEmail: 'alias@domain2.com', +}; + +describe('Pro compose tab - cross-account send routing (#461)', () => { + beforeEach(() => { + sendEmail.mockClear(); + }); + + it('submits through the client of the account owning the selected identity', async () => { + payloadRef.current = { ...basePayload, localAccountId: 'local-2' }; + render(); + + fireEvent.click(screen.getByText('send')); + + await waitFor(() => expect(sendEmail).toHaveBeenCalled()); + const args = sendEmail.mock.calls[0] as unknown[]; + expect(args[0]).toBe(otherClient); + expect(args[6]).toBe('identity-on-account-2'); + // The owning account travels on to the store so undo-send can cancel the + // submission on the right server. + expect(args[args.length - 1]).toMatchObject({ localAccountId: 'local-2' }); + }); + + it('falls back to the active client for same-account identities', async () => { + payloadRef.current = { ...basePayload }; + render(); + + fireEvent.click(screen.getByText('send')); + + await waitFor(() => expect(sendEmail).toHaveBeenCalled()); + expect(sendEmail.mock.calls[0][0]).toBe(activeClient); + }); + + it('falls back to the active client when the owning account is no longer connected', async () => { + payloadRef.current = { ...basePayload, localAccountId: 'local-gone' }; + render(); + + fireEvent.click(screen.getByText('send')); + + await waitFor(() => expect(sendEmail).toHaveBeenCalled()); + expect(sendEmail.mock.calls[0][0]).toBe(activeClient); + }); +}); diff --git a/components/pro/pro-compose-tab-body.tsx b/components/pro/pro-compose-tab-body.tsx index 5bf91a7bb..a1d0b48e5 100644 --- a/components/pro/pro-compose-tab-body.tsx +++ b/components/pro/pro-compose-tab-body.tsx @@ -52,13 +52,22 @@ export function ProComposeTabBody({ tabId, data }: ProComposeTabBodyProps) { const handleSend = useCallback(async (sendData: Parameters['onSend']>>[0]) => { if (!client) return; + // The Pro From dropdown lists identities from every connected account, so + // the chosen identity may belong to a different account than the active + // one. Submit through that account's own client - the identity id is only + // valid on its own server, and submitting on the active account's session + // makes the server sign with the wrong DKIM key (#461). The draft was + // already saved against the same account by the composer. + const sendClient = sendData.localAccountId + ? (useAuthStore.getState().getClientForAccount(sendData.localAccountId) ?? client) + : client; // See the inline composer's handleEmailSend: a resolved onSend tells the // composer the message is gone and it clears itself, so a genuine send // failure has to propagate rather than be logged away (#702). let submitted = false; try { const result = await sendEmail( - client, + sendClient, sendData.to, sendData.subject, sendData.body, @@ -74,6 +83,7 @@ export function ProComposeTabBody({ tabId, data }: ProComposeTabBodyProps) { sendData.references, sendData.delayedUntil, sendData.envelopeMailFrom, + { localAccountId: sendData.localAccountId }, ); submitted = true; diff --git a/stores/email-store.ts b/stores/email-store.ts index 785a4966e..1c1880a16 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -21,7 +21,16 @@ type ScheduledSubmissionMetadata = { const VIRTUAL_SCHEDULED_MAILBOX_ID = '__scheduled__'; -type PendingUndoSend = { submissionId: string; emailId?: string; identityId?: string; sendAt: string; isSmime: boolean }; +type PendingUndoSend = { + submissionId: string; + emailId?: string; + identityId?: string; + sendAt: string; + isSmime: boolean; + /** Local account that owns the submission, when the message was sent from + * an identity belonging to a non-active account (#461). */ + localAccountId?: string; +}; interface EmailStore { emails: Email[]; @@ -156,7 +165,7 @@ interface EmailStore { loadMoreEmails: (client: IJMAPClient) => Promise; fetchEmailContent: (client: IJMAPClient, emailId: string) => Promise; fetchQuota: (client: IJMAPClient) => Promise; - sendEmail: (client: IJMAPClient, to: string[], subject: string, body: string, cc?: string[], bcc?: string[], identityId?: string, fromEmail?: string, draftId?: string, fromName?: string, htmlBody?: string, attachments?: Array<{ blobId: string; name: string; type: string; size: number; disposition?: 'attachment' | 'inline'; cid?: string }>, inReplyTo?: string[], references?: string[], delayedUntil?: string, envelopeMailFrom?: string, options?: { requestReadReceipt?: boolean }) => Promise; + sendEmail: (client: IJMAPClient, to: string[], subject: string, body: string, cc?: string[], bcc?: string[], identityId?: string, fromEmail?: string, draftId?: string, fromName?: string, htmlBody?: string, attachments?: Array<{ blobId: string; name: string; type: string; size: number; disposition?: 'attachment' | 'inline'; cid?: string }>, inReplyTo?: string[], references?: string[], delayedUntil?: string, envelopeMailFrom?: string, options?: { requestReadReceipt?: boolean; localAccountId?: string }) => Promise; sendRawEmail: (client: IJMAPClient, rawMimeBlob: Blob, identityId: string, delayedUntil?: string, envelopeRecipients?: string[]) => Promise; deleteEmail: (client: IJMAPClient, emailId: string, forceDelete?: boolean) => Promise; markAsRead: (client: IJMAPClient, emailId: string, read: boolean) => Promise; @@ -1362,7 +1371,16 @@ export const useEmailStore = create((set, get) => ({ set({ isLoading: false, pendingUndoSend: result.scheduled && result.emailSubmissionId && result.sendAt - ? { submissionId: result.emailSubmissionId, emailId: result.emailId, identityId, sendAt: result.sendAt, isSmime: false } + ? { + submissionId: result.emailSubmissionId, + emailId: result.emailId, + identityId, + sendAt: result.sendAt, + isSmime: false, + // Cross-account send: undo/send-now must talk to the account that + // holds the submission, not the active one (#461). + localAccountId: options?.localAccountId, + } : get().pendingUndoSend, }); return result;