Repository: RaRe-Technologies/smart_open
Pull request #633 (2021-08-03 21:31:43)

Work around changes to `urllib.parse.urlsplit`
#### Motivation

This attempts to fix the behaviour around question marks in URIs for S3 and GCS. `urllib.parse.urlsplit` is used and a hack was put in place to replace `?` with `\n` to avoid issues withs params splitting. However, the behaviour of `urllib.parse.urlsplit` has changed and now `\n`, `\t` and `\r` are all stripped from the URL before splitting.

https://bugs.python.org/issue43882

My solution is to instead use the 'null character'. This could technically appear in a GCS Blob name... but should it? To me this feels fairrrrrllllly safe?

#### Checklist

Before you create the PR, please make sure you have:

- [ x] Picked a concise, informative and complete title
- [ x] Clearly explained the motivation behind the PR
- [ x] Linked to any existing issues that your PR will be solving
- [ x] Included tests for any new functionality
- [ x] Checked that all unit tests pass

#### Workflow

Please avoid rebasing and force-pushing to the branch of the PR once a review is in progress.
Rebasing can make your commits look a bit cleaner, but it also makes life more difficult from the reviewer, because they are no longer able to distinguish between code that has already been reviewed, and unreviewed code.


mpenkov: Thank you for this contribution. I was wondering what's going on with those question marks... this PR makes it obvious where the problem was.

However, I'm not sure if using the null character is the best way around it. It's risky, because the same thing that happened to the newline can happen to the null: the library implementation can change (I wonder why they did this, but it's out of our control) and then we'll be back to where we started.

I think the most robust way to solve this is:

1. Check if the S3 URL contains the notorious question mark character
2. If it does, pick a random string of readable characters that **does not already occur in the URL**
3. Replace the question mark with that random string
4. Perform the split
5. Replace that random string with the question mark

I think that would be safe. What are your thoughts?
mpenkov: @judahrand I improved your PR slightly by replacing null with a configurable placeholder.

@piskvorky Can you please have a look at this?

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c1e572b..fc21fba 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,7 @@ You may continue to use `ignore_ext` parameter for now, but it will be deprecate
 - Add new top-level compression parameter (PR [#609](https://github.com/RaRe-Technologies/smart_open/pull/609), [@dmcguire81](https://github.com/dmcguire81))
 - Drop mock dependency; standardize on unittest.mock (PR [#621](https://github.com/RaRe-Technologies/smart_open/pull/621), [@musicinmybrain](https://github.com/musicinmybrain))
 - Fix to_boto3 method (PR [#619](https://github.com/RaRe-Technologies/smart_open/pull/619), [@mpenkov](https://github.com/mpenkov))
+- Work around changes to `urllib.parse.urlsplit` (PR [#633](https://github.com/RaRe-Technologies/smart_open/pull/633), [@judahrand](https://github.com/judahrand)
 
 # 5.0.0, 30 Mar 2021
 
diff --git a/smart_open/utils.py b/smart_open/utils.py
index 94bac9e..00ebd9e 100644
--- a/smart_open/utils.py
+++ b/smart_open/utils.py
@@ -14,6 +14,9 @@ import urllib.parse
 
 logger = logging.getLogger(__name__)
 
+WORKAROUND_SCHEMES = ['s3', 's3n', 's3u', 's3a', 'gs']
+QUESTION_MARK_PLACEHOLDER = '///smart_open.utils.QUESTION_MARK_PLACEHOLDER///'
+
 
 def inspect_kwargs(kallable):
     #
@@ -150,16 +153,34 @@ def safe_urlsplit(url):
     querystring separately.  Unfortunately, question marks can also appear
     _inside_ the actual URL for some schemas like S3, GS.
 
-    Replaces question marks with newlines prior to splitting.  This is safe because:
-
-    1. The standard library's urlsplit completely ignores newlines
-    2. Raw newlines will never occur in innocuous URLs.  They are always URL-encoded.
+    Replaces question marks with a special placeholder substring prior to
+    splitting.  This work-around behavior is disabled in the unlikely event the
+    placeholder is already part of the URL.  If this affects you, consider
+    changing the value of QUESTION_MARK_PLACEHOLDER to something more suitable.
 
     See Also
     --------
+    https://bugs.python.org/issue43882
     https://github.com/python/cpython/blob/3.7/Lib/urllib/parse.py
     https://github.com/RaRe-Technologies/smart_open/issues/285
     https://github.com/RaRe-Technologies/smart_open/issues/458
+    smart_open/utils.py:QUESTION_MARK_PLACEHOLDER
     """
-    sr = urllib.parse.urlsplit(url.replace('?', '\n'), allow_fragments=False)
-    return urllib.parse.SplitResult(sr.scheme, sr.netloc, sr.path.replace('\n', '?'), '', '')
+    sr = urllib.parse.urlsplit(url, allow_fragments=False)
+
+    placeholder = None
+    if sr.scheme in WORKAROUND_SCHEMES and '?' in url and QUESTION_MARK_PLACEHOLDER not in url:
+        #
+        # This is safe because people will _almost never_ use the below
+        # substring in a URL.  If they do, then they're asking for trouble,
+        # and this special handling will simply not happen for them.
+        #
+        placeholder = QUESTION_MARK_PLACEHOLDER
+        url = url.replace('?', placeholder)
+        sr = urllib.parse.urlsplit(url, allow_fragments=False)
+
+    if placeholder is None:
+        return sr
+
+    path = sr.path.replace(placeholder, '?')
+    return urllib.parse.SplitResult(sr.scheme, sr.netloc, path, '', '')


Test patch:
diff --git a/smart_open/tests/test_utils.py b/smart_open/tests/test_utils.py
index 60b7ec5..17eaf6b 100644
--- a/smart_open/tests/test_utils.py
+++ b/smart_open/tests/test_utils.py
@@ -5,8 +5,10 @@
 # This code is distributed under the terms and conditions
 # from the MIT License (MIT).
 #
-
 import unittest
+import urllib.parse
+
+import pytest
 
 import smart_open.utils
 
@@ -28,3 +30,17 @@ def test_check_kwargs():
     kwargs = {'client': 'foo', 'unsupported': 'bar', 'client_kwargs': 'boaz'}
     supported = smart_open.utils.check_kwargs(kallable, kwargs)
     assert supported == {'client': 'foo', 'client_kwargs': 'boaz'}
+
+
+@pytest.mark.parametrize(
+    'url,expected',
+    [
+        ('s3://bucket/key', ('s3', 'bucket', '/key', '', '')),
+        ('s3://bucket/key?', ('s3', 'bucket', '/key?', '', '')),
+        ('s3://bucket/???', ('s3', 'bucket', '/???', '', '')),
+        ('https://host/path?foo=bar', ('https', 'host', '/path', 'foo=bar', '')),
+    ]
+)
+def test_safe_urlsplit(url, expected):
+    actual = smart_open.utils.safe_urlsplit(url)
+    assert actual == urllib.parse.SplitResult(*expected)