Repository navigation
Fix ReDoS from unclosed HTML tags (#707) - #732
Conversation
_sorta_html_tokenize_re could split a run of attributes several ways: the optional namespace group overlapped the attribute name, and the name could start with whitespace that \s+ also matched. An unclosed tag with repeated `a:b=1` or ` a=1` attributes backtracked exponentially. The name now starts at a non-space character and the redundant namespace group is gone, since `:` is already allowed in names. _tag_is_closed counted openers with `<tag(?:.*?)>`, which rescans the rest of the line from every unclosed `<tag` and goes quadratic. Count them with a linear scan that looks for `>` only up to the end of the current line, which matches what the regex counted. Add the three shapes from the issue to test/test_redos.py. Fixes #707
| line_end = text.find('\n', pos) | ||
| if line_end == -1: | ||
| line_end = len(text) | ||
| end = text.find('>', pos, line_end) |
There was a problem hiding this comment.
Why limit it to scanning only until the end of the line? What would it do for a snippet like this:
<div
style='...'
id='...'
...
>
</div>
There was a problem hiding this comment.
If you wanted to strictly mimic the behaviour of the original regex you could iterate over splitlines instead:
# find all open tags on each line. Ignore tags split over multiple lines because ...
open_tag = f'<{tag_name}'
open_count = 0
for line in text.splitlines():
pos = line.find(open_tag)
while pos != -1:
end = line.find('>', pos)
if end == -1:
break
open_count += 1
pos = line.find(open_tag, end + 1)
# check if number of open tags == number of close tags
if open_count != text.count('</%s>' % tag_name):
return FalseIt's a bit more readable and alot easier to understand
|
Thanks, that is clearer. The old regex <tag(?:.*?)> has no DOTALL, so . stopped at a newline and a tag split across lines was never counted. I kept that, which is why the first version stopped at the end of the line. For your <div example nothing changes: it was not counted before and is not counted now. I switched to your splitlines version in 511e95a. Breaking at the first unclosed <tag on a line keeps the scan linear. The ReDoS tests pass. |
|
Thank you! |
Fixes #707. Supersedes #708, which replaced the tokenizer with a hand-written scanner and was closed by its author. This keeps the existing tokenizer and removes the two slow paths instead.
Cause
Two separate slow paths sit behind the issue's input:
_tag_is_closedcounted opening tags withre.findall('<%s(?:.*?)>' % tag_name, text). From every unclosed<pthe lazy.*?scans to the end of the line before failing, so a line of'<p m="1"' * 15000is quadratic._sorta_html_tokenize_rehad two ways to split the same attribute text. The optional(?:[^\t<>"'=/]+:)?namespace group overlaps the attribute name class, which already allows:, and the name could begin with whitespace that the preceding\s+also matches. An unclosed tag such as'x <p' + ' a:b=1' * 40or'x <p' + ' a=1' * 40backtracks exponentially. Both_hash_html_spansand the tokenizer split use this regex, so the fix covers the spot raised in the review of fix: prevent ReDoS in HTML tokenizer via bounded substring matching (issue #707) #708.Change
[^\s<>"'=/][^<>"'=/]*=, so it starts at the first non-space character and the namespace group is gone. Each attribute has one parse. One degenerate case changes:<p =1>, with two spaces and an empty attribute name, no longer tokenizes as a tag._tag_is_closedcounts openers withstr.find, looking for>only up to the end of the current line. That gives the same count as the old regex (a fuzz comparison over 400,000 random strings of<p,<pre,>, newlines and text found no difference) and stays linear, including for many short lines followed by one distant>.test/test_redos.py, one per shape above, and a CHANGES.md line.Testing
make testredos: the three new cases each time out after 4s on master and pass with the change; 10 passed.make testone(python test.py -- -knownfailure): 288 passed.markdown()with the change:'<p m="1"' * 150000.22s, the two attribute shapes 0.18s each, all three over 4s on master.Only Windows 11 with Python 3.14 was tested. While checking, a separate slow path showed up on master and is left alone here:
markdown('<p m="1"\n' * 20000 + '>')takes about 23s both before and after this change, and a profile puts the time in are.subcall outside both functions changed here.Prepared with Claude Code assistance.