fix(signing): reject malformed structured-field input at step 1

[KonstantinMirin]

Stacked on #985, which is stacked on #980 — merge order: #980 → #985 → this.
Own commit: 051c717a. Everything else in the diff belongs to #980 and #985.
Incremental diff: KonstantinMirin/adcp-client-python@fix/target-uri-malformed-authority...fix/strict-header-precheck

Fixes #976. Closes #977 (verifier half — #985 closed the signer half).

Please read the "Known limit" section before approving. This PR cannot fully close the threat on WSGI, and I would rather that be an explicit review decision than a footnote.

What was wrong

The verifier had no strict step-1 stage, so malformed or ambiguous input flowed on to a later check that rejected it for a different reason. Conformance grades the code byte-for-byte, so "rejected anyway" is not a pass.

vector	today	required
021 duplicate Signature-Input key	components_incomplete, step 6	header_malformed, step 1
022 multi-valued Content-Type	invalid, step 10	header_malformed, step 1
023 duplicate Content-Digest key	invalid, step 10	header_malformed, step 1
026 non-ASCII Host	invalid, step 10	header_malformed, step 1

Design

A predicate table, not an if-chain, so a rule has one definition and can raise a different code at a different step elsewhere. host_has_raw_non_ascii is shared with canonicalization — which rejects a malformed authority as request_target_uri_malformed at step 6 — exactly so the U-label rule is not written twice. One rule, two codes, two steps.

Placed before the parse, not around it. RFC 8941 §3.2 permits a duplicate dictionary key to be dropped rather than rejected, and d[key] = v in a loop is what a parser reaches for by default. That is the smuggling vector: append a second entry with the same label and a weaker covered-component set, the parser keeps one, and the signature verifies over fewer components than the producer signed. Once resolved, the ambiguity is invisible.

Placed after the presence pre-check, so an unsigned request still reports request_signature_required rather than header_malformed. No vector covers that combination.

The raw_headers parameter

New, optional, keyword-only; both framework wrappers pass it. Non-breaking.

Every mapping view of headers resolves a repeated name to one value — measured on starlette 0.52.1, dict(...) is first-wins, so an injected second line is the one that disappears. The vectors express each malformed shape as a single comma-joined value because JSON has nowhere to put two lines with the same name; the threat their own $comments describe is a proxy inserting that second line. A gate written over the mapping passes all four vectors and misses the attack. Both forms are now graded.

Known limit — please review this explicitly

The repeated-line rule only bites on ASGI. This is not an implementation shortcut; it is a WSGI property and cannot be fixed here:

• PEP 3333 folds repeated HTTP_* headers into one comma-joined environ value, and
• make_environ writes CONTENT_TYPE / CONTENT_LENGTH to bare environ keys with last-wins and no join.

So 022's exact shape is destroyed one layer above this SDK, and werkzeug's EnvironHeaders.__iter__ iterates the environ dict — .items() structurally cannot yield a repeated name. The Flask raw_headers list therefore carries the same information as dict(request.headers).

Flask still gets every comma-joined rule and still passes all four vectors. It cannot get two-line protection from anywhere. The wrapper passes the list anyway so both arms run the same code path, and _header_precheck's module docstring says why it is inert there. If you would rather not ship an inert Flask arm, say so and I will drop it and document the asymmetry instead.

026 is checked twice, deliberately

Against the Host header and the URL authority. Neither alone is sufficient:

• Starlette drops a non-ASCII Host when building request.url — URL.__init__ gates on _HOST_RE.fullmatch and falls back to scope["server"] (measured: http://test:None/x while request.headers.raw still held the U-label bytes). In production the U-label survives only on the header.
• The vector ships no Host header at all and expresses the case through the URL.

Checking either one alone passes conformance while missing the other half of reality.

Not changed, on purpose

negative/011 and negative/024 already return the correct code and are untouched — a gate that cannot prove malformation must pass traffic through. A predicate simulation over all 40 vectors hits exactly these 4 with zero false positives.

Rejecting a repeated line does 401 an RFC 9110 §5.3-legal split of Signature-Input or Content-Digest. That is a deliberate profile choice: the ambiguity removed is worth more than the flexibility lost, and no known producer splits them. Flagging it because it is a real trade-off, not a free win.

Test plan

make ci-local: 5962 passed, 0 failed, 41 skipped, 2 xfailed (#985's baseline: 5953 / 6). Exactly +9 passed / −4 xfailed: four retired ledger entries plus five new tests.

tests/conformance/signing/test_header_precheck.py drives the two-line form the vectors cannot carry, plus two guards that matter as much as the rejections:

• a multi-algorithm Content-Digest (sha-256=…, sha-512=…) must NOT be rejected — RFC 9530 permits it, only a repeated key is the defect. A gate that rejected every comma would refuse traffic the spec requires accepting.
• the mapping-only path's blindness to a repeated line is pinned by a test, so if it ever changes the docstring is caught being stale.

KNOWN_VERIFIER_GAPS is now empty. All 40 request-signing vectors pass with zero xfail — with #985 and #987, that is the complete 3.1.8 request-signing vector set green.

[aao-ipr-bot]

Approving on the strength of a fail-closed step-1 gate plus a completeness pin that would have caught the hole this PR closes. Right principle: ambiguous input is refused before the parser resolves it, so "pick one" — the thing a proxy inserting a second header line is counting on — never happens.

code-reviewer: sound. security-reviewer: no-high-findings, net security improvement, fail-closed on every traced path. ad-tech-protocol-expert: sound-with-caveats — the caveats are the follow-ups below, none of them a divergence from the upstream wire shape.

Things I checked

• Exception ordering is load-bearing and correct. except TargetUriMalformedError precedes except (ValueError, KeyError) at the build_signature_base call in verifier.py. Since TargetUriMalformedError(ValueError), a reorder silently downgrades every malformed-authority rejection to request_signature_header_malformed with the suite still green — test_target_uri_malformed_boundary.py pins the ordering through the wire boundary, which no direct-call canonicalization test can see. Good instinct writing that guard.
• No import cycle. canonical.py is a leaf (imports only idna + _idna_canonicalize); errors.py → canonical and _header_precheck.py → canonical are one-directional; verifier.py pulls all three. The REQUEST_TARGET_URI_MALFORMED string lives in canonical.py specifically so errors can depend on canonicalization and never the reverse. Confirmed no import-time failure.
• Duplicate-key detection does not false-positive on legal traffic. _duplicate_dictionary_key_reason splits via split_structured_field, whose state machine respects RFC 8941 quoted spans and parens, so a :base64: payload or a quoted comma never false-splits. A multi-algorithm Content-Digest (sha-256=…, sha-512=…) yields distinct keys and is correctly not rejected — RFC 9530 permits it, and test_multi_algorithm_content_digest_is_not_rejected holds the line. Only a repeated key bites.
• U-label cannot reach the signature base uncaught. Checked on both the Host header and the URL authority — if on the header it's step 1; if urlsplit parses the authority it's step 1; if the URL is malformed enough that urlsplit throws, the authority is a required covered component so it dies at step 6. 026's real-traffic shape (str(request.url) drops the non-ASCII Host on Starlette) is why both are needed.
• Fail-open enumeration. Every branch that returns None/skips (_header_pairs latin-1 skip, _non_ascii_authority_reason swallowing TargetUriMalformedError, _wsgi_raw_headers returning None) was traced: none passes traffic that would otherwise be rejected. The WSGI two-line gap is fail-closed — a second line the SDK can't see is a value the producer didn't sign, so it fails verification anyway; it costs the step-1 code, not signature integrity.
• Completeness pin. vector_manifest.json content-hashes 40 vectors + canonicalization.json, and test_vector_completeness.py ties the pin to get_adcp_spec_version(). This is the right shape — a filename-only manifest would not have caught the 016/017/020 drift that motivated it.

On the "Known limit" — reviewed explicitly, as asked

Ship the inert Flask arm. The WSGI gap is a conformance-code gap, not a signature bypass: PEP 3333 folds the second line away above the SDK, and whatever single value survives is the one the signature is verified against — a smuggled line the SDK can't see fails verification, not passes it. security-reviewer confirms fail-closed. Keeping both arms on the same code path with the docstring explaining the asymmetry is worth more than dropping the arm.

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

• Trailing FQDN-root-dot stripping is a unilateral wire change. https://example.com./p now derives example.com, and the spec defines no root-dot handling and ships no vector. A signer on this SDK and a verifier on an older one disagree for FQDN-root URLs. The internal motivation (forcing the ASCII and IDNA host branches to agree) is a real correctness fix, but this should be gated behind an upstream canonicalization vector before it's settled. Test-pinned locally, which is the right holding pattern — file the upstream-vector ask.
• request_target_uri_malformed reaches the wire as a bare string from canonical.py, not registered in the request-family taxonomy block in errors.py the way every sibling code is. Confirm it's in the live 3.1.1 transport-error-taxonomy (the vendored vectors ship it as expected_error_code, and the manifest pins the tree byte-identical to dist/compliance/3.1.1, so it should be) and consider registering it alongside the others.
• Webhook retag onto the existing webhook_signature_header_malformed is the correct fail given no webhook_target_uri_malformed exists anywhere in the repo — but it's asymmetric with the request path (dedicated code there, generic here). Revisit if upstream mints a webhook twin.

Minor nits (non-blocking)

1. _non_ascii_authority_reason judges the full netloc. It calls host_has_raw_non_ascii(netloc) on userinfo+host+port, while _canon_authority strips userinfo before judging the host. A signed URL with non-ASCII userinfo but an ASCII host would 401 as header_malformed even though the host is fine. No realistic signed traffic carries non-ASCII userinfo — cosmetic asymmetry, not a live false positive. src/adcp/signing/_header_precheck.py.
2. Dead exception arm. _header_pairs catches (UnicodeDecodeError, AttributeError), but latin-1 decode is total over bytes and never raises UnicodeDecodeError; only the AttributeError guard is live. src/adcp/signing/_header_precheck.py.
3. canonicalization.json declares "version": "3.0" internally while the manifest pins 3.1.1 and test_manifest_pins_the_spec_version_the_sdk_targets asserts 3.1.1 — a 3.0-stamped fixture living inside a 3.1.1-pinned tree is an interesting artifact of verbatim vendoring. Harmless; upstream's drift to carry, not this PR's.

CI reports 5962 passed / 0 failed and KNOWN_VERIFIER_GAPS empty. Safe to merge.

[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]

Approving on the strength of a fail-closed step-1 gate that closes a real signature-smuggling vector, plus a canonicalizer that finally rejects what the profile requires be rejected. The architectural call is the right one: ambiguity must be refused before the parser resolves it, because "pick one" is exactly what a proxy inserting a second header line is counting on.

Things I checked

• Exception ordering — the load-bearing one. verifier.py:339 except TargetUriMalformedError precedes except (ValueError, KeyError) at the build_signature_base call. Since TargetUriMalformedError(ValueError), reversing them would silently downgrade every malformed-authority rejection to request_signature_header_malformed with the suite still green. Ordering is right, step=6 maps exc.code, and test_target_uri_malformed_boundary.py guards it. Clean.
• Smuggling vector genuinely closed on both arrival shapes. Two-line form (ASGI): repeated-line rule fires on signature-input/content-digest in _SINGLE_LINE_SIGNED_HEADERS before the parser runs. Comma-joined form (everywhere, incl. WSGI via PEP 3333 folding): _duplicate_dictionary_key_reason splits with the same split_structured_field the parser uses, so key detection can't diverge from parse_signature_input_header. security-reviewer: no crafting makes the parser resolve to sig1 last-wins while the precheck sees two keys.
• Multi-algorithm Content-Digest is not false-rejected. sha-256=:...:, sha-512=:...: — content-digest is repeated-line-only, not in _SINGLE_VALUE_HEADERS, so the comma-count rule never touches it; distinct keys pass the duplicate-key rule. RFC 9530 §2 satisfied, pinned by test_multi_algorithm_content_digest_is_not_rejected.
• _port_or_reject closes the raw-vs-canonical int() differential. canonical.py:379 — ASCII-digit-only guard rejects -80, 8_0, and Arabic-Indic ٨٠ that int() and str.isdigit() both accept. host:٨٠ and host:80 no longer collapse to the same @authority. Empty port dropped per RFC 3986 §3.2.3. Right call.
• _canon_host root-dot symmetry. canonical.py:409 strips the trailing dot once, before the ASCII/IDNA branch, with an empty-label recheck afterward so https://./p becomes a typed rejection instead of the https:///p empty authority it collapsed to before. IPv6 literals take the ASCII fast path and never reach canonicalize_host. Pinned by test_canonicalization.py:810 and :778.
• Public surface is additive. raw_headers on verify_request_signature is optional/defaulted (verifier.py:179) — non-breaking; new WEBHOOK_TARGET_URI_MALFORMED / REQUEST_TARGET_URI_MALFORMED codes are additions, not removals. context.py change is docstring-only, no runtime path — no ctx_metadata echo, no credential surface.
• Vector fidelity. ad-tech-protocol-expert: request-side codes and failed_step values match the SHA-256-pinned 3.1.8 canonicalization.json byte-for-byte; request_target_uri_malformed correctly drops the request_signature_ infix as a pre-crypto structural code.

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

• Verify webhook_target_uri_malformed against the spec, not just the commit message. ad-tech-protocol-expert flagged this as the one unverifiable item: it deliberately breaks the webhook_signature_ infix every other webhook code carries, justified solely by a prose claim that security.mdx's webhook checklist defines it at step 10 — and grep finds the string nowhere on the branch. Plausible (mirrors the request-side naming the vectors do use, retag is mechanical via REQUEST_TO_WEBHOOK_CODE), but confirm against adcontextprotocol/adcp @ 3.1.8 security.mdx before an adopter builds on it. If the spec doesn't define it, that's a wrong code on the wire.
• URL-authority non-ASCII check over-rejects non-ASCII userinfo. _header_precheck.py:196-208 runs host_has_raw_non_ascii over the whole netloc, including userinfo, while canonicalization strips userinfo (rsplit("@",1)[-1]) before deriving @authority. https://ünsom@host.example/p false-rejects at step 1 though its signed @authority is clean ASCII. Availability-only, rare, but the gate should judge the same host bytes canonicalization signs.
• ASCII root-dot stripping is stricter than RFC 3986 §6.2.2 and ships ahead of a spec vector. example.com. → example.com is not sanctioned by syntax-based normalization for reg-names; a peer on an RFC-3986-only canonicalizer derives example.com. and every signature between you disagrees. You've documented this and pinned it with an SDK-local test until upstream rules (#979 tracks the sibling empty-query gap) — right handling, just flagging it stays a divergence until the profile ratifies it.

Minor nits (non-blocking)

1. Dead decode branch. _header_precheck.py:_header_pairs catches UnicodeDecodeError, but latin-1 maps all 256 byte values and never raises it. Harmless; the AttributeError guard still earns its place. The docstring's "undecodable bytes are skipped" is slightly misleading.
2. _malformed_authority_reason docstring overstates its call sites. canonical.py claims it is called "from the as-received Host header at the verifier boundary." The precheck shares host_has_raw_non_ascii and _split_or_reject, not this function. Doc-only.
3. WSGI "inert" docstring is more pessimistic than reality. _header_precheck.py:47-53 says WSGI "cannot get two-line protection from anywhere." security-reviewer: the headline Signature-Input/Content-Digest smuggling is in fact still caught on WSGI via PEP 3333 comma-folding + the duplicate-key rule; the genuinely-degraded case is a repeated Content-Type (bare CONTENT_TYPE, last-wins) — and that's not a bypass, since Content-Type is a mandatory covered component and a swap fails crypto at step 10. Worth rewording so adopters don't over-estimate the residual.

Eleven-commit stack, four of them one-line review-follow-up corrections to earlier commits in the same stack — the port-validation and empty-authority fixes catching their own predecessors is the good kind of thorough. make ci-local reports 5962/0 with KNOWN_VERIFIER_GAPS empty and all 40 request-signing vectors green at zero xfail.

Safe to merge. Follow-ups noted above.

[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]

Approving. Step-1 strict rejection is the right shape: refuse ambiguous input before the parser resolves it, because "pick one" is exactly what the covered-component smuggle counts on. Fail-closed beats fail-open, and the shared-predicate design (host_has_raw_non_ascii, one definition, two codes at two steps) keeps the U-label rule from being written twice.

On the "please review this explicitly" ask — the inert Flask arm ships. Keep it. The WSGI reasoning is sound (PEP 3333 folds HTTP_*, make_environ writes CONTENT_TYPE last-wins with no join, so 022's shape dies one layer above this SDK), and the residual two-line gap on WSGI is fail-closed anyway: Content-Type is a covered component, so a proxy-mutated line reconstructs a different signature base and verification fails on its own. security-reviewer traced the same conclusion. Shipping both arms over one code path with the docstring saying why it's inert is the correct posture — better than an undocumented asymmetry.

Things I checked

• Ordering is correct: strict_header_precheck at verifier.py:207 runs after the presence pre-check (sig_input_raw/sig_raw lookup + request_signature_required) and before parse_signature_input_header. An unsigned request still reports request_signature_required; a duplicate key is caught before the parser can drop it. No vector covers that combination and the sequencing gets it right.
• _duplicate_dictionary_key_reason splits on split_structured_field(value, ",") — quote/paren aware — then keys on entry.split("=",1)[0]. A multi-algorithm Content-Digest (sha-256=:…:, sha-512=:…:) yields distinct keys and is NOT rejected; base64 = padding and :-delimited bytes don't confuse the split. RFC 9530 traffic survives; only a repeated key trips. ad-tech-protocol-expert: sound.
• content-digest is in _SINGLE_LINE_SIGNED_HEADERS (repeated-line rejected) but deliberately NOT in _SINGLE_VALUE_HEADERS (comma allowed) — correct, that's the whole distinction between RFC 9530 multi-algorithm and a duplicate key.
• No fail-open. _non_ascii_authority_reason catches TargetUriMalformedError and returns None — that's not a swallow, it hands the malformed authority to canonicalization's step-6 request_target_uri_malformed (canonical.py:148, _split_or_reject). split_structured_field and host_has_raw_non_ascii are total and don't raise, so the gate can't turn a 401 into a 500.
• raw_headers is a transient parameter consumed only inside the precheck. It never lands in RequestContext.metadata and cannot reach the context-echo or idempotency replay path. context.py change is docstring-only.
• Public-API audit: raw_headers is new, optional, keyword-only, defaulted. Non-breaking — fix(signing): without ! is the correct semver signal. verify_request_signature's existing callers are unaffected.
• Test-plan honesty: make ci-local reported 5962/5962 with KNOWN_VERIFIER_GAPS emptied and zero xfail across all 40 request-signing vectors. The new tests drive the two-line form the JSON vectors structurally can't carry, plus the multi-algorithm false-positive guard. No unchecked manual box on the primary path.

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

• Signature is in _SINGLE_LINE_SIGNED_HEADERS but not run through _duplicate_dictionary_key_reason. signature-input and content-digest get the duplicate-key check; Signature is itself an RFC 8941 Dictionary and a single-line sig1=:X:, sig1=:Y: isn't caught at step 1. Not a hole — covered components derive from Signature-Input, so forged Signature bytes fail verification rather than downgrade it — but it's an inconsistency with the step-1 philosophy and no vector covers it. ad-tech-protocol-expert flagged it. Intentional or a gap worth a test?
• Unquoted multipart boundary with a top-level comma. RFC 2046 bcharsnospace permits ,, so Content-Type: multipart/form-data; boundary=a,b (unquoted) has a top-level comma and gets refused at _header_precheck.py:96. Quoted boundaries are protected by split_structured_field. Real risk for AdCP is ~zero (requests are application/json), but it's a spec-legal input the gate refuses — worth a line in the trade-off list alongside the split-Signature-Input note.

Minor nits (non-blocking)

1. Dead except arm in _header_pairs. bytes.decode(\"latin-1\") maps all 256 values and never raises UnicodeDecodeError, so that arm is unreachable; the live catch is AttributeError when an adopter passes (str, str) tuples — in which case every pair is silently skipped and the header checks become no-ops (only the URL-authority check survives). Both shipped wrappers pass bytes, so deployed config is safe. A docstring contract or type assertion that entries must be bytes would harden it.
2. Docstring example mixes frameworks. context.py:211-216 pairs Flask's request.get_data() with raw_headers=getattr(request.headers, \"raw\", None), which is always None on a real EnvironHeaders. Technically the documented WSGI limit, but a mixed example may read as an ASGI capability an adopter can reach from Flask. Point at _wsgi_raw_headers or split ASGI/WSGI examples.

One dry note: the third signing PR in a row that closes a conformance gap by explaining, at length, exactly what it cannot close. That's the correct instinct — the "Known limit" section did more review work than the diff did.

LGTM. Follow-ups noted below.