gh-135661: Fix parsing start and end tags in HTMLParser#135930
gh-135661: Fix parsing start and end tags in HTMLParser#135930serhiy-storchaka merged 8 commits intopython:mainfrom
Conversation
* Whitespaces no longer accepted between `</` and the tag name. E.g. `</ script>` does not end the script section. * Vertical tabulation (`\v`) and non-ASCII whitespaces no longer recognized as whitespaces. The only whitespaces are `\t\n\r\f `. * Null character (U+0000) no longer ends the tag name. * End tag can have attributes and slashes after tag name. It no longer ends after the first `>` in quoted attribute value. E.g. `</script/foo=">"/>`. * Multiple slashes and whitespaces between the last attribute and closing `>` are now accepted in both start and end tags. E.g. `<a foo=bar/ //>`. * Multiple `=` between attribute name and value are no longer collapsed. E.g. `<a foo==bar>` produces attribute "foo" with value "=bar". * Whitespaces between the `=` separator and attribute name or value are no longer ignored. E.g. `<a foo =bar>` produces two attributes "foo" and "=bar", both with value None; `<a foo= bar>` produces two attributes: "foo" with value "" and "bar" with value None.
|
I tried to minimize changes and split this PR on several PRs, but they would not be independent, and all these changes are needed to fix the possible XSS. I am planning further refactoring, but this is only for the main branch. |
| @@ -36,29 +36,33 @@ | |||
| # explode, so don't do it. | |||
There was a problem hiding this comment.
I don't know if you saw and heeded the warning or if you just got lucky, but it looks like you were able to change these regex!
Since you renamed locatestarttagend, the comment at line 34 should also be updated.
In addition, make sure that existing comments are still relevant. In particular I would appreciate this for comments linking to specific sections of the HTML5 standard.
There was a problem hiding this comment.
There are links below, they still work, although they now redirect to other address. I updated them.
On other hand, section numbers were changed. I updated them in places which I touched.
| )? | ||
| [\t\n\r\f /]* # possibly followed by a space | ||
| )* | ||
| >? |
There was a problem hiding this comment.
These changes make sense to me.
I also noticed that you removed the start from locatestarttagend_tolerant, presumably because you are now using it to find the end of end tags too (which can contain attributes, even if they are invalid).
This variable is not documented however I can see two options:
- we consider it private and just rename it;
- we create an alias to the old name for backward compatibility, in case someone was using it;
Note that before there was also a set of *_strict variable that got removed, so the _tolerant suffix is no longer needed and it was kept for backward compatibility. Since you are refactoring/renaming (some of) these variables, you might want to consider dropping the _tolerant suffix altogether (and possibly adding aliases to preserve backward compatibility), either in this or in a separate PR.
There was a problem hiding this comment.
Restored the removed variables. I will remove them in the main branch in the following PR.
| self.cdata_elem = elem.lower() | ||
| self.interesting = re.compile(r'</\s*%s\s*>' % self.cdata_elem, re.I) | ||
| self.interesting = re.compile(r'</%s(?=[\t\n\r\f />])' % self.cdata_elem, | ||
| re.IGNORECASE|re.ASCII) |
There was a problem hiding this comment.
Any reason for adding re.ASCII here?
There was a problem hiding this comment.
Yes, it affects case-insensitive mode. Otherwise 'ſ' ~ 's' and 'ı' ~ 'i'. There may be more cases after adding support for title and textarea.
This is not actually a problem in the current code, but future changes could make this important.
Lib/html/parser.py
Outdated
| assert rawdata[i:i+2] == "</", "unexpected call to parse_endtag" | ||
| match = endendtag.search(rawdata, i+1) # > | ||
| if not match: | ||
| if rawdata.find('>', i+2) < 0: |
There was a problem hiding this comment.
| if rawdata.find('>', i+2) < 0: | |
| if rawdata.rfind('>', i+2) < 0: |
Probably inconsequential performance-wise, but using rfind seems more logical here (and possibly elsewhere).
There was a problem hiding this comment.
This check is not actually needed. It is simply an optimization for the case of truncated end tag, because it is faster than endtagopen.match() + locatetagend.match(). I do not know whether it really helps, but I left it as insurance against unpredicted performance degradation.
find may be faster than rfind in general, and in case of end tag, there is large chance to find ">" in first few characters.
Misc/NEWS.d/next/Library/2025-06-25-14-13-39.gh-issue-135661.idjQ0B.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2025-06-25-14-13-39.gh-issue-135661.idjQ0B.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2025-06-25-14-13-39.gh-issue-135661.idjQ0B.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Thank you for review, @ezio-melotti.
| @@ -36,29 +36,33 @@ | |||
| # explode, so don't do it. | |||
There was a problem hiding this comment.
There are links below, they still work, although they now redirect to other address. I updated them.
On other hand, section numbers were changed. I updated them in places which I touched.
| )? | ||
| [\t\n\r\f /]* # possibly followed by a space | ||
| )* | ||
| >? |
There was a problem hiding this comment.
Restored the removed variables. I will remove them in the main branch in the following PR.
| self.cdata_elem = elem.lower() | ||
| self.interesting = re.compile(r'</\s*%s\s*>' % self.cdata_elem, re.I) | ||
| self.interesting = re.compile(r'</%s(?=[\t\n\r\f />])' % self.cdata_elem, | ||
| re.IGNORECASE|re.ASCII) |
There was a problem hiding this comment.
Yes, it affects case-insensitive mode. Otherwise 'ſ' ~ 's' and 'ı' ~ 'i'. There may be more cases after adding support for title and textarea.
This is not actually a problem in the current code, but future changes could make this important.
Lib/html/parser.py
Outdated
| assert rawdata[i:i+2] == "</", "unexpected call to parse_endtag" | ||
| match = endendtag.search(rawdata, i+1) # > | ||
| if not match: | ||
| if rawdata.find('>', i+2) < 0: |
There was a problem hiding this comment.
This check is not actually needed. It is simply an optimization for the case of truncated end tag, because it is faster than endtagopen.match() + locatetagend.match(). I do not know whether it really helps, but I left it as insurance against unpredicted performance degradation.
find may be faster than rfind in general, and in case of end tag, there is large chance to find ">" in first few characters.
Misc/NEWS.d/next/Library/2025-06-25-14-13-39.gh-issue-135661.idjQ0B.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2025-06-25-14-13-39.gh-issue-135661.idjQ0B.rst
Outdated
Show resolved
Hide resolved
| @@ -36,29 +36,33 @@ | |||
| # explode, so don't do it. | |||
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10, 3.11, 3.12, 3.13, 3.14. |
|
GH-136255 is a backport of this pull request to the 3.14 branch. |
Whitespaces no longer accepted between
</and the tag name. E.g.</ script>does not end the script section.Vertical tabulation (
\v) and non-ASCII whitespaces no longer recognized as whitespaces. The only whitespaces are\t\n\r\f.Null character (U+0000) no longer ends the tag name.
End tag can have attributes and slashes after tag name. It no longer ends after the first
>in quoted attribute value. E.g.</script/foo=">"/>.Multiple slashes and whitespaces between the last attribute and closing
>are now accepted in both start and end tags. E.g.<a foo=bar/ //>.Multiple
=between attribute name and value are no longer collapsed. E.g.<a foo==bar>produces attribute "foo" with value "=bar".Whitespaces between the
=separator and attribute name or value are no longer ignored. E.g.<a foo =bar>produces two attributes "foo" and "=bar", both with value None;<a foo= bar>produces two attributes: "foo" with value "" and "bar" with value None.