fix(signing): preserve a trailing empty query in @target-uri

[KonstantinMirin]

Stacked on #985, which is itself stacked on #980 — merge order: #980 → #985 → this.
Own commit: 81648470. Everything else in the diff below belongs to #980 and #985.
Incremental diff: KonstantinMirin/adcp-client-python@fix/target-uri-malformed-authority...fix/trailing-empty-query

Fixes #979.

What was wrong

urlsplit maps both /p and /p? to query == "", and urlunsplit emits no ? for an empty string. So https://seller.example.com/p? and https://seller.example.com/p produced the same signature base: a signer that sent /p? and a verifier that reconstructed /p signed identical bytes for different URLs and agreed — exactly the confusion @target-uri exists to prevent.

The fix

The distinction is already gone by the time the split result exists, so it is recovered from the raw URL — but the fragment has to be excluded first, and that is the part worth reviewing:

input	correct output	a plain "?" in url test
https://e.com/p?	https://e.com/p?	✅
https://e.com/p?#frag	https://e.com/p?	✅
https://e.com/p#f?x	https://e.com/p	❌ resurrects a ? that belongs to the fragment

No vector combines a fragment with a query, so the naive test passes the conformance case and still gets the third row wrong. Splitting on # first is what makes it correct rather than merely green.

Why this had to be fixed here

No downstream consumer can prove this fix. ASGI hands query_string=b"" for both /p and /p?, so the distinction is destroyed before any verifier sees it. Same reason #977's IDN cases are producer-side. That is the argument for fixing it in the SDK against canonicalization.json directly rather than waiting for an adopter to demonstrate it — no adopter can.

Ledger

Retires the last entry in KNOWN_CANONICALIZATION_GAPS. All 31 canonicalization cases now pass with zero xfail and zero skip.

The ledger mechanism stays in place, empty. Deleting it would re-create #975 the next time the vectors are refreshed — an empty ratchet is the point.

Test plan

make ci-local: 5954 passed, 0 failed, 41 skipped, 5 xfailed (#985's baseline: 5953 / 6). The one retired xfail becomes the one new pass.

The four remaining xfails in the signing tree are negative/021, 022, 023 and 026 — all #976, which is PR B.

[aao-ipr-bot]

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

_{This is an automated message from the Argus AI review workflow.}

[aao-ipr-bot]

Clean fix for a real signature-confusion bug. Right shape: urlsplit collapsed /p? and /p to one @target-uri, so a signer and verifier signed identical bytes for different URLs and agreed — recovering the trailing ? from the raw URL, after excluding the fragment, is exactly the distinction @target-uri exists to preserve.

Stacked PR — own commit is 81648470 (fix(signing):). Everything else (vector vendoring, malformed-authority gate, IDN A-label conversion) belongs to #980/#985 and merges first. I weighted the review toward the own commit and treated the rest as inherited context.

Things I checked

• Own-commit logic at canonical.py:132 (if not parts.query and "?" in url.split("#", 1)[0]). For a well-formed URL the first ? before # is always the query delimiter, so an empty parts.query with a pre-fragment ? unambiguously means a trailing empty query. /p? keeps ?, /p?#frag keeps ?, /p#f?x drops it. No spurious add, no needed drop. ad-tech-protocol-expert: vector-backed — canonicalization.json ships trailing-empty-query-preserved distinct from no-query, correct per RFC 3986 §3.4/§6.2.3 and RFC 9421 §2.2.2.
• No signer/verifier asymmetry — both sides call the single canonicalize_target_uri, so the derivation is deterministic on the input string. security-reviewer: no two URLs collapse to one base, no URL maps to two bases. The confusion vector is closed, not moved.
• All six malformed-authority reject vectors raise TargetUriMalformedError with code == request_target_uri_malformed — including https://[::1/p, where _split_or_reject (canonical.py:95-98) normalizes urlsplit's own bare ValueError onto the typed error so the vector stops passing on someone else's exception.
• Verifier exception ordering — except TargetUriMalformedError precedes except (ValueError, KeyError) at verifier.py:316. Load-bearing: the typed error subclasses ValueError, so a reorder silently downgrades every malformed-authority rejection to request_signature_header_malformed. Guarded by test_target_uri_malformed_boundary.py.
• Fail-closed host canonicalization — _canon_host re-raises IDNA/Unicode failures as TargetUriMalformedError; security-reviewer confirmed no path yields a signature base over a host that could not be canonicalized. Fail-closed beats fail-open.
• No import cycle — errors.py → canonical.py → (_idna_canonicalize, idna); canonical.py declares REQUEST_TARGET_URI_MALFORMED locally rather than importing errors, keeping it a leaf. idna is already a hard dep via _idna_canonicalize.
• Test plan — make ci-local (5954 pass) is a full run; the primary change is validated by the shipped trailing-empty-query-preserved vector.

Follow-ups (non-blocking — file as issues)

• Guard the discriminating case. The PR body names https://e.com/p#f?x as the row that separates the correct split("#",1)[0] from the naive "?" in url — then ships no test for it. A regression to the naive test passes every shipped vector and the suite stays green. Add canonicalize_target_uri("https://e.com/p#f?x") == "https://e.com/p". (code-reviewer.)
• Root-dot strip is the one unblessed convention — example.com. → example.com is wire-visible, not spec-defined, and ships no vector. ad-tech-protocol-expert (sound-with-caveats): note the internal inconsistency — this stack goes to lengths to preserve the RFC-3986-distinct /p? byte while erasing the RFC-3986-distinct root dot. The forcing function is real (IDNA branch strips the dot in UTS-46 prep, ASCII branch does not), but preserve-on-both-branches stays closer to the wire than strip-on-both. This is #985's commit — push there, and confirm upstream's position before it hardens into a cross-SDK convention.
• Root-dot-only authority reduces to an empty host. https://./p is truthy, passes the empty-host gate, then _canon_host strips the dot and returns "" — an empty @authority. security-reviewer Low: not exploitable (symmetric, no real Host of .), but it defeats the gate's no-hostless-authority invariant. Reject an empty host after the root-dot strip. (#985's _canon_host.)
• Verify the vendored canonicalization.json is byte-faithful to upstream v3.1.1. ad-tech-protocol-expert: the manifest hashes are self-generated, so the completeness tests are internally circular — the only external anchor is get_adcp_spec_version(). Diff the vendored tree against dist/compliance/3.1.1/test-vectors/request-signing/ once. (#980.)

Minor nits (non-blocking)

1. Empty-path + trailing empty query is asymmetric. https://e.com?x=1 promotes the empty path to /, but https://e.com? does not — the promotion never fires for an empty query, so the new trailing-? yields https://e.com? with no path. No vector covers it and intra-SDK signer/verifier still agree; a peer that always promotes could diverge. (canonical.py, code-reviewer.)

Behaviour change worth a line in the upgrade notes: /p? now produces a different @target-uri than /p, so a signer on this SDK and a verifier whose framework strips the empty query will disagree. That is the secure direction — reject over silently agree — but adopters should expect it.

One process note: the wire-visible feat(signing): behavior changes (authority-less URIs now raise, root-dot stripped) land under #985's merge, not this one. Confirm at #985 whether those warrant feat!/BREAKING CHANGE: on public adcp.signing exports — not a #987 concern, but don't let it fall between the two PRs.

LGTM. Follow-ups noted below.

[aao-ipr-bot]

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

_{This is an automated message from the Argus AI review workflow.}

[aao-ipr-bot]

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

_{This is an automated message from the Argus AI review workflow.}

[aao-ipr-bot]

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

_{This is an automated message from the Argus AI review workflow.}

[bokelley]

Reviewed the restacked own commit. It restores the pre-fragment trailing empty-query distinction without retaining fragment-local question marks. The stacked signing suite passes locally: 533 passed, 4 expected xfails.

[aao-ipr-bot]

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

_{This is an automated message from the Argus AI review workflow.}