Cleanup URL percent-encoding behavior.

httpx/_urlparse.py

@@ -263,7 +263,7 @@ def urlparse(url: str = "", **kwargs: typing.Optional[str]) -> ParseResult:
     # We also exclude '/' because it is more robust to replace it with a percent
     # encoding despite it not being a requirement of the spec.
     parsed_query: typing.Optional[str] = (
-        None if query is None else quote(query, safe=SUB_DELIMS + ":?[]@")
+        None if query is None else quote(query, safe=SUB_DELIMS + ":/?[]@")
     )
     # For 'fragment' we can include all of the GEN_DELIMS set.
     parsed_fragment: typing.Optional[str] = (
@@ -432,13 +432,12 @@ def is_safe(string: str, safe: str = "/") -> bool:
         if char not in NON_ESCAPED_CHARS:
             return False
 
-    # Any '%' characters must be valid '%xx' escape sequences.
-    return string.count("%") == len(PERCENT_ENCODED_REGEX.findall(string))
+    return True
 
 
-def quote(string: str, safe: str = "/") -> str:
+def percent_encoded(string: str, safe: str = "/") -> str:
     """
-    Use percent-encoding to quote a string if required.
+    Use percent-encoding to quote a string.
     """
     if is_safe(string, safe=safe):
         return string
@@ -449,6 +448,32 @@ def quote(string: str, safe: str = "/") -> str:
     )
 
 
+def quote(string: str, safe: str = "/") -> str:
+    """
+    Use percent-encoding to quote a string, omitting existing '%xx' escape sequences.
+    """
+    parts = []
+    current_position = 0
+    for match in re.finditer(PERCENT_ENCODED_REGEX, string):
+        start_position, end_position = match.start(), match.end()
+        matched_text = match.group(0)
+        # Add any text up to the '%xx' escape sequence.
+        if start_position != current_position:
+            leading_text = string[current_position:start_position]
+            parts.append(percent_encoded(leading_text, safe=safe))
+
+        # Add the '%xx' escape sequence.
+        parts.append(matched_text)
+        current_position = end_position
+
+    # Add any text after the final '%xx' escape sequence.
+    if current_position != len(string):
+        trailing_text = string[current_position:]
+        parts.append(percent_encoded(trailing_text, safe=safe))
+
+    return "".join(parts)
+
+
 def urlencode(items: typing.List[typing.Tuple[str, str]]) -> str:
     """
     We can use a much simpler version of the stdlib urlencode here because
@@ -464,4 +489,9 @@ def urlencode(items: typing.List[typing.Tuple[str, str]]) -> str:
     - https://github.com/encode/httpx/issues/2721
     - https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlencode
     """
-    return "&".join([quote(k, safe="") + "=" + quote(v, safe="") for k, v in items])
+    return "&".join(
+        [
+            percent_encoded(k, safe="") + "=" + percent_encoded(v, safe="")
+            for k, v in items
+        ]
+    )

tests/models/test_url.py

@@ -70,56 +70,92 @@ def test_url_no_authority():
 # Tests for percent encoding across path, query, and fragment...
 
 
-def test_path_percent_encoding():
-    # Test percent encoding for SUB_DELIMS ALPHA NUM and allowable GEN_DELIMS
-    url = httpx.URL("https://example.com/!$&'()*+,;= abc ABC 123 :/[]@")
-    assert url.raw_path == b"/!$&'()*+,;=%20abc%20ABC%20123%20:/[]@"
-    assert url.path == "/!$&'()*+,;= abc ABC 123 :/[]@"
-    assert url.query == b""
-    assert url.fragment == ""
-
-
-def test_query_percent_encoding():
-    # Test percent encoding for SUB_DELIMS ALPHA NUM and allowable GEN_DELIMS
-    url = httpx.URL("https://example.com/?!$&'()*+,;= abc ABC 123 :/[]@" + "?")
-    assert url.raw_path == b"/?!$&'()*+,;=%20abc%20ABC%20123%20:%2F[]@?"
-    assert url.path == "/"
-    assert url.query == b"!$&'()*+,;=%20abc%20ABC%20123%20:%2F[]@?"
-    assert url.fragment == ""
-
-
-def test_fragment_percent_encoding():
-    # Test percent encoding for SUB_DELIMS ALPHA NUM and allowable GEN_DELIMS
-    url = httpx.URL("https://example.com/#!$&'()*+,;= abc ABC 123 :/[]@" + "?#")
-    assert url.raw_path == b"/"
-    assert url.path == "/"
-    assert url.query == b""
-    assert url.fragment == "!$&'()*+,;= abc ABC 123 :/[]@?#"
+@pytest.mark.parametrize(
+    "url,raw_path,path,query,fragment",
+    [
+        # URL with unescaped chars in path.
+        (
+            "https://example.com/!$&'()*+,;= abc ABC 123 :/[]@",
+            b"/!$&'()*+,;=%20abc%20ABC%20123%20:/[]@",
+            "/!$&'()*+,;= abc ABC 123 :/[]@",
+            b"",
+            "",
+        ),
+        # URL with escaped chars in path.
+        (
+            "https://example.com/!$&'()*+,;=%20abc%20ABC%20123%20:/[]@",
+            b"/!$&'()*+,;=%20abc%20ABC%20123%20:/[]@",
+            "/!$&'()*+,;= abc ABC 123 :/[]@",
+            b"",
+            "",
+        ),
+        # URL with mix of unescaped and escaped chars in path.
+        # WARNING: This has the incorrect behaviour, adding the test as an interim step.
+        (
+            "https://example.com/ %61%62%63",
+            b"/%20%61%62%63",
+            "/ abc",
+            b"",
+            "",
+        ),
+        # URL with unescaped chars in query.
+        (
+            "https://example.com/?!$&'()*+,;= abc ABC 123 :/[]@?",
+            b"/?!$&'()*+,;=%20abc%20ABC%20123%20:/[]@?",
+            "/",
+            b"!$&'()*+,;=%20abc%20ABC%20123%20:/[]@?",
+            "",
+        ),
+        # URL with escaped chars in query.
+        (
+            "https://example.com/?!$&%27()*+,;=%20abc%20ABC%20123%20:%2F[]@?",
+            b"/?!$&%27()*+,;=%20abc%20ABC%20123%20:%2F[]@?",
+            "/",
+            b"!$&%27()*+,;=%20abc%20ABC%20123%20:%2F[]@?",
+            "",
+        ),
+        # URL with mix of unescaped and escaped chars in query.
+        (
+            "https://example.com/?%20%97%98%99",
+            b"/?%20%97%98%99",
+            "/",
+            b"%20%97%98%99",
+            "",
+        ),
+        # URL encoding characters in fragment.
+        (
+            "https://example.com/#!$&'()*+,;= abc ABC 123 :/[]@?#",
+            b"/",
+            "/",
+            b"",
+            "!$&'()*+,;= abc ABC 123 :/[]@?#",
+        ),
+    ],
+)
+def test_path_query_fragment(url, raw_path, path, query, fragment):
+    url = httpx.URL(url)
+    assert url.raw_path == raw_path
+    assert url.path == path
+    assert url.query == query
+    assert url.fragment == fragment
 
 
 def test_url_query_encoding():
     """
-    URL query parameters should use '%20' to encoding spaces,
+    URL query parameters should use '%20' for encoding spaces,
     and should treat '/' as a safe character. This behaviour differs
     across clients, but we're matching browser behaviour here.
 
     See https://github.com/encode/httpx/issues/2536
     and https://github.com/encode/httpx/discussions/2460
     """
     url = httpx.URL("https://www.example.com/?a=b c&d=e/f")
-    assert url.raw_path == b"/?a=b%20c&d=e%2Ff"
+    assert url.raw_path == b"/?a=b%20c&d=e/f"
 
     url = httpx.URL("https://www.example.com/", params={"a": "b c", "d": "e/f"})
     assert url.raw_path == b"/?a=b%20c&d=e%2Ff"
 
 
-def test_url_with_url_encoded_path():
-    url = httpx.URL("https://www.example.com/path%20to%20somewhere")
-    assert url.path == "/path to somewhere"
-    assert url.query == b""
-    assert url.raw_path == b"/path%20to%20somewhere"
-
-
 def test_url_params():
     url = httpx.URL("https://example.org:123/path/to/somewhere", params={"a": "123"})
     assert str(url) == "https://example.org:123/path/to/somewhere?a=123"