diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/CMakeLists.txt | 1 | ||||
| -rw-r--r-- | src/composecontext.cpp | 21 | ||||
| -rw-r--r-- | src/composewindow.cpp | 187 | ||||
| -rw-r--r-- | src/composewindow.h | 31 | ||||
| -rw-r--r-- | src/htmlsanitiser.cpp | 336 | ||||
| -rw-r--r-- | src/htmlsanitiser.h | 84 | ||||
| -rw-r--r-- | src/messagebuilder.cpp | 35 | ||||
| -rw-r--r-- | src/types.h | 15 |
8 files changed, 706 insertions, 4 deletions
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 591e95f..85b5e56 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -6,6 +6,7 @@ add_library(qtmaildir_lib STATIC messagebuilder.cpp requestinterceptor.cpp htmlbuilder.cpp + htmlsanitiser.cpp cidschemehandler.cpp cardlayout.cpp avatar.cpp diff --git a/src/composecontext.cpp b/src/composecontext.cpp index 2dfee53..b02b790 100644 --- a/src/composecontext.cpp +++ b/src/composecontext.cpp @@ -30,6 +30,7 @@ #include <QDir> #include <QSet> #include <QRegularExpression> +#include <QTextDocumentFragment> namespace { @@ -630,10 +631,28 @@ QString ComposeContextBuilder::quoteBody(const ParsedMessage &message) .arg(message.date, message.from)); quoted.append(QString()); + // 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 with nothing to say so. Measured 2026-08-27, ~9% of the + // developer's inbox declares text/html with no text/plain. + // + // Only when there is no plain part. Rendering the HTML over a real plain + // part would change every ordinary reply, and the sender's own plain text + // is what they meant a text reader to see. + // + // QTextDocumentFragment, not a hand-written stripper: it is already + // linked, it decodes entities and collapses whitespace the way a reader + // expects, and reaching for a regex here would be re-implementing a parser + // badly. This restores the WORDS only; preserving the formatting is the + // multipart/alternative half of item 171. + QString source = message.plainBody; + if (source.isEmpty() && !message.htmlBody.isEmpty()) + source = QTextDocumentFragment::fromHtml(message.htmlBody).toPlainText(); + // Normalised to LF first. A CRLF body split on '\n' alone leaves a // carriage return at the end of every line, which survives into the sent // message as a stray CR in the middle of a quoted line. - QString body = message.plainBody; + QString body = source; body.replace(QStringLiteral("\r\n"), QStringLiteral("\n")); body.replace(QLatin1Char('\r'), QLatin1Char('\n')); diff --git a/src/composewindow.cpp b/src/composewindow.cpp index afcf6a2..7952efa 100644 --- a/src/composewindow.cpp +++ b/src/composewindow.cpp @@ -21,6 +21,10 @@ #include <QTemporaryDir> #include "draftstore.h" +#include "htmlsanitiser.h" +#include <QCheckBox> +#include <QTextBrowser> +#include <QSplitter> #include "maildirname.h" #include "messagebuilder.h" #include "mimeparser.h" @@ -134,6 +138,16 @@ ComposeWindow::ComposeWindow(const ComposeContext &context, buildUi(); buildFormatToolbar(); + + // BEFORE buildMenuBar() and seedBody(). The menus show the pane toggle, so + // the preview that owns it must exist first; and seedBody() needs to know + // whether this forward carries markup, because an HTML forward does not + // seed a text quote at all (the buffer would then show something the sent + // message does not contain). + readForwardedHtml(); + buildStripRemoteControl(); + buildForwardPreview(); + // AFTER buildFormatToolbar(): the menus show its actions, so they must // exist before a menu can hold them. buildMenuBar(); @@ -147,6 +161,7 @@ ComposeWindow::ComposeWindow(const ComposeContext &context, // on it, which is precisely the defect this fixes. extractForwardedAttachments(); + refreshAttachmentList(); // Seeding is not an edit. Every field was just filled from the context, so @@ -173,6 +188,127 @@ ComposeWindow::ComposeWindow(const ComposeContext &context, ComposeWindow::~ComposeWindow() = default; +void ComposeWindow::readForwardedHtml() +{ + if (m_context.kind != ComposeContext::Kind::Forward + || m_context.originalPath.isEmpty()) { + return; + } + + MimeParser parser; + const ParsedMessage original = parser.parse(m_context.originalPath); + if (original.ok) + m_forwardedHtmlRaw = original.htmlBody; +} + +void ComposeWindow::buildForwardPreview() +{ + if (m_forwardedHtmlRaw.isEmpty()) + return; + + m_forwardPreview = new QWidget(centralWidget()); + m_forwardPreview->setObjectName(QStringLiteral("forwardPreview")); + auto *previewLayout = new QVBoxLayout(m_forwardPreview); + previewLayout->setContentsMargins(0, 0, 0, 0); + + auto *label = new QLabel( + tr("Forwarded message, sent as it arrived:"), m_forwardPreview); + label->setObjectName(QStringLiteral("forwardPreviewLabel")); + previewLayout->addWidget(label); + + // **QTextBrowser, not a QWebEngineView.** A web view would mean a second + // Chromium render process per composer window and a second copy of + // MessageView's protections (the off-the-record profile, JavaScript off, + // the interceptor that fails closed), which is a lot of security surface + // for a preview. QTextBrowser renders Qt's own HTML subset, is read-only, + // and fetches nothing on its own. + // + // The consequence is deliberate and must be said in the UI rather than + // hidden: this shows the original ROUGHLY. Qt's subset is narrower than a + // mail client's, so the preview is an indication of content, not a + // faithful rendering of what the recipient will see. The label above says + // the message is sent as it arrived, so the user is not led to think this + // pane is what travels. + auto *view = new QTextBrowser(m_forwardPreview); + view->setObjectName(QStringLiteral("forwardPreviewBody")); + view->setOpenExternalLinks(false); + view->setOpenLinks(false); + + // The SANITISED markup when stripping is on, so the preview shows what + // will actually be sent rather than the original's own remote content. + // Re-rendered when the checkbox moves, for the same reason. + view->setHtml(HtmlSanitiser::stripRemoteContent(m_forwardedHtmlRaw)); + previewLayout->addWidget(view); + + m_split->addWidget(m_forwardPreview); + + // 60/40, the user's ratio. Set as STRETCH FACTORS rather than as pixel + // sizes: the window has no meaningful width yet at construction, and a + // setSizes() against a zero-width splitter divides nothing. Stretch + // survives the first real resize, which pixels would not. + m_split->setStretchFactor(0, 6); + m_split->setStretchFactor(1, 4); + m_split->setSizes({ 600, 400 }); + + // The toggle, on the View half of the menus so it sits with the other + // things that show and hide. Only created for a forward that has a pane to + // toggle: an action that can never do anything is worse than none, which + // is the same reasoning the attachment row's Remove button follows. + m_showForwardAction = new QAction(tr("Forwarded message"), this); + m_showForwardAction->setObjectName(QStringLiteral("compose_show_forward")); + m_showForwardAction->setCheckable(true); + m_showForwardAction->setChecked(true); + m_showForwardAction->setToolTip( + tr("Shows the message being forwarded beside what you are writing.")); + connect(m_showForwardAction, &QAction::toggled, m_forwardPreview, + &QWidget::setVisible); + // Placed by buildMenuBar(), which runs after this. + + if (m_stripRemote) { + connect(m_stripRemote, &QCheckBox::toggled, view, + [this, view](bool strip) { + view->setHtml(strip ? HtmlSanitiser::stripRemoteContent( + m_forwardedHtmlRaw) + : m_forwardedHtmlRaw); + }); + } +} + +void ComposeWindow::buildStripRemoteControl() +{ + if (m_forwardedHtmlRaw.isEmpty() + || !HtmlSanitiser::hasRemoteContent(m_forwardedHtmlRaw)) { + return; + } + + m_stripRemote = new QCheckBox( + tr("Strip remote content from the forwarded message"), this); + m_stripRemote->setObjectName(QStringLiteral("stripRemote")); + + // **Checked by default, and that default is the security property.** The + // markup leaves this process and is rendered by the recipient's client, + // where none of MessageView's protections apply: forwarding a tracking + // pixel forwards the tracking, and the original sender learns that the + // forwarded copy was opened and by how many people. + m_stripRemote->setChecked(true); + m_stripRemote->setToolTip( + tr("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.")); + + connect(m_stripRemote, &QCheckBox::toggled, this, &ComposeWindow::markDirty); + + // Above the attachment row, which is where the other per-message controls + // sit. Inserted rather than appended: the body must keep its stretch. + if (auto *layout = qobject_cast<QVBoxLayout *>(centralWidget()->layout())) { + const int at = layout->indexOf(m_attachmentRow); + if (at >= 0) + layout->insertWidget(at, m_stripRemote); + else + layout->addWidget(m_stripRemote); + } +} + void ComposeWindow::extractForwardedAttachments() { if (m_context.kind != ComposeContext::Kind::Forward @@ -353,7 +489,17 @@ void ComposeWindow::buildUi() m_body = new QPlainTextEdit(central); m_body->setObjectName(QStringLiteral("body")); - layout->addWidget(m_body, 1); + + // The editor lives in a splitter so an HTML forward can put the forwarded + // message BESIDE it rather than under it (item 171, the user's choice + // 2026-08-27). With nothing to show the splitter holds one widget and is + // indistinguishable from the plain editor it replaces, so every other + // composer is unaffected. + m_split = new QSplitter(Qt::Horizontal, central); + m_split->setObjectName(QStringLiteral("composeSplit")); + m_split->setChildrenCollapsible(false); + m_split->addWidget(m_body); + layout->addWidget(m_split, 1); // The attachment list, with Remove beside it: the control acts on the // list, so it lives with it, and both appear only once something is @@ -719,6 +865,13 @@ void ComposeWindow::buildMenuBar() // copy of its entries would go stale on the next rebuild. QAction *signature = format->addMenu(m_signatureSwitch->menu()); signature->setText(tr("Signature")); + + // Item 171. Only on a forward that has a pane to toggle; an action that + // can never do anything is worse than no action at all. + if (m_showForwardAction) { + format->addSeparator(); + format->addAction(m_showForwardAction); + } } void ComposeWindow::seedFields() @@ -791,6 +944,17 @@ void ComposeWindow::seedBody() if (m_context.quotedBody.isEmpty()) return; + // Item 171. An HTML forward carries the original as MARKUP, so seeding the + // text quote here would put something in the buffer that the sent message + // does not contain: the user could edit it and the edits would be + // discarded silently. The preview below the editor shows what is actually + // carried. A PLAIN forward is untouched, because there the quote in the + // buffer IS what gets sent. + if (m_context.kind == ComposeContext::Kind::Forward + && !m_forwardedHtmlRaw.isEmpty()) { + return; + } + // Applied when the window opens and never again. The buffer is text the // user owns after that, and there is deliberately no live toggle: // tracking "my text" and "the quote" as separate pieces to make a toggle @@ -1016,6 +1180,18 @@ OutgoingMessage ComposeWindow::currentMessage() const message.subject = m_subject->text(); message.markdownBody = m_body->toPlainText(); message.sendHtml = m_sendHtml->isChecked(); + + // Item 171. Sanitised HERE rather than at parse time, so the control can + // be toggled without re-reading the file, and so the raw markup is never + // what reaches OutgoingMessage by default. The checkbox only exists when + // there is remote content to strip; without it the raw markup IS the safe + // markup, which is why the fallback is `true` rather than `false`. + if (!m_forwardedHtmlRaw.isEmpty()) { + const bool strip = m_stripRemote ? m_stripRemote->isChecked() : true; + message.forwardedHtml = + strip ? HtmlSanitiser::stripRemoteContent(m_forwardedHtmlRaw) + : m_forwardedHtmlRaw; + } message.attachments = m_attachments; message.inReplyTo = m_context.inReplyTo; message.references = m_context.references; @@ -1227,8 +1403,15 @@ bool ComposeWindow::saveDraftNow() account.maildir + QLatin1Char('/') + account.drafts); const QString previousPath = m_draftPath; + + // "DS", not "D". `maildir.synchronize_flags` is on, so notmuch tags any + // message lacking the `S` flag `unread`, and a draft the user just wrote + // is seen by definition. Without the S it lands in the Unread view until + // the folder next syncs, at which point mbsync round-trips the file and + // the flag appears on its own; that self-healing is what made the defect + // look intermittent rather than constant. const DraftStore::Result written = - DraftStore::write(folder, built.bytes, QStringLiteral("D"), m_draftPath); + DraftStore::write(folder, built.bytes, QStringLiteral("DS"), m_draftPath); if (!written.ok()) { // A PERSISTENT banner, not a modal and not a status-bar line that diff --git a/src/composewindow.h b/src/composewindow.h index c8cc12a..9affce1 100644 --- a/src/composewindow.h +++ b/src/composewindow.h @@ -34,6 +34,7 @@ class QAction; class QCheckBox; +class QSplitter; class QComboBox; class QLabel; class QLineEdit; @@ -223,6 +224,15 @@ private: /// MessageBuilder refuses a build naming any path that later vanishes, so /// a silently wrong send is not among the outcomes. void extractForwardedAttachments(); + + /// Reads the forwarded original's HTML, before the body is seeded. + void readForwardedHtml(); + + /// Creates the strip-remote-content checkbox, for a forward that needs it. + void buildStripRemoteControl(); + + /// Creates the read-only preview of what an HTML forward will carry. + void buildForwardPreview(); void seedBody(); /// Applies \p name to the buffer, replacing whatever is there. @@ -300,6 +310,27 @@ private: QComboBox *m_from = nullptr; QPlainTextEdit *m_body = nullptr; QToolButton *m_sendHtml = nullptr; + + /// Item 171. Strips remote content from the forwarded original, checked by + /// default. Only created for a Forward whose original carries remote + /// content, so an ordinary message gains no control. + QCheckBox *m_stripRemote = nullptr; + + /// Read-only view of the original an HTML forward will carry. Item 171: + /// the buffer holds the user's note only, so this is what makes the rest + /// of the message visible without pretending it can be edited. + QWidget *m_forwardPreview = nullptr; + + /// Holds the editor, and the forward preview beside it when there is one. + QSplitter *m_split = nullptr; + + /// Shows and hides the forwarded-message pane. Only for a forward. + QAction *m_showForwardAction = nullptr; + + /// The forwarded original's HTML, as read from `originalPath` at + /// construction. Held raw; the stripping happens at currentMessage(), + /// so toggling the control does not need a re-parse. + QString m_forwardedHtmlRaw; QToolButton *m_signatureSwitch = nullptr; QString m_signatureDir; QString m_signatureName; ///< The selected signature, empty for None. diff --git a/src/htmlsanitiser.cpp b/src/htmlsanitiser.cpp new file mode 100644 index 0000000..656f4fd --- /dev/null +++ b/src/htmlsanitiser.cpp @@ -0,0 +1,336 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * 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 "htmlsanitiser.h" + +#include <QRegularExpression> +#include <QSet> +#include <QStringList> + +namespace { + +/// Elements that exist to fetch or redirect, removed with their content. +/// +/// `script` and `style` carry their payload as TEXT, so emptying an attribute +/// would leave the fetch intact; the element goes whole. `style` is here even +/// though CSS is otherwise kept, because a block can carry `@import` and +/// `url()` and rewriting inside it is the same problem as the attribute case +/// with different terminators. Presentational styling survives through +/// `style=""` attributes, which are handled per attribute below. +const QSet<QString> &elementsRemovedWhole() +{ + static const QSet<QString> set = { + QStringLiteral("script"), QStringLiteral("style"), + QStringLiteral("iframe"), QStringLiteral("object"), + QStringLiteral("embed"), QStringLiteral("applet"), + QStringLiteral("frame"), QStringLiteral("frameset"), + }; + return set; +} + +/// Void elements that fetch or redirect and have no closing tag. +const QSet<QString> &voidElementsRemoved() +{ + static const QSet<QString> set = { + QStringLiteral("link"), QStringLiteral("base"), + QStringLiteral("meta"), + }; + return set; +} + +/// True when \p value names something that would be fetched from off-message. +/// +/// The test is on the SCHEME, never on a substring: a `cid:` id may legitimately +/// contain "https" (`cid:https-logo@example.org`), and stripping that would +/// destroy a valid inline reference. +/// +/// Everything that is not plainly a `cid:` or a same-document fragment counts +/// as remote. That is the allow-list rule: an unanticipated scheme is remote by +/// default rather than by enumeration. +bool valueIsRemote(const QString &value) +{ + const QString trimmed = value.trimmed(); + if (trimmed.isEmpty()) + return false; + + // Protocol-relative: no scheme, still fetches, and a check for "http" + // misses it entirely. + if (trimmed.startsWith(QLatin1String("//"))) + return true; + + // A same-document fragment or a bare relative path fetches nothing new in + // a mail context and carries no scheme to judge. + const qsizetype colon = trimmed.indexOf(QLatin1Char(':')); + if (colon < 0) + return !trimmed.startsWith(QLatin1Char('#')) ? false : false; + + const QString scheme = trimmed.left(colon).toLower(); + + // A colon can appear in a relative path with no scheme before it; a scheme + // is letters, digits, '+', '-', '.' only. + static const QRegularExpression schemeShape( + QStringLiteral("^[a-z][a-z0-9+.-]*$")); + if (!schemeShape.match(scheme).hasMatch()) + return false; + + return scheme != QLatin1String("cid"); +} + +/// True when a CSS fragment reaches the network. +/// +/// `url()`, `@import` and `image-set()` all fetch, in a `style=""` attribute +/// and in a block alike. The bare `url(...)` form terminates on ')' rather +/// than on a quote, which is why this is a pass of its own rather than the +/// attribute logic reused. +bool cssIsRemote(const QString &css) +{ + static const QRegularExpression url( + QStringLiteral("url\\s*\\(\\s*['\"]?([^'\")]*)"), + QRegularExpression::CaseInsensitiveOption); + + auto it = url.globalMatch(css); + while (it.hasNext()) { + if (valueIsRemote(it.next().captured(1))) + return true; + } + + static const QRegularExpression atImport( + QStringLiteral("@import\\s+['\"]?([^'\";]*)"), + QRegularExpression::CaseInsensitiveOption); + auto imports = atImport.globalMatch(css); + while (imports.hasNext()) { + const QString target = imports.next().captured(1).trimmed(); + // `@import url(...)` is already covered by the url() pass; a bare + // `@import "x.css"` is not. + if (!target.startsWith(QLatin1String("url"), Qt::CaseInsensitive) + && valueIsRemote(target)) { + return true; + } + } + + // image-set() wraps url() in every real spelling, so the url() pass above + // covers it; a bare image-set("x.png") is caught here. + static const QRegularExpression imageSet( + QStringLiteral("image-set\\s*\\(\\s*['\"]([^'\"]*)"), + QRegularExpression::CaseInsensitiveOption); + auto sets = imageSet.globalMatch(css); + while (sets.hasNext()) { + if (valueIsRemote(sets.next().captured(1))) + return true; + } + + return false; +} + +/// One attribute, as parsed out of a tag. +struct Attribute +{ + QString name; ///< Lowercased. + QString raw; ///< The whole `name="value"` source, to re-emit verbatim. + QString value; ///< Unquoted. +}; + +/// Splits the inside of a tag into its attributes. +/// +/// Tolerates the three quoting forms and whitespace or newlines around '=', +/// all of which are real in mail and each of which alone defeats a naive +/// pattern. +QList<Attribute> parseAttributes(const QString &inner) +{ + QList<Attribute> out; + + static const QRegularExpression attr( + QStringLiteral("([a-zA-Z_:][-a-zA-Z0-9_:.]*)" // name + "(?:\\s*=\\s*" // = with space + "(?:\"([^\"]*)\"|'([^']*)'|([^\\s>]+))" // 3 quotings + ")?")); + + auto it = attr.globalMatch(inner); + while (it.hasNext()) { + const QRegularExpressionMatch m = it.next(); + Attribute a; + a.name = m.captured(1).toLower(); + a.raw = m.captured(0); + for (int group : { 2, 3, 4 }) { + if (m.hasCaptured(group)) { + a.value = m.captured(group); + break; + } + } + out.append(a); + } + + return out; +} + +/// True when this attribute must not survive, whatever element carries it. +bool attributeIsUnsafe(const Attribute &attr) +{ + // Event handlers. The recipient's client most likely disables scripting, + // but that is their policy and not ours to assume for them. + if (attr.name.startsWith(QLatin1String("on"))) + return true; + + // CSS reaches the network from inside a style attribute. + if (attr.name == QLatin1String("style")) + return cssIsRemote(attr.value); + + // **The allow-list.** Every other attribute is judged by its VALUE rather + // than by whether the name was enumerated. This is the difference from + // HtmlBuilder::namespaceCids(), which names the attributes it rewrites and + // scopes srcset out: a missed rewrite is a broken image, a missed strip is + // a beacon. srcset, poster, data-*, and whatever HTML adds next are all + // handled here by default. + // + // srcset carries a LIST of "url descriptor" pairs, so each entry is + // judged; a single remote entry condemns the attribute. + for (const QString &piece : attr.value.split(QLatin1Char(','))) { + const QString candidate = piece.trimmed().section(QLatin1Char(' '), 0, 0); + if (valueIsRemote(candidate)) + return true; + } + + return false; +} + +/// The shared walk. \p report is called for anything that would be removed; +/// when \p rewrite is false the walk only reports, which is what +/// hasRemoteContent() needs. +QString walk(const QString &html, bool *foundOut) +{ + QString out; + out.reserve(html.size()); + bool found = false; + + static const QRegularExpression tag(QStringLiteral("<(/?)([a-zA-Z][^\\s/>]*)([^>]*)>")); + + // Matched by hand from `pos` rather than with globalMatch(): skipping a + // removed element's CONTENT moves pos forward, and globalMatch iterates + // over matches found against the original string, so it would hand back + // tags from inside the region just skipped. That produced duplicated + // output and a surviving iframe, caught by + // aFetchingElementIsRemovedWhole(). + qsizetype pos = 0; + while (true) { + const QRegularExpressionMatch m = tag.match(html, pos); + if (!m.hasMatch()) + break; + + out += html.mid(pos, m.capturedStart() - pos); + pos = m.capturedEnd(); + + const bool closing = !m.captured(1).isEmpty(); + const QString name = m.captured(2).toLower(); + const QString inner = m.captured(3); + + if (elementsRemovedWhole().contains(name)) { + found = true; + if (!closing) { + // Drop the content too: for script and style it IS the payload. + const QRegularExpression until( + QStringLiteral("</\\s*%1\\s*>").arg(name), + QRegularExpression::CaseInsensitiveOption); + const QRegularExpressionMatch end = until.match(html, pos); + pos = end.hasMatch() ? end.capturedEnd() : html.size(); + } + continue; + } + + if (voidElementsRemoved().contains(name)) { + // meta is only dangerous as a refresh; the charset declaration is + // ordinary and harmless. + if (name == QLatin1String("meta")) { + bool refresh = false; + for (const Attribute &a : parseAttributes(inner)) { + if (a.name == QLatin1String("http-equiv") + && a.value.trimmed().compare(QLatin1String("refresh"), + Qt::CaseInsensitive) == 0) { + refresh = true; + } + } + if (!refresh) { + out += m.captured(0); + continue; + } + } + found = true; + continue; + } + + if (closing) { + out += m.captured(0); + continue; + } + + // Rebuild the tag from the attributes that survive. + QStringList kept; + bool dropped = false; + for (const Attribute &a : parseAttributes(inner)) { + if (attributeIsUnsafe(a)) { + dropped = true; + continue; + } + kept.append(a.raw); + } + + if (dropped) + found = true; + + // An <img> whose src was the thing removed would render as a broken + // image icon in the recipient's client, which is noisier than the gap + // the design asks for. Drop the element instead, and only when it has + // nothing left to show. + if (dropped && name == QLatin1String("img")) { + bool hasSrc = false; + for (const QString &k : kept) { + if (k.startsWith(QLatin1String("src"), Qt::CaseInsensitive)) + hasSrc = true; + } + if (!hasSrc) + continue; + } + + QString rebuilt = QStringLiteral("<") + m.captured(2); + if (!kept.isEmpty()) + rebuilt += QLatin1Char(' ') + kept.join(QLatin1Char(' ')); + if (inner.trimmed().endsWith(QLatin1Char('/'))) + rebuilt += QLatin1Char('/'); + rebuilt += QLatin1Char('>'); + out += rebuilt; + } + + out += html.mid(pos); + + if (foundOut) + *foundOut = found; + return out; +} + +} // namespace + +QString HtmlSanitiser::stripRemoteContent(const QString &html) +{ + return walk(html, nullptr); +} + +bool HtmlSanitiser::hasRemoteContent(const QString &html) +{ + bool found = false; + walk(html, &found); + return found; +} diff --git a/src/htmlsanitiser.h b/src/htmlsanitiser.h new file mode 100644 index 0000000..52257d5 --- /dev/null +++ b/src/htmlsanitiser.h @@ -0,0 +1,84 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * 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. + */ + +#pragma once + +#include <QString> + +/// Makes a stranger's HTML safe to put INSIDE A MESSAGE WE SEND. +/// +/// Item 171. This is not the message pane's problem restated: the pane renders +/// hostile markup TO THE USER, behind protections that live in `MessageView` +/// (an off-the-record profile, JavaScript disabled, and `RequestInterceptor` +/// blocking every request by default and failing closed). **None of those +/// apply to a forward.** The markup leaves this process and is rendered by +/// somebody else's client, under their policy, on their machine, and the +/// interceptor cannot help because it intercepts requests WE would make. +/// +/// So the sanitising happens to the bytes, before `MessageSender` sees them, +/// and there is no second line of defence behind it. Forwarding a tracking +/// pixel forwards the tracking: the original sender learns that the forwarded +/// copy was opened, by whom and how often, and the recipient never agreed to +/// that. +/// +/// **This is an ALLOW-LIST, and the distinction from `namespaceCids()` is the +/// whole design.** That function is a block-list: it names the attributes that +/// can carry a `cid:` and rewrites those, and it documents scoping `srcset=` +/// out because its quoting grammar differs. That trade is correct for +/// rewriting and wrong here, because the two failures are not comparable: +/// +/// | | a missed reference means | +/// |---|---| +/// | `namespaceCids` (rewrite) | one broken image | +/// | this (strip) | a beacon reaching the recipient | +/// +/// Anything not recognised is therefore REMOVED rather than kept. A new +/// attribute, an unanticipated quoting form, a `srcset`, a CSS `image-set()`, +/// an `@import`: each is handled by the default, not by having been enumerated +/// in advance. +/// +/// The invariant every test asserts, and the one to preserve under any edit: +/// **after sanitising, no attribute value and no CSS construct contains a URL +/// whose scheme is anything but `cid:`.** +namespace HtmlSanitiser { + +/// \p html with every remote-fetching construct removed. +/// +/// `cid:` references are KEPT: they travel inside the message, fetch nothing, +/// and are what lets an inline logo survive a forward. Structural and +/// presentational markup is kept too, `style=""` included, with its remote +/// constructs removed. +/// +/// Removed: any attribute value carrying a non-`cid:` URL (`http:`, `https:`, +/// protocol-relative `//host/path`, `data:`, `file:`); the elements that exist +/// to fetch or redirect (`link`, `script`, `iframe`, `object`, `embed`, +/// `base`, and `meta http-equiv="refresh"`); CSS `url()`, `@import` and +/// `image-set()` naming anything but a `cid:`; and event-handler attributes. +/// +/// A stripped `<img>` leaves a gap. That is correct, and must not be papered +/// over with a placeholder that itself fetches. +QString stripRemoteContent(const QString &html); + +/// True when \p html carries anything `stripRemoteContent()` would remove. +/// +/// Drives the composer's control: the checkbox is only worth showing for a +/// message that actually has remote content. Never used to DECIDE whether to +/// strip, only whether to offer the choice. +bool hasRemoteContent(const QString &html); + +} // namespace HtmlSanitiser diff --git a/src/messagebuilder.cpp b/src/messagebuilder.cpp index 42a0e31..fe23862 100644 --- a/src/messagebuilder.cpp +++ b/src/messagebuilder.cpp @@ -320,7 +320,40 @@ Result build(const OutgoingMessage &message, const Account &account) // second renderer whose output could disagree with the HTML one. GMimeObject *body = GMIME_OBJECT(makeTextPart("plain", message.markdownBody)); - if (message.sendHtml) { + // Item 171. A FORWARD sends one part, not an alternative, at the user's + // decision 2026-08-27: the Send-as-HTML toggle chooses which. A forward is + // a message whose shape the user has already decided by flipping that + // toggle, and sending both halves hands the choice to the recipient's + // client instead. + // + // The toggle is honoured even when the original had no plain-text part: + // with it off, an HTML-only original goes out as the text fallback that + // quoteBody() produced, and the formatting is lost. Chosen over forcing + // HTML for those messages, so the toggle means what it says. + // + // The markup arrives ALREADY SANITISED: whether to strip remote content is + // the user's per-forward choice, which a builder cannot see. Nothing is + // escaped here, deliberately, because this IS markup and escaping it would + // ship a message full of visible tags. + const bool forwarding = !message.forwardedHtml.isEmpty(); + + if (forwarding && message.sendHtml) { + // The original below the user's own text, separated by a rule so the + // two read as different messages. The plain part built above is + // discarded: this replaces it rather than joining it. + // `markdownBody` is the user's own note ALONE: the composer does not + // seed a text quote on an HTML forward, precisely so that what it + // shows and what it sends are the same thing (item 171). An earlier + // build seeded the quote and stripped it again here, which meant the + // user could edit a quote whose edits were discarded; the fix belongs + // at the composer, not in a subtraction here. + const QString htmlSource = MarkdownRenderer::toHtml(message.markdownBody) + + QStringLiteral("\n<hr>\n") + + message.forwardedHtml; + GMimePart *html = makeTextPart("html", htmlSource); + g_object_unref(body); + body = GMIME_OBJECT(html); + } else if (!forwarding && message.sendHtml) { GMimePart *html = makeTextPart("html", MarkdownRenderer::toHtml(message.markdownBody)); GMimeMultipart *alternative = g_mime_multipart_new_with_subtype("alternative"); // Least-rich FIRST. A client renders the LAST alternative it diff --git a/src/types.h b/src/types.h index c78ab76..8e24cd1 100644 --- a/src/types.h +++ b/src/types.h @@ -378,6 +378,21 @@ struct OutgoingMessage QStringList attachments; ///< Local paths, read at build time. QString inReplyTo; QStringList references; + + /// Item 171. The forwarded original's HTML, already sanitised, appended to + /// the HTML alternative below the user's own text. + /// + /// Empty for everything but a Forward of a message that had an HTML part. + /// Carrying it here rather than re-parsing at build time keeps + /// MessageBuilder a pure function of this struct, which is what lets the + /// MIME nesting be tested without a file on disk. + /// + /// **Sanitised by the CALLER**, with HtmlSanitiser::stripRemoteContent(), + /// because whether to strip is the user's per-forward choice and a + /// builder cannot see a checkbox. The one exception to that rule is the + /// user deliberately unchecking it. + QString forwardedHtml; + }; Q_DECLARE_METATYPE(ThreadSummary) |