Merge pull request #1125 from guisea/fix/inline-attachments-reply-forward

fix(email-composer): normalize bracketed content-ids for inline reply/forward images

components/email/__tests__/composer-inline-image-cid.test.tsx

@@ -0,0 +1,277 @@
+import { render, screen, fireEvent, waitFor } from '@testing-library/react';
+import { describe, it, expect, vi, afterEach } from 'vitest';
+import React from 'react';
+import { EmailComposer } from '../email-composer';
+import { useAuthStore } from '@/stores/auth-store';
+
+// ─── 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 }) }));
+vi.mock('@/hooks/use-pro-multi-account-identities', () => ({
+  useProMultiAccountIdentities: () => ({ enabled: false, groups: [], allIdentities: [] }),
+  stripCrossAccountIdentityPrefix: (id: string) => ({ localAccountId: null, rawId: id }),
+}));
+
+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: () => undefined };
+  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 };
+});
+
+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('@/lib/template-utils', () => ({ substitutePlaceholders: (s: string) => s }));
+
+// ─── Tests ────────────────────────────────────────────────────────────────────
+
+/**
+ * Senders may declare `Content-ID: <logo123@sender>` (angle brackets kept)
+ * while the body always references it bracket-free as `src="cid:logo123@sender"`.
+ * The viewers already normalize both sides before matching; the reply/forward
+ * hydration path didn't, so any such image silently stayed a blank placeholder
+ * in the composer and was dropped from the outgoing message on send.
+ */
+describe('reply/forward inline image cid normalization', () => {
+  afterEach(() => {
+    useAuthStore.setState({ client: null });
+    vi.clearAllMocks();
+  });
+
+  const PNG_MAGIC = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]).buffer;
+
+  function mockClient() {
+    const fetchBlobArrayBuffer = vi.fn().mockResolvedValue(PNG_MAGIC);
+    useAuthStore.setState({
+      client: {
+        fetchBlobArrayBuffer,
+        hasDelayedSend: () => false,
+        getMaxDelayedSend: () => 0,
+      } as never,
+    });
+    return { fetchBlobArrayBuffer };
+  }
+
+  it('sends a reply with the image inlined by cid, not duplicated as a regular attachment', async () => {
+    const { fetchBlobArrayBuffer } = mockClient();
+    const onSend = vi.fn();
+
+    render(
+      <EmailComposer
+        mode="reply"
+        onSend={onSend}
+        replyTo={{
+          from: [{ email: 'sender@example.com', name: 'Sender' }],
+          subject: 'Hello',
+          htmlBody: '<p>Hi</p><img src="cid:logo123@sender">',
+          quoteHeaderHtml: 'On X, Sender wrote:',
+          attachments: [
+            { blobId: 'blob-1', name: 'logo.png', type: 'image/png', size: 100, cid: '<logo123@sender>' },
+          ],
+        }}
+        onClose={vi.fn()}
+      />
+    );
+
+    // Hydration: the placeholder gets swapped for the fetched blob's data URL.
+    await waitFor(() => expect(fetchBlobArrayBuffer).toHaveBeenCalledWith('blob-1', 'logo.png', 'image/png'));
+
+    // Reply auto-fills "to" from replyTo.from, so Send is reachable immediately.
+    fireEvent.click(screen.getAllByTestId('composer-send')[0]);
+    await waitFor(() => expect(onSend).toHaveBeenCalledTimes(1));
+
+    const sent = onSend.mock.calls[0][0] as { htmlBody?: string; attachments?: Array<{ cid?: string; blobId: string; disposition?: string }> };
+    // The data URL must have been rewritten back to a cid: reference for sending.
+    expect(sent.htmlBody).toContain('cid:logo123@sender');
+    expect(sent.htmlBody).not.toContain('data:image');
+    // Exactly one attachment for the one image - a bracket-mismatch used to
+    // mean it was never matched at all; matching on both regular AND inline
+    // paths would instead send the same blob twice (see forward test below).
+    expect(sent.attachments).toEqual([
+      expect.objectContaining({ blobId: 'blob-1', cid: 'logo123@sender', disposition: 'inline' }),
+    ]);
+  });
+
+  it('does not duplicate a forwarded inline image (declared octet-stream, no disposition) as a regular attachment', async () => {
+    const { fetchBlobArrayBuffer } = mockClient();
+    const onSend = vi.fn();
+
+    render(
+      <EmailComposer
+        mode="forward"
+        onSend={onSend}
+        replyTo={{
+          from: [{ email: 'sender@example.com', name: 'Sender' }],
+          subject: 'Hello',
+          htmlBody: '<p>Hi</p><img src="cid:logo123@sender">',
+          quoteHeaderHtml: 'Forwarded message',
+          // Foxmail-style part (#543): no inline disposition, generic type -
+          // only the cid: body reference marks it as embedded.
+          attachments: [
+            { blobId: 'blob-1', name: 'logo.png', type: 'application/octet-stream', size: 100, cid: '<logo123@sender>' },
+          ],
+        }}
+        onClose={vi.fn()}
+      />
+    );
+
+    await waitFor(() => expect(fetchBlobArrayBuffer).toHaveBeenCalledWith('blob-1', 'logo.png', 'application/octet-stream'));
+
+    // The forward attachment-list filter must have recognized blob-1 as
+    // embedded and kept it out of the regular attachment chips.
+    expect(screen.queryByText('logo.png')).not.toBeInTheDocument();
+
+    // Forward doesn't auto-fill "to" - add a recipient so Send is reachable.
+    // A trailing delimiter is needed: a bare single-address paste is left as
+    // uncommitted input text rather than immediately chipped.
+    fireEvent.paste(screen.getByPlaceholderText('to_placeholder'), {
+      clipboardData: { getData: () => 'bob@example.com;' },
+    });
+
+    fireEvent.click(screen.getAllByTestId('composer-send')[0]);
+    await waitFor(() => expect(onSend).toHaveBeenCalledTimes(1));
+
+    const sent = onSend.mock.calls[0][0] as { attachments?: Array<{ cid?: string; blobId: string }> };
+    // Same blobId must appear exactly once, as the re-typed inline part -
+    // never also as a plain attachment (that would send it twice).
+    expect(sent.attachments).toEqual([
+      expect.objectContaining({ blobId: 'blob-1', cid: 'logo123@sender' }),
+    ]);
+  });
+});

components/email/email-composer.tsx

@@ -121,6 +121,7 @@ import type { ComposePrefill } from "@/lib/plugin-compose-gate";
 import { composerContextMode, composerMounted, composerUnmounted, draftSaved } from "@/lib/plugin-composer-hooks";
 import { isValidEmail } from "@/lib/validation";
 import { buildMentionCandidates } from "@/lib/recipient-mentions";
+import { normalizeCid } from "@/lib/attachment-visibility";
 import { RichTextEditor } from "@/components/email/rich-text-editor";
 import { isRecipientMentionActive } from "@/components/email/recipient-mention";
 import type { Editor } from "@tiptap/react";
@@ -867,9 +868,10 @@ export function EmailComposer({
       const embeddedCids = collectInlineImageCids(body);
       return replyTo.attachments
         // Skip inline cid-referenced images - they're embedded in the forwarded HTML body
-        // (matches the viewer's hideInlineImageAttachments logic).
+        // (matches the viewer's hideInlineImageAttachments logic). att.cid may still
+        // carry angle brackets while embeddedCids (parsed from the body) never does.
         .filter(att => !(att.cid && (
-          embeddedCids.has(att.cid) ||
+          embeddedCids.has(normalizeCid(att.cid)) ||
           (att.disposition === 'inline' && (att.type || '').startsWith('image/'))
         )))
         .map(att => ({
@@ -1302,15 +1304,20 @@ export function EmailComposer({
     // lenient (email-viewer.tsx: `att.cid && att.blobId`), so reading and
     // replying now agree. The real type is sniffed from the bytes below.
     const embeddedCids = collectInlineImageCids(body);
-    const inlineAtts = replyTo.attachments.filter((att) =>
-      att.cid && att.blobId && embeddedCids.has(att.cid)
-    );
+    // Attachment cids may still carry the RFC angle brackets (e.g.
+    // `<logo123@sender>`) while `src="cid:logo123@sender"` in the body never
+    // does - normalize here the same way email-viewer.tsx and
+    // thread-conversation-view.tsx do, so the comparisons below actually
+    // match (#912: inline images silently missing from every reply/forward).
+    const inlineAtts = replyTo.attachments
+      .filter((att) => att.cid && att.blobId)
+      .map((att) => ({ ...att, cid: normalizeCid(att.cid!) }))
+      .filter((att) => embeddedCids.has(att.cid));
     if (inlineAtts.length === 0) return;
 
     // Seed the ref synchronously so a fast Send still attaches the right blobs
     // even if the FileReader work below hasn't resolved yet.
     for (const att of inlineAtts) {
-      if (!att.cid) continue;
       if (inlineImagesRef.current.some((e) => e.cid === att.cid)) continue;
       inlineImagesRef.current.push({
         cid: att.cid,
@@ -1327,7 +1334,6 @@ export function EmailComposer({
     (async () => {
       const updates = new Map<string, string>();
       for (const att of inlineAtts) {
-        if (!att.cid) continue;
         try {
           const buffer = await originClient.fetchBlobArrayBuffer(
             att.blobId,

components/email/email-viewer.tsx

@@ -6,7 +6,7 @@ import { emailExportFilename, attachmentDownloadFilename, attachmentsBundleFilen
 import { EML_IMPORT_ACCEPT, expandImportableEmails } from "@/lib/eml-import";
 import { applyNewTabToAnchor, emailIframeCsp, escapeHtml, isExternalResourceUrl, isOpenableLinkHref, plainTextToSafeHtml, sanitizeEmailBodyForIframe, sanitizeEmailHtml, sanitizePlainTextRenderedHtml } from "@/lib/email-sanitization";
 import { getRenderableHtmlBody } from "@/lib/email-body-selection";
-import { collectReferencedCids, isEmbeddedInBody } from "@/lib/attachment-visibility";
+import { collectReferencedCids, isEmbeddedInBody, normalizeCid } from "@/lib/attachment-visibility";
 import { collapsePlainTextQuotes, setupQuoteCollapse } from "@/lib/quote-collapse";
 import { DARK_PENDING_CLASS, applyEmailDarkMode, emailColorScheme, emailDarkModeCss, emailHasNativeDarkMode as htmlHasNativeDarkMode, resolveColorSchemeMedia } from "@/lib/email-dark-mode";
 import { fitEmailBodyWidth } from "@/lib/email-fit-width";
@@ -2112,7 +2112,7 @@ export function EmailViewer({
       decryptedCidAttachments.forEach((att) => {
         const bytes = getAttachmentContentBytes(att);
         if (!bytes) return;
-        const cidValue = att.contentId!.replace(/^<|>$/g, '');
+        const cidValue = normalizeCid(att.contentId!);
         const buffer = bytes.buffer.slice(bytes.byteOffset, bytes.byteOffset + bytes.byteLength) as ArrayBuffer;
         // Sender-typed part: never let a script-bearing MIME type reach a
         // blob: URL in our origin (GHSA-xvjh-v9c6-qcvc).
@@ -2148,7 +2148,7 @@ export function EmailViewer({
     async function fetchCidBlobs() {
       const urls: Record<string, string> = {};
       await Promise.all(cidAttachments.map(async (att) => {
-        const cidValue = att.cid!.replace(/^<|>$/g, '');
+        const cidValue = normalizeCid(att.cid!);
         try {
           // The download URL echoes the sender-declared Content-Type back as
           // the Blob type; re-type anything that could execute as our origin

components/email/thread-conversation-view.tsx

@@ -8,7 +8,7 @@ import { emailDisplayDate } from "@/lib/email-date";
 import { getRenderableHtmlBody } from "@/lib/email-body-selection";
 import { findVerificationCode, verificationCodeBodyText } from "@/lib/verification-code";
 import { VerificationCodeChip } from "./verification-code-chip";
-import { collectReferencedCids, isEmbeddedInBody } from "@/lib/attachment-visibility";
+import { collectReferencedCids, isEmbeddedInBody, normalizeCid } from "@/lib/attachment-visibility";
 import { collapsePlainTextQuotes, setupQuoteCollapse } from "@/lib/quote-collapse";
 import { fitEmailBodyWidth } from "@/lib/email-fit-width";
 import { DARK_PENDING_CLASS, applyEmailDarkMode, emailDarkModeCss } from "@/lib/email-dark-mode";
@@ -547,7 +547,7 @@ export function ConversationEmailBody({
     async function fetchCidBlobs() {
       const urls: Record<string, string> = {};
       await Promise.all(cidAttachments.map(async (att) => {
-        const cidValue = att.cid!.replace(/^<|>$/g, '');
+        const cidValue = normalizeCid(att.cid!);
         try {
           // Re-type sender-declared script-bearing parts before they become a
           // blob: URL in our origin (GHSA-xvjh-v9c6-qcvc).

lib/attachment-visibility.ts

@@ -21,6 +21,16 @@ const CID_REFERENCE = /\bcid:([^"'\s)]+)/gi;
 
 const EMPTY: ReadonlySet<string> = new Set();
 
+/**
+ * Strips the RFC angle brackets a Content-ID header may still carry (e.g.
+ * `<logo123@sender>`). Body `cid:` references never have them, so every
+ * comparison between an attachment's `cid`/`contentId` and a body reference
+ * must normalize through this first.
+ */
+export function normalizeCid(cid: string): string {
+  return cid.replace(/^<|>$/g, '');
+}
+
 /**
  * Content-IDs a raw HTML body references via `cid:` URLs (img src, table
  * background, style url(...) - anything), exactly as written. `null` (the
@@ -65,6 +75,5 @@ export function isEmbeddedInBody(
   if (att.disposition === 'attachment') return false;
   const genericType = !type || type === 'application/octet-stream';
   if (!isImage && !genericType) return false;
-  // The Content-ID may still carry its angle brackets; body references never do.
-  return referencedCids.has(att.cid.replace(/^<|>$/g, ''));
+  return referencedCids.has(normalizeCid(att.cid));
 }