Skip to content

fix(html): keep a URL from being cut at a space or a stray parenthesis - #2519

Open
Lukas (L4XB) wants to merge 1 commit into
microsoft:mainfrom
L4XB:fix/uri-destinations
Open

Lukas (L4XB) wants to merge 1 commit into
microsoft:mainfrom
L4XB:fix/uri-destinations

Conversation

@L4XB

Copy link
Copy Markdown

What

A bare Markdown link destination ends at the first whitespace and at an unbalanced closing parenthesis. Both are legal in a query string or a fragment, and _CustomMarkdownify deliberately does not re-encode those components — there is a test pinning exactly that, test_html_href_does_not_quote_query_or_fragment, because percent-encoding a query would rewrite sub-delimiters the server may be reading.

The result is a destination that no longer round-trips:

<a href="https://example.com/s?q=a b">spaced</a>
  -> [spaced](https://example.com/s?q=a b)          destination ends at the space

<a href="https://example.com/s?q=a)b">result</a>
  -> [result](https://example.com/s?q=a)b)          destination ends at the `)`, "b)" becomes text

<a href="https://example.com/p#sec)1">anchor</a>
  -> [anchor](https://example.com/p#sec)1)

Reading that Markdown back gives a truncated URL, silently — the output is still valid Markdown, just pointing somewhere else.

convert_img has the same problem and one more: it was not escaping src at all, not even the path quoting convert_a applies.

<img src="https://example.com/a b.png" alt="pic">  -> ![pic](https://example.com/a b.png)
<img src="https://example.com/a)b.png" alt="pic">  -> ![pic](https://example.com/a)b.png)

An image destination is parsed by exactly the same rule as a link destination, so both truncate.

How

  • _escape_uri(url) — the path quoting convert_a already did, extracted so convert_img can share it. convert_a's behaviour is unchanged by the extraction.
  • _format_destination(url) — wraps the destination in <...> when it holds whitespace, a parenthesis or an angle bracket. That is CommonMark's own mechanism for the case, and it leaves the URL byte for byte as it was, which is the property the existing test is protecting. An angle bracket inside is percent-encoded, since it cannot appear in an angle-bracket destination.
  • convert_img gets _escape_uri + _format_destination. A data: URI skips the quoting: its payload is not a path, and quoting it would turn data:image/png;base64,... into data:image/png%3Bbase64,....

A balanced pair of parentheses in a query would parse fine bare, and is wrapped anyway. Telling balanced from unbalanced is more machinery than this warrants, and <...> is always valid.

[spaced](<https://example.com/s?q=a b>)
[result](<https://example.com/s?q=a)b>)
![pic](https://example.com/a%20b.png)
![pic](https://example.com/a%29b.png)

Test

packages/markitdown/tests/test_html_converter.py — five tests added, one changed.

main this branch
tests/test_html_converter.py 4 failed, 27 passed 31 passed

The changed one, so you do not have to find it: test_html_href_does_not_quote_query_or_fragment now expects [example](<https://example.com/a%20path?query=a b%20c#fragment with spaces>) instead of the same string without the angle brackets. The URL inside is the identical expected_href the test already declared — nothing is re-encoded, so what the test is named for still holds. Only the delimiters are added, and a comment in the test says why.

The new ones cover an unbalanced ) in an href, an image src with a space, an image src with an unbalanced ), plus two guards: an ordinary URL stays bare (no angle brackets), and a data URI is left exactly as it was.

Full suite: 897 passed, 14 skipped. black (the pinned 23.7.0 from .pre-commit-config.yaml) reports both files unchanged.

One thing this does not touch, in case you want it in the same change: the alt text of an image is still taken raw from the attribute, so an alt holding an unbalanced ] breaks ![...](...) the same way. That is a separate escape (text, not destination) and I left it out rather than widen this.

A bare Markdown link destination ends at the first whitespace and at an
unbalanced closing parenthesis. Both are legal in a query string or a fragment,
which this converter deliberately does not re-encode, so a URL carrying either
was written out in a form that reads back truncated:

    <a href="https://example.com/s?q=a b">   ->  [spaced](https://example.com/s?q=a b)
    <a href="https://example.com/s?q=a)b">   ->  [result](https://example.com/s?q=a)b)

Wrap such a destination in angle brackets, which is what CommonMark provides
for the case and which leaves the URL itself byte for byte as it was.

An image destination is parsed exactly like a link destination, and convert_img
was not escaping its src at all -- not even the path quoting convert_a has
applied for a while. It now shares both. A data URI is left alone, since its
payload is not a path to quote.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant