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/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 +++++++++++++++++ 6 files changed, 619 insertions(+), 9 deletions(-) create mode 100644 tests/test_htmlsanitiser.cpp (limited to 'tests') diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2cb3651..1646028 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -49,6 +49,7 @@ target_compile_definitions(test_mimeparser PRIVATE FIXTURE_DIR="${CMAKE_CURRENT_SOURCE_DIR}/fixtures") add_qtmaildir_test(interceptor) add_qtmaildir_test(htmlbuilder) +add_qtmaildir_test(htmlsanitiser) add_qtmaildir_test(notmuchworker) add_qtmaildir_test(tagcolors) add_qtmaildir_test(cardlayout) diff --git a/tests/test_composecontext.cpp b/tests/test_composecontext.cpp index bea390d..50b7ab3 100644 --- a/tests/test_composecontext.cpp +++ b/tests/test_composecontext.cpp @@ -96,6 +96,7 @@ private slots: // Quoting. void aQuotedBodyPrefixesEveryLine(); + void anHtmlOnlyBodyIsQuotedAsText(); private: QString writeConfig(const QString &contents); @@ -1124,5 +1125,57 @@ void TestComposeContext::aQuotedBodyPrefixesEveryLine() .arg(quotedCrlf))); } +/// Item 171's silent half. An HTML-only original has an EMPTY `plainBody`, so +/// quoting it produced an attribution line and nothing else: the content was +/// gone and nothing said so. Measured on the developer's own inbox 2026-08-27, +/// 30 of 342 sampled messages (~9%) declare text/html with no text/plain, so +/// this is not an edge case. +/// +/// The fallback renders the HTML down to text. It does NOT preserve +/// formatting, which is the separate half of item 171 and is answered by the +/// multipart/alternative build; this only guarantees the words survive. +void TestComposeContext::anHtmlOnlyBodyIsQuotedAsText() +{ + ParsedMessage message; + message.from = QStringLiteral("Sender "); + message.date = QStringLiteral("Thu, 20 Aug 2026 10:00:00 +0200"); + message.htmlBody = QStringLiteral( + "

Revenue rose 12% against forecast.

  • Region A
"); + // plainBody deliberately empty: this is the shape that lost the content. + + const QString quoted = ComposeContextBuilder::quoteBody(message); + + QVERIFY2(quoted.contains(QStringLiteral("Revenue rose")), + qPrintable(QStringLiteral("the body was lost:\n%1").arg(quoted))); + QVERIFY2(quoted.contains(QStringLiteral("Region A")), + qPrintable(QStringLiteral("list content was lost:\n%1").arg(quoted))); + QVERIFY2(quoted.contains(QStringLiteral("12%")), + qPrintable(QStringLiteral("emphasised text was lost:\n%1").arg(quoted))); + + // Quoted like any other body, not dumped raw. + QVERIFY2(quoted.contains(QStringLiteral("> Revenue rose")), + qPrintable(QStringLiteral("the fallback is not quoted:\n%1").arg(quoted))); + + // Text, not markup: the plain half of a message must not carry tags. + QVERIFY2(!quoted.contains(QStringLiteral("")), + qPrintable(QStringLiteral("markup reached the plain quote:\n%1").arg(quoted))); + QVERIFY2(!quoted.contains(QStringLiteral("

")), + qPrintable(QStringLiteral("markup reached the plain quote:\n%1").arg(quoted))); + + // A message WITH a plain part must keep using it, untouched: the fallback + // is for the empty case only, and rendering HTML over a real plain part + // would change every ordinary reply. + ParsedMessage both; + both.plainBody = QStringLiteral("the real plain part"); + both.htmlBody = QStringLiteral("

the html part

"); + const QString preferred = ComposeContextBuilder::quoteBody(both); + QVERIFY2(preferred.contains(QStringLiteral("> the real plain part")), + qPrintable(QStringLiteral("the plain part was not preferred:\n%1") + .arg(preferred))); + QVERIFY2(!preferred.contains(QStringLiteral("the html part")), + qPrintable(QStringLiteral("the html part was used anyway:\n%1") + .arg(preferred))); +} + QTEST_MAIN(TestComposeContext) #include "test_composecontext.moc" diff --git a/tests/test_composewindow.cpp b/tests/test_composewindow.cpp index 0da61ff..d95d55d 100644 --- a/tests/test_composewindow.cpp +++ b/tests/test_composewindow.cpp @@ -18,12 +18,14 @@ #include #include +#include #include #include #include #include #include #include +#include #include #include #include @@ -64,6 +66,8 @@ private slots: void theMenuBarReachesEveryComposerAction(); void saveDraftWritesAndReports(); void aSavedDraftIsFlaggedSeen(); + void aForwardCarriesTheOriginalHtmlAndStripsRemoteContent(); + void anHtmlForwardPreviewsTheOriginalInsteadOfQuotingIt(); void theMenusReuseTheToolbarActions(); void theHtmlMenuItemTracksTheToolbarButton(); void theAgeLineFollowsTheClock(); @@ -800,6 +804,201 @@ void TestComposeWindow::aSavedDraftIsFlaggedSeen() "tags it unread, got %1").arg(flags))); } +/// Item 171, the composer half. A forward of an HTML message carries the +/// original's markup, with remote content stripped BY DEFAULT and a control to +/// keep it. +/// +/// The default is the security-relevant half: forwarding a tracking pixel +/// forwards the tracking, and the original sender learns the recipient opened +/// it. The user chose "ask per forward, default to strip" over always +/// stripping and over keeping everything. +void TestComposeWindow::aForwardCarriesTheOriginalHtmlAndStripsRemoteContent() +{ + const Config config = configWithDrafts(); + + // A real file on disk: the composer reads originalPath itself, exactly as + // extractForwardedAttachments() does, so a fixture built in memory would + // not exercise the path that runs. + const QString path = m_dir->path() + QStringLiteral("/original.eml"); + writeFile(path, QStringLiteral( + "From: Sender \r\n" + "To: someone@example.org\r\n" + "Subject: Quarterly report\r\n" + "Date: Wed, 26 Aug 2026 10:00:00 +0200\r\n" + "MIME-Version: 1.0\r\n" + "Content-Type: text/html; charset=utf-8\r\n" + "\r\n" + "

Revenue rose 12%.

" + "\r\n")); + + ComposeContext context; + context.kind = ComposeContext::Kind::Forward; + context.accountKey = QStringLiteral("work"); + context.originalPath = path; + context.subject = QStringLiteral("Fwd: Quarterly report"); + + ComposeWindow window(context, config, m_dir->path()); + + auto *strip = window.findChild(QStringLiteral("stripRemote")); + QVERIFY2(strip, "there is no strip-remote-content control on a forward"); + QVERIFY2(strip->isChecked(), + "stripping must be the DEFAULT: a forward must not leak a " + "tracking pixel to the recipient unless the user asks for it"); + + // Checked: the markup survives, the beacon does not. + const OutgoingMessage stripped = window.currentMessage(); + QVERIFY2(stripped.forwardedHtml.contains(QStringLiteral("Revenue rose")), + qPrintable(QStringLiteral("the original's markup was lost:\n%1") + .arg(stripped.forwardedHtml))); + QVERIFY2(stripped.forwardedHtml.contains(QStringLiteral("")), + "the formatting was flattened, which is the defect being fixed"); + QVERIFY2(!stripped.forwardedHtml.contains(QStringLiteral("tracker.example")), + qPrintable(QStringLiteral("a tracking pixel survived:\n%1") + .arg(stripped.forwardedHtml))); + + // Unchecked: the user's explicit choice is honoured. + strip->setChecked(false); + const OutgoingMessage kept = window.currentMessage(); + QVERIFY2(kept.forwardedHtml.contains(QStringLiteral("tracker.example")), + "unchecking the control must actually keep the remote content"); + + // A New message has neither the control nor any forwarded markup. + ComposeContext fresh; + fresh.kind = ComposeContext::Kind::New; + fresh.accountKey = QStringLiteral("work"); + ComposeWindow plain(fresh, config, m_dir->path()); + QVERIFY2(plain.currentMessage().forwardedHtml.isEmpty(), + "a new message must carry no forwarded markup"); +} + +/// Item 171, the WYSIWYG half. **What the composer shows must be what gets +/// sent**, and for an HTML forward the editable buffer cannot be that. +/// +/// The first build seeded the text quote into the buffer and then dropped it +/// when building the HTML part, so the user could edit a quote whose edits +/// were silently discarded. That is worse than the defect it replaced: the +/// previous version at least sent what it displayed. +/// +/// So on an HTML forward the buffer holds the user's own note ONLY, and the +/// original appears in a read-only preview instead. Nothing shown is +/// editable-but-ignored, and nothing sent is unshown. The user chose this over +/// a rich-text composer (recorded as item 173, which is the real WYSIWYG +/// answer and a much larger piece of work) and over attaching the original. +void TestComposeWindow::anHtmlForwardPreviewsTheOriginalInsteadOfQuotingIt() +{ + const Config config = configWithDrafts(); + + const QString path = m_dir->path() + QStringLiteral("/original.eml"); + writeFile(path, QStringLiteral( + "From: Sender \r\n" + "To: someone@example.org\r\n" + "Subject: Quarterly report\r\n" + "Date: Wed, 26 Aug 2026 10:00:00 +0200\r\n" + "MIME-Version: 1.0\r\n" + "Content-Type: text/html; charset=utf-8\r\n" + "\r\n" + "

Revenue rose 12%.

\r\n")); + + ComposeContext context; + context.kind = ComposeContext::Kind::Forward; + context.accountKey = QStringLiteral("work"); + context.originalPath = path; + context.subject = QStringLiteral("Fwd: Quarterly report"); + context.quotedBody = QStringLiteral( + "On Wed, sender@example.org wrote:\n\n> Revenue rose 12%."); + + ComposeWindow window(context, config, m_dir->path()); + + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + + // The buffer carries the user's note only: no quote to edit in vain. + QVERIFY2(!body->toPlainText().contains(QStringLiteral("Revenue rose")), + qPrintable(QStringLiteral("the original was seeded into the " + "editable buffer:\n%1").arg(body->toPlainText()))); + + // The preview says what will be carried, and is NOT editable. + auto *preview = window.findChild(QStringLiteral("forwardPreview")); + QVERIFY2(preview, "an HTML forward must show what it will carry"); + QVERIFY2(preview->isVisibleTo(&window), + "the preview must not be hidden on an HTML forward"); + + // **Beside the editor, not under it**, at the user's request 2026-08-27: + // a vertical split, editor 60 and preview 40, so the note being written + // and the message being forwarded are read side by side. + auto *split = window.findChild(QStringLiteral("composeSplit")); + QVERIFY2(split, "the preview must share a splitter with the editor"); + QCOMPARE(split->orientation(), Qt::Horizontal); + QCOMPARE(split->count(), 2); + QCOMPARE(split->widget(0), static_cast(body)); + QCOMPARE(split->widget(1), preview); + + // **The ratio is asserted as STRETCH FACTORS, not as resulting pixels.** + // CLAUDE.md records that the offscreen platform cannot test window sizing: + // it prints "This plugin does not support propagateSizeHints()" and the + // splitter here has no real width to divide, so sizes() reports an equal + // 49/49 whatever the code asks for. Measured: a pixel assertion fails + // against correct code. The stretch factors are what the layout stores and + // what survives the first real resize, so they are the testable intent; + // the appearance is a hand test. + // QSplitter has no stretchFactor() getter: setStretchFactor() writes the + // value into the CHILD's size policy, which is where it can be read back. + QCOMPARE(body->sizePolicy().horizontalStretch(), 6); + QCOMPARE(preview->sizePolicy().horizontalStretch(), 4); + + // A toggle closes and reopens it. + auto *toggle = window.findChild(QStringLiteral("compose_show_forward")); + QVERIFY2(toggle, "there is no toggle for the forwarded-message pane"); + QVERIFY2(toggle->isCheckable(), "the pane toggle must be checkable"); + QVERIFY2(toggle->isChecked(), "the pane starts open on an HTML forward"); + + toggle->trigger(); + QVERIFY2(!preview->isVisibleTo(&window), + "unchecking the toggle must hide the forwarded-message pane"); + toggle->trigger(); + QVERIFY2(preview->isVisibleTo(&window), + "re-checking the toggle must bring the pane back"); + + // What is sent still contains the original, from the markup rather than + // from the buffer. + const OutgoingMessage message = window.currentMessage(); + QVERIFY2(message.forwardedHtml.contains(QStringLiteral("Revenue rose")), + "the forward must still carry the original"); + + // A PLAIN forward is unchanged: the quote goes in the buffer, where it is + // both editable and sent, so WYSIWYG already held there and must not be + // broken by this. + const QString plainPath = m_dir->path() + QStringLiteral("/plain.eml"); + writeFile(plainPath, QStringLiteral( + "From: Sender \r\n" + "Subject: Plain report\r\n" + "Date: Wed, 26 Aug 2026 10:00:00 +0200\r\n" + "\r\n" + "Revenue rose 12%.\r\n")); + + ComposeContext plainContext; + plainContext.kind = ComposeContext::Kind::Forward; + plainContext.accountKey = QStringLiteral("work"); + plainContext.originalPath = plainPath; + plainContext.quotedBody = QStringLiteral("> Revenue rose 12%."); + + ComposeWindow plainWindow(plainContext, config, m_dir->path()); + auto *plainBody = plainWindow.findChild(QStringLiteral("body")); + QVERIFY(plainBody); + QVERIFY2(plainBody->toPlainText().contains(QStringLiteral("Revenue rose")), + qPrintable(QStringLiteral("a plain forward lost its quote:\n%1") + .arg(plainBody->toPlainText()))); + + auto *noPreview = plainWindow.findChild(QStringLiteral("forwardPreview")); + QVERIFY2(!noPreview || !noPreview->isVisibleTo(&plainWindow), + "a plain forward needs no preview: its quote is in the buffer"); + + auto *noToggle = plainWindow.findChild( + QStringLiteral("compose_show_forward")); + QVERIFY2(!noToggle || !noToggle->isVisible(), + "a plain forward must not offer a pane toggle that does nothing"); +} + /// The same QAction objects, shown twice over, exactly as item 140 required /// for the message pane's bar. A copy would drift: an enablement change or a /// new shortcut would reach one surface and not the other. diff --git a/tests/test_htmlsanitiser.cpp b/tests/test_htmlsanitiser.cpp new file mode 100644 index 0000000..043891b --- /dev/null +++ b/tests/test_htmlsanitiser.cpp @@ -0,0 +1,249 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include +#include + +#include "htmlsanitiser.h" + +/// Item 171's security half. +/// +/// Every test here asserts on the STRING, never on a render. A rendering probe +/// cannot see this defect by construction: a tracking pixel is a 1x1 +/// transparent image, invisible by design, so a probe that renders the output +/// and looks at it would endorse the exact thing being guarded against. +class TestHtmlSanitiser : public QObject +{ + Q_OBJECT + +private slots: + void aCidReferenceSurvives(); + void aRemoteImageIsRemoved(); + void anUnquotedRemoteUrlIsRemoved(); + void aProtocolRelativeUrlIsRemoved(); + void quotingAndCaseAndWhitespaceDoNotHelp(); + void aFetchingElementIsRemovedWhole(); + void cssUrlIsStrippedInBothPlaces(); + void anEventHandlerIsRemoved(); + void aDataUrlIsRemoved(); + void anUnknownAttributeCarryingAUrlIsRemoved(); + void aCidWhoseIdLooksLikeAUrlSurvives(); + void structuralMarkupSurvives(); + void hasRemoteContentAnswersForTheComposer(); + +private: + /// The invariant, applied to a whole output: no scheme but cid: anywhere. + /// + /// Deliberately crude and deliberately independent of the implementation's + /// own patterns. A test that reused the production regexes would agree + /// with a bug rather than catch it. + void assertNoRemoteUrls(const QString &out); +}; + +void TestHtmlSanitiser::assertNoRemoteUrls(const QString &out) +{ + const QString lowered = out.toLower(); + for (const char *needle : { "http:", "https:", "//evil", "//host", + "data:", "file:", "ftp:" }) { + QVERIFY2(!lowered.contains(QLatin1String(needle)), + qPrintable(QStringLiteral("a %1 reference survived: %2") + .arg(QLatin1String(needle), out))); + } +} + +/// The one thing that must NOT be stripped. A cid: travels inside the message +/// and fetches nothing, so an inline logo survives a forward. +void TestHtmlSanitiser::aCidReferenceSurvives() +{ + const QString out = HtmlSanitiser::stripRemoteContent( + QStringLiteral("

hi

")); + + QVERIFY2(out.contains(QStringLiteral("cid:logo@example.org")), + qPrintable(QStringLiteral("the cid was lost: ") + out)); + QVERIFY2(out.contains(QStringLiteral("

hi

")), + qPrintable(QStringLiteral("the body was lost: ") + out)); +} + +/// The reported harm, in its plainest form. +void TestHtmlSanitiser::aRemoteImageIsRemoved() +{ + const QString out = HtmlSanitiser::stripRemoteContent( + QStringLiteral("

hi

")); + + assertNoRemoteUrls(out); + QVERIFY2(out.contains(QStringLiteral("

hi

")), + qPrintable(QStringLiteral("the body was lost: ") + out)); +} + +/// `` is valid HTML and unquoted references are seen in the +/// wild; namespaceCids() documents the same. An implementation that only +/// handles quoted values passes every tidy test and leaks on real mail. +void TestHtmlSanitiser::anUnquotedRemoteUrlIsRemoved() +{ + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent( + QStringLiteral(""))); +} + +/// No scheme at all, and it still fetches: the recipient's client supplies +/// whichever scheme it rendered the message under. A check for "http" misses +/// this entirely. +void TestHtmlSanitiser::aProtocolRelativeUrlIsRemoved() +{ + const QString out = HtmlSanitiser::stripRemoteContent( + QStringLiteral("")); + + QVERIFY2(!out.contains(QStringLiteral("//evil.example")), + qPrintable(QStringLiteral("a protocol-relative URL survived: ") + + out)); +} + +/// Uppercase tags, single quotes, and newlines around '=' are all real. Each +/// one alone defeats a naive pattern. +void TestHtmlSanitiser::quotingAndCaseAndWhitespaceDoNotHelp() +{ + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent( + QStringLiteral(""))); + + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent( + QStringLiteral(""))); + + assertNoRemoteUrls(HtmlSanitiser::stripRemoteContent( + QStringLiteral(""))); +} + +/// These elements exist to fetch or redirect. Emptying the attribute is not +/// enough for " + "" + "" + "")); + + 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" -- cgit v1.2.3