From 0c3eedf09a0e38d278d1a256a76b4825a3eb00be Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 3 Nov 2026 01:45:23 +0000 Subject: [PATCH] fix(accounts): never serve framework-internal account metadata or auth_info adcp.decisioning.Account carries two framework-internal fields, metadata (adopter state: upstream adapters, OAuth tokens, network ids) and auth_info (the verified principal). to_wire_account omits them by construction, but the defense-in-depth scrubber relied on that typed projection: a store returning plain dicts, or a handler spreading the internal object, served them on list_accounts rows, account_requests[], request_account, sync_accounts results and accounts embedded in media-buy / creative responses. The TS SDK drops the equivalent authInfo / ctx_metadata. strip_credentials_from_wire_result now drops both keys from account positions (an `account` object, an `accounts[]` row), not by bare name: metadata IS a wire field elsewhere on scrubbed responses (list_creatives creatives[].pricing_options[].metadata), and no account, account-with-authorization, sync_accounts or sync_governance row schema defines either key. project_account_for_response drops them from extra='allow' extras too. Requests are untouched: the scrubber only runs on responses. Co-Authored-By: Claude Opus 5.5 --- src/adcp/decisioning/account_projection.py | 54 ++++++++++++++++++++-- tests/test_account_projection.py | 23 +++++++++ tests/test_account_requests_seller.py | 18 +++++++- tests/test_credential_leak_strip.py | 50 ++++++++++++++++++++ 4 files changed, 140 insertions(+), 5 deletions(-) diff --git a/src/adcp/decisioning/account_projection.py b/src/adcp/decisioning/account_projection.py index 6b2e8b4907..efac04479c 100644 --- a/src/adcp/decisioning/account_projection.py +++ b/src/adcp/decisioning/account_projection.py @@ -34,6 +34,15 @@ model (``list_accounts`` rows, ``account_requests[]``, ``request_account``, ``sync_accounts`` results, embedded accounts). +The framework's own :class:`adcp.decisioning.Account` also carries two +framework-internal fields that are never wire fields: +``metadata`` (adopter state — upstream adapters, OAuth tokens, network +ids) and ``auth_info`` (the verified principal). :func:`to_wire_account` +omits them by construction; the scrubber drops them from every account +position (see :data:`_FRAMEWORK_INTERNAL_ACCOUNT_KEYS`) so a store that +returns plain dicts, or a handler that spreads the internal object, does +not serve them either. + The schemas describe both rules in docstrings; neither is structurally enforced by Pydantic. The helpers here ARE the structural enforcement. Defense-in-depth: even when an adopter returns a loosely-typed row @@ -87,7 +96,9 @@ def project_account_for_response(account: Account) -> Account: project through this helper before serializing on any response. Also clears :attr:`Account.destination_billing_entity`, a request-only staging field that MUST NOT appear in account read - models. + models, and drops the framework-internal ``metadata`` / + ``auth_info`` keys when they ride the model as ``extra='allow'`` + extras (neither is a wire ``Account`` field). Returns an unchanged copy when neither field needs stripping — defensive copy via ``model_copy()`` so callers can mutate the @@ -100,7 +111,13 @@ def project_account_for_response(account: Account) -> Account: update["destination_billing_entity"] = None if account.billing_entity is not None and account.billing_entity.bank is not None: update["billing_entity"] = account.billing_entity.model_copy(update={"bank": None}) - return account.model_copy(update=update) if update else account.model_copy() + projected = account.model_copy(update=update) if update else account.model_copy() + extra = projected.__pydantic_extra__ + if extra and not _FRAMEWORK_INTERNAL_ACCOUNT_KEYS.isdisjoint(extra): + projected.__pydantic_extra__ = { + k: v for k, v in extra.items() if k not in _FRAMEWORK_INTERNAL_ACCOUNT_KEYS + } + return projected def project_business_entity_for_response(entity: BusinessEntity) -> BusinessEntity: @@ -385,6 +402,25 @@ def to_wire_sync_governance_row(row: SyncGovernanceResultRow) -> dict[str, Any]: ) +#: Fields of the framework's internal :class:`adcp.decisioning.Account` +#: that are never wire fields: ``metadata`` (adopter state, which can hold +#: upstream adapters and OAuth tokens) and ``auth_info`` (the verified +#: principal). :func:`to_wire_account` omits them; the scrubber drops them +#: from account positions only (an ``account`` object, an ``accounts[]`` +#: row), because ``metadata`` IS a wire field elsewhere on scrubbed +#: responses (``list_creatives`` ``creatives[].pricing_options[].metadata``). +#: No AdCP ``Account``, account-with-authorization, ``sync_accounts`` or +#: ``sync_governance`` row schema defines either key. +_FRAMEWORK_INTERNAL_ACCOUNT_KEYS: frozenset[str] = frozenset({"metadata", "auth_info"}) + + +def _drop_framework_internal_account_keys(value: Any) -> Any: + """Drop :data:`_FRAMEWORK_INTERNAL_ACCOUNT_KEYS` from an account-position dict.""" + if isinstance(value, dict) and not _FRAMEWORK_INTERNAL_ACCOUNT_KEYS.isdisjoint(value): + return {k: v for k, v in value.items() if k not in _FRAMEWORK_INTERNAL_ACCOUNT_KEYS} + return value + + def _scrub_dict(value: dict[str, Any]) -> dict[str, Any]: """Return a copy of ``value`` with credential-shaped fields stripped. @@ -397,6 +433,11 @@ def _scrub_dict(value: dict[str, Any]) -> dict[str, Any]: * ``destination_billing_entity`` — request-only handoff staging field; dropped wholesale (non-bank fields included) because it MUST NOT appear in any account read model. + * ``metadata`` / ``auth_info`` on an ``account`` object or an + ``accounts[]`` row — framework-internal :class:`Account` fields + (:data:`_FRAMEWORK_INTERNAL_ACCOUNT_KEYS`), never wire fields. + Positional, not by bare name: ``metadata`` is a wire field + elsewhere. Walks recursively into nested dicts and lists. Returns a NEW dict — the input is not mutated, so callers (idempotency replay cache, @@ -440,6 +481,10 @@ def _scrub_dict(value: dict[str, Any]) -> dict[str, Any]: _scrub_error_dict(error) if isinstance(error, dict) else error for error in normalized_errors ] + elif key == "account": + out[key] = _drop_framework_internal_account_keys(_scrub_value(sub)) + elif key == "accounts" and isinstance(sub, list): + out[key] = [_drop_framework_internal_account_keys(_scrub_value(a)) for a in sub] elif key == "adcp_error": normalized_error = _scrub_value(sub) out[key] = ( @@ -497,8 +542,9 @@ def strip_credentials_from_wire_result(method_name: str, result: Any) -> Any: Defense-in-depth boundary called by the dispatcher on every response that may surface an :class:`Account` envelope. Removes - ``governance_agents[i].authentication``, ``billing_entity.bank`` and - the request-only ``destination_billing_entity`` recursively — the + ``governance_agents[i].authentication``, ``billing_entity.bank``, + the request-only ``destination_billing_entity`` and the + framework-internal account ``metadata`` / ``auth_info`` recursively — the same fields the typed projections (:func:`to_wire_account`, :func:`to_wire_sync_governance_row`) strip when the adopter returns the framework's typed dataclasses. diff --git a/tests/test_account_projection.py b/tests/test_account_projection.py index 8a4093eeb9..2818524b39 100644 --- a/tests/test_account_projection.py +++ b/tests/test_account_projection.py @@ -137,6 +137,29 @@ def test_project_account_clears_destination_billing_entity() -> None: assert account.destination_billing_entity == {"legal_name": "Destination GmbH"} +def test_project_account_drops_framework_internal_extras() -> None: + """``metadata`` / ``auth_info`` (framework-internal Account fields) never + ride a wire Account, even as ``extra='allow'`` extras.""" + account = Account.model_validate( + { + "account_id": "acct_x", + "name": "x", + "status": "active", + "metadata": {"upstream_access_token": "tok"}, + "auth_info": {"principal": "p"}, + "x_vendor": "kept", + } + ) + + projected = project_account_for_response(account) + + dumped = projected.model_dump(mode="json") + assert "metadata" not in dumped + assert "auth_info" not in dumped + assert dumped["x_vendor"] == "kept" + assert account.model_extra is not None and "metadata" in account.model_extra + + def test_project_account_when_billing_entity_is_none() -> None: """No billing_entity to project — return a defensive copy.""" account = Account(account_id="acct_x", name="x", status="active") diff --git a/tests/test_account_requests_seller.py b/tests/test_account_requests_seller.py index 9c24767ebf..3a17cf69aa 100644 --- a/tests/test_account_requests_seller.py +++ b/tests/test_account_requests_seller.py @@ -295,6 +295,9 @@ async def list_accounts(params: dict[str, Any]) -> TaskResult: _GOVERNANCE_SECRET = "gov-secret-token-0123456789abcdef" _WEBHOOK_SECRET = "webhook-secret-0123456789abcdefghij" _DESTINATION = {"legal_name": "Confidential Destination GmbH", "bank": dict(_BANK)} +# Framework-internal Account fields (adcp.decisioning.Account.metadata / .auth_info). +_UPSTREAM_TOKEN = "upstream-oauth-token-0123456789abcdef" +_PRINCIPAL = "principal-internal-0123456789" def _credentialed_account() -> dict[str, Any]: @@ -317,6 +320,9 @@ def _credentialed_account() -> dict[str, Any]: } ], "destination_billing_entity": dict(_DESTINATION), + # A store returning its internal record as a plain dict. + "metadata": {"network_id": "net-1", "upstream_access_token": _UPSTREAM_TOKEN}, + "auth_info": {"principal": _PRINCIPAL, "scopes": ["accounts:read"]}, } @@ -326,7 +332,17 @@ def _assert_scrubbed(account: dict[str, Any]) -> None: assert "credentials" not in account["notification_configs"][0]["authentication"] # Request-only staging field: never on an account read, not even its non-bank fields. assert "destination_billing_entity" not in account - for secret in (_BANK["iban"], _GOVERNANCE_SECRET, _WEBHOOK_SECRET, _DESTINATION["legal_name"]): + # Framework-internal Account fields: never wire fields. + assert "metadata" not in account + assert "auth_info" not in account + for secret in ( + _BANK["iban"], + _GOVERNANCE_SECRET, + _WEBHOOK_SECRET, + _DESTINATION["legal_name"], + _UPSTREAM_TOKEN, + _PRINCIPAL, + ): assert secret not in repr(account) diff --git a/tests/test_credential_leak_strip.py b/tests/test_credential_leak_strip.py index b356842256..874e02b44b 100644 --- a/tests/test_credential_leak_strip.py +++ b/tests/test_credential_leak_strip.py @@ -272,6 +272,56 @@ def test_strip_credentials_walks_recursively() -> None: assert "authentication" not in str(out) +def test_scrubber_drops_framework_internal_account_keys_at_account_positions() -> None: + """``metadata`` / ``auth_info`` are fields of the framework's internal + ``adcp.decisioning.Account``, never wire fields. A store or handler that + serves the internal record as a dict (``vars(account)``, a spread) must + not put them on the wire, wherever an account is embedded.""" + internal = { + "metadata": {"upstream_access_token": _BEARER}, + "auth_info": {"principal": "principal-internal"}, + } + out = strip_credentials_from_wire_result( + "get_media_buys", + { + "media_buys": [ + { + "media_buy_id": "mb-1", + "account": {"account_id": "acc-1", "name": "Acme", **internal}, + } + ] + }, + ) + assert out["media_buys"][0]["account"] == {"account_id": "acc-1", "name": "Acme"} + governance = strip_credentials_from_wire_result( + "sync_governance", + {"accounts": [{"account": {"account_id": "acc-1", **internal}, "status": "synced"}]}, + ) + assert governance["accounts"] == [{"account": {"account_id": "acc-1"}, "status": "synced"}] + assert _BEARER not in str(out) + str(governance) + + +def test_scrubber_keeps_wire_metadata_outside_account_positions() -> None: + """``metadata`` IS a wire field off the account + (``list_creatives`` ``creatives[].pricing_options[].metadata``); the + framework-internal drop is positional, never by bare name.""" + payload = { + "creatives": [ + { + "creative_id": "c-1", + "pricing_options": [{"pricing_option_id": "po-1", "metadata": {"tier": "gold"}}], + "account": {"account_id": "acc-1", "metadata": {"internal": True}}, + } + ] + } + out = strip_credentials_from_wire_result("list_creatives", payload) + (creative,) = out["creatives"] + assert creative["pricing_options"][0]["metadata"] == {"tier": "gold"} + assert creative["account"] == {"account_id": "acc-1"} + # Input not mutated. + assert payload["creatives"][0]["account"]["metadata"] == {"internal": True} + + def test_dispatch_scrubber_strips_bank_from_nested_pydantic_entities() -> None: entity = _EntityWithBank( legal_name="Acme Inc.",