Skip to content

Commit e8bd78f

Browse files
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.
1 parent 7548cc8 commit e8bd78f

2 files changed

Lines changed: 12 additions & 6 deletions

File tree

src/adcp/signing/canonical.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,17 @@ def canonicalize_target_uri(url: str) -> str:
126126
path = "/"
127127
# RFC 9421 §2.2.2 + RFC 7230 §5.5: effective request URI excludes the
128128
# fragment (client-local, never sent on wire).
129-
return urlunsplit((scheme, netloc, path, parts.query, ""))
129+
target = urlunsplit((scheme, netloc, path, parts.query, ""))
130+
if not parts.query and "?" in url.split("#", 1)[0]:
131+
# `urlsplit` maps both `/p` and `/p?` to `query == ""`, and `urlunsplit`
132+
# emits no `?` for an empty string -- so the two collapse to one
133+
# signature base. A signer that sent `/p?` and a verifier that
134+
# reconstructs `/p` then sign different bytes for different URLs and
135+
# agree, which is exactly the confusion `@target-uri` exists to prevent.
136+
# The distinction has to be recovered from the raw URL because it is
137+
# already lost by the time `parts` exists.
138+
target += "?"
139+
return target
130140

131141

132142
def canonicalize_authority(url: str) -> str:

tests/conformance/signing/test_canonicalization.py

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,7 @@
2929
# Cases from canonicalization.json that the SDK does not yet satisfy, mapped to
3030
# the issue tracking each gap. Marked strict so a fix XPASSes and forces the
3131
# entry to be retired instead of lingering.
32-
KNOWN_CANONICALIZATION_GAPS: dict[str, str] = {
33-
# urlunsplit() cannot distinguish "no query" from "empty query", so the
34-
# trailing '?' is dropped and signer/verifier can disagree on the base.
35-
"trailing-empty-query-preserved": "#979: trailing '?' with an empty query is dropped",
36-
}
32+
KNOWN_CANONICALIZATION_GAPS: dict[str, str] = {}
3733

3834

3935
def _vectors_with_expected_base() -> list[tuple[str, Path]]:

0 commit comments

Comments
 (0)