Skip to content

✅ TEST(#143): add categorical XSS guard across plugin renderers - #151

Merged
chrisjsewell merged 5 commits into
executablebooks:claude/practical-goldberg-xaa9j6from
KyleKing:kyle/xss-and-attrs-fixes-143
Nov 9, 2026
Merged

✅ TEST(#143): add categorical XSS guard across plugin renderers#151
chrisjsewell merged 5 commits into
executablebooks:claude/practical-goldberg-xaa9j6from
KyleKing:kyle/xss-and-attrs-fixes-143

Conversation

@KyleKing

@KyleKing KyleKing commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

I started working on a fix locally, but it is the same net result with one addition a of categorical check to complement the per-plugin regression tests. The implementation follows the same pattern for test_redos in my other PR to proactively surface these types of security issues to PR authors rather than in code review

KyleKing and others added 2 commits July 21, 2026 09:02
@KyleKing @claude
🐛 FIX: escape default renderers and harden attrs against XSS (executa…
046fc16 
…blebooks#143)

Resolve the XSS advisories from executablebooks#143 where plugins interpolated
attacker-controlled markdown into their default HTML render rules without
escaping:

- texmath: escape the math source in the default render() stub and the
  math_block_eqno equation number, across every delimiter flavor
- dollarmath: escape the equation label in the id/href attributes (the
  content was already escaped)
- myst_block: escape the target label in the <a> body and href

Harden attrs/attrs_block: with no explicit allowed list, divert event-handler
(on*) and style attributes to token.meta["insecure_attrs"] instead of emitting
them, so the default configuration cannot inject script. The inline span path
previously set token.attrs directly and bypassed the filter; route it through
_add_attrs with the allowed set threaded in. Other attribute names are
unchanged.

Update the texmath/myst fixtures that encoded the old unescaped output, and add
a categorical guard (tests/test_xss.py) that renders injection probes through
each plugin to catch the next unescaped interpolation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L4mwfRr7ZkjiwPATm6Q4ue
@KyleKing
test(143): add categorical XSS guard across plugin renderers
811e7e0
@KyleKing

KyleKing commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

FYI: @chrisjsewell, feel free to consider merging this change into the base PR (#150) to fully address all five findings and proactively catch them going forward

@KyleKing
KyleKing force-pushed the kyle/xss-and-attrs-fixes-143 branch from e1e501a to c977b60 Compare July 21, 2026 16:56
KyleKing
KyleKing commented Jul 21, 2026
View reviewed changes
Comment thread mdit_py_plugins/texmath/index.py

def render(tex: str, displayMode: bool, macros: Any) -> str:
return tex
return escapeHtml(tex)

@KyleKing KyleKing Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the rendering logic below is implemented, we want the escaping to be owned here because doing so around render would break the valid HTML and reasonable to be owned by render

@chrisjsewell chrisjsewell mentioned this pull request Nov 9, 2026
Merged
chrisjsewell added a commit that referenced this pull request Nov 9, 2026
@chrisjsewell
🔒 FIX: escape colon_fence class and tasklists label_after content (#165)
d399df5 
## What and why

Two more HTML-injection points in the #143 family, both reachable with
`html=False`:

- **colon_fence** (default renderer): the block name was interpolated
into `class="block-..."` unescaped,
  after entities in the info string had been unescaped.
`:::a"onmouseover="alert(1)` rendered
`class="block-a"onmouseover="alert(1)"`,
  and `:::a&quot;&gt;&lt;xss&gt;` rendered a raw `<xss>` element.
  The name is now passed through `escapeHtml`.
- **tasklists** with `label=True, label_after=True`: the label is built
from the item's source text
and emitted as an `html_inline` token, so `- [ ] <img src=x
onerror=alert(1)>` rendered a raw `<img>`.
  The label text is now passed through `escapeHtml`.

## How it was tested

- New regression tests, both with `html=False` and exact expected
output:
`tests/test_colon_fence.py::test_block_name_is_escaped` (raw-quote and
entity-encoded payloads, plus a benign `&`)
and `tests/test_tasklists.py::test_label_after_is_escaped` (raw HTML,
`&`, plain text),
with `test_label_after_entity_is_shown_literally` pinning how a source
entity appears in the label
(it asserts the label only, as earlier inline children of a
`label_after` item are also rendered outside it,
  an existing quirk not changed here).
- Every case except the plain-text one fails on master and passes with
its fix;
  reverting either source file at HEAD makes its tests fail again.
- `pytest -q`: 518 passed (511 on master plus the 7 new cases),
markdown-it-py 4.2.0, Python 3.12.
- `pre-commit run --all-files` (ruff, ruff-format, mypy): clean.
- No fixture file changed; all existing colon_fence and tasklists
fixtures render byte-identical.

Part of #143

This does not close #143. What remains of it is in other open PRs:
the texmath, dollarmath and myst_block escaping and the `attrs` default
(#150),
the ReDoS fixes (#148),
and a test that guards every plugin renderer against XSS (#151).

## Notes for the reviewer

- Output changes for benign input:
a colon_fence block name containing `&` now emits `&amp;`
(`class="block-a&amp;b"` instead of `class="block-a&b"`),
  which is the same attribute value;
and in tasklists `label_after` mode, label text containing `&`, `<`, `>`
or `"`
  is now escaped (`a & b` becomes `a &amp; b`, which is correct HTML).
The label is the item's raw source text (it already showed `*emphasis*`
literally),
so a source entity such as `&amp;` now displays as the literal text
`&amp;`.
This applies with `html=True` as well; on master, inline HTML in a
`label_after` item already produced broken markup.
  No existing fixture uses `label_after`.
- Adding these two plugins to `tests/test_xss.py` (which arrives with
#151) is left for after those PRs land.
@chrisjsewell
chrisjsewell merged commit c977b60 into executablebooks:claude/practical-goldberg-xaa9j6 Nov 9, 2026
10 checks passed
@chrisjsewell chrisjsewell mentioned this pull request Nov 10, 2026
Merged
@chrisjsewell

chrisjsewell commented Nov 10, 2026

Copy link
Copy Markdown
Member

Thanks @KyleKing! This was merged into #150's branch, and #150 has now landed in 00bb6ec with you as co-author of the squash commit. On top of your tests we ran the guard with html=False (the commonmark preset allows raw HTML), added a check per case that the probe actually reached the plugin's renderer, added quote-only attribute probes, and added the colon_fence and tasklists cases from #165. The texmath equation-number case turned out to pass without the fix, because texmath never renders equation numbers (the B023 late-binding closure), so it is a strict xfail that turns red once that is fixed.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@KyleKing @chrisjsewell