馃悰 Use the CommonMark whitespace set, not the Python one - #418
Open
Nexory wants to merge 1 commit into
Open
Conversation
str.strip() and str.split() without arguments use str.isspace(), which treats U+001C, U+001D, U+001E, U+001F and U+0085 as whitespace. CommonMark and String.prototype.trim do not, so upstream markdown-it keeps them. Ten call sites relied on the Python behaviour, while the module already defines the correct set a few lines above them in MD_WHITESPACE. Two consequences, both measured against markdown-it@14.1.0: - normalizeReference folded distinct labels together, so a link reference definition whose label differs from the usage resolved it anyway. The input "[a b]" with a definition "[a\x85b]: http://evil" produced a link in Python and did not in JavaScript. - The characters were dropped from paragraphs, headings, table cells and fence info strings. Add MD_TRIM_CHARS and mdTrim() to common/utils, and use them at the sites that were relying on str. In the fence renderer, replace str.split() with a split over the same set, for the same reason. MD_TRIM_CHARS is String.prototype.trim minus U+FEFF. trim() does remove U+FEFF, and doing the same here would reintroduce exactly the label folding this change removes. Deliberately unchanged: validateLink() in common/normalize_url.py also uses str.strip(), but there the wider Python set is stricter rather than looser, and aligning it would let more URLs through.
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
str.strip()andstr.split()without arguments usestr.isspace(), which treatsU+001C,U+001D,U+001E,U+001FandU+0085as whitespace. CommonMark does not, and neither doesString.prototype.trim, so upstream markdown-it keeps those characters. Ten call sites in this package relied on the Python behaviour, whilecommon/utils.pyalready defines the correct set a few lines above them inMD_WHITESPACE.Two consequences, both measured against
markdown-it@14.1.0on the same input:A reference definition can resolve a usage that does not name it.
normalizeReferencecollapsed the extra characters, so[a\x85b]and[a b]became the same label:The characters are dropped from the output. They are content, not whitespace:
The change
MD_TRIM_CHARSandmdTrim()incommon/utils.py, used at the sites that were relying onstr. Inrenderer.pythe fence info string was also being cut withstr.split(), which splits on the same wider set, so that becomes a split overMD_TRIM_CHARStoo.MD_TRIM_CHARSis whatString.prototype.trimremoves, minusU+FEFF. The set this module already defines,MD_WHITESPACEtogether withU+2000toU+200A, turned out to be a strict subset oftrim; the three missing characters areU+2028,U+2029andU+FEFF. The first two behave identically in both implementations today, so they are included.U+FEFFis excluded on purpose:trimdoes remove it, and that produces exactly the label folding shown above, with the roles reversed.I did not touch that direction, since making the port faithful there would mean importing the defect.
Deliberately not changed
validateLink()incommon/normalize_url.pyalso usesstr.strip(), but there the wider Python set is stricter rather than looser. Measured:[a](\x85javascript:alert(1))produces no link here and produces<a href="%C2%85javascript:alert(1)">upstream. Aligning it would let more URLs through, so it stays as it is.rules_inline/backticks.pyuses.strip()in the code span edge rule. The rendered output does differ between the two implementations for` \x85 `, but I could not explain what upstream does there, and I would rather leave it than guess.markdown_it/utils.py:184reads spec fixture files and is not on a parsing path.Testing
tests/test_port/test_whitespace.py, 15 cases. Against the currentmaster12 of them fail on the assertion; the 3 control cases, which check that real whitespace is still trimmed and that a real space still separates the fence language from its attributes, pass before and after. That is the point of including them.pytest tests/, the commandtox.iniruns.mypystrict: no issues in 66 source files.ruff checkon the touched files: 2 findings before, the same 2 after. Both pre-existing.python:3.12-slim, digestsha256:2c941e860699f878900b0edc2403613c234d4b32eda3cc9fa7036991a2a63c4a.One note on the checklist
AGENTS.mdasks for aCHANGELOG.mdentry. The three most recently merged PRs, #389, #391 and #394, do not touch that file, and the entries there carry pull request links that only exist after merge, so I left it out rather than create a conflict at release time. Happy to add one if you would prefer it in the PR.