Repository: aio-libs/yarl

Issue:
basic-auth user and password are converted to lowercase when passed as part of "host" to URL.build
### Describe the bug

user and password casing is preserved when using the constructor:

    >>> yarl.URL('httPS://usER:passWORD@hostNAME/paTH')
    URL('https://usER:passWORD@hostname/paTH')
    >>> yarl.URL('httPS://usER:passWORD@hostNAME/paTH').user
    'usER'

user and password casing is lost when using `URL.build`:

    >>> yarl.URL.build(scheme='httPS', host='usER:passWORD@hostNAME', path='/paTH')
    URL('httPS://user:password@hostname/paTH')
    >>> yarl.URL.build(scheme='httPS', host='usER:passWORD@hostNAME', path='/paTH').user
    'user'

Note that the user & password are extracted from the string passed as `host`.

passing user and password explicitly also preserves the casing:

    >>> yarl.URL.build(scheme='httPS', user='usER', password='passWORD', host='hostNAME', path='/paTH')
    URL('httPS://usER:passWORD@hostname/paTH')
    >>> yarl.URL.build(scheme='httPS', user='usER', password='passWORD', host='hostNAME', path='/paTH').user
    'usER'

I'd expect either user and password to preserve casing when passed into `host` (preferably) or them not being extracted into the `user` and `password` attributes and instead remain in the lowercase and encoded `host` attribute.

maybe caused by https://github.com/aio-libs/yarl/issues/386

converting the host to lowercase should be done after extracting username / password.

### To Reproduce

    >>> yarl.URL.build(scheme='httPS', host='usER:passWORD@hostNAME', path='/paTH')
    URL('httPS://user:password@hostname/paTH')
    >>> yarl.URL.build(scheme='httPS', host='usER:passWORD@hostNAME', path='/paTH').user
    'user'

### Expected behavior

    >>> yarl.URL.build(scheme='httPS', host='usER:passWORD@hostNAME', path='/paTH')
    URL('httPS://user:password@hostname/paTH')
    >>> yarl.URL.build(scheme='httPS', host='usER:passWORD@hostNAME', path='/paTH').user
    'usER'

### Logs/tracebacks

```python-traceback
N/A
```


### Python Version

```console
$ python --version
Python 3.10.10
```


### multidict Version

```console
$ python -m pip show multidict
Name: multidict
Version: 6.0.4
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /usr/lib/python3.10/site-packages
Requires: 
Required-by: aiohttp, httpie, yarl
```


### yarl Version

```console
$ python -m pip show yarl
Name: yarl
Version: 1.8.2
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl/
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /home/stefan/.local/lib/python3.10/site-packages
Requires: idna, multidict
Required-by: aiohttp
```


### OS

Arch Linux

### Additional context

_No response_

### Code of Conduct

- [X] I agree to follow the aio-libs Code of Conduct

Discussion:
webknjaz: @mjpieters do you want me to reject the release build, wait and include this PR?
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/aio-libs/yarl/pull/954?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) Report
Attention: `11 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`ceaad70`)](https://app.codecov.io/gh/aio-libs/yarl/commit/ceaad70896f186c1604fb5f12e2ce63138ea600f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) 0.00% compared to head [(`1c6ecff`)](https://app.codecov.io/gh/aio-libs/yarl/pull/954?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) 0.00%.
> Report is 3 commits behind head on master.

| [Files](https://app.codecov.io/gh/aio-libs/yarl/pull/954?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | Patch % | Lines |
|---|---|---|
| [yarl/\_url.py](https://app.codecov.io/gh/aio-libs/yarl/pull/954?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs#diff-eWFybC9fdXJsLnB5) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/aio-libs/yarl/pull/954?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) |

<details><summary>Additional details and impacted files</summary>


```diff
@@          Coverage Diff           @@
##           master    #954   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           4       4           
  Lines         772     782   +10     
======================================
- Misses        772     782   +10     
```

| [Flag](https://app.codecov.io/gh/aio-libs/yarl/pull/954/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | Coverage Δ | |
|---|---|---|
| [unit](https://app.codecov.io/gh/aio-libs/yarl/pull/954/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `0.00% <0.00%> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs#carryforward-flags-in-the-pull-request-comment) to find out more.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/aio-libs/yarl/pull/954?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs).

webknjaz: > ```console
> [towncrier-fragments]:13: : Spell check: hostnames: Started rejecting ASCII hostnames with invalid characters. For host strings that.
> ```

@mjpieters this can be fixed by adding "hostnames" to `docs/spelling_wordlist.txt`. Though, I hope to integrate `codespell` later that would replace `sphinxcontrib-spelling` and seems to have less false positives.
mjpieters: While implementing my PR I noticed that if you used a non-ASCII hostname with `@` or any other invalid character (so not unreserved, percent encoded or sub-delims), then `idna.encode()` would raise an exception. But when I tried to reproduce this behaviour, just now, I noticed **we swallow that exception**, because it is a `UnicodeError` subclass 🤦 .

I'm not sure why the encoder uses `except UnicodeError` with a fallback to `host.encode('idna').decode('ascii')`, because that completely defeats the valid codepoint check.

I can hoist the validation to apply to non-ASCII hostnames too but that means I need to explicitly test for all disallowed ASCII codepoint (instead of the negative character classes I use now).

I'll go search through the commit history to see if I cannot find out more here.
mjpieters: > @mjpieters this can be fixed by adding "hostnames" to `docs/spelling_wordlist.txt`. Though, I hope to integrate `codespell` later that would replace `sphinxcontrib-spelling` and seems to have less false positives.

:-D I already have that change staged locally :-P
webknjaz: @mjpieters so do you think it's worth putting into a separate release? The automation is there so it wouldn't take as long to make new releases from now on...
mjpieters: > Not sure why the changelog bot is failing to notice the towncrier fragment... Not sure why but it's safe to ignore for now.

It is looking for the files in the wrong location, `news/` instead of `CHANGES/`.
mjpieters: > @mjpieters do you want me to reject the release build, wait and include this PR?

I think it's fine to put this in a next release. I don't see this as urgent.
webknjaz: > > @mjpieters do you want me to reject the release build, wait and include this PR?
> 
> I think it's fine to put this in a next release. I don't see this as urgent.

Okay, will proceed with releasing, then. The build is half-way ready: https://github.com/aio-libs/yarl/actions/runs/6931722870.
mjpieters: > Okay, will proceed with releasing, then. The build is half-way ready: https://github.com/aio-libs/yarl/actions/runs/6931722870.

What a glorious deployment pipeline we have! 😁 
webknjaz: > > Not sure why the changelog bot is failing to notice the towncrier fragment... Not sure why but it's safe to ignore for now.
> 
> It is looking for the files in the wrong location, `news/` instead of `CHANGES/`.

Oh, that makes sense — this is because I moved the config to a dedicated file and haven't updates the bot to account for that. Thanks for pointing it out!
webknjaz: > > Okay, will proceed with releasing, then. The build is half-way ready: https://github.com/aio-libs/yarl/actions/runs/6931722870.
> 
> What a glorious deployment pipeline we have! 😁

Hopefully, I'll make it scalable and reuse for other projects. I have similar things in many other repos but they all have tiny differences which makes them hard to reuse as is.
I made the CI run tests from the exact sdists we publish and install the wheels instead of rebuilding from Git source in each job. Though, I broke codecov reporting along the way so ignore it for now, I'll fix it later, hopefully.
webknjaz: > Okay, will proceed with releasing, then. The build is half-way ready: https://github.com/aio-libs/yarl/actions/runs/6931722870.

It took a few reattempts to get the last bit right... But now we have v1.9.3:
- https://pypi.org/project/yarl/1.9.3/
- https://github.com/aio-libs/yarl/discussions/957
- https://github.com/aio-libs/yarl/releases/tag/v1.9.3
webknjaz: > Though, I broke codecov reporting along the way so ignore it for now, I'll fix it later, hopefully.

Fixed it. The coverage metrics are back up!
webknjaz: > I'm not sure why the encoder uses `except UnicodeError` with a fallback to `host.encode('idna').decode('ascii')`, because that completely defeats the valid codepoint check.

I checked the coverage reported before I broke it temporarily, and turns out that the line you're talking about is _the only one uncovered_ in the entire project: https://app.codecov.io/gh/aio-libs/yarl/commit/65333fab5f5b2bcce9fbb6aa810e9618fa10c6f0/blob/yarl/_url.py#L1176.

So I guess this might also need some extra extensive testing...
webknjaz: > the line you're talking about is _the only one uncovered_ in the entire project: [app.codecov.io/gh/aio-libs/yarl/commit/65333fab5f5b2bcce9fbb6aa810e9618fa10c6f0/blob/yarl/_url.py#L1176](https://app.codecov.io/gh/aio-libs/yarl/commit/65333fab5f5b2bcce9fbb6aa810e9618fa10c6f0/blob/yarl/_url.py#L1176).

This is no longer true since I implemented collecting Cython and MyPy coverage...

Patch:
diff --git a/CHANGES/880.bugfix.rst b/CHANGES/880.bugfix.rst
new file mode 100644
index 0000000..39a64ea
--- /dev/null
+++ b/CHANGES/880.bugfix.rst
@@ -0,0 +1,3 @@
+Started rejecting ASCII hostnames with invalid characters. For host strings that
+look like authority strings, the exception message includes advice on what to do
+instead -- by :user:`mjpieters`.
diff --git a/CHANGES/954.bugfix.rst b/CHANGES/954.bugfix.rst
new file mode 120000
index 0000000..e6d2884
--- /dev/null
+++ b/CHANGES/954.bugfix.rst
@@ -0,0 +1,1 @@
+880.bugfix.rst
\ No newline at end of file
diff --git a/docs/api.rst b/docs/api.rst
index 170df7f..b7b80cf 100644
--- a/docs/api.rst
+++ b/docs/api.rst
@@ -1037,19 +1037,20 @@ Default port substitution
 Cache control
 -------------
 
-IDNA conversion and IP Address parsing used for host encoding are quite expensive
-operations, that's why the ``yarl`` library caches these calls by storing
-last ``256`` results in the global LRU cache.
+IDNA conversion, host validation, and IP Address parsing used for host
+encoding are quite expensive operations, that's why the ``yarl``
+library caches these calls by storing last ``256`` results in the
+global LRU cache.
 
 .. function:: cache_clear()
 
-   Clear IDNA and IP Address caches.
+   Clear IDNA, host validation, and IP Address caches.
 
 
 .. function:: cache_info()
 
-   Return a dictionary with ``"idna_encode"``, ``"idna_decode"``, and
-   ``"ip_address"`` keys, each value
+   Return a dictionary with ``"idna_encode"``, ``"idna_decode"``, ``"ip_address"``,
+   and ``"host_validate"`` keys, each value
    points to corresponding ``CacheInfo`` structure (see :func:`functools.lru_cache` for
    details):
 
@@ -1059,12 +1060,15 @@ last ``256`` results in the global LRU cache.
       >>> yarl.cache_info()
       {'idna_encode': CacheInfo(hits=5, misses=5, maxsize=256, currsize=5),
        'idna_decode': CacheInfo(hits=24, misses=15, maxsize=256, currsize=15),
-       'ip_address': CacheInfo(hits=46933, misses=84, maxsize=256, currsize=101)}
+       'ip_address': CacheInfo(hits=46933, misses=84, maxsize=256, currsize=101),
+       'host_validate': CacheInfo(hits=0, misses=0, maxsize=256, currsize=0)}
 
 
-.. function:: cache_configure(*, idna_encode_size=256, idna_decode_size=256, ip_address_size=256)
 
-   Set the IP Address and IDNA encode and decode cache sizes (``256`` for each by default).
+.. function:: cache_configure(*, idna_encode_size=256, idna_decode_size=256, ip_address_size=256, host_validate_size=256)
+
+   Set the IP Address, host validation, and IDNA encode and
+   decode cache sizes (``256`` for each by default).
 
    Pass ``None`` to make the corresponding cache unbounded (may speed up host encoding
    operation a little but the memory footprint can be very high,
diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt
index 6b90d22..8e66ca9 100644
--- a/docs/spelling_wordlist.txt
+++ b/docs/spelling_wordlist.txt
@@ -27,6 +27,7 @@ facto
 glibc
 google
 hardcoded
+hostnames
 macOS
 mailto
 manylinux
diff --git a/yarl/_url.py b/yarl/_url.py
index 37e4739..25988f8 100644
--- a/yarl/_url.py
+++ b/yarl/_url.py
@@ -1,4 +1,5 @@
 import math
+import re
 import sys
 import warnings
 from collections.abc import Mapping, Sequence
@@ -45,6 +46,22 @@ SCHEME_REQUIRES_HOST = frozenset(("http", "https", "ws", "wss", "ftp"))
 
 sentinel = object()
 
+# reg-name: unreserved / pct-encoded / sub-delims
+# this pattern matches anything that is *not* in those classes. and is only used
+# on lower-cased ASCII values.
+_not_reg_name = re.compile(
+    r"""
+        # any character not in the unreserved or sub-delims sets, plus %
+        # (validated with the additional check for pct-encoded sequences below)
+        [^a-z0-9\-._~!$&'()*+,;=%]
+    |
+        # % only allowed if it is part of a pct-encoded
+        # sequence of 2 hex digits.
+        %(?![0-9a-f]{2})
+    """,
+    re.VERBOSE,
+)
+
 SimpleQuery = Union[str, int, float]
 QueryVariable = Union[SimpleQuery, "Sequence[SimpleQuery]"]
 Query = Union[
@@ -64,6 +81,7 @@ class CacheInfo(TypedDict):
     idna_encode: _CacheInfo
     idna_decode: _CacheInfo
     ip_address: _CacheInfo
+    host_validate: _CacheInfo
 
 
 class _SplitResultDict(TypedDict, total=False):
@@ -269,7 +287,7 @@ class URL:
                         raise ValueError(msg)
                     else:
                         host = ""
-                host = cls._encode_host(host)
+                host = cls._encode_host(host, validate_host=False)
                 raw_user = None if username is None else cls._REQUOTER(username)
                 raw_password = None if password is None else cls._REQUOTER(password)
                 netloc = cls._make_netloc(
@@ -959,7 +977,9 @@ class URL:
         return prefix + "/".join(_normalize_path_segments(segments))
 
     @classmethod
-    def _encode_host(cls, host: str, human: bool = False) -> str:
+    def _encode_host(
+        cls, host: str, human: bool = False, validate_host: bool = True
+    ) -> str:
         if "%" in host:
             raw_ip, sep, zone = host.partition("%")
         else:
@@ -999,11 +1019,18 @@ class URL:
                 return host
 
         host = host.lower()
+        if human:
+            return host
+
         # IDNA encoding is slow,
         # skip it for ASCII-only strings
         # Don't move the check into _idna_encode() helper
         # to reduce the cache size
-        if human or host.isascii():
+        if host.isascii():
+            # Check for invalid characters explicitly; _idna_encode() does this
+            # for non-ascii host names.
+            if validate_host:
+                _host_validate(host)
             return host
         return _idna_encode(host)
 
@@ -1559,12 +1586,31 @@ def _ip_compressed_version(raw_ip: str) -> Tuple[str, int]:
     return ip.compressed, ip.version
 
 
+@lru_cache(_MAXCACHE)
+def _host_validate(host: str) -> None:
+    """Validate an ascii host name."""
+    invalid = _not_reg_name.search(host)
+    if invalid is None:
+        return
+    value, pos, extra = invalid.group(), invalid.start(), ""
+    if value == "@" or (value == ":" and "@" in host[pos:]):
+        # this looks like an authority string
+        extra = (
+            ", if the value includes a username or password, "
+            "use 'authority' instead of 'host'"
+        )
+    raise ValueError(
+        f"Host {host!r} cannot contain {value!r} (at position " f"{pos}){extra}"
+    ) from None
+
+
 @rewrite_module
 def cache_clear() -> None:
     """Clear all LRU caches."""
     _idna_decode.cache_clear()
     _idna_encode.cache_clear()
     _ip_compressed_version.cache_clear()
+    _host_validate.cache_clear()
 
 
 @rewrite_module
@@ -1574,6 +1620,7 @@ def cache_info() -> CacheInfo:
         "idna_encode": _idna_encode.cache_info(),
         "idna_decode": _idna_decode.cache_info(),
         "ip_address": _ip_compressed_version.cache_info(),
+        "host_validate": _host_validate.cache_info(),
     }
 
 
@@ -1583,12 +1630,14 @@ def cache_configure(
     idna_encode_size: Union[int, None] = _MAXCACHE,
     idna_decode_size: Union[int, None] = _MAXCACHE,
     ip_address_size: Union[int, None] = _MAXCACHE,
+    host_validate_size: Union[int, None] = _MAXCACHE,
 ) -> None:
     """Configure LRU cache sizes."""
-    global _idna_decode, _idna_encode, _ip_compressed_version
+    global _idna_decode, _idna_encode, _ip_compressed_version, _host_validate
 
     _idna_encode = lru_cache(idna_encode_size)(_idna_encode.__wrapped__)
     _idna_decode = lru_cache(idna_decode_size)(_idna_decode.__wrapped__)
     _ip_compressed_version = lru_cache(ip_address_size)(
         _ip_compressed_version.__wrapped__
     )
+    _host_validate = lru_cache(host_validate_size)(_host_validate.__wrapped__)


Test patch:
diff --git a/tests/test_cache.py b/tests/test_cache.py
index 46d5b01..9b0e75a 100644
--- a/tests/test_cache.py
+++ b/tests/test_cache.py
@@ -13,7 +13,7 @@ def test_cache_clear() -> None:
 
 def test_cache_info() -> None:
     info = yarl.cache_info()
-    assert info.keys() == {"idna_encode", "idna_decode", "ip_address"}
+    assert info.keys() == {"idna_encode", "idna_decode", "ip_address", "host_validate"}
 
 
 def test_cache_configure_default() -> None:
@@ -22,11 +22,17 @@ def test_cache_configure_default() -> None:
 
 def test_cache_configure_None() -> None:
     yarl.cache_configure(
-        idna_encode_size=None, idna_decode_size=None, ip_address_size=None
+        idna_encode_size=None,
+        idna_decode_size=None,
+        ip_address_size=None,
+        host_validate_size=None,
     )
 
 
 def test_cache_configure_explicit() -> None:
     yarl.cache_configure(
-        idna_encode_size=128, idna_decode_size=128, ip_address_size=128
+        idna_encode_size=128,
+        idna_decode_size=128,
+        ip_address_size=128,
+        host_validate_size=128,
     )
diff --git a/tests/test_url.py b/tests/test_url.py
index 5115c87..49245b8 100644
--- a/tests/test_url.py
+++ b/tests/test_url.py
@@ -2058,3 +2058,14 @@ def test_parsing_populates_cache():
     assert url._cache["raw_query_string"] == "a=b"
     assert url._cache["raw_fragment"] == "frag"
     assert url._cache["scheme"] == "http"
+
+
+@pytest.mark.parametrize(
+    ("host", "is_authority"),
+    [
+        *(("other_gen_delim_" + c, False) for c in "[]"),
+    ],
+)
+def test_build_with_invalid_ipv6_host(host: str, is_authority: bool):
+    with pytest.raises(ValueError, match="Invalid IPv6 URL"):
+        URL(f"http://{host}/")
diff --git a/tests/test_url_build.py b/tests/test_url_build.py
index b6675c2..0b7b9cd 100644
--- a/tests/test_url_build.py
+++ b/tests/test_url_build.py
@@ -120,6 +120,25 @@ def test_build_with_authority_and_host():
         URL.build(authority="host.com", host="example.com")
 
 
+@pytest.mark.parametrize(
+    ("host", "is_authority"),
+    [
+        ("user:pass@host.com", True),
+        ("user@host.com", True),
+        ("host:com", False),
+        ("not_percent_encoded%Zf", False),
+        ("still_not_percent_encoded%fZ", False),
+        *(("other_gen_delim_" + c, False) for c in "/?#[]"),
+    ],
+)
+def test_build_with_invalid_host(host: str, is_authority: bool):
+    match = r"Host '[^']+' cannot contain '[^']+' \(at position \d+\)"
+    if is_authority:
+        match += ", if .* use 'authority' instead of 'host'"
+    with pytest.raises(ValueError, match=f"{match}$"):
+        URL.build(host=host)
+
+
 def test_build_with_authority():
     url = URL.build(scheme="http", authority="степан:bar@host.com:8000", path="path")
     assert (
diff --git a/tests/test_url_update_netloc.py b/tests/test_url_update_netloc.py
index 295d620..cdb07a2 100644
--- a/tests/test_url_update_netloc.py
+++ b/tests/test_url_update_netloc.py
@@ -171,6 +171,26 @@ def test_with_host_non_ascii():
     assert url2.authority == "оун-упа.укр:123"
 
 
+@pytest.mark.parametrize(
+    ("host", "is_authority"),
+    [
+        ("user:pass@host.com", True),
+        ("user@host.com", True),
+        ("host:com", False),
+        ("not_percent_encoded%Zf", False),
+        ("still_not_percent_encoded%fZ", False),
+        *(("other_gen_delim_" + c, False) for c in "/?#[]"),
+    ],
+)
+def test_with_invalid_host(host: str, is_authority: bool):
+    url = URL("http://example.com:123")
+    match = r"Host '[^']+' cannot contain '[^']+' \(at position \d+\)"
+    if is_authority:
+        match += ", if .* use 'authority' instead of 'host'"
+    with pytest.raises(ValueError, match=f"{match}$"):
+        url.with_host(host=host)
+
+
 def test_with_host_percent_encoded():
     url = URL("http://%25cf%2580%cf%80:%25cf%2580%cf%80@example.com:123")
     url2 = url.with_host("%cf%80.org")