From e3c5773e8f3ada75548a672cd7f2760b55495d53 Mon Sep 17 00:00:00 2001
From: peter <peter@cygnus.systems>
Date: Thu, 3 Sep 2026 10:44:49 +0100
Subject: [PATCH] Fix #014: sanitize themed text kinds + script-blocking CSP
 (stored XSS)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

markdown/deck/html_fragment rendered author-influenced content verbatim,
so <script>/<img onerror>/etc. executed on the Daybook origin — self-XSS
against the owner's session, and (post-sharing) unauthenticated via /s/.

Two layers, both in artifacts/views.py:
- Create-time allowlist sanitization via nh3 (maintained ammonia/Rust
  sanitizer, not deprecated bleach). Keeps formatting/tables/links/images
  (incl. relative /assets URLs) and codehilite markup; drops script, on*
  handlers, style, unknown tags, and javascript:/data: URLs. html_full is
  deliberately NOT sanitized (documented verbatim escape hatch).
- Serve-time CSP (script-src 'none') for the sanitized kinds, on both
  /a/{id} and the unauthenticated /s/{token} route. Being a response
  header, it also neutralizes scripts in artifacts stored before this fix.
  Not applied to html_full or json/csv (json/csv data is already escaped;
  their viewers need their own JS).

Verified live across all kinds; 7 regression tests added. 102 tests pass.

Not addressed (noted in the issue): CSV formula injection,
CSRF_COOKIE_HTTPONLY (moot with XSS closed), nonce CSP for json/csv/full.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 artifacts/views.py                            | 77 +++++++++++++++----
 docs/api.md                                   | 24 +++++-
 ...4-stored-xss-in-rendered-text-artifacts.md | 41 +++++++++-
 requirements.txt                              |  1 +
 skills/daybook-posting/SKILL.md               |  7 ++
 tests/api/test_artifacts.py                   | 65 ++++++++++++++++
 6 files changed, 196 insertions(+), 19 deletions(-)

diff --git a/artifacts/views.py b/artifacts/views.py
index e7650f9..3e47865 100644
--- a/artifacts/views.py
+++ b/artifacts/views.py
@@ -11,6 +11,7 @@ from pathlib import Path
 from urllib.parse import urlencode
 
 import markdown
+import nh3
 import openpyxl
 from django.conf import settings
 from django.contrib import messages
@@ -536,7 +537,18 @@ def serve_artifact_file(request, artifact):
         raise Http404("Artifact file not found")
 
     content_type, _ = mimetypes.guess_type(path.name)
-    return FileResponse(path.open("rb"), content_type=content_type or "application/octet-stream")
+    response = FileResponse(path.open("rb"), content_type=content_type or "application/octet-stream")
+    # The sanitized kinds legitimately contain zero scripts, so a
+    # script-blocking CSP is both safe and strong here — and because it's a
+    # response header it also neutralizes any script in artifacts stored
+    # *before* create-time sanitization existed. Not applied to html_full
+    # (poster's own scripts by contract) or json/csv (their viewers need JS;
+    # their user data is separately escaped).
+    if artifact.kind in SANITIZED_KINDS:
+        response["Content-Security-Policy"] = (
+            "default-src 'self'; script-src 'none'; object-src 'none'; base-uri 'none'"
+        )
+    return response
 
 
 def create_text_artifact(data, request, user):
@@ -553,26 +565,24 @@ def create_text_artifact(data, request, user):
     target_dir.mkdir(parents=True, exist_ok=True)
 
     if kind == Artifact.HTML_FULL:
+        # Deliberately verbatim: html_full is the "poster owns everything,
+        # including risk" kind. Not sanitized. See issue #014.
         rendered = content
     elif kind == Artifact.MARKDOWN:
-        rendered = wrap_html(
-            markdown.markdown(
-                content,
-                extensions=["extra", "sane_lists", "codehilite"],
-                extension_configs={"codehilite": {"guess_lang": False}},
-            ),
-            title,
-            theme,
-            request,
+        md_html = markdown.markdown(
+            content,
+            extensions=["extra", "sane_lists", "codehilite"],
+            extension_configs={"codehilite": {"guess_lang": False}},
         )
+        rendered = wrap_html(sanitize_html(md_html), title, theme, request)
     elif kind == Artifact.DECK:
-        rendered = wrap_deck(content, title, request)
+        rendered = wrap_deck(sanitize_html(content), title, request)
     elif kind == Artifact.JSON:
         rendered = wrap_json_viewer(content, title, request)
     elif kind == Artifact.CSV:
         rendered = wrap_csv_viewer(content, title, request)
-    else:
-        rendered = wrap_html(content, title, theme, request)
+    else:  # html_fragment
+        rendered = wrap_html(sanitize_html(content), title, theme, request)
 
     sidecar_suffix = SIDECAR_SUFFIXES.get(kind)
     if sidecar_suffix:
@@ -728,6 +738,47 @@ def create_binary_artifact(data, request, user):
     return artifact
 
 
+# Allowlist for sanitizing the "themed" text kinds (markdown, deck,
+# html_fragment) — kinds where Daybook, not the poster, owns presentation, so
+# they must never be a script-execution vector (issue #014). html_full is the
+# deliberate exception (verbatim, poster owns everything) and is NOT run
+# through this. The list keeps ordinary formatting + tables + code-highlight
+# markup and drops everything else (script, on* handlers, style, unknown tags).
+SANITIZE_ALLOWED_TAGS = {
+    "a", "abbr", "b", "blockquote", "br", "caption", "code", "col", "colgroup",
+    "dd", "del", "details", "div", "dl", "dt", "em", "figcaption", "figure",
+    "h1", "h2", "h3", "h4", "h5", "h6", "hr", "i", "img", "ins", "kbd", "li",
+    "mark", "ol", "p", "pre", "s", "section", "small", "span", "strong", "sub",
+    "summary", "sup", "table", "tbody", "td", "tfoot", "th", "thead", "tr",
+    "u", "ul",
+}
+SANITIZE_ALLOWED_ATTRS = {
+    "*": {"class", "id", "title"},
+    "a": {"href", "title"},  # nh3 adds rel="noopener noreferrer" itself
+    "img": {"src", "alt", "title", "width", "height"},
+    "td": {"colspan", "rowspan"},
+    "th": {"colspan", "rowspan", "scope"},
+    "col": {"span"},
+    "colgroup": {"span"},
+}
+# Kinds whose stored HTML is sanitized at create time and served with a
+# script-blocking CSP (they legitimately contain no JS of their own).
+SANITIZED_KINDS = {Artifact.MARKDOWN, Artifact.DECK, Artifact.HTML_FRAGMENT}
+
+
+def sanitize_html(body):
+    """Allowlist-sanitize author-influenced HTML. nh3 parses to a DOM and keeps
+    only known-safe nodes (never a blocklist), and re-serializes well-formed
+    HTML as a side effect. javascript:/data: URLs are dropped by restricting
+    url_schemes; relative URLs (e.g. /assets/<id> images) pass through."""
+    return nh3.clean(
+        body,
+        tags=SANITIZE_ALLOWED_TAGS,
+        attributes=SANITIZE_ALLOWED_ATTRS,
+        url_schemes={"http", "https", "mailto"},
+    )
+
+
 def wrap_html(body, title, theme, request):
     stylesheet = request.build_absolute_uri(f"/static/themes/{theme}.css")
     pygments_stylesheet = request.build_absolute_uri("/static/themes/pygments.css")
diff --git a/docs/api.md b/docs/api.md
index 362300e..b412aba 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -28,10 +28,10 @@ Currently supported kinds:
 
 | Kind | Upload type | Rendered behavior | Raw support |
 |---|---|---|---|
-| `html_fragment` | JSON text | Wrapped with a Daybook theme; tables styled automatically | No |
-| `html_full` | JSON text | Stored as complete HTML, verbatim — no theme or styling injected | Yes |
-| `markdown` | JSON text | Rendered to HTML (code blocks syntax-highlighted, tables styled) and original `.md` preserved | Yes |
-| `deck` | JSON text | Wrapped as a simple HTML slide/deck page | No |
+| `html_fragment` | JSON text | Wrapped with a Daybook theme; tables styled automatically; **HTML sanitized** | No |
+| `html_full` | JSON text | Stored as complete HTML, verbatim — no theme, styling, or sanitization | Yes |
+| `markdown` | JSON text | Rendered to HTML (code blocks syntax-highlighted, tables styled), **sanitized**, original `.md` preserved | Yes |
+| `deck` | JSON text | Wrapped as a simple HTML slide/deck page; **HTML sanitized** | No |
 | `image` | Multipart file | Browser image preview | No |
 | `pdf` | Multipart file | Browser PDF preview | No |
 | `binary` | Multipart file | Stored and downloadable | No |
@@ -42,6 +42,22 @@ Currently supported kinds:
 though the value itself is JSON/CSV text — i.e. `{"kind": "json", "content":
 "{\"a\": 1}"}`, not a nested JSON object in the request body.
 
+### HTML sanitization
+
+The "themed" kinds — `markdown`, `html_fragment`, `deck` — have their
+rendered HTML run through an allowlist sanitizer (nh3) before storage.
+Ordinary formatting, tables, links, images (including relative
+`/assets/<id>` URLs), and code-highlight markup are preserved; `<script>`,
+event handlers (`on*`), `<style>`, `javascript:`/`data:` URLs, and unknown
+tags are dropped. These kinds are also served with a `script-src 'none'`
+Content-Security-Policy. As a side effect, sanitized output is always
+well-formed HTML.
+
+`html_full` is the deliberate exception: it is stored and served **exactly**
+as posted, with no sanitization and no restrictive CSP. It is the
+"poster owns all presentation, including any scripts and the security
+implications" escape hatch — only post trusted content as `html_full`.
+
 ## Source Fields
 
 Every artifact can identify who or what produced it. `source` is a free-text label —
diff --git a/issues/014-stored-xss-in-rendered-text-artifacts.md b/issues/014-stored-xss-in-rendered-text-artifacts.md
index 948b349..88edbd9 100644
--- a/issues/014-stored-xss-in-rendered-text-artifacts.md
+++ b/issues/014-stored-xss-in-rendered-text-artifacts.md
@@ -1,6 +1,6 @@
 # 014 — Stored XSS: markdown (and deck/html_fragment) render unsanitized script; no CSP; now reachable unauthenticated via public share links
 
-**Status:** Open
+**Status:** Fixed
 **Severity:** High — stored XSS executing on the Daybook origin, with a
 concrete escalation chain against the owner's session, and (since the
 sharing feature shipped in `91ea19d`) reachable by unauthenticated third
@@ -129,4 +129,41 @@ Two things raise the stakes well past "you attacked your own account":
 
 ## Resolution
 
-_(pending)_
+Fixed with two layers (both in `artifacts/views.py`):
+
+1. **Allowlist sanitization at create time** for the themed kinds
+   (`markdown`, `deck`, `html_fragment`) via `nh3` (the maintained
+   `ammonia`/Rust sanitizer, per the issue's recommendation — not the
+   deprecated `bleach`). `sanitize_html()` keeps ordinary formatting,
+   tables, images, links, and the codehilite/Pygments markup, and drops
+   `<script>`, `on*` handlers, `<style>`, unknown tags, and
+   `javascript:`/`data:` URLs (`url_schemes` limited to http/https/mailto;
+   relative `/assets/<id>` image URLs pass through). Verified live that
+   markdown/deck/html_fragment strip script/onerror/onload/svg while tables,
+   syntax highlighting, and relative images survive. `html_full` is
+   deliberately **not** sanitized — it remains the documented verbatim
+   escape hatch.
+
+2. **Script-blocking CSP at serve time** for the sanitized kinds:
+   `default-src 'self'; script-src 'none'; object-src 'none'; base-uri
+   'none'`, set in `serve_artifact_file` (so it covers both `/a/{id}` and
+   the unauthenticated `/s/{token}` share route). Because it's a response
+   header, it also neutralizes any script in artifacts stored *before* this
+   fix — closing the retroactive gap that create-time sanitization alone
+   would miss. Not applied to `html_full` (its scripts are by contract) or
+   `json`/`csv` (their viewers need JS; their user data is separately
+   escaped via `escape_script_body`).
+
+Regression tests added in `tests/api/test_artifacts.py` (7): script/handler
+stripping for each themed kind, formatting/highlighting/relative-image
+preservation, html_full staying verbatim, and the CSP header being present
+for sanitized kinds and absent for html_full.
+
+**Not addressed here (separate, lower-priority items the issue noted):**
+- CSV formula injection (`=cmd|…` in a cell) — left as documented behavior.
+- `CSRF_COOKIE_HTTPONLY` — moot now that the XSS vector is closed, but a
+  cheap extra hardening if wanted (our JS reads the token from a form input,
+  not the cookie, so enabling it wouldn't break anything).
+- A nonce/hash CSP for the `json`/`csv`/`html_full` pages — those keep their
+  current behavior; the strict CSP is scoped to the no-JS themed kinds where
+  it's provably safe.
diff --git a/requirements.txt b/requirements.txt
index 367b364..4219dbf 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -2,6 +2,7 @@ Django>=5.0,<6.0
 djangorestframework>=3.15,<4.0
 Markdown>=3.6,<4.0
 Pygments>=2.17,<3.0
+nh3>=0.2,<0.3
 openpyxl>=3.1,<4.0
 gunicorn>=23.0,<24.0
 whitenoise>=6.7,<7.0
diff --git a/skills/daybook-posting/SKILL.md b/skills/daybook-posting/SKILL.md
index c48952f..9edbb4d 100644
--- a/skills/daybook-posting/SKILL.md
+++ b/skills/daybook-posting/SKILL.md
@@ -50,6 +50,13 @@ DAYBOOK_SOURCE=agent
 
 For images, PDFs, SVGs, and other files, use `--file`; the helper infers multipart and content type.
 
+**HTML in `markdown`, `html_fragment`, and `deck` is sanitized** — ordinary
+formatting, tables, links, and images are kept, but `<script>`, event
+handlers, `<style>`, and `javascript:`/`data:` URLs are stripped. If you
+need JavaScript (a chart library, an interactive widget), use `html_full`,
+which is served verbatim — that's what it's for. Don't put scripts in the
+themed kinds expecting them to run; they won't.
+
 `--theme` (for `markdown`/`html_fragment` only) accepts `report` (default —
 serif, warm background), `tasks`, or `base` (minimal, no color styling).
 `deck`/`html_full`/`json`/`csv` ignore `--theme` — each has its own fixed
diff --git a/tests/api/test_artifacts.py b/tests/api/test_artifacts.py
index 0608707..65ba529 100644
--- a/tests/api/test_artifacts.py
+++ b/tests/api/test_artifacts.py
@@ -529,3 +529,68 @@ class ArtifactAPITestCase(APITestCase):
         )
         self.assertEqual(response.status_code, 400)
 
+    # -- XSS sanitization of themed kinds (issue #014) -----------------------
+
+    def _stored_html(self, artifact_id):
+        artifact = Artifact.objects.get(pk=artifact_id)
+        return (self.artifacts_root / artifact.path).read_text(encoding="utf-8")
+
+    def test_markdown_strips_script_and_event_handlers(self):
+        aid = self.create_artifact(
+            kind="markdown",
+            content="# Hi\n\n<script>alert(document.cookie)</script>\n\n<img src=x onerror=alert(1)>\n\n[x](javascript:alert(1))",
+        ).data["id"]
+        html = self._stored_html(aid).lower()
+        self.assertNotIn("<script", html)
+        self.assertNotIn("onerror", html)
+        self.assertNotIn("javascript:", html)
+
+    def test_html_fragment_strips_script_and_svg_onload(self):
+        aid = self.create_artifact(
+            kind="html_fragment",
+            content="<p>hi</p><script>alert(1)</script><svg onload=alert(2)></svg>",
+        ).data["id"]
+        html = self._stored_html(aid).lower()
+        self.assertNotIn("<script", html)
+        self.assertNotIn("onload", html)
+        self.assertNotIn("<svg", html)
+
+    def test_deck_strips_script(self):
+        aid = self.create_artifact(
+            kind="deck", content="<section>Slide<script>alert(1)</script></section>"
+        ).data["id"]
+        self.assertNotIn("<script", self._stored_html(aid).lower())
+
+    def test_sanitization_preserves_legitimate_formatting(self):
+        aid = self.create_artifact(
+            kind="markdown",
+            content="# Title\n\n| a | b |\n|---|---|\n| 1 | 2 |\n\n```python\ndef f():\n    return 1\n```\n\n![c](/assets/ab12cd34ef56)",
+        ).data["id"]
+        html = self._stored_html(aid)
+        self.assertIn("<table>", html)
+        self.assertIn('class="codehilite"', html)
+        self.assertIn('<span class="k">def</span>', html)
+        self.assertIn("/assets/ab12cd34ef56", html)  # relative image URL survives
+
+    def test_html_full_is_not_sanitized(self):
+        aid = self.create_artifact(
+            kind="html_full",
+            content="<!doctype html><html><head></head><body><script>ok()</script></body></html>",
+        ).data["id"]
+        # html_full is the deliberate verbatim escape hatch — its script stays.
+        self.assertIn("<script>ok()</script>", self._stored_html(aid))
+
+    def test_sanitized_kinds_served_with_script_blocking_csp(self):
+        aid = self.create_artifact(kind="markdown", content="# hi").data["id"]
+        self.client.force_login(self.user)
+        response = self.client.get(reverse("artifact", args=[aid]))
+        self.assertIn("script-src 'none'", response.headers.get("Content-Security-Policy", ""))
+
+    def test_html_full_served_without_restrictive_csp(self):
+        aid = self.create_artifact(
+            kind="html_full", content="<!doctype html><html><body>x</body></html>"
+        ).data["id"]
+        self.client.force_login(self.user)
+        response = self.client.get(reverse("artifact", args=[aid]))
+        self.assertIsNone(response.headers.get("Content-Security-Policy"))
+