From 5570d0e7495a42a90acf951d396c9185b0319eb9 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 27 Aug 2026 13:05:17 +0200 Subject: feat: forward an HTML message with its formatting Item 171. A forward carried only the plain-text version of the original, so formatting was lost; and an original with no plain-text part at all (30 of 342 sampled inbox messages, ~9%) forwarded as an empty quote with its content silently gone. A forward now sends ONE part chosen by the Send-as-HTML toggle: the original's markup when on, the text quote when off. Not a multipart/alternative, at the user's decision: a forward's shape is already decided by that toggle, and sending both hands the choice to the recipient's client. The toggle is honoured even for an HTML-only original, which then forwards as a text fallback. HtmlSanitiser strips remote content from the forwarded markup, checked by default with a per-forward opt-out. This is the security-critical part: the markup leaves this process and is rendered by the recipient's client, where none of MessageView's protections apply, so forwarding a tracking pixel forwards the tracking. It is an ALLOW-LIST, unlike HtmlBuilder::namespaceCids(), because a missed rewrite is a broken image while a missed strip is a beacon reaching the recipient. An HTML forward does not seed a text quote into the editor. The first build did, then subtracted it when building the HTML part, so the user could edit a quote whose edits were discarded; what the composer shows must be what gets sent. The forwarded message appears in a read-only pane beside the editor instead, a QSplitter at 60/40 with a toggle in the Format menu. A plain forward is unchanged. ComposeContextBuilder::quoteBody() renders htmlBody down to text when there is no plain part, so the plain path never emits an empty quote. Design in docs/superpowers/specs/2026-08-27-forward-html-design.md. Two tests repaired for the splitter: the 60/40 assertion reads stretch factors rather than pixels, since the offscreen platform gives the splitter no width and reports 49/49 whatever the code asks; and theComposerSplitsItsToolbarByScope looked for the body directly in the composer's column. Not yet hand-tested in this arrangement. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AtUzfNjMD8fiYfamDd3ywW --- CHANGELOG.md | 21 ++ .../2026-08-03-post-0.1.0-usability-closed.md | 153 ++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 114 ++++--- .../specs/2026-08-27-forward-html-design.md | 181 +++++++++++ src/CMakeLists.txt | 1 + src/composecontext.cpp | 21 +- src/composewindow.cpp | 178 ++++++++++- src/composewindow.h | 31 ++ src/htmlsanitiser.cpp | 336 +++++++++++++++++++++ src/htmlsanitiser.h | 84 ++++++ src/messagebuilder.cpp | 35 ++- src/types.h | 15 + tests/CMakeLists.txt | 1 + tests/test_composecontext.cpp | 53 ++++ tests/test_composewindow.cpp | 199 ++++++++++++ tests/test_htmlsanitiser.cpp | 249 +++++++++++++++ tests/test_mainwindow.cpp | 26 +- tests/test_messagebuilder.cpp | 100 ++++++ translations/qtmaildir_it_IT.ts | 26 +- 19 files changed, 1749 insertions(+), 75 deletions(-) create mode 100644 docs/superpowers/specs/2026-08-27-forward-html-design.md create mode 100644 src/htmlsanitiser.cpp create mode 100644 src/htmlsanitiser.h create mode 100644 tests/test_htmlsanitiser.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 69b09b9..062fa8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,27 @@ point at which they are stable. ### Fixed +- **Forwarding an HTML message kept its formatting.** A forward carried only + the plain-text version of the original, so tables, emphasis and layout were + flattened, and a message with no plain-text part at all (about one in eleven + of the mail measured here) forwarded as an empty quote with its content + silently gone. A forward now carries the original's own HTML when **Send as + HTML** is on, and the text version when it is off: one or the other, chosen + by that toggle, rather than both. + + On a forward that carries HTML, the message being forwarded is shown in its + own pane beside what you are writing, at a 60/40 split, with a toggle under + **Format** to close it. The editor holds your own note only, so everything + you can edit is something that gets sent. A plain-text forward is unchanged + and still quotes into the editor as before. + + **Remote content is stripped by default.** Images and styles loaded from the + internet are removed before the forward is sent, so the sender of the + original cannot learn that you forwarded it or that your recipient opened + it. A checkbox on the forward lets you keep them for a sender you trust; it + appears only when there is something to strip. Inline images that travel + inside the message itself are unaffected and still display. + - **A draft you wrote was marked unread.** Drafts were written to disk without the Maildir "seen" flag, and notmuch tags anything without it `unread`, so a draft you had just typed appeared in the Unread view. It corrected itself diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index 4556b48..77d4726 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -8935,3 +8935,156 @@ That is fixed. The reported tag set `draft inbox unread` is fully explained: `unread` from the missing `S` flag on the draft, `inbox` from the arrived message sharing its thread. +## 171. A forwarded HTML message reaches the recipient as plain text + +**Observed (user, from the notes):** "forwarding an html message doesn't +maintain the html formatting of the original message. #bug" + +**Cause (verified in the code, 2026-08-27).** The forward path builds its body +through `ComposeContextBuilder::quoteBody()` (`src/composecontext.cpp`), which +reads `message.plainBody` and nothing else. `MimeParser` parses both halves and +`ParsedMessage` carries `htmlBody` beside `plainBody` (`src/mimeparser.h:150`), +so the HTML is available and simply never asked for. + +Two consequences follow, and they are not the same severity: + +- An original with both parts forwards its text/plain alternative, losing the + sender's formatting. Recoverable-looking, since the words survive. +- An original with an HTML part ONLY has an empty `plainBody`, so the forward + carries the attribution line and an empty quote. The message's content is + gone, and nothing says so. + +`MainWindow::composeReply()` already treats the two kinds differently for the +composer's own HTML state: `context.seedHtml` is the CONFIG's `sendHtml` for a +forward and `original.hasHtml()` for a reply, on the stated reasoning that an +HTML part is a fact about the sender's software. That reasoning is sound for +how the user WRITES and does not decide what the forward CARRIES, which is the +question here. + +**Approach.** The decision comes first; this is not a changed call site. + +A forward is a different act from a reply: the point is to hand somebody else +what arrived, and quoting is the wrong shape for it. Three candidates, in +increasing fidelity: + +- Render `htmlBody` down to text when `plainBody` is empty, so nothing is + silently lost. The smallest fix, and it does not answer the note: formatting + is still gone. +- Carry the original as a `message/rfc822` part, which is what item 130 already + describes and what GMime builds natively. Perfect fidelity, and every + attachment comes with it, but the recipient sees an attached message rather + than a body. +- Build the forward as `multipart/alternative` with the original's HTML nested + in the HTML half, which is what Thunderbird's inline forward does. + +**Constraints.** + +- **The HTML is input from a stranger and the composer is not the message + pane.** The pane's protections (off-the-record profile, JavaScript off, the + interceptor blocking every request) are `MessageView`'s, not + `ComposeWindow`'s. Any route that puts the original's markup into an outgoing + message must decide what it strips, and remote references in particular: + forwarding a tracking pixel forwards the tracking to the new recipient. +- Item 130 overlaps and may subsume this. Decide the two together rather than + building `message/rfc822` twice. +- The markdown body is the composer's source of truth, and markdown has no + syntax for arbitrary HTML the user can then edit. A route that keeps the + original's markup has to keep it OUTSIDE the editable buffer, which is the + same nesting problem item 129 carries. +- `quoteBody()` is shared with Reply. A change there reaches both; the + behaviour asked for is the forward's alone. + +--- + +**BUILT 2026-08-27.** Design in +`docs/superpowers/specs/2026-08-27-forward-html-design.md`, which is the +document to read; this entry records only what changed and what was learned. + +The user chose **carrying the original's markup inline** over attaching the +original as `message/rfc822` (item 130's mechanism, still open for its own +sake) and over a text-only fallback, and chose **strip remote content by +default with a per-forward opt-out** over always stripping and over keeping +everything. + +**Amended the same day, after the first build**: a forward sends ONE part +rather than a `multipart/alternative`, chosen by the Send-as-HTML toggle. The +first build sent both halves and also FORCED html on when there was markup to +carry; both were reversed. A forward's shape is something the user has already +decided by flipping that toggle, and sending both hands the choice to the +recipient's client. The consequence was put to the user explicitly and +accepted: with the toggle off, an HTML-only original forwards as the text +fallback and its formatting is lost. + +Four parts, each independently useful: + +1. **`HtmlSanitiser`** (`src/htmlsanitiser.h/.cpp`), a namespace of free + functions so the security property is testable without a widget. +2. **`quoteBody()`'s empty-plain fallback**, via + `QTextDocumentFragment::fromHtml().toPlainText()`. Closes the silent half + on its own. +3. **The MIME nesting** in `MessageBuilder`, asserted by parsing the result + back through `MimeParser` rather than by reading the RFC. +4. **The composer control**, created only when the original actually carries + remote content. + +**The allow-list rule is the part to preserve.** `HtmlBuilder::namespaceCids()` +is a block-list and documents scoping `srcset=` out; that trade is right for +rewriting and wrong for stripping, because a missed rewrite is a broken image +and a missed strip is a beacon reaching the recipient. `HtmlSanitiser` judges +every attribute by its VALUE, so `srcset`, `poster`, `data-*` and whatever HTML +adds next are handled by the default, which is removal. + +**A real bug was caught by the tests and is worth recording**: the walk used +`QRegularExpression::globalMatch()` while also advancing `pos` past a removed +element's content. `globalMatch` iterates over matches found against the +ORIGINAL string, so it handed back tags from inside the region just skipped; +the output duplicated content and an `" + "" + "")); + + assertNoRemoteUrls(out); + QVERIFY2(!out.toLower().contains(QStringLiteral("keep

")), + qPrintable(QStringLiteral("the body was lost: ") + out)); +} + +/// CSS fetches too, and it reaches the same network from two different places +/// with different terminator rules. namespaceCids() handles both for the same +/// reason. +void TestHtmlSanitiser::cssUrlIsStrippedInBothPlaces() +{ + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent(QStringLiteral( + "
x
"))); + + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent(QStringLiteral( + ""))); + + // The bare form terminates on ')', not on a quote. + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent(QStringLiteral( + "
x
"))); +} + +/// The recipient's client most likely disables scripting. That is their +/// policy, not ours to assume on their behalf. +void TestHtmlSanitiser::anEventHandlerIsRemoved() +{ + const QString out = HtmlSanitiser::stripRemoteContent(QStringLiteral( + "")); + + assertNoRemoteUrls(out); + QVERIFY2(!out.toLower().contains(QStringLiteral("onerror")), + qPrintable(QStringLiteral("an event handler survived: ") + out)); +} + +/// A data: URL carries its payload inline, so it does not fetch, but it CAN +/// carry markup and is a standard sanitiser bypass. Removed on the allow-list +/// rule: it is not cid:, so it goes. +void TestHtmlSanitiser::aDataUrlIsRemoved() +{ + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent(QStringLiteral( + ""))); +} + +/// **The allow-list's whole point.** `namespaceCids()` enumerates the +/// attributes it rewrites and scopes srcset out; doing that here would leak. +/// An attribute nobody anticipated must be handled by the DEFAULT. +void TestHtmlSanitiser::anUnknownAttributeCarryingAUrlIsRemoved() +{ + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent(QStringLiteral( + ""))); + + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent(QStringLiteral( + "
x
"))); + + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent(QStringLiteral( + ""))); +} + +/// A cid: id may legitimately contain something URL-shaped. Stripping on a +/// substring match rather than on the SCHEME would destroy a valid reference. +void TestHtmlSanitiser::aCidWhoseIdLooksLikeAUrlSurvives() +{ + const QString out = HtmlSanitiser::stripRemoteContent( + QStringLiteral("")); + + QVERIFY2(out.contains(QStringLiteral("cid:https-logo@example.org")), + qPrintable(QStringLiteral("a valid cid was destroyed: ") + out)); +} + +/// The formatting is the entire point of the feature. A sanitiser that keeps +/// the user safe by emptying the message has not solved item 171. +void TestHtmlSanitiser::structuralMarkupSurvives() +{ + const QString out = HtmlSanitiser::stripRemoteContent(QStringLiteral( + "" + "
Revenueup 12%
  • Region A
")); + + QVERIFY2(out.contains(QStringLiteral("Region A")), + qPrintable(QStringLiteral("the list was lost: ") + out)); +} + +/// Drives whether the composer offers the checkbox at all. It must never +/// decide whether to strip. +void TestHtmlSanitiser::hasRemoteContentAnswersForTheComposer() +{ + QVERIFY(HtmlSanitiser::hasRemoteContent( + QStringLiteral(""))); + QVERIFY(HtmlSanitiser::hasRemoteContent( + QStringLiteral("
x
"))); + + QVERIFY(!HtmlSanitiser::hasRemoteContent( + QStringLiteral("

plain

"))); + QVERIFY(!HtmlSanitiser::hasRemoteContent(QStringLiteral("

plain

"))); +} + +QTEST_MAIN(TestHtmlSanitiser) +#include "test_htmlsanitiser.moc" diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 2fbdb20..08589b2 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -12955,15 +12955,23 @@ void TestMainWindow::theComposerSplitsItsToolbarByScope() auto *column = qobject_cast(central->layout()); QVERIFY2(column, "the composer is not laid out in a vertical column"); - int barIndex = -1; - int bodyIndex = -1; - for (int i = 0; i < column->count(); ++i) { - QLayoutItem *item = column->itemAt(i); - if (item->widget() == editorBar) - barIndex = i; - else if (item->widget() == body) - bodyIndex = i; - } + // The editor sits inside a QSplitter since item 171, so its position in + // the column is the SPLITTER's: a forward puts the forwarded message + // beside the editor, and the toolbar must stay above both. Walking up to + // whichever child of the column contains the body keeps this test about + // the toolbar's position rather than about the editor's parentage. + const auto columnChildOf = [column](QWidget *widget) { + for (QWidget *w = widget; w; w = w->parentWidget()) { + for (int i = 0; i < column->count(); ++i) { + if (column->itemAt(i)->widget() == w) + return i; + } + } + return -1; + }; + + const int barIndex = columnChildOf(editorBar); + const int bodyIndex = columnChildOf(body); QVERIFY2(barIndex >= 0 && bodyIndex >= 0, "the editor bar or the body is not in the composer's column"); QVERIFY2(barIndex < bodyIndex, "the editor bar is not above the editor"); diff --git a/tests/test_messagebuilder.cpp b/tests/test_messagebuilder.cpp index 73d388c..2ea14f0 100644 --- a/tests/test_messagebuilder.cpp +++ b/tests/test_messagebuilder.cpp @@ -29,6 +29,7 @@ #include "config.h" #include "messagebuilder.h" +#include "mimeparser.h" #include "types.h" /// MessageBuilder's tests assert on the GENERATED BYTES, never by round-tripping @@ -55,6 +56,7 @@ private slots: void aDirectoryAttachmentFailsRatherThanHangingTheProcess(); void anUnparseableRecipientFailsRatherThanVanishing(); void everyMessageCarriesADateAndMessageId(); + void aForwardSendsOnePartChosenByTheHtmlToggle(); void recipientsAppearInTheirOwnHeaders(); void anAccountWithNoAddressFailsRatherThanBuildingHeaderlessMail(); @@ -462,5 +464,103 @@ void TestMessageBuilder::anAccountWithNoAddressFailsRatherThanBuildingHeaderless QVERIFY(r.bytes.isEmpty()); } +/// Item 171. A forward sends ONE part, chosen by the Send-as-HTML toggle: +/// the original's markup when it is on, the text quote when it is off. +/// +/// **No multipart/alternative on a forward**, at the user's decision +/// 2026-08-27, reversing the first build. A forward is a message the user has +/// already decided the shape of by flipping that toggle, and sending both +/// halves means the recipient's client picks, which is the choice being taken +/// away from them. +/// +/// The toggle is honoured even when the original has no plain-text part: with +/// it off, an HTML-only original forwards as the text fallback and the +/// formatting is lost. That is the toggle meaning what it says, chosen over +/// forcing HTML for those messages. +/// +/// `forwardedHtml` arrives ALREADY SANITISED: whether to strip remote content +/// is the user's per-forward choice and a builder cannot see a checkbox. The +/// security property is asserted in test_htmlsanitiser; what matters here is +/// that the right single part goes out. +void TestMessageBuilder::aForwardSendsOnePartChosenByTheHtmlToggle() +{ + OutgoingMessage m = baseMessage(); + m.sendHtml = true; + // As the composer really supplies it: on an HTML forward the buffer holds + // the user's own note ALONE, the original travelling as markup instead, so + // that what the composer shows is what gets sent (item 171). + m.markdownBody = QStringLiteral("Passing this on."); + m.forwardedHtml = QStringLiteral("

Revenue rose 12%.

"); + + const MessageBuilder::Result r = MessageBuilder::build(m, m_account); + QVERIFY2(r.ok(), qPrintable(r.error)); + + const QString text = QString::fromUtf8(r.bytes); + + // ONE part, not an alternative. + QVERIFY2(!text.contains(QStringLiteral("multipart/alternative")), + qPrintable(QStringLiteral("a forward must not send both halves:\n%1") + .arg(text))); + QVERIFY2(text.contains(QStringLiteral("text/html")), + qPrintable(QStringLiteral("no html part:\n%1").arg(text))); + QVERIFY2(!text.contains(QStringLiteral("text/plain")), + qPrintable(QStringLiteral("a plain part went out too:\n%1").arg(text))); + + QVERIFY2(text.contains(QStringLiteral("Revenue rose")), + qPrintable(QStringLiteral("the forwarded body is missing:\n%1").arg(text))); + QVERIFY2(text.contains(QStringLiteral("Passing this on")), + qPrintable(QStringLiteral("the user's own text was lost:\n%1").arg(text))); + + // **The original must appear ONCE.** The composer seeds the text quote + // into the editable body so the user can trim it, so `markdownBody` + // already carries a flattened copy of the original; rendering that AND + // appending the markup shipped the whole message twice, the first copy + // with its URLs naked and mangled. Found by hand-testing on 2026-08-27 + // against a real newsletter, where it read as two messages stacked. + QVERIFY2(!text.contains(QStringLiteral("
")), + qPrintable(QStringLiteral("the text quote was rendered into the " + "html as well as the markup:\n%1").arg(text))); + QCOMPARE(text.count(QStringLiteral("Revenue rose")), 1); + + // **The structure is right**, checked by parsing back rather than by + // reading the RFC: MimeParser is what the application itself uses. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString path = dir.path() + QStringLiteral("/forward.eml"); + QFile out(path); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(r.bytes); + out.close(); + + MimeParser parser; + const ParsedMessage parsed = parser.parse(path); + QVERIFY2(parsed.ok, "the built forward does not parse back"); + QVERIFY2(parsed.htmlBody.contains(QStringLiteral("Revenue rose")), + qPrintable(QStringLiteral("the forwarded markup is not in the html " + "part on the way back:\n%1").arg(parsed.htmlBody))); + + // Toggle OFF: the plain quote alone, and the markup must not leak into it. + OutgoingMessage plainForward = baseMessage(); + plainForward.sendHtml = false; + plainForward.markdownBody = QStringLiteral("Passing this on.\n\n> Revenue rose 12%."); + plainForward.forwardedHtml = QStringLiteral("

Revenue rose 12%.

"); + + const MessageBuilder::Result r2 = MessageBuilder::build(plainForward, m_account); + QVERIFY2(r2.ok(), qPrintable(r2.error)); + const QString text2 = QString::fromUtf8(r2.bytes); + + QVERIFY2(!text2.contains(QStringLiteral("multipart/alternative")), + qPrintable(QStringLiteral("a plain forward must be one part:\n%1") + .arg(text2))); + QVERIFY2(!text2.contains(QStringLiteral("text/html")), + qPrintable(QStringLiteral("html went out with the toggle off:\n%1") + .arg(text2))); + QVERIFY2(!text2.contains(QStringLiteral("")), + qPrintable(QStringLiteral("markup leaked into a plain forward:\n%1") + .arg(text2))); + QVERIFY2(text2.contains(QStringLiteral("Passing this on")), + qPrintable(QStringLiteral("the user's own text was lost:\n%1").arg(text2))); +} + QTEST_MAIN(TestMessageBuilder) #include "test_messagebuilder.moc" diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 85b978f..040af5c 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -7,6 +7,26 @@ Compose[*] Componi[*] + + Forwarded message, sent as it arrived: + Messaggio inoltrato, inviato come è arrivato: + + + Forwarded message + Messaggio inoltrato + + + Shows the message being forwarded beside what you are writing. + Mostra il messaggio che stai inoltrando accanto a quello che stai scrivendo. + + + Strip remote content from the forwarded message + Rimuovi i contenuti remoti dal messaggio inoltrato + + + Images and styles loaded from the internet are removed, so the sender of the original cannot tell that you forwarded it. Uncheck only for a sender you trust. + Le immagini e gli stili caricati da internet vengono rimossi, così il mittente dell'originale non può sapere che lo hai inoltrato. Togli la spunta solo per un mittente di cui ti fidi. + The forwarded attachments could not be extracted. Non è stato possibile estrarre gli allegati inoltrati. @@ -1629,11 +1649,11 @@ Il messaggio È stato inviato. Non inviarlo di nuovo. Changes made here that a sync has not yet carried to the mail store. This list is a snapshot taken when it was opened. - Modifiche fatte qui che una sincronizzazione non ha ancora portato all'archivio di posta. Questo elenco è un'istantanea presa al momento dell'apertura. + Modifiche fatte qui che una sincronizzazione non ha ancora portato all'archivio di posta. Questo elenco è un'istantanea presa al momento dell'apertura. (no longer in the index) - (non più nell'indice) + (non più nell'indice) %1 (whole thread, %n message(s)) @@ -1644,7 +1664,7 @@ Il messaggio È stato inviato. Non inviarlo di nuovo. Nothing is waiting to be synced. - Non c'è nulla in attesa di sincronizzazione. + Non c'è nulla in attesa di sincronizzazione. -- cgit v1.2.3