fix: escape character references in autolink destinations - #4053
fix: escape character references in autolink destinations#4053Kjubikstronk wants to merge 6 commits into
Conversation
Character references are not resolved inside an autolink, so its destination and text are literal, but both were written into the output unescaped. A destination containing a valid reference such as `<` was decoded again by the browser, so the link pointed somewhere other than what was written. Mark autolink and extended url tokens and escape every `&` when rendering them. Inline links keep the source text, which is already valid in an attribute, so they are unchanged.
|
@Kjubikstronk is attempting to deploy a commit to the MarkedJS Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Looks like this also doesn't follow CommonMark for [example](http://example.com?foo=1&bar=2)<p><a href="http://example.com?foo=1&bar=2">example</a></p> |
|
You're right, and my reasoning in the description was too narrow. I checked that a destination containing a reference round-trips correctly and concluded inline destinations were fine, but a bare The two need different treatment, and
Which is a one-line change now that the autolink flag exists: href = escapeHtmlEntities(cleanHref, autolink);Image sources had the same gap, so they are escaped too. Tests added for the bare-ampersand case in both a link and an image, plus one pinning that a reference in an inline destination is left alone.
|
An inline link destination keeps its source text, so a reference such as `<` is already correct in an attribute, but a bare `&` is not. Escape the ones that cannot begin a reference, in image sources as well, and keep escaping everything inside an autolink.
| <p><a href="https://example.com/?a=1&b=2">t</a></p> | ||
| <p><a href="http://example.com?foo=1&bar=2">example</a></p> | ||
| <p><img src="http://example.com?foo=1&bar=2" alt="i"></p> | ||
| <p><a href="http://example.com?a=1<2">t</a></p> |
There was a problem hiding this comment.
According to this CommonMark demo this line should be
<p><a href="http://example.com?a=1%3C2">t</a></p>
There was a problem hiding this comment.
Can you use these as the tests:
---
renderExact: true
---
https://example.com/?x=1<2
https://example.com/?y=1&2
https://example.com/?a=1&b=2
<https://example.com/?x=1<2>
<https://example.com/?y=1&2>
<https://example.com/?a=1&b=2>
[https://example.com/?x=1<2](https://example.com/?x=1<2)
[https://example.com/?y=1&2](https://example.com/?y=1&2)
[https://example.com/?a=1&b=2](https://example.com/?a=1&b=2)
[https://example.com/?x=1<2][link1]
[https://example.com/?y=1&2][link2]
[https://example.com/?a=1&b=2][link3]



[link1]: https://example.com/?x=1<2
[link2]: https://example.com/?y=1&2
[link3]: https://example.com/?a=1&b=2
<p><a href="https://example.com/?x=1&lt;2">https://example.com/?x=1&lt;2</a></p>
<p><a href="https://example.com/?y=1&amp;2">https://example.com/?y=1&amp;2</a></p>
<p><a href="https://example.com/?a=1&b=2">https://example.com/?a=1&b=2</a></p>
<p><a href="https://example.com/?x=1&lt;2">https://example.com/?x=1&lt;2</a></p>
<p><a href="https://example.com/?y=1&amp;2">https://example.com/?y=1&amp;2</a></p>
<p><a href="https://example.com/?a=1&b=2">https://example.com/?a=1&b=2</a></p>
<p><a href="https://example.com/?x=1%3C2">https://example.com/?x=1<2</a></p>
<p><a href="https://example.com/?y=1&2">https://example.com/?y=1&2</a></p>
<p><a href="https://example.com/?a=1&b=2">https://example.com/?a=1&b=2</a></p>
<p><a href="https://example.com/?x=1%3C2">https://example.com/?x=1<2</a></p>
<p><a href="https://example.com/?y=1&2">https://example.com/?y=1&2</a></p>
<p><a href="https://example.com/?a=1&b=2">https://example.com/?a=1&b=2</a></p>
<p><img src="https://example.com/?x=1%3C2" alt="https://example.com/?x=1<2"></p>
<p><img src="https://example.com/?y=1&2" alt="https://example.com/?y=1&2"></p>
<p><img src="https://example.com/?a=1&b=2" alt="https://example.com/?a=1&b=2"></p>
There was a problem hiding this comment.
updated the tests to add reflinks and show the difference between the href and text of some
There was a problem hiding this comment.
Swapped in your cases, thanks — the reflinks and the href/text split make the autolink-vs-inline difference much clearer than what I had.
On the %3C one: you're right that CommonMark gives http://example.com?a=1%3C2 there, but that line comes out the same on master as on this branch. I diffed the whole fixture both ways — this PR only changes the six autolink lines and the ?a=1&b=2 cases, where master emits a raw & in the href. The three < inline/reference/image destinations are byte-identical before and after.
So the missing percent-encoding looks like a separate gap rather than something I've introduced. Happy to take it on, either here or as its own PR — just say which you'd prefer, since it's a different code path from the entity escaping.
There was a problem hiding this comment.
Can you add a comment in the file about the lines that don't match CommonMark
There was a problem hiding this comment.
Added it to the frontmatter — front-matter runs the block through YAML, so # comments are stripped and never reach the rendered output. The fixture still passes byte for byte.
It calls out that the < inline links, reference links and images are the ones that differ: CommonMark resolves the reference and percent-encodes it to ?x=1%3C2 where marked keeps ?x=1<2. I noted that this comes from URL encoding rather than from the escaping in this PR, and that it is unchanged on master, so nobody reads those lines later as something this PR introduced.
|
Interestingly GitHub doesn't parse this one correctly GitHub https://example.com/?y=1&2 |
Fixes #4052.
Following up on my comment there — the one-line change the issue suggests regresses inline links, so this takes the narrower route instead.
Why it is autolink-specific
CommonMark resolves character references in link destinations but not in autolinks ("backslash-escapes and entity references do not work in autolinks"). marked never resolves them anywhere —
token.hrefkeeps the literal source text — and that produces two different outcomes:[t](…?x=1<2)— the destination value should be?x=1<2, which is written back into an attribute as<. The unresolved source text is already exactly that, so the current output is right.<…?x=1<2>— the destination value is the literal text?x=1<2, which has to be written as&lt;. Emitting it raw lets the browser decode it, so the link points at?x=1<2.That is why escaping every destination breaks the first case while fixing the second:
<https://example.com/?x=1<2>?x=1<2❌?x=1&lt;2✅?x=1&lt;2✅[t](https://example.com/?a=1&b=2)?a=1&b=2✅?a=1&amp;b=2❌?a=1&b=2✅The change
Tokenizer.autolink()andTokenizer.url()now setautolink: trueon the token, andRenderer.link()escapes the destination and the text withescapeHtmlEntities(…, true)when it is set. The flag is an optional addition toTokens.Link, so existing custom renderers and extensions are unaffected.The text needed it too, not just the href — for
<…?x=1<2>the anchor text was also rendering as<, because the default text escaping deliberately skips anything that already looks like a reference. Both halves now match the expected output in the issue.This also fixes CommonMark example 595
Example 595 is in
test/specs/commonmarkwithshouldFail: false, so it is expected to pass — but marked's actual output for it was:against an expected
hrefof…?q=hello&id=22&boolean. The href and the link text disagreed with each other.It was reported as passing because the spec runner compares with
HtmlDiffer, which parses both sides — and in an attribute,&id=22and&id=22parse to the same value. That normalisation hides the difference whenever the text is not a valid reference. It only becomes visible with something like<, where the two parse to different values, which is what the issue reporter demonstrated with parse5.So the spec suite could not have caught this, and still cannot. I've written the new tests as exact string comparisons in
test/unit/marked.test.jsrather than adding spec fixtures, for that reason.I have deliberately not touched the unresolved-reference behaviour itself — examples 32 and 33 are still marked
shouldFail, and[foo](/föö)still renders unchanged. That is the larger gap tracked in #4050 and wants its own discussion.Verification
test:specs— 1779 pass, 0 fail, unchanged before and after.test:unit— 189 pass, up from 185; the 5execfailures are pre-existing onmainand fail identically with this change stashed.tsc --noEmitandeslintclean on the touched files.The four
Lexertoken assertions for autolinks and urls were updated for the new field.