fix(signing): harden trust and request atomicity - #999
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| release.set() | ||
|
|
||
| first_result, second_result = await asyncio.gather(first, second) | ||
| assert calls == 1 |
|
|
||
| release.set() | ||
| first_result, second_result = await asyncio.gather(first, second) | ||
| assert calls == 1 |
| assert calls == 1 | ||
| release.set() | ||
| retry_result = await retry | ||
| assert calls == 1 |
|
|
||
| def claim(self, keyid: str, nonce: str, ttl_seconds: float) -> ReplayClaimResult: | ||
| """Atomically reserve a nonce, or report why it cannot be reserved.""" | ||
| ... |
| await entered.wait() | ||
| first.cancel("client disconnected") | ||
| with pytest.raises(asyncio.CancelledError, match="client disconnected"): | ||
| await first |
| try: | ||
| start.wait() | ||
| results.append(resolver("replacement")) | ||
| except BaseException as exc: # pragma: no cover - asserted below |
KonstantinMirin
left a comment
KonstantinMirin
left a comment
There was a problem hiding this comment.
Review — PR #999
Overview — Read as a Codex security-CLI pass, this behaves like one: the primitives it adds are sound, and the failures cluster into three recognisable shapes rather than scattering. Worth keeping as-is — the atomic-claim design in InMemoryReplayStore, the indexed expiry heap and the _NoItemsDict test, and the single-flight threading.Lock in CachingJwksResolver, which is proven by a test that fails on revert.
The three shapes, all one root: a local security property tightened without the surface it depends on.
- Capability detected by shape instead of by contract.
type(self.backend).hold is not IdempotencyBackend.holdis a correct-looking duck-type probe that a delegating wrapper always satisfies.LazyBackenddefines both methods as forwarders, so the probe answers "native" for whatever it wraps: webhook replay dedup off and silent, request pathNotImplementedError. This one is a regression againstmain(1, 11). - Fail-closed flips landed without threading the surface they need.
expected_key_origins is Nonecollapsed with{}(4);covers_content_digestflipped to"required"against the pinned 3.1.8 schema default (5). Each is right read alone and rejects traffic the spec says must verify. - Bounds that count the wrong quantity or invent a number the spec does not have. A byte budget that never bounds decompression (9); cache lifetimes 2× and 4× the ceilings they derive from (14).
The reason none of it surfaced is one finding, and it is the one to fix first: 31 one-at-a-time reverts leave the suite green (16) — including test_concurrent_deliveries_have_exactly_one_first_seen passing verbatim against the pre-PR non-atomic get-then-put sequence. Until a revert turns something red, this class of change cannot be told apart from a working one, and the same three shapes will land again on the next pass.
Should fix
1. Backend capability probed by method identity — LazyBackend defeats it
IdempotencyStore._hold (store.py:144) and WebhookDedupStore._put_if_absent (webhook_dedup.py:99-101) decide whether a backend is atomic with type(self.backend).hold is not IdempotencyBackend.hold. LazyBackend always defines hold and put_if_absent as forwarding wrappers (lazy.py:114, :118), so the probe reports "native atomic backend" for whatever it wraps, including the legacy backends the compatibility path exists for. The inner call then reaches IdempotencyBackend.hold's raise NotImplementedError (backends.py:114).
# LegacyBackend (get/put/delete_expired only) behind LazyBackend:
PR head main
webhook check_and_record -> True True True True False False
deprecation warnings -> [] []
entries written -> 0 1
request path -> NotImplementedError OK
Webhook replay dedup is off and silent: check_and_record's except Exception: ... return True (webhook_dedup.py:126-159) swallows the NotImplementedError and reports first-seen on every delivery. The request path raises NotImplementedError on every idempotent call. Both contradict the PR body's "Legacy replay/idempotency backends continue through warned process-local compatibility locks".
Root cause: there is no capability contract, only a same-object method-identity probe living in the layer that consumes the backend. IdempotencyBackend.hold is declared -> Any with a runtime NotImplementedError instead of an @abstractmethod returning AbstractAsyncContextManager[None], which is also why mypy is happy. Make the capability a declared, delegable property of the backend contract (a supports_atomic_hold() predicate, or a runtime_checkable Protocol) and have LazyBackend answer for the backend it resolves. The same root drives a DRY problem: the keyed-lock fallback is written three times in this diff (backends.py:170-181, store.py:152-165, webhook_dedup.py:114-124), and the two store copies keep per-instance lock tables, so two stores over one backend do not serialize against each other. Once the capability is a contract, wrap a legacy backend in a _ProcessLocalHold adapter at construction and both store bodies collapse to one path.
2. The supervising task shadows operation, so IDEMPOTENCY_CONFLICT returns a Task repr to the buyer
store.py:217 binds operation = getattr(handler, "__name__", "handler"). store.py:293 rebinds the same local: operation = asyncio.create_task(_execute_locked()). _execute_locked closes over that cell and reads it at store.py:254, after the rebind.
PR head main
exc.operation -> Task str
str(exc) -> <Task pending name='Task-3' coro=<... create_media_buy: idempotency_key
running at /private/tmp/.../wt-999/src/ reused with a different payload
adcp/server/idempotency/store.py:253> ...
Any authenticated buyer that reuses one idempotency_key with a changed payload gets the deployment's absolute source path and Python install path in the error message, and the spec-defined IDEMPOTENCY_CONFLICT loses the operation field that dispatch and audit layers key on (exceptions.py:381). Rename the task (supervised, execution_task) and assert exc.operation == "create_media_buy" on the conflict path — the existing conflict tests check only the exception type (tests/test_server_idempotency.py:679,967), which is why this path is green.
3. Every handler exception is ERROR-logged as a post-cancellation failure
store.py:293-296 attaches _finish_supervised_operation unconditionally, and the callback (store.py:71-81) logs at ERROR whenever task.exception() is not None. That is the normal terminal state for any handler exception the caller already received through await asyncio.shield(operation), including the spec-defined IDEMPOTENCY_CONFLICT above. Three ordinary handler ValueErrors produce three ERROR records claiming cancellation with a full traceback each.
So a buyer sending malformed input drives ERROR-level log volume at request rate, the message asserts something false, and the operator signal the callback exists to provide ("work continued past a client disconnect") is buried in noise. The callback cannot know whether the awaiter was cancelled; the awaiter can. Record a flag when await asyncio.shield(...) raises CancelledError and log only in that case. A test asserting no ERROR record on the conflict path pins it — the file already has the caplog idiom in test_put_failure_logs_warning_and_returns_handler_result.
4. The key-origin fail-closed flip landed without the webhook surface, so brand.json webhook receivers reject everything with the wrong purpose
verifier.py:576 stopped skipping the step-7 check when expected_key_origins is None, and :614 passes expected_key_origins or {}. verify_webhook_signature (webhook_verifier.py:141-160) builds its inner VerifyOptions with neither expected_key_origins nor signing_purpose, and WebhookVerifyOptions has no field for either.
# resolver with jwks_source = "brand_json" (the spec's canonical webhook trust anchor):
PR head -> webhook_signature_key_origin_missing
"identity.key_origins.request_signing declaration missing"
main -> accepted (with the warning that named the affordance)
Two defects. Every webhook is rejected for a receiver conforming to the exported BrandSourcedJwksResolver protocol, and the emitted purpose is request_signing where AdCP 3.1.8 names webhook_signing for webhook delivery (get-adcp-capabilities-response.json → identity.key_origins.webhook_signing; security.mdx:1442).
Root cause: None ("the SDK caller supplied no map") was collapsed with {} ("capabilities were observed and declared no map"). The spec's reject predicate is a statement about the counterparty's capabilities document — security.mdx:1108 @ 3.1.8 says "If the agent declares signing without a corresponding identity.key_origins.{purpose} entry, reject" — while the schema says of an absent map: "Absent means the operator has not declared a separation scheme; receivers SHOULD assume shared-origin." The pre-PR code drew exactly that distinction, and the deleted docstring documented the affordance ("pass an empty dict if the operator advertises no map"). Keep the two cases distinct, have verify_from_agent_url pass resolution.key_origins or {} so the capabilities-derived path fails closed where the spec requires, add expected_key_origins and posture to WebhookVerifyOptions, and pass signing_purpose="webhook_signing" from verify_webhook_signature. Cite security.mdx:1108 @ 3.1.8 on the rejecting branch. No test caught this: every webhook test uses a bare StaticJwksResolver with no jwks_source.
5. covers_content_digest default flipped to "required", against the 3.1.8 schema default and this repo's own migration guide
verifier.py:105. The pinned schema defines the default: dist/schemas/3.1.8/protocol/get-adcp-capabilities-response.json → properties.request_signing.properties.covers_content_digest.default == "either". agent_resolver.py:767 builds VerifierCapability(supported=True) when the caller omits capability, so verify_from_agent_url inherits the flip.
vector positive/001-basic-post.json "Basic POST, Ed25519, no content-digest coverage"
expected_outcome.success: True
verified with VerifierCapability(supported=True)
-> request_signature_components_incomplete (step 6)
The vectors pass only because the harness hard-codes cap_data.get("covers_content_digest", "either") (test_verifier_vectors.py:50), so the graded artifact still encodes the spec default while the SDK no longer does. docs/request-signing-migration.md:123 says "Don't enable covers_content_digest="required" yet" and :191 tells adopters to stay on "either" when intermediaries touch bodies.
Root cause: VerifierCapability is both the block a verifier advertises on get_adcp_capabilities (its own docstring says so) and the local enforcement policy, so hardening one falsifies the other. A seller rendering capabilities from the wire model now advertises either and enforces required. Restore "either" as the dataclass default and express the strict posture as a separately named opt-in, or split the wire mirror from a VerifierPolicy that defaults strict. Either way docs/request-signing-migration.md moves in the same commit. This rejects previously-accepted spec-legal traffic through the production helper, so it needs a deliberate call on whether it ships under fix without !.
6. InMemoryReplayStore.remember silently drops the write at cap
replay.py:93-98: when either cap is reached and the key is new, remember returns without recording.
s = InMemoryReplayStore(per_keyid_cap=2, global_cap=10)
s.remember('k','n1',60); s.remember('k','n2',60); s.remember('k','n3',60)
s.seen('k','n3') # PR head: False main: True
remember is on the public ReplayStore Protocol (replay.py:34) and seen then remember is the documented legacy composition that verifier._claim_replay_nonce and _NamespacedReplayStore still use, so a caller composing them accepts that nonce for the rest of the TTL. AdCP 3.1.8 security.mdx:1322 is explicit about this failure mode: "On cap exceeded, verifiers MUST reject new signatures from that keyid with request_signature_rate_abuse — NOT silently evict … Silent eviction is the dangerous mode: it creates replay windows exactly when the verifier is under attack." Before this PR remember always recorded and the cap was enforced by at_capacity. Either keep recording over cap, or make the refusal observable (raise, or change the Protocol's return type) so a two-step caller cannot mistake "not recorded" for "recorded". test_renewal_heap_remains_bounded currently pins the silent drop rather than flagging it.
7. Cold-path revocation freshness raises outside the SignatureVerificationError family and erases the spec's grace window
revocation_fetcher.py:452-455 rejects a successfully fetched, signed list the instant next_update has passed, with RevocationListParseError and zero skew tolerance — while the sibling updated check two lines above allows 60 s. On the cold path (_current_list is None) _ensure_fresh does not catch it, so it escapes __call__, escapes the unguarded options.revocation_checker(keyid) call in verify_request_signature, and reaches the adopter as a type unauthorized_response_headers cannot map (it takes SignatureVerificationError).
cold checker, list next_update 14:15Z, now 14:16Z (1 min past, inside grace)
-> RevocationListParseError: revocation list next_update '2026-04-18T14:15:00Z' is already expired
(not a SignatureVerificationError -> 500, not 401 + request_signature_revocation_stale)
AdCP 3.1.8 puts the boundary elsewhere: security.mdx:1333 — "verifiers that have not refreshed within next_update + grace MUST reject new request-signed mutations with request_signature_revocation_stale". At the spec's 1-minute polling floor any fetch latency across the boundary trips this. Root cause: a freshness decision placed in the parse/validate layer, which owns neither the grace window nor the wire code. The static-list path at verifier.py:279 answers the same question correctly with REQUEST_SIGNATURE_REVOCATION_STALE at step 9, so the diff now has two answers. Install the fetched list and let _ensure_fresh's grace check own the rejection.
8. The cached JWKS failure object is re-raised, so request N gets request M's wire code and a traceback that grows 2 frames per request
jwks.py:471 and :612 do raise self._last_failure or SignatureVerificationError(...), re-raising the instance stored at :491/:499 and :644/:652.
first refresh fails with SSRFValidationError -> request_signature_jwks_untrusted (3 tb frames)
200 further calls inside the 30s cooldown:
codes: {'request_signature_jwks_untrusted'} same object: True
tb frames: 403 upstream fetches: 0
Two consequences. The wire code is a per-request judgment and request_signature_jwks_untrusted denotes an SSRF verdict on this resolution, so requests whose key resolved fine from cache are rejected with a verdict about an earlier request. And during a publisher outage at production rate the traceback on one long-lived shared object grows without bound, so every handler that formats it writes a linearly growing record. The fallback branch already builds the right per-request code. Cache (code, message) and construct a fresh SignatureVerificationError(...) at each raise, carrying the prior failure as __cause__.
9. The new byte budget does not bound decompression
_bounded_http.read_limited_bytes / async_read_limited_bytes (_bounded_http.py:30, :50) count accumulated decoded bytes from iter_bytes/aiter_bytes. httpx's content decoder inflates each raw chunk in full before iter_bytes re-chunks the output, so a compressed body walks straight through the budget. The content-length pre-check is the encoded length and is absent under chunked transfer.
203 KB gzip stream decoding to 200 MB, read with limit=1024
-> raised: response exceeds 1024 bytes (received at least 1025)
-> peak RSS: 295 MB
None of the four converted fetchers overrides Accept-Encoding, so httpx advertises gzip on all of them: jwks.py:377 / :547, brand_jwks.py:641, agent_resolver.py:269, revocation_fetcher.py:284 / :331. Every one of those reads from a counterparty-controlled origin, and with the real defaults (256 KiB brand.json, 1 MiB JWKS) the amplification budget is far larger than in this repro. Send Accept-Encoding: identity on these fetches (they are small JSON/JWS documents) or bound the raw stream via iter_raw and decode with zlib.decompressobj().decompress(chunk, max_length=remaining). Add a content-encoding: gzip case to tests/conformance/signing/test_bounded_fetches.py, which today covers identity-encoded chunking only.
10. One process-wide replay store shares its global_cap across every counterparty, and the namespace key is not canonicalized
agent_resolver.py:585 creates one process-wide InMemoryReplayStore() (global_cap=1_000_000) and :760-763 wraps it per identity in _NamespacedReplayStore(..., str(httpx.URL(resolved_agent_url))). The wrapper prefixes the keyid, so it isolates the per-keyid cap. It does not isolate the global cap, which claim and at_capacity both check.
shared = InMemoryReplayStore(global_cap=3)
a.claim('kid1', n0..n2) -> ['claimed','claimed','claimed']
b.claim('kid1','fresh') -> capacity # b's first-ever request
b.at_capacity('kid1') -> True # -> REQUEST_SIGNATURE_RATE_ABUSE
The one bound that constrains a hostile counterparty is the resource every other counterparty in the process depends on. Second half: the namespace string is resolution.agent_entry.get("url"), taken from the counterparty's own brand.json, normalized only by str(httpx.URL(...)). That is not an origin canonicalization, so query and fragment spellings mint distinct namespaces and one kid gets N × per_keyid_cap live nonces:
'https://a.evil/mcp' -> 5 'https://a.evil/mcp?x=1' -> 5 'https://a.evil/mcp#f' -> 5
total live entries under one kid, per_keyid_cap=5: 15
Give each namespace its own budget (per-namespace global_cap, or one store per resolved identity in a bounded LRU), and derive the namespace from a canonicalized origin using _idna_canonicalize.canonicalize_host, which the repo already owns. Replay state ownership belongs with the verifier rather than as import-time global state in the resolver, so the test seam is a parameter rather than monkeypatch.setattr on a private module attribute (tests/test_verify_from_agent_url.py:303).
11. One capability, three detection mechanisms, an unused Protocol, and a warning that fires per request
replay.py:39 adds AtomicReplayStore(ReplayStore, Protocol) with claim. It appears in zero call sites and zero tests, and it is not @runtime_checkable, so it cannot be used for the check it was added for. The capability is instead detected three ways: getattr(store, "claim", None) in verifier.py:377, a second getattr probe in _NamespacedReplayStore.claim (agent_resolver.py:607-612) with no warning, and type(x).m is not Base.m in store.py:144.
Consequences beyond the duplication. verifier.py:381 writes result: ReplayClaimResult = claim(...) — an annotation-only assertion about a value from adopter code, which is what the Protocol was for; a store whose claim() returns True verifies the identical signature three times in a row with seen/remember never called and no warning. And because _NamespacedReplayStore always exposes claim, wrapping any store in it makes _claim_replay_nonce take the atomic branch unconditionally, suppressing the non-atomic-backend signal. Meanwhile verifier.py:385-391 warns on every verification where its two siblings added in this same diff warn once (store.py:139/149/157, webhook_dedup.py:90/104/113):
RuntimeWarnings emitted for 500 verifications: 500
warnings.warn(..., stacklevel=2) costs stack introspection per request, and an adopter running -W error::RuntimeWarning gets a 100% failure rate after crypto verification has already succeeded.
Mark AtomicReplayStore @runtime_checkable and narrow with isinstance (or add one _supports_atomic_claim(store) helper in replay.py), route _NamespacedReplayStore through the same helper instead of carrying its own copy, and mirror the once-flag on the warning. Declaring _NamespacedReplayStore against ReplayStore/AtomicReplayStore also gets it type-checked, which the bare duck-typed class is not.
12. PgBackend.hold holds a lock-pool connection for the whole handler, including cache-hit replays
backends.py:466-479 checks out a lock_pool connection and opens a transaction around the yield, and store.py:239-243 takes the hold before the cache lookup, so a pure replay (cache hit, no handler run) consumes a lock connection and an advisory lock. The class docstring example (backends.py:251-252) sizes both pools at max_size=10, which makes 10 the ceiling on concurrent idempotent requests; the 11th waits out the psycopg_pool timeout and raises PoolTimeout, a 5xx to a buyer who retries with the same key. asyncio.shield makes it worse: a disconnected client's operation keeps its lock connection checked out to completion.
Three further mismatches. backends.py:317 describes lock_pool as "reserved for advisory-lock transactions", but get/put inside a hold run on that connection (:418-420, :456-458), so it carries the wrapped path's cache traffic. The held connection travels through a per-instance ContextVar compared against asyncio.current_task(), and put_if_absent and delete_expired do not participate, so inside one hold block two of four data methods run in the lock transaction and two run on separate pool connections, with nothing in the signature saying which. And the lock_pool is pool check at :339-340 is a spot check: a second pool the adopter also uses for handler SQL reproduces the deadlock it claims to prevent.
Root cause: hold yields nothing, so the connection is passed by an implicit channel instead of an explicit handle. Have hold yield a bound session the store threads into get/put/put_if_absent, and annotate the ABC's hold as AbstractAsyncContextManager[<handle>] instead of Any so a backend that ignores the handle fails type-check. Read the cache once outside the lock and acquire the hold only on a miss. Also: lock_pool is now a required keyword (backends.py:334), so PgBackend(pool=pool) raises TypeError on upgrade and docs/handler-authoring.md:632 still shows exactly that call — update the doc in this PR, and decide whether a required-kwarg break ships under fix(...).
13. client._origin re-rolls host comparison instead of using the repo's canonicalizer
src/adcp/signing/client.py:95 compares hosts with parsed.hostname.lower(). _idna_canonicalize.py exists for this and its module docstring enumerates the callsites that canonicalize host strings for comparison; jwks.py, ip_pinned_transport.py, revocation_fetcher.py, key_origins.py, brand_jwks.py and etld.py all use it, and the two commits immediately before this PR (b7ef1dfc, 827e4f4e) fixed this same class of bug.
_origin("https://sellér.example.com") -> ('https', 'sellér.example.com', 443)
_origin(httpx.Request(...).url) -> ('https', 'xn--sellr-esa.example.com', 443)
match: False
An operator who passes a unicode expected_origin has every signed request refused, blamed on a redirect that never happened. Fail-closed, so not exploitable, but it is a silent misdiagnosed outage for a legitimate config. Route both sides of _origin through canonicalize_host, which already short-circuits IP literals. The same primitive fixes the namespace half of finding 10. Related: the invariant "a hook signs for exactly one origin" now lives in one of the two signing request hooks. ADCPClient._sign_outgoing_request (src/adcp/client.py:968-1049) is the semantically equivalent hook the SDK's own transports install (client.py:581 → mcp.py:370,1020, a2a.py:263) and it has no origin binding. Those paths pin follow_redirects=False, so the redirect-capture vector is not live there today; the structural problem is that the next change to either hook drifts. Build ADCPClient's hook through install_signing_event_hook(..., expected_origin=...) and delete the duplicate body.
14. New cache-lifetime and count defaults exceed or invent the 3.1.8 bounds they derive from
Three uncited constants, all new and exported in this diff:
DEFAULT_JWKS_MAX_AGE_SECONDS = 3600.0(jwks.py:49).security.mdx:1457@ 3.1.8: "JWKS cache TTL bounded above by the revocation-list polling interval (floor 1 min, ceiling 30 min)". 3600 s is 2× the ceiling and 4× the spec's owncacheMaxAge: 15 * 60 * 1000example, so a compromisedkidadded torevoked_kidskeeps verifying on the JWKS path for up to an hour. Introducing a max-age at all is the right call —:1457also says verifiers MUST NOT pin one snapshot for a task's lifetime — the number is what needs fixing.DEFAULT_MAX_STALE_SECONDS = 3600.0(brand_jwks.py:100) stacks onDEFAULT_MAX_AGE_SECONDS = 3600.0for up to 7200 s of trust in an expired brand.json snapshot.security.mdx:1103@ 3.1.8: "Cache TTL on a successful fetch MUST be bounded above by the JWKS revocation polling interval (so a key rotation cannot be masked by a stale brand.json)".DEFAULT_MAX_SCHEMA_IDS = 32(references.py:45, enforced at:589). The normativeformat_schemafetch contract (docs/creative/canonical-formats.mdx:217-231@ 3.1.8) enumerates its bounds —$refdepth ≤ 8, total$refcount ≤ 256, compiled keyword count ≤ 10 000, 1 MiB streaming cap, ≤ 5 s timeout — and has no$idbound. A schema with 200$refs and 40$ids satisfies every listed bound and is nowINVALID_SCHEMA, which per:230means the buyer loses the product declaration. Two conformant SDKs then disagree on whether the same digest-pinned document resolves.
Derive the first two from the configured revocation polling interval so the spec's "bounded above by" relation holds by construction, and cite security.mdx:1457 / :1103 @ 3.1.8 at each constant. For the third: the cost being defended is _validate_schema_id_value's _resolve_public_https call per $id, and $id must already be same-origin or the AAO catalog, so at most two distinct hosts per document — memoize the host resolution instead of lowering the acceptance bar, or raise the default to the spec's own 256.
15. The post-crypto over-cap rejection is stamped step="9a", and the step-9a citation was deleted
verifier.py:346-351 raises REQUEST_SIGNATURE_RATE_ABUSE with step="9a" after crypto verify. AdCP 3.1.8 security.mdx:1324 names a different step: "step 9a remains the cheap amplification guard, step 13 is the authoritative enforcement point. A verifier whose atomic insert returns over-cap MUST reject the request with request_signature_rate_abuse". step is adopter-facing through middleware.verify_starlette_request, so it should name the step that actually failed. That paragraph is also the grounding for the whole atomic-claim design and it is cited nowhere in the diff, while verifier.py:289-291 removed the one citation that was there ("Step 9a (per spec, after adcp#2342): the per-keyid cap runs between JWKS resolution and crypto verify"), leaving the ordering constraint the early check exists to satisfy undocumented. Stamp the post-crypto rejection step=13, keep the code, and restore the ordering citation on the early at_capacity guard.
16. Most of the changed behavior survives deletion with the suite green
31 one-at-a-time reverts were run against the 706-test selection for the touched areas. The suite stayed green for all of the following, so none of this behavior is fenced:
test_concurrent_deliveries_have_exactly_one_first_seenpasses verbatim against the pre-PRget-then-putsequence (confirmed: 20 tasks,Truecount 1 either way).MemoryBackend.get/puttake an uncontendedasyncio.Lock, so no coroutine ever suspends and the 20 tasks serialize by scheduling accident. Give the backend a real suspension point between read and write, or assertMemoryBackend.put_if_absentreturnsFalsefor the second caller directly.- Deleting the native branch in
_hold, deleting it in_put_if_absent, and replacing the ABC's documentedhold+get+putcomposition withreturn Trueare each green. Nothing detects a silent downgrade from aPgBackend's cross-process guarantee to per-process locking, which is the load-bearing decision of this PR. Assert the absence of the compatibilityDeprecationWarningon the supported backends, and cover a hold-only custom backend through the ABC default. _keyid()→return keyid(namespacing removed) is green. The namespacing is what justifies the process-wide shared default store; nothing asserts two agent URLs can claim the same(kid, nonce).- Deleting both JWKS cooldown
raiseblocks is green. The behavior is observable: with the guard, a JWKS-origin outage costs 2 upstream fetches across 5 inbound requests; without it, 5. - All four of
BrandJsonJwksResolver's new fail-closed branches mutate cleanly (unbounded stale-serve restored,can_refreshre-anchored onfetched_at,_sync_selector's clearing removed,_do_refresh's_last_errorrecording removed).test_authz_stale_on_error_is_boundedcovers the authorization resolver; the JWKS resolver serves key material andDEFAULT_MAX_STALE_SECONDSappears in no test. - Deleting the
claim == "capacity"block (a full replay cache then accepts) and dropping the global-cap term fromat_capacityare both green. Both sites emit a normative wire code. - A
claim()returningTrue(wrong type, right arity) verifies the identical signature three times with no warning.AtomicReplayStoreandReplayClaimResultappear in zero test files, while this repo does pin protocol conformance elsewhere (test_pg_idempotency_backend.py::test_satisfies_idempotency_backend_protocol). - The
content-lengthpre-check in both readers deletes green;_index_jwks_keys's non-string-kidand non-list-keysrejections delete green ({"keys": {}}then yields an empty primed cache that satisfies the cooldown formax_ageseconds); thebase_url-fallback origin binding and theexpected_origin-with-no-schemeValueErrorboth delete green; the async_ensure_fresh304 change deletes green while its proven sync twin does not; weakeningput'sactive[1] is asyncio.current_task()check toif active is not None:deletes green. - Removing the SSRF re-validation on the capabilities redirect target (
agent_resolver.py:249-255) is green —grepfinds no 3xx /Location/max_redirectscase for that fetcher anywhere. Note this branch is unreachable atDEFAULT_CAPABILITIES_MAX_REDIRECTS = 0(hop 0 == max_redirects raises the limit error first) and only live when an adopter raises the limit, which is itself untested.
The two Postgres files skip locally for lack of psycopg and run in the pg-conformance job, so this is what they assert rather than whether they run: test_concurrent_claim_same_nonce_has_one_winner's one-winner outcome already follows from the (keyid, nonce) primary key plus ON CONFLICT … WHERE expires_at <= now() RETURNING 1 without pg_advisory_xact_lock — the advisory lock serializes the capacity leg, which no test races, and PgReplayStore.claim's "capacity" return has no PG test. test_concurrent_wrapped_calls_execute_once is single-process, so the process-local fallback satisfies it and it does not demonstrate the cross-process guarantee lock_pool was introduced for. Reasoned from the SQL, not measured — worth the author's confirmation.
Notes
DEFAULT_GRACE_MULTIPLIER = 2.0with the comment "Spec recommends 2×" (revocation_fetcher.py:76) contradicts pinned 3.1.8, which says grace = 4× the previous polling interval in both places it appears (security.mdx:721,:1333). The line is untouched context in this diff, so out of scope here — it compounds finding 7, which removes the grace window entirely on the cold path. Worth its own ticket.tests/test_pg_idempotency_backend.py::test_delete_expired_defaults_to_wall_clockfailed once in ~20 full runs (before <= cutoff_dt.timestamp() <= after, float round-trip throughdatetime.fromtimestamp). The assertion is pre-existing and this PR touched only the constructor line in that test, so it is out of scope for this diff.PgBackend.hold,PgBackend.put_if_absentandPgReplayStore.claimwere not exercised against a live Postgres in this review — the advisory-lock andON CONFLICT … WHERE expires_at <= now()logic in finding 12 is reviewed by reading only.
| """Use backend atomicity or a warned process-local legacy fallback.""" | ||
| has_native_put = type(self.backend).put_if_absent is not IdempotencyBackend.put_if_absent | ||
| has_native_hold = type(self.backend).hold is not IdempotencyBackend.hold | ||
| if has_native_put or has_native_hold: |
There was a problem hiding this comment.
LazyBackend always defines put_if_absent and hold as forwarding wrappers (lazy.py:114, :118), so this probe reports "native atomic backend" for whatever it wraps, including the legacy backends the fallback below exists for. The inner call reaches IdempotencyBackend.hold's raise NotImplementedError, which check_and_record's except Exception: ... return True swallows.
LegacyBackend behind LazyBackend, three deliveries of the same key:
PR head -> True True True warnings: [] entries written: 0
main -> True False False warnings: [] entries written: 1
Webhook replay dedup is off and silent. Root cause: IdempotencyBackend.hold is declared -> Any with a runtime NotImplementedError rather than an @abstractmethod returning AbstractAsyncContextManager[None], so there is no capability contract for a delegating wrapper to forward. Make the capability a declared predicate (or a runtime_checkable Protocol) that LazyBackend answers for its resolved target, and add a LazyBackend-wrapped variant to test_legacy_backend_warns_and_preserves_repeat_dedup — the current test uses a bare legacy backend and passes.
| @asynccontextmanager | ||
| async def _hold(self, scope_key: str, key: str) -> AsyncIterator[None]: | ||
| """Use native backend locking or a deprecated process-local fallback.""" | ||
| if type(self.backend).hold is not IdempotencyBackend.hold: |
There was a problem hiding this comment.
Same method-identity probe as webhook_dedup.py:101, same blind spot: LazyBackend defines hold, so this takes the native branch and every idempotent request raises NotImplementedError from IdempotencyBackend.hold. Verified against a LegacyBackend behind LazyBackend (main returns the response fine).
Deleting this whole native branch also leaves the 706-test selection green, so nothing detects a PgBackend deployment silently downgrading from its cross-process guarantee to per-process locking. Assert the absence of the compatibility DeprecationWarning under warnings.simplefilter("error", DeprecationWarning) for MemoryBackend and PgBackend.
| ) | ||
| return response | ||
|
|
||
| operation = asyncio.create_task(_execute_locked()) |
There was a problem hiding this comment.
This rebinds operation, bound at line 217 to getattr(handler, "__name__", "handler"). _execute_locked closes over the same cell and reads it at line 254, after the rebind, so IdempotencyConflictError(operation=...) receives the Task.
PR head: exc.operation -> Task
str(exc) -> "<Task pending name='Task-3' coro=<... running at /private/tmp/.../src/adcp/
server/idempotency/store.py:253> ...>: idempotency_key reused with a ..."
main: exc.operation -> str
str(exc) -> "create_media_buy: idempotency_key reused with a different payload ..."
Any buyer reusing one idempotency_key with a changed payload gets the deployment's absolute source path back, and IDEMPOTENCY_CONFLICT loses the operation field that exceptions.py:381 publishes to dispatch and audit. Rename the task (supervised) and assert exc.operation == "create_media_buy" — the existing conflict tests check only the exception type.
| exc = task.exception() | ||
| if exc is not None: | ||
| logger.error( | ||
| "Idempotency operation failed after its request was cancelled", |
There was a problem hiding this comment.
This fires for any handler exception, not only for one that outlived a cancelled awaiter. task.exception() is not None is the normal terminal state for an exception the caller already received through await asyncio.shield(operation), including the IdempotencyConflictError raised at line 254.
So a buyer sending malformed input drives one ERROR record with a full traceback per request, the message asserts something false, and the "work continued past a client disconnect" signal this callback exists to provide is buried. The callback cannot know whether the awaiter was cancelled; the awaiter can. Set a flag when await asyncio.shield(...) raises CancelledError and log only then. test_put_failure_logs_warning_and_returns_handler_result already shows the caplog idiom for pinning it.
| UserWarning, | ||
| stacklevel=2, | ||
| ) | ||
| if expected_key_origins is None and source != "brand_json": |
There was a problem hiding this comment.
Dropping the expected_key_origins is None skip collapses two distinct cases: None means "the SDK caller supplied no map", {} means "capabilities were observed and declared no map". The spec's reject predicate is about the counterparty's document — security.mdx:1108 @ 3.1.8, "If the agent declares signing without a corresponding identity.key_origins.{purpose} entry, reject" — while the schema says of an absent map: "Absent means the operator has not declared a separation scheme; receivers SHOULD assume shared-origin." The pre-PR code drew that distinction and the deleted docstring documented the affordance.
The live consequence is on the webhook path. verify_webhook_signature builds its inner VerifyOptions with neither expected_key_origins nor signing_purpose, and WebhookVerifyOptions has no field for either:
resolver with jwks_source = "brand_json" (the spec's canonical webhook trust anchor):
PR head -> webhook_signature_key_origin_missing
"identity.key_origins.request_signing declaration missing"
main -> accepted
Every webhook is rejected, and the wire purpose says request_signing where 3.1.8 names webhook_signing for webhook delivery. Keep None and {} distinct, have verify_from_agent_url pass resolution.key_origins or {}, add expected_key_origins/posture to WebhookVerifyOptions, and pass signing_purpose="webhook_signing" from the webhook verifier. Cite security.mdx:1108 @ 3.1.8 on the rejecting branch.
| self, | ||
| *, | ||
| pool: Any, # psycopg_pool.AsyncConnectionPool — Any avoids runtime psycopg import | ||
| lock_pool: Any, |
There was a problem hiding this comment.
lock_pool has no default, so PgBackend(pool=pool) raises TypeError on upgrade — and docs/handler-authoring.md:632 still shows exactly that call. Update the doc in this PR. The reason for the second pool is an internal property of hold (it keeps a connection checked out while adopter code runs), which the backend could satisfy itself rather than making every adopter model it: default lock_pool=None and derive one, keeping the is pool rejection for an explicitly passed pool. Either way, is a required-kwarg break shipped under fix(...) the intent, or does this want !?
| raise ValueError(f"invalid signing origin: {url!s}") from exc | ||
| if port is None: | ||
| port = 443 if parsed.scheme.lower() == "https" else 80 | ||
| return parsed.scheme.lower(), parsed.hostname.lower(), port |
There was a problem hiding this comment.
parsed.hostname.lower() is not the repo's host canonicalization. _idna_canonicalize.py exists for this and its module docstring enumerates the callsites that canonicalize host strings for comparison; jwks.py, ip_pinned_transport.py, revocation_fetcher.py, key_origins.py, brand_jwks.py and etld.py all use it, and the two commits immediately before this PR (b7ef1dfc "canonicalize both origins in the brand.json jwks gate", 827e4f4e "idna-normalize hosts in etld+1 binding") fixed this same class of bug.
_origin("https://sellér.example.com") -> ('https', 'sellér.example.com', 443)
_origin(httpx.Request(...).url) -> ('https', 'xn--sellr-esa.example.com', 443)
match: False
An operator who passes a unicode expected_origin has every signed request refused, blamed on a redirect that never happened. Fail-closed, so not exploitable, but it is a silent misdiagnosed outage for a legitimate config. Route both sides through canonicalize_host, which already short-circuits IP literals.
Separately, the invariant this hook now enforces ("a hook signs for exactly one origin") is absent from ADCPClient._sign_outgoing_request (src/adcp/client.py:968-1049), the semantically equivalent hook the SDK's own transports install (client.py:581 → mcp.py:370,1020, a2a.py:263). Those paths pin follow_redirects=False so the capture vector is not live there today; the problem is two implementations of one hook. Build ADCPClient's through install_signing_event_hook(..., expected_origin=...) and delete the duplicate body.
| ) | ||
|
|
||
| DEFAULT_JWKS_COOLDOWN_SECONDS = 30.0 | ||
| DEFAULT_JWKS_MAX_AGE_SECONDS = 3600.0 |
There was a problem hiding this comment.
security.mdx:1457 @ 3.1.8: "JWKS cache TTL bounded above by the revocation-list polling interval (floor 1 min, ceiling 30 min)." 3600 s is 2× that ceiling and 4× the spec's own cacheMaxAge: 15 * 60 * 1000 example, so out of the box a compromised kid added to revoked_kids keeps verifying on the JWKS path for up to an hour. Introducing a max-age is the right call — :1457 also says verifiers MUST NOT pin one snapshot for a task's lifetime — the number is what needs fixing. Derive it from the configured revocation polling interval so the "bounded above by" relation holds by construction, and cite security.mdx:1457 @ 3.1.8 here.
|
|
||
| DEFAULT_MIN_COOLDOWN_SECONDS = 30.0 | ||
| DEFAULT_MAX_AGE_SECONDS = 3600.0 | ||
| DEFAULT_MAX_STALE_SECONDS = 3600.0 |
There was a problem hiding this comment.
This stacks on DEFAULT_MAX_AGE_SECONDS = 3600.0 for up to 7200 s of trust in an expired brand.json snapshot. security.mdx:1103 @ 3.1.8: "Cache TTL on a successful fetch MUST be bounded above by the JWKS revocation polling interval (so a key rotation cannot be masked by a stale brand.json)", with that interval capped at 30 min. Bounding stale-on-error is a real improvement over the pre-PR except BrandJsonResolverError: return snap; size max_age + max_stale against the deployment's polling interval and cite security.mdx:1103 @ 3.1.8 on both constants.
The fail-closed branches this constant governs are also unproven: all four mutate cleanly (unbounded stale-serve restored in resolve(), can_refresh re-anchored on snap.fetched_at, _sync_selector's clearing of _selected/_inner removed, _do_refresh's _last_error recording removed). test_authz_stale_on_error_is_bounded covers the authorization resolver; this one serves key material, and DEFAULT_MAX_STALE_SECONDS appears in no test.
| DEFAULT_REFERENCE_TIMEOUT_SECONDS = 5.0 | ||
| DEFAULT_REFERENCE_BODY_LIMIT_BYTES = 1024 * 1024 | ||
| DEFAULT_MAX_SCHEMA_REFS = 256 | ||
| DEFAULT_MAX_SCHEMA_IDS = 32 |
There was a problem hiding this comment.
The normative format_schema fetch contract (docs/creative/canonical-formats.mdx:217-231 @ 3.1.8) enumerates its bounds — $ref depth ≤ 8, total $ref count ≤ 256, compiled keyword count ≤ 10 000, 1 MiB streaming cap, ≤ 5 s timeout — and has no $id bound. A publisher schema with 200 $refs and 40 $ids satisfies every listed bound and is now INVALID_SCHEMA, which per :230 means "same as digest mismatch (unresolvable, surface via errors[], skip)" — buyer-visible loss of the product declaration, and two conformant SDKs disagreeing on the same digest-pinned document.
The DoS motivation is real: _validate_schema_id_value calls _resolve_public_https per $id. But $id must already be same-origin or the AAO catalog, so a document reaches at most two distinct hosts — memoize the host resolution per document rather than lowering the acceptance bar, or raise the default to the spec's own 256. Cite canonical-formats.mdx §format_schema fetch contract @ 3.1.8 next to the constant.
Also note this addition breaks the ASCII ordering of the __all__ list it joins at :724, which was sorted before this diff.
2 participants
Summary
Why
The security audit identified race conditions in replay/idempotency handling and trust-boundary gaps around remote signing metadata. Concurrent requests could pass non-atomic checks, while unbounded fetches and permissive key-origin behavior weakened failure containment.
Validation
origin/mainCompatibility
Digest verification is required by default, and missing brand
key_originsnow fails closed. Legacy replay/idempotency backends continue through warned process-local compatibility locks; deployments should migrate to atomic implementations for cross-process guarantees. PostgreSQL idempotency users must configure a distinct lock pool.