FAIL: test_no_new_lines (tests.unit.test_utils.TestIsValidEndpointURL) ; with Python versions patched for bpo-43882

[mgorny]

Describe the bug
The recent fix to bpo43882 has caused the following test to start to fail:

======================================================================
FAIL: test_no_new_lines (tests.unit.test_utils.TestIsValidEndpointURL)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/portage/dev-python/botocore-1.20.64/work/botocore-1.20.64/tests/unit/test_utils.py", line 713, in test_no_new_lines
    self.assertFalse(is_valid_endpoint_url('https://foo.bar.com\nbar/'))
AssertionError: <re.Match object; span=(0, 14), match='foo.bar.combar'> is not false

The relevant behavior change in CPython is that all newlines (as well as CRs and tabs) are stripped from the URL, so they're not present in the split components anymore.

Steps to reproduce
Install Python 3.10.0b1 or 3.9 from git branch (the change is going to be the part of the next release), and run tests with it.

Expected behavior
Tests passing ;-). I suppose you could either check URLs for newlines early on, and reject all newlines, or agree with the new behavior of stripping them. In the latter case, it might make sense to add stripping inside botocore too, for consistent behavior across Python versions.

Debug logs
n/a

[mgorny]

Small correction: the Python change was already released as 3.9.5.

[nateprewitt]

Hi @mgorny,

Thanks for the report! It looks like this made it into 3.9 but missed the window for a 3.8 backport until later this year. My initial hunch is this change may cause some waves through the rest of the HTTP stack for Python in Requests/Urllib3 as well. We'll have to go validate that though.

For the time being, this test is going to be tricky to fix. Some versions of 3.9 will now strip these for us, while we still need to enforce the behavior in older ones. We'll need to set up some testing infrastructure to do checks based on the point release for 3.9 before making assertions around this. We'll also likely need to make a way to back port it for other arbitrary versions of Python if 3.8 is going to change behavior mid-lifecycle. botocore still doesn't officially support 3.9, but this seems like a reasonable thing to get fixed sooner than later.

[mgorny]

Well, I've worked it around locally with this ugly hack:

diff --git a/botocore/utils.py b/botocore/utils.py
index 378972248..b154469bc 100644
--- a/botocore/utils.py
+++ b/botocore/utils.py
@@ -977,6 +977,8 @@ class ArgumentGenerator(object):
 
 
 def is_valid_ipv6_endpoint_url(endpoint_url):
+    if '\n' in endpoint_url or '\r' in endpoint_url or '\t' in endpoint_url:
+        return False
     netloc = urlparse(endpoint_url).netloc
     return IPV6_ADDRZ_RE.match(netloc) is not None
 
@@ -990,6 +992,8 @@ def is_valid_endpoint_url(endpoint_url):
     :return: True if the endpoint url is valid. False otherwise.
 
     """
+    if '\n' in endpoint_url or '\r' in endpoint_url or '\t' in endpoint_url:
+        return False
     parts = urlsplit(endpoint_url)
     hostname = parts.hostname
     if hostname is None:

I suppose if you replaced that with a single regex, it might be good enough to add.

[mgorny]

For the record, this is affecting Gentoo users today as we're proactively backporting Python security fixes. Other distros may be doing the same, given Python upstream's long security response period.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.