Skip to content

Commit e464d50

Browse files
bokelleyclaude
andcommitted
feat(decisioning): wire credential strip into dispatch + webhook + registry, add ctx_metadata guidance
The typed projections (`to_wire_account` / `to_wire_sync_accounts_row` / `to_wire_sync_governance_row` from PR #469) shipped as public-API helpers but no framework code called them — adopters returning loose dicts or Pydantic ``extra='allow'`` models bypassed the strip entirely. This wires defense-in-depth across every wire-emit boundary so the strip is load-bearing regardless of return shape. H1 — `_invoke_platform_method` runs `strip_credentials_from_wire_result` on every sync return, recursively scrubbing `governance_agents[i].authentication` and `billing_entity.bank` from dicts/lists. Method-gated to `CREDENTIAL_BEARING_METHODS` so non-account tools (`get_products`, `get_signals`) skip the walk on the hot path. Typed Pydantic response models pass through (the response-side schema forbids `authentication` structurally). H2 — `maybe_emit_sync_completion` re-applies the strip before passing `result` to the buyer-controlled webhook URL. Defense-in-depth so the strip fires regardless of upstream sanitization (custom shims, direct adopter calls). H3 — `_project_handoff` strips before `await registry.complete(...)`. Durable registries (Postgres, Redis) write the artifact to disk; even in-memory, `tasks/get` returns it verbatim. M1 — INTERNAL_ERROR `caused_by` now carries only the exception class name, not its `str()`. Any truncation length useful for diagnostics (200 chars) also fits a full OAuth client secret. Full traceback stays in server logs via `logger.exception`. M2 — `adcp.server.responses._serialize` runs `_strip_write_only_fields` on dict items before emit. Adopters hand-building responses with ``{**db_record, ...}`` no longer smuggle credentials through. M3 — `_build_request_context` fail-closes when `ctx_metadata` carries keys ending in `credential`, `credentials`, `token`, `secret`, `api_key`, `apikey`, `password`, `bearer` (case-insensitive, walks nested dicts). The AdCP context-echo contract round-trips metadata into responses; credential-shaped keys belong in `AuthInfo.credential` / typed credential classes. Documented in the new "ctx_metadata: write-only credentials prohibited" section of CLAUDE.md. Paths now covered: - Synchronous return path through `_invoke_platform_method` - TaskHandoff completion path through `registry.complete` - Sync-completion webhook path through `maybe_emit_sync_completion` - INTERNAL_ERROR wire envelope (`caused_by.message` removed) - Hand-built response builder path through `adcp.server.responses._serialize` - ctx_metadata fail-closed gate at `_build_request_context` Refs #463, #452. Completes the wiring PR #469's projections were missing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8dd5dab commit e464d50

7 files changed

Lines changed: 972 additions & 42 deletions

File tree

CLAUDE.md

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,47 @@ All other source code should import from `adcp.types` (the public API).
4343
- Add specific `type: ignore` comments (e.g., `# type: ignore[no-any-return]`) rather than blanket ignores
4444
- Test type checking in CI across multiple Python versions (3.10+)
4545

46+
## ctx_metadata: write-only credentials prohibited
47+
48+
`RequestContext.metadata` (populated from the wire request's `context` extension)
49+
is **echoed back into responses** per the AdCP context-echo contract. Adopters who
50+
treat `metadata` as a generic KV bucket and store a credential there will discover
51+
it round-trips to the buyer — and lands in the idempotency replay cache.
52+
53+
The dispatcher fail-closes on credential-shaped keys at `_build_request_context`.
54+
If you see a `ValueError` like `ctx_metadata may not contain credential-shaped
55+
keys`, migrate the value to `AuthInfo.credential` or a typed credential class.
56+
57+
**Wrong** — credential stored in metadata, round-trips into response context:
58+
59+
```python
60+
ctx = RequestContext(metadata={"upstream.api_token": secret}) # ValueError
61+
```
62+
63+
**Right** — credential stored in the typed `AuthInfo.credential` field:
64+
65+
```python
66+
auth = AuthInfo(
67+
kind="api_key",
68+
key_id="kid_1",
69+
principal="agent.example.com",
70+
credential=ApiKeyCredential(kind="api_key", key_id="kid_1"),
71+
)
72+
ctx = RequestContext(auth_info=auth, metadata={"correlation_id": "req_xyz"})
73+
```
74+
75+
The credential-shaped key suffix list is in
76+
`adcp.decisioning.dispatch._CREDENTIAL_SHAPED_KEY_SUFFIXES` and matches
77+
case-insensitively at any nesting depth: `credential`, `credentials`, `token`,
78+
`secret`, `api_key`, `apikey`, `password`, `bearer`. Keys that don't match
79+
(`correlation_id`, `feature_flag.beta_pricing`, `trace_id`) pass through.
80+
81+
For credentials the framework propagates to upstream calls (governance agents,
82+
signal providers, audience activations), use the typed credential classes from
83+
`adcp.decisioning`: `ApiKeyCredential`, `OAuthCredential`, `HttpSigCredential`.
84+
The framework dispatch threads these explicitly without going through the
85+
context-echo path.
86+
4687
## Testing Strategy
4788

4889
**Mock at the Right Level**

src/adcp/decisioning/account_projection.py

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,9 +329,118 @@ def to_wire_sync_governance_row(row: SyncGovernanceResultRow) -> dict[str, Any]:
329329
return wire
330330

331331

332+
# ---------------------------------------------------------------------------
333+
# Defense-in-depth scrubber — runs at every wire-emit boundary
334+
# ---------------------------------------------------------------------------
335+
336+
337+
#: Methods whose response payload may carry credential-shaped fields.
338+
#: The dispatcher gates the recursive scrubber on this set so unrelated
339+
#: tools (``get_products``, ``get_signals``) skip the walk entirely —
340+
#: the scrubber is O(n) in result size and not free for large catalogs.
341+
#:
342+
#: This set is intentionally broad: any tool whose response surfaces
343+
#: an ``Account`` envelope (``billing_entity``, ``governance_agents``)
344+
#: or a joined record carrying those keys belongs here.
345+
CREDENTIAL_BEARING_METHODS: frozenset[str] = frozenset(
346+
{
347+
"list_accounts",
348+
"sync_accounts",
349+
"sync_governance",
350+
"create_media_buy",
351+
"update_media_buy",
352+
"get_media_buys",
353+
"sync_creatives",
354+
"list_creatives",
355+
}
356+
)
357+
358+
359+
def _scrub_dict(value: dict[str, Any]) -> dict[str, Any]:
360+
"""Return a copy of ``value`` with credential-shaped fields stripped.
361+
362+
Strips:
363+
364+
* ``governance_agents[i].authentication`` — write-only credential.
365+
* ``billing_entity.bank`` — write-only bank coordinates.
366+
367+
Walks recursively into nested dicts and lists. Returns a NEW dict —
368+
the input is not mutated, so callers (idempotency replay cache,
369+
middleware) can rely on input stability.
370+
"""
371+
out: dict[str, Any] = {}
372+
for key, sub in value.items():
373+
if key == "governance_agents" and isinstance(sub, list):
374+
out[key] = [_scrub_governance_agent_dict(a) if isinstance(a, dict) else a for a in sub]
375+
elif key == "billing_entity" and isinstance(sub, dict):
376+
out[key] = {k: v for k, v in _scrub_value(sub).items() if k != "bank"}
377+
else:
378+
out[key] = _scrub_value(sub)
379+
return out
380+
381+
382+
def _scrub_governance_agent_dict(agent: dict[str, Any]) -> dict[str, Any]:
383+
"""Strip ``authentication`` from a governance-agent dict and recurse
384+
into the remaining fields."""
385+
return {k: _scrub_value(v) for k, v in agent.items() if k != "authentication"}
386+
387+
388+
def _scrub_value(value: Any) -> Any:
389+
"""Recurse into dicts / lists; return primitives unchanged."""
390+
if isinstance(value, dict):
391+
return _scrub_dict(value)
392+
if isinstance(value, list):
393+
return [_scrub_value(v) for v in value]
394+
return value
395+
396+
397+
def strip_credentials_from_wire_result(method_name: str, result: Any) -> Any:
398+
"""Strip write-only credential fields from a wire-shape result.
399+
400+
Defense-in-depth boundary called by the dispatcher on every
401+
response that may surface an :class:`Account` envelope. Removes
402+
``governance_agents[i].authentication`` and ``billing_entity.bank``
403+
recursively — the same fields the typed projections
404+
(:func:`to_wire_account`, :func:`to_wire_sync_governance_row`)
405+
strip when the adopter returns the framework's typed dataclasses.
406+
407+
Adopters returning a loose dict (or a Pydantic model with
408+
``extra='allow'``) bypass the typed projections; this scrubber
409+
catches them. Adopters returning the typed dataclasses get
410+
double-stripped — the second pass is a no-op since the typed
411+
projections already removed the fields.
412+
413+
Method gate: the scrubber is O(n) in result size; we only run it
414+
on methods in :data:`CREDENTIAL_BEARING_METHODS`. Non-account
415+
methods (``get_products``, ``get_signals``, ``activate_signal``)
416+
skip the walk entirely and pass through unchanged.
417+
418+
The input is not mutated — returns a new value.
419+
"""
420+
if method_name not in CREDENTIAL_BEARING_METHODS:
421+
return result
422+
if isinstance(result, dict):
423+
return _scrub_dict(result)
424+
if isinstance(result, list):
425+
return [_scrub_value(v) for v in result]
426+
# Typed Pydantic response models pass through unchanged — the
427+
# response-side codegen'd shapes don't define ``authentication``
428+
# on ``GovernanceAgent`` or ``bank`` on the response-side
429+
# ``BusinessEntity``, so the schema enforces the strip
430+
# structurally. Dumping-and-scrubbing a model would force
431+
# downstream callers to lose typed-model identity for no
432+
# security gain. The leak vector is loose dicts and Pydantic
433+
# ``extra='allow'`` models that smuggle credentials past the
434+
# codegen schema; both arrive as ``dict`` after the adopter's
435+
# method returns or via the registry's ``model_dump`` path.
436+
return result
437+
438+
332439
__all__ = [
440+
"CREDENTIAL_BEARING_METHODS",
333441
"project_account_for_response",
334442
"project_business_entity_for_response",
443+
"strip_credentials_from_wire_result",
335444
"to_wire_account",
336445
"to_wire_sync_accounts_row",
337446
"to_wire_sync_governance_row",

src/adcp/decisioning/dispatch.py

Lines changed: 120 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,9 @@
4444
from concurrent.futures import ThreadPoolExecutor
4545
from typing import TYPE_CHECKING, Any
4646

47+
from adcp.decisioning.account_projection import (
48+
strip_credentials_from_wire_result,
49+
)
4750
from adcp.decisioning.platform import (
4851
GOVERNANCE_SPECIALISMS,
4952
DecisioningCapabilities,
@@ -393,10 +396,78 @@ def _strict_validate_platform() -> bool:
393396
# INTERNAL_ERROR breadcrumbs (Emma AudioStack P2)
394397
# ---------------------------------------------------------------------------
395398

396-
#: Cap on the message+repr we expose to the wire. Long stack traces or
397-
#: secret-shaped repr (e.g., a ``Credential`` repr that includes the
398-
#: token) get truncated. Stack trace lives in server logs only.
399-
_INTERNAL_ERROR_DETAIL_CHARS = 200
399+
#: Substring suffixes that flag a ctx_metadata key as credential-shaped.
400+
#: Lowercased for case-insensitive matching against the user-supplied
401+
#: key. The list intentionally errs broad — a key like
402+
#: ``"upstream.api_key"`` belongs in :class:`AuthInfo.credential`, not
403+
#: ``ctx.metadata`` which round-trips into responses.
404+
#:
405+
#: Drift policy: when the spec or adopter conventions add a new
406+
#: credential-shaped suffix, append here. The gate is fail-closed by
407+
#: design — false positives require the adopter to rename the key, NOT
408+
#: silently echo the credential.
409+
_CREDENTIAL_SHAPED_KEY_SUFFIXES: tuple[str, ...] = (
410+
"credential",
411+
"credentials",
412+
"token",
413+
"secret",
414+
"api_key",
415+
"apikey",
416+
"password",
417+
"bearer",
418+
)
419+
420+
421+
def _validate_ctx_metadata_credentials(metadata: Any) -> None:
422+
"""Fail-closed gate: ctx.metadata must not carry credential-shaped
423+
keys.
424+
425+
The framework projects buyer-supplied ``context`` extensions into
426+
``tool_ctx.metadata`` and echoes context back on responses per
427+
the AdCP spec. An adopter who treats ``metadata`` as a generic
428+
KV bucket can accidentally round-trip a credential to the buyer.
429+
The ergonomic path for credentials is
430+
:class:`AuthInfo.credential` / typed credential classes
431+
(:class:`ApiKeyCredential`, :class:`OAuthCredential`,
432+
:class:`HttpSigCredential`); ``ctx.metadata`` is for non-secret
433+
request-scope hints (correlation ids, feature flags, trace ids).
434+
435+
Matches against any key whose lowercased form ends with one of
436+
:data:`_CREDENTIAL_SHAPED_KEY_SUFFIXES`. Sub-keys at any nesting
437+
depth count — a buyer-supplied
438+
``{"upstream": {"api_token": "..."}}`` is rejected the same as
439+
a flat ``{"api_token": "..."}``.
440+
441+
:raises ValueError: when any credential-shaped key is found. The
442+
exception message names the offending key path so the adopter
443+
knows which field to migrate to ``AuthInfo.credential``.
444+
"""
445+
if not metadata:
446+
return
447+
if not isinstance(metadata, dict):
448+
return
449+
for key, value in metadata.items():
450+
if isinstance(key, str):
451+
lower = key.lower()
452+
for suffix in _CREDENTIAL_SHAPED_KEY_SUFFIXES:
453+
if lower.endswith(suffix):
454+
raise ValueError(
455+
"ctx_metadata may not contain credential-shaped keys; "
456+
"use AuthInfo.credential (or a typed credential class "
457+
"like ApiKeyCredential / OAuthCredential / "
458+
"HttpSigCredential) instead. Found: "
459+
f"{key!r} (matched suffix {suffix!r}). "
460+
"ctx.metadata round-trips into response context per "
461+
"the AdCP spec; placing a credential here echoes it "
462+
"to the buyer."
463+
)
464+
if isinstance(value, dict):
465+
try:
466+
_validate_ctx_metadata_credentials(value)
467+
except ValueError as exc:
468+
# Re-raise with the parent key prefixed so the diagnostic
469+
# walks the adopter to the offending path.
470+
raise ValueError(f"In ctx_metadata[{key!r}]: {exc}") from None
400471

401472

402473
def _internal_error_message(method_name: str, exc: BaseException) -> str:
@@ -415,15 +486,18 @@ def _internal_error_details(exc: BaseException) -> dict[str, Any]:
415486
"""Build the wire-side ``details`` payload for an INTERNAL_ERROR
416487
wrap.
417488
418-
``details.caused_by`` carries the exception class name + truncated
419-
str — no traceback, no module path, no chained ``__cause__``.
420-
The class name lets adopters distinguish ``AttributeError``
421-
(typo-shaped) from ``KeyError`` (missing-config-shaped) from
422-
``ConnectionError`` (network-shaped) at a glance.
489+
``details.caused_by`` carries ONLY the exception class name —
490+
``"AttributeError"`` (typo-shaped), ``"KeyError"``
491+
(missing-config-shaped), ``"ConnectionError"`` (network-shaped) —
492+
enough for the seller dev to triage at a glance. The exception's
493+
``str()`` is deliberately omitted: any truncation length large
494+
enough to be useful (200 chars) is also large enough to leak a
495+
full OAuth client secret or bearer token if the adopter raised
496+
on secret material. The full traceback (with message) lives in
497+
the server log via ``logger.exception``; only the wire response
498+
is sanitized to a class-name breadcrumb.
423499
424500
**``caused_by.type`` is a debug breadcrumb, not a wire contract.**
425-
The value is Python's exception class name verbatim
426-
(``"AttributeError"``, ``"KeyError"``, ``"ValidationError"``).
427501
Buyers built against the JS SDK won't see Python-flavoured class
428502
names from JS sellers — only Python sellers leak Python types.
429503
Treat this field as "hint to the seller dev reading their own
@@ -435,11 +509,6 @@ def _internal_error_details(exc: BaseException) -> dict[str, Any]:
435509
``recovery`` (terminal/correctable/transient) which IS the
436510
cross-language contract.
437511
438-
Truncation is defense-in-depth against an adopter who throws on
439-
secret material and ends up with a repr that includes the secret
440-
value verbatim. The full traceback is in the server log via
441-
``logger.exception``; only the wire response is sanitized.
442-
443512
**ValidationError special case** (Stability AI Emma P1 from the
444513
post-#340 matrix): when the platform method raises a pydantic
445514
``ValidationError`` directly — typically because the seller's
@@ -454,13 +523,9 @@ def _internal_error_details(exc: BaseException) -> dict[str, Any]:
454523
where a structured field list is meaningful, so we don't
455524
generalize this to other exception types.
456525
"""
457-
raw = str(exc)
458-
if len(raw) > _INTERNAL_ERROR_DETAIL_CHARS:
459-
raw = raw[: _INTERNAL_ERROR_DETAIL_CHARS - 3] + "..."
460526
details: dict[str, Any] = {
461527
"caused_by": {
462528
"type": type(exc).__name__,
463-
"message": raw,
464529
}
465530
}
466531
# Try to import lazily so a future refactor that splits the
@@ -876,6 +941,19 @@ def _build_request_context(
876941

877942
auth_principal = auth_info.principal if auth_info is not None else None
878943

944+
# ctx_metadata credential gate — fail-closed before any platform
945+
# method sees the metadata. Buyers can populate ``context``
946+
# extensions on the wire request that the framework projects into
947+
# ``tool_ctx.metadata``; an adopter who treats ``metadata`` as a
948+
# general-purpose KV bucket might shove a credential through it,
949+
# only to discover the value round-trips into the response (the
950+
# framework echoes context into responses per the AdCP spec).
951+
# The ergonomic path for credentials is :class:`AuthInfo.credential`
952+
# / typed credential classes; ``metadata`` is for non-secret
953+
# request-scope hints. See the "ctx_metadata: write-only credentials
954+
# prohibited" section in CLAUDE.md.
955+
_validate_ctx_metadata_credentials(tool_ctx.metadata)
956+
879957
# Composite cache scope key when store is supplied (production
880958
# path). Falls back to tool_ctx.caller_identity for test fixtures.
881959
caller_identity: str | None
@@ -1053,7 +1131,14 @@ async def _invoke_platform_method(
10531131
registry=registry,
10541132
executor=executor,
10551133
)
1056-
return result
1134+
1135+
# Defense-in-depth credential strip on every sync return. The typed
1136+
# projections (:func:`to_wire_account` etc.) handle the case where
1137+
# the adopter returns the framework's typed dataclasses; this
1138+
# boundary catches loose dicts and Pydantic models with
1139+
# ``extra='allow'``. Method-gated to avoid walking large product
1140+
# / signal catalogs that can't carry credentials.
1141+
return strip_credentials_from_wire_result(method_name, result)
10571142

10581143

10591144
async def _project_handoff(
@@ -1141,16 +1226,27 @@ async def _run() -> None:
11411226

11421227
# Persist terminal artifact. Pydantic responses get
11431228
# ``model_dump()``; dict responses pass through.
1229+
#
1230+
# Credential strip BEFORE persistence: durable registries
1231+
# (Postgres, Redis) write the artifact to disk; even in-memory,
1232+
# ``tasks/get`` returns it verbatim. A bearer credential
1233+
# surviving the typed projection (e.g., Pydantic
1234+
# ``extra='allow'`` model carrying ``governance_agents[i].
1235+
# authentication``) would land in the buyer's ``tasks/get``
1236+
# poll AND the idempotency replay cache. Method-gated so
1237+
# non-account methods skip the recursive walk.
11441238
if hasattr(result, "model_dump"):
1145-
await registry.complete(task_id, result.model_dump())
1239+
persisted = result.model_dump()
11461240
elif isinstance(result, dict):
1147-
await registry.complete(task_id, result)
1241+
persisted = result
11481242
else:
11491243
# Adopter returned an unexpected type (not Pydantic, not
11501244
# dict). Best effort: stringify into a 'value' wrapper so
11511245
# tasks/get returns something. Real impls always return
11521246
# the typed Pydantic response.
1153-
await registry.complete(task_id, {"value": str(result)})
1247+
persisted = {"value": str(result)}
1248+
persisted = strip_credentials_from_wire_result(method_name, persisted)
1249+
await registry.complete(task_id, persisted)
11541250

11551251
# ``asyncio.create_task`` only weak-refs the resulting Task — under
11561252
# GC pressure or with no outer awaiter, the task can be collected

0 commit comments

Comments
 (0)