Three divergent Host-header normalizers: all mangle IPv6 authorities, one leaks userinfo into the tenant key

[KonstantinMirin]

Summary

Three separate implementations of "normalize a Host header for tenant lookup" exist in the tree. They disagree with each other, all three mangle IPv6 authorities, and one leaks userinfo into the lookup key. One of the three is in examples/, which is where adopters copy from.

The three copies

	location	body
A	src/adcp/server/tenant_router.py:443-454 (_normalize_host)	host.strip().lower(), then split(":", 1)[0]
B	src/adcp/server/tenant_registry.py:227-246 (_normalize_host)	urlparse(raw).netloc or raw, then rsplit(":", 1)[0], then .lower()
C	examples/v3_reference_seller/src/tenant_router.py:58 (inlined in resolve())	host.strip().lower().split(":", 1)[0]

Reproduce

from adcp.server.tenant_router import _normalize_host as A
from adcp.server.tenant_registry import TenantRegistry
B = TenantRegistry._normalize_host
def C(h): return h.strip().lower().split(":", 1)[0]

input	A (router)	B (registry)	C (example)
[::1]:8080	'['	'[::1]'	'['
[::1]	'['	'[:'	'['
[2001:db8::1]:443	'[2001'	'[2001:db8::1]'	'[2001'
ACME.example.com:443	'acme.example.com'	'acme.example.com'	'acme.example.com'
https://user:pw@acme.example.com	'https'	'user'	'https'
acme.example.com.	'acme.example.com.'	'acme.example.com.'	'acme.example.com.'

Three defects visible in that table:

1. IPv6 authorities are destroyed. A and C split on the first colon, so any bracketed IPv6 Host collapses to '[' or '[2001'. B splits on the last colon and so keeps the brackets it should have stripped ('[::1]') — and for a portless [::1] produces '[:'.
2. B accepts userinfo into the key. It uses .netloc, not .hostname, so https://user:pw@acme.example.com normalizes to 'user'. B's docstring says it accepts full URLs, so this is a reachable input for resolve_by_host / resolve callers who pass an agent_url.
3. No trailing-dot handling anywhere. acme.example.com. and acme.example.com are different tenants in all three.

Row 5 for A and C ('https') is an artifact of feeding them a URL, which their contract does not promise to accept — not a defect on its own, but it shows how far apart the three contracts have drifted while all three are named the same thing.

Impact

Fail-closed in all three cases: a mangled key matches no tenant row, and the request 404s. There is no cross-tenant resolution here — no realistic tenant is registered under '[' or 'user'. So this is a correctness and consistency defect, not an isolation break.

What it costs in practice:

• A deployment reached over an IPv6 literal (health checkers, direct-IP probes, dual-stack ingress that forwards the literal) 404s on every request with no diagnostic pointing at host parsing.
• An adopter who registers a tenant by agent_url through TenantRegistry and happens to have userinfo in that URL registers it under a garbage key.
• Copy C is in the reference seller example, so the bug propagates into adopter code by design.

Related hazard, worth deciding before fixing

The obvious fix — route all three through adcp.signing._idna_canonicalize.canonicalize_host, which strips brackets, lowercases, strips one trailing dot and A-label-encodes — is not a drop-in here. The Host header is attacker-controlled, and canonicalize_host raises idna.IDNAError on underscore hosts and over-long labels. An unwrapped call turns today's clean 404 into an unhandled exception on the ASGI path, i.e. trades a correctness bug for a reachable 500. Whatever the fix is, it needs an explicit except that degrades to "no tenant" rather than propagating.

urlsplit(f"//{host}").hostname is the cheaper option if the IDNA half is not wanted: it de-brackets IPv6, lowercases, strips the port correctly, and drops userinfo, without introducing a raising path.

Suggested direction

One shared helper, used by all three sites, with an explicit non-raising contract (returns the best-effort key, never raises). Update the example in the same change so it stops being a divergent copy — otherwise the next adopter reintroduces C.

[bokelley]

Triage

Classification: Bug — three divergent host normalizers, all mangling IPv6 authorities; one leaks userinfo into the tenant key
Bucket(s): server, examples (no repo labels for either)
Status: ready-for-human

What the experts said:

• code-reviewer: urlsplit(f"//{raw}") is wrong for full-URL inputs — it parses the scheme as the hostname, so https://acme.example.com normalizes to 'https'. No tests cover IPv6 or port-stripping in any of the three paths today. Both are blockers before merging.
• security-reviewer: .hostname raises ValueError on unclosed IPv6 brackets (e.g. Host: [bad_ipv6) — the proposed helper is not actually non-raising, so a single crafted request converts today's fail-closed 404 into a reachable unauthenticated 500 on the ASGI path. Also flagged: returning "" on parse failure risks matching a catch-all tenant if any adopter registers "" as a key. Verdict: security-sensitive, flag for human review.

My take: Both experts agree on the consolidated-urlsplit direction, but the security reviewer found that the naive implementation trades a fail-closed 404 for a reachable single-packet DoS — a net regression. The fix is correct in intent but requires the non-raising contract to be enforced explicitly.

Required before implementing:

• Branch on "://" before prepending // (full-URL registration path)
• Wrap .hostname access in try/except (ValueError, UnicodeError): return ""
• Guard if not normalized: return None before each _host_map lookup in resolve() paths
• Use h[:-1] if h.endswith(".") else h, not rstrip("."), for trailing-dot stripping
• Parameterized tests: [::1]:8080, [::1], bare host with port, full URL with userinfo, trailing dot

@bokelley — security reviewer flagged Q1 as a reachable single-packet DoS if the try/except guard is missing. Please review the non-raising contract approach before authorizing.

---

Triaged by Claude Code.

---

Generated by Claude Code