Merge pull request #1142 from lucamzanon/fix/forward-parts-from-other-account

fix: copy forwarded parts to the sending account before saving or sending

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<typeof state>) => 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<typeof state>) => 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<typeof state>) => 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<typeof state>) => 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<typeof state>) => 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<typeof state>) => 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<typeof state>) => 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(
+    <EmailComposer
+      mode="forward"
+      replyTo={{ subject: 'Scans', accountId, attachments: [SCAN] }}
+      onClose={vi.fn()}
+    />,
+  );
+  // 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 },
+    ]);
+  });
+});

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<Array<{ cid: string; blobId: string; type: string; name: string; size: number; dataUrl: string }>>([]);
+  const inlineImagesRef = useRef<Array<{ cid: string; blobId: string; type: string; name: string; size: number; dataUrl: string; sourceAccountId?: string }>>([]);
   const fileInputRef = useRef<HTMLInputElement>(null);
   const [validationErrors, setValidationErrors] = useState<{ to?: boolean; body?: boolean }>({});
   const [shakeField, setShakeField] = useState<string | null>(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<void> => {
+    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<ComposerAttachment, ComposerAttachment>();
+    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 <img>, 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