Skip to content

markdown: escape raw HTML inside an image's alt attribute - #46249

Open
robobun wants to merge 1 commit into
mainfrom
robobun/31529171/markdown-escape-html-in-alt
Open

robobun wants to merge 1 commit into
mainfrom
robobun/31529171/markdown-escape-html-in-alt

Conversation

@robobun

@robobun robobun commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • Bun.markdown.html() and .md imports write raw HTML inside an image description verbatim into alt. ![x <a href="u">y</a> z](i.png) gives alt="x <a href="u">y</a> z". The first " of the tag ends alt, and y z" /> shows as text.
  • The TextType::Html arm of HtmlRenderer::text (src/md/html_renderer.rs:373) ignores in_image. No issue reports this.

Fix

  • The arm escapes the chunk while an image is open: alt="x &lt;a href=&quot;u&quot;&gt;y&lt;/a&gt; z", as GitHub prints it. The check runs before the tag filter.
  • Correct because text() is the only writer between the quotes of alt. md4c, the origin of this renderer, made the same change in md_html: escape raw HTML in image alt attribute mity/md4c#376.
  • Verified: test/js/bun/md/md-edge-cases.test.ts (20 new tests, all fail without the fix), and the other files in test/js/bun/md/.
  • Self-reviewed: 18 concerns raised, 16 addressed. The other two are in Notes.

Background

  • The parser reports text to a renderer as typed events. TextType::Html is one chunk of raw inline HTML, for example a tag.
  • An image description becomes the alt value. image_nesting_level counts the open images.
  • Considered a retype of the event in the parser: it moves heading ids. Considered an alt path at the top of text(): every text event pays a branch.

Downsides

  • The page changes for a tag with " in an image description. ![<img src="a.jpg" width="100" />](a.jpg) no longer puts width on the image.
  • This is not a sanitizer. html() still passes raw HTML such as <img onerror>.
  • Cost: 1 more conditional jump per raw HTML event outside an image, 0 for other text. Text segment: -256 B.
Notes

Origin. There is no GitHub issue and no user report. A read of HtmlRenderer::text found it. The write is in the code since Bun.markdown was added (#26440, first release v1.3.8). md4c fixed the same arm later (mity/md4c@65c6c9d, in v0.6.0).

What changes. Only the output of html(), of .md imports and of bundled .md, and only for raw inline HTML inside an image description: a tag, a comment, a processing instruction, a CDATA section or a declaration. All three use the one HtmlRenderer.

  • A tag with a ": before, alt ends at the first ", the rest of the tag becomes attributes of the <img>, and the tail shows as text. For ![<img src="a.jpg" width="100" />](a.jpg) an HTML parser reads alt="<img src=", a junk attribute a.jpg", width="100", and the text " />. Now the whole tag is the value of alt.
  • A tag without a " (![a <b>x</b> c](i.png)): only the bytes change. The parsed alt is a <b>x</b> c before and after.
  • With tagFilter: true the result is the same as without it. Before, the filter escaped only the < of its nine tag names, so ![<script title="x" onerror="alert(1)">](i.png) gave alt="&lt;script title="x" onerror="alert(1)">".
  • The output is now what noHtmlSpans: true already printed for the same input.

What does not change.

  • Raw HTML outside an image, with and without tagFilter.
  • The id of a heading. HeadingIdTracker::track_text still skips the tag.
  • Every value that react(), render() and ansi() receive, and the events of the parser. The parser still reports TextType::Html inside an image description, as md4c does.
  • html() is not a sanitizer. <img src=x onerror="alert(1)"> passes through with default options and with tagFilter, and [x](javascript:alert(1)) becomes a link, before and after.

References, for ![x <a href="u">y</a> z](i.png).

  • GitHub (POST /markdown, mode gfm): alt="x &lt;a href=&quot;u&quot;&gt;y&lt;/a&gt; z". The same bytes as this PR.
  • cmark, read from source (src/html.c, the state->plain != NULL branch): CMARK_NODE_HTML_INLINE goes through escape_html.
  • md4c (md_html: escape raw HTML in image alt attribute mity/md4c#376): MD_TEXT_HTML goes through render_html_escaped while image_nesting_level is above 0. md4c's escaper also writes ' as &#x27; (mity/md4c@16a8df7). Bun escapes & < > ", as cmark does, so ![a <i c='d'> b](i.png) gives alt="a &lt;i c='d'&gt; b". One test row pins that.
  • Run from npm: markdown-it 15.0.3, marked 18.1.1 and micromark 4.0.3 (default options) print the escaped text. commonmark.js 0.31.2 and micromark with allowDangerousHtml print the tag verbatim, as Bun did.

Left alone on purpose.

  • The fallback of write_entity (self.write(entity_text)) still writes raw. No input reaches it: find_entity accepts a numeric reference, which parse_entity_codepoint always decodes, or a name that entity::lookup has. An entity also holds no ".
  • test/js/bun/md/regressions.txt. md4c's example for this fix is not copied. test/cli/lint/conformance/markdown-trees.zst records a tree for every example of that directory, and test/cli/lint/js-parsers.test.ts pins the count. The rows in md-edge-cases.test.ts pin the behaviour.
  • Self-review concern not done: a licence notice for the md4c fixture files in test/js/bun/md/. This PR copies no md4c text. The notice for the files that are already there is a decision for a maintainer.
  • Self-review concern not done: a looser ansi() assertion. It is an exact match, like the other ansi() assertions in that file.

Not fixed here. Each is the same before and after.

  • What alt holds still differs per renderer. ![a \nb](i.png) has alt="a b" in html() and alt: "ab" in react().
  • Bun.markdown.render() gives callbacks plain text and plain meta (language, href, src, title). A callback that writes HTML must escape both.
  • docs/runtime/markdown.mdx does not say that html() does not sanitize.

Related pull requests. #45219 drops spans inside image alt text in the parser. #46069 and #45508 edit other hunks of src/md/html_renderer.rs. #39493 edits the Code arm below this one. None of them touches the Html arm.

Measurements. Two release builds (LTO, linux-x64) of this branch, one hunk apart, made before the last rebase. Counts are for one call of HtmlRenderer::text, inside its own symbol (gdb nexti). perf, valgrind and bloaty are not installed here.

  • A text event that is not raw HTML, instructions / conditional jumps: Normal 34/1 to 33/1, Code 62/9 to 60/9. NullChar 45/4, Br 49/5, Softbr 50/5 and Entity 34/1 do not change.
  • A raw HTML event outside an image: 51/5 to 51/6 with default options, 127/24 to 128/25 with tagFilter. The new jump is not taken.
  • Raw HTML events per render (of all text events): 0 of 11, 0 of 75 and 0 of 1,500 for small, medium and large of bench/snippets/markdown.mjs, 56 of 509 for README.md.
  • Size: HtmlRenderer::text 3,292 B to 2,987 B, write_html_escaped 1,928 B both (llvm-nm -S). Text segment of the stripped binary 107,098,974 B to 107,098,718 B (size). File size 107,276,000 B both.
  • Allocator calls inside render_to_html_with_options (mi_malloc / mi_realloc / mi_free, gdb hit counts): large 12/29/39 both, 1,000 described images 1,008/26/1,032 both. 1,000 copies of the repro line: 8/26/32 to 8/27/33, because the output is longer.
  • The repro line costs 2 more write_html_escaped calls per image, one per raw chunk.
  • No regression in existing documents: the html() output of the 827 examples in test/js/bun/md/*.txt and of 4,979 .md and .markdown files on disk in a built checkout (tracked files, node_modules, vendor/) is the same before and after, with default options, tagFilter and headings (SHA-256 per output).

Tests run on the debug ASAN build. The 204 tests of md-edge-cases pass. So do the 1,043 tests of md-spec, gfm-compat, md-heading-ids, md-react, md-render-callback and test/cli/run/markdown-entrypoint.test.ts. With bun 1.4.3-canary.1 (367d939) the 20 new tests fail. On the release build without the hunk, each table row fails both of its assertions, so the order of the in_image and tag_filter checks is covered.

The text of an image description is the value of the `alt` attribute.
`HtmlRenderer::text` escapes every other text type there, but it wrote a
raw inline HTML chunk (`TextType::Html`) as it is. The first `"` of the
raw tag ended the attribute. The rest of the tag became attributes of the
`<img>` and text behind it.

The `Html` arm now escapes the chunk while an image is open. It checks
that before the tag filter, so the `&lt;` of the filter is not escaped
again. Raw HTML outside an image does not change.

md4c made the same change to this arm in its commit 65c6c9d ("md_html:
Escape raw HTML in image alt attribute").

Co-authored-by: Ilia Alshanetsky <ilia@ilia.ws>
@robobun

robobun commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: the fix is pushed and is ready for review. The Buildkite build passed (#129684).

The one red check on this commit comes from a review workflow that failed on every pull request and is disabled since 2026-10-21. It did not look at this diff.

How I reproduced it. With bun 1.4.3-canary.1 (367d939), and with a release build of this branch that has the one source hunk reverted:

$ bun -e 'console.log(Bun.markdown.html("![x <a href=\"u\">y</a> z](i.png)\n"))'
<p><img src="i.png" alt="x <a href="u">y</a> z" /></p>

With this branch the same command prints:

<p><img src="i.png" alt="x &lt;a href=&quot;u&quot;&gt;y&lt;/a&gt; z" /></p>

The 20 new tests in test/js/bun/md/md-edge-cases.test.ts fail without the fix and pass with it.

@robobun

robobun commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:05 PM PT - Oct 20th, 2026

✅ @robobun, your commit 3bd955679b175419caac500bc3965e04a3fdf777 passed in Build #129684! 🎉


🧪   To try this PR locally:

bunx bun-pr 46249

That installs a local version of the PR into your bun-46249 executable, so you can run:

bun-46249 --bun

@coderabbitai

coderabbitai Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: d9657d58-7f96-4f33-95ac-ccef025e32fb
📥 Commits

Reviewing files that changed from the base of the PR and between 9b4284f and 3bd9556.

📒 Files selected for processing: 2
  • src/md/html_renderer.rs
  • test/js/bun/md/md-edge-cases.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Walkthrough

HTML text inside image descriptions is now escaped before it is written to the alt attribute. Regression tests cover rendering APIs, surrounding markup, and imported and bundled Markdown.

Changes

Image alt text escaping

Layer / File(s) Summary
Escape image text and validate outputs
src/md/html_renderer.rs, test/js/bun/md/md-edge-cases.test.ts
The renderer escapes HTML text inside image descriptions. Tests cover rendering APIs, surrounding markup, and imported and bundled Markdown.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 3bd95

The described change confines escaping to image alt text, and no merge-blocking issue is identified. Normal test checks remain appropriate.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: escaping raw HTML inside image alt attributes.
Description check Passed The description explains the problem, fix, scope, limitations, references, and verification results. It does not use the exact template headings, but it provides the required change summary and detail…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, fix, scope, limitations, references, and verification results. It does not use the exact template headings, but it provides the required change summary and detailed test evidence.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — a three-line branch in the TextType::Html arm that routes raw HTML through the existing write_html_escaped while an image is open, plus a thorough test matrix.

What was reviewed:

  • image_nesting_level bookkeeping in enter_span/leave_span is unchanged and correct for nested images and images inside links; text() is the only writer between the alt quotes.
  • write_html_escaped escapes & < > " and is the same helper already used for Normal text, so the .unwrap() at html_renderer.rs:473 is not a newly reachable user-input path; track_text still skips TextType::Html, so heading ids are unaffected.
  • Ordering of the in_image check before tag_filter prevents double-escaping; the tests assert identical output with and without tagFilter, and react()/render()/ansi() are untouched as they do not share this renderer.
Extended reasoning...

The diff adds a three-line conditional in src/md/html_renderer.rs so raw inline HTML emitted inside an image description is HTML-escaped instead of written verbatim into the img alt attribute, closing an attribute-breakout where a " in the tag ended alt and leaked the rest as markup. It touches HTML output escaping, which is injection-adjacent, but the change strictly tightens escaping at the output layer that owns the invariant and reuses the existing escaper. The remaining hunk is 105 lines of tests covering every raw-HTML kind, every image form, tagFilter on/off, heading ids, the other renderers, and the .md module loader and bundler via subprocess with pipes drained concurrently. The change is small and self-contained, the bug hunt ran dry with no findings, no CODEOWNER covers the files, and no third-party objections are outstanding.

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant