8000 bpo-46811: Make test suite support Expat >=2.4.5 by hartwork · Pull Request #31453 · python/cpython · GitHub
[go: up one dir, main page]
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

Changes from 1 commit
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
Next Next commit
test_xml_etree.py: Drop mistaken test_issue3151 
Curly brackets were never allowed in namespace URIs
according to RFC 3986, and so-called namespace-validating
XML parsers have the right to reject them a invalid URIs.

libexpat >=2.4.5 has become strcter in that regard due to
related security issues; with ET.XML instantiating a
namespace-aware parser under the hood, this test has no
future in CPython.

References:
- https://datatracker.ietf.org/doc/html/rfc3968
- https://www.w3.org/TR/xml-names/
  • Loading branch information
@hartwork
hartwork committed Feb 20, 2022
commit dd7da01325ca32796e139507a38da08886f8f972
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
0