Repository navigation
markdown: hand react(), render() and ansi() the resolved href and title through SpanDetail accessors - #46069
markdown: hand react(), render() and ansi() the resolved href and title through SpanDetail accessors#46069robobun wants to merge 3 commits into
Conversation
…le through SpanDetail accessors
|
Updated 3:05 AM PT - Oct 21st, 2026
❌ @robobun, your commit d559e13 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46069That installs a local version of the PR into your bun-46069 --bun |
|
Status: the pull request is open. It waits for CI and for a maintainer review. How I reproduced the problem on a release build of main (91e7e3b): const text = String.raw`[wiki](<https://example.com/wiki/Foo_\(bar\)> "R&D \"notes\""), , <a@b.co>`;
const seen = [];
const take = (children, meta) => (seen.push(meta), children);
Bun.markdown.render(text, { link: take, image: take });
console.log(JSON.stringify(seen));
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughMarkdown link and image metadata now resolves backslash escapes and character references. Autolink metadata includes its scheme. ANSI image handling uses resolved and written paths and sanitizes terminal addresses. Tests and documentation cover the updated behavior. ChangesMarkdown metadata resolution
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main change: resolved link and title values are provided through SpanDetail accessors to markdown APIs. |
| Description check | |
The description is detailed and covers the problem, fix, scope, tradeoffs, design, tests, and verification. It does not use the template headings exactly, but it provides the required information. |
| Linked Issues check | |
Issue #31936 requires Bun.markdown.react() to include mailto: for email autolinks. SpanDetail::href() adds mailto: for email autolinks and http:// for www. autolinks. The React renderer us… |
| Out of Scope Changes check | |
The parser, SpanDetail accessors, render(), ansi(), image prefetch, documentation, and tests use one shared link-metadata contract. These changes support consistent resolved values and the linke… |
Full details: Linked Issues check
-
Explanation
Issue
#31936requiresBun.markdown.react()to includemailto:for email autolinks.SpanDetail::href()addsmailto:for email autolinks andhttp://forwww.autolinks. The React renderer useshref().md-react.test.tsverifies the email case and matching schemes. The implementation satisfies the linked coding requirement.
Full details: Out of Scope Changes check
-
Explanation
The parser,
SpanDetailaccessors,render(),ansi(), image prefetch, documentation, and tests use one shared link-metadata contract. These changes support consistent resolved values and the linked autolink behavior. No unrelated change is demonstrated.
- 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.
Comment @coderabbitai help to get the list of available commands.
✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/md/ansi_renderer.rs— Terminal users clicking a link whose plain destination holds[or]now get a truncated URL; on the base the same document gave the full URL. resolve_escapes turnsinto a raw ESC before strip_controls runs, and strip_controls at src/md/ansi_renderer.rs:2397-2423 then swallows the whole following CSI or OSC sequence, not just the ESC byte. The base only reached this for a raw ESC inside<...>; a plain destination rejected controls at src/md/links.rs:127, sowas passed through as text. Fix: when sanitizing a URL, drop only the control byte itself and keep the bytes after it, so the OSC 8 target and the(url)fallback match what html() percent-encodes.Why this was flagged
Input
[a](https://example.com/x]y/long/path)rendered bybun file.mdor Bun.markdown.ansi(). SpanDetail::href() at src/md/types.rs:286 now resolvesto byte 0x1b (helpers::resolve_escapes, src/md/helpers.rs:444). resolve_href at src/md/ansi_renderer.rs:2481 passes that to sanitized_url -> strip_controls. At src/md/ansi_renderer.rs:2407-2419 the]branch consumes every byte to the end of the slice looking for BEL or ST, so the OSC 8 target written at 582 and the(url)text at 651 becomehttps://example.com/x. On the base,stayed as six literal characters because a plain destination cannot hold a raw control (src/md/links.rs:127 returns None), so the full URL was emitted. Thepre_existingdismissal only holds for the<...>form with a raw ESC; the population is now every plain destination with a numeric character reference for a control. html() writes%1B]y/long/pathand keeps the path. Remedy: in the URL variant of strip_controls, remove only disallowed bytes and never skip the characters following an ESC.Verification:
scan_link_destination(src/md/links.rs:127) rejects raw controls in a plain destination but passesas text, so on the base the literal URL was emitted unchanged. After the changeresolve_escapesmaps 27 to 0x1b, andstrip_controls(src/md/ansi_renderer.rs:2407-2419) drops the whole tail after], so the OSC 8 target at line 582 and the(url)fallback at 651 becomehttps://example.com/x.
| let dest: Box<[u8]> = Box::from(&ref_def.dest[..]); | ||
| let title: Box<[u8]> = Box::from(&ref_def.title[..]); | ||
| let has_escape = ref_def.has_escape; | ||
| if !self.charge_ref_def_output(dest.len(), title.len()) { |
There was a problem hiding this comment.
🟡 (optional) Callers of react() and render() can get more reference-link output than the amplification budget allows; the base branch charged the exact bytes handed on. src/md/links.rs:513 charges dest.len() and title.len() of the raw RefDef, but MarkdownObject.rs:850 and :1364 now copy detail.href() and detail.title(), which resolve_escapes (helpers.rs:444) can make longer than the raw bytes, e.g. ≫⃒ (5 bytes) becomes 6 bytes of UTF-8. The guard at parser.rs:304 therefore under-counts by up to a fifth on every reference link that resolves. Fix: charge the resolved length (or the worst-case expansion) at both charge sites, links.rs:513 and :550, so the budget bounds what the renderer actually allocates.
Why this was flagged
Input: a document with one reference definition [r]: /≫⃒≫⃒≫⃒... whose destination is thousands of two-codepoint named entities, followed by as many [a][r] references as fit. Each reference reaches links.rs:510-513, where charge_ref_def_output (src/md/parser.rs:304-313) subtracts the raw dest length from a budget of 16 × input size capped at 1 MiB (parser.rs:360). ParseRenderer::enter_span at src/runtime/api/MarkdownObject.rs:850 then allocates Box::from(detail.href()), and resolve_escapes at src/md/helpers.rs:444-479 expands each ≫⃒ from 5 to 6 bytes, so the total bytes held by react() output exceed the budget by about 20%. On the base branch the Box copied detail.href as written, so the charge and the allocation matched exactly. The guard still exists but no longer bounds what it is meant to bound. Remedy: charge the resolved length, or scale the charge by the maximum entity expansion, at both reference-link sites.
Verification: nit. Trigger: a reference definition destination packed with entities such as ≫⃒ (5 raw bytes -> 6 UTF-8 bytes), referenced many times. src/md/links.rs:510-513 charges the raw RefDef bytes, while src/runtime/api/MarkdownObject.rs:850-851 now copies detail.href() / detail.title() after resolve_escapes. On the base branch those sites copied the raw bytes, so the charge was exact.
There was a problem hiding this comment.
I did not change the charge. The reasons:
- The budget is the bound of md4c on the growth from reference links. It counts the bytes of the definition as written.
html()already writes more than the charged bytes. It percent-encodes a destination, so one byte can become three. It escapes a title, so"becomes".- A resolved value is at most 1.2 times as long as the written value. Of the 2,125 named character references, only
≫⃒and≪⃒resolve to more bytes than they are written (5 to 6). A numeric reference never does. I checked each one against this branch. - So the output stays linear in the charged bytes.
- A charge by the resolved length would move the point where
html()stops resolving references. A resolved value is usually shorter than the written one, sohtml()would resolve more of them. This change does not touch the output ofhtml().
…p a tab for the image file lookup
|
About the review finding on
|
…esolve-link-attrs
Fixes #31936
Problem
Bun.markdown.react()andrender()give user code the source text of a link destination and title. For[a](</Foo_\(bar\)> "R&D"),hrefis/Foo_\(bar\)andtitleisR&D. Inhtml()the values are/Foo_(bar)andR&D.www.autolink has no scheme:<a@b.co>giveshref: "a@b.co", notmailto:a@b.co(Bun.markdown.react()andBun.markdown.html()do not produce the same results when usingautolinks: trueon email links #31936).ansi()and thebun file.mdimage download do the same. Cause: the parser passes source slices inSpanDetail, and onlyHtmlRendererresolves them.Fix
SpanDetail::href()andtitle()return the value thathtml()writes before it encodes.href_as_written()andtitle_as_written()are the named opt-out forHtmlRendererandbun fmt.react(),render(),ansi()and the image prefetch call them. A value without a\or&is lent with no scan: the scan that cuts it notes them.test/js/bun/md/andtest/cli/run/markdown-entrypoint.test.ts. 39 new tests fail without the fix.html()(Notes).Background
\*means*and&means&(CommonMark 6.3).SpanDetailis what the parser gives a renderer for a link or an image. Its fields are private now.MarkdownObject.rsonly. It leavesansi()and the prefetch raw.Downsides
href,srcortitleitself, or addsmailto:itself, now does it twice.[x](javascript:alert(1))giveshref: "javascript:alert(1)".realloc. No link without an escape got slower..textgrows 3,840 bytes.Notes
Where it came from. A fuzzing report that a maintainer relayed. Its question: "Could both resolve escapes and entities before they hand the values on?" No issue exists for that half. #31936 is the user report for the scheme.
Values, before and after (
react()props andrender()meta,html()is unchanged)[a](<https://example.com/Foo_\(bar\)> "R&D \"n\"")hrefhttps://example.com/Foo_\(bar\)https://example.com/Foo_(bar)titleR&D \"n\"R&D "n"[a](/?a=1&b=2&c)href/?a=1&b=2&c/?a=1&b=2&csrca\_b.pnga_b.png[a][r]with[r]: </a\_b> 'x © y'href,title/a\_b,x © y/a_b,x © y<a@b.co>, ora@b.cowithautolinks: truehrefa@b.comailto:a@b.cowww.example.comwithautolinks: truehrefwww.example.comhttp://www.example.com[a](a\b&nosuch;)hrefa\b&nosuch;<https://example.com/a\_b>hrefhttps://example.com/a\_b[text](a@b.co)hrefa@b.coansi()changes in the same way: the OSC 8 target, the(url)fallback, the image source and title.bun file.mddownloads a remote image from the resolved URL.Design
SpanDetailhas six readers:HtmlRenderer,AnsiRenderer,ImageUrlCollector, the two renderers inMarkdownObject.rs, and the tree builder ofbun fmt. Two of them want the source bytes. So the contract is onSpanDetail: private fields, the resolved value under the short name, a named accessor for the source bytes.helpers::resolve_escapeshas the rules of the writers inhtml_renderer.rs. Those writers stay as they are: they resolve and encode in one pass with no allocation.scan_link_destinationand the two title loops. A reference definition keeps it inRefDef. An earlier draft took it from the marks of the whole block. Alinkcallback can write into the inputBuffer, which the parser reads in place, so that flag could be stale.ansi_renderer.rsthe values are resolved before the control bytes are stripped, becausestands for one. From the address of a link or an image only the control bytes go. What follows an ESC stays, because it is part of the address, as inhtml(). What is written into an OSC 8 sequence also loses a tab and a line break (	, ), which would break the sequence. The file lookup of an image keeps them: a file name can have a tab. The prefetch and the renderer make the key of a remote image with the same function.has worked inansi()on every platform, because the path join takes\as a separator. With the escapes resolved that path is....\img\logo.png. Soansi()tries the resolved path first and the path as written second. If botha_b.pnganda/_b.pngexist,now showsa_b.png.Related pull requests
languagebelongs on itsFencedCode::language()and comes after it. If markdown: report fenced code to a renderer with its info string, not an offset into the source #45508 takes its other shape, the resolve is one helper on the info string offset.noHtmlSpansand emit no span inside image alt text #45219 and markdown: keep letters outside ASCII in heading ids and give each heading its own id #45896 each get a text conflict with this change, inlinks.rs(aSpanDetailliteral against the new constructors) and inhelpers.rs(two insertions at one place).Numbers (release builds of the merge base 91e7e3b and of this branch, x64 with AVX-512,
BUN_JSC_useJIT=0, documents of 1,000 elements, the run with 0 iterations subtracted, instructions per element)instructions:u, median of 5 runs of 2,000 iterations, run-to-run spread 1 to 2 per element). html() with<...>destinations: -32.ansi()had the scheme before.\or&, 1 on an attribute of a link that holds one (calls ofresolve_escapes, hardware breakpoint counts).reallocin react() and render(), amallocin ansi() (hardware breakpoint counts on the mimalloc entry points)..text+3,840 bytes (size), the stripped file +4,096 bytes.SpanDetailstays 40 bytes (aconstassertion).RefDefgrows from 32 to 40 bytes.newfstatatchanges froma/_b.pngtoa_b.png, and the request from/r.png?a=1&b=2to/r.png?a=1&b=2. A local image whose resolved path does not exist costs one morenewfstatat.instructions:ufromperf_event_open. The call counts are hardware breakpoints (PERF_TYPE_BREAKPOINT). The syscalls are counted withptrace.Tests
md-react.test.ts,md-render-callback.test.ts: a table of inputs for each API, as a string and as aBuffer. The scheme rows of markdown: add the mailto: and http:// autolink scheme to react() and render() hrefs #42757. Alinkcallback that writes an escape into a later destination.md-edge-cases.test.ts: a sweep compares thehref,srcandtitleofreact()with the attributes thathtml()writes, onspec.txt,spec-gfm.txt,spec-permissive-autolinks.txt,regressions.txtandcoverage.txt: 204 values in 176 of 763 examples.html()also percent-encodes, so the sweep applies a model of that encoding to thereact()value first. Examples with a raw<a>or<img>are left out. A second table has inputs that the spec files lack, with the values written out. The existing test that folds thereact()tree into therender()output tiesrender()to the same values.md-edge-cases.test.ts,ansi(): resolved values, and character references that stand for ESC, BEL, CSI, a tab and a line feed.markdown-entrypoint.test.ts: local images by resolved path and by the path as written, one with a tab in its name. A remote image with&in the URL is requested with&and shown.react()tree and compare it withhtml(). It needs an HTML writer for the tree in the test, and it pins differences that are not about links.test/cli/format/conformance.test.ts,test/cli/lint/explain.test.ts,test/cli/lint/conformance.test.ts,test/internal/source-lints/,test/integration/bun-types/bun-types.test.ts.Not in this change
language: onlyhtml()resolves it. See markdown: report fenced code to a renderer with its info string, not an offset into the source #45508 above.html()percent-encodes a destination.react(),render()andansi()do not.<https://example.com/?a=1&b=2>keeps&inhtml(), so it does here.html()and inreact().�gives U+FFFD.html()and a line ending inreact(),render()andansi().