Archive resolved issues; add open issues #014-#016
Moves the 11 Fixed issues (#001-006, #008-012) into issues/_archive_/, which is gitignored — they stay on local disk and in git history but drop out of the committed tree, keeping the issues/ dir focused on what's open. The sibling test project reads this dir by local path, so it still sees the archived files. Adds the three new open issues found in round-3 testing: #014 (stored XSS in markdown/deck/html_fragment — High), #015 (RecursionError -> 500 on deeply nested JSON), #016 (ragged CSV rows lose data in the rendered table). Documents the archive convention in the README. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
292c386209
commit
6d2fe2147c
15 changed files with 319 additions and 848 deletions
132
issues/014-stored-xss-in-rendered-text-artifacts.md
Normal file
132
issues/014-stored-xss-in-rendered-text-artifacts.md
Normal file
|
|
@ -0,0 +1,132 @@
|
|||
# 014 — Stored XSS: markdown (and deck/html_fragment) render unsanitized script; no CSP; now reachable unauthenticated via public share links
|
||||
|
||||
**Status:** Open
|
||||
**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
|
||||
parties.
|
||||
|
||||
Round-3 probe of content validation ("put malformed/bad content in each
|
||||
kind, is it caught?"). Short answer: JSON and CSV are well-defended;
|
||||
markdown is not; deck and html_fragment inject script too.
|
||||
|
||||
## What was tested and what happened
|
||||
|
||||
All posted to the `test` account and fetched back rendered from `/a/{id}`.
|
||||
|
||||
| Kind | Injected | Result |
|
||||
|---|---|---|
|
||||
| `markdown` | `<script>…</script>`, `<img src=x onerror=…>`, `<a href="javascript:…">` | **All pass through verbatim into rendered HTML — executes.** |
|
||||
| `deck` | `<section>…<script>alert(1)</script>` | **Script passes through into the themed deck page — executes.** |
|
||||
| `html_fragment` | `<p>hi</p><script>…</script><svg onload=…>` | **Script/`svg onload` pass through into the themed page — executes.** (The #004 content-shape check blocks `<html>/<body>/<!doctype>` but never `<script>`.) |
|
||||
| `html_full` | broken/garbage doc with `<script>` | Stored verbatim. **By design** — documented as "you own all presentation." Not claimed as a bug here. |
|
||||
| `json` | `{"note":"</SCRIPT><img src=x onerror=…>"}` | **Defended.** `escape_script_body` (`re.compile(r"</script", re.IGNORECASE)`) turns it into `<\/script` inside the `<script type="application/json">` data block; renderjson shows it as inert text. Case-variation bypass (`</SCRIPT>`) correctly caught. |
|
||||
| `csv` | `</script>` in a cell | **Defended.** Same escaping via `json.dumps` + `escape_script_body`. |
|
||||
| `csv` | `=cmd|/c calc`, `+SUM(...)` as cell values | Passes through into the `?raw=1` sidecar verbatim (classic CSV/formula injection). Low severity / debatable — see below. |
|
||||
|
||||
## Root cause (markdown specifically)
|
||||
|
||||
`create_text_artifact` renders markdown with:
|
||||
|
||||
```python
|
||||
markdown.markdown(content, extensions=["extra", "sane_lists", "codehilite"], ...)
|
||||
```
|
||||
|
||||
Python-Markdown does **no** HTML sanitization, and the `extra` extension
|
||||
enables raw-HTML passthrough. Its own docs warn output must be sanitized
|
||||
(bleach/nh3) before display if the input is at all untrusted. The rendered
|
||||
HTML is then written to the artifact's `.html` file and served as-is.
|
||||
|
||||
Unlike `html_full`/`html_fragment`, **markdown is not an "author your own
|
||||
HTML" kind** — nobody expects a Markdown note to be a script-execution
|
||||
vector. In an agent-driven system this is exactly the kind most likely to
|
||||
carry *indirectly* untrusted content: an agent that summarizes a scraped
|
||||
page, an email, or an ingested document can emit a summary containing an
|
||||
attacker-controlled `<script>`/`<img onerror>` that ends up verbatim in the
|
||||
posted markdown.
|
||||
|
||||
## Why this is High, not self-XSS
|
||||
|
||||
Two things raise the stakes well past "you attacked your own account":
|
||||
|
||||
1. **No `Content-Security-Policy` header.** Rendered artifact responses
|
||||
carry only `X-Frame-Options: SAMEORIGIN` and `X-Content-Type-Options:
|
||||
nosniff` — nothing stops inline script execution. (A CSP was probably
|
||||
avoided because the json/csv/deck viewers themselves use inline
|
||||
scripts; a nonce-based CSP that tags Daybook's own inline scripts but
|
||||
not user content would fix both this and the by-design kinds at once.)
|
||||
|
||||
2. **Escalation chain against the owner's own session.** When the owner
|
||||
views the malicious artifact at `/a/{id}` (their normal feed preview),
|
||||
the script runs same-origin with the owner's session cookie. Reads are
|
||||
session-authed, so it can `GET /api/artifacts` and exfiltrate every
|
||||
private artifact's metadata. Worse, the `csrftoken` cookie is not
|
||||
HttpOnly, so injected JS can read it and forge session+CSRF POSTs — e.g.
|
||||
hit `POST /a/{id}/share` for every artifact, minting public share links
|
||||
for the owner's entire private feed and exfiltrating them. (Bearer-token
|
||||
writes like PATCH/DELETE are safe — the token isn't in the browser — but
|
||||
share-toggle, mark-read, and keep are session+CSRF and thus reachable.)
|
||||
|
||||
3. **Unauthenticated blast radius via sharing (`91ea19d`).** A shared
|
||||
artifact at `/s/{token}` serves this same rendered HTML with no auth, so
|
||||
a malicious/compromised artifact runs arbitrary JS in the browser of
|
||||
anyone the owner sends the link to — a drive-by/phishing vector on the
|
||||
Daybook origin, not just a self-inflicted one.
|
||||
|
||||
## Suggested Fix
|
||||
|
||||
- **Sanitize rendered output for the non-authoring kinds** — at minimum
|
||||
`markdown`, and I'd argue `deck` too (a "slide sections" kind isn't
|
||||
obviously an arbitrary-script host). Run the generated HTML through an
|
||||
**allowlist** sanitizer that keeps normal formatting (headings, lists,
|
||||
tables, code, images, links) but drops everything else — `<script>`,
|
||||
`on*` handlers, `javascript:`/`data:` URLs. Pygments/codehilite output is
|
||||
a fixed structure, so an allowlist that permits `<pre>/<code>/<span>` plus
|
||||
the highlight `class` names preserves highlighting.
|
||||
|
||||
Two things to get right, both confirmed against current (2026) tooling:
|
||||
|
||||
1. **Use `nh3`, not `bleach`.** `bleach` — the library most people reach
|
||||
for first — is **deprecated and no longer receives security fixes**
|
||||
(it was built on the unmaintained `html5lib`). `nh3` is the Python
|
||||
binding to Rust's `ammonia` sanitizer: allowlist-based, actively
|
||||
maintained, ~20× faster, and single-purpose. It exposes exactly the
|
||||
knobs needed here (`tags`, `attributes`, `allowed_classes`,
|
||||
`url_schemes`).
|
||||
2. **Allowlist, never blocklist.** Do *not* try to fix this by detecting
|
||||
or stripping "bad" patterns (`<script>`, known `on*` handlers, etc.) —
|
||||
that approach loses to case tricks, encoding, novel event handlers,
|
||||
and SVG/MathML vectors, and is the historical source of XSS filter
|
||||
bypasses. Parse-to-DOM + keep-only-known-safe (what `nh3`/`ammonia`
|
||||
does) is the only robust model. Validity checking is a *separate*
|
||||
concern and gives no security benefit — a perfectly valid document can
|
||||
host a malicious `<script>` — so it must not be mistaken for the fix
|
||||
here (it has a legitimate but lesser role as poster-facing "your
|
||||
html_fragment is malformed" UX feedback, tracked separately if wanted).
|
||||
- **Decide `html_fragment` explicitly.** It's marketed as themed
|
||||
"reports/cards/tables," not full pages, and it's shareable — either
|
||||
sanitize it like markdown, or if script is intended, document that
|
||||
`html_fragment` executes arbitrary JS so posters treat it like
|
||||
`html_full` trust-wise. Right now it's an in-between with neither
|
||||
sanitization nor a warning.
|
||||
- **Add a `Content-Security-Policy`** to artifact/shared views. A
|
||||
nonce-based policy (nonce on Daybook's own renderjson/gridjs/deck inline
|
||||
scripts, `script-src 'self' 'nonce-…'`) neuters injected inline scripts
|
||||
even in the HTML-authoring kinds, and is the single highest-leverage
|
||||
mitigation since it covers every kind at once.
|
||||
- **CSV formula injection** (low/debatable): consider prefixing cells that
|
||||
begin with `= + - @` with a `'` in the raw sidecar, or just document it.
|
||||
Many tools treat this as the spreadsheet app's problem; noting it only
|
||||
for completeness, not urging a fix.
|
||||
|
||||
## Notes
|
||||
|
||||
- html_full is intentionally verbatim and is **not** being reported as a
|
||||
bug — only flagged so the fix doesn't accidentally start sanitizing it
|
||||
and break its documented contract.
|
||||
- json/csv defenses are genuinely solid; this issue is not about them
|
||||
beyond recording that they were tested and held.
|
||||
|
||||
## Resolution
|
||||
|
||||
_(pending)_
|
||||
Loading…
Add table
Add a link
Reference in a new issue