Repository navigation
sanitize_public_auth_url's "host must be public" check only inspects IP literals #210
Description
Found by a sweep for the #202 shape — a decision that returns a permissive value from an except block. This is the one candidate of five that is not already deliberate.
What it does
_is_public_auth_host (src/pmcp/auth.py:127):
def _is_public_auth_host(hostname: str) -> bool:
if hostname.lower() == "localhost":
return False
try:
address = ip_address(hostname)
except ValueError:
return True # <- any DNS name
return not (address.is_private or address.is_loopback
or address.is_link_local or address.is_multicast
or address.is_unspecified)The except ValueError branch fires when the hostname is not an IP literal — i.e. for every DNS name — and returns True, meaning "public". The caller at :162 then allows it:
if not _is_public_auth_host(hostname):
raise ValueError("Public auth URL host must be public.")Measured
10.0.0.5 public=False rejected
127.0.0.1 public=False rejected
169.254.169.254 public=False rejected
localhost public=False rejected
internal.corp public=True ACCEPTED
metadata.google.internal public=True ACCEPTED
example.com public=True ACCEPTED
And end to end: sanitize_public_auth_url("https://metadata.google.internal/token") returns normally.
So the guard is effective against IP literals — including link-local 169.254.169.254, which it correctly rejects — and inert against names. A DNS name resolving to a private or link-local address passes a check whose error message says "host must be public".
Why this is not obviously "just resolve the name"
Resolving at validation time brings its own problems: a TOCTOU gap between validation and use, a DNS lookup on a user-supplied string (itself an SSRF-adjacent primitive), and failure behaviour to decide when resolution fails. So the fix is a design question, not a one-liner, which is why this is an issue rather than a PR.
Plausible directions, in rough order of cost:
- Document the actual guarantee and rename or reword so the check does not read as stronger than it is. It genuinely stops the common accident of pasting a loopback or RFC1918 literal.
- Allowlist the expected auth hosts, if the set is knowable, which sidesteps resolution entirely.
- Resolve and re-check, with an explicit decision about resolution failure and about pinning the resolved address for the subsequent connection.
Severity
Depends on whether an attacker can influence the auth URL, which I have not established — I found this by reading, not by tracing reachability. It is not obviously exploitable, and the IP-literal cases are handled correctly. Filing it because the gate's stated guarantee is stronger than its behaviour, which is the same class of defect as several corrected this week: a claim the code does not support.
Sweep context
The other four permissive-return sites are fine: identity.py:235 is a documented trade to avoid the #84 import-crash class and warns; auth.py:586/590 are parse helpers; handlers.py:1325 returns an empty registry, which is the correct empty state.
Activity
docs(readme): reflow the auth paragraph after the wording change (#210)
test(auth): name the int-quirk test for what it asserts (#210)
fix(auth): match ISATAP on its full interface identifier, not the 5ef…
fix(auth): classify IP literals correctly, including embedded and num…
ViperJuice commentedon Aug 30, 2026
Closed by #212 (merged as
a9842bc). Verified onmain: 17 probe hosts, 0 mismatches.Two distinct defects, neither of which this issue originally described. I filed it about DNS names being accepted unresolved — that turned out to be the deferred part, now #211. The real, provable defects were in the IP-literal path I had called effective.
1. The classifier subtracted a list of bad properties and had holes.
is_private or is_loopback or is_link_local or is_multicast or is_unspecifiedmisses RFC 6598 shared space and IPv6 site-local, and misses every IPv6 form that carries an IPv4 address in its low 32 bits. Now: unwrap RFC 4291 mapped and compatible, RFC 6052 NAT64, and RFC 5214 ISATAP, then classify positively asis_global and not is_site_local and not is_multicast.2. Legacy numeric forms bypassed the check entirely.
ip_address()raises on them, and the code treated "raised" as "this is a DNS name, accept it" — conflating a name whose target is unknowable with an address written in a form this code cannot parse.0xA9FEA9FEand2852039166were accepted and resolve to 169.254.169.254;0177.0.0.1to127.0.0.1. Fixed by canonicalising through theinet_atongrammar — not by resolving. A host is rejected if any valid reading is non-public, so010.0.0.1fails on its decimal reading.22 literals go from accepted to rejected, all demonstrated RED against a clean
origin/mainexport at Python 3.10 and 3.12.Six plan revisions, each wrong in a way the next caught, recorded because the pattern is the lesson: a severity claim that no attacker path existed (disproved); an acceptance criterion that would have frozen the bugs; a
is_globalrecipe that leftfec0::1broken and would have regressed multicast; anis_reservedvariant that rejected::ffff:8.8.8.8; incomplete unwrapping missing IPv4-compatible and ISATAP; and finally an ISATAP check matching the5efemarker hextet alone, which over-rejected the genuinely global2606:4700::1234:5efe:a00:5. RFC 5214 requires the full00-00-5E-FE/02-00-5E-FEinterface identifier.That last one is the instructive failure. CI stayed green through it because the test matrix enumerated the reject side thoroughly and under-specified the accept side — a matrix built from known-bad inputs tests one direction only. The over-rejection trap is now a must-accept row that fails against the old predicate.
Still open, deliberately: a DNS name is accepted without resolution, and a trailing-dot IPv4 (
169.254.169.254.) takes the name path. Both are name-shaped and tracked in #211, along with the trust split,fetch_json_metadata, and the relay path.README.mdandSECURITY.mdno longer promise a verified public host, and say what is actually checked.chore: v2.7.2 (#213)