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 --- tests/test_messagebuilder.cpp | 100 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) (limited to 'tests/test_messagebuilder.cpp') 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" -- cgit v1.2.3