Skip to content

Commit 3e6d457

Browse files
fix(signing): add the webhook target-uri code and reject authorities 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 #978, #977.
1 parent 9be012b commit 3e6d457

4 files changed

Lines changed: 50 additions & 14 deletions

File tree

src/adcp/signing/canonical.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,14 @@ def _canon_host(host: str, netloc: str) -> str:
267267
"""
268268
if host.endswith("."):
269269
host = host[:-1]
270+
if not host or any(label == "" for label in host.split(".")):
271+
# Re-checked AFTER the strip. `_malformed_authority_reason` runs on the
272+
# raw netloc, where `.` and `..` are non-empty and look like hosts; it
273+
# is only stripping the root dot that empties them. Without this,
274+
# `https://./p` canonicalized to `https:///p` -- the empty authority
275+
# this very module rejects two functions up, reached by a path that
276+
# skipped the check.
277+
raise TargetUriMalformedError(netloc, "the authority carries no host once normalized")
270278
if not host_has_raw_non_ascii(host):
271279
return host.lower()
272280
try:

src/adcp/signing/errors.py

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -116,21 +116,21 @@ def __init__(
116116
WEBHOOK_SIGNATURE_KEY_ORIGIN_MISMATCH = "webhook_signature_key_origin_mismatch"
117117
WEBHOOK_SIGNATURE_KEY_ORIGIN_MISSING = "webhook_signature_key_origin_missing"
118118

119+
# Structural code for a malformed authority on the webhook profile. Named
120+
# without the ``webhook_signature_`` prefix the rest of this family carries
121+
# because the spec names it that way: security.mdx's webhook checklist lists
122+
# ``webhook_target_uri_malformed`` in the error taxonomy and requires it at
123+
# step 10 for a malformed or mismatched authority.
124+
WEBHOOK_TARGET_URI_MALFORMED = "webhook_target_uri_malformed"
125+
119126
# Code-family translation used by the webhook verifier wrapper. The verifier
120127
# pipeline raises request_signature_* codes; the wrapper retags them into
121128
# webhook_signature_* before exposing to callers. Keeps the 300-line verifier
122129
# unchanged and guarantees webhook routes never leak request-family codes.
123130
REQUEST_TO_WEBHOOK_CODE = {
124131
REQUEST_SIGNATURE_REQUIRED: WEBHOOK_SIGNATURE_REQUIRED,
125132
REQUEST_SIGNATURE_HEADER_MALFORMED: WEBHOOK_SIGNATURE_HEADER_MALFORMED,
126-
# Deliberately NOT a new `webhook_target_uri_malformed`. The AdCP spec
127-
# defines `request_target_uri_malformed` (it is graded by the shipped
128-
# canonicalization vectors) but names no webhook twin, and no bundled
129-
# schema enumerates webhook signing codes. Minting one here would put a
130-
# string on the wire that nothing upstream defines. A malformed target URI
131-
# is the same failure class as a malformed header, so it retags onto the
132-
# existing code until the spec says otherwise.
133-
REQUEST_TARGET_URI_MALFORMED: WEBHOOK_SIGNATURE_HEADER_MALFORMED,
133+
REQUEST_TARGET_URI_MALFORMED: WEBHOOK_TARGET_URI_MALFORMED,
134134
REQUEST_SIGNATURE_PARAMS_INCOMPLETE: WEBHOOK_SIGNATURE_PARAMS_INCOMPLETE,
135135
REQUEST_SIGNATURE_TAG_INVALID: WEBHOOK_SIGNATURE_TAG_INVALID,
136136
REQUEST_SIGNATURE_ALG_NOT_ALLOWED: WEBHOOK_SIGNATURE_ALG_NOT_ALLOWED,

tests/conformance/signing/test_canonicalization.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,3 +203,28 @@ def test_multi_label_signature_input_selects_sig1() -> None:
203203
assert labels["sig2"].components == ("@method", "@target-uri")
204204
assert labels["sig1"].params["nonce"] == "KXYnfEfJ0PBRZXQyVXfVQA"
205205
assert labels["sig2"].params["nonce"] == "DIFFERENT-NONCE-FOR-SIG2____"
206+
207+
208+
@pytest.mark.parametrize(
209+
"url",
210+
["https://./p", "https://../p", "https://.:443/p", "https://a..b/p"],
211+
ids=["root-dot-only", "double-dot", "root-dot-with-port", "interior-empty-label"],
212+
)
213+
def test_authority_that_empties_after_normalization_is_rejected(url: str) -> None:
214+
"""A host must still be a host once the root dot comes off.
215+
216+
``_malformed_authority_reason`` judges the raw netloc, where ``.`` and
217+
``..`` are non-empty and pass as hosts. Stripping the root dot is what
218+
empties them, so the check has to run again afterwards. Without it
219+
``https://./p`` canonicalized to ``https:///p`` -- the empty authority
220+
``malformed-empty-authority`` rejects, reached by a path that skipped the
221+
gate.
222+
223+
``a..b`` is here because the rule is "no empty label", not merely
224+
"not empty".
225+
"""
226+
with pytest.raises(ValueError) as excinfo:
227+
canonicalize_target_uri(url)
228+
assert getattr(excinfo.value, "code", None) == "request_target_uri_malformed"
229+
with pytest.raises(ValueError):
230+
canonicalize_authority(url)

tests/conformance/signing/test_target_uri_malformed_boundary.py

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
verify_request_signature,
3939
)
4040
from adcp.signing.errors import (
41-
WEBHOOK_SIGNATURE_HEADER_MALFORMED,
41+
WEBHOOK_TARGET_URI_MALFORMED,
4242
SignatureVerificationError,
4343
)
4444
from adcp.webhooks import (
@@ -164,9 +164,12 @@ def test_webhook_verifier_retags_target_uri_malformed_without_map_warning(
164164
no entry is therefore silently *tolerated* -- the caller just gets a vaguer
165165
code and the receiver's logs get noisier.
166166
167-
The expected twin is the EXISTING ``webhook_signature_header_malformed``: same
168-
failure class, and no new wire-visible string is minted for a code the AdCP
169-
webhook taxonomy does not define anywhere in this repo.
167+
The expected twin is ``webhook_target_uri_malformed``. It is named without
168+
the ``webhook_signature_`` prefix the rest of the family carries because the
169+
spec names it that way: security.mdx's webhook checklist lists it in the
170+
error taxonomy and requires it at step 10 for a malformed or mismatched
171+
authority. Retagging onto ``webhook_signature_header_malformed`` instead
172+
would put a stable but WRONG code on the wire.
170173
"""
171174
headers = _signed_webhook_headers()
172175

@@ -180,9 +183,9 @@ def test_webhook_verifier_retags_target_uri_malformed_without_map_warning(
180183
options=_webhook_verify_options(),
181184
)
182185

183-
assert exc_info.value.code == WEBHOOK_SIGNATURE_HEADER_MALFORMED, (
186+
assert exc_info.value.code == WEBHOOK_TARGET_URI_MALFORMED, (
184187
f"verify_webhook_signature({MALFORMED_URL!r}) must retag "
185-
f"{REQUEST_TARGET_URI_MALFORMED!r} to {WEBHOOK_SIGNATURE_HEADER_MALFORMED!r}; "
188+
f"{REQUEST_TARGET_URI_MALFORMED!r} to {WEBHOOK_TARGET_URI_MALFORMED!r}; "
186189
f"got {exc_info.value.code!r}"
187190
)
188191
unmapped = [

0 commit comments

Comments
 (0)