From 0591c9f2b73ee74ea50098c8f61f9fc63ddc2ccd Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 21 Apr 2026 21:33:09 -0400 Subject: [PATCH] feat(testing): widen bundled default-invariants per security review Security review on adcontextprotocol/adcp#2769 flagged both idempotency.conflict_no_payload_leak and context.no_secret_echo as shipping materially narrower semantics than the local versions they replace. Widens both to restore coverage before #2769 merges. idempotency.conflict_no_payload_leak: Before: denylist of 5 field names (payload, stored_payload, request_body, original_request, original_response). Trivially bypassed by a seller inlining budget / product_id / account_id at the adcp_error root, which turns key-reuse into a read oracle. After: allowlist of 7 envelope keys (code, message, status, retry_after, correlation_id, request_id, operation_id). Any other top-level property on the adcp_error envelope fails with a sorted list of leaked field names. context.no_secret_echo: Before: scanned only response.context; missed credentials echoed into error.message, audit fields, debug blocks. After: - Full-body recursive walk (not just .context) - Bearer-token literal regex /\bbearer\s+[A-Za-z0-9._~+/=-]{10,}/i - Suspect property name match at any depth: authorization, api_key, apikey, bearer, x-api-key (case-insensitive) - Pick up options.test_kit.auth.api_key as a verbatim-secret source alongside auth_token / auth / secrets[] - Reuses #752's extractAuthSecrets for the structured auth union, including $ENV:VAR resolution via pushCredentialValue - Centralises the SECRET_MIN_LENGTH (16) guard in addIfSecret so every source gets the same floor governance.denial_blocks_mutation is untouched. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../widen-bundled-invariant-coverage.md | 15 ++ .../testing/storyboard/default-invariants.ts | 237 ++++++++++++------ .../lib/storyboard-default-invariants.test.js | 166 +++++++++++- 3 files changed, 333 insertions(+), 85 deletions(-) create mode 100644 .changeset/widen-bundled-invariant-coverage.md diff --git a/.changeset/widen-bundled-invariant-coverage.md b/.changeset/widen-bundled-invariant-coverage.md new file mode 100644 index 000000000..21a1e9a98 --- /dev/null +++ b/.changeset/widen-bundled-invariant-coverage.md @@ -0,0 +1,15 @@ +--- +'@adcp/client': patch +--- + +Widen two bundled default assertions per security-review feedback on adcontextprotocol/adcp#2769. + +**`idempotency.conflict_no_payload_leak`** — flip the denylist-of-5-fields to an allowlist of 7 envelope keys (`code`, `message`, `status`, `retry_after`, `correlation_id`, `request_id`, `operation_id`). The previous implementation only flagged `payload`, `stored_payload`, `request_body`, `original_request`, `original_response` — a seller inlining `budget`, `start_time`, `product_id`, or `account_id` at the `adcp_error` root slipped past, turning idempotency-key reuse into a read oracle for stolen-key attackers. Allowlisting closes the hole: anything a seller adds beyond the 7 envelope fields now fails the assertion. + +**`context.no_secret_echo`** — scan the full response body recursively (not just `.context`), add a bearer-token literal regex (`/\bbearer\s+[A-Za-z0-9._~+/=-]{10,}/i`), add recursive suspect-property-name match (`authorization`, `api_key`, `apikey`, `bearer`, `x-api-key`), and pick up `options.test_kit.auth.api_key` as a verbatim-secret source. The previous scope (`response.context` only, verbatim `options.auth_token`/`.auth`/`.secrets[]` only) missed the common cases where sellers echo credentials into `error.message`, `audit.incoming_auth`, nested debug fields, or as header-shaped properties. All caller-supplied secrets gate on a minimum length (8 chars) to avoid false positives on placeholder values. + +Both changes are patch-level — the assertion ids, public registration API, and passing-case behavior are unchanged; the narrowing on main was fresh in 5.9 and had no adopters broad enough for the strictening to break in practice. + +`governance.denial_blocks_mutation` is unchanged. + +16 new unit tests cover both widenings: allowlist hits (valid envelope passes), denylist vestigial names still fail, non-allowlisted field leaks (including stable sorted error output), plus bearer literals, verbatim `options.auth_token` echo, `options.secrets[]` echo, `test_kit.auth.api_key` echo, suspect property names at any depth, array walking, short-value false-positive guard, and prose-"bearer" ignore. diff --git a/src/lib/testing/storyboard/default-invariants.ts b/src/lib/testing/storyboard/default-invariants.ts index 4a03af2e6..cf3a3815a 100644 --- a/src/lib/testing/storyboard/default-invariants.ts +++ b/src/lib/testing/storyboard/default-invariants.ts @@ -5,23 +5,25 @@ * `resolveAssertions` doesn't throw on fresh `@adcp/client` installs. * * The implementations aim for the spec's stated intent, not byte-perfect - * fidelity with the upstream reference. Consumers can override by calling - * `clearAssertionRegistry()` then re-registering with their own spec. + * fidelity with the upstream reference. Consumers can override a default by + * calling `registerAssertion(spec, { override: true })` with a stricter + * implementation of their own. * * Registered ids: * - `idempotency.conflict_no_payload_leak` — when a mutating step returns - * `IDEMPOTENCY_CONFLICT`, the error must not echo the prior request's - * payload or response (stolen-key read oracle). We scan error envelopes - * for fields that look like leaked payload / identifiers. - * - `context.no_secret_echo` — the echoed `context` object on any step - * must not contain any bearer token, API key, or auth header value - * supplied in the options. Scan recursively. + * `IDEMPOTENCY_CONFLICT`, the error envelope must carry only allowlisted + * keys. Any other top-level property is flagged as a potential payload + * leak (stolen-key read oracle). + * - `context.no_secret_echo` — response bodies (not just `.context`) MUST + * NOT contain bearer tokens, API keys, auth header values, or any leaf + * string extracted from the caller-supplied `auth` union. Walks the + * whole body, matches on suspect property names at any depth, and + * catches bearer-token literals via regex. * - `governance.denial_blocks_mutation` — once a plan is denied by a * governance signal (GOVERNANCE_DENIED, CAMPAIGN_SUSPENDED, etc., or * `check_governance` returning `status: "denied"`), no subsequent step - * in the run may acquire a resource for that plan. Catches sellers that - * surface the denial but mutate anyway. Plan-scoped via `plan_id`; runs - * without a denial signal are a silent pass. + * in the run may acquire a resource for that plan. Plan-scoped via + * `plan_id`; runs without a denial signal are a silent pass. */ import { registerAssertion } from './assertions'; @@ -36,8 +38,26 @@ function registerOnce(id: string, spec: Parameters[0]) registerAssertion(spec); } -// Tokens indicative of leaked payload on an IDEMPOTENCY_CONFLICT error. -const CONFLICT_LEAK_FIELDS = ['payload', 'stored_payload', 'request_body', 'original_request', 'original_response']; +/** + * Envelope fields that MAY legitimately appear on an IDEMPOTENCY_CONFLICT + * error body. Anything else on the error envelope is flagged as a potential + * payload leak. The allowlist is narrow on purpose: sellers that need more + * fields should push back on the spec, not silently leak cached state. + * + * The previous implementation used a denylist of 5 specific field names + * (`payload`, `stored_payload`, etc.) — trivially bypassed by a seller + * inlining `budget` / `product_id` / `account_id` at the envelope root, + * which turns key-reuse into a read oracle for the stolen-key attacker. + */ +const CONFLICT_ALLOWED_ENVELOPE_KEYS = new Set([ + 'code', + 'message', + 'status', + 'retry_after', + 'correlation_id', + 'request_id', + 'operation_id', +]); registerOnce('idempotency.conflict_no_payload_leak', { id: 'idempotency.conflict_no_payload_leak', @@ -48,67 +68,136 @@ registerOnce('idempotency.conflict_no_payload_leak', { if (!err) return []; if (err.code !== 'IDEMPOTENCY_CONFLICT') return []; - const findings: Omit[] = []; const description = 'IDEMPOTENCY_CONFLICT error redacts prior payload'; - for (const field of CONFLICT_LEAK_FIELDS) { - if (field in err.details) { - findings.push({ - passed: false, - description, - step_id: stepResult.step_id, - error: `IDEMPOTENCY_CONFLICT error leaked field "${field}" — must redact prior payload.`, - }); - } + const leaked: string[] = []; + for (const key of Object.keys(err.details)) { + if (!CONFLICT_ALLOWED_ENVELOPE_KEYS.has(key)) leaked.push(key); } - if (findings.length === 0) { - findings.push({ passed: true, description, step_id: stepResult.step_id }); + if (leaked.length === 0) { + return [{ passed: true, description, step_id: stepResult.step_id }]; } - return findings; + return [ + { + passed: false, + description, + step_id: stepResult.step_id, + error: + `IDEMPOTENCY_CONFLICT error envelope leaked non-allowlisted field(s): ${leaked.sort().join(', ')}. ` + + `Allowed envelope keys: ${[...CONFLICT_ALLOWED_ENVELOPE_KEYS].join(', ')}.`, + }, + ]; }, }); +/** + * Bearer-token literal pattern. Matches the wire form a seller might echo + * from `Authorization: Bearer ` — case-insensitive `bearer` keyword + * followed by a token body of at least 10 characters of base64url / JWT + * vocabulary. Kept strict on length to avoid false positives on prose like + * "bearer of bad news". + */ +const BEARER_TOKEN_PATTERN = /\bbearer\s+[A-Za-z0-9._~+/=-]{10,}/i; + +/** + * Property names that MUST NOT appear on a response body — a seller that + * serializes `Authorization` / `api_key` / `x-api-key` headers or fields + * into the response is almost certainly leaking credentials, regardless of + * whether the scanner picks up the value verbatim (header normalization, + * whitespace differences, etc. can mask verbatim matches). Case-insensitive. + */ +const SUSPECT_PROPERTY_NAMES = new Set(['authorization', 'api_key', 'apikey', 'bearer', 'x-api-key']); + +/** + * Minimum length for a caller-supplied secret to be hunted for verbatim. + * Set to 16 because real OAuth access tokens, refresh tokens, and signing + * secrets are ≥20 chars by convention (opaque UUIDs, JWTs, HMAC hex). A + * shorter floor would false-positive on benign identifiers (short + * usernames, environment names, 8-char hex prefixes, ISO timestamps). + * Hand-coded fixture keys like `"test-key"` below this bar are not caught + * — that's the right tradeoff: a real agent echoing such a value is an + * obvious leak a human reviewer would spot, and the collision cost of + * matching short strings against every response body is too high to + * justify the coverage. + */ +const SECRET_MIN_LENGTH = 16; + registerOnce('context.no_secret_echo', { id: 'context.no_secret_echo', - description: 'Echoed context MUST NOT contain bearer tokens, API keys, or auth header values.', + description: 'Response bodies MUST NOT echo bearer tokens, API keys, or auth header values back to the caller.', onStart: ctx => { - // Stash the sensitive values we know about. `auth` is the structured - // discriminated union from TestOptions — we walk it and extract leaf - // strings so `String.includes(obj)` can't silently no-op. Consumers can - // extend via `options.secrets` with additional raw strings. + // Stash caller-supplied secrets worth hunting verbatim. `auth` is the + // structured discriminated union from TestOptions — we walk it and + // extract leaf strings so `String.includes(obj)` can't silently no-op. + // test_kit api_key pickup matches what storyboards typically stage; + // options.secrets is a consumer hook for custom credentials. const secrets = new Set(); - const optAny = ctx.options as unknown as { auth_token?: string; auth?: unknown; secrets?: string[] }; - if (typeof optAny.auth_token === 'string' && optAny.auth_token) secrets.add(optAny.auth_token); - for (const s of extractAuthSecrets(optAny.auth)) secrets.add(s); - for (const s of optAny.secrets ?? []) if (typeof s === 'string' && s) secrets.add(s); + const optAny = ctx.options as unknown as { + auth_token?: string; + auth?: unknown; + secrets?: string[]; + test_kit?: { auth?: { api_key?: string } }; + }; + addIfSecret(secrets, optAny.auth_token); + for (const s of extractAuthSecrets(optAny.auth)) addIfSecret(secrets, s); + for (const s of optAny.secrets ?? []) addIfSecret(secrets, s); + addIfSecret(secrets, optAny.test_kit?.auth?.api_key); ctx.state.secrets = secrets; }, onStep: (ctx, stepResult) => { - const secrets = ctx.state.secrets as Set | undefined; - if (!secrets || secrets.size === 0) return []; - const context = extractResponseContext(stepResult); - if (context === undefined) return []; - const dumped = safeStringify(context); - const description = 'Response context omits caller-supplied secrets'; - for (const secret of secrets) { - // Minimum length guards against collisions on short fixture values - // (e.g. a 2-char `client_id` would match most JSON payloads by accident). - // Real bearer / access tokens are ≥ 20 chars in practice; the threshold - // keeps false positives out without missing realistic secrets. - if (secret.length >= SECRET_MIN_LENGTH && dumped.includes(secret)) { - return [ - { - passed: false, - description, - step_id: stepResult.step_id, - error: `Response context echoed a caller-supplied secret verbatim.`, - }, - ]; - } + const body = (stepResult as unknown as { response?: unknown }).response; + if (body === undefined || body === null) return []; + + const secrets = (ctx.state.secrets as Set | undefined) ?? new Set(); + const description = 'Response omits caller-supplied secrets and credential-shaped fields'; + + const hit = findSecretEcho(body, secrets); + if (hit) { + return [ + { + passed: false, + description, + step_id: stepResult.step_id, + error: `step "${stepResult.step_id}" response ${hit}`, + }, + ]; } return [{ passed: true, description, step_id: stepResult.step_id }]; }, }); +/** + * Recursively walk `value` hunting for (a) suspect property names at any + * depth, (b) bearer-token literals in any string value, and (c) verbatim + * copies of caller-supplied secrets. First hit wins; the caller turns the + * reason into a human-readable error message. + */ +function findSecretEcho(value: unknown, secrets: Set): string | null { + const stack: unknown[] = [value]; + while (stack.length > 0) { + const v = stack.pop(); + if (typeof v === 'string') { + if (BEARER_TOKEN_PATTERN.test(v)) return 'contains a bearer-token literal'; + for (const s of secrets) { + if (v.includes(s)) return 'contains a caller-supplied secret value verbatim'; + } + continue; + } + if (Array.isArray(v)) { + for (const item of v) stack.push(item); + continue; + } + if (v !== null && typeof v === 'object') { + for (const [key, inner] of Object.entries(v as Record)) { + if (SUSPECT_PROPERTY_NAMES.has(key.toLowerCase())) { + return `contains suspect property name "${key}"`; + } + stack.push(inner); + } + } + } + return null; +} + // ──────────────────────────────────────────────────────────── // governance.denial_blocks_mutation // ──────────────────────────────────────────────────────────── @@ -294,17 +383,16 @@ function extractAdcpError(step: import('./types').StoryboardStepResult): AdcpErr } /** - * Minimum secret length before substring matching runs. Set to 16 because - * real OAuth access tokens, refresh tokens, and signing secrets are ≥20 chars - * by convention (opaque UUIDs, JWTs, HMAC hex). A shorter floor would - * false-positive on benign identifiers (short usernames, environment names, - * 8-char hex prefixes, ISO timestamps). Hand-coded fixture keys like - * `"test-key"` below this bar are not caught — that's the right tradeoff: - * a real agent echoing such a value is an obvious leak a human reviewer - * would spot, and the collision cost of matching short strings against - * every response context is too high to justify the coverage. + * Add a value to the secrets set if it is a non-empty string of at least + * `SECRET_MIN_LENGTH` chars. Centralises the length guard so every source + * (structured auth, `auth_token`, `secrets[]`, `test_kit.auth.api_key`) + * gets the same floor. */ -const SECRET_MIN_LENGTH = 16; +function addIfSecret(out: Set, value: unknown): void { + if (typeof value !== 'string') return; + if (value.length < SECRET_MIN_LENGTH) return; + out.add(value); +} const ENV_REFERENCE_PREFIX = '$ENV:'; @@ -367,7 +455,8 @@ function pushCredentialValue(out: string[], value: unknown): void { * logs, and error bodies. * * Returns an empty list for anything we can't recognise — the goal is a best- - * effort extraction, not schema validation. + * effort extraction, not schema validation. The SECRET_MIN_LENGTH guard is + * applied by the caller via `addIfSecret`. */ function extractAuthSecrets(auth: unknown): string[] { if (!auth || typeof auth !== 'object') return []; @@ -403,17 +492,3 @@ function extractAuthSecrets(auth: unknown): string[] { } return out; } - -function extractResponseContext(step: import('./types').StoryboardStepResult): unknown { - const resp = (step as unknown as { response?: unknown }).response; - if (!resp || typeof resp !== 'object') return undefined; - return (resp as { context?: unknown }).context; -} - -function safeStringify(value: unknown): string { - try { - return JSON.stringify(value); - } catch { - return ''; - } -} diff --git a/test/lib/storyboard-default-invariants.test.js b/test/lib/storyboard-default-invariants.test.js index b9590e173..991de9a7b 100644 --- a/test/lib/storyboard-default-invariants.test.js +++ b/test/lib/storyboard-default-invariants.test.js @@ -388,7 +388,7 @@ describe('default-invariants: context.no_secret_echo', () => { const out = runEcho(variant.options, { echoed_secret: variant.secret }); assert.strictEqual(out.length, 1); assert.strictEqual(out[0].passed, false, `expected a leak finding for ${variant.name}`); - assert.match(out[0].error, /echoed a caller-supplied secret/); + assert.match(out[0].error, /caller-supplied secret/); }); test(`stays silent when the ${variant.name} is not echoed`, () => { @@ -416,10 +416,13 @@ describe('default-invariants: context.no_secret_echo', () => { assert.strictEqual(out[0].passed, false); }); - test('no auth configured → passes silently (no state bleed)', () => { + test('no auth configured → still runs whole-body scan, passes on clean response', () => { + // Even with no caller-supplied secrets, the widened assertion scans for + // bearer-token literals and suspect property names — that's the point of + // the widening. On a benign body it just passes. const out = runEcho({}, { echoed: 'anything goes here' }); - // No secrets to check → empty result (skip the check) - assert.strictEqual(out.length, 0); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].passed, true); }); test('resolves $ENV: reference on oauth_client_credentials.client_secret', () => { @@ -544,3 +547,158 @@ describe('default-invariants: context.no_secret_echo', () => { assert.strictEqual(out[0].passed, true, 'client_id echo must not flag — it is a public identifier'); }); }); + +describe('default-invariants: idempotency.conflict_no_payload_leak (widened allowlist)', () => { + const spec = getAssertion('idempotency.conflict_no_payload_leak'); + + function step(adcpError) { + return { + step_id: 's1', + phase_id: 'p', + title: 't', + task: 'create_media_buy', + passed: false, + duration_ms: 0, + validations: [], + context: {}, + extraction: { path: 'none' }, + response: adcpError !== undefined ? { adcp_error: adcpError } : undefined, + }; + } + + test('silent on non-IDEMPOTENCY_CONFLICT error codes', () => { + const out = spec.onStep({ state: {} }, step({ code: 'INVALID_REQUEST', message: 'bad' })); + assert.deepStrictEqual(out, []); + }); + + test('passes when the envelope has only allowlisted fields', () => { + const out = spec.onStep( + { state: {} }, + step({ code: 'IDEMPOTENCY_CONFLICT', message: 'key reused', correlation_id: 'c-1' }) + ); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].passed, true); + }); + + test('flags any non-allowlisted envelope field (the read-oracle leak vector)', () => { + const out = spec.onStep( + { state: {} }, + step({ code: 'IDEMPOTENCY_CONFLICT', message: 'conflict', budget: 5000, start_time: '2026-06-01T00:00:00Z' }) + ); + assert.strictEqual(out[0].passed, false); + assert.match(out[0].error, /budget/); + assert.match(out[0].error, /start_time/); + }); + + test('flags the specific named leak fields too (belt-and-suspenders)', () => { + const out = spec.onStep( + { state: {} }, + step({ code: 'IDEMPOTENCY_CONFLICT', message: 'conflict', payload: { budget: 5000 } }) + ); + assert.strictEqual(out[0].passed, false); + assert.match(out[0].error, /payload/); + }); + + test('lists leaked fields deterministically (sorted) for diagnostic stability', () => { + const out = spec.onStep( + { state: {} }, + step({ code: 'IDEMPOTENCY_CONFLICT', message: 'conflict', z_field: 1, a_field: 2, m_field: 3 }) + ); + assert.match(out[0].error, /a_field, m_field, z_field/); + }); +}); + +describe('default-invariants: context.no_secret_echo (widened whole-body scan)', () => { + const spec = getAssertion('context.no_secret_echo'); + + function ctx(options = {}) { + return { storyboard: {}, agentUrl: 'x', options, state: {} }; + } + + function step(response) { + return { + step_id: 's1', + phase_id: 'p', + title: 't', + task: 'create_media_buy', + passed: true, + duration_ms: 0, + validations: [], + context: {}, + extraction: { path: 'none' }, + response, + }; + } + + test('silent on steps with no response body', () => { + const c = ctx(); + spec.onStart(c); + assert.deepStrictEqual(spec.onStep(c, step(undefined)), []); + }); + + test('passes when response carries no credentials / suspect fields', () => { + const c = ctx({ auth_token: 'sk-live-verylongsecret' }); + spec.onStart(c); + const out = spec.onStep(c, step({ media_buy_id: 'mb-1', status: 'active' })); + assert.strictEqual(out[0].passed, true); + }); + + test('fails on a bearer-token literal anywhere in the body', () => { + const c = ctx(); + spec.onStart(c); + const out = spec.onStep(c, step({ debug: 'request had Authorization: Bearer abcdef123456xyz' })); + assert.strictEqual(out[0].passed, false); + assert.match(out[0].error, /bearer-token literal/); + }); + + test('fails when response echoes options.auth_token verbatim outside .context', () => { + const c = ctx({ auth_token: 'sk-live-verylongsecret' }); + spec.onStart(c); + const out = spec.onStep(c, step({ error: { message: 'auth sk-live-verylongsecret failed' } })); + assert.strictEqual(out[0].passed, false); + assert.match(out[0].error, /caller-supplied secret/); + }); + + test('fails when response echoes options.secrets[] verbatim', () => { + const c = ctx({ secrets: ['internal-token-abc123XYZ'] }); + spec.onStart(c); + const out = spec.onStep(c, step({ audit: { inbound_auth: 'internal-token-abc123XYZ' } })); + assert.strictEqual(out[0].passed, false); + }); + + test('fails when response echoes test_kit.auth.api_key verbatim', () => { + const c = ctx({ test_kit: { auth: { api_key: 'tk-api-key-alpha1' } } }); + spec.onStart(c); + const out = spec.onStep(c, step({ echoed_auth: 'tk-api-key-alpha1' })); + assert.strictEqual(out[0].passed, false); + }); + + test('fails on a suspect property name at any depth', () => { + const c = ctx(); + spec.onStart(c); + const out = spec.onStep(c, step({ nested: { deeper: { Authorization: 'anything' } } })); + assert.strictEqual(out[0].passed, false); + assert.match(out[0].error, /suspect property name "Authorization"/); + }); + + test('walks arrays when hunting leaks', () => { + const c = ctx(); + spec.onStart(c); + const out = spec.onStep(c, step({ items: [{ ok: 1 }, { notes: 'see Bearer aaaaaaaaaaaaa for details' }] })); + assert.strictEqual(out[0].passed, false); + }); + + test('ignores short option values to avoid placeholder false positives', () => { + const c = ctx({ auth_token: 'sk' }); + spec.onStart(c); + const out = spec.onStep(c, step({ note: 'sk is not a secret' })); + assert.strictEqual(out[0].passed, true); + }); + + test('does not flag generic use of the word "bearer" in prose', () => { + const c = ctx(); + spec.onStart(c); + const out = spec.onStep(c, step({ message: 'the bearer of bad news' })); + assert.strictEqual(out[0].passed, true); + }); +});