diff --git a/.planning/notes/journal-html-sanitizing-bluemonday.md b/.planning/notes/journal-html-sanitizing-bluemonday.md new file mode 100644 index 0000000..9d1fdc4 --- /dev/null +++ b/.planning/notes/journal-html-sanitizing-bluemonday.md @@ -0,0 +1,56 @@ +# Decision: bluemonday sanitizes journal post HTML + +**Date:** 2026-10-09 +**Status:** Accepted (fixes Phase 16.1 code review CR-01 and WR-01) +**Supersedes:** the three-regex `rejectUnsafe` gate in `sm-journal-plugin/classes/format_html.go` and the Phase 16.1 threat-register claim T-16.1-02 + +## Decision + +`sm-journal-plugin` adds `github.com/microcosm-cc/bluemonday` v1.0.27 (the latest tag; the repository is not archived and has had no release since July 2024). `classes.FormatHTML` renders markdown with goldmark `html.WithUnsafe()` as before, then sanitizes the result with a package-level bluemonday policy. The plugin also imports `golang.org/x/net/html` directly. It was already in the build through bluemonday and the framework. + +The policy is `bluemonday.UGCPolicy()` with these changes, each taken from goldmark's own output or from the posts and categories on the UAT database: + +| Change | Why | +|---|---| +| `RequireNoFollowOnLinks(false)` | Winter does not add `rel="nofollow"`, so stored links keep their form. | +| `footer` element | An imported author footer (post 64). | +| `class` on every element, matching `SpaceSeparatedTokens` | Imported footers and goldmark's `{.class}` attributes and footnote classes. | +| `itemscope` (empty), `itemtype` (`https://schema.org/` only), `itemprop` | schema.org microdata in the imported author footer. | +| Styles `color`, `font-size`, `font-weight`, `text-align` | Inline styles in the imported author footer, and goldmark's table cell alignment. bluemonday validates each value. | +| `role` = `doc-noteref`, `doc-endnotes` or `doc-backlink` on `a` and `div` | goldmark's footnote markup. | +| `dir` = `ltr`, `rtl` or `auto` | Pasted chat transcripts in category bodies use `dir="auto"`. UGC's `Direction` allows only `ltr` and `rtl`. | +| `data-message-author-role`, `data-message-id` on `div` (`[A-Za-z0-9_-]+`) | The same pasted transcripts. This is not `AllowDataAttributes()`, which would allow every `data-*` attribute. | + +Everything else that UGC refuses is refused: `script`, `style`, `iframe`, `object`, `embed`, `form`, `input`, `base`, `meta`, `link`, `svg`, `math`, comments, every `on*` attribute, `target`, `rel`, `title` values outside UGC's `Paragraph` pattern, and URLs other than `http`, `https`, `mailto` and relative URLs. bluemonday checks URLs after the tokenizer has decoded entities, and it requires them to parse with `net/url`. + +## Strip vs refuse + +- **Posts strip.** `FormatHTML` stores the sanitized output and never fails because of unsafe markup. This is parity: Winter's `Post::formatHtml` runs `Html::clean` for every writer without `backend.allow_unsafe_markdown` (a CLI import has no backend user, so it is always cleaned). `Html::clean` strips `on*` and `xmlns` attributes, `javascript:`, `vbscript:` and `data:` URLs, `style` attributes, and the `script`, `style`, `iframe`, `embed`, `object`, `meta`, `base` and `link` tags. It never refuses. The Go port sanitizes for every writer. There is no permission that skips it, because superusers in PHP hold `backend.allow_unsafe_markdown` implicitly, and that loophole is exactly the escalation CR-01 describes. No post test or API contract required a 422 for unsafe content. Only the old `classes` unit test did, and it now asserts stripping. `content` itself is still stored as submitted, as in PHP. +- **Byte-stable when nothing is stripped.** `sanitizeHTML` returns the goldmark output unchanged when that output and bluemonday's output tokenize to the same tokens: the same type, tag name, decoded text and set of decoded attributes. Otherwise it returns bluemonday's output. bluemonday re-escapes `"` as `"` and `'` as `'`, so storing its output every time would rewrite every imported post on its next save. Returning the original bytes is safe only on that token-level equality. Any element, attribute or comment that the policy drops makes the tokens differ, and then the sanitized output is stored. +- **Category content refuses.** `classes.CheckHTML` keeps its exported signature and its 422 contract. D-21 stores category content exactly as submitted, and `TestCategoriesAdminContent` asserts a 422 for unsafe content. CheckHTML now uses the same policy and comparison, and refuses whenever the policy would change the HTML. + +## Rationale + +The regex blocklist could be bypassed once raw HTML passed through. These payloads came back unchanged with no error: `` (no space before the attribute), ``, `javascript:`, `javascript:`, `javascript:`, `javascript:`, MathML `xlink:href`, and the inert-looking but dangerous ``, `` and `
`. The `data:` pattern also matched ordinary text, so "Data: 12 maja 2024" and "metadata:" were refused (WR-01). A sanitizer that tokenizes the HTML checks attributes and URL values in the same way a browser parses them, so it does not depend on any text pattern. + +## Alternatives considered + +- **A hand-built allow-list gate on `golang.org/x/net/html`.** It would add no new module, but the element and attribute allow-lists, the URL normalization (entity decoding, stripping ASCII whitespace and control characters, scheme checks) and the CSS value checks would be new security code that this project would own. bluemonday is the maintained Go implementation of exactly that, with its own test corpus. The project prefers a maintained library for protocol and security code. +- **Anchoring the regexes to attribute values** (the WR-01 stop-gap). It fixes the false refusals but not the bypasses. +- **Refusing posts with a 422 instead of stripping.** That is not PHP parity. It would also turn every pasted editor fragment that contains a stray `style` element or attribute into a failed save or import. + +## Consequences + +- UAT impact scan, read-only, run on 2026-10-09 over 104 posts, 2 `en` translations and 6 category bodies: + - **No stored post or category holds an XSS payload.** + - Only post 64 changes when it is next re-saved. Its 24-line `