Skip to content

🔒 FIX: escape user content in renderers and harden attrs default#150

Open
chrisjsewell wants to merge 1 commit into
masterfrom
claude/practical-goldberg-xaa9j6
Open

🔒 FIX: escape user content in renderers and harden attrs default#150
chrisjsewell wants to merge 1 commit into
masterfrom
claude/practical-goldberg-xaa9j6

Conversation

@chrisjsewell

@chrisjsewell chrisjsewell commented Jul 21, 2026

Copy link
Copy Markdown
Member

Addresses the XSS advisories reported against default configurations in #143 (thanks to @KrisKennawayDD / Datadog for the detailed write-ups).

Each fix was reproduced against current master before and after, and covered by a regression test.

Advisories addressed

# Advisory Fix
1 XSS via unescaped math content in texmath HTML-escape math content (and the equation-number label) in the default renderer, consistent with dollarmath/amsmath
2 XSS via unescaped equation label in dollarmath Escape the label in the generated id attribute and permalink href
3 XSS via unescaped target label in myst_block Escape the target label (name)= in the rendered anchor
5 attrs/attrs_block allow arbitrary event handlers by default When no allowed list is given, strip event-handler (on*) and style attributes by default

Advisory 4 (quadratic ReDoS in admon) is not included here — it's already handled in #148.

Notes on the attrs change (#5)

This is the one behavioural change, so calling it out:

  • With no explicit allowed list, on* and style attributes are now removed by default (moved into token.meta["insecure_attrs"], same mechanism as the existing allow-list). Benign attributes (width, data-*, class, …) are unaffected.
  • I chose a denylist baseline rather than a secure-by-default allow-list (("id", "class")) to avoid breaking the plugin's core "add arbitrary attributes" feature for the common trusted-content case. This is a baseline protection, not a full sanitiser — it won't stop e.g. href="javascript:"; untrusted input should still pass an explicit allowed list. Documented in both docstrings and the changelog. Happy to switch to the stricter allow-list default if preferred.
  • While fixing this I found that the span path ([text]{...}) assigned token.attrs directly and bypassed _add_attrs entirely — so it ignored the allowed allow-list too, not just the new denylist. That path (the advisory's own PoC, [click me]{onclick=...}) now routes through the filter, so an explicit allowed list finally applies to spans as well.

Tests & fixtures

  • Added regression tests for all four fixes (test_texmath, test_dollarmath, test_myst_block, test_attrs).
  • Regenerated three fixtures (texmath_dollar.md, texmath_bracket.md, myst_block.md) that previously encoded the unescaped output; only the expected blocks changed, inputs untouched. Notably texmath now matches what dollarmath already did for the same $1+1<3$ input.
  • Full suite passes (525), ruff, ruff format, and mypy --strict all clean.

Addresses the XSS advisories reported against default configurations in #143:

- texmath: HTML-escape math content and equation-number labels in the default
  renderer (consistent with dollarmath/amsmath). Input such as
  `$<img src=x onerror=alert(1)>$` previously emitted live markup.
- dollarmath: escape equation labels in the generated `id` attribute and the
  permalink `href`.
- myst_block: escape target labels `(name)=` in the rendered anchor.
- attrs/attrs_block: when no `allowed` list is given, strip event-handler
  (`on*`) and `style` attributes by default (kept in meta `insecure_attrs`).
  Text spans `[text]{...}` now also honour the `allowed` list, which they
  previously bypassed. This is a baseline protection, not a full sanitiser.

The admonition ReDoS (advisory 4) is addressed separately in #148.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DS3iVi5q1TepHaMymmmeJy
@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.01%. Comparing base (d11bdaf) to head (4249d11).
⚠️ Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   92.80%   93.01%   +0.20%     
==========================================
  Files          31       40       +9     
  Lines        1835     2290     +455     
==========================================
+ Hits         1703     2130     +427     
- Misses        132      160      +28     
Flag Coverage Δ
pytests 93.01% <100.00%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants