From 979bbb49e4af6165e6601540abc3e1088e78e633 Mon Sep 17 00:00:00 2001 From: "Constantine.mirin" Date: Wed, 29 Jul 2026 12:48:32 +0200 Subject: [PATCH 1/2] fix(webhooks): bracket and structurally validate DockerLocalhostRewrite rewrite_to `rewrite_to` was interpolated straight into a netloc with no validation. A bare IPv6 value produced an unbracketed authority -- the one shape RFC 3986 makes ambiguous with a port -- so `rewrite_to="::1"` against a URL with a port yielded `https://::1:9000/hook`, which `_canon_authority` mis-splits at the last colon. This is the only place a non-signing layer synthesizes the netloc that later becomes the signed `@authority`. Bare IPv6 literals are now bracketed at construction (`"::1"` -> `"[::1]"`) and folded to canonical compressed form, so the assembled authority is unambiguous and the signed value is stable. The validation is deliberately STRUCTURAL, not a hostname-syntax check. It rejects only what can move the boundary between authority, userinfo, port, path, query or fragment -- path injection, userinfo injection, embedded ports, raw whitespace and control characters, and RFC 6874 IPv6 zone IDs (`%` cannot appear unencoded in a URI authority). It deliberately does NOT enforce hostname syntax. Docker Compose service names legally contain underscores (`my_service`, `host_gateway`) which RFC 952/1123 and IDNA both reject, and Docker's embedded DNS resolves them. Routing `rewrite_to` through the IDNA canonicalizer would have rejected those at construction -- turning a working deployment into a startup crash on upgrade, in the class that exists specifically to serve Docker. Whether a name resolves is the resolver's business; whether it restructures the signed URL is ours. Non-ASCII names are still IDNA-encoded to A-labels, since those cannot go on the wire as-is. Validation lives in `validate_for_sender` rather than `__post_init__` so it also covers direct `apply_hooks` use and hooks not attached to a sender. Fixes #991. --- src/adcp/webhook_transport_hooks.py | 105 ++++++++++++++++++ .../signing/test_webhook_transport_hooks.py | 85 ++++++++++++++ 2 files changed, 190 insertions(+) diff --git a/src/adcp/webhook_transport_hooks.py b/src/adcp/webhook_transport_hooks.py index c7346f9c..d4c23e7f 100644 --- a/src/adcp/webhook_transport_hooks.py +++ b/src/adcp/webhook_transport_hooks.py @@ -32,10 +32,18 @@ from __future__ import annotations +import ipaddress from dataclasses import dataclass from typing import Protocol from urllib.parse import urlsplit, urlunsplit +#: Characters that, interpolated into a netloc, move the boundary between +#: authority / userinfo / port / path / query / fragment -- i.e. rewrite the +#: signed URL into a different URL. `%` covers RFC 6874 IPv6 zone IDs. +#: `:` is deliberately absent: IPv6 literals are made of them, so a stray port +#: is caught after the IP-literal parse instead. +_STRUCTURAL_CHARS = frozenset('/@?#\\[]%"<>^`{|}') + class TransportHook(Protocol): """Rewrite the destination URL before SSRF runs. @@ -77,10 +85,107 @@ class DockerLocalhostRewrite: The check happens via :meth:`validate_for_sender`, called by :meth:`WebhookSender._from_strategy` (and ``__init__``) when ``transport_hooks`` is set. + + ``rewrite_to`` is validated and canonicalized at construction: it + must be a hostname or IP literal, is lower-cased, and bare IPv6 + literals are bracketed automatically (``"::1"`` is stored as + ``"[::1]"``) so the assembled authority is unambiguous with a port. + IPv4-mapped IPv6 is re-formatted to its canonical compressed form + (``"::ffff:127.0.0.1"`` becomes ``"[::ffff:7f00:1]"`` — the same + address, spelled canonically). Non-ASCII hostnames are IDNA-encoded + to A-labels. IPv6 zone IDs (``"fe80::1%eth0"``) are rejected: RFC + 6874 requires the ``%`` be percent-encoded inside a URI, and + silently emitting an invalid authority is worse than failing at + wiring time. + + The validation is deliberately **structural**, not a hostname-syntax + check. ASCII names are accepted as-is once they cannot alter the + URL's shape, because Docker Compose service names legally contain + underscores (``my_service``, ``host_gateway``) which RFC 952/1123 + and IDNA both reject — and Docker's embedded DNS resolves them. + Enforcing hostname syntax here would refuse the exact configuration + this class exists to serve. Whether the name resolves is the + resolver's business; whether it rewrites the signed URL is ours. """ rewrite_to: str = "host.docker.internal" + def __post_init__(self) -> None: + # Deferred import: ``adcp.signing``'s package __init__ is an + # order of magnitude heavier than this leaf module, and both + # real consumers (webhook_sender, webhooks) already import it. + # This runs once per hook construction, never per delivery. + from adcp.signing._idna_canonicalize import canonicalize_host + + value = self.rewrite_to + if not value: + raise ValueError( + "DockerLocalhostRewrite(rewrite_to=...) must be a hostname or IP " + "literal; got an empty string" + ) + + # Accept an already-bracketed IPv6 literal by unwrapping it first; + # the brackets are re-applied below from the canonical form. + inner = value[1:-1] if value.startswith("[") and value.endswith("]") else value + + # Structural rejection comes FIRST and is the actual point of this + # guard: `rewrite_to` is interpolated straight into the netloc, so any + # character that can move the boundary between authority, path, query, + # fragment or userinfo rewrites the signed URL into a different URL. + # `%` is here for RFC 6874 IPv6 zone IDs, which are not representable + # in a URI authority without percent-encoding. + bad = {ch for ch in inner if ch in _STRUCTURAL_CHARS or ord(ch) < 0x21 or ord(ch) == 0x7F} + if bad: + raise ValueError( + f"DockerLocalhostRewrite(rewrite_to=...) must be a hostname or IP " + f"literal; got {value!r}, which contains {sorted(bad)!r} and would " + f"change the structure of the signed URL" + ) + + try: + ip = ipaddress.ip_address(inner) + except ValueError: + pass + else: + # Bracket v6 so the assembled authority is unambiguous with a port + # -- the defect this guard exists to close. `str(ip)` also folds the + # literal to its canonical compressed form. + canonical = f"[{ip}]" if ip.version == 6 else str(ip) + object.__setattr__(self, "rewrite_to", canonical) + return + + if ":" in inner: + # Not an IP literal, so a colon is a port -- which `rewrite_url` + # re-appends itself, and which `apply_hooks`' port guard would then + # see as a port change. + raise ValueError( + f"DockerLocalhostRewrite(rewrite_to=...) must be a hostname or IP " + f"literal without a port; got {value!r}" + ) + + if inner.isascii(): + # Deliberately NOT routed through `canonicalize_host`: this is a + # Docker helper, and Docker Compose service names legally contain + # underscores (`my_service`, `host_gateway`), which IDNA rejects + # under RFC 952/1123. Docker's embedded DNS resolves them, so + # refusing them here would break the case this class exists for. + # Structural safety is already established above; anything further + # is the resolver's business, not ours. + object.__setattr__(self, "rewrite_to", inner.lower().rstrip(".")) + return + + # Non-ASCII: convert to A-labels so the netloc is wire-legal. Failure + # here is a genuinely unusable host, not a naming-convention quibble. + try: + canonical = canonicalize_host(inner) + except (UnicodeError, ValueError) as exc: + # ``idna.IDNAError`` subclasses ``UnicodeError``. + raise ValueError( + f"DockerLocalhostRewrite(rewrite_to=...) must be a hostname or IP " + f"literal; got {value!r} ({exc})" + ) from exc + object.__setattr__(self, "rewrite_to", canonical) + def rewrite_url(self, url: str) -> str | None: parsed = urlsplit(url) # ``hostname`` lower-cases and strips brackets from IPv6 — match diff --git a/tests/conformance/signing/test_webhook_transport_hooks.py b/tests/conformance/signing/test_webhook_transport_hooks.py index 5808cb50..0c927318 100644 --- a/tests/conformance/signing/test_webhook_transport_hooks.py +++ b/tests/conformance/signing/test_webhook_transport_hooks.py @@ -11,6 +11,7 @@ import pytest from adcp.signing import SSRFValidationError +from adcp.signing.canonical import canonicalize_authority from adcp.webhook_sender import WebhookSender from adcp.webhook_transport_hooks import ( DockerLocalhostRewrite, @@ -65,6 +66,90 @@ def test_rewrite_preserves_query_and_fragment() -> None: ) +# ---------- rewrite_to validation ---------- + + +def test_bare_ipv6_rewrite_to_is_bracketed() -> None: + """A bare IPv6 ``rewrite_to`` yields an unbracketed authority that RFC 3986 + makes ambiguous with a port. Bracket it at construction — the operator's + intent is unambiguous and the docstring encourages IP-literal values.""" + hook = DockerLocalhostRewrite(rewrite_to="::1") + assert hook.rewrite_url("https://localhost:9000/hook") == "https://[::1]:9000/hook" + + +def test_bare_ipv6_rewrite_survives_apply_hooks_and_signing_canonicalization() -> None: + """The bracketing must hold all the way through the framework's port guard + and into signing canonicalization. Unbracketed, ``urlsplit(...).port`` inside + ``apply_hooks`` raises an opaque stdlib ValueError about a port the operator + never configured.""" + hook = DockerLocalhostRewrite(rewrite_to="2001:db8::1") + out = apply_hooks("https://localhost:9000/hook", (hook,)) + assert out == "https://[2001:db8::1]:9000/hook" + assert canonicalize_authority(out) == "[2001:db8::1]:9000" + + +@pytest.mark.parametrize( + "bad", + ["", "attacker.com/path", "user@evil.example", "host.docker.internal:1234", "ho st"], +) +def test_rewrite_to_rejects_non_host_values(bad: str) -> None: + """``rewrite_to`` is interpolated into the netloc; anything that is not a + hostname or IP literal changes the signed URL in ways apply_hooks' scheme/ + port guard does not catch (path injection, userinfo injection, raw spaces).""" + with pytest.raises(ValueError, match="rewrite_to"): + DockerLocalhostRewrite(rewrite_to=bad) + + +def test_rewrite_to_rejects_ipv6_zone_id() -> None: + """``ipaddress`` accepts scoped addresses, but RFC 6874 requires the ``%`` + be percent-encoded as ``%25`` inside a URI. Rather than emit an authority + that is invalid on the wire, reject the zone-ID form at construction.""" + with pytest.raises(ValueError, match="rewrite_to"): + DockerLocalhostRewrite(rewrite_to="fe80::1%eth0") + + +def test_bracketed_ipv6_rewrite_to_accepted_unchanged() -> None: + hook = DockerLocalhostRewrite(rewrite_to="[::1]") + assert hook.rewrite_url("https://localhost:9000/hook") == "https://[::1]:9000/hook" + + +def test_rewrite_to_hostname_and_ipv4_still_accepted() -> None: + assert ( + DockerLocalhostRewrite().rewrite_url("http://localhost:8080/webhook") + == "http://host.docker.internal:8080/webhook" + ) + assert ( + DockerLocalhostRewrite(rewrite_to="172.17.0.1").rewrite_url("http://localhost/x") + == "http://172.17.0.1/x" + ) + + +@pytest.mark.parametrize( + "name", ["my_service", "host_gateway", "docker_host.local", "_dns-sd._udp.local"] +) +def test_rewrite_to_accepts_docker_legal_service_names(name: str) -> None: + """Underscored names must keep working -- this is a Docker helper. + + Docker Compose service names legally contain underscores and Docker's + embedded DNS resolves them, but RFC 952/1123 and IDNA both reject them. + Validating ``rewrite_to`` as a *hostname* rather than structurally would + refuse the exact configuration this class exists to serve, and would do it + at construction -- turning a working deployment into a startup crash on + upgrade. The guard only has to prove the value cannot restructure the URL. + """ + assert DockerLocalhostRewrite(rewrite_to=name).rewrite_to == name + + +def test_rewrite_to_is_normalized_at_construction() -> None: + """Canonicalization is visible on the field: case-folded, trailing FQDN + root dot dropped, IDN encoded to A-labels, IPv6 bracketed.""" + assert DockerLocalhostRewrite(rewrite_to="HOST.Docker.Internal.").rewrite_to == ( + "host.docker.internal" + ) + assert DockerLocalhostRewrite(rewrite_to="bücher.example").rewrite_to == "xn--bcher-kva.example" + assert DockerLocalhostRewrite(rewrite_to="[2001:DB8::1]").rewrite_to == "[2001:db8::1]" + + # ---------- apply_hooks (framework) ---------- From 9de2b4f2eef1874a97d743873e76b1daa5ea3386 Mon Sep 17 00:00:00 2001 From: "Constantine.mirin" Date: Wed, 29 Jul 2026 15:00:29 +0200 Subject: [PATCH 2/2] fix(webhooks): reject rewrite_to values that normalize to no host Review follow-up. The guard rejected a structurally unsafe value but still let three shapes through to an empty host, which then assembled to `https://:9000/hook` -- an authority with a port and no host, exactly the shape @target-uri canonicalization rejects, produced by the hook meant to keep the authority well-formed. Each reached the empty host past a different check: - `"[]"` is non-empty until the brackets come off. - `"."` is non-empty until the trailing root dot comes off. - `".."` survived a single-dot strip as `"."`, because the strip used rstrip('.') and ate every dot rather than the one root dot canonicalize_host removes. Rejection is now stated as 'no empty label', which also covers an interior empty label (`"a..b"`). Refs #991. --- src/adcp/webhook_transport_hooks.py | 25 ++++++++++++++++++- .../signing/test_webhook_transport_hooks.py | 18 +++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/adcp/webhook_transport_hooks.py b/src/adcp/webhook_transport_hooks.py index d4c23e7f..5262689c 100644 --- a/src/adcp/webhook_transport_hooks.py +++ b/src/adcp/webhook_transport_hooks.py @@ -127,6 +127,15 @@ def __post_init__(self) -> None: # Accept an already-bracketed IPv6 literal by unwrapping it first; # the brackets are re-applied below from the canonical form. inner = value[1:-1] if value.startswith("[") and value.endswith("]") else value + if not inner: + # `"[]"` survives the non-empty check above and empties here. An + # empty host is the one outcome this guard exists to prevent: it + # assembles to `https://:9000/hook`, an authority with a port and + # no host -- exactly the shape @target-uri canonicalization rejects. + raise ValueError( + f"DockerLocalhostRewrite(rewrite_to=...) must be a hostname or IP " + f"literal; got {value!r}, which has no host" + ) # Structural rejection comes FIRST and is the actual point of this # guard: `rewrite_to` is interpolated straight into the netloc, so any @@ -171,7 +180,21 @@ def __post_init__(self) -> None: # refusing them here would break the case this class exists for. # Structural safety is already established above; anything further # is the resolver's business, not ours. - object.__setattr__(self, "rewrite_to", inner.lower().rstrip(".")) + # One trailing root dot, matching `canonicalize_host` -- `rstrip` + # would eat every dot, so `"."` and `".."` normalized to the empty + # host rather than being rejected. + ascii_host = inner.lower() + if ascii_host.endswith("."): + ascii_host = ascii_host[:-1] + if not ascii_host or any(label == "" for label in ascii_host.split(".")): + # Catches `"."`, `".."` and `"a..b"`. An empty label is not a + # host, and `".."` in particular survives a single-dot strip as + # `"."` -- non-empty, but still no host. + raise ValueError( + f"DockerLocalhostRewrite(rewrite_to=...) must be a hostname or IP " + f"literal; got {value!r}, which normalizes to an empty label" + ) + object.__setattr__(self, "rewrite_to", ascii_host) return # Non-ASCII: convert to A-labels so the netloc is wire-legal. Failure diff --git a/tests/conformance/signing/test_webhook_transport_hooks.py b/tests/conformance/signing/test_webhook_transport_hooks.py index 0c927318..9570fe45 100644 --- a/tests/conformance/signing/test_webhook_transport_hooks.py +++ b/tests/conformance/signing/test_webhook_transport_hooks.py @@ -124,6 +124,24 @@ def test_rewrite_to_hostname_and_ipv4_still_accepted() -> None: ) +@pytest.mark.parametrize("bad", ["[]", "[.]", ".", "..", "...", "a..b"]) +def test_rewrite_to_rejects_values_that_normalize_to_no_host(bad: str) -> None: + """An empty host is the outcome this guard exists to prevent. + + These reach the empty host by two different routes the earlier checks each + miss: ``"[]"`` is non-empty until the brackets come off, and ``"."`` / + ``".."`` are non-empty until the trailing root dot is stripped. Both used + to assemble to ``https://:9000/hook`` — an authority with a port and no + host, which is precisely the shape ``@target-uri`` canonicalization + rejects, produced by the hook meant to keep the authority well-formed. + + ``"a..b"`` is here because the fix is stated as "no empty label" rather + than "not empty", and an interior empty label is the same defect. + """ + with pytest.raises(ValueError, match="rewrite_to"): + DockerLocalhostRewrite(rewrite_to=bad) + + @pytest.mark.parametrize( "name", ["my_service", "host_gateway", "docker_host.local", "_dns-sd._udp.local"] )