fix(identity): enforce tenant and credential isolation - #1001
Conversation
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """The request host cannot replace the tenant authenticated by the token.""" | ||
| import src.app as app_module |
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """An account id owned by another buyer is indistinguishable from missing.""" | ||
| import src.platform as platform_module |
| async def test_account_store_upsert_cannot_overwrite_another_buyers_account( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| import src.platform as platform_module |
| import sqlalchemy as sa | ||
| from alembic import context, op | ||
|
|
||
| revision: str = "0003" |
| from alembic import context, op | ||
|
|
||
| revision: str = "0003" | ||
| down_revision: str | None = "0002" |
|
|
||
| revision: str = "0003" | ||
| down_revision: str | None = "0002" | ||
| branch_labels: str | Sequence[str] | None = None |
| revision: str = "0003" | ||
| down_revision: str | None = "0002" | ||
| branch_labels: str | Sequence[str] | None = None | ||
| depends_on: str | Sequence[str] | None = None |
There was a problem hiding this comment.
Solid audit fix — the isolation work is correct and well-tested. One blocker, and it's the semver signal, not the code: a required-argument change to a public export is shipping under fix: without a breaking marker.
MUST FIX (blocking)
Breaking public-API change without the semver signal. create_roster_account_store is a public export (src/adcp/decisioning/__init__.py:455). This PR adds a required authorize argument (src/adcp/decisioning/roster_store.py:299), and your own test test_omitted_authorization_callback_fails_at_construction asserts the old call shape now raises TypeError. Failure mode in one sentence: any adopter calling create_roster_account_store(roster=...) — the documented signature until this PR — gets TypeError: missing 1 required keyword-only argument: 'authorize' after upgrading within what release-please will cut as a fix: patch.
The change itself is right — roster membership is not authorization, and failing closed at construction beats a silent fail-open. The block is purely the conventional-commit signal. release-please reads the commit/PR title (fix(identity): enforce tenant and credential isolation), not the PR body's Compatibility section. Retitle to fix(identity)!: or add a BREAKING CHANGE: footer naming the roster-store constructor change, so this cuts a major bump instead of a patch. The migration note in the PR body is already good — it just needs to be where release-please can see it.
Things I checked
code-reviewer: proposal_store composite-key migration ((account_id, proposal_id)) is consistent across every read/write/evict/index path includingget_by_media_buy_idand_record_key's ambiguous-unscoped resolver. No bare-proposal_id lookup survives.code-reviewer:registry_cache.pycharges the aggregate bucket before the per-lookup bucket (_spend_locked), so rotating lookup keys can't bypass the tenant cap; prune-then-spend is behavior-preserving since a re-created bucket starts at full burst.code-reviewer: bounded LRU client cache inserts-then-evicts (platform.py:621-624), never evicting the just-created client;_close_evicted_upstream_clienthandles the no-running-loop case.security-reviewer: sound, no High. Credential redaction closes both the loose-dict and the Pydantic-model path inresponses.py(_serializenow dumps then scrubs), confirmed bytest_response_builder_scrubs_notification_credentials_from_pydantic_models. Idempotency replay cache dumps-then-scrubs atdispatch.py:1855.security-reviewer: MCP session binding (server/auth.py) is enforced by a real ASGI integration test — a second valid bearer replaying alice'sMcp-Session-Idgets 404, alice's own bearer gets 200.ad-tech-protocol-expert: sound-with-caveats.notification_config.jsonmarksAuthentication.credentialswrite-only (same class asBusinessEntity.bank), so stripping on the response path is spec-correct.schemes: min_length=1 max_length=1matches the generated model exactly.ad-tech-protocol-expert: cross-tenantsync_accounts→ACCOUNT_NOT_FOUNDis the spec-correct existence-hiding code, aligningupsertwith the pre-existingresolvebehavior. Both codes are terminal, so conforming buyers' retry logic is unaffected — not a breaking wire change.- Migration
0003preflightsHAVING COUNT(*) > 1and fails closed before touching indexes; creates the unique index before dropping the old one so a duplicate rolls back. RuntimeLIMIT 2+fetchalldefense-in-depth agrees with the DB guarantee. upstream.py:_project_statusno longer takesbody_text; the only other error path interpolates aJSONDecodeErrorposition string, not the body.test_error_response_body_is_not_exposedcovers it.
Follow-ups (non-blocking — file as issues)
- Timing-oracle equalization for the new
ACCOUNT_NOT_FOUNDbranch.ad-tech-protocol-expertflagged that cross-tenant rejection moved out of thePERMISSION_DENIEDbranch that_permission_denied_budget.pytiming-clamps. The cross-tenant path does more work (resolve succeeds, then tenant compare fails) than the unknown-ref path (earlyNone). Confirm the clamp covers the cross-tenantACCOUNT_NOT_FOUNDbranch, or existence-hiding leaks through timing. - Raw bearer in
scope['user'].server/auth.pysetsAccessToken(token=bearer, ...)while the adjacent comment claims only derived context is stored. No round-trip today (the framework readsrequest.state, keepscredential=None), but a logging middleware serializingscope['user']would surface the plaintext token. Pass a hash/opaque id since only the derived identity drives binding. - Unguarded
mcpimport on the authenticated path. Thefrom mcp.server.auth...imports inserver/auth.pyrun per-request outside any try/except; an mcp version drift turns every authenticated request into a 500. Import once at module load or guard it. - Discovery + stale bearer now 401s. The
and not bearernarrowing means a client attaching an expired token toget_adcp_capabilities/initializenow gets 401 instead of the pre-auth discovery response. More correct, but a behavioral change worth a release note for buyers that token every request. - Reference-seller order ownership keyed on
advertiser_id, creatives on_account_scope. Two different isolation identities inexamples/.../platform.py. Safe only if(network_code, advertiser_id)is 1:1 with an account under the seeding contract — worth a one-line justification in the file.
Minor nits (non-blocking)
- Stale rationale comment.
account_projection.py:457-467still saysstrip_credentials_from_wire_resultpasses Pydantic models through because "response-side codegen shapes don't define authentication/bank." The base model now structurally carriesnotification_configs[i].authentication.credentials— that's whyprojections.pyneeded_NotificationConfigResponse. The load-bearing safety is the handler-layermodel_dump, not schema structure; point the comment there. SchemaVariantonlistdropsmax_length=16. Perad-tech-protocol-expert, the marker is correctly required here (invariantlist[...]would otherwise be a Liskov violation), but the runtime collapse silently drops the base field's length constraint on the response projection. Harmless on a read edge — worth a one-line comment.
Fix the commit prefix and this is a clean ship. Everything else is a follow-up.
KonstantinMirin
left a comment
There was a problem hiding this comment.
Review — PR #1001
Overview — Same Codex security-CLI shape as #999: the mechanisms hold in the direction they were written for, and the failures are all in the directions the generator did not model. Holding: a second principal cannot replay another's Mcp-Session-Id, the proposal composite key (account_id, proposal_id) has no surviving bare-proposal_id write path, and _serialize dumping before scrubbing genuinely closes the typed-model path at the response-builder boundary.
Three shapes, each reproduced at head:
- The guard sits where the current request's credential is visible, not where the fact belongs. Session ownership is derived from whether this request carried a credential rather than from who created the session — so it 404s the session's own owner on the documented pre-auth handshake and simultaneously fails open for an anonymous caller on an unowned session. Both cells are 200 before this PR (1).
- Isolation built on a process-global resource under a single budget. One tenant probing at exactly its permitted rate, with zero denials for itself, denies every other tenant's first-ever lookup with
PERMISSION_DENIED/correctable— a seller-side capacity condition wearing an authorization code (2). - One spec fact hand-copied into three walkers, the new rule landed on one. The dispatcher's model arm returns typed models unchanged and its justifying comment is checkable and false —
"bank"is inBusinessEntity.model_fields;responses.pypasses nested models through and is missing the authorization rule, sosync_accounts_responseechoesoauth.access_token(4).
As on #999, coverage is what let all three through: deleting the cross-buyer WHERE predicate, the creative owner gate, or the whole AUTH_REQUIRED block each leaves the example suite at 59 passed. Seven mutations survive in total. Fix that first and the rest stop being invisible.
Still open from 2026-07-29 — lead item
Breaking public-API change shipping as a plain fix:. Raised in the aao-ipr-bot CHANGES_REQUESTED review on 2026-07-29, not addressed on this push. git log -1 --format=%B at head 27a79980 is exactly fix(identity): enforce tenant and credential isolation — no !, no BREAKING CHANGE: footer. create_roster_account_store is a public export (src/adcp/decisioning/__init__.py:455), authorize is now a required keyword-only argument (src/adcp/decisioning/roster_store.py:296-300), and the diff's own test_omitted_authorization_callback_fails_at_construction asserts the old call shape raises TypeError. release-please reads the title, so this cuts a patch. Retitle to fix(identity)!: or add the footer naming the roster-store constructor change.
Should fix
1. Session ownership is derived from whether the request carried a credential, not from who created the session.
src/adcp/server/auth.py:509 mirrors the validated principal into request.scope["user"] inside the bearer-validated branch only. src/adcp/server/auth.py:441 returns early on the discovery bypass with scope["user"] unset, so mcp_sessions._handle_stateful_request computes requestor = None, and ownership is recorded only if requestor is not None (src/adcp/server/mcp_sessions.py:200-201). initialize, notifications/initialized and tools/list are all in DISCOVERY_METHODS (src/adcp/server/mcp_tools.py:1141-1143) and get_adcp_capabilities is in DISCOVERY_TOOLS.
Measured at head; every cell on the second row is 200 before this PR:
[authed-initialize] tools/list as alice(owner) -> 200 as bob(other) -> 404 anonymous -> 404
[pre-auth-initialize] tools/list as alice(owner) -> 404 as bob(other) -> 404 anonymous -> 200
Two defects on one line. A buyer that follows the documented AdCP entry point (connect, read get_adcp_capabilities, then authenticate) is locked out of the session it just created, permanently and indistinguishably from an expired session. And the session created that way has no owner, so any anonymous caller who learns the session id can drive it — the binding fails open for exactly the sessions the middleware did not authenticate. allow_unauthenticated=True deployments land in the same bucket, since that branch also returns before line 509.
Root cause is the identity model, not the line: ownership is being read off the current request's credential when it is a property of the session. Establish the owner once where the session is created — ADCPStreamableHTTPSessionManager._handle_stateful_request, from the request state the framework already threads via _read_request_state_auth — and make an unowned session fail closed instead of matching any caller (requestor is None must not equal _session_owners.get(...) is None). The MCP-shaped AuthenticatedUser/AccessToken construction belongs in mcp_sessions.py, which already imports both at module level, rather than as a function-local import in the transport-agnostic bearer middleware whose A2ABearerAuthMiddleware sibling publishes a different scope["user"] shape (auth.py:1453). The coverage to add is the mixed-auth matrix above, not another replay case; the rejection half of the and not bearer narrowing is also untested (tests/test_auth_middleware.py:245, :270, :293 all send discovery requests with no Authorization header), and the 401 it now returns for discovery-plus-stale-token carries no code and no recovery where AdCP 3.1.8 names AUTH_INVALID/terminal.
2. The new rate-limit state is one table serving three different budgets.
self._buckets is a single OrderedDict holding both (tenant_id, lookup_key) and (tenant_id, "__tenant_aggregate__") entries, capped globally by max_buckets, and both tiers are built from the same self._rate/self._burst. Four consequences, all measured at head with shipped defaults:
- Global cap, per-tenant budget.
src/adcp/decisioning/registry_cache.py:589returnsFalsefor any missing key once the table is full — including a brand-new tenant's aggregate bucket, created on that tenant's first lookup. A tenant probing freshagent_urls at exactly its permitted 100 rps fills all 10,000 slots in 100 simulated seconds with zero denials for itself, after which an unrelated tenant's first-ever lookup raisesPERMISSION_DENIED._prune_idlecannot relieve it: it returns at the first bucket younger than the 300 s TTL, and cycling 9,999 keys at 100 rps keeps every one of them warm. No attacker needed either — the footprint issum over tenants of (1 aggregate + live lookup keys), so 100 tenants × 100 buyer agents reaches the cap. Both recommended stacks wire the default (examples/v3_reference_seller/src/buyer_registry.py:173,PgBuyerAgentRegistry.with_hardeningatsrc/adcp/decisioning/pg/buyer_agent_registry.py:470). - Shared fate inside a tenant.
:566-572charges the aggregate before the per-lookup bucket, andresolve_by_agent_url/resolve_by_credentialare reachable before the caller is authorised — that is the premise of the class's credential-stuffing docstring. Measured:attacker denied after 100 junk probes, thenlegit buyer with untouched per-key bucket: DENIED PERMISSION_DENIED / correctable. On main, junk probes drained only their own buckets. This diff deletestest_rate_limit_isolates_distinct_lookup_keys, the test that pinned that property, and replaces it with one asserting the new shared-fate behavior. - The per-lookup tier can never bind. The aggregate is charged on every lookup and the per-key bucket on a subset, with identical rate and burst, so
tokens_aggregate <= tokens_keyalways holds. Replacingexhausted = not self._spend_locked(key, now)withpassleaves 25 of 27 tests intests/test_buyer_agent_registry_cache.pygreen; the two failures assertlen(limiter._buckets) == N, not a rate-limit outcome. _prune_idlerestores full burst. The docstring's safety argument at:490-491("Discarding an idle bucket is safe because it would have refilled to its full burst") holds only whenburst / rps_per_tenant <= bucket_idle_ttl_seconds.__init__validates each of the three values in isolation (:517-526) and never the relation, while the class docstring invites the violating config ("Adopters with bursty real traffic raise this"). Measured withrps=1.0, burst=1000.0, ttl=300:drained after 1000 requests, thenafter 301s idle, allowed: 1000where honest refill allows 301. It applies to the aggregate bucket too, so the whole tenant budget resets.
One root: enumeration control, per-tenant fairness and the memory bound are three different budgets sharing one table, one rate/burst pair and one cap. Split them — keep the aggregate bucket in a map the cap never touches so a new tenant can always allocate, cap lookup buckets per tenant and evict that tenant's own LRU rather than refusing allocation, give the per-lookup tier its own tighter rate/burst so it can bind (or drop the tier), and validate burst / rps_per_tenant <= bucket_idle_ttl_seconds at construction. Then decide the wire code: the victim currently gets PERMISSION_DENIED / recovery="correctable" — "you are not authorized, fix your request" — for a seller-side capacity condition that AdCP 3.1.8 classifies transient (RATE_LIMITED with retry_after, or SERVICE_UNAVAILABLE, per schemas/cache/3.1/enums/error-code.json@3.1.8). The wire-uniformity rationale at :122-130 covers a caller who was rate-limited; it does not cover one who never was. Tests must assert admitted-request counts, not len(_buckets), and both new tests use a single tenant.
3. The write-only credential strip is three hand-copied walkers, and each is missing rules the others have.
Four measured gaps, all at head:
strip_credentials_from_wire_result('list_accounts', Account(...)) model arm leaks: True
strip_credentials_from_wire_result('list_accounts', {'accounts': [Account(...)]}) leaks: True
sync_accounts_response([{... 'notification_configs': [NotificationConfig(...)]}]) leaks: True
builder path leaks authorization.oauth.access_token: True internal_connection_id: True
src/adcp/decisioning/account_projection.py:469-485returns Pydantic models unchanged, so_invoke_platform_method's return (src/adcp/decisioning/dispatch.py:1596),task_registry.complete(:472) andwebhook_emit(:302,:479) hand the adopter's typed model straight to the transport, which dumps it verbatim (mcp_tools.py:2589,a2a_server.py:499,serve.py:2727). The justifying comment says the response-side codegen shapes "don't defineauthenticationonGovernanceAgentorbankon the response-sideBusinessEntity, so the schema enforces the strip structurally". That is false, and this diff is what proves it:'bank' in BusinessEntity.model_fieldsisTrue, andprojections.pyneeded a whole_NotificationConfigResponseprecisely because the base model carriesnotification_configs[].authentication.credentials.src/adcp/server/responses.py:158appends any non-dict config untouched, so a loose dict holding typed sub-objects — the{**db_record, "notification_configs": [NotificationConfig(...)]}shape the module exists to defend against — still echoes the secret._serializedumps only the top-level item. Every rule in that walker has the same hole, because there is no model-normalisation step at recursion entry.- The
responses.pywalker is missing theauthorizationand error-detailsrules thataccount_projection._scrub_dicthas, sosync_accounts_response()— the function this PR just declared a security boundary — echoesauthorization.oauth.access_tokenandinternal_connection_id. - Both walkers key on the literal parent field name.
invoice_recipient.bankis the most frequentlywriteOnly: true-marked path in the bundled schemas (52 occurrences underschemas/cache/*/bundled/**at 3.1.8) and is stripped by neither.
Root: one spec fact is encoded as a hand-written literal-key walker in three modules (account_projection, server/responses, types/projections), derived from nobody's schema, so the next write-only field gets added to one or two of them. That is how notification_configs came in from an external audit rather than from this repo. Make account_projection the single owner — it already has the complete rule set — normalise models once at recursion entry (if hasattr(value, "model_dump"): value = value.model_dump(mode="json")) and delete the per-branch non-dict passthroughs, and have responses._serialize call the owner rather than keep a second copy. adcp.server already imports adcp.decisioning at a dozen sites, so the direction introduces no cycle. Then derive the strip set from writeOnly: true in the bundled schemas, or add a conformance test that walks schemas/cache/*/bundled/** for writeOnly paths and asserts both scrubbers remove each one — an empty-allowlist ratchet in the style of KNOWN_VERIFIER_GAPS, which turns this class of bug into a build failure. The prose half needs the same edit: account_projection.py:450-452 and docs/handler-authoring.md:906-909 still enumerate the old two-field list, and docs/handler-authoring.md:912-915 points adopters at create_roster_account_store without mentioning the now-required authorize argument.
4. The reference seller now has three isolation identities, and the media-buy gate uses the one the repo's own seed data defeats.
examples/v3_reference_seller/src/platform.py:887 compares order["advertiser_id"] against ctx.account.metadata["advertiser_id"]. examples/v3_reference_seller/seed.py:117-142 seeds a_acme_1 (buyer agent ba_acme_signed) and a_acme_2 (ba_acme_bearer) with the same ext = {"network_code": "net_premium_us", "advertiser_id": "adv_volta_motors"}. Two buyer agents, one advertiser id, so the check passes straight across the account boundary the rest of this PR spends 1400 lines enforcing. The same gate guards update_media_buy (:1394), get_media_buy_delivery (:1685 — reads another buyer's spend and impressions) and provide_performance_feedback (:1912 — writes conversions onto another buyer's campaign). test_update_media_buy_rejects_foreign_advertiser_order only exercises a different advertiser id.
Meanwhile the stronger identity exists in the same class: create_media_buy records _account_scope(ctx) onto _buy_state at :1110-1112, and _creative_id_map/_creative_id_reverse were correctly re-keyed to (account_scope, id). _buy_state was not — the scope lives in the value, checked at one of eight read sites (:1625; :1331, :1422, :1499, :1640, :1704, :1791, :1819 are unscoped). Counting AccountRow.buyer_agent_id, the column this same PR starts enforcing in resolve/upsert, there are three owner identities in one class and no accessor owns "who owns this".
Pick one identity — buyer_agent_id is what the DB enforces — and put it in the key, so no reader can forget the check. Derive it once from a typed metadata model instead of dict[str, Any] lookups; Account is generic in TMeta, so a TypedDict with required tenant_id / account_id / advertiser_id / network_code / buyer_agent_id turns roughly ten metadata["..."] / .get(...) or "" sites into checked attribute access and removes _account_scope's silent empty-tenant fallback at :871. Two more edges in the same helper: an absent advertiser_id on the upstream GET /orders/{id} is indistinguishable from a foreign order (order.get(...) != expected), so upstream schema drift becomes a silent total MEDIA_BUY_NOT_FOUND outage that looks spec-correct — an absent field should be an operator-visible SERVICE_UNAVAILABLE. And the comment at :888 claims foreign and nonexistent ids are indistinguishable, but the two envelopes differ:
FOREIGN: {'code':'MEDIA_BUY_NOT_FOUND','message':"Media buy 'ord_foreign' was not found.",'recovery':'terminal','field':'media_buy_id'}
MISSING: {'code':'MEDIA_BUY_NOT_FOUND','message':'upstream GET /v1/orders/ord_missing failed: 404','recovery':'terminal'}
AdCP 3.1.8's *_NOT_FOUND uniform-response rule requires the response shape itself not be the oracle (schemas/cache/3.1/enums/error-code.json@3.1.8, and PERMISSION_DENIED extends it to "every observable channel (response shape, HTTP/A2A/MCP status, headers, side effects, observability, latency parity)"). Route both arms through one constructor and assert foreign.to_wire() == missing.to_wire(); the three new _get_owned_order tests assert only excinfo.value.code, so the invariant is ungraded.
5. The cross-tenant guard raises from context_factory, a hook no transport projects — and the rule belongs in the framework.
examples/v3_reference_seller/src/app.py:113 raises AdcpError("PERMISSION_DENIED") from inside the context factory. src/adcp/server/serve.py:2656 calls context_factory(meta) above the try whose except ADCPError at :2682 builds the conformant envelope, and src/adcp/server/a2a_server.py:302 has the same shape. Measured on MCP:
{"result":{"content":[{"text":"Error executing tool get_products: AdcpError[PERMISSION_DENIED / terminal]: Bearer credential is not valid for this tenant.","type":"text"}],"isError":true}}No adcp_error, no code, no recovery — a conforming buyer sees an untyped tool failure, and the exception repr is what lands in the tool text, for a condition any holder of a valid token can trigger by changing the request host. On A2A the same raise escapes execute() and surfaces as a JSON-RPC transport error rather than a failed task with an adcp_error DataPart: one rule, two non-conformant and mutually different envelopes.
Second defect on the same line: the guard is if ctx.tenant_id is not None and ctx.tenant_id != tenant.id. Principal.tenant_id defaults to None and src/adcp/server/auth.py:148-152 documents leaving it unset as a supported configuration, so those deployments still silently rebind the credential to whatever tenant the Host names — the thing the new comment says it prevents. Fail closed when a host tenant was resolved and the token carries none.
Root: the rule is framework-shaped and the framework neither enforces nor offers it. auth_context_factory sets ToolContext.tenant_id from the validated bearer; the documented multi-tenant composition then overwrites it with the Host-derived tenant and never compares the two. That recipe is printed twice in framework docstrings — src/adcp/server/tenant_router.py:55-61 and TenantRegistry.as_platform at src/adcp/server/tenant_registry.py:770-776 ("wire tenant_id in your context_factory like this: t = current_tenant(); return {"tenant_id": t.id if t else None}") — so every adopter who follows it lets a bearer issued for tenant A act as tenant B. Both identities are visible at one point: the auth ContextVar holds the token's tenant, tenant_router.current_tenant() holds the host-resolved one. Do the comparison there (in auth_context_factory, or a subdomain_auth_context_factory), update both docstring recipes, and delete the bespoke check from the example — which also gets it a conformant wire error for free. If the factory hook is meant to be allowed to reject, that is a framework gap: move the context_factory(meta) call inside the existing try in both serve.py and a2a_server.py, because the hook has no error-translation contract today. test_bearer_context_rejects_cross_tenant_rebinding calls _build_context_factory() directly and asserts only excinfo.value.code, so the wire shape is uncovered, as are both allow paths the same edit governs (ctx.tenant_id is None must still pin from the subdomain; ctx.tenant_id == tenant.id must pin, not reject).
6. Existence-hiding landed in upsert and not in sync_governance, whose docstring says "same rules".
src/adcp/decisioning/tenant_store.py:391 splits the combined branch into auth_tid is None → PERMISSION_DENIED / auth_tid != entry_tid → ACCOUNT_NOT_FOUND. sync_governance, twelve methods down at :494, still runs if auth_tid is None or auth_tid != entry_tid: PERMISSION_DENIED, and its message discloses the reason outright. Same store, same cross-tenant AccountReference, both arms:
upsert cross-tenant : {'code': 'ACCOUNT_NOT_FOUND', 'message': 'Unknown operator: pinnacle.example'}
sync_governance cross-tenant: {'code': 'PERMISSION_DENIED', 'message': "Buyer agent has no authority over 'pinnacle.example' (tenant mismatch or auth principal not registered)."}
So the enumeration oracle upsert just closed is open one method down: a buyer probes {brand, operator} refs and reads PERMISSION_DENIED ("exists under another tenant") against ACCOUNT_NOT_FOUND ("no such account"). AdCP 3.1.8's sync-governance-response.json illustrates existence-hiding on exactly this surface, with the failed-row example using ACCOUNT_NOT_FOUND and the message "Account 'acct-unknown' does not exist or is not accessible to the authenticated agent."
Root: the auth-tenant/entry-tenant comparison and the failed-row construction are copy-pasted per method, so fixing one arm structurally cannot fix the other. Extract the ref → (entry_account, code, message) classification into one private method and have both upsert and sync_governance build their row shapes from it; cite schemas/cache/3.1/account/sync-governance-response.json@3.1.8 at the shared decision site. Four prose sites now assert the old rule — tenant_store.py:6-8, :20-21, sync_governance's own docstring at :445-447, and :10-14, which claims the security semantics mirror the JS createTenantStore unchanged. Since error-code strings are the normative wire surface, is the JS store moving to ACCOUNT_NOT_FOUND too? A one-sided change makes the two SDKs answer the same probe differently. On the test side, tests/test_tenant_store.py:1-8 still documents PERMISSION_DENIED, :307-310 still states in prose that ACCOUNT_NOT_FOUND is "distinct from PERMISSION_DENIED ('ref valid but you're not authorized')" — the distinction the change deletes — :361-364 says the entry surfaces "as PERMISSION_DENIED" while its assertion was flipped, and the changed test at :298-305 lost its docstring, so a reader cannot tell deliberate existence-hiding from a regression.
7. Every isolation guarantee that lives in a SQL WHERE clause or a composite dict key is asserted through a mock that answers the same either way.
The example suite stubs session.execute with AsyncMock(side_effect=[...]), so a query's predicate is unobservable. Verified by deletion, each against pytest examples/v3_reference_seller/tests/:
- delete
AccountRow.buyer_agent_id == buyer_agent.id(examples/v3_reference_seller/src/platform.py:321) →59 passed, 2 deselected. The cross-buyer account read path reopens with a green suite.test_account_store_explicit_id_is_bound_to_authenticated_buyerproves only thatresolvenow issues two queries; the sibling predicate on the brand-shaped path at:347has the same exposure. - replace the assignment resolver's
state.get("account_scope") == self._account_scope(ctx)(:1626) with== "NEVER_MATCHES"→59 passed. Same for pointing thelist_creativesreverse lookup (:2013) at a literal"WRONG_SCOPE".test_creative_id_mapping_is_scoped_to_accountassertsroute.call_count == 2, so nothing checks that the read side reconstructs the key the write side stored — and a mismatch does not raise, it falls back to the upstream id, so the buyer silently receives upstream creative ids instead of their own andattach_creativefires with the wrong id. - delete the whole
if not principal: raise AdcpError("AUTH_REQUIRED", ...)block (:293-298) →59 passed. Both halves of that change are unasserted: that an unauthenticated resolve is refused at all, and that a buyer who previously gotACCOUNT_NOT_FOUND/correctable/field="account.account_id"(with a message telling them what to send) now gets a terminal code.
The shape to copy is already in the diff: test_account_store_upsert_cannot_overwrite_another_buyers_account uses a real AccountRow for the foreign row and asserts foreign.buyer_agent_id == "ba_other" afterwards, so it catches both the missing rejection and a partial write. Give the resolve path the same treatment — a real sqlite/asyncpg session with two AccountRows in tenant t_acme, one owned by ba_caller and one by ba_other — and the same fixture carries the AUTH_REQUIRED cases at no extra setup cost. Add one same-account creative round trip (sync_creatives with buyer id shared-id, then list_creatives asserting creatives[0]["creative_id"] == "shared-id") plus the cross-account negative.
Three more coverage items in the same diff. test_provide_performance_feedback_404_translates_to_media_buy_not_found (examples/v3_reference_seller/tests/test_smoke_broadening.py:1451) had its mocked 404 moved from POST /v1/orders/ord_missing/conversions to GET /v1/orders/ord_missing, so it now exercises _get_owned_order while its name and docstring still describe the POST-404 projection — which is covered by nothing, though the POST can still 404 in production. Removing the delivery fallback (examples/v3_reference_seller/src/platform.py:1683-1701) changed a wire shape with no test on either side: the old code emitted a well-formed active row when the delivery row existed but the order was gone, and the new code omits the row entirely. And migration 0003's load-bearing claim is an ordering claim ("Create first so a concurrent/legacy duplicate makes the migration fail while the old lookup index remains available") that no test asserts — all three tests in test_unique_api_key_migration.py patch op.create_index/op.drop_index and check create_index.call_args.kwargs["unique"] is True, leaving the index name, table, column list and partial-WHERE predicate unasserted.
8. The upstream-client cache became the owner of pools every caller borrows, and its drain method has no callers.
src/adcp/decisioning/platform.py:622-624 evicts and closes. upstream_for hands adopters a raw client with no lifetime contract, and the cache key is id(auth) "so different DynamicBearer closures for different tenants need distinct clients" (:571-574) — so a platform with more than 128 tenants and concurrent traffic closes a connection pool another coroutine is mid-request on, and round-robin over more than 128 identities also thrashes the cache into never reusing a pool, which inverts the stated purpose. test_bounded_cache_closes_evicted_client does not catch it because the failure needs a real socket; under respx the in-flight request survives.
Three defects in the same lifecycle gap:
loop.create_task(client.aclose())at:638discards the task. asyncio holds only a weak reference, so it can be collected mid-close and an exception inacloseis never retrieved. The repo's ruff select is["E","F","I","N","W","UP"], soRUF006will not flag it. Retain it in a set withadd_done_callback(discard)._upstream_clients_pending_close(:632-636) is drained only byaclose_upstream_clients, andgrep -rn "aclose_upstream_clients" src/ tests/ docs/ examples/returns its own definition and nothing else — no lifespan wiring, no docs, no test. On the no-running-loop branch the list grows and no pool is ever closed, so the net effect of the new bound in a sync context is that the cache stops growing and the sockets leak instead.- The public
upstream_fordocstring at:485-486still says clients are "cached per-platform-instance keyed by(base_url, id(auth))", with no mention of the bound or that eviction closes the pool. Adopters read that one, not the private helper's.
Fix the ownership rather than the symptom: extract the keyed pool into an UpstreamClientPool in adcp/decisioning/upstream.py (which owns the pool) with get(...) and aclose(), hold it as one attribute instead of three ad-hoc ones on the platform mixin, and wire its aclose() into the framework shutdown path so the no-loop deferral has a real drain point instead of an adopter obligation nobody is told about. Coverage is short too: the deferral branch, aclose_upstream_clients, the upstream_client_cache_size < 1 ValueError (which raises at first-request time rather than at construction) and the cache.move_to_end(key) recency behavior are all untested.
9. New raise sites hardcode recovery against the pinned enumMetadata, and one emits a value that is not in the enum.
schemas/cache/3.1/enums/error-code.json#enumMetadata @ 3.1.8: MEDIA_BUY_NOT_FOUND: correctable, PERMISSION_DENIED: correctable, AUTH_REQUIRED: correctable, ACCOUNT_NOT_FOUND: terminal. schemas/cache/3.1/core/error.json calls recovery the normative carrier of recovery semantics and enumerates it as ["transient","correctable","terminal"]. Every new site hand-rolls AdcpError(code, ..., recovery="terminal"):
examples/v3_reference_seller/src/platform.py:295—AUTH_REQUIRED/terminal. Two problems: the recovery contradictsenumMetadata, and 3.1.8 marks the code itself Deprecated ("useAUTH_MISSING(no credentials presented) orAUTH_INVALID(credentials presented and rejected)"). The condition here is exactly "no credentials presented", so emitAUTH_MISSINGwithrecovery="correctable"and cite the schema at the raise site.examples/v3_reference_seller/src/platform.py:892—MEDIA_BUY_NOT_FOUND/terminalwhere the pinned enum says correctable.examples/v3_reference_seller/src/app.py:117—PERMISSION_DENIED/terminal. The enum classifies it correctable, and terminal applies only whendetails.reasonis present, which this site does not emit. For a rejected credential the spec-consistent pair isAUTH_INVALID/terminal.src/adcp/decisioning/upstream.py:146—recovery="retry_with_changes"reachesto_wire()verbatim with no normalisation step, so theINVALID_REQUESTbranch emits a value outside the pinned enum and a buyer dispatching onrecoveryfalls off the switch.AdcpError's own docstring calls it a legacy alias forcorrectable. This line is not itself inside a hunk, but the enclosing_project_statussignature and every returned message are rewritten here (:103-147), so it is a one-token fix in the same edit.src/adcp/decisioning/tenant_store.py's_build_failed_sync_accounts_rowemitserrors=[{"code","message"}]with norecoveryat all (src/adcp/decisioning/account_projection.py:299-300), so the flip in finding 6 moves a conforming buyer from retry-after-fix to escalate-to-human with the spec's authoritative carrier absent from the row.
Root: these sites reach for the base AdcpError when the repo already owns the abstraction. src/adcp/decisioning/errors.py binds each code to its enumMetadata recovery — verified: MediaBuyNotFoundError → correctable, PermissionDeniedError → correctable, AuthRequiredError → correctable, AccountNotFoundError → terminal — and its module docstring says "Recovery values are normative — they MUST match the enumMetadata block." Raise the typed subclasses, thread recovery through _build_failed_sync_accounts_row from enumMetadata, and fix the three _project_status branches in the same pass.
10. The credential-uniqueness ratchet landed at one of three lookup implementations.
src/adcp/decisioning/pg/buyer_agent_registry.py:245 and src/adcp/decisioning/pg/buyer_agent_registry.sql:64 both now emit CREATE UNIQUE INDEX IF NOT EXISTS ..._api_key_id_uidx. Against an existing deployment that aborts the whole create_schema() DDL batch with a raw UniqueViolation and none of the "rotate or remove duplicate bearer credentials, then rerun" guidance examples/v3_reference_seller/alembic/versions/0003_unique_buyer_api_key.py:46-50 carefully provides. It also orphans the legacy non-unique ..._api_key_id_idx forever, because the index name changed and nothing drops the old one — while the docstring two lines above still says "Idempotent via CREATE ... IF NOT EXISTS; safe to call on every app boot." Give create_schema() the same duplicate preflight, drop the superseded index, and fix the docstring together.
Same window, third implementation: the diff adds LIMIT 2 + fetchall + fail-closed-with-log to PgBuyerAgentRegistry._sync_lookup_by_api_key_id explicitly "for deployments that have not yet applied the unique-index migration", but the reference seller ships that same pre-migration window and its resolve_by_credential still does scalar_one_or_none() (examples/v3_reference_seller/src/buyer_registry.py:108), which raises SQLAlchemy MultipleResultsFound on a legacy duplicate rather than denying. Its where clause is tenant-scoped, so pre-migration a duplicated api_key_id resolves to whichever tenant's host the request arrived on — the exact ambiguity this PR closes. Apply the same posture there.
Related, same file: upsert now lets psycopg.errors.UniqueViolation escape the abstraction, and tests/conformance/decisioning/test_pg_buyer_agent_registry.py::test_upsert_rejects_duplicate_credential_identifier pins that raw driver exception as the contract. Translate it into a typed AdcpError ("credential already bound to another buyer agent") so admin callers are not pattern-matching psycopg types.
11. The roster authorization callback fails closed with no signal, and three of its branches have no test.
src/adcp/decisioning/roster_store.py:145 wraps the adopter callback in except Exception: return False and grep -c logg src/adcp/decisioning/roster_store.py is 0 — the module imports no logger. Failing closed is right; failing closed invisibly is not. A callback that raises on every call (bad attribute name, dead DB handle) turns resolve into a framework-projected ACCOUNT_NOT_FOUND and list into a permanently empty array, which is a spec-valid response either way, with nothing separating "policy denied" from "policy is broken". The convention is already set in two places, one of them in this PR: BearerTokenAuthMiddleware does logger.exception("token validator raised") before failing closed (src/adcp/server/auth.py:472), and pg/buyer_agent_registry.py:518-524 logs at error when it fails closed on an ambiguous credential. Add a module logger and logger.exception("roster authorize callback raised; denying").
Three documented behaviors have no test (grep finds no async def authorizer, no raising authorizer, and no other create_roster_account_store call site in tests/, src/ or examples/): except Exception: return False; inspect.isawaitable(result); and result is True strictness, which denies a truthy non-bool. The awaitable branch is the dangerous one — if it regresses, an async authorizer returns a coroutine, result is True is False, and every account is silently denied for every async adopter, a total outage that fails closed and so emits nothing.
12. AccountResponse widens the schema it projects, and the new auth model hand-copies codegen output.
Account REJECTED 20 notification_configs
AccountResponse ACCEPTED 20 (max_length=16 dropped)
src/adcp/types/projections.py:208's SchemaVariant[list[_NotificationConfigResponse] | None] replaces the base field and loses its MaxLen(16), which mirrors schemas/cache/3.1/core/account.json's maxItems: 16 at 3.1.8. to_account_response is on the emit path, so a seller building through the class whose job is to be the safe response shape can emit a body that fails buyer-side validation. Carry the bound through: SchemaVariant[Annotated[list[_NotificationConfigResponse], Field(max_length=16)] | None], with a test asserting AccountResponse rejects 17. Raised on 2026-07-29, still open.
Second half at :169-174: _NotificationAuthenticationResponse subclasses AdCPBaseModel and re-declares schemes: list[AuthenticationScheme] = Field(min_length=1, max_length=1) by hand, while adcp.types.NotificationAuthentication is the exact generated model NotificationConfig.authentication uses (same MRO base, same extra="forbid", same MinLen(1)/MaxLen(1)). That is a hand-copy of codegen output whose constraints need manual re-sync on every schema re-vendor, and it is inconsistent with BusinessEntityResponse(BusinessEntity) twenty lines above. Subclass NotificationAuthentication and narrow only credentials.
13. A per-entry ownership rejection aborts the whole sync_accounts batch.
examples/v3_reference_seller/src/platform.py:444 raises inside the for incoming in refs loop, inside async with session.begin(), so a batch where any single entry names another buyer's account rolls back the entries that already succeeded:
OPERATION-LEVEL RAISE: {'code':'ACCOUNT_NOT_FOUND','message':"Account 'two.example::op.example' is not visible to the authenticated buyer agent.",'recovery':'terminal','field':'accounts'}
entry-1 rows lost; session.add had been called for: ['one.example::op.example']
schemas/cache/3.1/account/sync-accounts-response.json @ 3.1.8 reserves the operation-level error channel for "complete failure"; the per-entry channel is accounts[i].action="failed" + status="rejected" + errors[]. This is also inconsistent with what the same PR does one layer up — tenant_store.upsert emits a per-entry ACCOUNT_NOT_FOUND row for the identical cross-boundary condition. Append a SyncAccountsResultRow(action="failed", status="rejected", errors=[...]) and continue. While there, field="accounts" is unindexed where the spec's convention is an exact path (FIELD_NOT_PERMITTED: "error.field MUST identify the exact offending field path (e.g., packages[0].budget)") — point it at accounts[<i>]. test_account_store_upsert_cannot_overwrite_another_buyers_account uses a single-entry batch, so add the mixed-batch case asserting the clean entry still lands.
14. The plaintext bearer is written into request.scope["user"], and nothing reads it.
src/adcp/server/auth.py:511 sets AccessToken(token=bearer, ...), readable by every downstream ASGI middleware and by handlers:
plaintext bearer readable from scope: super-secret-bearer
The adjacent comment claims "The raw token remains request-scoped; the session manager stores only its derived (client_id, issuer, subject) authorization context." The second clause is accurate; the first is not — the scope dict travels with the request through the whole stack, and any error or logging middleware that serializes it surfaces the token. Nothing in the binding reads .token: session ownership is authorization_context(user) → principal_components(token) → (token.client_id, claims["iss"], token.subject) (mcp 2.0.0, mcp/server/auth/middleware/bearer_auth.py:22-38), so a derived opaque value is behaviourally identical. This is the rule the project fail-closes on elsewhere (_CREDENTIAL_SHAPED_KEY_SUFFIXES rejects credential-shaped keys in ctx_metadata; bearer flows deliberately set AuthInfo.credential=None at auth.py:737), and the _A2AAuthenticatedUser sibling twenty lines away in this same file carries only derived identity (:1453-1456). Pass hashlib.sha256(bearer.encode()).hexdigest() or an opaque per-request id, correct the comment, and assert in tests/test_mcp_stateful_session.py that the bearer string does not appear anywhere in scope["user"]. Raised as a follow-up on 2026-07-29; still open, and this diff is the code that introduces it.
15. InMemoryProposalStore.get kept its optional account scope, so the diff had to add a cross-account resolver.
src/adcp/decisioning/proposal_store.py:422-429 exists only to serve expected_account_id=None, and what _record_key does is scan every account's records and return another account's record whenever the proposal id happens to be globally unique — then return None once a second account uses the same id. One account's result depends on whether another account has registered that id, which the new test pins as intended (assert await store.get("shared") is None). Every sibling method on the Protocol already requires expected_account_id: str (:209, :231, :257, :273, :291, :310, :326); get is the lone str | None (:190), and all three framework call sites pass it (proposal_dispatch.py:197, :374, proposal_lifecycle.py:102). Make it required, delete _record_key, and both the O(n) scan under the lock and the cross-account observability channel disappear. The residual record.account_id != expected_account_id check in get (:496-500) becomes unreachable then — delete it so the invariant lives in one place.
Notes
- The 2026-07-29 timing-oracle follow-up on the new
ACCOUNT_NOT_FOUNDbranch is closed, not a finding:PermissionDeniedBudgetis constructed only atsrc/adcp/decisioning/handler.py:572and:734(the buyer-agent identity gate) and never wrappedtenant_store.upsert, so nothing moved out of a clamp. BothACCOUNT_NOT_FOUNDbranches inupsertpay the sameresolve_by_refcall and format through the same_account_not_found_message(ref), leaving a dict comparison as the only delta. - The per-request
from mcp.server.auth...import insidedispatch()is not a 500 risk:mcp>=2.0.0,<3.0is a core dependency (pyproject.toml:73), not an extra, soImportErroris unreachable in a valid install. Its placement is covered by finding 1's call to action, not raised separately. examples/v3_reference_seller/src/app.py:166-170stores the live bearer underctx.metadata["api_key_id"]—row.api_key_idis thetoken_mapkey, and_CREDENTIAL_SHAPED_KEY_SUFFIXESmisses it because it ends in_id. Out of scope as a fix item: the line is context, not a diff line, and I could not find a path that echoesctx.metadatato the buyer (inject_contextatsrc/adcp/server/helpers.py:378-383echoes only the buyer's own requestcontext, andsrc/adcp/server/idempotency/does not persist it). Adding_id-suffixed shapes tosrc/adcp/decisioning/dispatch.py:434-443is worth a separate change.push_notification_config.authentication.credentialsandreporting_webhook.authentication.credentialsare not marked write-only inschemas/cache/3.1/core/*.json@ 3.1.8, so leaving them out of the scrubbers is correct as-is. Noted because the shapes are otherwise identical tonotification_configand a future reader may assume the scrubber is incomplete.invoice_recipient.bankis a different case and is in finding 3.registry_cache._current_tenant_idlazily importsadcp.serverfrom the decisioning layer (src/adcp/decisioning/registry_cache.py:145), a lower layer reaching up to the transport layer. Pre-existing and untouched by this diff, so raising it would be an unrelated refactor.
| ) | ||
| from mcp.server.auth.provider import AccessToken # noqa: PLC0415 | ||
|
|
||
| request.scope["user"] = AuthenticatedUser( |
There was a problem hiding this comment.
Session ownership ends up derived from whether this request carried a credential, not from who created the session. This block runs only in the bearer-validated branch, so a token-less initialize takes the bypass at line 441 with scope["user"] unset, mcp_sessions._handle_stateful_request computes requestor = None, and ownership is recorded only if requestor is not None (mcp_sessions.py:200-201).
Measured at head (both rows are 200/200/200 before this PR):
[authed-initialize] tools/list owner -> 200 other -> 404 anonymous -> 404
[pre-auth-initialize] tools/list owner -> 404 other -> 404 anonymous -> 200
So the documented pre-auth handshake (connect, read get_adcp_capabilities, then authenticate) permanently 404s the session's own owner, and the session it created has no owner, so any anonymous caller who learns the session id can drive it. allow_unauthenticated=True is in the same bucket.
Set the owner where the session is created — in ADCPStreamableHTTPSessionManager._handle_stateful_request, from the request state the framework already threads via _read_request_state_auth — and make an unowned session fail closed rather than match any caller. The MCP-shaped AuthenticatedUser/AccessToken construction belongs in mcp_sessions.py, which already imports both at module level, not as a function-local import in the transport-agnostic middleware whose A2ABearerAuthMiddleware sibling publishes a different scope["user"] shape (auth.py:1453). Coverage needed is the mixed-auth matrix above.
|
|
||
| request.scope["user"] = AuthenticatedUser( | ||
| AccessToken( | ||
| token=bearer, |
There was a problem hiding this comment.
token=bearer puts the plaintext credential in request.scope["user"], readable by every downstream ASGI middleware and by handlers as scope["user"].access_token.token — verified against a live ASGI app: plaintext bearer readable from scope: super-secret-bearer.
The comment above says the raw token "remains request-scoped"; the scope dict travels with the request through the whole stack, so any error or logging middleware that serializes it surfaces the token. Nothing in the binding reads .token: ownership is principal_components(token) → (client_id, claims["iss"], subject) (mcp 2.0.0, mcp/server/auth/middleware/bearer_auth.py:22-38). The _A2AAuthenticatedUser sibling at :1453-1456 deliberately carries only derived identity, and bearer flows set AuthInfo.credential=None at :737.
Pass hashlib.sha256(bearer.encode()).hexdigest() or an opaque per-request id, fix the comment, and assert in tests/test_mcp_stateful_session.py that the bearer string appears nowhere in scope["user"]. Raised as a follow-up on 2026-07-29; still open.
| def _spend_locked(self, key: tuple[str | None, str], now: float) -> bool: | ||
| bucket = self._buckets.get(key) | ||
| if bucket is None: | ||
| if len(self._buckets) >= self._max_buckets: |
There was a problem hiding this comment.
The bucket namespace is global while the budget that fills it is per-tenant, so allocation failure crosses the isolation boundary this class exists to enforce. self._buckets holds both (tenant_id, lookup_key) and (tenant_id, "__tenant_aggregate__") entries, and the bucket a new tenant needs is its own aggregate bucket, created on its first lookup.
Measured at shipped defaults (100 rps, 10,000 buckets, fake clock): one tenant probing fresh agent_urls at exactly its permitted rate fills all 10,000 slots in 100 simulated seconds with zero denials for itself, and then an unrelated tenant's first-ever lookup raises PERMISSION_DENIED / recovery="correctable". _prune_idle cannot relieve it — it returns at the first bucket younger than the 300 s TTL, and cycling 9,999 keys at 100 rps keeps every one warm. No attacker required either: 100 tenants × 100 buyer agents reaches the cap, and both recommended stacks take the default (examples/v3_reference_seller/src/buyer_registry.py:173, pg/buyer_agent_registry.py:470).
Cap per tenant, keep the aggregate bucket in a map the cap never touches so a new tenant can always allocate, and when a tenant's own sub-cap is full evict that tenant's LRU lookup bucket instead of refusing — safe for the reason the docstring already gives at :490-491, and bounded anyway by the aggregate bucket that now precedes it. Then decide the wire code: 3.1.8 classifies a capacity condition transient (RATE_LIMITED with retry_after, or SERVICE_UNAVAILABLE); correctable tells a buyer who was never rate-limited to fix their request. Add a two-tenant test — both new tests use one tenant.
| """Discard up to 16 least-recently-used buckets that are safely idle.""" | ||
| for _ in range(min(16, len(self._buckets))): | ||
| key, bucket = next(iter(self._buckets.items())) | ||
| if now - bucket.last_refill < self._bucket_idle_ttl: |
There was a problem hiding this comment.
The prune's stated safety argument (:490-491, "Discarding an idle bucket is safe because it would have refilled to its full burst") holds only when burst / rps_per_tenant <= bucket_idle_ttl_seconds. __init__ validates each of the three values in isolation at :517-526 and never the relation, while the class docstring invites the violating config ("Adopters with bursty real traffic raise this").
Measured with rps_per_tenant=1.0, burst=1000.0, bucket_idle_ttl_seconds=300.0:
drained after 1000 requests; buckets: 2
after 301s idle, allowed: 1000 (honest refill would allow ~301)
An enumeration prober gets burst probes per idle window instead of rate * ttl — 3.3x here, scaling with burst/rate — and it applies to the aggregate bucket, so the whole tenant budget resets. Not reachable at defaults (burst == rps), which is why nothing notices. Validate burst / rps_per_tenant <= bucket_idle_ttl_seconds at construction, or prune only when elapsed * rate >= burst. test_rate_limit_idle_buckets_expire asserts len(_buckets), which stays true exactly when the rate invariant breaks — assert admitted requests instead.
| exhausted = False | ||
| self._prune_idle(now) | ||
| aggregate_key = (tenant_id, "__tenant_aggregate__") | ||
| exhausted = not self._spend_locked(aggregate_key, now) |
There was a problem hiding this comment.
Charging the aggregate before the per-lookup bucket makes the tier shared-fate, and resolve_by_agent_url/resolve_by_credential are reachable before the caller is authorised — that is the premise of this class's credential-stuffing docstring, and the attacker picks the agent_url.
Measured at stock rate/burst (100/100), no clock advance:
attacker denied after 100 junk probes
legit buyer with untouched per-key bucket: DENIED PERMISSION_DENIED / correctable
Worse than a throttle on the wire: _denied_error() is the uniform "Buyer agent is not authorized for this seller… Resolve out-of-band via the seller's onboarding contact", so the victim cannot tell it was a rate limit. On main, junk probes drained only their own per-key buckets. This diff also deletes test_rate_limit_isolates_distinct_lookup_keys, the test that pinned that property, replacing it with one asserting the new behavior.
Separately, both tiers are built from the same self._rate/self._burst, and the aggregate is charged on every lookup while a per-key bucket is charged on a subset, so tokens_aggregate <= tokens_key always holds and the per-lookup tier can never bind. Replacing exhausted = not self._spend_locked(key, now) with pass leaves 25 of 27 tests in tests/test_buyer_agent_registry_cache.py green; the two failures assert len(limiter._buckets) == N. Charge the aggregate only on lookups that miss (an unknown key is the enumeration signal; a resolving one is not), or give the aggregate its own much larger allowance — and give the per-lookup tier its own tighter rate/burst so it can deny and be tested.
| """ | ||
|
|
||
| billing_entity: BusinessEntityResponse | None = None | ||
| notification_configs: SchemaVariant[list[_NotificationConfigResponse] | None] = None |
There was a problem hiding this comment.
The SchemaVariant override replaces the base field and drops its MaxLen(16), so the response projection is more permissive than the model it narrows:
Account.model_fields['notification_configs'].metadata -> [MaxLen(max_length=16)]
AccountResponse.model_fields['notification_configs'].metadata -> []
Account REJECTED 20 configs / AccountResponse ACCEPTED 20
schemas/cache/3.1/core/account.json @ 3.1.8 sets maxItems: 16, and to_account_response is on the emit path, so a seller building through the class whose job is to be the safe response shape can emit a body that fails buyer-side validation. Carry the bound through: SchemaVariant[Annotated[list[_NotificationConfigResponse], Field(max_length=16)] | None], with a test asserting AccountResponse rejects 17. Raised on 2026-07-29, still open.
|
|
||
| model_config = ConfigDict(extra="forbid") | ||
|
|
||
| schemes: list[AuthenticationScheme] = Field(min_length=1, max_length=1) |
There was a problem hiding this comment.
This re-declares constraints that already exist in codegen. adcp.types.NotificationAuthentication is the generated model NotificationConfig.authentication uses — same MRO base, same extra="forbid", same MinLen(1)/MaxLen(1) on schemes — so this is a hand-copy whose bounds need manual re-sync on every schema re-vendor, and it is inconsistent with BusinessEntityResponse(BusinessEntity) twenty lines above and with CLAUDE.md's rule that adcp.types is the public surface. Write class _NotificationAuthenticationResponse(NotificationAuthentication) and narrow only credentials.
| f" updated_at TIMESTAMPTZ NOT NULL DEFAULT now()" | ||
| f");" | ||
| f"CREATE INDEX IF NOT EXISTS {table}_api_key_id_idx " # noqa: S608 | ||
| f"CREATE UNIQUE INDEX IF NOT EXISTS {table}_api_key_id_uidx " # noqa: S608 |
There was a problem hiding this comment.
The migration got a preflight; create_schema() got the same ratchet without one. Against an existing deployment this aborts the whole DDL batch with a raw UniqueViolation and none of the "rotate or remove duplicate bearer credentials, then rerun" guidance that examples/v3_reference_seller/alembic/versions/0003_unique_buyer_api_key.py:46-50 provides — and the docstring two lines above still says "Idempotent via CREATE ... IF NOT EXISTS; safe to call on every app boot." The index name also changed from ..._api_key_id_idx to ..._api_key_id_uidx and nothing drops the old one, so the legacy non-unique index is orphaned forever. Add the duplicate preflight, drop the superseded index, and fix the docstring together. Same for buyer_agent_registry.sql:64.
The SDK also has no upgrade-path coverage for this: test_schema_bootstrap_creates_partial_unique_credential_index asserts three substrings of the DDL string against a hand-rolled cursor, which cannot observe any of the above. A Postgres conformance case in tests/conformance/decisioning/test_pg_buyer_agent_registry.py — legacy non-unique index, two rows sharing an api_key_id, then create_schema() — pins whichever outcome you choose.
One more implementation in the same pre-migration window was skipped: this file's LIMIT 2 + fail-closed posture exists explicitly "for deployments that have not yet applied the unique-index migration", but examples/v3_reference_seller/src/buyer_registry.py:108 still does scalar_one_or_none(), which raises SQLAlchemy MultipleResultsFound on a legacy duplicate rather than denying — and its where is tenant-scoped, so pre-migration a duplicated api_key_id resolves to whichever tenant's host the request arrived on.
| suffix = f" — {snippet}" if snippet else "" | ||
| base = f"upstream {method} {path} failed: {status_code}{suffix}" | ||
| """Project an upstream status without exposing its untrusted response body.""" | ||
| base = f"upstream {method} {path} failed: {status_code}" |
There was a problem hiding this comment.
Dropping the body from the message is right for buyer-facing output, but the snippet is not relocated — not even to a log line — so the seller loses the only record of why the upstream failed. logger.warning the status plus a truncated body here; test_error_response_body_is_not_exposed grades the buyer-facing half only.
While this function's signature and every returned message are being rewritten: recovery="retry_with_changes" at :146 is not in the pinned enum. schemas/cache/3.1/core/error.json @ 3.1.8 gives recovery: ["transient", "correctable", "terminal"], and there is no normalisation step, so it reaches to_wire() verbatim and a buyer dispatching on recovery falls off the switch. AdcpError's own docstring calls it a legacy alias for correctable — make it correctable. The three terminal branches at :116, :122, :128 also disagree with enumMetadata (AUTH_REQUIRED, PERMISSION_DENIED and MEDIA_BUY_NOT_FOUND are all correctable); src/adcp/decisioning/errors.py already binds each code to its normative recovery.
| """Resolve a composite key, refusing ambiguous unscoped lookups.""" | ||
| if expected_account_id is not None: | ||
| return (expected_account_id, proposal_id) | ||
| matches = [key for key in self._records if key[1] == proposal_id] |
There was a problem hiding this comment.
_record_key exists only to serve expected_account_id=None, and this scan returns another account's record whenever the proposal id happens to be globally unique, then None once a second account uses the same id. One account's result therefore depends on whether another account registered that id — which the new test pins as intended (assert await store.get("shared") is None).
Every sibling method on the Protocol already requires expected_account_id: str (:209, :231, :257, :273, :291, :310, :326); get is the lone str | None (:190), and all three framework call sites pass it (proposal_dispatch.py:197, :374, proposal_lifecycle.py:102). Make it required and delete _record_key — the O(n)-under-lock scan and the cross-account observability channel both disappear. The residual record.account_id != expected_account_id check in get (:496-500) then becomes unreachable; delete it so the invariant lives in one place.
Summary
Why
The audit found cross-tenant lookup paths, response shapes that could expose credentials, and race-prone credential ownership without a database uniqueness guarantee.
Validation
origin/mainCompatibility
Roster stores now require an authorization callback at construction. The migration preflights duplicate credential hashes and fails safely before creating the unique index.