fix(signing): canonicalize both origins in the brand.json jwks gate - #998
Merged
bokelley merged 1 commit intoJul 29, 2026
Merged
Conversation
Two defects, and they are the same defect seen from two sides. `_canonicalize_url` rebuilt the authority from `urlsplit(...).hostname`, which returns IPv6 literals DE-bracketed. `https://[::1]/x` became `https://::1/x` -- a string with no parseable host -- and `https://[2001:db8::1]:8443/x` became `https://2001:db8::1:8443/x`, where the port is no longer separable from the address. Both are fed straight to the IP-pinned transport builder on every redirect hop, so the first refused the URL and the second re-parsed wrongly. `_default_jwks_uri` then compared a CANONICAL origin against a RAW one: `final_brand_url` has been through the canonicalizer, `agent.url` is raw as published. A publisher who spelled the same origin identically on both sides -- mixed case, an explicit :443, a trailing root dot, a U-label -- was told their agent was on a different origin from their brand.json, which it was not. That is precisely the confusing diagnostic the gate exists to avoid, produced by the gate itself. Fixing only the first would have made the second WORSE: a stronger canonicalizer on one side of an asymmetric comparison widens the gap. Both sides now run through one `_canonical_origin`. It is built from `.hostname`, not `.netloc`. `.netloc` is the one accessor that retains userinfo, so `https://user@brand.example` compared unequal to `https://brand.example` -- a trust decision turning on a credential that is not part of an origin at all. The cross-origin fence is unmoved: an attacker-controlled brand.json naming an agent on another origin is still rejected with `jwks_origin_mismatch`. Canonicalizing closes spelling differences, not origin differences. Also moves `build_async_ip_pinned_transport` inside the fetch try-block so a transport-side SSRF refusal surfaces as `fetch_failed` rather than escaping the resolver as a raw `SSRFValidationError`. Fixes adcontextprotocol#989.
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. IPv6 authorities remain parseable after canonicalization, both sides of the default-JWKS origin comparison now use the same canonical form, and the cross-origin fence remains intact. 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 #989 — both halves.
Independent of the signing stack — branched off
main, touches no file #980/#985/#987/#993 touch.Two defects, and they are the same defect from two sides
1.
_canonicalize_urldropped IPv6 brackets. It rebuilt the authority fromurlsplit(...).hostname, which returns IPv6 literals de-bracketed:Both are fed straight to
build_async_ip_pinned_transporton every redirect hop, so the first was refused outright and the second re-parsed wrongly.2.
_default_jwks_uricompared a canonical origin against a raw one.final_brand_urlhas been through the canonicalizer;agent.urlis raw as published. So a publisher spelling the same origin identically on both sides was told their agent lives somewhere else:https://Brand.Example/…jwks_origin_mismatchhttps://brand.example:443/…jwks_origin_mismatchhttps://brand.example./…jwks_origin_mismatchhttps://user@brand.example/…jwks_origin_mismatchhttps://bücher.example/…jwks_origin_mismatchhttps://evil.example/…vsbrand.examplejwks_origin_mismatchThat is exactly the confusing diagnostic the gate exists to avoid — produced by the gate itself.
Why they had to be fixed together
Fixing only #1 makes #2 worse. A stronger canonicalizer on one side of an asymmetric comparison widens the gap. An earlier draft of this change did only the bracket fix, and it broke three publisher configurations that work on
maintoday — a U-label on both sides, a trailing root dot on both sides, and an underscore host. Anyone reviewing the bracket fix in isolation should know it is not safely separable.Both sides now run through one
_canonical_origin, sharing host handling with_canonicalize_url: samecanonicalize_host, same re-bracketing, same default-port elision.It is built from
.hostname, not.netloc..netlocis the one accessor that retains userinfo, sohttps://user@brand.examplecompared unequal tohttps://brand.example— a trust decision turning on a credential that is not part of an origin at all.The security fence is unmoved
An attacker-controlled brand.json naming
agent.urlon another origin is still rejected withjwks_origin_mismatch, andtest_cross_origin_agent_is_still_rejectedpins it. Canonicalizing closes spelling differences, not origin differences — worth stating explicitly, because "we made an origin comparison more permissive" is the shape of a real vulnerability and this one is not.Also in scope, from the issue's suggested direction:
build_async_ip_pinned_transportmoves inside the fetchtryblock, so a transport-side SSRF refusal surfaces asfetch_failedinstead of escaping the resolver as a rawSSRFValidationError.On the issue's severity framing
#989 describes the origin comparison as a diagnostics/interop problem, not a trust-pivot, and that is correct — I re-verified it. The comparison is byte equality against an already-canonical value, so a normalization divergence can only make the raw side unequal; no input makes two different origins compare equal, and the defaulted
jwks_uriis built on the equality branch where both strings are identical. The user-visible cost was publishers losing the default-jwks_uripath and being told something false about their own configuration.Test plan
make ci-local: 5913 passed, 0 failed, 41 skipped, 1 xfailed.17 tests in
tests/conformance/signing/test_brand_jwks_canonicalization.py.For the origin-gate tests specifically: 4 of the new ones fail with
src/adcp/signing/brand_jwks.pyreverted to base and the test file kept — verified by stashing only the source file, since stashing everything removes the tests too and proves nothing. The remaining rows (u-label,ipv6, and the cross-origin fence) pass either way and are regression guards rather than new coverage; calling that out rather than claiming nine red tests.