`&` not HTML-escaped in link/autolink href, silently changing the link target

[maximilliangrand]

Marked version: 18.0.9 (also reproduces on every version tested, back to v12)

Markdown flavor: CommonMark

Repro

const { marked } = require('marked');
marked('<https://example.com/?x=1&lt;2>');

Expected (commonmark.js and CommonMark spec example 595 — & in the destination is escaped to &):

<p><a href="https://example.com/?x=1&amp;lt;2">https://example.com/?x=1&amp;lt;2</a></p>

Actual (raw & in the href):

<p><a href="https://example.com/?x=1&lt;2">https://example.com/?x=1&lt;2</a></p>

Because < is a valid HTML character reference, a browser decodes it inside the attribute, so the link actually points to https://example.com/?x=1<2 — a different URL than was written. Confirmed by parsing marked's output with parse5:

marked href, parsed back: "https://example.com/?x=1<2"     (wrong)
commonmark href, parsed:  "https://example.com/?x=1&lt;2"   (correct)

Note marked already escapes the same URL in the anchor text (&) but not in the href, so the output is internally inconsistent. Any URL containing a valid entity sequence (<, &, ©, …) is affected; this is silent link-target corruption for user- or attacker-supplied URLs.

Cause
Renderer.link() writes cleanUrl(href) straight into the attribute without escapeHtmlEntities() — while the title attribute one line below is escaped. cleanUrl only runs encodeURI, which never encodes &, so the raw & reaches the attribute. (Renderer.image() has the same pattern for src.)

[UziTech]

Thanks for reporting this. If you can create a PR to fix this that would be very helpful 😁👍

[Kjubikstronk]

I had a go at this and hit something worth flagging before anyone writes the patch: escaping href in Renderer.link() on its own fixes the autolink case but regresses inline links.

Applying exactly the suggested change (escapeHtmlEntities(href, true) on the href, and the same for src in image()):

Input	before	after
<https://example.com/?x=1&lt;2>	href="…?x=1&lt;2" ❌	href="…?x=1&amp;lt;2" ✅
[t](https://example.com/?a=1&amp;b=2)	href="…?a=1&amp;b=2" ✅	href="…?a=1&amp;amp;b=2" ❌

The second row is the problem — a browser decodes that to ?a=1&b=2, so the change swaps one silently-wrong link target for another.

The reason is that the destination is never decoded. marked.lexer() keeps the literal source text on the token:

'<https://example.com/?x=1&lt;2>'       token.href = "https://example.com/?x=1&lt;2"
'[t](https://example.com/?a=1&amp;b=2)' token.href = "https://example.com/?a=1&amp;b=2"

For inline links that means "no decode" and "no escape" currently cancel out, and the output happens to be right. Escaping only one half of that breaks it.

CommonMark wants the two constructs treated differently — entity and numeric character references are recognised in link destinations, but not in autolinks ("backslash-escapes and entity references do not work in autolinks"). So a complete fix looks like resolving character references when parsing an inline link destination, leaving autolink destinations literal, and then escaping on output — renderer-only changes can't separate the two cases, because by then the information about which construct it was is gone.

One more thing worth knowing: the spec suite passes either way. npm run test:specs gives 1779 pass / 0 fail both before and after the change above, so it catches neither the original bug nor the regression it introduces. Whatever the fix ends up being probably wants a test that asserts on the parsed-back href value rather than the HTML string.

Happy to put together a PR along those lines if that approach sounds right to you — wanted to check the direction first, since it's more than the one-liner the issue implies.