Fix #014: sanitize themed text kinds + script-blocking CSP (stored XSS)
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>
This commit is contained in:
parent
6d2fe2147c
commit
e3c5773e8f
6 changed files with 196 additions and 19 deletions
|
|
@ -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")
|
||||
|
|
|
|||
24
docs/api.md
24
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 —
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
).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"))
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue