Insecure-by-default gates: test-controller sandbox marker, ungated comply controller, and ref-routed tenant resolution

[bokelley]

From codex-security candidate verification. Items 1 and 2 were reproduced against the built dist and are straightforward code fixes. Item 3 is a deliberate documented default that needs a maintainer decision, not a patch.

The common thread: three places where a warning stands in for a gate. In all three the repo already agrees with itself on the right shape — createAdcpServer refuses InMemoryStateStore outside {NODE_ENV=test, NODE_ENV=development} unless ADCP_DECISIONING_ALLOW_INMEMORY_STATE=1, and assertSandboxAccount fails closed on undefined. A state-mutating test surface deserves at least what an in-memory store gets.

1. Test-controller bridge: the documented trust boundary does not hold (candidate-cd887c5188044f75) — High

The scan's framing ("when no account resolver is configured") understates this. Reproduced against a server that does wire resolveAccount:

construction warns matching trust-boundary: 0
creatives: [ 'real-1', 'FIXTURE-LEAK' ]
response sandbox flag: true

Request was list_creatives with no account field and context: { sandbox: true }.

Cause: create-adcp-server.ts:5007 runs the resolver only when params.account != null, and the else if at :5047 requires resolveAccountFromAuth. Wire only resolveAccount — the primary documented path — and ctx.account stays undefined for any request omitting the account ref, so the merge gate's ctx.account === undefined || arm admits on a buyer-supplied marker alone. The #1784 warning at :3745 fires only when both resolvers are absent, so the misconfiguration that actually matters is silent. Same hole when resolveAccountFromAuth returns null (:5062, unknown principal).

Impact: seeded-fixture disclosure into production read responses, plus responses falsely stamped sandbox: true — a wire-honesty violation. Not by itself cross-tenant real-data disclosure, but makeAutoSeedBridge (from-platform.ts:6535-6537) falls back to the buyer-claimed account_id when ctx.account is absent, so the fixture namespace is caller-selectable.

Second, independent defect found while confirming: the gate compares ctx.account.sandbox === true literally and never calls getAccountMode. An account resolved as { account_id: 'sb', mode: 'sandbox' } is refused (reproduced: mode:sandbox account -> creatives: []). So the bridge gate and the comply-controller gate implement the same trust boundary two different ways, and adopters who migrate to mode per src/lib/server/account-mode.ts silently lose their fixtures.

Fix: require ctx.account !== undefined for the merge; use isSandboxOrMockAccount(ctx.account) rather than raw .sandbox === true; fire the construction warning unless both resolvers are wired.

2. Comply controller: env flags suppress the warning rather than gating anything (candidate-ecd76e4fd9107db2) — Medium

src/lib/testing/comply-controller.ts:338-355, :685-778. With no sandboxGate, reproduced:

force response: {"status":"completed","ok":true}
adapter invoked with: [{ media_buy_id: 'mb_victim', status: 'completed', ... }]

Note what ADCP_SANDBOX / ADCP_COMPLY_CONTROLLER_UNGATED at :757-763 actually do: they silence the warning. They gate nothing. That is the inverse of the pattern used correctly elsewhere in this repo.

Partial refutation worth recording: createAdcpServerFromPlatform bypasses controller.register and registers the tool itself behind a real resolved-account gate (from-platform.ts:2394-2620) — Phase 2 of #1435 is present, contradicting the stale "lands in a follow-up" comment at src/lib/server/account-mode.ts:11-13 (worth deleting). So this is scoped to adopters calling createComplyController(...).register(server) directly via @adcp/sdk/testing.

Two residual questions: the mcp == null branch at :2503-2508 deliberately falls back to the ungated controller.register(server), and it's unclear whether A2A dispatch routes through the MCP-registered handler (and therefore the gate) or has its own path. Both worth a targeted check.

Fix: make sandboxGate required, or fail closed in register() unless NODE_ENV ∈ {test, development} and an explicit ack env is set.

3. createTenantStore ref-routed default — needs your call, not a patch (candidate-bd22564f2d8ecfb6)

REFUTED as an undiscovered bug. src/lib/server/decisioning/tenant-store.ts:217-235 behaves exactly as the scan described, but test/lib/create-tenant-store.test.js:109-117 is a deliberate characterization test — its name is "default ('ref-routed') resolves a cross-tenant ref without checking the caller" — paired with four 'auth-scoped' tests proving the fail-closed alternative works. CHANGELOG.md:259 records that refAccess was added specifically to close this, and that the docs were corrected because they "previously implied resolve was always an isolation gate; it is not." The module header, the refAccess JSDoc, and the createTenantStore JSDoc all say so.

What is real is a docs gap with real teeth. skills/build-holdco-agent/SKILL.md — the file an agent-builder actually reads, whose front-matter promises "per-tenant data isolation" and whose § "Account resolution + tenant-isolation gate" presents createTenantStore as the answer — never mentions refAccess. Its "What the helper guarantees" list covers upsertRow / syncGovernanceRow / non-writability and omits the resolve carve-out entirely.

That matters more than "reads" implies: accounts.resolve is the account path for create_media_buy and update_media_buy too, so under the default a Meridian credential naming Pinnacle's operator resolves Pinnacle's Account (including its ctx_metadata) and every downstream handler runs scoped to Pinnacle — cross-tenant spend, not just cross-tenant reads. The upsert/syncGovernance gate doesn't cover this; those only guard sync_accounts / sync_governance.

Recommendation:

• Now: add the resolve carve-out to skills/build-holdco-agent/SKILL.md, in the "What the helper guarantees" list.
• Next major: flip the default to 'auth-scoped', keeping 'ref-routed' available behind an explicit opt-in. This is the one item here that breaks real agency-hub adopters, which is why it's your decision rather than a patch.

[bokelley]

Triage

Classification: Bug (security)
Bucket(s): library, skills
Status: ready-for-human

What the experts said:

• security-reviewer: Item 1/sub-A is a genuine exploitable path — buyer sends context.sandbox:true on an account-less tool, resolveAccountFromAuth absent means ctx.account === undefined, bridge passes and stamps live responses sandbox:true with fixture data; Items 1/sub-B + 2 are non-breaking and can land; Item 3 is a SKILL.md docs gap (code is correct)
• ad-tech-protocol-expert: Item 3 has the highest adoption impact — accounts.resolve is the account path for create_media_buy/update_media_buy, so the ref-routed default enables cross-tenant spend not just reads; Item 1 corrupts AdCP wire semantics for sandbox:true; Item 2 env-var pattern silences the only misconfiguration signal without adding a gate; all three confirmed genuine

My take: Three non-breaking sub-items are implementation-ready. Two decisions block the remaining work: the fix shape for Item 1/sub-A (breaking vs. warning extension), and whether to flip createTenantStore's ref-routed default next major. @bokelley's call on both.

Non-breaking implementation scope (ready):

• Item 1/sub-B — src/lib/server/create-adcp-server.ts:5373: replace .sandbox === true literal with isSandboxOrMockAccount(ctx.account) (already used in from-platform.ts:2331)
• Item 2 — src/lib/testing/comply-controller.ts:763: add one explicit sentence to warning text clarifying ADCP_COMPLY_CONTROLLER_UNGATED suppresses the warning only — does not create a request gate
• Item 3/SKILL.md — skills/build-holdco-agent/SKILL.md "What the helper guarantees": add bullet — "resolve path is not isolation-scoped by default; set refAccess: 'auth-scoped' when accounts.resolve backs mutating tools (create_media_buy, update_media_buy)"

Needs maintainer decision:

• Item 1/sub-A: Remove ctx.account === undefined admitting arm (breaking for storyboard runners) vs. extend construction warning to cover testControllerBridge != null && resolveAccountFromAuth === undefined (non-breaking)
• Item 3/default: Flip refAccess default to 'auth-scoped' — minor version bump + migration note; keeps 'ref-routed' as explicit opt-in for agency-hub adopters

---

Triaged by Claude Code. Session: https://claude.ai/code/${CLAUDE_CODE_REMOTE_SESSION_ID}

---

Generated by Claude Code

[bokelley]

PR #2433 opened for item 3 — implements option C from our discussion: refAccess becomes required with no default, rather than flipping the default to 'auth-scoped'.

Reasoning: flipping the default would break agency-hub adopters quietly (their calls start returning null and they debug it as a resolution bug). A required field breaks them at tsc, before they ship, and forces the one decision only they can make. Neither behavior changed. The helper also throws at construction so JS adopters and as any casts can't fall through silently — the type alone would only bind TS callers.

The doc gap is fixed in the same PR, since that was the larger half: skills/build-holdco-agent/SKILL.md promised "per-tenant data isolation" while never mentioning refAccess, and its "What the helper guarantees" list omitted resolve entirely.

Items 1 and 2 remain open here and are unaffected — both are ordinary fixes needing no decision:

• Test-controller bridge admits on a buyer-supplied sandbox marker when only resolveAccount is wired (and the #1784 warning doesn't fire in exactly that configuration). Also compares .sandbox === true literally instead of calling getAccountMode, so a mode: 'sandbox' account is refused.
• Comply controller's ADCP_SANDBOX / ADCP_COMPLY_CONTROLLER_UNGATED suppress the warning rather than gating anything.

Worth splitting those into their own issue so this one can close with #2433 — say the word and I'll do it.

[bokelley]

Triage

Classification: Bug (security) — re-engagement on MEMBER comment
Bucket(s): library
Status: ready-to-implement

What the experts said (carrying forward from prior analysis):

• security-reviewer: Items 1/sub-B and 2 are non-breaking and can land; sub-A is the admission gate, not just the literal comparison
• ad-tech-protocol-expert: Item 1 corrupts AdCP sandbox:true wire semantics; item 2 inverts the env-gate pattern used correctly elsewhere in this repo

My take: PR #2433 covers item 3 and the SKILL.md gap using Addresses, so this issue stays open for items 1 and 2 on merge — no split needed. @bokelley's "no decision needed" resolves the prior open question on sub-A fix shape.

Item 1 implementation scope (create-adcp-server.ts):

• :5007 gate: require ctx.account !== undefined before the bridge merge; fix the construction warning (:3745) to fire when testControllerBridge != null && resolveAccountFromAuth === undefined (currently silent for the resolveAccount-only wiring)
• :5373 gate: replace ctx.account.sandbox === true literal with isSandboxOrMockAccount(ctx.account) (already imported at from-platform.ts:2331)
• Tests: resolveAccount-only construction warning case; mode: 'sandbox' bridge inclusion

Item 2 implementation scope (comply-controller.ts:338–355, 757–763):

• register() should fail-closed unless NODE_ENV ∈ {test, development} and ADCP_COMPLY_CONTROLLER_UNGATED=1; env flags should gate the handler, not just suppress the warning — mirrors the ADCP_DECISIONING_ALLOW_INMEMORY_STATE=1 pattern
• Tests: unset env / partial env / full ack combinations

Changeset (patch) needed per item.

---

Triaged by Claude Code. Session: https://claude.ai/code/${CLAUDE_CODE_REMOTE_SESSION_ID}

---

Generated by Claude Code