Skip to content

Commit 0830b4e

Browse files
fix(signing)!: apply canonicalization step 5 whatever the query does (#1456)
Step 5 of the AdCP URL-canonicalization algorithm substitutes `/` for an empty path when an authority is present. `canonicalize_target_uri` gated that on the query string as well, so `https://host` canonicalized to `https://host` where the algorithm and RFC 3986 section 6.2.3 both require `https://host/`. Only the queried spelling got its slash. The gate was wrong on its own terms too: `https://host?` carries a `?` in the raw URL but `urlsplit` reports an empty query, so the condition read false and the output was `https://host?` -- a path-less URI no RFC 3986 normalizer produces. `canonicalization.json` cannot detect either case. Of its 37 cases exactly one has an empty path, and that one carries `?x=1`, which is why the divergence passed 37/37. The consequence reaches past request signing. This function is the identifier comparer for `adagents.json` authorized_agents urls, `brand.json` agents urls, the TMP seller agent url and the governance issuer, so a publisher who registered an agent without a trailing slash did not match a caller that arrived with one, and the refusal named authorization rather than canonicalization. On the identifier surfaces the new condition is strictly more permissive: every input that received `/` before still does. On the signing path the `@target-uri` base changes for the empty-path spellings, which is a wire change, so the commit carries the breaking marker and the upgrade is sequenced in `MIGRATION_v8_to_v9.md` and section 6 of `docs/request-signing-migration.md`. Vector suite stays 37/37. BREAKING CHANGE: `canonicalize_target_uri` returns `https://host/` for an authority-only URL where it returned `https://host`, so the `@target-uri` signature base changes for the bare-authority, non-default-port, IPv6-literal, default-port and trailing-`?` spellings. A signer on this release and a verifier on an earlier one derive different bases for the same request, and verification fails with `request_signature_invalid`. Dial signed clients at the explicit root path -- `https://host/` canonicalizes the same way before and after, so both ends can then upgrade in any order -- or move an agent URL you cannot respell with its verifier in one window, or hold the affected operation at `warn_for` across that window. Identifier matching on `adagents.json`, `brand.json`, TMP `seller_agent.agent_url`, `format-id.agent_url` and the governance issuer only becomes more permissive: the two spellings converge, so a lookup that refused before can now succeed and none that matched stops matching. Migration note: `MIGRATION_v8_to_v9.md` and `docs/request-signing-migration.md` section 6.
1 parent 0c7e8b8 commit 0830b4e

4 files changed

Lines changed: 114 additions & 1 deletion

File tree

‎MIGRATION_v8_to_v9.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ changes below before upgrading each client, server and reporting worker.
1717
| Format-kind fields preserve open strings | Replace enum identity comparisons with string equality. Opt into `CanonicalFormatKindStr` when your application needs a closed vocabulary. See [format-kind validation](docs/canonical-format-kinds-migration.md). |
1818
| Reporting generations require authenticated caller ownership | Include `consumer_id` in custom store/binding implementations and typed constructors. Migrate populated ledgers with stopped workers; an entirely empty ledger can use the explicit replacement API. See [reporting ownership migration](docs/reporting-caller-ownership-migration.md). |
1919
| New source requests and sealed artifacts include their owning consumer | Read `request.identity.consumer_id` when implementing adapters. Historical ownerless artifacts retain their original hashes, but active owned generations refuse ownerless or differently owned frozen acquisitions; reconcile retained state and admit a new generation instead of relabelling evidence. See [reporting ownership migration](docs/reporting-caller-ownership-migration.md). |
20+
| URL canonicalization gives an empty path its `/` whatever the query does | The `@target-uri` signature base changes for bare-authority URLs such as `https://seller.example.com`. Dial signed clients at the explicit root path, which canonicalizes identically on both versions, or move an unrespellable agent URL's signer and verifier in one window. Identifier matching on `adagents.json`, `brand.json`, TMP and the governance issuer only becomes more permissive. See [the empty-path canonicalization fix](docs/request-signing-migration.md#6-upgrading-across-the-empty-path-canonicalization-fix). |
2021
| Initial automatic source reads default to a deterministic five-minute jitter window | Reads wait for the selected offering's declared availability and then spread within the available delivery headroom. Configure `read_jitter_window=timedelta(0)` to opt out. Pass an explicit window to custom producer factories only when they accept and forward the new keyword. Manual acquisitions, persisted retries and official-close timing retain their behavior. See [reporting service timing](docs/reliable-reporting-service.md). |
2122

2223
The credential rule applies to all configured carriers, including opted-in

‎docs/request-signing-migration.md‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,33 @@ Ordering is different — the old kid must stop being trusted *before* anything
314314
- [ ] Redirect-following is disabled on every signing client.
315315
- [ ] Clock sync monitoring is in place on signer and verifier hosts.
316316

317+
## 6. Upgrading across the empty-path canonicalization fix
318+
319+
`canonicalize_target_uri` substitutes `/` for an empty path whenever an authority is present, which is what step 5 of the [AdCP URL-canonicalization algorithm](https://github.com/adcontextprotocol/adcp/blob/main/docs/reference/url-canonicalization.mdx) and RFC 3986 §6.2.3 require. Earlier releases applied the substitution only when the URL also carried a query string, so an empty path kept its emptiness unless a `?x=1` followed it:
320+
321+
| URL dialled | Earlier releases | This release |
322+
| --- | --- | --- |
323+
| `https://seller.example.com` | `https://seller.example.com` | `https://seller.example.com/` |
324+
| `https://seller.example.com:8443` | `https://seller.example.com:8443` | `https://seller.example.com:8443/` |
325+
| `https://[2001:db8::1]` | `https://[2001:db8::1]` | `https://[2001:db8::1]/` |
326+
| `http://seller.example.com:80` | `http://seller.example.com` | `http://seller.example.com/` |
327+
| `https://seller.example.com?` | `https://seller.example.com?` | `https://seller.example.com/?` |
328+
329+
A URL with any non-empty path, and `https://seller.example.com?x=1`, canonicalize exactly as they did before.
330+
331+
### On the signing path
332+
333+
`@target-uri` is part of the signature base, so for the URLs above the signed bytes change. A signer on this release and a verifier on an earlier one — or the reverse — derive different bases for the same request, and verification fails with `request_signature_invalid`. Nothing in that error names canonicalization, so sequence the upgrade rather than diagnosing it afterwards.
334+
335+
Two ways across:
336+
337+
- **Dial the explicit root path.** `https://seller.example.com/` canonicalizes to `https://seller.example.com/` on every release, before and after this change. Point signed clients at the URL with its path spelled out and both versions derive the same base, so signer and verifier upgrade independently and in any order. Worth making permanent: an agent URL registered with its path never depended on the fix.
338+
- **Upgrade both ends together.** For an agent URL you cannot respell, move its signer and its verifier in one window the way you would a key rotation, or hold the affected operation at `warn_for` (§2, step B) across the window so a base mismatch logs instead of rejecting.
339+
340+
### Off the signing path
341+
342+
The same function is the identifier comparer for `adagents.json` `authorized_agents[].url`, `brand.json` `agents[].url`, TMP `seller_agent.agent_url`, `format-id.agent_url` and the governance issuer. There the change is strictly more permissive: `https://agent.example.com` and `https://agent.example.com/` now compare equal, where before a publisher who registered the first did not match a caller who arrived as the second, and the refusal named authorization rather than canonicalization. No registration needs editing, and no pair that matched before stops matching.
343+
317344
## Related
318345

319346
- [`adcp.signing.install_signing_event_hook`](../src/adcp/signing/client.py) — buyer-side preset for adapters not using `ADCPClient`.

‎src/adcp/signing/canonical.py‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,12 @@ def canonicalize_target_uri(url: str) -> str:
122122
scheme = parts.scheme.lower()
123123
netloc = _canon_authority(parts.netloc, scheme)
124124
path = _normalize_path(parts.path)
125-
if not path and parts.query:
125+
if not path:
126+
# Step 5: an empty path with an authority present canonicalizes to "/"
127+
# (RFC 3986 §6.2.3). The condition is the path and the authority, never
128+
# the query: `https://host` takes the slash exactly as `https://host?x=1`
129+
# does. `_canon_authority` above has already rejected an authority with
130+
# no host, so reaching this line guarantees one is present.
126131
path = "/"
127132
# RFC 9421 §2.2.2 + RFC 7230 §5.5: effective request URI excludes the
128133
# fragment (client-local, never sent on wire).

‎tests/conformance/signing/test_canonicalization.py‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,3 +264,83 @@ def test_non_digit_port_is_rejected_with_the_spec_code(url: str) -> None:
264264
with pytest.raises(ValueError) as excinfo:
265265
canonicalize_authority(url)
266266
assert getattr(excinfo.value, "code", None) == "request_target_uri_malformed"
267+
268+
269+
# ---- step 5: empty path + authority -> "/", whatever the query does ----
270+
271+
272+
@pytest.mark.parametrize(
273+
("url", "expected"),
274+
[
275+
# The canonicalization.json case, kept here so the no-query rows below
276+
# are read against the one spelling the vector set already pins.
277+
("https://seller.example.com?x=1", "https://seller.example.com/?x=1"),
278+
# No query at all -- the rows the vector set does not reach.
279+
("https://seller.example.com", "https://seller.example.com/"),
280+
("https://seller.example.com:8443", "https://seller.example.com:8443/"),
281+
("https://[2001:db8::1]", "https://[2001:db8::1]/"),
282+
("http://seller.example.com:80", "http://seller.example.com/"),
283+
# Empty query, i.e. a bare trailing "?". Step 5 supplies the slash and
284+
# step 7 keeps the "?"; the two rules compose rather than compete.
285+
("https://seller.example.com?", "https://seller.example.com/?"),
286+
],
287+
ids=[
288+
"query",
289+
"no-query",
290+
"no-query-port",
291+
"no-query-ipv6",
292+
"no-query-default-port",
293+
"empty-query",
294+
],
295+
)
296+
def test_empty_path_with_authority_becomes_slash_regardless_of_query(
297+
url: str, expected: str
298+
) -> None:
299+
"""Step 5 keys on the path and the authority, never on the query.
300+
301+
The spec's step 5 reads "If the path is empty AND an authority is present,
302+
substitute `/`"; the query appears only inside its illustrative example
303+
(`https://host?x=1` -> `https://host/?x=1`). RFC 3986 §6.2.3 is the same --
304+
scheme-based normalization gives an authority-bearing hierarchical URI the
305+
`/` path whether or not a query follows.
306+
307+
This was implemented as `if not path and parts.query`, so `https://host`
308+
canonicalized to `https://host` and only the queried spelling got its
309+
slash. `canonicalization.json` could not catch it: of its 37 cases exactly
310+
one has an empty path, and that one carries `?x=1`. The empty-query row is
311+
the same gate failing on its own terms -- `https://host?` has a `?` in the
312+
raw URL but `urlsplit` reports `query == ""`, so the condition read False
313+
and the output was `https://host?`, a path-less URI no RFC 3986 normalizer
314+
produces.
315+
316+
Both consequences are silent until they are not. On the signing path a
317+
producer applying step 5 and a comparer skipping it derive different
318+
`@target-uri` bytes for one URL, and every signature between them fails
319+
verification with nothing in either log naming the cause. Off the signing
320+
path the same function is the identifier comparer for `adagents.json`
321+
`authorized_agents[].url`, `brand.json` `agents[].url`, TMP
322+
`seller_agent.agent_url` and the governance issuer, which is what
323+
`test_bare_authority_and_trailing_slash_are_one_identifier` pins.
324+
"""
325+
assert canonicalize_target_uri(url) == expected
326+
327+
328+
def test_bare_authority_and_trailing_slash_are_one_identifier() -> None:
329+
"""`https://host` and `https://host/` are the same URL, so one canonical form.
330+
331+
Stated as a relationship rather than literals: what matters is that the two
332+
spellings converge, not which string they converge on. Before step 5 ran
333+
unconditionally they did not, and because this canonicalizer is shared by
334+
every URL-identifier surface in the SDK -- not just `@target-uri` -- a
335+
publisher who registered an agent as `https://agent.example.com` failed to
336+
match a caller arriving as `https://agent.example.com/`, and the rejection
337+
named authorization rather than canonicalization.
338+
"""
339+
for bare in (
340+
"https://agent.example.com",
341+
"https://agent.example.com:8443",
342+
"https://[2001:db8::1]",
343+
):
344+
assert canonicalize_target_uri(bare) == canonicalize_target_uri(
345+
f"{bare}/"
346+
), f"bare authority and trailing slash diverge for {bare!r}"

0 commit comments

Comments
 (0)