golem15/summercms
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(16.1): decide bluemonday for journal HTML sanitizing

Browse Source
This commit is contained in:
Jakub Zych
2026-10-09 23:35:17 +02:00
parent cd076b881d
commit 5be98a0c83
1 changed files with 56 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

56
.planning/notes/journal-html-sanitizing-bluemonday.md Normal file
View File

@@ -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/<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 `&#34;` and `'` as `&#39;`, 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: `<img src="x"onerror=...>` (no space before the attribute), `<svg/onload=...>`, `javascript&#58;`, `javascript&colon;`, `&#106;avascript:`, `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
- Move the post 64 author-footer CSS (`.author-footer`, `.author-info`, `.author-photo`, `.author-details`) into the public theme before the post is re-saved.
- `vue-journal-layer/app/utils/htmlGate.ts` still uses the same three regexes, so it has the same bypasses and the same `data:` false positive. It gives no defence in depth. Replace it with DOMPurify (with a decision note) in that repository.
- 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.