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 --- .../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 +++++++++++++++++++++ 3 files changed, 388 insertions(+), 60 deletions(-) create mode 100644 docs/superpowers/specs/2026-08-27-forward-html-design.md (limited to 'docs/superpowers') 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 `