Merge branch 'master' into add-rate-limit-middleware

Author: rodrigobnogueira

CHANGES/8.bugfix.rst

@@ -0,0 +1 @@
+Fixed ``DigestAuthMiddleware`` raising an ``IndexError`` on empty domain -- by :user:`Dreamsorcerer`.

CHANGES/9.bugfix.rst

@@ -0,0 +1,3 @@
+Fixed :class:`~aiohttp_client_middlewares.DigestAuthMiddleware` corrupting the
+``Digest`` challenge when a ``WWW-Authenticate`` response offered more than one
+authentication scheme -- by :user:`Dreamsorcerer`.

aiohttp_client_middlewares/digest_auth.py

@@ -54,27 +54,23 @@ class DigestAuthChallenge(TypedDict, total=False):
 
 # Compile the regex pattern once at module level for performance
 _HEADER_PAIRS_PATTERN = re.compile(
-    r'(?:^|\s|,\s*)(\w+)\s*=\s*(?:"((?:[^"\\]|\\.)*)"|([^\s,]+))'
+    r'(?:^|\s|,\s*)(\w+)(?:\s*=\s*(?:"((?:[^"\\]|\\.)*)"|([^\s,]+)))?'
     if sys.version_info < (3, 11)
-    else r'(?:^|\s|,\s*)((?>\w+))\s*=\s*(?:"((?:[^"\\]|\\.)*)"|([^\s,]+))'
-    # +------------|--------|--|-|-|--|----|------|----|--||-----|-> Match valid start/sep
-    #              +--------|--|-|-|--|----|------|----|--||-----|-> alphanumeric key (atomic
-    #                       |  | | |  |    |      |    |  ||     |   group reduces backtracking)
-    #                       +--|-|-|--|----|------|----|--||-----|-> maybe whitespace
-    #                          | | |  |    |      |    |  ||     |
-    #                          +-|-|--|----|------|----|--||-----|-> = (delimiter)
-    #                            +-|--|----|------|----|--||-----|-> maybe whitespace
-    #                              |  |    |      |    |  ||     |
-    #                              +--|----|------|----|--||-----|-> group quoted or unquoted
-    #                                 |    |      |    |  ||     |
-    #                                 +----|------|----|--||-----|-> if quoted...
-    #                                      +------|----|--||-----|-> anything but " or \
-    #                                             +----|--||-----|-> escaped characters allowed
-    #                                                  +--||-----|-> or can be empty string
-    #                                                     ||     |
-    #                                                     +|-----|-> if unquoted...
-    #                                                      +-----|-> anything but , or <space>
-    #                                                            +-> at least one char req'd
+    else r'(?:^|\s|,\s*)((?>\w+))(?:\s*=\s*(?:"((?:[^"\\]|\\.)*)"|([^\s,]+)))?'
+    # +------------|--------|--|--||--|--|----|------|---|---||-----|-> Match valid start/sep
+    #              +--------|--|--||--|--|----|------|---|---||-----|-> alphanumeric key (atomic group reduces backtracking)
+    #                       +--|--||--|--|----|------|---|---||-----|-> optional value; absent => bare auth-scheme token
+    #                          +--||--|--|----|------|---|---||-----|-> maybe whitespace
+    #                             +|--|--|----|------|---|---||-----|-> = (delimiter)
+    #                              +--|--|----|------|---|---||-----|-> maybe whitespace
+    #                                 +--|----|------|---|---||-----|-> group quoted or unquoted
+    #                                    +----|------|---|---||-----|-> if quoted...
+    #                                         +------|---|---||-----|-> anything but " or \
+    #                                                +---|---||-----|-> escaped characters allowed
+    #                                                    +---||-----|-> or can be empty string
+    #                                                        +|-----|-> if unquoted...
+    #                                                         +-----|-> anything but , or <space>
+    #                                                               +-> at least one char req'd
 )
 
 
@@ -119,12 +115,17 @@ def unescape_quotes(value: str) -> str:
 
 def parse_header_pairs(header: str) -> dict[str, str]:
     """
-    Parse key-value pairs from WWW-Authenticate or similar HTTP headers.
+    Parse key-value pairs from the first challenge of a WWW-Authenticate header.
 
     This function handles the complex format of WWW-Authenticate header values,
     supporting both quoted and unquoted values, proper handling of commas in
     quoted values, and whitespace variations per RFC 7616.
 
+    A single header may carry several challenges
+    (https://www.rfc-editor.org/rfc/rfc7235#section-4.1). Parsing
+    stops at the next auth-scheme token so a later challenge's parameters cannot
+    overwrite the first challenge's values; a leading scheme token is skipped.
+
     Examples of supported formats:
       - key1="value1", key2=value2
       - key1 = "value1" , key2="value, with, commas"
@@ -137,11 +138,21 @@ def parse_header_pairs(header: str) -> dict[str, str]:
     Returns:
         Dictionary mapping parameter names to their values
     """
-    return {
-        stripped_key: unescape_quotes(quoted_val) if quoted_val else unquoted_val
-        for key, quoted_val, unquoted_val in _HEADER_PAIRS_PATTERN.findall(header)
-        if (stripped_key := key.strip())
-    }
+    pairs: dict[str, str] = {}
+    for match in _HEADER_PAIRS_PATTERN.finditer(header):
+        key = match.group(1)
+        quoted_val, unquoted_val = match.group(2), match.group(3)
+        if quoted_val is None and unquoted_val is None:
+            # Bare token with no "=value": an auth-scheme name, not a parameter.
+            # Skip a leading scheme; once parameters exist, a new scheme marks
+            # the start of the next challenge, so stop here.
+            if pairs:
+                break
+            continue
+        pairs[key] = (
+            unescape_quotes(quoted_val) if quoted_val is not None else unquoted_val
+        )
+    return pairs
 
 
 class DigestAuthMiddleware:
@@ -436,21 +447,23 @@ def _authenticate(self, response: ClientResponse) -> bool:
 
         # Update protection space based on domain parameter or default to origin
         origin = response.url.origin()
+        self._protection_space = []
 
         if domain := self._challenge.get("domain"):
             # Parse space-separated list of URIs
-            self._protection_space = []
             for uri in domain.split():
                 # Remove quotes if present
                 uri = uri.strip('"')
+                if not uri:
+                    continue
                 if uri.startswith("/"):
                     # Path-absolute, relative to origin
                     self._protection_space.append(str(origin.join(URL(uri))))
                 else:
                     # Absolute URI
                     self._protection_space.append(str(URL(uri)))
-        else:
-            # No domain specified, protection space is entire origin
+
+        if not self._protection_space:
             self._protection_space = [str(origin)]
 
         # Return True only if we found at least one challenge parameter

tests/test_digest_auth.py

@@ -123,6 +123,18 @@ def mock_md5_digest() -> Generator[mock.MagicMock, None, None]:
             True,
             {"realm": "", "nonce": "abc", "qop": "auth"},
         ),
+        # Multi-scheme header: a second challenge (Basic) in the same
+        # WWW-Authenticate value must not overwrite the Digest realm/nonce.
+        # https://www.rfc-editor.org/rfc/rfc7235#section-4.1
+        (
+            401,
+            {
+                "www-authenticate": 'Digest realm="protected", nonce="n1", '
+                'qop="auth", Basic realm="other"'
+            },
+            True,
+            {"realm": "protected", "nonce": "n1", "qop": "auth"},
+        ),
         # Non-401 status
         (200, {}, False, {}),  # No challenge should be set
     ],
@@ -471,6 +483,18 @@ async def test_digest_response_exact_match(
         ),
         # Empty header
         ("", {}),
+        # Multi-scheme header: parsing stops at the next auth-scheme so a later
+        # challenge cannot overwrite the first's values.
+        # https://www.rfc-editor.org/rfc/rfc7235#section-4.1
+        (
+            'realm="protected", nonce="n1", qop="auth", Basic realm="other"',
+            {"realm": "protected", "nonce": "n1", "qop": "auth"},
+        ),
+        # Multi-scheme header including the leading scheme token
+        (
+            'Digest realm="protected", nonce="n1", Basic realm="other"',
+            {"realm": "protected", "nonce": "n1"},
+        ),
     ],
     ids=[
         "fully_quoted_header",
@@ -482,6 +506,8 @@ async def test_digest_response_exact_match(
         "escaped_quotes",
         "single_quotes_as_regular_chars",
         "empty_header",
+        "multi_scheme_second_challenge_ignored",
+        "multi_scheme_with_leading_scheme",
     ],
 )
 def test_parse_header_pairs(header: str, expected_result: dict[str, str]) -> None:
@@ -1499,6 +1525,74 @@ def test_in_protection_space_multiple_spaces(
     assert digest_auth_mw._in_protection_space(URL("http://example.com/other")) is False
 
 
+@pytest.mark.parametrize(
+    "domain_value",
+    (r'"\""', '"'),
+    ids=("quoted_escaped_quote", "bare_quote"),
+)
+def test_authenticate_domain_only_quote_does_not_poison_protection_space(
+    digest_auth_mw: DigestAuthMiddleware,
+    domain_value: str,
+) -> None:
+    response = mock.create_autospec(ClientResponse, spec_set=True, instance=True)
+    response.status = 401
+    response.url = URL("http://example.com/resource")
+    response.headers = {
+        "www-authenticate": f'Digest realm="test", nonce="abc", domain={domain_value}'
+    }
+
+    assert digest_auth_mw._authenticate(response) is True
+    assert digest_auth_mw._challenge["domain"] == '"'
+    assert digest_auth_mw._protection_space == ["http://example.com"]
+    assert "" not in digest_auth_mw._protection_space
+    # Must not raise IndexError and must still scope to the anchor origin.
+    assert digest_auth_mw._in_protection_space(URL("http://example.com/other")) is True
+    assert digest_auth_mw._in_protection_space(URL("http://other.com/x")) is False
+
+
+async def test_double_quote_domain_does_not_break_future_requests(
+    aiohttp_server: AiohttpServer,
+) -> None:
+    """End-to-end regression for a ``domain`` directive of just a double quote.
+
+    The first request triggers the challenge and authenticates on retry. Before
+    the fix, the bogus ``domain`` left an empty string in the protection space,
+    so the next request's preemptive-auth check raised ``IndexError``.
+    """
+    digest_auth_mw = DigestAuthMiddleware("user", "pass", preemptive=True)
+    auth_headers: list[str | None] = []
+
+    async def handler(request: Request) -> Response:
+        auth_headers.append(request.headers.get(hdrs.AUTHORIZATION))
+        if request.headers.get(hdrs.AUTHORIZATION) is None:
+            challenge = (
+                'Digest realm="test", nonce="abc123", qop="auth", '
+                'algorithm=MD5, domain="\\""'
+            )
+            return Response(
+                status=401,
+                headers={"WWW-Authenticate": challenge},
+                text="Unauthorized",
+            )
+        return Response(text="OK")
+
+    app = Application()
+    app.router.add_get("/path1", handler)
+    app.router.add_get("/path2", handler)
+    server = await aiohttp_server(app)
+
+    async with ClientSession(middlewares=(digest_auth_mw,)) as session:
+        async with session.get(server.make_url("/path1")) as resp:
+            assert resp.status == 200
+        # Previously raised IndexError inside the preemptive-auth check.
+        async with session.get(server.make_url("/path2")) as resp:
+            assert resp.status == 200
+
+    assert auth_headers[0] is None  # First request: no auth, gets challenge
+    assert auth_headers[1] is not None  # Retry carries the digest response
+    assert auth_headers[2] is not None  # Second request: preemptive auth
+
+
 async def test_case_sensitive_algorithm_server(
     aiohttp_server: AiohttpServer,
 ) -> None:
@@ -1565,5 +1659,6 @@ def test_regex_performance() -> None:
         f"Regex took {elapsed * 1000:.1f}ms, "
         f"expected <{REGEX_TIME_THRESHOLD_SECONDS * 1000:.0f}ms - potential ReDoS issue"
     )
-    # This example shouldn't produce a match either.
-    assert not matches
+    # The lone run of word characters matches as a bare auth-scheme token
+    # with empty value groups, so it never becomes a key=value pair.
+    assert all(not quoted and not unquoted for _, quoted, unquoted in matches)