From 64bfd6d4a8440d8e2c5792e58b702e3b57f77ada Mon Sep 17 00:00:00 2001 From: "Constantine.mirin" Date: Wed, 29 Jul 2026 11:32:30 +0200 Subject: [PATCH] fix(signing): preserve a trailing empty query in @target-uri `urlsplit` maps both `/p` and `/p?` to `query == ""`, and `urlunsplit` emits no `?` for an empty string, so the two URLs collapsed to one signature base. A signer that sent `/p?` and a verifier that reconstructed `/p` produced identical bytes for different URLs and agreed -- the confusion `@target-uri` exists to prevent. The distinction is already lost by the time the split result exists, so it is recovered from the raw URL. The fragment is excluded first: in `https://e.com/p#f?x` the `?` belongs to the fragment and must NOT be resurrected, while `https://e.com/p?#frag` must keep its `?`. No vector combines a fragment with a query, so a plain `"?" in url` test passes the vector and gets that case wrong. Retires the last entry in KNOWN_CANONICALIZATION_GAPS. All 31 canonicalization cases now pass with zero xfail and zero skip. The ledger mechanism stays -- deleting it would re-create #975 at the next vector refresh. Fixes #979. --- src/adcp/signing/canonical.py | 12 +++++++++++- tests/conformance/signing/test_canonicalization.py | 6 +----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/adcp/signing/canonical.py b/src/adcp/signing/canonical.py index 9c7acd6d..befae2a4 100644 --- a/src/adcp/signing/canonical.py +++ b/src/adcp/signing/canonical.py @@ -126,7 +126,17 @@ def canonicalize_target_uri(url: str) -> str: path = "/" # RFC 9421 §2.2.2 + RFC 7230 §5.5: effective request URI excludes the # fragment (client-local, never sent on wire). - return urlunsplit((scheme, netloc, path, parts.query, "")) + target = urlunsplit((scheme, netloc, path, parts.query, "")) + if not parts.query and "?" in url.split("#", 1)[0]: + # `urlsplit` maps both `/p` and `/p?` to `query == ""`, and `urlunsplit` + # emits no `?` for an empty string -- so the two collapse to one + # signature base. A signer that sent `/p?` and a verifier that + # reconstructs `/p` then sign different bytes for different URLs and + # agree, which is exactly the confusion `@target-uri` exists to prevent. + # The distinction has to be recovered from the raw URL because it is + # already lost by the time `parts` exists. + target += "?" + return target def canonicalize_authority(url: str) -> str: diff --git a/tests/conformance/signing/test_canonicalization.py b/tests/conformance/signing/test_canonicalization.py index 96a4da10..3b7c6590 100644 --- a/tests/conformance/signing/test_canonicalization.py +++ b/tests/conformance/signing/test_canonicalization.py @@ -29,11 +29,7 @@ # Cases from canonicalization.json that the SDK does not yet satisfy, mapped to # the issue tracking each gap. Marked strict so a fix XPASSes and forces the # entry to be retired instead of lingering. -KNOWN_CANONICALIZATION_GAPS: dict[str, str] = { - # urlunsplit() cannot distinguish "no query" from "empty query", so the - # trailing '?' is dropped and signer/verifier can disagree on the base. - "trailing-empty-query-preserved": "#979: trailing '?' with an empty query is dropped", -} +KNOWN_CANONICALIZATION_GAPS: dict[str, str] = {} def _vectors_with_expected_base() -> list[tuple[str, Path]]: