fix(signing): reject malformed authorities, convert IDN hosts to A-labels

src/adcp/signing/canonical.py

@@ -12,7 +12,45 @@
 
 from collections.abc import Mapping
 from dataclasses import dataclass
-from urllib.parse import urlsplit, urlunsplit
+from urllib.parse import SplitResult, urlsplit, urlunsplit
+
+import idna
+
+from adcp.signing._idna_canonicalize import canonicalize_host
+
+#: Spec code for an authority the AdCP profile requires be rejected rather than
+#: canonicalized. Declared here rather than imported from ``errors`` so this
+#: module stays a leaf: ``errors`` may depend on canonicalization, never the
+#: reverse. ``verifier`` maps this onto the wire error at its boundary.
+REQUEST_TARGET_URI_MALFORMED = "request_target_uri_malformed"
+
+
+class TargetUriMalformedError(ValueError):
+    """A URL whose authority the profile requires be rejected.
+
+    Subclasses ``ValueError`` because every existing caller of the
+    canonicalizers already treats a bad URL as a ``ValueError``; carrying
+    ``.code`` lets the verifier boundary emit the spec code instead of
+    re-deriving one. The ``reason`` names which rule fired, so a rejection
+    message says what was wrong rather than restating "malformed".
+    """
+
+    code = REQUEST_TARGET_URI_MALFORMED
+
+    def __init__(self, subject: str, reason: str) -> None:
+        super().__init__(f"{reason}: {subject!r}")
+        self.reason = reason
+
+
+def host_has_raw_non_ascii(host: str) -> bool:
+    """Whether *host* carries raw non-ASCII bytes (an un-normalized U-label).
+
+    One definition, deliberately two call sites with two different codes: here
+    it selects the UTS-46 branch on the signing path, while the verifier's
+    header precheck uses it to reject a U-label arriving on the wire as
+    ``request_signature_header_malformed``. Share the predicate, never the code.
+    """
+    return not host.isascii()
 
 
 @dataclass(frozen=True)
@@ -80,7 +118,7 @@ def build_signature_base(
 
 def canonicalize_target_uri(url: str) -> str:
     """Produce the `@target-uri` derived-component value per AdCP profile."""
-    parts = urlsplit(url)
+    parts = _split_or_reject(url)
     scheme = parts.scheme.lower()
     netloc = _canon_authority(parts.netloc, scheme)
     path = _normalize_path(parts.path)
@@ -93,10 +131,25 @@ def canonicalize_target_uri(url: str) -> str:
 
 def canonicalize_authority(url: str) -> str:
     """Produce the `@authority` derived-component value per AdCP profile."""
-    parts = urlsplit(url)
+    parts = _split_or_reject(url)
     return _canon_authority(parts.netloc, parts.scheme.lower())
 
 
+def _split_or_reject(url: str) -> SplitResult:
+    """`urlsplit`, with its bare refusals normalized onto the typed error.
+
+    `urlsplit` rejects some malformed authorities itself -- `https://[::1/p`
+    among them -- with a plain `ValueError` carrying no code. That refusal is
+    correct but anonymous, so it is re-raised here as the same typed error the
+    authority gate raises. Without this, one of the six malformed-authority
+    vectors would "pass" on someone else's exception.
+    """
+    try:
+        return urlsplit(url)
+    except ValueError as exc:
+        raise TargetUriMalformedError(url, f"the URL could not be parsed ({exc})") from exc
+
+
 _DEFAULT_PORTS = {"http": 80, "https": 443}
 
 # RFC 3986 §2.3 unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
@@ -125,30 +178,149 @@ def _lookup(headers: Mapping[str, str], name_lower: str) -> str | None:
     return None
 
 
+def _malformed_authority_reason(authority: str) -> str | None:
+    """Why *authority* is malformed per the profile's steps 2-3, or `None`.
+
+    A reason rather than a bool, so every caller's rejection names the rule that
+    fired. *authority* is the raw netloc as received -- from a URL's `netloc`
+    here, from the as-received `Host` header at the verifier boundary; the rule
+    is identical on both and must not be written twice.
+
+    Note what is deliberately absent: a raw-non-ASCII-host rejection. This
+    module is shared by the signer and the verifier, and the signer is required
+    to CONVERT a U-label to its A-label form, not refuse it (see the
+    `idn-to-punycode` vector). Rejecting a U-label received on the wire is the
+    verifier's header precheck, which raises a different code at an earlier
+    step; it shares `host_has_raw_non_ascii` with this module rather than
+    re-deriving the test.
+    """
+    host = authority.rsplit("@", 1)[-1]  # step 3: strip userinfo before judging the host
+    if not host:
+        return "the authority carries no host (empty, or userinfo/port with nothing before it)"
+    if host.startswith("["):
+        return _bracketed_host_reason(host)
+    if host.count(":") > 1:
+        return "an IPv6 address outside brackets is ambiguous with a port and is malformed"
+    if not host.split(":", 1)[0]:
+        return "the authority carries a port but no host"
+    return None
+
+
+def _bracketed_host_reason(host: str) -> str | None:
+    """Step 2's two IPv6-literal rejections."""
+    end = host.find("]")
+    if end < 0:
+        return "a bracketed IPv6 host missing its closing bracket is malformed"
+    if "%" in host[1:end]:
+        return (
+            "an IPv6 zone identifier (RFC 6874) is node-local "
+            "and MUST be rejected in signed URLs"
+        )
+    return None
+
+
 def _canon_authority(netloc: str, scheme: str) -> str:
+    reason = _malformed_authority_reason(netloc)
+    if reason is not None:
+        raise TargetUriMalformedError(netloc, reason)
     if "@" in netloc:
         netloc = netloc.rsplit("@", 1)[1]
     host: str
     port: int | None = None
     if netloc.startswith("["):
         end = netloc.find("]")
-        if end < 0:
-            raise ValueError(f"unterminated IPv6 literal in authority: {netloc!r}")
+        if end < 0:  # pragma: no cover - _malformed_authority_reason rejects this first
+            raise TargetUriMalformedError(
+                netloc, "a bracketed IPv6 host missing its closing bracket"
+            )
         host = netloc[: end + 1]
         tail = netloc[end + 1 :]
         if tail.startswith(":"):
-            port = int(tail[1:])
+            port = _port_or_reject(tail[1:], netloc)
     elif ":" in netloc:
         host, portstr = netloc.rsplit(":", 1)
-        port = int(portstr)
+        port = _port_or_reject(portstr, netloc)
     else:
         host = netloc
-    host = host.lower()
+    host = _canon_host(host, netloc)
     if port is not None and port != _DEFAULT_PORTS.get(scheme):
         return f"{host}:{port}"
     return host
 
 
+_ASCII_DIGITS = frozenset("0123456789")
+
+
+def _port_or_reject(portstr: str, netloc: str) -> int | None:
+    """Parse a port per RFC 3986 §3.2.3 (`port = *DIGIT`), or reject.
+
+    The port used to go straight into `int()`, which is far more permissive
+    than the grammar and produced three distinct problems:
+
+    * `int("-80")` gave the authority `host:-80`, which is not an authority.
+    * `int("8_0")` is 80 -- Python accepts underscore digit separators.
+    * `int("٨٠")` is also 80 -- `int()` accepts non-ASCII digits, so
+      `host:٨٠` and `host:80` collapsed to the SAME canonical authority. A
+      peer that does not fold Arabic-Indic digits derives a different
+      `@authority` from identical bytes, and the signature fails for a reason
+      neither side can see in its own logs.
+
+    `str.isdigit()` does not close that last one -- `"٨٠".isdigit()` is True --
+    so the test is ASCII digits specifically.
+
+    An EMPTY port is legal and means "default": the grammar is `*DIGIT`, and
+    §3.2.3 says a normalizer should drop the port and its colon when empty. So
+    `https://host:/p` normalizes to `host` rather than being rejected.
+    """
+    if not portstr:
+        return None
+    if not all(ch in _ASCII_DIGITS for ch in portstr):
+        raise TargetUriMalformedError(
+            netloc, f"the port {portstr!r} is not a sequence of ASCII digits"
+        )
+    return int(portstr)
+
+
+def _canon_host(host: str, netloc: str) -> str:
+    """Lower-case an ASCII host, or convert a U-label to its A-label form.
+
+    The trailing FQDN-root dot is stripped BEFORE the branch, not inside it.
+    `canonicalize_host` strips it as part of UTS-46 preparation while a bare
+    `.lower()` keeps it, so branching first would make `example.com.` and
+    `bücher.example.` normalize differently -- a signer and a verifier reading
+    the same host in two spellings would derive two authorities and every
+    signature between them would fail. Stripping once, up front, is the only
+    place the two branches can be made to agree without re-implementing the
+    helper here and becoming another host normalizer in a tree that already has
+    six.
+
+    The ASCII fast path is load-bearing, not an optimization: `canonicalize_host`
+    strips IPv6 brackets and raises on underscore hosts and over-long labels,
+    all of which sign fine today.
+    """
+    if host.endswith("."):
+        host = host[:-1]
+    if not host or any(label == "" for label in host.split(".")):
+        # Re-checked AFTER the strip. `_malformed_authority_reason` runs on the
+        # raw netloc, where `.` and `..` are non-empty and look like hosts; it
+        # is only stripping the root dot that empties them. Without this,
+        # `https://./p` canonicalized to `https:///p` -- the empty authority
+        # this very module rejects two functions up, reached by a path that
+        # skipped the check.
+        raise TargetUriMalformedError(netloc, "the authority carries no host once normalized")
+    if not host_has_raw_non_ascii(host):
+        return host.lower()
+    try:
+        return canonicalize_host(host)
+    except (idna.IDNAError, UnicodeError) as exc:
+        # Fail closed. A signature base must never be computed over a host we
+        # could not canonicalize: the alternative is signing bytes the peer will
+        # derive differently. The permissive fallback used by some sibling
+        # callers is right for comparison paths, where raising would turn a
+        # mismatch into an outage; it is wrong here.
+        raise TargetUriMalformedError(netloc, f"the host is not a valid IDNA name ({exc})") from exc
+
+
 def _normalize_path(path: str) -> str:
     return _normalize_pct(_remove_dot_segments(path))
 

src/adcp/signing/errors.py

@@ -9,6 +9,8 @@
 
 from collections.abc import Mapping
 
+from adcp.signing.canonical import REQUEST_TARGET_URI_MALFORMED
+
 
 class SignatureVerificationError(Exception):
     """Raised when a request signature fails any step of the verifier checklist.
@@ -114,13 +116,21 @@ def __init__(
 WEBHOOK_SIGNATURE_KEY_ORIGIN_MISMATCH = "webhook_signature_key_origin_mismatch"
 WEBHOOK_SIGNATURE_KEY_ORIGIN_MISSING = "webhook_signature_key_origin_missing"
 
+# Structural code for a malformed authority on the webhook profile. Named
+# without the ``webhook_signature_`` prefix the rest of this family carries
+# because the spec names it that way: security.mdx's webhook checklist lists
+# ``webhook_target_uri_malformed`` in the error taxonomy and requires it at
+# step 10 for a malformed or mismatched authority.
+WEBHOOK_TARGET_URI_MALFORMED = "webhook_target_uri_malformed"
+
 # Code-family translation used by the webhook verifier wrapper. The verifier
 # pipeline raises request_signature_* codes; the wrapper retags them into
 # webhook_signature_* before exposing to callers. Keeps the 300-line verifier
 # unchanged and guarantees webhook routes never leak request-family codes.
 REQUEST_TO_WEBHOOK_CODE = {
     REQUEST_SIGNATURE_REQUIRED: WEBHOOK_SIGNATURE_REQUIRED,
     REQUEST_SIGNATURE_HEADER_MALFORMED: WEBHOOK_SIGNATURE_HEADER_MALFORMED,
+    REQUEST_TARGET_URI_MALFORMED: WEBHOOK_TARGET_URI_MALFORMED,
     REQUEST_SIGNATURE_PARAMS_INCOMPLETE: WEBHOOK_SIGNATURE_PARAMS_INCOMPLETE,
     REQUEST_SIGNATURE_TAG_INVALID: WEBHOOK_SIGNATURE_TAG_INVALID,
     REQUEST_SIGNATURE_ALG_NOT_ALLOWED: WEBHOOK_SIGNATURE_ALG_NOT_ALLOWED,

src/adcp/signing/verifier.py

@@ -15,6 +15,7 @@
 from typing import Any, Literal
 
 from adcp.signing.canonical import (
+    TargetUriMalformedError,
     _lookup,
     build_signature_base,
     parse_signature_input_header,
@@ -312,6 +313,17 @@ def verify_request_signature(
         base = build_signature_base(method=method, url=url, headers=headers, parsed=parsed).encode(
             "utf-8"
         )
+    except TargetUriMalformedError as exc:
+        # MUST precede the (ValueError, KeyError) clause below: TargetUriMalformedError
+        # IS a ValueError, so reordering these two silently downgrades every
+        # malformed-authority rejection to request_signature_header_malformed
+        # with the suite still green. test_target_uri_malformed_boundary.py
+        # guards the ordering.
+        raise SignatureVerificationError(
+            exc.code,
+            step=6,
+            message=f"signature base construction failed: {exc}",
+        ) from exc
     except (ValueError, KeyError) as exc:
         raise SignatureVerificationError(
             REQUEST_SIGNATURE_HEADER_MALFORMED,