Skip to content

fix(signing): reject malformed authorities, convert IDN hosts to A-labels - #985

Merged
bokelley merged 3 commits into
adcontextprotocol:mainfrom
KonstantinMirin:fix/target-uri-malformed-authority
Jul 29, 2026
Merged

fix(signing): reject malformed authorities, convert IDN hosts to A-labels#985
bokelley merged 3 commits into
adcontextprotocol:mainfrom
KonstantinMirin:fix/target-uri-malformed-authority

Conversation

@KonstantinMirin

@KonstantinMirin KonstantinMirin commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #980 — do not merge before it.
Own commit: 9be012b8. Everything else in the diff below is #980's vendored vectors.
Incremental diff: KonstantinMirin/adcp-client-python@fix/vendor-complete-signing-vectors...fix/target-uri-malformed-authority

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_malformed was 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-uri and @authority inherit it once rather than each carrying a copy. urlsplit's own bare ValueError is normalized onto the same typed error — without that, malformed-ipv6-missing-closing-bracket passes 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.py is 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 as host_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_host instead of hand-rolling IDNA here. That helper already has six consumers and canonical.py was 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.py and revocation_fetcher.py fail closed, ip_pinned_transport.py and key_origins.py fail 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./p derives authority example.com, where it derived example.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_host strips the root dot as part of UTS-46 preparation; a bare .lower() keeps it. Branching first would therefore make example.com. and bü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.json is SHA-256 pinned in vector_manifest.json, and test_vendored_tree_matches_manifest_exactly fails 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_malformed retags onto webhook_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 the webhook_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_malformed and 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 in 7548cc84; 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 by negative/026, which expects request_signature_header_malformed at 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-100 synthesizes a netloc from the unvalidated rewrite_to config string. An IPv6 value (rewrite_to="::1") yields an unbracketed netloc that _canon_authority mis-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:/p still escapes as an uncoded ValueError from int(""), so "every authority rejection carries .code" is not literally true yet. Left alone deliberately — a gate that cannot prove malformation should pass traffic through.
  • Titled 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 to fix: 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.py drives verify_request_signature and verify_webhook_signature with a gate-tripping URL. The verifier one guards clause ordering: TargetUriMalformedError is a ValueError, so moving the new except below the existing except (ValueError, KeyError) silently downgrades every rejection to request_signature_header_malformed. Without this test that reorder is invisible.
  • test_canonicalization.py pins root-dot symmetry across both host branches.

@aao-ipr-bot

aao-ipr-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

⚠️ 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

aao-ipr-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

⚠️ 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 bokelley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bokelley bokelley changed the title feat(signing): reject malformed authorities, convert IDN hosts to A-labels fix(signing): reject malformed authorities, convert IDN hosts to A-labels Jul 29, 2026
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes Jul 29, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py is load-bearing and correct. except TargetUriMalformedError precedes except (ValueError, KeyError); since TargetUriMalformedError subclasses ValueError, reversing them would silently downgrade every malformed-authority rejection to request_signature_header_malformed with the suite still green. Guarded by test_target_uri_malformed_boundary.py.
  • The webhook_target_uri_malformed correction is spec-accurate, not cosmetic. ad-tech-protocol-expert: confirmed against 3.1.1 security.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 onto webhook_signature_header_malformed put a stable-but-wrong code on the wire; errors.py mapping REQUEST_TARGET_URI_MALFORMED → WEBHOOK_TARGET_URI_MALFORMED is what the spec requires.
  • The six rejections are one-for-one with the SHA-pinned canonicalization.json reject 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_host root-dot re-check is necessary, not redundant. _malformed_authority_reason runs on the raw netloc where . and .. read as non-empty hosts; only the strip empties them. if not host or any(label == "" ...) catches https://./p, https://../p, https://a..b/p after the strip, and [::1] splits to ["[::1]"] so bracketed IPv6 is not false-rejected.
  • No import cycle. errors → canonical → _idna_canonicalize; canonical deliberately does not import errors. idna>=3.7,<4 is a declared runtime dep, so the new top-level import idna in canonical.py is safe.
  • Fail-closed on IDNA failure is correct for this path. _canon_host re-raises TargetUriMalformedError on idna.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 for feat:/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. 026 is held strict-xfail; between this PR and PR B a verifier silently converts a wire U-label where the spec (security.mdx reference step 2) says it MUST reject. ad-tech-protocol-expert rates it moderate, security-reviewer Low (the @authority component 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-reviewer flagged _maybe_check_key_origin in verifier.py warns-and-skips the ADCP #3690 step-7 key-origin binding when expected_key_origins is unset — a fail-open on a brand_json-sourced resolver. This PR does not touch that path; worth its own issue before enabling brand_json resolvers publicly.

Minor nits (non-blocking)

  1. Port parse escapes the typed error. canonical.py _canon_authority: port = int(portstr) / int(tail[1:]) raise a bare ValueError on a non-numeric or empty port (https://host:/p, https://host:abc/p), so the verifier tags them request_signature_header_malformed rather than request_target_uri_malformed — and webhook_signature_header_malformed rather than the webhook_target_uri_malformed the 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.
  2. 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 to webhook_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.

@KonstantinMirin

Copy link
Copy Markdown
Collaborator Author

Both fixed in 7548cc84.

1. The webhook twin. You're right, and my claim was wrong for a specific reason worth naming: I grepped this repo for webhook_*_malformed, found nothing, and reported "the spec defines no webhook twin" as a finding. That was only the limit of where I looked, stated with far more confidence than the evidence supported.

Added WEBHOOK_TARGET_URI_MALFORMED and mapped REQUEST_TARGET_URI_MALFORMED onto it, with a comment noting the name deliberately breaks the webhook_signature_ prefix the rest of the family carries because the spec names it that way. I also rewrote the retag test's docstring, which had my wrong reasoning baked in as justification and would have read to the next person as a considered spec finding.

2. The empty authority. Reproduced, and slightly worse than reported: https://./p was producing https:///p — exactly the malformed-empty-authority shape this PR rejects — reached by a path that ran the gate before the root-dot strip could empty the host. https://../p yielded the authority ..

_malformed_authority_reason judges the raw netloc, where . is non-empty and passes as a host, so the check has to run again after the strip. Stated as "no empty label" rather than "not empty", so https://a..b/p is covered too. Regression test over all four shapes; example.com., bücher.example. and [::1] still canonicalize normally.

Also restacked onto the 3.1.8 base (see #980). make ci-local: 6013 passed, 0 failed.

aao-ipr-bot[bot]
aao-ipr-bot Bot previously requested changes Jul 29, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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./p now derives authority example.com, not example.com. (_canon_host, canonical.py). A 6.7 signer and a 6.6 verifier derive different @authority for 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 TargetUriMalformedError sits before except (ValueError, KeyError) at verifier.py:315; since TargetUriMalformedError subclasses ValueError, a reorder silently downgrades every rejection to request_signature_header_malformed. test_target_uri_malformed_boundary.py pins it. Real risk, real guard.
  • Empty-label re-check after root-dot strip closes a real hole (_canon_host, canonical.py). https://./p used to canonicalize to https:///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.py importing REQUEST_TARGET_URI_MALFORMED from canonical keeps the leaf direction — canonical never imports errors. Diamond through verifier, not a cycle.
  • Webhook retag is spec-correct. REQUEST_TARGET_URI_MALFORMED -> WEBHOOK_TARGET_URI_MALFORMED in REQUEST_TO_WEBHOOK_CODE (errors.py) avoids the webhook_signature_invalid fallthrough; the dropped webhook_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 %zone inside 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_reason validates the host but never the port, so the port flows straight into int() in _canon_authority. https://host:/p gives int(""), a bare ValueError (no .code), which lands as request_signature_header_malformed instead of request_target_uri_malformed — the exact "passes on someone else's exception" failure _split_or_reject was 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. Gate portstr on digits-only before int(), at both parse sites. You flag the int("") 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)

  1. The "Open question for the maintainer" is stale and points the wrong way. It still argues request_target_uri_malformed should retag onto webhook_signature_header_malformed because "no spec text defines a webhook twin" — but commit 7548cc8 already 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.

@KonstantinMirin
KonstantinMirin force-pushed the fix/target-uri-malformed-authority branch from 7548cc8 to fbab8f4 Compare July 29, 2026 14:18
@aao-ipr-bot

aao-ipr-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

⚠️ 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

aao-ipr-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

⚠️ 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.

@KonstantinMirin

Copy link
Copy Markdown
Collaborator Author

Thanks — the port find is real and the stale-body catch was correct. Both addressed in 4331ac84. One correction to the suggested remedy, and the semver point I'm deferring rather than deciding.

Port validation — fixed, with a change to the prescription

Gating portstr on digits-only would reject https://host:/p, but RFC 3986 §3.2.3 defines port = *DIGIT, so an empty port is legal, means "default", and normalizers are directed to drop it along with its colon. urlsplit agrees (port=None). That is also the better answer to the bare-ValueError complaint: the correct outcome there was never an error at all, so it now normalizes to host rather than raising anything.

Second, and this one matters more: str.isdigit() does not close the case it was aimed at. "٨٠".isdigit() is True in Python, so a digits-only gate would leave host:٨٠ and host:80 folding to the same canonical authority — exactly the differential you flagged, surviving the fix meant to remove it. The gate tests ASCII digits explicitly, and both the code and the test docstring say why, so nobody "simplifies" it back to isdigit() later.

Resulting rules:

https://host:/p       -> host        (empty port omitted, per §3.2.3)
https://[::1]:/p      -> [::1]
https://host:8443/p   -> host:8443   (unchanged)
https://host:-80/p    -> request_target_uri_malformed
https://host:8_0/p    -> request_target_uri_malformed   (int() accepts _ separators)
https://host:٨٠/p     -> request_target_uri_malformed   (int() accepts non-ASCII digits)
https://host:8a/p     -> request_target_uri_malformed

Six tests red before the change. You're right that this was the same "passes on someone else's exception" failure _split_or_reject exists to prevent, one frame lower — it belonged in this PR rather than a follow-up, since the PR's own claim is that every authority rejection carries the spec code.

Stale PR description — fixed

Correct, and worth stating plainly: the body argued for mapping onto webhook_signature_header_malformed on the grounds that no spec text defined a webhook twin. That claim came from grepping this repo rather than the spec, and 7548cc84 had already overturned it. The section is rewritten, with the wrong reasoning left visible rather than silently deleted so the trail is readable.

Semver — deferring to @bokelley, who has already ruled

He retitled these featfix, on the grounds that they restore spec compliance and that adopter-visible behaviour alone doesn't make something a feature. I've aligned the commit subjects to match, since the PR title and the commit subject disagreeing would have made the release type depend on which merge button got pressed.

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 fix(signing)!: is the construction that satisfies both readings if he wants the major.

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 ! isn't there.

Follow-ups you raised

bokelley
bokelley previously approved these changes Jul 29, 2026

@bokelley bokelley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bokelley
bokelley dismissed aao-ipr-bot[bot]’s stale review July 29, 2026 22:15

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.
@bokelley
bokelley force-pushed the fix/target-uri-malformed-authority branch from 4331ac8 to cdb0ed4 Compare July 29, 2026 22:30
@bokelley
bokelley merged commit be233e4 into adcontextprotocol:main Jul 29, 2026
18 checks passed

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-expert verified all six reject shapes and request_target_uri_malformed 1:1 against the SHA-256-pinned canonicalization.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 matches 026-non-ascii-host.json (step 1, deferred to #976/PR B).
  • Verifier clause ordering is correct and load-bearing. except TargetUriMalformedError precedes except (ValueError, KeyError) at verifier.py:313. Since TargetUriMalformedError IS a ValueError, first position is mandatory — reordering silently downgrades every rejection to request_signature_header_malformed. Guarded by test_target_uri_malformed_boundary.py.
  • Fail-closed, no uncoded escape. _split_or_reject normalizes urlsplit's bare ValueError onto the typed error (canonical.py:_split_or_reject); _canon_host catches (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 both int(\"٨٠\")==80 (Arabic-Indic fold) and int(\"8_0\")==80 (underscore separator) — str.isdigit() would not have. Empty port → None per RFC 3986 §3.2.3.
  • Root-dot strip is symmetric. Stripped once before the ASCII/IDNA branch in _canon_host, so both branches converge with canonicalize_host. Pinned by test_trailing_root_dot_is_stripped_on_both_host_branches.
  • Webhook retag is wired. REQUEST_TO_WEBHOOK_CODE maps the new code (errors.py), so _retag_to_webhook doesn't fall through to webhook_signature_invalid + the update-the-map warning. Guarded by test.
  • No layering/cycle breach. canonical declares its own REQUEST_TARGET_URI_MALFORMED constant; errors imports from canonical, 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, arguably feat!." Recommend retitling to feat(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 under fix: 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_malformed appears nowhere in this checkout — no security.mdx, no vector — so unlike the request-side code it is pinned only by PR prose, and the mapping was reversed once in 7548cc84. Confirm security.mdx L1469/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.
  • #986 tracks the root-dot case as a real upstream vector, superseding the SDK-local test. Right containment.

Minor nits (non-blocking)

  1. host_has_raw_non_ascii docstring overstates the present. It says "deliberately two call sites with two different codes," but the verifier-precheck caller doesn't exist yet — 026 is still xfailed in test_verifier_vectors.py, landing with #976/PR B. Soften to "will be shared by the verifier precheck."
  2. Bracketed trailing garbage still passes. https://[::1]foo/purlsplit yields netloc=\"[::1]foo\", and _canon_authority's bracketed branch ignores tail unless 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.
  3. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants