fix(server): unify divergent host normalizers behind one helper

[KonstantinMirin]

Fixes #990.

Independent of the signing stack — branched off main, touches no file #980/#985/#987/#993 touch.

What was wrong

Three separate implementations of "normalize a Host header for tenant lookup": the subdomain router, the tenant registry, and the v3 reference seller example. They disagreed with each other, all three mangled IPv6 authorities, and one leaked userinfo into the lookup key.

The IPv6 case is worse than the issue implies. Splitting on the first colon reduces every bracketed literal to the key "[", so two distinct IPv6 tenants collapse onto the same entry. That is cross-tenant mis-resolution, not a fail-closed 404.

[::1]:8080            -> ::1
[2001:DB8::0:1]:443   -> 2001:db8::1
user:pw@acme.example.com -> acme.example.com
acme.example.com.     -> acme.example.com

How

One normalize_host_key, delegating to canonicalize_host — the helper four signing modules already use. Net implementations go from three divergent ones down to one; no fourth normalizer in a tree that already had too many. The examples/ copy is included because it is what adopters clone.

The helper never raises. Host is attacker-controlled, so a canonicalizer that raised would turn a clean 404 into a 500 — which is exactly why these rows were DEFER rather than MIGRATE in the review that found them. It falls back to the lower-cased input on anything IDNA cannot process. 26 hostile shapes are pinned by test ('', '.', '[::1', '@', '%00.example.com', 'http://', 100-char labels, underscores), and every output is idempotent under a second pass — load-bearing, because InMemory registration keys are normalized at registration and again at lookup.

Adopter-visible changes — none isolation-breaking, all worth a release note

1. Host values that previously 404'd now resolve to the bare host's tenant.

user@acme.example.com   -> acme.example.com
acme.example.com/x      -> acme.example.com
acme.example.com:abc    -> acme.example.com

Same tenant in every case, so there is no cross-tenant leak — but it is a Host-header parsing widening, and it matters to anyone whose upstream cache or WAF keys on the raw Host.

2. UTS-46 folding means compatibility forms now match. ＡＣＭＥ.example.com reaches the tenant registered as acme.example.com. Correct IDNA behaviour and consistent with DNS, but if two registration keys fold together InMemorySubdomainTenantRouter.__init__ silently keeps the last one with no error. That was already true for case variants; the folding enlarges the silently-colliding class.

3. import adcp.server now pulls adcp.signing (~0.13s, 30 modules) because of the module-level canonicalize_host import. idna is already a core dependency so nothing breaks, and there is precedent (canonical_formats/references.py imports the same private module). Per-request normalization cost went 0.1µs → 14µs, which is irrelevant next to a DNS lookup.

Test plan

make ci-local: 5902 passed, 0 failed, 41 skipped, 1 xfailed.

5 of the 6 new tests fail against the old implementations — verified by re-injecting them via a pytest plugin that rebinds tenant_router._normalize_host / normalize_host_key and tenant_registry.normalize_host_key, not by reasoning about the diff. The 6th (test_normalize_host_key_never_raises_on_hostile_input) passes either way; it is kept deliberately, because it guards the hazard this change introduces — the new canonicalize_host call is the thing that could start raising.

Known wart

normalize_host_key strips a trailing dot itself even though canonicalize_host already strips one, so acme.example.com.. double-folds to acme.example.com. Harmless for tenant lookup and arguably desirable, but it is a second dot-strip rather than one — noting it rather than leaving it for a reviewer to find.

[aao-ipr-bot]

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

_{This is an automated message from the Argus AI review workflow.}

[aao-ipr-bot]

Approving. Collapsing three divergent Host normalizers to one shared key function is the right shape — a registration key and a request-time Host header now agree because they run the same code, and that is the invariant that was broken.

The headline fix is real: the old split(":", 1)[0] reduced every bracketed IPv6 authority to the key "[", and because InMemorySubdomainTenantRouter.__init__ normalizes registration keys the same way, two distinct IPv6 tenants collapsed onto one dict slot — last-write-wins cross-tenant resolution, not a fail-closed 404. The IP-literal short-circuit through ipaddress.ip_address (src/adcp/server/tenant_router.py, new normalize_host_key) fixes it, and tests/test_subdomain_tenant_router.py:135-156 pins exactly that non-collision.

Things I checked

• No new collision class in the fail-soft. except (UnicodeError, ValueError) returns the pre-IDNA host. security-reviewer: a fallback key can only equal another key produced by the same failing string on the same fallback path — i.e. the genuinely same host — so fail-soft yields a 404 for unknown hosts, never a cross-tenant hit. idna.IDNAError subclasses UnicodeError, so every documented raise is caught.
• Idempotency holds — load-bearing because InMemory normalizes registration keys and lookup hosts. code-reviewer traced every branch: valid ASCII A-labels are a fixed point (idna.alabel short-circuits valid ASCII before punycode), and the bare-literal short-circuit is what makes the output form ::1 re-normalize to itself instead of urlsplit reading it as host:port. [::1]:8080, [::1], and bare ::1 all key to ::1.
• userinfo cannot bypass routing. user:pw@acme.example.com keys on acme.example.com; tenant routing is not the auth boundary, so reaching a tenant still needs valid AuthInfo (security-reviewer: no privilege gained).
• DoS surface closed. Attacker Host now flows into idna.encode, but pyproject.toml:110 pins idna>=3.7 (past the CVE-2024-3651 quadratic fix, per the comment at :104-105), and the 100-char label test takes the O(1) reject path.
• Backwards-compat. _normalize_host retained as a delegating alias in both tenant_router.py:445 and tenant_registry.py:228; every existing caller still resolves. No circular import — tenant_registry imports normalize_host_key from tenant_router at module top but tenant_router never imports tenant_registry, and the adcp.signing pull stays deferred.
• Conventional-commit prefix fix(server): is correct — no public export removed, renamed, or retyped; nothing breaking on the wire.
• The double dot-strip wart (acme.example.com.. folds to acme.example.com) is harmless and idempotent, as disclosed.

Follow-ups (non-blocking — file as issues)

• Export normalize_host_key properly. Two docstrings now instruct custom/adopter resolve impls to route through it, yet it is absent from tenant_router.__all__ (src/adcp/server/tenant_router.py:495) and not re-exported from src/adcp/server/__init__.py, forcing examples/v3_reference_seller/src/tenant_router.py:28 to reach into a private submodule with an apologetic comment. If adopters are told to use it, it should be public.
• Silent last-write-wins on folded keys is widened, not new. Registering two IDN-equivalent hosts silently drops one at tenant_router.py:164-166 and tenant_registry.py:301,371 (security-reviewer, Low). Operator-controlled, not attacker-reachable, and folding DNS-equivalents is correct — but raising on an already-present normalized key at registration would turn a silent tenant loss into an error.
• "Never raises" is guaranteed for str only. value.strip() on a non-str value raises uncaught. Callers are typed str and ASGI extraction guards it, so this is a docstring-scoping note, not a live path.

Minor nits (non-blocking)

1. PR body item #3 documents an implementation this PR does not ship. The changelog note claims import adcp.server now pulls adcp.signing "because of the module-level canonicalize_host import" — but the code (normalize_host_key, the deferred from adcp.signing._idna_canonicalize import canonicalize_host) and the commit message both defer that import specifically so the server does not drag in signing. Rewrite item #3 to match the deferred import before it lands in the changelog, or it ships a phantom import-cost regression note that lost the argument to its own commit message.
2. Hostile-shape count is off. Body and commit both say "26 hostile shapes are pinned by test"; test_normalize_host_key_never_raises_on_hostile_input pins 10. Cosmetic, but the number is stated twice.

LGTM. Follow-ups noted below.

[aao-ipr-bot]

Approving. Collapsing three divergent host normalizers to one that both registration and lookup share is the right shape — a registration key and a request-time Host can no longer disagree because they run the same code.

The IPv6 fix is the load-bearing part: the old first-colon split reduced every bracketed literal to the key "[", merging all IPv6 tenants into one dict slot. That was cross-tenant mis-resolution, not a fail-closed 404. Good catch, and the two regression tests (test_in_memory_router_distinct_ipv6_tenants_do_not_collide, test_register_with_ipv6_agent_url_is_reachable_by_host_header) pin it from both the router and the registry side.

Things I checked

• No cross-tenant leak. Walked the widening cases with security-reviewer: user@acme.example.com, acme.example.com/x, acme.example.com:abc all resolve to the same tenant as bare acme.example.com via urlsplit(...).hostname. The Host header is attacker-controlled, so this grants no access an attacker didn't already have by sending the bare host. UTS-46/IDNA folding only maps a non-ASCII host onto an ASCII key when it's the same DNS name (U-label ↔ A-label, fullwidth ↔ ASCII). No privilege escalation. Verdict: Low, net-positive for isolation.
• Fail-closed holds. Every fallback path (urlsplit raises → raw.lower(); empty hostname → raw.lower(); canonicalize_host raises → pre-IDNA host) yields a key that misses → 404, never an accidental match. test_normalize_host_key_never_raises_on_hostile_input pins 10 hostile shapes.
• No import cycle. tenant_registry.py:38 imports normalize_host_key from tenant_router one-directionally; tenant_router has no module-level adcp.server import and reaches canonicalize_host via a lazy in-function import (tenant_router.py:194). adcp.signing does not import adcp.server. Clean.
• No public-API break. normalize_host_key is not re-exported from adcp.server.__init__ — reachable only via the submodule, which is why the example imports it there. _normalize_host is kept as a deprecated alias; all four internal callers still resolve. Additive, so fix(server): is the correct semver signal.
• The ASCII fast path is correct, not just fast. For all-ASCII input canonicalize_host either returns exactly the fast-path value or raises into the same fallback — the answer is identical, and it keeps the 30-module adcp.signing import off every import adcp.server. Right call, and the comment at tenant_router.py:485-499 earns its length.

Follow-ups (non-blocking — file as issues)

• The idempotency invariant is false for a port-bearing non-canonical IPv6 literal, and the PR advertises the value it doesn't produce. normalize_host_key (tenant_router.py): the bare-literal short-circuit compresses via str(ipaddress.ip_address(...)), but the port-bearing path returns urlsplit(...).hostname uncompressed. So [2001:DB8::0:1]:443 → 2001:db8::0:1 on the first pass, then 2001:db8::1 on a second — not idempotent. Consequence: a tenant registered bare as [2001:db8::0:1] (→ 2001:db8::1) is unreachable when the Host carries a port (→ 2001:db8::0:1). Fail-closed 404, no leak, and confined to non-canonical IPv6 literals — which is why it doesn't block. But the code comment "every output is idempotent under a second pass — load-bearing" is provably false, and the docstring row [2001:DB8::0:1]:443 → 2001:db8::1 prints the second-pass value, not the real output. One-line fix: re-run host = parts.hostname through the ipaddress.ip_address short-circuit so both branches converge on the compressed form. That also makes the docstring true. The existing IPv6 tests miss this because they only use already-canonical addresses. (code-reviewer: High, fail-closed; security-reviewer: no isolation impact.)
• Silent last-write-wins on colliding registration keys. InMemorySubdomainTenantRouter.__init__ (tenant_router.py:164-165) and TenantRegistry._host_map (tenant_registry.py): two hosts that fold to the same key drop one silently. Pre-existing for case variants; IDNA folding enlarges the class. In a component whose whole job is tenant isolation, a ValueError naming the colliding hosts beats a silent drop. Worth doing before this is enabled for untrusted registration.
• Host-parsing widening can desync an edge WAF/cache that keys on the raw Host. Acknowledged in the PR body — keep the release-note callout.

Minor nits (non-blocking)

1. Trailing double-dot isn't folded on the ASCII path. normalize_host_key strips one dot then the ASCII fast path returns before canonicalize_host's second strip, so acme.example.com.. → acme.example.com., not the acme.example.com the "Known wart" note claims. Fail-closed. host.rstrip(".") fixes it and makes the note accurate.

The commit message is unusually candid about the storyboard-runner cost story and the known warts — good, though it does mean the one inaccurate invariant is stated with more confidence than it earned. Correct the code (preferred, one line) or the claim; don't ship the false "load-bearing" comment for the next reader to trust.

LGTM. Follow-ups noted below.

[bokelley]

normalize_host_key does not canonicalize an IPv6 literal after extracting it from a port-bearing authority. Reproduced on current main + this PR:\n\nnormalize_host_key("[2001:DB8::0:1]:443") == "2001:db8::0:1"\nnormalize_host_key("2001:db8::0:1") == "2001:db8::1"\n\nSo the advertised idempotency invariant is false, and a tenant registered under bare [2001:DB8::0:1] is unreachable from the equivalent Host value with :443. Please run parts.hostname through ipaddress.ip_address before the ASCII fast path and add a non-canonical, port-bearing IPv6 regression test. The focused suite otherwise passes (95 tests).

[aao-ipr-bot]

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

_{This is an automated message from the Argus AI review workflow.}

[KonstantinMirin]

Fixed in fe0cceb8. Your repro was exact.

The bare-literal short-circuit only inspects the raw input. Once a port is attached it is urlsplit that removes the brackets, so the address reached the rest of the function uncompressed and was never folded — hence [2001:DB8::0:1]:443 keying to 2001:db8::0:1 while the bare form keyed to 2001:db8::1.

Worth being explicit that this makes the docstring's idempotency claim false, not merely incomplete. That matters more than the lookup miss: InMemorySubdomainTenantRouter normalizes registration keys at construction and normalizes the Host again at lookup, so a non-idempotent key silently fails to match itself.

Now re-runs the IP-literal test on the extracted host. Two of the four new tests fail without the change (verified by stashing only the source file — stashing everything removes the tests too and proves nothing); the other two are regression guards and I've said so rather than counting them as coverage.

make ci-local: 5906 passed, 0 failed.

[aao-ipr-bot]

Right fix, right shape. Collapsing three divergent Host normalizers onto one idempotent normalize_host_key is what makes a registration key and a request-time Host header agree — and the IPv6 collapse it kills ([::1], [::2], [2001:db8::1] all keying to [) was a real cross-tenant mis-resolution, not a fail-closed 404. fix(server): is the correct semver signal: public exports unchanged, _normalize_host retained as an alias, no model shape touched.

Things I checked

• Never-raises holds for str input. ipaddress.ip_address raises only ValueError (caught at both short-circuits); urlsplit/.hostname raise only ValueError (caught; .port — the one that raises separately — is never accessed); canonicalize_host raises only idna.IDNAError (a UnicodeError subclass) or ValueError, both caught by except (UnicodeError, ValueError). Attacker-controlled Host 404s instead of 500ing. fail-closed beats fail-open.
• ASCII fast-path equivalence is sound. For lowercased, non-IP, single-dot-stripped ASCII, idna.encode(uts46=True, transitional=False) either returns the identical bytes or raises — and the raise falls back to exactly that value. No ASCII input maps to a different non-raising value, so skipping the adcp.signing import (30 modules, ~0.2s) on the ASCII path costs no correctness. Load-bearing for the storyboard readiness budget, per the commit body.
• Both ip_address short-circuits are necessary. First handles bare/bracketed literals before urlsplit reads 2001:db8::1 as host="2001"; second re-canonicalizes the de-bracketed uncompressed address urlsplit produces from [..]:port — which is what makes [2001:DB8::0:1]:443 and the bare form converge. The follow-up commit that added the second one is the load-bearing half.
• No circular import. tenant_router imports only stdlib at module level and defers canonicalize_host into the function body; tenant_registry → tenant_router is a one-way edge. adcp.server.__init__ importing tenant_registry before tenant_router resolves cleanly.
• Backward compat intact. _normalize_host alias preserved; register/resolve stay symmetric through the same helper.
• Fail-soft opens no bypass. The raw.lower() and except → return host fallbacks preserve the non-canonical string, which won't equal any canonicalized registered key — unparseable input 404s rather than matching an unintended tenant. (security-reviewer: no High.)

Follow-ups (non-blocking — file as issues)

• Registration-collision detection before any self-service onboarding. security-reviewer (Medium, self-serve; Low, operator-seeded): both consumers are last-write-wins — tenant_registry.py:301 (self._host_map[...] = tenant_id) and the InMemory dict comprehension. The wider folding (userinfo strip, UTS-46 confusables like ff→ff / soft-hyphen, trailing-dot) strictly enlarges the set of distinct inputs that collapse onto one key versus the old first-colon split. Operator-seeded tables are not attacker-reachable, so this doesn't block — but if adopters ever let tenants register their own agent_url, a confusable-host registration silently overwrites a victim's key. Reject-on-collision (raise or log-and-refuse) rather than silent overwrite, at both sites.
• Stale re-seed for existing adopters. DBs keyed to the old split(":",1)[0] output (IPv6, userinfo, trailing-dot forms) 404 until re-seeded. The examples/ comment flags it; a CHANGELOG/release note beyond the PR body would help adopters whose upstream cache or WAF keys on the raw Host.

Minor nits (non-blocking)

1. Idempotency claim is false for multi-trailing-dot hosts. The docstring calls idempotency load-bearing, but the ASCII path strips a single trailing dot: normalize_host_key(\"acme.example.com..\") → \"acme.example.com.\", and a second pass → \"acme.example.com\", so f(f(x)) != f(x). A non-ASCII .. host strips a second dot inside canonicalize_host, so the two paths also disagree. Practical impact is nil — a .. Host just 404s and nobody registers such a key — but it contradicts a documented invariant, and test_normalize_host_key_folds_trailing_root_dot only covers the single-dot case. host = host.rstrip(\".\") before the isascii() branch closes it; or soften the docstring. Notable, given the known-wart section already calls out the double dot-strip and stops one step short of this.
2. never raises is scoped to str. A non-str value raises AttributeError at value.strip(). Every real caller passes the Host header or a netloc, so it's within contract — one word in the docstring would make the guarantee unconditional.

LGTM. Follow-ups noted below.

[bokelley]

Verified the extracted host is re-run through IP canonicalization, making port-bearing non-canonical IPv6 values converge with their bare form. My previous request is fully addressed.