From 70aaf0aac1f5d81be836c50a214042fa31a35881 Mon Sep 17 00:00:00 2001 From: Stefan Hildebrandt <695494+hildebrandttk@users.noreply.github.com> Date: Wed, 24 Jun 2026 19:49:57 +0200 Subject: [PATCH 1/2] fix: stop unified-mailbox from mutating client-returned email objects fetchUnifiedEmails, fanOutUnifiedQuery and the cross-account fanOutCrossQuery stamped accountId/accountLabel/source* directly onto each email object returned by the per-account client. Those objects are shared references; mutating them in place could surprise any caller that retained them (and corrupt an account-state snapshot). Decorate shallow copies instead, at all three fan-out sites. The original fix/unified-mailbox-no-mutation branch predated the cross-account "All accounts" feature and only covered two sites; this re-applies the fix to main's current code, including the third (shared/group) fan-out site, and preserves all five stamped fields. Flips the characterisation test to assert the client's object is left untouched. --- lib/__tests__/unified-mailbox.test.ts | 11 +++--- lib/unified-mailbox.ts | 57 +++++++++++++++------------ 2 files changed, 38 insertions(+), 30 deletions(-) diff --git a/lib/__tests__/unified-mailbox.test.ts b/lib/__tests__/unified-mailbox.test.ts index a03c90b3..2fb1780a 100644 --- a/lib/__tests__/unified-mailbox.test.ts +++ b/lib/__tests__/unified-mailbox.test.ts @@ -120,16 +120,17 @@ describe('fetchUnifiedEmails', () => { expect(result).toEqual({ emails: [], total: 0, hasMore: false, errors: new Map() }); }); - it('CHARACTERISATION: mutates the source email objects in place (shared reference)', async () => { + it('does NOT mutate the source email objects (decorates copies)', async () => { const original = makeEmail('m1', '2026-01-01T00:00:00Z'); const acc = makeAccount( { accountId: 'A', accountLabel: 'Label A', mailboxes: [makeMailbox({ role: 'inbox' })] }, { getEmails: vi.fn(async (): Promise => ({ emails: [original], total: 1, hasMore: false })) }, ); - await fetchUnifiedEmails([acc], 'inbox', 20, 0); - // The very object passed back by the client was mutated, not a copy. - expect(original.accountId).toBe('A'); - expect(original.accountLabel).toBe('Label A'); + const res = await fetchUnifiedEmails([acc], 'inbox', 20, 0); + // The returned email carries the account info, but the client's object is untouched. + expect(res.emails[0]).toMatchObject({ id: 'm1', accountId: 'A', accountLabel: 'Label A' }); + expect('accountId' in original).toBe(false); + expect('accountLabel' in original).toBe(false); }); }); diff --git a/lib/unified-mailbox.ts b/lib/unified-mailbox.ts index 9bd0c11d..5789068a 100644 --- a/lib/unified-mailbox.ts +++ b/lib/unified-mailbox.ts @@ -119,16 +119,19 @@ export async function fetchUnifiedEmails( const { account, result } = outcome.value; - // Decorate each email with the source account info. - for (const email of result.emails) { - email.accountId = account.accountId; - email.accountLabel = account.accountLabel; - email.sourceClientAccountId = account.clientAccountId; - email.sourceAccountId = account.jmapAccountId; - email.sourceFolder = resolveSourceFolderName(email, account.mailboxes); - } + // Decorate each email with the source account info. The per-account client + // returns shared object references; decorate shallow copies instead of + // mutating them in place so retained callers/snapshots aren't corrupted. + const decorated = result.emails.map((email) => ({ + ...email, + accountId: account.accountId, + accountLabel: account.accountLabel, + sourceClientAccountId: account.clientAccountId, + sourceAccountId: account.jmapAccountId, + sourceFolder: resolveSourceFolderName(email, account.mailboxes), + })); - mergedEmails = mergedEmails.concat(result.emails); + mergedEmails = mergedEmails.concat(decorated); totalSum += result.total; if (result.hasMore) { anyHasMore = true; @@ -247,14 +250,16 @@ async function fanOutUnifiedQuery( for (const outcome of results) { if (outcome.status !== 'fulfilled' || outcome.value === null) continue; const { account, result } = outcome.value; - for (const email of result.emails) { - email.accountId = account.accountId; - email.accountLabel = account.accountLabel; - email.sourceClientAccountId = account.clientAccountId; - email.sourceAccountId = account.jmapAccountId; - email.sourceFolder = resolveSourceFolderName(email, account.mailboxes); - } - mergedEmails = mergedEmails.concat(result.emails); + // Decorate shallow copies, not the shared client-returned objects. + const decorated = result.emails.map((email) => ({ + ...email, + accountId: account.accountId, + accountLabel: account.accountLabel, + sourceClientAccountId: account.clientAccountId, + sourceAccountId: account.jmapAccountId, + sourceFolder: resolveSourceFolderName(email, account.mailboxes), + })); + mergedEmails = mergedEmails.concat(decorated); totalSum += result.total; if (result.hasMore) anyHasMore = true; } @@ -383,14 +388,16 @@ async function fanOutCrossQuery( for (const outcome of results) { if (outcome.status !== 'fulfilled' || outcome.value === null) continue; const { account, result } = outcome.value; - for (const email of result.emails) { - email.accountId = account.accountId; - email.accountLabel = account.accountLabel; - email.sourceClientAccountId = account.clientAccountId; - email.sourceAccountId = account.jmapAccountId; - email.sourceFolder = resolveSourceFolderName(email, account.mailboxes); - } - mergedEmails = mergedEmails.concat(result.emails); + // Decorate shallow copies, not the shared client-returned objects. + const decorated = result.emails.map((email) => ({ + ...email, + accountId: account.accountId, + accountLabel: account.accountLabel, + sourceClientAccountId: account.clientAccountId, + sourceAccountId: account.jmapAccountId, + sourceFolder: resolveSourceFolderName(email, account.mailboxes), + })); + mergedEmails = mergedEmails.concat(decorated); totalSum += result.total; if (result.hasMore) anyHasMore = true; } From d863b1fd4ba39109cb70fec36d4f7cbb1cb413cd Mon Sep 17 00:00:00 2001 From: Stefan Hildebrandt <695494+hildebrandttk@users.noreply.github.com> Date: Wed, 24 Jun 2026 20:04:53 +0200 Subject: [PATCH 2/2] fix: HTML-escape sender/subject in reply/forward quote header (#482) The forward quote header renders "From: Name ", but the HTML variant interpolated the sender string unescaped. In the rich-text composer the "" portion is parsed by the browser as a bogus HTML tag and dropped, so the address silently disappears - the user sees only "From: Display Name". The plain-text variant and the details panel escape correctly, which is why the address shows there. This is the regression from #367, which added the "" into the HTML string without escaping it. Fix: HTML-escape the user-controlled values (sender, subject, date) in every HTML quote-header path - the production builder in lib/quote-header.ts and the composer's inline fallback (both htmlBody and plain-body branches), for forward and reply. The reply line keeps the bare display name by design (#367), but its HTML form is now escaped too so a display name containing markup can't break out. As a side benefit this closes an HTML-injection vector: a crafted subject or display name was previously injected raw into the composer document. Adds lib/__tests__/quote-header.test.ts covering: forward text keeps "Name "; forward HTML escapes the angle brackets (address survives) and a markup subject/display name; reply stays bare-name and HTML-safe. --- components/email/email-composer.tsx | 21 +++---- lib/__tests__/quote-header.test.ts | 92 +++++++++++++++++++++++++++++ lib/quote-header.ts | 21 ++++--- 3 files changed, 116 insertions(+), 18 deletions(-) create mode 100644 lib/__tests__/quote-header.test.ts diff --git a/components/email/email-composer.tsx b/components/email/email-composer.tsx index be2d23e2..9456d5c2 100644 --- a/components/email/email-composer.tsx +++ b/components/email/email-composer.tsx @@ -11,7 +11,7 @@ import { debug } from "@/lib/debug"; import { toast } from "@/stores/toast-store"; import { useContextMenu } from "@/hooks/use-context-menu"; import { ContextMenu, ContextMenuItem, ContextMenuSeparator } from "@/components/ui/context-menu"; -import { sanitizeSignatureHtml, sanitizeEmailHtml } from "@/lib/email-sanitization"; +import { sanitizeSignatureHtml, sanitizeEmailHtml, escapeHtml } from "@/lib/email-sanitization"; import { buildReplySubject, buildForwardSubject } from "@/lib/subject-prefix"; import { isFilePreviewable } from "@/lib/file-preview"; import { buildQuotedHtmlBlock, serializeEditorContent } from "@/components/email/quoted-html"; @@ -347,9 +347,8 @@ export function EmailComposer({ const date = replyTo.receivedAt ? formatDateTime(replyTo.receivedAt, timeFormat, { weekday: 'short', year: 'numeric', month: 'short', day: 'numeric' }) : ""; const from = replyTo.from?.[0]; - const fromStr = from ? `${from.name || from.email}` : tCommon('unknown'); - // Forward "From:" shows the full sender incl. address; reply line keeps - // the bare name (reads naturally in the localized "On … wrote:" line). + // Forward "From:" and the reply "On … wrote:" line both show the full + // sender incl. address ("Name "), like Gmail/Outlook (#482). const fromStrFull = from ? (from.name && from.email && from.name !== from.email ? `${from.name} <${from.email}>` @@ -377,7 +376,7 @@ export function EmailComposer({ if (mode === 'forward') { return `${prefix}${signatureBlock}\n\n${tQuote('forwarded_separator')}\n${tQuote('from_label')}: ${fromStrFull}\n${tQuote('date_label')}: ${date}\n${tQuote('subject_label')}: ${replyTo.subject || ''}\n\n${originalText}`; } else if (mode === 'reply' || mode === 'replyAll') { - return `${prefix}${signatureBlock}\n\n${tQuote('reply_line', { date, from: fromStr })}\n${quotedText}`; + return `${prefix}${signatureBlock}\n\n${tQuote('reply_line', { date, from: fromStrFull })}\n${quotedText}`; } return prefix; } @@ -399,7 +398,7 @@ export function EmailComposer({ const date = replyTo.receivedAt ? formatDateTime(replyTo.receivedAt, timeFormat, { weekday: 'short', year: 'numeric', month: 'short', day: 'numeric' }) : ""; const from = replyTo.from?.[0]; - const fromStr = from ? `${from.name || from.email}` : tCommon('unknown'); + // Forward and reply quote lines both show the full "Name " sender (#482). const fromStrFull = from ? (from.name && from.email && from.name !== from.email ? `${from.name} <${from.email}>` @@ -434,9 +433,11 @@ export function EmailComposer({ // Build quoted content as HTML if (replyTo.htmlBody && (mode === 'reply' || mode === 'replyAll' || mode === 'forward')) { + // HTML-escape user-controlled values: an unescaped sender "Name " + // has its "" eaten as a bogus HTML tag by the rich-text editor (#482). const quoteHeader = mode === 'forward' - ? `${tQuote('forwarded_separator')}
${tQuote('from_label')}: ${fromStrFull}
${tQuote('date_label')}: ${date}
${tQuote('subject_label')}: ${replyTo.subject || ''}

` - : `${tQuote('reply_line', { date, from: fromStr })}
`; + ? `${tQuote('forwarded_separator')}
${tQuote('from_label')}: ${escapeHtml(fromStrFull)}
${tQuote('date_label')}: ${escapeHtml(date)}
${tQuote('subject_label')}: ${escapeHtml(replyTo.subject || '')}

` + : `${tQuote('reply_line', { date: escapeHtml(date), from: escapeHtml(fromStrFull) })}
`; // Embed the original as a QuotedHtml island (verbatim, schema-free) so // its layout survives the editor round-trip. Sanitize first to strip // scripts/styles/head; cid rewrite afterwards so data-cid markers @@ -450,9 +451,9 @@ export function EmailComposer({ if (replyTo.body) { const escapedOriginal = replyTo.body.replace(/&/g, '&').replace(//g, '>').replace(/\n/g, '
'); if (mode === 'forward') { - return `${prefix}${signatureBlock}

${tQuote('forwarded_separator')}
${tQuote('from_label')}: ${fromStrFull}
${tQuote('date_label')}: ${date}
${tQuote('subject_label')}: ${replyTo.subject || ''}

${escapedOriginal}`; + return `${prefix}${signatureBlock}

${tQuote('forwarded_separator')}
${tQuote('from_label')}: ${escapeHtml(fromStrFull)}
${tQuote('date_label')}: ${escapeHtml(date)}
${tQuote('subject_label')}: ${escapeHtml(replyTo.subject || '')}

${escapedOriginal}`; } else if (mode === 'reply' || mode === 'replyAll') { - return `${prefix}${signatureBlock}

${tQuote('reply_line', { date, from: fromStr })}
${escapedOriginal}
`; + return `${prefix}${signatureBlock}

${tQuote('reply_line', { date: escapeHtml(date), from: escapeHtml(fromStrFull) })}
${escapedOriginal}
`; } } return prefix; diff --git a/lib/__tests__/quote-header.test.ts b/lib/__tests__/quote-header.test.ts new file mode 100644 index 00000000..5493f646 --- /dev/null +++ b/lib/__tests__/quote-header.test.ts @@ -0,0 +1,92 @@ +import { describe, it, expect } from 'vitest'; +import { buildQuoteHeader } from '@/lib/quote-header'; + +const base = { + newTo: [] as string[], + newCc: [] as string[], + locale: 'en', + timeFormat: '24h' as const, + unknownLabel: 'Unknown', +}; + +const sender = { name: 'Display Name', email: 'user@domain.tld' }; + +describe('buildQuoteHeader (#482 — sender address survives HTML rendering)', () => { + it('forward TEXT keeps the full "Name " sender', async () => { + const h = await buildQuoteHeader({ + mode: 'forward', + email: { from: [sender], subject: 'Hello', receivedAt: '2026-01-01T10:00:00Z' }, + ...base, + }); + expect(h.text).toContain('From: Display Name '); + }); + + it('forward HTML escapes the angle brackets so the address is not eaten as a tag', async () => { + const h = await buildQuoteHeader({ + mode: 'forward', + email: { from: [sender], subject: 'Hello', receivedAt: '2026-01-01T10:00:00Z' }, + ...base, + }); + // The regression: a raw "" is parsed as an HTML tag by the + // rich-text composer and dropped, leaving only "From: Display Name". + expect(h.html).toContain('Display Name <user@domain.tld>'); + expect(h.html).not.toContain(''); + }); + + it('forward HTML escapes a subject containing markup (injection hardening)', async () => { + const h = await buildQuoteHeader({ + mode: 'forward', + email: { from: [sender], subject: 'Hi x', receivedAt: '2026-01-01T10:00:00Z' }, + ...base, + }); + expect(h.html).toContain('Hi <b>x</b>'); + expect(h.html).not.toContain('x'); + }); + + it('forward HTML escapes a malicious display name', async () => { + const h = await buildQuoteHeader({ + mode: 'forward', + email: { + from: [{ name: '', email: 'evil@x.tld' }], + subject: 'Hello', + receivedAt: '2026-01-01T10:00:00Z', + }, + ...base, + }); + expect(h.html).not.toContain('" sender, escaped in HTML', async () => { + const h = await buildQuoteHeader({ + mode: 'reply', + email: { from: [sender], subject: 'Hello', receivedAt: '2026-01-01T10:00:00Z' }, + ...base, + }); + // TEXT keeps the real angle brackets ("On , Display Name wrote:"). + expect(h.text).toContain('Display Name wrote:'); + // HTML escapes them so the address survives the rich-text editor (#482). + expect(h.html).toContain('Display Name <user@domain.tld>'); + expect(h.html).not.toContain(''); + }); + + it('reply line stays HTML-safe for a display name containing markup', async () => { + const evil = await buildQuoteHeader({ + mode: 'reply', + email: { from: [{ name: 'x', email: 'e@x.tld' }], subject: 'Hello', receivedAt: '2026-01-01T10:00:00Z' }, + ...base, + }); + expect(evil.html).not.toContain('x'); + expect(evil.html).toContain('<b>x</b>'); + }); + + it('reply line falls back to bare email when there is no display name', async () => { + const h = await buildQuoteHeader({ + mode: 'reply', + email: { from: [{ email: 'noname@x.tld' }], subject: 'Hello', receivedAt: '2026-01-01T10:00:00Z' }, + ...base, + }); + expect(h.text).toContain('noname@x.tld wrote:'); + expect(h.text).not.toContain(''); + }); +}); diff --git a/lib/quote-header.ts b/lib/quote-header.ts index c55eb046..686df5e1 100644 --- a/lib/quote-header.ts +++ b/lib/quote-header.ts @@ -8,6 +8,7 @@ import { formatDateTime } from "@/lib/utils"; import { emailHooks } from "@/lib/plugin-hooks"; +import { escapeHtml } from "@/lib/email-sanitization"; import type { QuoteHeader, QuoteHeaderContext } from "@/lib/plugin-types"; // Localized label set the caller passes in. Labels live on the client where @@ -62,10 +63,8 @@ function defaultHeader(args: BuildArgs): QuoteHeader { }) : ""; const from = email.from?.[0]; - const fromStr = from ? `${from.name || from.email}` : unknownLabel; - // Forward header "From:" shows the full sender incl. address ("Name - // "), like every mail client. The reply line keeps the bare name - // (reads more naturally in "On … wrote:"). + // Both the forward "From:" line and the reply "On … wrote:" line show the + // full sender incl. address ("Name "), like Gmail/Outlook (#482). const fromStrFull = from ? (from.name && from.email && from.name !== from.email ? `${from.name} <${from.email}>` @@ -75,13 +74,19 @@ function defaultHeader(args: BuildArgs): QuoteHeader { if (mode === "forward") { const text = `${labels.forwardedSeparator}\n${labels.fromLabel}: ${fromStrFull}\n${labels.dateLabel}: ${date}\n${labels.subjectLabel}: ${subject}\n`; - const html = `
${labels.forwardedSeparator}
${labels.fromLabel}: ${fromStrFull}
${labels.dateLabel}: ${date}
${labels.subjectLabel}: ${subject}

`; + // Escape the interpolated values for the HTML variant: the sender string is + // "Name ", and the unescaped "" would be parsed as an HTML tag + // by the rich-text composer and silently dropped (#482). Subject/name are + // likewise user-controlled. Label/separator strings are trusted i18n text. + const html = `
${labels.forwardedSeparator}
${labels.fromLabel}: ${escapeHtml(fromStrFull)}
${labels.dateLabel}: ${escapeHtml(date)}
${labels.subjectLabel}: ${escapeHtml(subject)}

`; return { html, text, wrapInBlockquote: false }; } - const replyLine = labels.formatReplyLine({ date, from: fromStr }); - const text = `${replyLine}\n`; - const html = `
${replyLine}
`; + const text = `${labels.formatReplyLine({ date, from: fromStrFull })}\n`; + // Escape the interpolated sender/date for the HTML reply line: the sender is + // now "Name ", and the unescaped "" would be parsed as an HTML + // tag by the rich-text composer and dropped (#482). Label template is trusted. + const html = `
${labels.formatReplyLine({ date: escapeHtml(date), from: escapeHtml(fromStrFull) })}
`; return { html, text, wrapInBlockquote: true }; }