71 lines
10 KiB
Markdown
71 lines
10 KiB
Markdown
# 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/<Type>` 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.
|
|
|
|
- **Amended 2026-10-10 (Phase 16.2 D-10): category content strips too, and admin saves report what was stripped.** The user chose one rule for both: post and category admin writes store the sanitized output and never fail. `CheckHTML`'s 422 contract for categories and the `TestCategoriesAdminContent` 422 assertion are retired in Phase 16.2. Winter stored category content unsanitized; that was a gap, not a contract. When the policy removes anything, the admin save response names what was removed. The policy (and the layer's sanitize-html mirror) is extended only for the Blog Editor's own output, such as the `figure data-embed`/`data-id` embed placeholder.
|
|
|
|
Phase 16.2 plan 03 implements this. The Blog Editor field types decode every locale's value with `classes.SanitizeHTMLReport`, store its output (the submitted bytes when nothing is removed) and return what was removed as admin notices; an admin save of post content writes the same HTML to `content` and `content_html` without goldmark. `newHTMLPolicy` gains exactly four rules for the editor's own output, and nothing else (no iframe, no other `data-*` attribute, no `target`, `rel`, style property or URL scheme). Plan 02's `tests/blogeditor/fixtures/editor-corpus.json` pins them: `TestEditorOutputPassesPolicy` requires every corpus entry to come back from `SanitizeHTML` byte for byte with an empty report, and checks that other providers, malformed ids and other attributes are still stripped. Plan 04 adds the same four rules to the layer's sanitize-html mirror and checks the same corpus. A fifth rule is added only to both policies together.
|
|
|
|
| Change (Phase 16.2) | Why |
|
|
|---|---|
|
|
| `data-embed` matching `^youtube$` on `figure` | The editor's embed placeholder `<figure data-embed="youtube" data-id="..."></figure>`; the layer expands it, no iframe is stored. |
|
|
| `data-id` matching `^[A-Za-z0-9_-]{11}$` on `figure` | The YouTube video id of the same placeholder. |
|
|
| `alt` with any text on `img` | The editor's alt field takes free text; UGC's `Paragraph` pattern drops `:`, `?`, `"` and more. The value is escaped on output and carries no URL. |
|
|
| `start` matching `^[0-9]+$` on `ol` | TipTap's ordered-list input rule writes it when a list does not start at 1. |
|
|
|
|
- **Translations follow the same rule (2026-10-10, audit W-1).** `classes.FormatTranslation` runs on every locale of every API and import write: a non-blank `content` re-derives `content_html` with `FormatHTML` (a supplied `content_html` is replaced, as for the host column); a `content_html` sent alone goes through `classes.SanitizeHTML`, the same policy. A translation keyed on the default locale, which `translate.SetTranslated` writes straight into the host column, is covered too. PHP stored a client-supplied translated `content_html` as-is on the API and import paths; the port follows the PHP admin widget, which always derived it.
|
|
- **Duplicate attributes count as a change (audit W-2).** `golang.org/x/net/html` drops a repeated attribute while tokenizing, so the byte-stable comparison could not see `href="https://x" href="javascript:..."`. `htmlTokens` compares the raw attribute names of each start tag (`rawAttrNames`) with the attributes the tokenizer kept; any difference stores the sanitized output and makes `CheckHTML` refuse.
|
|
|
|
## Rationale
|
|
|
|
The regex blocklist could be bypassed once raw HTML passed through. These payloads came back unchanged with no error: `<img src="x"onerror=...>` (no space before the attribute), `<svg/onload=...>`, `javascript:`, `javascript:`, `javascript:`, `java<TAB>script:`, MathML `xlink:href`, and the inert-looking but dangerous `<base>`, `<meta http-equiv=refresh>` and `<form>`. 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 `<style>` block for the author footer is removed, so the footer loses its flex layout and the round photo. The footer's `itemscope` is re-serialized as `itemscope=""`, and its inline styles lose the trailing `;`. The text, image and microdata stay.
|
|
- All 6 category bodies pass `CheckHTML`.
|
|
- The stored `content_html` does not change until a post is saved again.
|
|
- The README Posts bullet, the `CheckHTML` row and the Dependencies line in `sm-journal-plugin/README.md` describe the sanitizer.
|
|
- `sm-grzybyfunkcjonalne-app` includes the plugin as a submodule (`plugins/golem15/journal`), so it gets the fix when that submodule is bumped. Its workspace then needs bluemonday's `go.sum` lines.
|
|
|
|
## Follow-ups
|
|
|
|
- Done: the post 64 author-footer CSS moved into vue-grzybyfunkcjonalne-app `app/assets/css/base.css` (2c46185).
|
|
- Done: `vue-journal-layer` replaced its regex gate with sanitize-html 2.18.0 mirroring this policy (5beb1c5; decision note `docs/decisions/0001-sanitize-html.md` in that repository, which explains why not DOMPurify).
|
|
- If editors need `target="_blank"` links, `rel`, or `title` values with quotes or `&`, extend the policy deliberately. These are not allowed now because no imported content uses them.
|