Fix safeLinkHref scheme check running before entity decoding - #22
Merged
jonathanKingston merged 2 commits intoJul 5, 2026
Merged
Conversation
The javascript|data|vbscript denylist tested the raw destination string, but encodeHrefForOutput decoded HTML character references afterwards. So a reference definition like `[r]: javascript:alert(1)` passed the scheme check and then decoded to a live `javascript:` URL in the emitted href. The inline `[x](dest)` path was safe only by accident (entities are pre-decoded to inert PUA before safeLinkHref runs); the reference-definition path was exploitable. Fix: decode HTML character references (undoing source escaping and PUA punctuation first) BEFORE validating, then percent-encode the already-decoded href directly without a second decode pass (which could re-hide a scheme behind double-encoded entities). Replace the 3-entry denylist with a scheme allowlist (http, https, mailto, tel, sms, ftp, ftps + scheme-less relative/fragment/path forms) so unknown and future dangerous schemes fail closed by default. Split percentEncodeHref out of encodeHrefForOutput so validation and encoding share one decode. Add regression tests covering entity-encoded, double-encoded, dangerous, and unknown schemes plus the reference-definition end-to-end path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B88dLwh4TdWDbsDwuqxQUs
Expose the safeLinkHref allowlist as configuration instead of a hardcoded const, mirroring the existing setLinkDecorator injection pattern: - DEFAULT_SAFE_HREF_SCHEMES: the built-in set (http, https, mailto, tel, sms, ftp, ftps). - setSafeHrefSchemes(schemes | null): override the active set (matched case-insensitively) or pass null to restore the default. - getSafeHrefSchemes(): read the active set. Relative/fragment/path destinations remain allowed regardless of the set, and resolution still decodes character references before checking the scheme, so a custom allowlist cannot be bypassed with entity-encoded schemes. Documented in the README with a note that narrowing is always safe but widening must avoid script-executing schemes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B88dLwh4TdWDbsDwuqxQUs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
safeLinkHrefvalidated the link destination's scheme before HTML character references were decoded, so an entity-encoded dangerous scheme slipped past the check and only decoded to a live URL afterwards (duringencodeHrefForOutput).The
javascript|data|vbscriptdenylist tested the raw string;encodeHrefForOutputthen calleddecodeHTMLStrict. Sojavascript:alert(1)passed the check and decoded to a workingjavascript:URL in the emittedhref.renderMarkdown('[click][r]\n\n[r]: javascript:alert(1)')→<a href="javascript:alert(1)">(alsodata:viadata:…, and the double-encoded&#x6a;…).[x](dest)path was safe only by accident (entities are pre-decoded to inert PUA beforesafeLinkHrefruns). The reference-definition path — whose destination is stored raw — was exploitable.autolinkHrefhas the same shape but is not vulnerable: the autolink grammar requires a literalscheme:(letters + literal colon), so entities can't disguise the scheme there. Left unchanged.Fix
safeLinkHrefnow fully resolves the href (undo source HTML-escaping → decode PUA punctuation → decode HTML character references), then validates, then percent-encodes the already-decoded string directly — no second decode pass that could re-hide a scheme behind double-encoded entities.percentEncodeHrefout ofencodeHrefForOutputso validation and encoding share one decode.java	script:) is neutralized too — the percent-encoder turns the tab into%09, breaking the scheme.Configurable allowlist
DEFAULT_SAFE_HREF_SCHEMES—http,https,mailto,tel,sms,ftp,ftps.setSafeHrefSchemes(schemes | null)— override the active set (case-insensitive) or restore the default; mirrors the existingsetLinkDecoratorinjection pattern.getSafeHrefSchemes()— read the active set.Relative destinations stay allowed regardless of the set, and resolution still decodes entities before the scheme check, so a custom allowlist can't be bypassed with entity-encoded schemes. Documented in the README (narrowing is always safe; widening must avoid script-executing schemes).
Tests
npm run typecheckclean; 435/435 tests pass, including the CommonMark conformance baseline (unchanged — the spec's non-http schemes are all autolinks).setSafeHrefSchemesnarrow/restore, case-insensitivity, and no-bypass-on-widen behavior.🤖 Generated with Claude Code
https://claude.ai/code/session_01B88dLwh4TdWDbsDwuqxQUs
Generated by Claude Code