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_htmlsanitiser.cpp | 249 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 249 insertions(+) create mode 100644 tests/test_htmlsanitiser.cpp (limited to 'tests/test_htmlsanitiser.cpp') 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" -- cgit v1.2.3