Repository navigation
✅ TEST(#143): add categorical XSS guard across plugin renderers - #151
Merged
chrisjsewell merged 5 commits intoNov 9, 2026
Conversation
…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
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
force-pushed
the
kyle/xss-and-attrs-fixes-143
branch
from
July 21, 2026 16:56
e1e501a to
c977b60
Compare
KyleKing
commented
Jul 21, 2026
|
|
||
| def render(tex: str, displayMode: bool, macros: Any) -> str: | ||
| return tex | ||
| return escapeHtml(tex) |
Contributor
Author
There was a problem hiding this comment.
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
added a commit
that referenced
this pull request
Nov 9, 2026
## 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"><xss>` 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 `&` (`class="block-a&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 & 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 `&` now displays as the literal text `&`. 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
merged commit Nov 9, 2026
c977b60
into
executablebooks:claude/practical-goldberg-xaa9j6
10 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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