Skip to content

bpo-46811: Make test suite support Expat >=2.4.5 #31453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 21, 2022
Merged

bpo-46811: Make test suite support Expat >=2.4.5 #31453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd7da01
test_xml_etree.py: Drop mistaken test_issue3151
hartwork Feb 20, 2022
aa7523f
test_minidom.py: Support Expat >=2.4.5
hartwork Feb 20, 2022
2dcb305
Add blurp file for bpo-46811 to section "Library"
hartwork Feb 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions Lib/test/test_minidom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@
from test import support
import unittest

import pyexpat
import xml.dom.minidom

from xml.dom.minidom import parse, Node, Document, parseString
from xml.dom.minidom import getDOMImplementation
from xml.parsers.expat import ExpatError


tstfile = support.findfile("test.xml", subdir="xmltestdata")
Expand Down Expand Up @@ -1147,7 +1149,13 @@ def testEncodings(self):

# Verify that character decoding errors raise exceptions instead
# of crashing
self.assertRaises(UnicodeDecodeError, parseString,
if pyexpat.version_info >= (2, 4, 5):
self.assertRaises(ExpatError, parseString,
b'<fran\xe7ais></fran\xe7ais>')
self.assertRaises(ExpatError, parseString,
b'<franais>Comment \xe7a va ? Tr\xe8s bien ?</franais>')
else:
self.assertRaises(UnicodeDecodeError, parseString,
b'<fran\xe7ais>Comment \xe7a va ? Tr\xe8s bien ?</fran\xe7ais>')

doc.unlink()
Expand Down Expand Up @@ -1609,7 +1617,12 @@ def testEmptyXMLNSValue(self):
self.confirm(doc2.namespaceURI == xml.dom.EMPTY_NAMESPACE)

def testExceptionOnSpacesInXMLNSValue(self):
with self.assertRaisesRegex(ValueError, 'Unsupported syntax'):
if pyexpat.version_info >= (2, 4, 5):
context = self.assertRaisesRegex(ExpatError, 'syntax error')
else:
context = self.assertRaisesRegex(ValueError, 'Unsupported syntax')

with context:
parseString('<element xmlns:abc="http:abc.com/de f g/hi/j k"><abc:foo /></element>')

def testDocRemoveChild(self):
Expand Down
6 changes: 0 additions & 6 deletions Lib/test/test_xml_etree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2192,12 +2192,6 @@ def test_issue6233(self):
b"<?xml version='1.0' encoding='ascii'?>\n"
b'<body>t&#227;g</body>')

def test_issue3151(self):
e = ET.XML('<prefix:localname xmlns:prefix="${stuff}"/>')
self.assertEqual(e.tag, '{${stuff}}localname')
t = ET.ElementTree(e)
self.assertEqual(ET.tostring(e), b'<ns0:localname xmlns:ns0="${stuff}" />')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the other changes. For this one, can you explain why this needs to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ambv, have you checked the commit message at dd7da01 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed I have not. I have now. This will sadly be a breaking change. Was the elevated strictness here security-related as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will sadly be a breaking change.

Could you elaborate? I should note that xmlns has also been about URIs.

Was the elevated strictness here security-related as well?

Yes, please see https://github.com/libexpat/libexpat/pull/561/files#diff-d1bcab18f24ba66b34aeb2e156f7fde58ef3de1a165514b0fccf0d04c26838f8R3758-R3767 . This allowed code execution through Expat in another application.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate?

While the use was incorrect per spec, clearly parsing what seems to be XML template files was a use case that existed in the wild when BPO-3151 was filed. The curly-brace and dollar sign suggest some ZOPE-related template (or JBOSS, or JavaScript, or the Sun Java System Web Server, etc. etc.). Whatever this usage was, it will now break with expat 2.4.5+

But since this is security-related, there's nothing we can do other than move on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But since this is security-related, there's nothing we can do other than move on.

@ambv I notice now that (while Expat doesn't do full validation), moving the namespace separator in ElementTree off current } could make this work longer. A space or a newline would be other options, for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ambv it's here:

cpython/Modules/_elementtree.c

Line 3660 in 2cae938

self->parser = EXPAT(ParserCreate_MM)(encoding, &ExpatMemoryHandler, "}");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that may need a closer look, it could be a breaking change too.

def test_issue6565(self):
elem = ET.XML("<body><tag/></body>")
self.assertEqual(summarize_list(elem), ['tag'])
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2022-02-20-21-03-31.bpo-46811.8BxgdQ.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make test suite support Expat >=2.4.5