fix(signing): reject malformed authorities, convert IDN hosts to A-labels - #985
Conversation
|
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. |
|
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
left a comment
There was a problem hiding this comment.
Two corrections before merge:\n\n1. The spec does define the webhook twin. AdCP 3.1.1's webhook checklist names webhook_target_uri_malformed as a structural code, step 10 requires it for malformed/mismatched authority, and the webhook error taxonomy lists it explicitly: https://github.com/adcontextprotocol/adcp/blob/v3.1.1/docs/building/by-layer/L1/security.mdx#L1469-L1483 and https://github.com/adcontextprotocol/adcp/blob/v3.1.1/docs/building/by-layer/L1/security.mdx#L1549-L1567. Please add WEBHOOK_TARGET_URI_MALFORMED and map REQUEST_TARGET_URI_MALFORMED to it; mapping to webhook_signature_header_malformed emits the wrong stable wire code.\n\n2. _canon_host strips a trailing root dot after _malformed_authority_reason has accepted a non-empty host. Consequently https://./p canonicalizes to an empty authority instead of rejecting. Please reject when the post-strip host is empty and add a regression test.\n\nThe rest of the malformed-authority/IDNA implementation looks sound.
There was a problem hiding this comment.
Approving. Canonicalization is the signature base, and this closes six authority shapes the profile requires be rejected while making the signer and verifier derive the same bytes for the same host — the right shape for a signing path.
Things I checked
- Clause ordering in
verifier.pyis load-bearing and correct.except TargetUriMalformedErrorprecedesexcept (ValueError, KeyError); sinceTargetUriMalformedErrorsubclassesValueError, reversing them would silently downgrade every malformed-authority rejection torequest_signature_header_malformedwith the suite still green. Guarded bytest_target_uri_malformed_boundary.py. - The
webhook_target_uri_malformedcorrection is spec-accurate, not cosmetic.ad-tech-protocol-expert: confirmed against 3.1.1security.mdx— the code is named at line 1469, mandated at step 10 (line 1483), and listed in the webhook taxonomy (line 1566), prefix break and all. The earlier retag ontowebhook_signature_header_malformedput a stable-but-wrong code on the wire;errors.pymappingREQUEST_TARGET_URI_MALFORMED → WEBHOOK_TARGET_URI_MALFORMEDis what the spec requires. - The six rejections are one-for-one with the SHA-pinned
canonicalization.jsonreject cases — empty authority, port-without-host, userinfo-without-host, bare unbracketed IPv6, missing closing bracket, RFC 6874 zone id. Nothing the spec says to canonicalize is wrongly rejected. _canon_hostroot-dot re-check is necessary, not redundant._malformed_authority_reasonruns on the raw netloc where.and..read as non-empty hosts; only the strip empties them.if not host or any(label == "" ...)catcheshttps://./p,https://../p,https://a..b/pafter the strip, and[::1]splits to["[::1]"]so bracketed IPv6 is not false-rejected.- No import cycle.
errors → canonical → _idna_canonicalize;canonicaldeliberately does not importerrors.idna>=3.7,<4is a declared runtime dep, so the new top-levelimport idnaincanonical.pyis safe. - Fail-closed on IDNA failure is correct for this path.
_canon_hostre-raisesTargetUriMalformedErroronidna.IDNAError/UnicodeError— a signature base must never be computed over a host we could not canonicalize.security-reviewer: no authority-confusion or homograph collision; distinct codepoints map to distinct Punycode, never onto a trusted A-label.
Follow-ups (non-blocking — file as issues)
- Semver signal. Title is
fix(signing):(patch) but the diff changes two adopter-visible wire behaviors — trailing FQDN-root dot now stripped, authority-less URIs now raise — and the body argues forfeat:/minor. A rejection-set expansion arriving in a patch is the surprise the author himself flags. Reconcile the title before release-please cuts it. - Deferred verifier U-label reject.
026is held strict-xfail; between this PR and PR B a verifier silently converts a wire U-label where the spec (security.mdxreference step 2) says it MUST reject.ad-tech-protocol-expertrates it moderate,security-reviewerLow (the@authoritycomponent is covered, so an accepted request still needs a valid signature). Not a forgery window — but land PR B promptly, as the PR body itself argues. - Root-dot behavior is pinned only by an SDK-local test + #986. The spec ships no vector; stripping-before-the-branch is the internally-correct call (it makes the ASCII and UTS-46 paths agree), but it stays undefined wire behavior until upstream rules. Tracked correctly.
- Pre-existing, out of scope for this PR:
security-reviewerflagged_maybe_check_key_origininverifier.pywarns-and-skips the ADCP #3690 step-7 key-origin binding whenexpected_key_originsis unset — a fail-open on abrand_json-sourced resolver. This PR does not touch that path; worth its own issue before enablingbrand_jsonresolvers publicly.
Minor nits (non-blocking)
- Port parse escapes the typed error.
canonical.py_canon_authority:port = int(portstr)/int(tail[1:])raise a bareValueErroron a non-numeric or empty port (https://host:/p,https://host:abc/p), so the verifier tags themrequest_signature_header_malformedrather thanrequest_target_uri_malformed— andwebhook_signature_header_malformedrather than thewebhook_target_uri_malformedthe spec assigns a malformed authority at step 10. Still fail-closed, so not a block, but it is a gap in exactly the taxonomy this PR is tightening. The empty-port case is documented; the non-numeric case is the same class. - PR body lags the code. The "Open question for the maintainer" section still argues for retagging onto
webhook_signature_header_malformed, which the final commit (3e6d457) already corrected towebhook_target_uri_malformed. Worth a body edit so a reader doesn't grade the stale plan.
Merge ordering: stacked on #980 — do not merge before it.
Safe to merge once CI is green.
3e6d457 to
7548cc8
Compare
|
Both fixed in 1. The webhook twin. You're right, and my claim was wrong for a specific reason worth naming: I grepped this repo for Added 2. The empty authority. Reproduced, and slightly worse than reported:
Also restacked onto the 3.1.8 base (see #980). |
There was a problem hiding this comment.
Correct fix, correct code names, wrong release signal. The canonicalization logic is sound and the wire codes match the spec — but two public exports change signed-byte behavior and this ships as a minor.
The block is narrow and it is the semver signal, not the code. security-reviewer: no High, fail-closed on every host path. ad-tech-protocol-expert: sound-with-caveats — webhook_target_uri_malformed is the right code (security.mdx L1469/L1566), the retag in REQUEST_TO_WEBHOOK_CODE is right, the prefix break is spec-sanctioned. code-reviewer: no Blocker. The implementation is good. The release semantics are not.
The block
canonicalize_target_uri and canonicalize_authority are public exports (adcp/signing/__init__.py:417-418), and this is a post-1.0 signing library (v6.6.0). The diff changes what those two functions do to inputs that used to succeed:
https://example.com./pnow derives authorityexample.com, notexample.com.(_canon_host, canonical.py). A 6.7 signer and a 6.6 verifier derive different@authorityfor the same FQDN-root host, and every signature between them fails. You document this yourself: "a signer on this SDK and a verifier on an older one will disagree." That is the textbook breaking change for a signing library — the bytes under the signature moved.- Authority-less URIs (
mailto:, relative paths) that round-tripped now raise.
Shipped as feat(signing):, release-please cuts 6.7.0. Adopters pinned >=6,<7 pull a signed-byte change on a minor. For a signing library, changing what gets signed is exactly what ! exists for. Fail-closed beats fail-open applies to release semantics too: when unsure whether a wire change is breaking, tag it breaking.
The remedy is one line, not a rewrite: retitle to feat(signing)!: (or add a BREAKING CHANGE: footer naming the root-dot and authority-less-URI changes) so release-please cuts a major. Your commit body already reads "BEHAVIOUR CHANGES, both wire-visible" — release-please does not key on that prose, only on ! or the literal footer. Make the machine see what you already wrote.
I considered treating this as a follow-up. It is not: wrong commit scope is a follow-up, but a breaking change to a public export without the semver signal is the block, and this repo auto-publishes from the prefix.
Things I checked
- Exception ordering is load-bearing and correct.
except TargetUriMalformedErrorsits beforeexcept (ValueError, KeyError)atverifier.py:315; sinceTargetUriMalformedErrorsubclassesValueError, a reorder silently downgrades every rejection torequest_signature_header_malformed.test_target_uri_malformed_boundary.pypins it. Real risk, real guard. - Empty-label re-check after root-dot strip closes a real hole (
_canon_host, canonical.py).https://./pused to canonicalize tohttps:///p— the empty authority this module rejects two frames up. IPv6 ([::1]) splits on.to a single non-empty element, so the re-check does not false-positive on bracketed hosts. - No import cycle.
errors.pyimportingREQUEST_TARGET_URI_MALFORMEDfromcanonicalkeeps the leaf direction —canonicalnever importserrors. Diamond throughverifier, not a cycle. - Webhook retag is spec-correct.
REQUEST_TARGET_URI_MALFORMED -> WEBHOOK_TARGET_URI_MALFORMEDinREQUEST_TO_WEBHOOK_CODE(errors.py) avoids thewebhook_signature_invalidfallthrough; the droppedwebhook_signature_prefix is per spec (structural codes:webhook_target_uri_malformed,webhook_mode_mismatch,webhook_body_malformed). - Zone-identifier rejection (RFC 6874) is the right call — a node-local
%zoneinside brackets carries no meaning across a network boundary; rejecting beats canonicalizing. - Fast path is genuinely non-regressing — ASCII hosts take
host.lower()exactly as before, preserving underscore/over-long hosts that sign today; only the intended trailing-dot strip and U-label to A-label conversion change.
Follow-ups (non-blocking — file as issues)
- Port syntax is ungated.
_malformed_authority_reasonvalidates the host but never the port, so the port flows straight intoint()in_canon_authority.https://host:/pgivesint(""), a bareValueError(no.code), which lands asrequest_signature_header_malformedinstead ofrequest_target_uri_malformed— the exact "passes on someone else's exception" failure_split_or_rejectwas written to prevent, just one frame lower. Worse,int()accepts-80,8_0, and non-ASCII digits, so a non-ASCII-digit port and its ASCII equivalent collapse to one canonical authority — self-consistent within the SDK, but a raw-vs-canonical differential a strict peer or downstream router trips over. Gateportstron digits-only beforeint(), at both parse sites. You flag theint("")case in the notes; the non-ASCII-digit case is the one worth an issue. - Root-dot needs an upstream ruling before it is a wire commitment. The spec is silent and no vector covers it (#986). Containing it with an SDK-local test is the right call; just don't let it ossify as settled behavior — it needs a real vector.
- The interim U-label window is a tracked non-conformance. Between this and PR B a verifier converts a wire U-label where the spec says reject at step 1. Punycode keeps A-labels distinct so it is not cross-domain confusion, but land PR B promptly rather than letting it sit.
Minor nits (non-blocking)
- The "Open question for the maintainer" is stale and points the wrong way. It still argues
request_target_uri_malformedshould retag ontowebhook_signature_header_malformedbecause "no spec text defines a webhook twin" — but commit7548cc8already found the twin and mapped it correctly. The code is right; the PR body argues against the code. Fix the description so nobody merges on the strength of a paragraph the diff already overturned.
Flip me to approve with the ! / BREAKING CHANGE: footer. Everything else is clean and the experts cleared correctness and security. Stacked on #980, so it can't land ahead of the semver fix anyway.
7548cc8 to
fbab8f4
Compare
|
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. |
|
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. |
|
Thanks — the port find is real and the stale-body catch was correct. Both addressed in Port validation — fixed, with a change to the prescriptionGating Second, and this one matters more: Resulting rules: Six tests red before the change. You're right that this was the same "passes on someone else's exception" failure Stale PR description — fixedCorrect, and worth stating plainly: the body argued for mapping onto Semver — deferring to @bokelley, who has already ruledHe retitled these I think you and he are arguing different axes rather than disagreeing: his point is that this isn't a feature, yours is that it's breaking. Both can hold — a fix can be breaking — and For what it's worth as the author, I'd argue against the major: the only signed-byte change is the trailing FQDN-root dot, the spec is silent on it and ships no vector (#986 asks upstream to rule), and a major bump over an unspecified edge case seems disproportionate at this stage. But it's his release and his call — I'm not going to settle it by retitling, which is why the Follow-ups you raised
|
bokelley
left a comment
There was a problem hiding this comment.
Reviewed the latest head, including the spec-defined webhook_target_uri_malformed mapping, the post-normalization empty-host rejection, and RFC 3986 port validation. I am treating these as a patch-level spec-conformance correction rather than a major-version trigger. The stacked signing suite passes locally: 533 passed, 4 expected xfails.
Superseded by the current head and maintainer semver decision: this restores specified signing behavior and will ship as a patch-level conformance fix.
…bels Six authority shapes the profile requires be rejected were canonicalized instead, and `request_target_uri_malformed` was not implemented at all. The signing path also never called the package's own host canonicalizer, so an IDN authority signed under a raw U-label. Rejections now live inside `_canon_authority`, so `@target-uri` and `@authority` inherit the rule once rather than twice. `urlsplit`'s own bare `ValueError` is normalized onto the same typed error -- without that, the missing-closing-bracket vector passes on someone else's exception and grades nothing. The predicate is ported from a downstream implementation minus its raw-non-ASCII-host branch: this module is shared by signer and verifier, and the signer must CONVERT a U-label, not refuse it. Rejecting a U-label arriving on the wire is the verifier's step-1 precheck, which raises a different code at an earlier step and shares `host_has_raw_non_ascii` rather than re-deriving it. Host canonicalization reuses `_idna_canonicalize.canonicalize_host` behind an ASCII fast path. The fast path is load-bearing, not an optimization: the helper strips IPv6 brackets and raises on underscore hosts and over-long labels, all of which sign correctly today. Retires eight strict-xfail ledger entries. `trailing-empty-query-preserved` (adcontextprotocol#979) stays. BEHAVIOUR CHANGES, both wire-visible: - A trailing FQDN-root dot is now stripped: `https://example.com./p` derives authority `example.com`, not `example.com.`. The spec does not define root-dot handling and ships no vector for it; stripping once before the ASCII/IDNA branch is what stops the two branches disagreeing. Pinned by an SDK-local test until upstream rules. - Authority-less URIs (`mailto:`, relative paths) now raise where they round-tripped unchanged. `request_target_uri_malformed` retags onto the existing `webhook_signature_header_malformed` rather than minting a webhook twin: no spec text, schema, or vector anywhere defines one. Fixes adcontextprotocol#978. Partially fixes adcontextprotocol#977 (signer half).
…that empty Two review corrections. 1. The spec DOES define the webhook twin. security.mdx's webhook checklist lists `webhook_target_uri_malformed` in the error taxonomy and requires it at step 10 for a malformed or mismatched authority. The earlier claim that no webhook twin existed came from grepping this repo rather than the spec, and retagging onto `webhook_signature_header_malformed` put a stable but WRONG code on the wire. Adds WEBHOOK_TARGET_URI_MALFORMED and maps REQUEST_TARGET_URI_MALFORMED to it. The name breaks the `webhook_signature_` prefix the rest of the family carries because the spec names it that way. 2. `_canon_host` stripped the trailing root dot AFTER `_malformed_authority_reason` had already accepted the netloc, and the raw netloc `.` is non-empty and passes as a host. So `https://./p` canonicalized to `https:///p` -- the empty authority this module rejects as `malformed-empty-authority`, reached by a path that ran the gate too early. `https://../p` yielded the authority `.`. The check now re-runs after the strip and is stated as "no empty label", so `https://a..b/p` is covered too. Refs adcontextprotocol#978, adcontextprotocol#977.
Review follow-up. `_malformed_authority_reason` judged the host but never the
port, so `portstr` went straight into `int()` -- far more permissive than the
grammar `port = *DIGIT`, in three distinct ways:
- `int("-80")` produced the authority `host:-80`, which is not an authority.
- `int("8_0")` is 80: Python accepts underscore digit separators.
- `int("٨٠")` is also 80: `int()` accepts non-ASCII digits, so `host:٨٠` and
`host:80` collapsed to the SAME canonical authority. That is a
raw-vs-canonical differential -- a peer that does not fold Arabic-Indic
digits derives a different @authority from identical bytes and the signature
fails for a reason neither side can see in its own logs.
`str.isdigit()` does NOT close the third case -- `"٨٠".isdigit()` is True --
so the gate tests ASCII digits specifically.
An empty port is NOT rejected. RFC 3986 §3.2.3 makes it legal (`*DIGIT`),
meaning "default", and directs normalizers to drop the port and its colon, so
`https://host:/p` normalizes to `host`. `urlsplit` agrees (`port=None`).
Rejecting it would refuse a valid URI; it previously raised a bare `ValueError`
from `int("")` carrying no code -- the same "passes on someone else's
exception" failure `_split_or_reject` exists to prevent, one frame lower.
Refs adcontextprotocol#978.
4331ac8 to
cdb0ed4
Compare
There was a problem hiding this comment.
Approving. Request-side codes match the SHA-pinned vector set, the security pass is a net hardening, and the reasoning is load-bearing throughout. Retitle and verify the two follow-ups before merge.
The architectural call that makes this right: the rejection set lives once inside _canon_authority/_malformed_authority_reason, so @target-uri and @authority inherit it rather than each carrying a copy, and the signer/verifier split shares the predicate (host_has_raw_non_ascii) without sharing the code — one rule, two steps.
Things I checked
- Request-side codes are spec-exact.
ad-tech-protocol-expertverified all six reject shapes andrequest_target_uri_malformed1:1 against the SHA-256-pinnedcanonicalization.json— port-without-host, userinfo-without-host, empty authority, missing closing bracket, bare IPv6, RFC 6874 zone-id. Nothing over- or under-rejects. The signer-converts / verifier-rejects split matches026-non-ascii-host.json(step 1, deferred to #976/PR B). - Verifier clause ordering is correct and load-bearing.
except TargetUriMalformedErrorprecedesexcept (ValueError, KeyError)atverifier.py:313. SinceTargetUriMalformedErrorIS aValueError, first position is mandatory — reordering silently downgrades every rejection torequest_signature_header_malformed. Guarded bytest_target_uri_malformed_boundary.py. - Fail-closed, no uncoded escape.
_split_or_rejectnormalizesurlsplit's bareValueErroronto the typed error (canonical.py:_split_or_reject);_canon_hostcatches(idna.IDNAError, UnicodeError)and re-raises coded.security-reviewer: CLEAR — no path where two URLs collapse to the same@authority, every residual divergence is fail-closed (signature won't verify), not a forgery. - Port grammar closes real differentials.
_port_or_reject's ASCII-digit frozenset closes bothint(\"٨٠\")==80(Arabic-Indic fold) andint(\"8_0\")==80(underscore separator) —str.isdigit()would not have. Empty port →Noneper RFC 3986 §3.2.3. - Root-dot strip is symmetric. Stripped once before the ASCII/IDNA branch in
_canon_host, so both branches converge withcanonicalize_host. Pinned bytest_trailing_root_dot_is_stripped_on_both_host_branches. - Webhook retag is wired.
REQUEST_TO_WEBHOOK_CODEmaps the new code (errors.py), so_retag_to_webhookdoesn't fall through towebhook_signature_invalid+ the update-the-map warning. Guarded by test. - No layering/cycle breach.
canonicaldeclares its ownREQUEST_TARGET_URI_MALFORMEDconstant;errorsimports fromcanonical, never the reverse.
Follow-ups (non-blocking — file as issues)
- Semver signal — lean
feat.fix(signing):cuts a patch, but the signer's output bytes change for IDN and FQDN-root hosts, and two public exports (canonicalize_target_uri/canonicalize_authority) go from round-trip to raise for authority-less input.ad-tech-protocol-expert: "feat(signing):at minimum, arguablyfeat!." Recommend retitling tofeat(signing):so release-please cuts a minor — you flagged the trade-off yourself in the body. A rejection-set expansion and a wire-visible signer-output change arriving underfix:is a notable choice for a patch. Not blocking it because the IDN change is a conformance fix (the old raw-U-label signing was already non-interoperable with any conformant peer), and the maintainer owns release cadence. - Verify the webhook code name before merge.
webhook_target_uri_malformedappears nowhere in this checkout — nosecurity.mdx, no vector — so unlike the request-side code it is pinned only by PR prose, and the mapping was reversed once in7548cc84. Confirmsecurity.mdxL1469/L1566/step-10 actually spells it before this ships a webhook wire code. Correct-by-analogy is not confirmed. webhook_transport_hooks.py:96-100. Per your own note,rewrite_to=\"::1\"now hard-rejects where it mis-signed before. Worth a changelog line for anyone carrying that config.#986tracks the root-dot case as a real upstream vector, superseding the SDK-local test. Right containment.
Minor nits (non-blocking)
host_has_raw_non_asciidocstring overstates the present. It says "deliberately two call sites with two different codes," but the verifier-precheck caller doesn't exist yet —026is still xfailed intest_verifier_vectors.py, landing with #976/PR B. Soften to "will be shared by the verifier precheck."- Bracketed trailing garbage still passes.
https://[::1]foo/p—urlsplityieldsnetloc=\"[::1]foo\", and_canon_authority's bracketed branch ignorestailunless it starts with:, so it canonicalizes to[::1]rather than rejecting. Pre-existing and not among the six vectors, but this PR is precisely about rejecting malformed authorities — worth a tracking note. - ASCII fast path is intentionally lenient vs
canonicalize_host(underscore hosts, leading-zero IPv4 sign as literal lowercased). Fail-closed and pre-existing; a one-line code comment noting the intentional asymmetry would save the next reader the trip through_idna_canonicalize.
Approving on the strength of the request-side vector match plus a clean security pass. Retitle to feat and confirm the webhook code name before merge.
Fixes #978. Partially fixes #977 (signer half — see Why #977 is only half-closed below).
What was wrong
Six authority shapes the profile requires be rejected were canonicalized instead, and
request_target_uri_malformedwas not implemented at all. Separately, the signing path never called the package's own host canonicalizer, so an IDN authority signed and verified under a raw U-label.What changed
The rejection set lives inside
_canon_authority, so@target-uriand@authorityinherit it once rather than each carrying a copy.urlsplit's own bareValueErroris normalized onto the same typed error — without that,malformed-ipv6-missing-closing-bracketpasses on someone else's exception and grades nothing. (#980 had exactly this false green; it was fixed there so all six reject cases xfail together, and they retire together here.)The predicate is ported minus one branch. The reference implementation rejects raw non-ASCII hosts. That is correct there, because it wraps a verifier — but
canonical.pyis shared by signer and verifier, and the signer is required to convert a U-label to its A-label form (idn-to-punycode), not refuse it. Porting that branch verbatim would have made the two halves of #977 fight each other. The test survives ashost_has_raw_non_ascii, which PR B's step-1 precheck imports rather than re-deriving: one rule, two codes, two steps.Host canonicalization reuses
_idna_canonicalize.canonicalize_hostinstead of hand-rolling IDNA here. That helper already has six consumers andcanonical.pywas conspicuously absent from them — adding a private IDNA step would have made this the seventh host normalizer in the tree.The ASCII fast path is load-bearing, not an optimization: the helper strips IPv6 brackets and raises on underscore hosts and >63-byte labels, all of which sign correctly today.
Failure mode is closed, deliberately. The existing callers are split —
jwks.pyandrevocation_fetcher.pyfail closed,ip_pinned_transport.pyandkey_origins.pyfail open — so precedent was not available to appeal to. The reason to fail closed here is that a signature base must never be computed over a host we could not canonicalize; the fail-open callers are comparison paths, where raising would turn a mismatch into an outage.Behaviour changes — both wire-visible
1. A trailing FQDN-root dot is now stripped.
https://example.com./pderives authorityexample.com, where it derivedexample.com.before. A signer on this SDK and a verifier on an older one will disagree for FQDN-root URLs.This one deserves scrutiny, because the spec does not decide it.
canonicalize_hoststrips the root dot as part of UTS-46 preparation; a bare.lower()keeps it. Branching first would therefore makeexample.com.andbücher.example.normalize differently — a signer emitting the A-label form and a verifier reading a Host-header U-label would derive two different authorities for the same host. Stripping once, before the branch, is what makes the two agree, and it makes them agree with the package's designated normalizer rather than with an accident of the old code.No vector covers this. I could not add one:
canonicalization.jsonis SHA-256 pinned invector_manifest.json, andtest_vendored_tree_matches_manifest_exactlyfails on extra files as well as changed ones — correctly, since #980 exists to stop exactly that drift. So the behaviour is pinned by an SDK-local test and tracked in #986, which asks that the case be defined upstream and shipped as a real vector. When that lands, the local test is superseded.2. Authority-less URIs now raise.
canonicalize_target_uri("mailto:x@example.com")and relative paths round-tripped unchanged before; both are public exports.The webhook code (resolved — this section previously said the opposite)
request_target_uri_malformedretags ontowebhook_target_uri_malformed, which the spec defines: security.mdx's webhook checklist lists it in the error taxonomy (L1469, L1566) and requires it at step 10 for a malformed or mismatched authority. The name deliberately drops thewebhook_signature_prefix the rest of the family carries, because that is how the spec names structural codes.An earlier revision of this PR mapped onto
webhook_signature_header_malformedand this section argued for it on the grounds that "no spec text defines a webhook twin". That was wrong — it reflected a grep of this repo rather than the spec. Corrected in7548cc84; the paragraph is kept here only so the reasoning trail is visible rather than silently rewritten.Why #977 is only half-closed
#977 has two halves. The signer half — no A-label conversion — is fixed here and graded by the two positive
idn-*vectors. The verifier half — a raw U-label arriving on the wire must be rejected, not converted — is graded bynegative/026, which expectsrequest_signature_header_malformedat step 1. That is #976's precheck, so it lands in PR B and closes #977 there.026's ledger entry is retagged accordingly rather than left naming an issue whose other clause is now false.Worth stating plainly: between this PR and PR B, a verifier will silently convert a wire U-label where the spec says it must refuse. That is an argument for landing PR B promptly rather than letting it sit.
Notes
webhook_transport_hooks.py:96-100synthesizes a netloc from the unvalidatedrewrite_toconfig string. An IPv6 value (rewrite_to="::1") yields an unbracketed netloc that_canon_authoritymis-splits at the last colon today; after this PR it becomes a hard rejection. Failing loudly beats mis-signing, but anyone with that config will notice.https://host:/pstill escapes as an uncodedValueErrorfromint(""), so "every authority rejection carries.code" is not literally true yet. Left alone deliberately — a gate that cannot prove malformation should pass traffic through.feat(signing):so release-please cuts a minor. Both behaviour changes above are adopter-visible, and a rejection-set expansion arriving in a patch is how integrators get surprised. Retitle tofix:if you'd rather it went out as a patch.Test plan
make ci-local: 5953 passed, 0 failed, 41 skipped, 6 xfailed (baseline on #980's branch: 5938 passed / 14 xfailed). The delta is exactly +15 passed / −8 xfailed: eight retired ledger entries plus seven new tests.Two gaps the vectors provably do not cover, now covered — a probe across the full suite found no test driving either path:
test_target_uri_malformed_boundary.pydrivesverify_request_signatureandverify_webhook_signaturewith a gate-tripping URL. The verifier one guards clause ordering:TargetUriMalformedErroris aValueError, so moving the newexceptbelow the existingexcept (ValueError, KeyError)silently downgrades every rejection torequest_signature_header_malformed. Without this test that reorder is invisible.test_canonicalization.pypins root-dot symmetry across both host branches.