From f1bc4613db0e437b8225a29226c7df586417f0dc Mon Sep 17 00:00:00 2001 From: "Constantine.mirin" Date: Wed, 29 Jul 2026 16:18:09 +0200 Subject: [PATCH 1/3] fix(signing): reject malformed authorities, convert IDN hosts to A-labels Six authority shapes the profile requires be rejected were canonicalized instead, and `request_target_uri_malformed` was not implemented at all. The signing path also never called the package's own host canonicalizer, so an IDN authority signed under a raw U-label. Rejections now live inside `_canon_authority`, so `@target-uri` and `@authority` inherit the rule once rather than twice. `urlsplit`'s own bare `ValueError` is normalized onto the same typed error -- without that, the missing-closing-bracket vector passes on someone else's exception and grades nothing. The predicate is ported from a downstream implementation minus its raw-non-ASCII-host branch: this module is shared by signer and verifier, and the signer must CONVERT a U-label, not refuse it. Rejecting a U-label arriving on the wire is the verifier's step-1 precheck, which raises a different code at an earlier step and shares `host_has_raw_non_ascii` rather than re-deriving it. Host canonicalization reuses `_idna_canonicalize.canonicalize_host` behind an ASCII fast path. The fast path is load-bearing, not an optimization: the helper strips IPv6 brackets and raises on underscore hosts and over-long labels, all of which sign correctly today. Retires eight strict-xfail ledger entries. `trailing-empty-query-preserved` (#979) stays. BEHAVIOUR CHANGES, both wire-visible: - A trailing FQDN-root dot is now stripped: `https://example.com./p` derives authority `example.com`, not `example.com.`. The spec does not define root-dot handling and ships no vector for it; stripping once before the ASCII/IDNA branch is what stops the two branches disagreeing. Pinned by an SDK-local test until upstream rules. - Authority-less URIs (`mailto:`, relative paths) now raise where they round-tripped unchanged. `request_target_uri_malformed` retags onto the existing `webhook_signature_header_malformed` rather than minting a webhook twin: no spec text, schema, or vector anywhere defines one. Fixes #978. Partially fixes #977 (signer half). --- src/adcp/signing/canonical.py | 143 ++++++++++++- src/adcp/signing/errors.py | 10 + src/adcp/signing/verifier.py | 12 ++ .../signing/test_canonicalization.py | 91 ++++++-- .../test_target_uri_malformed_boundary.py | 196 ++++++++++++++++++ .../signing/test_verifier_vectors.py | 10 +- 6 files changed, 436 insertions(+), 26 deletions(-) create mode 100644 tests/conformance/signing/test_target_uri_malformed_boundary.py diff --git a/src/adcp/signing/canonical.py b/src/adcp/signing/canonical.py index ffbea961..88033f9e 100644 --- a/src/adcp/signing/canonical.py +++ b/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,15 +178,61 @@ 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(":"): @@ -143,12 +242,44 @@ def _canon_authority(netloc: str, scheme: str) -> str: port = int(portstr) 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 +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_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)) diff --git a/src/adcp/signing/errors.py b/src/adcp/signing/errors.py index 801f2138..05a40051 100644 --- a/src/adcp/signing/errors.py +++ b/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. @@ -121,6 +123,14 @@ def __init__( REQUEST_TO_WEBHOOK_CODE = { REQUEST_SIGNATURE_REQUIRED: WEBHOOK_SIGNATURE_REQUIRED, REQUEST_SIGNATURE_HEADER_MALFORMED: WEBHOOK_SIGNATURE_HEADER_MALFORMED, + # Deliberately NOT a new `webhook_target_uri_malformed`. The AdCP spec + # defines `request_target_uri_malformed` (it is graded by the shipped + # canonicalization vectors) but names no webhook twin, and no bundled + # schema enumerates webhook signing codes. Minting one here would put a + # string on the wire that nothing upstream defines. A malformed target URI + # is the same failure class as a malformed header, so it retags onto the + # existing code until the spec says otherwise. + REQUEST_TARGET_URI_MALFORMED: WEBHOOK_SIGNATURE_HEADER_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, diff --git a/src/adcp/signing/verifier.py b/src/adcp/signing/verifier.py index e813896e..0256cef7 100644 --- a/src/adcp/signing/verifier.py +++ b/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, diff --git a/tests/conformance/signing/test_canonicalization.py b/tests/conformance/signing/test_canonicalization.py index 077479a1..0110bac2 100644 --- a/tests/conformance/signing/test_canonicalization.py +++ b/tests/conformance/signing/test_canonicalization.py @@ -30,27 +30,9 @@ # the issue tracking each gap. Marked strict so a fix XPASSes and forces the # entry to be retired instead of lingering. KNOWN_CANONICALIZATION_GAPS: dict[str, str] = { - # No UTS-46 A-label conversion on the signing path. adcp.signing has a - # canonicalize_host() helper, but canonical.py does not call it, so an IDN - # authority signs and verifies under a non-ASCII host. - "idn-to-punycode": "#977: IDN host is not converted to a Punycode A-label", - "idn-mixed-case-to-punycode": "#977: IDN host is not converted to a Punycode A-label", # urlunsplit() cannot distinguish "no query" from "empty query", so the # trailing '?' is dropped and signer/verifier can disagree on the base. "trailing-empty-query-preserved": "#979: trailing '?' with an empty query is dropped", - # Malformed authorities are canonicalized rather than rejected; the spec's - # request_target_uri_malformed code is not implemented at all. - "malformed-port-without-host": "#978: authority with a port but no host is accepted", - "malformed-userinfo-without-host": "#978: authority with userinfo but no host is accepted", - "malformed-empty-authority": "#978: empty authority is accepted", - "malformed-bare-ipv6": "#978: unbracketed IPv6 literal is accepted", - "malformed-ipv6-zone-identifier": "#978: RFC 6874 zone identifier is accepted", - # This one is refused today, but by urlsplit() rather than by us: a bare - # ValueError carrying no code. It belongs in the ledger with its five - # siblings so all six reject cases retire together under #978. - "malformed-ipv6-missing-closing-bracket": ( - "#978: refused by urlsplit with a bare ValueError that carries no error code" - ), } @@ -132,6 +114,79 @@ def test_canonicalization_case(name: str, case: dict[str, Any]) -> None: ), f"{name}: @authority mismatch for {url!r} ({case['rule']})" +# ---- trailing FQDN-root dot: the two host branches must stay symmetric ---- + +# `bücher.example` is the U-label form; `xn--bcher-kva.example` is its A-label +# form. Same host, two spellings -- the canonical authority must not depend on +# which one arrived. +_U_LABEL_HOST = "bücher.example" +_A_LABEL_HOST = "xn--bcher-kva.example" + + +@pytest.mark.parametrize( + ("url", "expected_authority"), + [ + # ASCII branch: already an A-label, root dot present. + (f"https://{_A_LABEL_HOST}./p", _A_LABEL_HOST), + # Non-ASCII branch: U-label, root dot present. + (f"https://{_U_LABEL_HOST}./p", _A_LABEL_HOST), + # Root dot in front of a non-default port -- the dot belongs to the + # host, not to the netloc, so stripping it must not eat the port. + (f"https://{_U_LABEL_HOST}.:8443/p", f"{_A_LABEL_HOST}:8443"), + (f"https://{_A_LABEL_HOST}.:8443/p", f"{_A_LABEL_HOST}:8443"), + ], + ids=["ascii-root-dot", "u-label-root-dot", "u-label-root-dot-port", "ascii-root-dot-port"], +) +def test_trailing_root_dot_is_stripped_on_both_host_branches( + url: str, expected_authority: str +) -> None: + """A trailing FQDN-root dot is stripped identically on both host branches. + + No case in `canonicalization.json` carries a root dot, so nothing upstream + pins this. It matters because the two branches disagree by construction: the + ASCII path lowercases in place, while the IDNA helper + (`_idna_canonicalize.canonicalize_host`) strips one trailing dot as part of + its UTS-46 preparation. Left unguarded, `https://example.com./p` would keep + its dot while `https://bücher.example./p` lost one -- a signer emitting the + A-label form and a verifier reading a Host-header U-label would then compute + different `@authority` values for the same host, and every signature between + them would fail to verify. + + The rule this pins: strip the root dot ONCE, before choosing a branch, so + both branches agree -- and agree with `canonicalize_host`, the package's + designated host normalizer. Handling the dot inside `canonical.py` instead + would make it the seventh host normalizer in the tree, which is the very + duplication this change exists to remove. + + This is deliberately wire-visible: `https://example.com./p` now derives + `example.com` where it derived `example.com.` before, so a signer on this + SDK and a verifier on an older one disagree for FQDN-root URLs. The AdCP + spec does not define root-dot handling and ships no vector for it; see the + tracking issue referenced in the changelog. When upstream rules, this test + is superseded by a real vector. + """ + assert canonicalize_authority(url) == expected_authority + assert canonicalize_target_uri(url) == f"https://{expected_authority}/p" + + +def test_root_dot_is_not_observable_in_the_canonical_authority() -> None: + """Dotted and undotted forms converge, on either branch. + + Stated as a relationship rather than a literal so it keeps holding if the + A-label spelling ever changes, and so it catches a fix that special-cases + the dotted form instead of handling the dot uniformly. + """ + for host in (_U_LABEL_HOST, _A_LABEL_HOST): + undotted = canonicalize_authority(f"https://{host}/p") + dotted = canonicalize_authority(f"https://{host}./p") + assert dotted == undotted, f"root-dot handling diverges for {host!r}" + + # ...and both spellings converge on the same canonical authority. + assert canonicalize_authority(f"https://{_U_LABEL_HOST}./p") == canonicalize_authority( + f"https://{_A_LABEL_HOST}./p" + ) + + def test_multi_label_signature_input_selects_sig1() -> None: """Per spec, verifiers process exactly sig1 and ignore additional labels.""" vector = json.loads( diff --git a/tests/conformance/signing/test_target_uri_malformed_boundary.py b/tests/conformance/signing/test_target_uri_malformed_boundary.py new file mode 100644 index 00000000..5daa7bf3 --- /dev/null +++ b/tests/conformance/signing/test_target_uri_malformed_boundary.py @@ -0,0 +1,196 @@ +"""The malformed-authority gate, observed through the two verifier entry points. + +`canonicalization.json` grades `canonicalize_target_uri()` by calling it +directly (test_canonicalization.py), so the vectors prove the *rule* but say +nothing about the two boundaries that translate it onto the wire: + +* :func:`adcp.signing.verify_request_signature` must surface the gate as the + ``request_target_uri_malformed`` code at step 6 -- and *not* as the + ``request_signature_header_malformed`` produced by the neighbouring + ``except (ValueError, KeyError)`` clause. The typed canonicalization error is + itself a ``ValueError``, so the narrow clause is only reachable while it sits + first; a later reorder is invisible to every direct-call test. +* :func:`adcp.webhooks.verify_webhook_signature` must retag that code into the + webhook family through ``REQUEST_TO_WEBHOOK_CODE`` rather than falling through + to the generic ``webhook_signature_invalid`` + "add to REQUEST_TO_WEBHOOK_CODE" + warning. + +Both tests sign against a well-formed URL and then present the malformed URL to +the verifier. Signing the malformed URL directly is not an option: the same gate +lives on the signing path, so the fixture would die before producing headers. +""" + +from __future__ import annotations + +import copy +import json +import logging +from pathlib import Path + +import pytest + +from adcp.signing import ( + StaticJwksResolver, + VerifierCapability, + VerifyOptions, + private_key_from_jwk, + sign_request, + verify_request_signature, +) +from adcp.signing.errors import ( + WEBHOOK_SIGNATURE_HEADER_MALFORMED, + SignatureVerificationError, +) +from adcp.webhooks import ( + WebhookVerifyOptions, + sign_webhook, + verify_webhook_signature, +) + +VECTORS_DIR = Path(__file__).parent.parent / "vectors" / "request-signing" +KEYS = json.loads((VECTORS_DIR / "keys.json").read_text())["keys"] +REQUEST_ED25519 = next(k for k in KEYS if k["kid"] == "test-ed25519-2026") +WEBHOOK_ED25519 = { + **copy.deepcopy(REQUEST_ED25519), + "kid": "test-webhook-ed25519-2026", + "adcp_use": "webhook-signing", +} + +# The code string canonicalization.json ships for every `reject: true` case. +# Spelled out rather than imported: it does not exist in `adcp.signing` yet, and +# an ImportError would make this module fail at collection instead of failing on +# the assertion that states the contract. +REQUEST_TARGET_URI_MALFORMED = "request_target_uri_malformed" + +# `malformed-bare-ipv6` from canonicalization.json: an unbracketed IPv6 literal +# in the authority. Chosen over the other five reject shapes because it survives +# `urlsplit()` intact, so it reaches the verifier's signature-base construction +# (step 6) rather than dying earlier -- exactly the path the boundary owns. +MALFORMED_URL = "https://fe80::1/p" + +SIGNED_URL = "https://seller.example.com/adcp/create_media_buy" +WEBHOOK_URL = "https://buyer.example.com/webhooks/adcp" +CREATED = 1776520800 +BODY = b'{"idempotency_key":"whk_abc123","task_id":"t1"}' + + +def _signed_request_headers(*, url: str = SIGNED_URL) -> dict[str, str]: + """Real request-signing headers produced by the real signer.""" + private_key = private_key_from_jwk(REQUEST_ED25519, d_field="_private_d_for_test_only") + headers = {"Content-Type": "application/json"} + signed = sign_request( + method="POST", + url=url, + headers=headers, + body=BODY, + private_key=private_key, + key_id=REQUEST_ED25519["kid"], + alg="ed25519", + created=CREATED, + ) + return {**headers, **signed.as_dict()} + + +def _request_verify_options() -> VerifyOptions: + return VerifyOptions( + now=float(CREATED), + capability=VerifierCapability(covers_content_digest="either"), + operation="create_media_buy", + jwks_resolver=StaticJwksResolver({"keys": [REQUEST_ED25519]}), + ) + + +def _signed_webhook_headers(*, url: str = WEBHOOK_URL) -> dict[str, str]: + """Real webhook-signing headers produced by the real webhook signer.""" + private_key = private_key_from_jwk(WEBHOOK_ED25519, d_field="_private_d_for_test_only") + headers = {"Content-Type": "application/json"} + signed = sign_webhook( + method="POST", + url=url, + headers=headers, + body=BODY, + private_key=private_key, + key_id=WEBHOOK_ED25519["kid"], + alg="ed25519", + ) + return {**headers, **signed.as_dict()} + + +def _webhook_verify_options() -> WebhookVerifyOptions: + return WebhookVerifyOptions( + jwks_resolver=StaticJwksResolver({"keys": [WEBHOOK_ED25519]}), + ) + + +def test_request_verifier_emits_target_uri_malformed_for_bad_authority() -> None: + """The gate reaches the wire as its own code, not the header-malformed catch-all. + + The request carries a valid signature over a well-formed URL; the verifier is + handed the malformed one. Signature-base construction is the first thing that + touches the URL, so whichever clause catches the canonicalization failure + decides the code the caller (and the 401's ``WWW-Authenticate``) sees. + + This assertion fails if the narrow ``except TargetUriMalformedError`` clause is + absent, or is moved below the ``except (ValueError, KeyError)`` clause that + yields ``request_signature_header_malformed`` -- the reorder that no + direct-call canonicalization test can see. + """ + headers = _signed_request_headers() + + with pytest.raises(SignatureVerificationError) as exc_info: + verify_request_signature( + method="POST", + url=MALFORMED_URL, + headers=headers, + body=BODY, + options=_request_verify_options(), + ) + + assert exc_info.value.code == REQUEST_TARGET_URI_MALFORMED, ( + f"verify_request_signature({MALFORMED_URL!r}) must reject with " + f"{REQUEST_TARGET_URI_MALFORMED!r}; got {exc_info.value.code!r}" + ) + assert exc_info.value.step == 6 + + +def test_webhook_verifier_retags_target_uri_malformed_without_map_warning( + caplog: pytest.LogCaptureFixture, +) -> None: + """The webhook route retags the new code instead of falling through. + + ``_retag_to_webhook`` maps request-family codes via ``REQUEST_TO_WEBHOOK_CODE`` + and, on a miss, emits ``webhook_signature_invalid`` plus a per-request warning + that literally asks for the map to be updated. A new request-family code with + no entry is therefore silently *tolerated* -- the caller just gets a vaguer + code and the receiver's logs get noisier. + + The expected twin is the EXISTING ``webhook_signature_header_malformed``: same + failure class, and no new wire-visible string is minted for a code the AdCP + webhook taxonomy does not define anywhere in this repo. + """ + headers = _signed_webhook_headers() + + with caplog.at_level(logging.WARNING, logger="adcp.signing.webhook_verifier"): + with pytest.raises(SignatureVerificationError) as exc_info: + verify_webhook_signature( + method="POST", + url=MALFORMED_URL, + headers=headers, + body=BODY, + options=_webhook_verify_options(), + ) + + assert exc_info.value.code == WEBHOOK_SIGNATURE_HEADER_MALFORMED, ( + f"verify_webhook_signature({MALFORMED_URL!r}) must retag " + f"{REQUEST_TARGET_URI_MALFORMED!r} to {WEBHOOK_SIGNATURE_HEADER_MALFORMED!r}; " + f"got {exc_info.value.code!r}" + ) + unmapped = [ + record.getMessage() + for record in caplog.records + if "REQUEST_TO_WEBHOOK_CODE" in record.getMessage() + ] + assert not unmapped, ( + "the request-family code must be present in REQUEST_TO_WEBHOOK_CODE; " + f"the retag fell through to the unknown-code branch: {unmapped}" + ) diff --git a/tests/conformance/signing/test_verifier_vectors.py b/tests/conformance/signing/test_verifier_vectors.py index 274a70f0..409b448f 100644 --- a/tests/conformance/signing/test_verifier_vectors.py +++ b/tests/conformance/signing/test_verifier_vectors.py @@ -46,9 +46,15 @@ "023-multi-valued-content-digest.json": ( "#976: multi-valued Content-Digest (duplicate RFC 9530 algorithm) is not rejected at parse" ), + # The signer half of #977 has landed: @target-uri canonicalization now + # converts a U-label to its A-label form. This vector is the verifier half + # -- a U-label arriving on the wire must be REJECTED, not converted -- and + # it expects request_signature_header_malformed at step 1, which is #976's + # precheck. Same predicate (canonical.host_has_raw_non_ascii), different + # code at a different step; it retires with #976, not with the conversion. "026-non-ascii-host.json": ( - "#977: raw IDN U-label in the authority is not rejected; @target-uri canonicalization " - "applies no UTS-46 A-label conversion" + "#977 (verifier half): raw IDN U-label on the wire is not rejected at step 1; " + "lands with #976's strict header precheck" ), } From d0c626a138631dae077b2c7685ea8f2925c5b948 Mon Sep 17 00:00:00 2001 From: "Constantine.mirin" Date: Wed, 29 Jul 2026 16:18:09 +0200 Subject: [PATCH 2/3] fix(signing): add the webhook target-uri code and reject authorities that empty Two review corrections. 1. The spec DOES define the webhook twin. 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. The earlier claim that no webhook twin existed came from grepping this repo rather than the spec, and retagging onto `webhook_signature_header_malformed` put a stable but WRONG code on the wire. Adds WEBHOOK_TARGET_URI_MALFORMED and maps REQUEST_TARGET_URI_MALFORMED to it. The name breaks the `webhook_signature_` prefix the rest of the family carries because the spec names it that way. 2. `_canon_host` stripped the trailing root dot AFTER `_malformed_authority_reason` had already accepted the netloc, and the raw netloc `.` is non-empty and passes as a host. So `https://./p` canonicalized to `https:///p` -- the empty authority this module rejects as `malformed-empty-authority`, reached by a path that ran the gate too early. `https://../p` yielded the authority `.`. The check now re-runs after the strip and is stated as "no empty label", so `https://a..b/p` is covered too. Refs #978, #977. --- src/adcp/signing/canonical.py | 8 ++++++ src/adcp/signing/errors.py | 16 ++++++------ .../signing/test_canonicalization.py | 25 +++++++++++++++++++ .../test_target_uri_malformed_boundary.py | 15 ++++++----- 4 files changed, 50 insertions(+), 14 deletions(-) diff --git a/src/adcp/signing/canonical.py b/src/adcp/signing/canonical.py index 88033f9e..78cfb6bc 100644 --- a/src/adcp/signing/canonical.py +++ b/src/adcp/signing/canonical.py @@ -267,6 +267,14 @@ def _canon_host(host: str, netloc: str) -> str: """ 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: diff --git a/src/adcp/signing/errors.py b/src/adcp/signing/errors.py index 05a40051..80415811 100644 --- a/src/adcp/signing/errors.py +++ b/src/adcp/signing/errors.py @@ -116,6 +116,13 @@ 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 @@ -123,14 +130,7 @@ def __init__( REQUEST_TO_WEBHOOK_CODE = { REQUEST_SIGNATURE_REQUIRED: WEBHOOK_SIGNATURE_REQUIRED, REQUEST_SIGNATURE_HEADER_MALFORMED: WEBHOOK_SIGNATURE_HEADER_MALFORMED, - # Deliberately NOT a new `webhook_target_uri_malformed`. The AdCP spec - # defines `request_target_uri_malformed` (it is graded by the shipped - # canonicalization vectors) but names no webhook twin, and no bundled - # schema enumerates webhook signing codes. Minting one here would put a - # string on the wire that nothing upstream defines. A malformed target URI - # is the same failure class as a malformed header, so it retags onto the - # existing code until the spec says otherwise. - REQUEST_TARGET_URI_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, diff --git a/tests/conformance/signing/test_canonicalization.py b/tests/conformance/signing/test_canonicalization.py index 0110bac2..36cde4ad 100644 --- a/tests/conformance/signing/test_canonicalization.py +++ b/tests/conformance/signing/test_canonicalization.py @@ -203,3 +203,28 @@ def test_multi_label_signature_input_selects_sig1() -> None: assert labels["sig2"].components == ("@method", "@target-uri") assert labels["sig1"].params["nonce"] == "KXYnfEfJ0PBRZXQyVXfVQA" assert labels["sig2"].params["nonce"] == "DIFFERENT-NONCE-FOR-SIG2____" + + +@pytest.mark.parametrize( + "url", + ["https://./p", "https://../p", "https://.:443/p", "https://a..b/p"], + ids=["root-dot-only", "double-dot", "root-dot-with-port", "interior-empty-label"], +) +def test_authority_that_empties_after_normalization_is_rejected(url: str) -> None: + """A host must still be a host once the root dot comes off. + + ``_malformed_authority_reason`` judges the raw netloc, where ``.`` and + ``..`` are non-empty and pass as hosts. Stripping the root dot is what + empties them, so the check has to run again afterwards. Without it + ``https://./p`` canonicalized to ``https:///p`` -- the empty authority + ``malformed-empty-authority`` rejects, reached by a path that skipped the + gate. + + ``a..b`` is here because the rule is "no empty label", not merely + "not empty". + """ + with pytest.raises(ValueError) as excinfo: + canonicalize_target_uri(url) + assert getattr(excinfo.value, "code", None) == "request_target_uri_malformed" + with pytest.raises(ValueError): + canonicalize_authority(url) diff --git a/tests/conformance/signing/test_target_uri_malformed_boundary.py b/tests/conformance/signing/test_target_uri_malformed_boundary.py index 5daa7bf3..5577c4b6 100644 --- a/tests/conformance/signing/test_target_uri_malformed_boundary.py +++ b/tests/conformance/signing/test_target_uri_malformed_boundary.py @@ -38,7 +38,7 @@ verify_request_signature, ) from adcp.signing.errors import ( - WEBHOOK_SIGNATURE_HEADER_MALFORMED, + WEBHOOK_TARGET_URI_MALFORMED, SignatureVerificationError, ) from adcp.webhooks import ( @@ -164,9 +164,12 @@ def test_webhook_verifier_retags_target_uri_malformed_without_map_warning( no entry is therefore silently *tolerated* -- the caller just gets a vaguer code and the receiver's logs get noisier. - The expected twin is the EXISTING ``webhook_signature_header_malformed``: same - failure class, and no new wire-visible string is minted for a code the AdCP - webhook taxonomy does not define anywhere in this repo. + The expected twin is ``webhook_target_uri_malformed``. It is named without + the ``webhook_signature_`` prefix the rest of the family carries because the + spec names it that way: security.mdx's webhook checklist lists it in the + error taxonomy and requires it at step 10 for a malformed or mismatched + authority. Retagging onto ``webhook_signature_header_malformed`` instead + would put a stable but WRONG code on the wire. """ headers = _signed_webhook_headers() @@ -180,9 +183,9 @@ def test_webhook_verifier_retags_target_uri_malformed_without_map_warning( options=_webhook_verify_options(), ) - assert exc_info.value.code == WEBHOOK_SIGNATURE_HEADER_MALFORMED, ( + assert exc_info.value.code == WEBHOOK_TARGET_URI_MALFORMED, ( f"verify_webhook_signature({MALFORMED_URL!r}) must retag " - f"{REQUEST_TARGET_URI_MALFORMED!r} to {WEBHOOK_SIGNATURE_HEADER_MALFORMED!r}; " + f"{REQUEST_TARGET_URI_MALFORMED!r} to {WEBHOOK_TARGET_URI_MALFORMED!r}; " f"got {exc_info.value.code!r}" ) unmapped = [ From cdb0ed485c3dbf3984d63278f5b4e285977e8131 Mon Sep 17 00:00:00 2001 From: "Constantine.mirin" Date: Wed, 29 Jul 2026 17:57:55 +0200 Subject: [PATCH 3/3] fix(signing): validate the port per RFC 3986 instead of trusting int() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. `_malformed_authority_reason` judged the host but never the port, so `portstr` went straight into `int()` -- far more permissive than the grammar `port = *DIGIT`, in three distinct ways: - `int("-80")` produced 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. That is a raw-vs-canonical differential -- 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 the third case -- `"٨٠".isdigit()` is True -- so the gate tests ASCII digits specifically. An empty port is NOT rejected. RFC 3986 §3.2.3 makes it legal (`*DIGIT`), meaning "default", and directs normalizers to drop the port and its colon, so `https://host:/p` normalizes to `host`. `urlsplit` agrees (`port=None`). Rejecting it would refuse a valid URI; it previously raised a bare `ValueError` from `int("")` carrying no code -- the same "passes on someone else's exception" failure `_split_or_reject` exists to prevent, one frame lower. Refs #978. --- src/adcp/signing/canonical.py | 37 +++++++++++++++- .../signing/test_canonicalization.py | 42 +++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/adcp/signing/canonical.py b/src/adcp/signing/canonical.py index 78cfb6bc..9c7acd6d 100644 --- a/src/adcp/signing/canonical.py +++ b/src/adcp/signing/canonical.py @@ -236,10 +236,10 @@ def _canon_authority(netloc: str, scheme: str) -> str: 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 = _canon_host(host, netloc) @@ -248,6 +248,39 @@ def _canon_authority(netloc: str, scheme: str) -> str: 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. diff --git a/tests/conformance/signing/test_canonicalization.py b/tests/conformance/signing/test_canonicalization.py index 36cde4ad..96a4da10 100644 --- a/tests/conformance/signing/test_canonicalization.py +++ b/tests/conformance/signing/test_canonicalization.py @@ -228,3 +228,45 @@ def test_authority_that_empties_after_normalization_is_rejected(url: str) -> Non assert getattr(excinfo.value, "code", None) == "request_target_uri_malformed" with pytest.raises(ValueError): canonicalize_authority(url) + + +@pytest.mark.parametrize( + ("url", "expected"), + [ + # RFC 3986 §3.2.3: `port = *DIGIT`, so an EMPTY port is legal and means + # "default" -- normalizers SHOULD drop it and its colon. Rejecting it + # would refuse a valid URI. + ("https://host:/p", "host"), + ("https://[::1]:/p", "[::1]"), + # ...and a real port still survives. + ("https://host:8443/p", "host:8443"), + ], + ids=["empty-port", "empty-port-ipv6", "real-port"], +) +def test_empty_port_is_normalized_away_not_rejected(url: str, expected: str) -> None: + assert canonicalize_authority(url) == expected + + +@pytest.mark.parametrize( + "url", + ["https://host:-80/p", "https://host:8_0/p", "https://host:٨٠/p", "https://host:8a/p"], + ids=["negative", "underscore-separator", "arabic-indic-digits", "alphanumeric"], +) +def test_non_digit_port_is_rejected_with_the_spec_code(url: str) -> None: + """The port was never validated -- it went straight into `int()`. + + Three distinct failures came out of that. `int("-80")` yielded the + authority ``host:-80``, which is not a valid authority at all. `int("8_0")` + is 80, because Python accepts underscore digit separators. And + `int("٨٠")` is also 80, because `int()` accepts non-ASCII digits -- so + ``host:٨٠`` and ``host:80`` collapsed to the SAME canonical authority. + That last one is a raw-vs-canonical differential: a peer that does not + fold Arabic-Indic digits computes a different `@authority` for the same + bytes, and the signature fails for a reason neither side can see. + + Note `str.isdigit()` alone does not close this -- `"٨٠".isdigit()` is + True. The gate has to be ASCII digits specifically. + """ + with pytest.raises(ValueError) as excinfo: + canonicalize_authority(url) + assert getattr(excinfo.value, "code", None) == "request_target_uri_malformed"