feat(testing): widen bundled default-invariants per security review#753
Merged
Conversation
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) <noreply@anthropic.com>
bokelley
force-pushed
the
bokelley/widen-bundled-invariants
branch
from
b32db2f to
0591c9f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security-review on adcontextprotocol/adcp#2769 (which drops local assertion modules in favor of the bundled 5.9 defaults) flagged both
idempotency.conflict_no_payload_leakandcontext.no_secret_echoas shipping materially narrower semantics than the local versions they replace. Widens both to restore coverage before #2769 merges.Blocker per security-review:
idempotency.conflict_no_payload_leakflipped from an allowlist to a 5-field denylist in 5.9, turning key-reuse into an undetected read oracle for a seller inliningbudget/product_id/account_idat theadcp_errorroot. Fix is back to allowlist.governance.denial_blocks_mutationis untouched.Widened behavior
idempotency.conflict_no_payload_leakBefore: denylist of 5 field names (
payload,stored_payload,request_body,original_request,original_response). A seller puttingbudget: 5000, start_time: '2026-06-01...'on the envelope passed.After: allowlist of 7 envelope keys (
code,message,status,retry_after,correlation_id,request_id,operation_id). Any other top-level property on theadcp_errorenvelope fails with a sorted list of leaked field names.context.no_secret_echoBefore: scanned only
response.context; looked for verbatimoptions.auth_token/options.auth/options.secrets[]. Missed credentials echoed intoerror.message, audit fields, debug blocks.After:
.context)/\bbearer\s+[A-Za-z0-9._~+/=-]{10,}/iauthorization,api_key,apikey,bearer,x-api-key(case-insensitive)options.test_kit.auth.api_keyas a verbatim-secret source alongsideoptions.auth_token/.auth/.secrets[]"sk"don't false-positive on legitimate protocol vocabularyHousekeeping
Drop the now-unused
extractResponseContext/safeStringifyhelpers.Test coverage
16 new unit tests in
test/lib/storyboard-default-invariants.test.js:IDEMPOTENCY_CONFLICTcodes; passes on allowlist-only envelope; fails on non-allowlisted fields (with sorted diagnostic); still fails on the vestigial denylist names; deterministic output on multiple leaks.auth_token/secrets[]/test_kit.auth.api_keyecho in non-.contextlocations; fails suspect property names at any depth; walks arrays; ignores short (< 8 char) option values; ignores prose like "bearer of bad news".38/38 tests passing (up from 22). TypeScript clean.
Backward compatibility
Patch-level — assertion ids, registration API, and passing-case behavior unchanged. The strictening on main has been out for <24 hours in 5.9.0 and no adopters would see breakage in practice (they'd see more violations caught).
Related
🤖 Generated with Claude Code