diff options
Diffstat (limited to 'docs/superpowers')
3 files changed, 388 insertions, 60 deletions
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 `<iframe>` survived. It matches by hand +from `pos` now. A tidy test would not have found this: it needed a removed +element with content, mid-document, followed by more markup. + +**Measured, not assumed:** 30 of 342 sampled inbox messages (~9%) declare +`text/html` with no `text/plain`, so the silent-loss half was not an edge case. + +**Hand-tested 2026-08-27, and it found two defects, the second of which +changed the design.** + +1. The original shipped TWICE inside the one HTML part: the composer seeds the + text quote into the editable body, so `markdownBody` already carried a + flattened copy, and the markup was appended to it. It read as two messages + stacked, the first with its URLs naked and mangled. A screenshot of a real + forward is what showed it; no test had covered the composer and the builder + together. + +2. The first fix subtracted the quote in `MessageBuilder`. **The user rejected + it**: the quote was still shown in the composer and no longer sent, so it + could be edited and the edits silently discarded. "If it is in the composer + but it's not sent, is worse." That is the right objection and the general + principle behind it, **what the composer shows must be what gets sent**, is + what the design now follows. + +So an HTML forward **does not seed a text quote at all**. The buffer holds the +user's own note alone, and the forwarded message appears in a read-only pane +BESIDE the editor, a `QSplitter` at 60/40 with a toggle in the Format menu (the +user's chosen arrangement). A plain forward is untouched: its quote is in the +buffer, where it is both editable and sent, so WYSIWYG already held there. + +The pane is a `QTextBrowser`, not a `QWebEngineView`: a web view would mean a +second Chromium render process per composer and a second copy of MessageView's +protections. The cost is that Qt's HTML subset is narrower than a mail +client's, so the pane shows the original ROUGHLY. Its label says the message +is sent as it arrived, so the pane is not mistaken for what travels. + +**The user asked for a real rich-text composer as the proper answer**, recorded +as item 173, which supersedes this middle ground and subsumes item 133. + +Two test traps hit while building it, both already in CLAUDE.md and both hit +anyway: the offscreen platform gives a `QSplitter` no width, so `sizes()` +reports 49/49 whatever the code asks and a pixel assertion fails against +correct code (assert the stretch factors, which land in the child's size +policy since `QSplitter` has no getter); and moving the editor into a splitter +broke `theComposerSplitsItsToolbarByScope`, which looked for the body directly +in the composer's column. + +Still to do: the hand test of the new arrangement. Forward a real HTML message +with the box checked and unchecked and confirm what arrives. + diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index fddb746..9cb8ba9 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -202,7 +202,7 @@ taking that too literally. | 130 | A message cannot be attached to another message directly | v2 | S | open, 2026-08-20, from the item 123 brainstorm. **Blocked on 123.** A `message/rfc822` part, which GMime builds natively. The manual route exists from 123's first commit: `save_message` writes the `.eml` and it is attached as a file | | 131 | The markdown dialect and extensions are fixed | v2 | S | open, 2026-08-20, from the item 123 brainstorm. **Blocked on 123.** Configurable in the shape Hugo's config uses. Deliberately fixed initially: CommonMark plus autolink, strikethrough and tasklist | | 132 | Every action must have a shortcut, and that no longer serves | policy | S | done, 2026-08-20. `everyActionHasAShortcut` is deleted and nothing replaces it: `everyActionIsReachableFromAMenu()` is the required rule and a shortcut is now a chosen subset. Nothing else needed changing, since `showShortcutReference()` already printed `(unbound)` for an empty sequence. Verified by unbinding `tag_rules` and running the suite green, which would have failed before | -| 133 | The composer shows no markdown syntax highlighting | v2 | S | open, 2026-08-20, from the item 123 brainstorm. **Blocked on 123.** A `QSyntaxHighlighter` over the composer's editor, so `**bold**` reads as bold while the buffer stays plain markdown. Standard Qt, no dependency. Deliberately after 123's formatting toolbar: agreeing with the grammar about nesting and about code spans suppressing what is inside them is the expensive part, and the toolbar is what makes the feature usable | +| 133 | The composer shows no markdown syntax highlighting | v2 | S | open, 2026-08-20, from the item 123 brainstorm. **Blocked on 123.** A `QSyntaxHighlighter` over the composer's editor, so `**bold**` reads as bold while the buffer stays plain markdown. Standard Qt, no dependency. Deliberately after 123's formatting toolbar: agreeing with the grammar about nesting and about code spans suppressing what is inside them is the expensive part, and the toolbar is what makes the feature usable . **Subsumed by item 173** 2026-08-27: that is a rich-text editor, this is the cheap answer to the same want. Build one or the other, never both | | 134 | The busy indicator is built inline and is about to be built twice | maintenance | S | done, 2026-08-20, af902e0. `BusyIndicator` (`src/busyindicator.h`) carries both modes: `MainWindow` uses the indeterminate one, and item 123's send popup takes the determinate half for its undo countdown, switching the same widget over when the command starts. Only the BAR was extracted, not the status label this row paired with it. `m_statusLabel` has 34 uses across `MainWindow` for transient messages, selection counts and sync phases, so it belongs to the window rather than to the indicator, and the send popup owns its own phase text | | 135 | The formatting toolbar's buttons stack rather than toggle | v2 | S | open, 2026-08-21, asked for by the user during item 123 task 8 and reverted the same session. **A spec change, not a defect**: it conflicts with spec:236 ("deliberately no live toggle") and spec:187-190. Both sites need amending FIRST, and the amendment must resolve what replaces bold-then-italic, which is the gesture spec:187's preserved selection exists to serve and which a toggle makes unreachable. That question is the work; the state machine is understood and written up in the section | | 136 | `undoMovesTheMessageBack` fails when run ALONE, passes in the full suite | defect | ? | open, 2026-08-21, re-measured 2026-08-24 and it is not what the row said. Filed as an intermittent race (1 in 6); it is in fact **deterministic on the selection**: 6 failures in 6 when named on the command line, and, as of 2026-08-24, it fails in the FULL run too: measured at 58f13ad with the day's work stashed out, 274 passed and this one failed. The "passes in the suite" half of this row is therefore no longer true, and the selection-dependence it was named for may not be either. Re-measure before theorising. All three of its 15s `QTRY` timeouts expire, giving 45s against a 25s whole-suite run, so undo never moves the file rather than losing a race. A test that needs its predecessors is the likely shape (the `init()` lock-table fixture of item 61 is one candidate), which makes it a TEST defect until shown otherwise. Not caused by item 149, and re-confirmed 2026-08-24 as not caused by item 152 either, by running the test at the preceding commit in a throwaway worktree. The assertion that fails names the real question: the restored file is in NEITHER `cur` nor `new` of the account inbox, so establish where it went before theorising about a race | @@ -244,8 +244,9 @@ taking that too literally. | 168 | Delete is offered on mail already in the trash, and does nothing | defect | S | **done 2026-08-25**, unreleased. Delete is hidden when every selected row is already in its account's trash, Restore when none is, both keyed on the PATH rather than the `deleted` tag. Delete also drops `unread` now, in the same TagChange so one undo returns the folder and the tag together | | 169 | A card shows the account only as a bar, with no fade and no avatar | presentation | M | **done** 2026-08-26, unreleased, on `card-avatars`, merged fast-forward. Both halves: a `QLinearGradient` from the account colour to the pane's base across the card, and a squircle avatar with initials, given a rect in `CardLayout` so the geometry is asserted without a painter. **Hand-testing found four defects**, all fixed in 9ae43f9: `Avatar::initialsFor()` normalises the display name first (drops the angle-addr, takes the first comma-separated author, unwraps quotes, treats a bare address as no name, requires a word to carry a letter or digit); the two-tone gradient axis spans the DIAMETER rather than a radius, which was letting one hue fill the whole face; the account fade runs right to left, anchored opaque at the card's right edge; and a flat view hashes `ThreadSummary::firstMessageRecipient` rather than the user's own address. The vCard half stays blocked on item 72 | | 170 | A row that stops matching the view only leaves it on the Delete path | defect | S | open, 2026-08-26, from the notes, **cause found the same day and the premise is NOT stale**. The optimistic REPAINT is universal; the optimistic MEMBERSHIP is not. `removeThreadsWithoutTag()` has exactly one caller, on the move path, so marking a message read in the Unread view repaints the row and leaves it in a list it no longer belongs to | -| 171 | A forwarded HTML message reaches the recipient as plain text | defect | S | open, 2026-08-27, from the notes. `ComposeContextBuilder::quoteBody()` reads `ParsedMessage::plainBody` only, so the original's `htmlBody` is dropped whatever the composer's own Send-as-HTML state is. Needs a DECISION on what a forward carries before any code, see the entry | +| 171 | A forwarded HTML message reaches the recipient as plain text | defect | M | **done** 2026-08-27, unreleased. Design in `specs/2026-08-27-forward-html-design.md`. The user chose inline `multipart/alternative` over attaching the original, and remote content stripped by default with a per-forward opt-out. Four parts: `HtmlSanitiser` (an ALLOW-LIST, unlike `namespaceCids()`, because a missed strip is a beacon where a missed rewrite is a broken image), a text fallback for the ~9% of mail with no plain part, the MIME nesting, and the composer control | | 172 | A draft this application writes is tagged `unread` | defect | XS | **done** 2026-08-27, unreleased. `DraftStore::write()` was called with `"D"`, and `maildir.synchronize_flags` makes notmuch tag anything without `S` as `unread`. Self-healing on the next sync of that folder, which is what made it look intermittent | +| 173 | The composer is a plain-text editor, not WYSIWYG | v2 | L | open, 2026-08-27, **asked for by the user** while hand-testing 171. This is a GUI mail client and should edit rich text the way one does: the forwarded original, and the user's own formatting, visible and editable in place. Supersedes the preview 171 shipped as a middle ground, and **subsumes item 133** (markdown syntax highlighting), which is the same want answered cheaply. See the entry: the draft format and the markdown-as-source-of-truth model both change | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1210,61 +1211,54 @@ every tag write passes. Move it, or call it from both send paths. - Undo goes back through the same funnel, so a removal must not make an undone mark-read invisible in the view it was undone in. -## 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. +## 173. The composer is a plain-text editor, not WYSIWYG + +**Observed (user, 2026-08-27):** asked for directly while hand-testing item +171. "This is a GUI mail client and should have a wysiwyg editor." + +**Where it came from.** Item 171 forwards an HTML message by carrying the +original's markup, and the markup cannot be shown in a `QPlainTextEdit`. The +first build seeded a text quote into the editable buffer and then dropped it +when building the HTML part, so the user could edit a quote whose edits were +silently discarded. The user's objection is the right one and is more general +than that bug: **what the composer shows should be what gets sent.** + +171 shipped the middle ground, a read-only preview beside the editor. This +item is the real answer. + +**Cause (verified in the code).** The composer is a `QPlainTextEdit` over +markdown, deliberately: `OutgoingMessage::markdownBody` is "the source text, +exactly as typed", `MarkdownRenderer` turns it into HTML at build time, and +`DraftStore` autosaves that same markdown. There is nowhere in that model for +a stranger's markup, or for the user's own rich text, to live and be edited. + +**Approach.** A rich-text editor for the body, which is what every graphical +mail client does. Thunderbird is the reference: forwarding HTML opens a +rich-text composer with the original inside it, editable. + +**This is not a widget swap, and the constraints are why it is L.** + +- **The draft format changes.** A draft currently round-trips as markdown; a + rich-text composer means storing HTML, and a draft written by one and read + by the other loses formatting silently. Items 163 and 165 are already about + draft identity and are worth settling first. +- **`QTextEdit`'s HTML subset is narrow.** It is not a browser: real + newsletter markup (tables, modern CSS) degrades in it. So a naive swap makes + the FIDELITY of a forward worse than what 171 currently sends, while making + the editing better. Measure before committing to it. +- **The formatting toolbar has two masters.** `MarkdownFormat` answers "what + does this button do to a selection" over markdown text. A rich-text editor + has its own notion, and the toolbar must drive whichever is active without + the two disagreeing. +- **Plain text must stay reachable.** Not every message should be HTML, and + the `send_html` config default plus the per-message toggle both already say + so. A rich-text composer that cannot produce clean plain text regresses the + common case. +- **The security work does not go away.** A forwarded original is still input + from a stranger. Whatever renders it for editing must not fetch remote + content, and `HtmlSanitiser` (item 171) is what already answers that. + +**It subsumes item 133**, markdown syntax highlighting: that is a +`QSyntaxHighlighter` over the plain editor, which is the cheap answer to the +same want ("show me what I am writing"). If this is built, 133 is moot; if +this is deferred, 133 is the thing to do instead. Do not build both. diff --git a/docs/superpowers/specs/2026-08-27-forward-html-design.md b/docs/superpowers/specs/2026-08-27-forward-html-design.md new file mode 100644 index 0000000..a71cc54 --- /dev/null +++ b/docs/superpowers/specs/2026-08-27-forward-html-design.md @@ -0,0 +1,181 @@ +# Forwarding an HTML message with its formatting + +Item 171. Design, 2026-08-27. **Read this before the backlog row**, which +records only the cause. + +## The defect + +`ComposeContextBuilder::quoteBody()` reads `ParsedMessage::plainBody` and +nothing else, so the forward path drops the original's `htmlBody` entirely. +Two consequences, and they are not the same severity: + +- An original with both parts forwards its text/plain alternative. The words + survive, the sender's formatting does not. This is what the user reported. +- **An original with an HTML part ONLY has an empty `plainBody`**, so the + forward carries an attribution line and an empty quote. The content is gone + and nothing says so. + +Measured on the developer's own inbox 2026-08-27: **30 of 342 sampled +messages** (~9%) declare `text/html` with no `text/plain` part. The silent +half is not an edge case. + +## What was decided, and by whom + +The user chose, 2026-08-27, from three routes put to them: + +1. **Inline, carrying the original's markup** — CHOSEN. The forward carries + the original's own HTML rather than a flattened quote. Highest inline + fidelity, and the most work, because it is the only route that puts a + stranger's markup into an outgoing message. + + **Amended 2026-08-27, after the first build**: a forward sends ONE part, + not a `multipart/alternative`. The Send-as-HTML toggle chooses which — the + original's markup when on, the text quote when off. The user's reasoning is + that a forward's shape is something they have already decided by flipping + that toggle, and sending both halves hands the choice to the recipient's + client instead. An ordinary (non-forward) message still sends the + alternative as before; only the forward path is single-part. +2. Attach the original as `message/rfc822` (item 130's mechanism). Rejected + here, though item 130 may still build it for its own sake: the user wants + the content inline, not as an attachment. +3. Text fallback only. Rejected: it fixes the silent-loss half and does not + answer the note at all, since formatting is still lost on every forward. + +**Remote content is STRIPPED BY DEFAULT, with a per-forward opt-out**, also +the user's choice against "always strip" and "keep everything". A control in +the composer, checked by default, reading roughly "Strip remote content from +the forwarded message". + +## Why this is the security-critical item in the backlog + +Every other HTML path in this application renders a stranger's 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 protections apply here.** The markup leaves this process and +is rendered by somebody else's mail client, under their policy, on their +machine. The interceptor cannot help: it intercepts requests *we* would make. +So the sanitising has to happen to the bytes, before they are handed to +`MessageSender`, and there is no second line of defence behind it. + +The concrete harm, in the user's own words when the decision was put to them: +forwarding a tracking pixel forwards the tracking. The original sender learns +that the forwarded copy was opened, by whom, and how many times, and the +user's recipient never consented to that. + +## The allow-list rule, which is not negotiable + +`HtmlBuilder::namespaceCids()` is the closest prior art and it is a +**block-list**: it names the attributes that can carry a `cid:` and rewrites +those. It documents scoping `srcset=` out, on the reasoning that its quoting +grammar differs and `cid:` in `srcset` is not seen in the wild. + +**That trade is correct for rewriting and WRONG for stripping**, and the +asymmetry is the whole design: + +| | a missed reference means | +|---|---| +| `namespaceCids` (rewrite) | one broken image | +| this sanitiser (strip) | a tracking beacon reaching the recipient | + +So the sanitiser must **allow-list what may remain**, not block-list what must +go. Anything not recognised is removed. A new HTML attribute, a quoting form +not anticipated, a `srcset`, a CSS `image-set()`, an `@import` — each is +handled by the default, which is removal, rather than by having been +enumerated in advance. + +Stated as the invariant to test against: **after sanitising, no attribute +value and no CSS construct in the output may contain a URL whose scheme is +anything other than `cid:`, and no element that fetches may remain without +one.** + +## What is kept and what goes + +Kept: + +- `cid:` references. They travel inside the message, fetch nothing, and are + what makes an inline logo survive. Item 129 will need the same machinery. +- Structural and presentational markup: tables, lists, headings, spans, + `style=""` attributes with their remote constructs removed. + +Removed: + +- Any `src`, `href`, `background`, `poster`, `srcset`, `data-*` or other + attribute value carrying a non-`cid:` URL scheme. `http:`, `https:`, + `//host/path`, `data:` (which can carry markup), `file:` above all. +- `<link>`, `<script>`, `<iframe>`, `<object>`, `<embed>`, `<base>`, and + `<meta http-equiv="refresh">`. +- CSS `url()`, `@import` and `image-set()` naming anything but a `cid:`, + whether in a `style=""` attribute or a `<style>` block. +- Event-handler attributes (`onload`, `onerror`, …). The recipient's client + most likely disables scripting, but that is their policy and not ours to + assume. + +A stripped `<img>` leaves a gap where the image was. That is the correct +outcome and should not be papered over with a placeholder that itself fetches. + +## Shape of the change + +- **`HtmlSanitiser`, a new namespace of free functions over values** + (`src/htmlsanitiser.h/.cpp`), matching `MarkdownRenderer` and + `MessageBuilder`. No widget, so the security property is testable without a + painter or a web engine. This is where the allow-list lives. +- **`OutgoingMessage` gains the forwarded HTML and the strip flag.** It + currently carries `markdownBody` and `sendHtml` only, so there is nowhere to + put a second HTML source; both are new fields rather than a changed call + site. +- **`MessageBuilder::build()` chooses the part.** On a forward with the toggle + ON, the body is one `text/html` part: the user's rendered markdown, a rule, + then the sanitised original. With the toggle OFF it is one `text/plain` part + carrying the markdown and the text quote. No alternative either way. + + **The toggle is honoured even when the original had no plain-text part**, so + an HTML-only original forwarded with the toggle off goes out as the text + fallback and its formatting is lost. Chosen deliberately 2026-08-27 over + forcing HTML for those messages, so that the toggle means what it says; the + first build forced it and that was reversed. +- **`quoteBody()` is not the fix and must not become it.** It is shared with + Reply, and the behaviour asked for is the forward's alone. It keeps + producing the text quote for the plain half. Its one change is the + silent-loss case: when `plainBody` is empty it should render `htmlBody` down + to text rather than emitting an empty quote, so the plain half is never + blank. +- **The composer control** sits with the existing per-message toggles, checked + by default, and only appears on a Forward carrying HTML. + +## Traps recorded in advance + +- **`quoteBody()` is shared with Reply.** A change there reaches both paths. +- **`QString::arg()` does not collapse `%%`.** CLAUDE.md records the 0.11.0 + placeholder losing its mask, its glow and both gradients this way while + still painting a plausible pane. Any generated CSS here is subject to it. +- **A geometry or rendering probe cannot see this.** The property under test + is "no remote URL survives", which is a property of the STRING. Assert on + the generated bytes. CLAUDE.md's "Rendering probes lie" section is the + general warning; here a probe that renders the result and looks at it would + endorse a tracking pixel it cannot see, because a 1x1 transparent image is + invisible by design. +- **Test with real hostile shapes**, not tidy markup: unquoted attribute + values (`<img src=http://x/p>`), mixed quoting, whitespace and newlines + around `=`, uppercase tags and attributes, a `cid:` whose id contains a + URL-looking substring, and a `style` attribute carrying `url(...)` with no + quotes. `namespaceCids()` documents each of these forms as real. +- **`MimeParser` must parse what we build.** A round trip through + `MessageBuilder` and back is the check that the nesting is right, and it is + cheaper than reading the RFC. + +## Order of work + +1. `HtmlSanitiser` with the allow-list, tested against the hostile shapes + above. Nothing else depends on decisions inside it. +2. `quoteBody()`'s empty-plain fallback, which closes the silent-loss half on + its own and is independently useful. +3. `OutgoingMessage` fields and the `MessageBuilder` nesting, with a + parse-back round trip. +4. The composer control, defaulting to strip. +5. Hand test: forward a real HTML message to yourself with the box checked and + unchecked, and confirm what arrives. + +Steps 1 and 2 are separable and each ship a real improvement, so this does not +have to land as one commit. |