From cd9536dcf180b9fcf5db8100ca2bc9b317ba8975 Mon Sep 17 00:00:00 2001 From: lucamzanon Date: Thu, 1 Oct 2026 04:43:49 +0200 Subject: [PATCH] fix: copy forwarded parts to the sending account before saving or sending A forward carries the original message's attachments, and the quoted body's inline images, as part blobIds of the account that holds it (replyTo.accountId, #1104). When the forward goes out through another account - a From picked from another account's identities, or the holding account has none of its own - the draft was saved and sent through that other account, which cannot resolve those blobs: Email/set failed with blobNotFound and the forward could not be sent. Parts of the original are now tagged with the holding account. Before a draft save or a send, any part owned by another account is downloaded through that account's client and uploaded through the composing one, once; later saves reuse the copy. Blob/copy cannot do it, as every login is its own JMAP session. Quoted inline images are loaded through the holding account's client as well. Forwards that go out from the holding account keep referencing the original blobs exactly as before. --- .../composer-forward-other-account.test.tsx | 299 ++++++++++++++++++ components/email/email-composer.tsx | 75 ++++- 2 files changed, 370 insertions(+), 4 deletions(-) create mode 100644 components/email/__tests__/composer-forward-other-account.test.tsx diff --git a/components/email/__tests__/composer-forward-other-account.test.tsx b/components/email/__tests__/composer-forward-other-account.test.tsx new file mode 100644 index 000000000..06c407c20 --- /dev/null +++ b/components/email/__tests__/composer-forward-other-account.test.tsx @@ -0,0 +1,299 @@ +import { render, screen, fireEvent, act } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import React from 'react'; +import { EmailComposer } from '../email-composer'; +import { useAuthStore } from '@/stores/auth-store'; + +// A forward carries the original message's attachments as part blobIds of the +// account that holds it. When the forward goes out through another account - +// a From from another account's identities - Email/set failed with +// blobNotFound, since blobs are account-scoped. The parts must be copied to +// the composing account first: once, and only when the accounts differ. + +// ─── Heavy component mocks (mirrors composer-draft-attachments.test.tsx) ───── + +vi.mock('@/components/email/rich-text-editor', () => ({ + RichTextEditor: () => React.createElement('div', { 'data-testid': 'rich-text-editor' }), +})); + +vi.mock('@/components/plugins/plugin-slot', () => ({ PluginSlot: () => null })); +vi.mock('@/components/identity/sub-address-helper', () => ({ SubAddressHelper: () => null })); +vi.mock('@/components/templates/template-picker', () => ({ TemplatePicker: () => null })); +vi.mock('@/components/templates/template-form', () => ({ TemplateForm: () => null })); +vi.mock('@/components/files/file-preview-modal', () => ({ FilePreviewModal: () => null })); +vi.mock('@/hooks/use-focus-trap', () => ({ + useFocusTrap: () => ({ current: null }), +})); +// Two connected logins. Which identities each one offers is set per test. +const multi = vi.hoisted(() => ({ allIdentities: [] as Array<{ id: string; email: string; name: string }> })); +vi.mock('@/hooks/use-pro-multi-account-identities', () => ({ + useProMultiAccountIdentities: () => ({ enabled: true, groups: [], allIdentities: multi.allIdentities }), + stripCrossAccountIdentityPrefix: (id: string) => { + const at = id.indexOf('::'); + return at < 0 ? { localAccountId: null, rawId: id } : { localAccountId: id.slice(0, at), rawId: id.slice(at + 2) }; + }, +})); + +// ─── Store mocks ────────────────────────────────────────────────────────────── + +vi.mock('@/stores/auth-store', () => { + const state = { + client: null, + identities: [], + primaryIdentity: null, + isAuthenticated: false, + isDemoMode: false, + activeAccountId: null, + connectionLost: false, + getClientForAccount: () => undefined, + getAllConnectedClients: () => new Map(), + syncIdentities: () => {}, + refreshIdentities: async () => {}, + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useAuthStore: hook }; +}); + +vi.mock('@/stores/identity-store', () => { + const state = { + identities: [{ id: 'id-me', email: 'me@example.com', name: 'Me' }], + defaultIdentityId: 'id-me', + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useIdentityStore: hook }; +}); + +vi.mock('@/stores/account-store', () => { + const state = { accounts: [], getAccountById: (id: string) => ({ id, isConnected: true }) }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useAccountStore: hook }; +}); + +vi.mock('@/stores/email-store', () => { + const state = { + draftSaveEnabled: false, + sendRawEmail: async () => ({ sent: true }), + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useEmailStore: hook }; +}); + +vi.mock('@/stores/settings-store', () => { + const state = { + timeFormat: '24h', + plainTextMode: false, + subAddressDelimiter: '+', + autoSelectReplyIdentity: true, + attachmentReminderEnabled: false, + attachmentReminderKeywords: [], + emptySubjectWarningEnabled: true, + sendDelaySeconds: 0, + signaturePosition: 'above_quote', + signatureSeparatorEnabled: false, + requestReadReceiptDefault: false, + addTrustedSender: () => {}, + trustedSendersAddressBook: null, + updateSetting: () => {}, + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useSettingsStore: hook }; +}); + +vi.mock('@/stores/contact-store', () => { + const state = { + contacts: [], + getAutocomplete: async () => [], + addToTrustedSendersBook: async () => {}, + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useContactStore: hook }; +}); + +vi.mock('@/stores/template-store', () => { + const state = { templates: [], addTemplate: async () => {} }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useTemplateStore: hook }; +}); + +// ─── Misc dependency mocks ──────────────────────────────────────────────────── + +vi.mock('@/stores/toast-store', () => ({ + toast: { info: () => {}, error: () => {}, success: () => {} }, +})); + +vi.mock('@/lib/plugin-hooks', () => ({ + emailHooks: { + onComposerOpen: { call: async () => [] }, + onRecipientChange: { call: async () => [] }, + getRecipientSuggestions: { call: async () => [] }, + onRecipientChipsChange: { transform: async (chips: unknown) => chips }, + onDraftChange: { emit: () => {} }, + onBeforeDraftAutoSave: { transform: async (draft: unknown) => draft }, + onBeforeEmailSend: { intercept: async () => true }, + onComposeSend: { intercept: async () => true }, + onTransformOutgoingEmail: { transform: async (email: unknown) => email }, + }, + contactHooks: { + search: { call: async () => [] }, + onProvideRecipientSuggestions: { transform: async (initial: unknown) => initial }, + }, +})); + +vi.mock('@/lib/email-sanitization', () => ({ + sanitizeSignatureHtml: (v: string) => v, + sanitizeEmailHtml: (v: string) => v, + parseHtmlSafely: (html: string) => new DOMParser().parseFromString(html, 'text/html'), +})); + +vi.mock('@/lib/email-threading', () => ({ + computeReplyThreadingHeaders: () => ({ inReplyTo: [], references: [] }), +})); +vi.mock('@/lib/signature-utils', () => ({ + appendPlainTextSignature: (body: string) => body, + getPlainTextSignature: () => '', + plainTextBodyHasSignature: () => false, + plainTextBodyWithoutSignature: (body: string) => body, +})); +vi.mock('@/lib/sub-addressing', () => ({ generateSubAddress: () => '' })); +vi.mock('@/lib/debug', () => ({ debug: { log: () => {}, warn: () => {}, error: () => {} } })); +vi.mock('@/components/email/quoted-html', () => ({ + buildQuotedHtmlBlock: () => '', + serializeEditorContent: () => '', +})); +vi.mock('@/lib/template-utils', () => ({ substitutePlaceholders: (s: string) => s })); + +// ─── Tests ──────────────────────────────────────────────────────────────────── + +const SCAN = { blobId: 'blob-src', name: 'scan.pdf', type: 'application/pdf', size: 5 }; +const ACTIVE_IDENTITY = { id: 'acct-active::id-me', email: 'me@example.com', name: 'Me' }; +const SOURCE_IDENTITY = { id: 'acct-src::id-src', email: 'me@other.example', name: 'Me' }; + +function draftClient(prefix: string) { + return { + uploadBlob: vi.fn().mockResolvedValue({ blobId: 'blob-copied' }), + fetchBlobArrayBuffer: vi.fn().mockResolvedValue(new ArrayBuffer(5)), + createDraft: vi.fn() + .mockResolvedValueOnce(`${prefix}-draft-1`) + .mockResolvedValueOnce(`${prefix}-draft-2`), + getEmail: vi.fn().mockResolvedValue({ id: `${prefix}-draft-1`, attachments: [] }), + hasDelayedSend: () => false, + getMaxDelayedSend: () => 0, + }; +} + +function mockClients() { + const activeClient = draftClient('active'); + const sourceClient = draftClient('source'); + useAuthStore.setState({ + client: activeClient as never, + activeAccountId: 'acct-active' as never, + getClientForAccount: ((id: string) => ( + id === 'acct-src' ? sourceClient : id === 'acct-active' ? activeClient : undefined + )) as never, + }); + return { activeClient, sourceClient }; +} + +async function forwardAndAutosave(accountId?: string) { + render( + , + ); + // Make the draft dirty so the autosave debounce arms. + const subject = screen.getByDisplayValue(/Scans/); + fireEvent.change(subject, { target: { value: 'Scans for you' } }); + await act(async () => { await vi.advanceTimersByTimeAsync(2500); }); +} + +describe('forwarding a message held by another account', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + useAuthStore.setState({ + client: null, + activeAccountId: null, + getClientForAccount: (() => undefined) as never, + }); + vi.clearAllMocks(); + }); + + it('copies the forwarded parts to the sending account before saving, once', async () => { + // The holding account has no identity of its own, so the forward goes out + // from the active account's address. + multi.allIdentities = [ACTIVE_IDENTITY]; + const { activeClient, sourceClient } = mockClients(); + await forwardAndAutosave('acct-src'); + + expect(sourceClient.fetchBlobArrayBuffer).toHaveBeenCalledWith('blob-src', 'scan.pdf', 'application/pdf'); + expect(activeClient.uploadBlob).toHaveBeenCalledTimes(1); + const uploaded = activeClient.uploadBlob.mock.calls[0][0] as File; + expect(uploaded.name).toBe('scan.pdf'); + expect(uploaded.type).toBe('application/pdf'); + expect(activeClient.createDraft).toHaveBeenCalledTimes(1); + expect(activeClient.createDraft.mock.calls[0][8]).toEqual([ + { blobId: 'blob-copied', name: 'scan.pdf', type: 'application/pdf', size: 5 }, + ]); + + // A later save reuses the copy instead of copying again. + fireEvent.change(screen.getByDisplayValue('Scans for you'), { target: { value: 'Scans, again' } }); + await act(async () => { await vi.advanceTimersByTimeAsync(2500); }); + expect(activeClient.createDraft).toHaveBeenCalledTimes(2); + expect(activeClient.createDraft.mock.calls[1][8]).toEqual([ + { blobId: 'blob-copied', name: 'scan.pdf', type: 'application/pdf', size: 5 }, + ]); + expect(sourceClient.fetchBlobArrayBuffer).toHaveBeenCalledTimes(1); + expect(activeClient.uploadBlob).toHaveBeenCalledTimes(1); + }); + + it('references the original blobs when it goes out from the account that holds them', async () => { + multi.allIdentities = [ACTIVE_IDENTITY, SOURCE_IDENTITY]; + const { activeClient, sourceClient } = mockClients(); + await forwardAndAutosave('acct-src'); + + expect(sourceClient.fetchBlobArrayBuffer).not.toHaveBeenCalled(); + expect(sourceClient.uploadBlob).not.toHaveBeenCalled(); + expect(activeClient.createDraft).not.toHaveBeenCalled(); + expect(sourceClient.createDraft.mock.calls[0][8]).toEqual([ + { blobId: 'blob-src', name: 'scan.pdf', type: 'application/pdf', size: 5 }, + ]); + }); + + it('leaves the blobs alone for a message on the active account', async () => { + multi.allIdentities = [ACTIVE_IDENTITY, SOURCE_IDENTITY]; + const { activeClient, sourceClient } = mockClients(); + await forwardAndAutosave(undefined); + + expect(sourceClient.fetchBlobArrayBuffer).not.toHaveBeenCalled(); + expect(activeClient.uploadBlob).not.toHaveBeenCalled(); + expect(activeClient.createDraft.mock.calls[0][8]).toEqual([ + { blobId: 'blob-src', name: 'scan.pdf', type: 'application/pdf', size: 5 }, + ]); + }); +}); diff --git a/components/email/email-composer.tsx b/components/email/email-composer.tsx index ca7d7b11f..9558ee586 100644 --- a/components/email/email-composer.tsx +++ b/components/email/email-composer.tsx @@ -238,6 +238,10 @@ interface EmailComposerProps { type ComposerAttachment = { file?: File; + // Login whose server holds `blobId`, set for parts of the original message + // on a forward - the message may go out through another account, which + // cannot resolve them. See rehomeForeignBlobs. + sourceAccountId?: string; name: string; type: string; size: number; @@ -661,11 +665,12 @@ export function EmailComposer({ type: att.type || 'application/octet-stream', size: att.size, blobId: att.blobId, + sourceAccountId: sourceAccountId ?? undefined, })); } return []; }); - const inlineImagesRef = useRef>([]); + const inlineImagesRef = useRef>([]); const fileInputRef = useRef(null); const [validationErrors, setValidationErrors] = useState<{ to?: boolean; body?: boolean }>({}); const [shakeField, setShakeField] = useState(null); @@ -742,6 +747,46 @@ export function EmailComposer({ const composerClientRef = useRef(composerClient); composerClientRef.current = composerClient; const currentIdentityRawId = currentIdentityParts.rawId ?? currentIdentity?.id; + // Login that owns the blobs uploaded through `composerClient`. + const composerAccountId = currentIdentityParts.localAccountId ?? activeAccountId ?? undefined; + + // Forwarded attachments and quoted inline images reference part blobs of the + // original message, which only the account that holds it can resolve. When + // the message goes out through another account - a From picked by hand from + // another account's identities - those blobs are copied over first, or + // Email/set fails with blobNotFound. Blob/copy cannot do it: every login is + // its own JMAP session. Each part is copied once; later saves reuse it. + const rehomeForeignBlobs = async (): Promise => { + const target = composerClient; + if (!target || !composerAccountId) return; + const isForeign = (owner?: string) => !!owner && owner !== composerAccountId; + const copyBlob = async (part: { blobId: string; name: string; type: string; sourceAccountId?: string }) => { + const source = useAuthStore.getState().getClientForAccount(part.sourceAccountId!); + if (!source) throw new Error(`No connected client for account ${part.sourceAccountId}`); + const buffer = await source.fetchBlobArrayBuffer(part.blobId, part.name, part.type); + const { blobId } = await target.uploadBlob(new File([buffer], part.name, { type: part.type })); + return blobId; + }; + + const moved = new Map(); + for (const att of attachmentsRef.current) { + if (!att.blobId || att.uploading || !isForeign(att.sourceAccountId)) continue; + const blobId = await copyBlob({ ...att, blobId: att.blobId }); + moved.set(att, { ...att, blobId, sourceAccountId: composerAccountId }); + } + if (moved.size) { + const remap = (list: ComposerAttachment[]) => list.map(att => moved.get(att) ?? att); + // Ref first, synchronously: callers read attachmentsRef right after. + attachmentsRef.current = remap(attachmentsRef.current); + setAttachments(remap); + } + + for (const entry of inlineImagesRef.current) { + if (!isForeign(entry.sourceAccountId)) continue; + entry.blobId = await copyBlob(entry); + entry.sourceAccountId = composerAccountId; + } + }; // RFC 5322 §3.6.4 threading for this message: computed from the original // on a reply (not a forward), carried over from a re-opened reply draft. // Drafts store it too, so a reply stays in its thread when re-opened or @@ -962,7 +1007,12 @@ export function EmailComposer({ useEffect(() => { if (plainTextMode) return; if (mode !== 'reply' && mode !== 'replyAll' && mode !== 'forward') return; - if (!composerClient || !replyTo?.attachments?.length) return; + // The quoted parts live on the account that holds the original message, + // which is not necessarily the one this message goes out through. + const originClient = (sourceAccountId && sourceAccountId !== composerAccountId + ? useAuthStore.getState().getClientForAccount(sourceAccountId) + : undefined) ?? composerClient; + if (!originClient || !replyTo?.attachments?.length) return; // Hydrate every cid the quoted body actually renders as an , rather // than only parts declared `image/*` + `inline`. Some clients (notably @@ -990,6 +1040,7 @@ export function EmailComposer({ name: att.name || 'inline', size: att.size, dataUrl: '', + sourceAccountId: sourceAccountId ?? undefined, }); } @@ -999,7 +1050,7 @@ export function EmailComposer({ for (const att of inlineAtts) { if (!att.cid) continue; try { - const buffer = await composerClient.fetchBlobArrayBuffer( + const buffer = await originClient.fetchBlobArrayBuffer( att.blobId, att.name || 'inline', att.type, @@ -1846,9 +1897,16 @@ export function EmailComposer({ return null; } + try { + await rehomeForeignBlobs(); + } catch (error) { + // The save below then fails with blobNotFound and reports it. + debug.warn('email', 'Failed to copy forwarded parts to the sending account:', error); + } + // Prepare attachments for draft. cid/disposition ride along so inline // parts of a re-opened draft keep matching the body's cid: references. - const uploadedAttachments = attachments + const uploadedAttachments = attachmentsRef.current .filter(att => att.blobId && !att.uploading) .map(att => ({ blobId: att.blobId!, @@ -2343,6 +2401,15 @@ export function EmailComposer({ ? (signatureAlreadyInBody ? body : appendPlainTextSignature(body, signatureIdentity, signatureOpts)) : (signatureAlreadyInBody ? htmlToPlainText(body) : appendPlainTextSignature(htmlToPlainText(body), signatureIdentity, signatureOpts)); + // Before any blobId is read: forwarded parts may still sit on the account + // that holds the original message. + try { + await rehomeForeignBlobs(); + } catch (error) { + // The send below then fails with blobNotFound and reports it. + debug.warn('email', 'Failed to copy forwarded parts to the sending account:', error); + } + const rewritten = plainTextMode ? null : rewriteInlineImages(body); const finalHtmlBody = plainTextMode ? undefined