fix(signing): idna-normalize hosts in etld+1 binding - #995
Merged
bokelley merged 1 commit intoJul 29, 2026
Merged
Conversation
tldextract is IDNA-agnostic: given a U-label it returns a U-label. Because host_from only case-folded, registrable_domain emitted 'straße.de' for the U-label spelling and 'xn--strae-oqa.de' for the A-label spelling of the same host, so same_registrable_domain compared two strings that can never be equal and returned False. That predicate is step 2a of the brand-authorization binding (brand_authz.py:255) and the redirect-containment gate (adagents.py:404). A brand whose brand.json and agent URL disagreed on spelling had its legitimate agent refused with reason="binding_failed"; the authorized_operators[] fallback compared the same mis-normalized values and missed too. Nothing was wrongly accepted, but a correct agent could not bind. Both branches of host_from now delegate to _idna_canonicalize.canonicalize_host — the same normalizer jwks, revocation_fetcher and key_origins already use. etld was the last signing module still on a bare .lower() after PR adcontextprotocol#777. That also fixes a second asymmetry the old docstring got wrong: the URL branch trimmed no trailing root dot while the bare-host branch trimmed all of them, so 'https://Example.COM./' and 'Example.COM.' normalized differently. registrable_domain maps idna.IDNAError onto None rather than letting it propagate or falling back to the raw string. Fail-closed is deliberate: - Propagating is not viable. IDNAError is a ValueError subclass, so it would be silently reclassified as brand_domain_invalid at one callsite and escape uncaught at five others mid-verification. - Failing open would loosen an existing check. adagents._idna_ascii_host is today the only gate rejecting an IDNA-invalid redirect target (_check_safe_host does not reject underscores). With a raw-string fallback, 'under_score.brand.com' would reduce to 'brand.com', match the origin, and the redirect would be accepted. - It widens a category the module already documents rather than inventing a contract: a string that is not an encodable hostname has no derivable eTLD+1, alongside IP literals and single-label hosts. The cost, stated plainly: hosts with an underscore label, a label over 63 bytes, or a leading/trailing-hyphen label now yield None instead of an eTLD+1. These are not valid hostnames per RFC 952/1123 and cannot appear in a fetchable https:// agent URL, but this is the one place an agent that binds today stops binding. Adopter-visible: registrable_domain and host_from are public exports and now return A-labels for IDN input; host_from additionally raises on IDNA-invalid input where it previously returned the string unchanged. Both sides of same_registrable_domain move together, so the predicate stays correct. adagents._idna_ascii_host is left in place — canonicalize_host is idempotent on the A-label output it produces, so it becomes a harmless double normalization. Removing that duplicate belongs in a follow-up that touches adagents. Fixes adcontextprotocol#988
Contributor
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
bokelley
approved these changes
Jul 29, 2026
bokelley
left a comment
Contributor
There was a problem hiding this comment.
Reviewed against current main. The shared IDNA canonicalizer makes U-label/A-label eTLD+1 comparisons converge while preserving the fail-closed behavior for invalid hosts. Focused integration coverage passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #988.
Independent of the signing stack (#980/#985/#987/#993) — branched off
main, touches no file any of them touch.What was wrong
etld.host_fromapplied no IDNA conversion, sosame_registrable_domain— the eTLD+1 gate the brand-authorization binding runs on — returnedFalsewhen the two sides spelled the same IDN host differently. A brand publishingbrand.jsonunder a U-label host and listing its agent under the A-label form (or the reverse) failed the binding.It was also asymmetric with itself: the URL branch never stripped the trailing FQDN dot despite the docstring claiming it did, so
host_from("https://Example.COM./")→"example.com."whilehost_from("Example.COM.")→"example.com".Measured before / after:
How
Both branches of
host_fromnow delegate to_idna_canonicalize.canonicalize_host— the helper #777 established and thatjwks,revocation_fetcher,key_originsandip_pinned_transportalready use. This finishes that migration rather than forking it;etld.pywas simply missed in the original pass. No new normalizer: the change deletes hand-rolled logic rather than adding a seventh implementation to a tree that already had six.The fail-open vs fail-closed decision
canonicalize_hostraises on underscore hosts, labels over 63 bytes, and leading hyphens, wherehost_frompreviously passed them through. The existing callers are split —jwksandrevocation_fetcherfail closed,ip_pinned_transportandkey_originsfail open — so "match the convention" was not available as an answer.This fails closed:
registrable_domainalready documents aNonecontract for unusable input, and a binding gate that cannot canonicalize a host should refuse to bind rather than guess.idna.IDNAErrorsubclassesUnicodeErrorsubclassesValueError, sobrand_authz.py:207's existingexcept ValueErrorstill catches it — an IDNA-invalidbrand_domaindegrades toreason="brand_domain_invalid"instead of escaping mid-verification. The reasoning is in the module docstring and the commit message, as #988 asked.Adopter-visible changes
registrable_domainreturns A-labels for IDN input (xn--strae-oqa.de, notstraße.de).host_fromnow raises for hosts IDNA cannot process;registrable_domainmaps that toNone.same_registrable_domain("example.com", "example.com")is nowTrue. Correct IDNA behaviour and consistent with how DNS resolves it, but it is a widening beyond the U-label/A-label case the issue described, so calling it out explicitly.Test plan
tests/conformance/signing/test_etld.py, new. 7 of its 10 tests fail against the pre-fix module — verified by copying the test file alone onto the merge base and running it, not by reasoning. The other three are deliberate regression fences (empty/dot-only still raise; IP literals still returnNone).The tests use
.derather than.example, because.exampleis not in the public suffix list and an eTLD+1 assertion against it would pass vacuously.Full suite on the branch: 5906 passed, 41 skipped, 1 xfailed. ruff, ruff format and mypy clean.
Not done here
#988 mentions
adagents._idna_ascii_hostas a candidate for deletion. Left alone deliberately — its claimed divergences turned out to be unreachable (whitespace is stripped upstream,_check_safe_hostruns first, andidna.encodealready defaultstransitional=False), so removing it is a cleanup with no behaviour change and belongs in its own PR.