Skip to content

Commit 52b15fa

Browse files
bokelleyclaude
andauthored
feat(decisioning): credential-leak strip on every echo path + ctx_metadata guidance (#481)
* 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> * fix(decisioning): close credential-leak gaps in builder, registry, and ctx_metadata gate Three security-review should-fixes against the credential-leak strip: * sync_governance_response / sync_accounts_response builders now route items through `_serialize`, so loose-dict adopters spreading `governance_agents[i].authentication` or `billing_entity.bank` onto a hand-built response get the same scrub as `_invoke_platform_method`. Replaces a placeholder test that documented the gap with `if leaked: pass`. * `InMemoryTaskRegistry.complete()` now strips before persisting. The WorkflowHandoff path has the adopter calling `registry.complete()` directly from an external workflow — the framework is not on the call stack at that point, so the dispatcher-side strip cannot fire. Method gate uses the persisted `record.task_type`; idempotent on already-stripped payloads, so the dispatcher's pre-strip remains a no-op double-pass. * `_validate_ctx_metadata_credentials` now recurses into list values via `_walk_ctx_metadata_list`. A buyer-supplied `{"upstream_configs": [{"api_token": "..."}]}` previously slipped past silently; it now trips the gate the same as a flat key, with the diagnostic walking to the offending list index. Tests assert real behavior on every gap (no `if leaked: pass`); broad regression suite stays green. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 9985086 commit 52b15fa

8 files changed

Lines changed: 1191 additions & 46 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",

0 commit comments

Comments
 (0)