Skip to content

🔒 FIX: escape colon_fence class and tasklists label_after content - #165

Merged
chrisjsewell merged 4 commits into
masterfrom
maint/escape-colon-fence-tasklists
Nov 9, 2026
Merged

chrisjsewell merged 4 commits into
masterfrom
maint/escape-colon-fence-tasklists

Conversation

@chrisjsewell

Copy link
Copy Markdown
Member

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 ✅ TEST(#143): add categorical XSS guard across plugin renderers #151) is left for after those PRs land.

The default renderer interpolated the block name into
`class="block-..."` unescaped, after unescaping entities in the info
string, so `:::a"onmouseover="alert(1)` (or its entity-encoded form)
injected attributes and markup even with `html=False` (#143).
With `label=True, label_after=True` the label was 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>` even with
`html=False` (#143). The label text is now HTML-escaped.
A colon_fence block name containing '&' now emits '&amp;' (same attribute
value), and a tasklists label_after source entity is shown as written.
@codecov

codecov Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.30%. Comparing base (d11bdaf) to head (3ef7b13).
⚠️ Report is 19 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
+ Coverage   92.80%   93.30%   +0.49%     
==========================================
  Files          31       40       +9     
  Lines        1835     2285     +450     
==========================================
+ Hits         1703     2132     +429     
- Misses        132      153      +21     
Flag Coverage Δ
pytests 93.30% <100.00%> (+0.49%) ⬆️

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.

@chrisjsewell
chrisjsewell merged commit d399df5 into master Nov 9, 2026
11 checks passed
@chrisjsewell
chrisjsewell deleted the maint/escape-colon-fence-tasklists branch November 9, 2026 18:52
chrisjsewell added a commit that referenced this pull request Nov 9, 2026
- Build markdown-it with `html=False`: the `commonmark` preset allows raw
  HTML, so the docstring's claim that it is disabled was wrong.
- Each case states text the output must contain, so a payload the plugin
  does not parse cannot pass without testing anything.
- Add quote-only attribute breakout probes (`a" onxss="1`) for the
  dollarmath label, the myst target and the colon_fence class, which the
  `"><xss>` probe alone does not catch.
- Add colon_fence block name and tasklists `label_after` cases (#165),
  and a `javascript:` URL in an attrs `href`.
- The texmath equation number case passed without its fix, because
  texmath never renders equation numbers (its render rules close over the
  loop variable, B023). It is now its own strict xfail, so fixing that
  bug turns it red and the marker is removed.
- `style` is kept by the attrs default, so its case is gone.
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.

Reporting security vulnerabilities

1 participant