gh-135661: Fix CDATA section parsing in HTMLParser #135665
+94
−25
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,6 +144,7 @@ def reset(self): | |
| self.lasttag = '???' | ||
| self.interesting = interesting_normal | ||
| self.cdata_elem = None | ||
| self._support_cdata = False | ||
| super().reset() | ||
|
|
||
| def feed(self, data): | ||
|
|
@@ -174,6 +175,9 @@ def clear_cdata_mode(self): | |
| self.interesting = interesting_normal | ||
| self.cdata_elem = None | ||
|
|
||
| def support_cdata(self, flag=True): | ||
| self._support_cdata = flag | ||
|
Member
There was a problem hiding this comment. I'm not convinced this is the best way to handle the issue.
Member
There was a problem hiding this comment. Private means that users shouldn't call it. Given that nothing in It looks like the issue can't be solved in CPython alone -- users need to adapt their code?
Member
Author
There was a problem hiding this comment. This can be solved in CPython, and I hope we will, but it looks like the solution will be too complex to risk backporting it to security-only branches. Providing a method to alter behavior we pass the ball to user side. This is not good, but otherwise the problem will left unsolved. And without closing this hole, all other security fixes for HTMLParser are worthless. I will try other approach, but can't guarantee anything.
Comment on lines
+187
to
+198
Member
There was a problem hiding this comment. Is there any reason to have a setter method, rather than changing the value of
Member
Author
There was a problem hiding this comment. We can change it in future if we need more complex logic. Of course, in Python we can just add a property. And yes, docstring. |
||
|
|
||
| # Internal -- handle data as far as reasonable. May leave state | ||
| # and data to be processed by a subsequent call. If 'end' is | ||
| # true, force handling all data as if followed by EOF marker. | ||
|
|
@@ -249,7 +253,10 @@ def goahead(self, end): | |
| break | ||
| self.handle_comment(rawdata[i+4:j]) | ||
| elif startswith("<![CDATA[", i): | ||
| if self._support_cdata: | ||
| self.unknown_decl(rawdata[i+3:]) | ||
| else: | ||
| self.handle_comment(rawdata[i+1:]) | ||
| elif rawdata[i:i+9].lower() == '<!doctype': | ||
| self.handle_decl(rawdata[i+2:]) | ||
| elif startswith("<!", i): | ||
|
|
@@ -325,7 +332,14 @@ def parse_html_declaration(self, i): | |
| # this case is actually already handled in goahead() | ||
| return self.parse_comment(i) | ||
| elif rawdata[i:i+9] == '<![CDATA[': | ||
| if self._support_cdata: | ||
| j = rawdata.find(']]>', i+9) | ||
| if j < 0: | ||
| return -1 | ||
| self.unknown_decl(rawdata[i+3: j]) | ||
| return j + 3 | ||
| else: | ||
| return self.parse_bogus_comment(i) | ||
| elif rawdata[i:i+9].lower() == '<!doctype': | ||
| # find the closing > | ||
| gtpos = rawdata.find('>', i+9) | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,12 +34,16 @@ def get_events(self): | |
|
|
||
| def handle_starttag(self, tag, attrs): | ||
| self.append(("starttag", tag, attrs)) | ||
| if tag == 'svg': | ||
| self.support_cdata(True) | ||
|
|
||
| def handle_startendtag(self, tag, attrs): | ||
| self.append(("startendtag", tag, attrs)) | ||
|
|
||
| def handle_endtag(self, tag): | ||
| self.append(("endtag", tag)) | ||
| if tag == 'svg': | ||
| self.support_cdata(False) | ||
|
|
||
| # all other markup | ||
|
|
||
|
|
@@ -643,10 +647,22 @@ def test_eof_in_declarations(self): | |
| ('<!', [('comment', '')]), | ||
| ('<!-', [('comment', '-')]), | ||
| ('<![', [('comment', '[')]), | ||
| ('<![CDATA[', [('comment', '![CDATA[')]), | ||
| ('<![CDATA[x', [('comment', '![CDATA[x')]), | ||
| ('<![CDATA[x]', [('comment', '![CDATA[x]')]), | ||
| ('<![CDATA[x]]', [('comment', '![CDATA[x]]')]), | ||
| ('<svg><text y="100"><![CDATA[', | ||
| [('starttag', 'svg', []), ('starttag', 'text', [('y', '100')]), | ||
| ('unknown decl', 'CDATA[')]), | ||
| ('<svg><text y="100"><![CDATA[x', | ||
| [('starttag', 'svg', []), ('starttag', 'text', [('y', '100')]), | ||
| ('unknown decl', 'CDATA[x')]), | ||
| ('<svg><text y="100"><![CDATA[x]', | ||
| [('starttag', 'svg', []), ('starttag', 'text', [('y', '100')]), | ||
| ('unknown decl', 'CDATA[x]')]), | ||
| ('<svg><text y="100"><![CDATA[x]]', | ||
| [('starttag', 'svg', []), ('starttag', 'text', [('y', '100')]), | ||
| ('unknown decl', 'CDATA[x]]')]), | ||
| ('<!DOCTYPE', [('decl', 'DOCTYPE')]), | ||
| ('<!DOCTYPE ', [('decl', 'DOCTYPE ')]), | ||
| ('<!DOCTYPE html', [('decl', 'DOCTYPE html')]), | ||
|
|
@@ -721,26 +737,50 @@ def test_broken_condcoms(self): | |
| ] | ||
| self._run_check(html, expected) | ||
|
|
||
| @support.subTests('content', [ | ||
| 'just some plain text', | ||
| '<!-- not a comment -->', | ||
| '¬-an-entity-ref;', | ||
| "<not a='start tag'>", | ||
| '', | ||
| '[[I have many brackets]]', | ||
| 'I have a > in the middle', | ||
| 'I have a ]] in the middle', | ||
| '] ]>', | ||
| ']] >', | ||
| ('\n' | ||
| ' if (a < b && a > b) {\n' | ||
| ' printf("[<marquee>How?</marquee>]");\n' | ||
| ' }\n'), | ||
| ]) | ||
| def test_cdata_section_content(self, content): | ||
| # See "13.2.5.42 Markup declaration open state", | ||
| # "13.2.5.69 CDATA section state", and issue bpo-32876. | ||
| html = f'<svg><text y="100"><![CDATA[{content}]]></text></svg>' | ||
| expected = [ | ||
| ('starttag', 'svg', []), | ||
| ('starttag', 'text', [('y', '100')]), | ||
| ('unknown decl', 'CDATA[' + content), | ||
| ('endtag', 'text'), | ||
| ('endtag', 'svg'), | ||
| ] | ||
| self._run_check(html, expected) | ||
|
|
||
| def test_cdata_section(self): | ||
| # See "13.2.5.42 Markup declaration open state". | ||
| html = ('<![CDATA[foo<br>bar]]>' | ||
| '<svg><text y="100"><![CDATA[foo<br>bar]]></text></svg>' | ||
| '<![CDATA[foo<br>bar]]>') | ||
| expected = [ | ||
| ('comment', '[CDATA[foo<br'), | ||
| ('data', 'bar]]>'), | ||
| ('starttag', 'svg', []), | ||
| ('starttag', 'text', [('y', '100')]), | ||
| ('unknown decl', 'CDATA[foo<br>bar'), | ||
| ('endtag', 'text'), | ||
| ('endtag', 'svg'), | ||
| ('comment', '[CDATA[foo<br'), | ||
| ('data', 'bar]]>'), | ||
| ] | ||
| self._run_check(html, expected) | ||
|
|
||
|
|
||
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,2 @@ | ||||||||||||||||||
| Fix CDATA section parsing in :class:`html.parser.HTMLParser` according to | ||||||||||||||||||
| the HTML5 standard: ``] ]>`` and ``]] >`` no longer end the CDATA section. | ||||||||||||||||||
|
Comment on lines
+1
to
+2
Member
There was a problem hiding this comment.
Suggested change
|
||||||||||||||||||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should mention the default behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a weak point of such approach. It should be true by default to be able to parse valid HTML (when
<![CDATA[...]]>is only used in foreign content) by default. But secure parsing needs to set it to false at the beginning and the set to true or false after every open or close tag, depending on the complex algorithm.So we should set it to true by default if we keep this approach.