Conversation
… fixtures, log sanitization Lands the three residuals security review flagged on PR #2522 before close. Admission-pressure baseline anchor (#2545): triple-threshold shape in webhook replay dedup sizing. 3x 24hr moving avg OR 2x 30-day P95 OR fixed 50-new-keyids-per-5-min ceiling, whichever triggers first. Prevents patient attackers from dragging baseline up during multi-week ramp-up — P95 over 30 days is dominated by baseline-traffic tail, not attack ramp. Signer-side conformance fixtures (#2546): new signer_input_rejection_vectors array with two vectors — top-level and deep-nested duplicate-key rejection. New signer_action_values enum defines reject-input-before-sign token. Test harness adds five structural assertions including byte-level check that fixture actually contains duplicate keys. security.mdx clause now references both fixtures and mandates interop harnesses exercise both. Key-name logging sanitization (#2547): step 14b mandates truncate-to-64-bytes, replace-non-printables-with-placeholder, cap-count-at-8 before logging duplicate key names. Closes attacker-controlled-byte log-injection channel without losing diagnostic value. Closes #2545, #2546, #2547. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Schema Link Check ResultsCommit:
|
Admission-pressure rule tightened to quadruple-threshold: 3x 24hr MA OR 2x 30-day P95 OR 1.5x 90-day P99 OR max(20, 10% of 30-day unique-keyid count) per 5-min, whichever triggers first. Alarm payload names triggering clause. 90-day P99 resists multi-month ramps; proportional ceiling auto-scales with operator size (1,000 for 10k-keyid verifier, 20 for 20-keyid verifier). Spec makes explicit these are operator-tunable defaults — published normative values would be an attacker oracle. Step 14b logging discipline tightened: truncate at first non-printable and log <sanitized:N> (was fixed <non-printable> which leaked position), truncate to 32 bytes at last complete UTF-8 codepoint boundary (was 64), cap count at 4 (was 8). Signer-side clause normatively requires same sanitization on signer-surfaced key names. Test vectors restructured from flat signer_input_rejection_vectors + signer_action_values into top-level signer_side object with action_values, rejection_vectors, positive_vectors. Rejection vectors expanded from 2 to 4 shape-classes: top-level, plain-nested, array-contained (real-world AdCP payload shape — packages[], creative_assets[]), three-deep. New positive_vectors array with signer-upstream-clean-input so reject-everything signers cannot trivially pass conformance. Test harness: 9 new structural assertions + scope-aware duplicate-key detector (walks JSON tracking object vs array nesting, correctly distinguishes duplicate keys at same object scope from same key name in distinct array-contained objects). Previously-flagged regex false-positive comment added at tests/webhook-hmac-vectors.test.cjs for the legacy heuristic check used as a secondary probe. Two further follow-ups filed separately: (a) CI reference-signer harness to actually enforce the 'interop harnesses MUST exercise both' spec language; (b) threshold publication debate — concrete numbers in spec vs operator guide. Closes #2545, #2546, #2547. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tor tuning guide + conformance harness Round-5 expert review landed three types of fixes in one PR: SECURITY FIX: step 14b sanitization was ASCII-only (< 0x20 || === 0x7F); bidi overrides (U+202E), line separators (U+2028/U+2029), zero-width chars (U+200B-200D), C1 controls (U+0080-009F), and BOM (U+FEFF) all passed through, reopening the log-injection channel the rule exists to close. Fixed: security.mdx step 14b now normatively enumerates the minimum non-printable set with rationale; implementations MAY extend but MUST NOT narrow; ASCII-only is called out as non-conformant. Reference signer implements via isNonPrintableCodepoint with the full range; conformance tests cover every class plus UTF-8 codepoint-boundary truncation for multi-byte sequences. STRUCTURE: reference signer extracted to tests/helpers/reference-webhook-signer.cjs with explicit CONTRACT BOUNDARY comment — fixtures ARE the contract, reference signer is one implementation. Test file split: webhook-hmac-vectors.test.cjs keeps structural/signature checks, webhook-hmac-signer-conformance.test.cjs runs signer harness. hasDuplicateKeyInAnyObjectScope delegates to findDuplicateKeyNames (single-sourced parser). 75 tests (52 structural + 23 signer-conformance). NON-NORMATIVE TUNING GUIDE: new docs/building/implementation/webhook-verifier-tuning.mdx replacing inline concrete threshold numbers in security.mdx. Contains starting-values table, first-30-days oracle warning (operators MUST tune within 30 days; implementations SHOULD randomize defaults ±30% on first deployment), baselining methodology, 10 attack scenarios (added: key-rotation storm, thin-history window, intermittent low-volume rule-shape limitation, onboarding-window-timed attack), tuning-adjustments table, 'DO NOT publish' three-audience split (public prohibited / NDA-attested permitted / internal runbooks required) replacing the prior too-absolute rule. security.mdx normative text drops concrete numbers and 'order' hints — only category names survive. Scenario 4 wording fixed (the previous version said 'What trips: clause (d)' then concluded 15 did NOT trip (d) — contradictory). Positive-vector tests assert result.error is absent — a signer emitting both signed_frame AND error is ambiguous response shape. Mintlify Note/Warning callouts at the tuning guide top replace bold-inline non-normative framing. Closes #2545, #2546, #2547, #2549, #2551. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ning-overdue hook, scenario adds Round-5 final expert review flagged 2 should-fix and 5 consider items. All landed here to avoid a return trip. Security should-fix: widen jitter from ±30% (1.86x spread) to log-uniform [0.5x, 2x] (4x spread) — narrower fleet distribution lets a disciplined attacker tune to 0.7x-published and stay under every jittered deployment. Scenario 9 corrected: previously said per-keyid + aggregate caps cover slow-drip; they don't — 1,440 new keyids/month is 0.014% of a 10M cap. Tuning guide now states operators with slow-drip in scope MUST layer application-level detection, not MAY. Protocol/security consider: normative error identifier 'duplicate_key_input' — cross-SDK stable for multi-SDK error dispatch; internal error-object shape stays implementation-defined. 'threshold_tuning_overdue' logging SHOULD on implementations gives the 30-day operator-tuning obligation a testable hook rather than operator-diligence-only. Protocol consider: added Scenario 11 (baseline reset at mature verifier — failover, cache rebuild, config change). Mirrors Scenario 8's thin-history posture at a deployment that was supposed to be mature; mitigation is baseline-state persistence across failover (Redis / shared dedup service — same infrastructure choice the spec already requires for cross-endpoint replay-cache scoping). Code consider: step 14b split into three bulleted sub-rules for scannability. Tuning guide Related section tightened with direct anchor to #webhook-replay-dedup-sizing plus scroll-target prose. Conformance harness asserts the normative error code. 75 tests still pass. Closes #2545, #2546, #2547, #2549, #2551. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley
added a commit
that referenced
this pull request
Apr 21, 2026
…2549) (#2609) * test(webhooks): wire signer-conformance harness into npm test (#2549) The harness (tests/webhook-hmac-signer-conformance.test.cjs) and fixtures (static/test-vectors/webhook-hmac-sha256.json signer_side block) were added by #2548 but the harness had no npm script wiring, so it wasn't on the default test path. A regression in the reference signer or a fixture miscategorization would not have failed CI. Adds test:hmac-signer-conformance npm script and slots it into the top-level test chain next to test:hmac-vectors. No code or fixture changes. Closes #2546 (fixture was landed by #2548). Closes #2549 (harness wired to CI by this change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(webhooks): actually run HMAC conformance on PRs + fixture guard Follow-up from code review on the initial PR. The previous commit added test:hmac-signer-conformance to the top-level npm test chain, but CI doesn't invoke npm test — it runs a hand-picked subset of scripts via build-check.yml and schema-validation.yml. The script was discoverable locally but unreachable on PRs. Turns out the sibling test:hmac-vectors had the same dead-wiring — landed as an npm script but never called by any workflow. Changes: - .github/workflows/schema-validation.yml: new step runs both test:hmac-vectors (verifier-side) and test:hmac-signer-conformance (signer-side). One gate, both sides of the duplicate-key contract. - tests/webhook-hmac-signer-conformance.test.cjs: assert the signer_side fixture block exists and both vector kinds are non-empty, so a future fixture refactor can't silently make the gate vacuous (the for-of loops would otherwise exit 0 with zero assertions). The npm script and test-chain slot-in from the prior commit stay — still useful for local discovery and for any future CI path that does invoke the top-level npm test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
bokelley
added a commit
that referenced
this pull request
Apr 21, 2026
…hook-reg 9421-required MUST (#2557, #2559) (#2616) Two normative SHOULD → MUST tightenings from the external 3.0 security review, landing in the 3.0 pre-GA window rather than 3.1.0 — SHOULD → MUST is a breaking tightening and cannot ride a minor release under the release-cadence policy. - Idempotency cache insert-rate limits MUST apply per-(agent, account), with RATE_LIMITED + retry_after when the ceiling is crossed. Recommended first-deployment ceiling: 60/sec sustained, 300/sec burst over rolling 10-second windows, 3,600/min. Sized consistent with the existing 100k per-keyid webhook replay cap and 1M per-keyid request replay cap. Closes a nonce-flood DoS amplification vector. Ceiling is tunable; sellers SHOULD NOT publish the exact configured value numerically (ecosystem-oracle risk). Closes #2559. - Sellers that support request signing MUST reject webhook-registration requests carrying push_notification_config.authentication over bearer with request_signature_required — structural defense against on-path mutators injecting or stripping the authentication block. Fully unsigned-only and fully signing-required sellers are unaffected; the breakage window is sellers with conditional signing posture. Closes #2557. - Broadens the request_signature_required row in the transport error taxonomy to cover payload-triggered signing requirements (not just required_for membership). - New negative test vector 027-webhook-registration-authentication-unsigned. - Runtime storyboard grading for the idempotency ceiling is tracked as follow-up #2615 — needs a burst-runner test-kit contract not yet defined. 3.0 GA coverage is spec-level MUST plus implementer attestation in universal/idempotency.yaml. - #2551 (threshold-number restructure) was already addressed in PR #2548; closed without further change. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Closes #2545, #2546, #2547, #2549, #2551. All five follow-ups to PR #2522's duplicate-key MUST-reject tightening, plus all findings from three rounds of expert review on this PR.
Security fix (round 5)
Step 14b sanitization — widen non-printable set from ASCII to Unicode. The prior implementation checked
< 0x20 || === 0x7Fonly. Security review flagged this reopens the log-injection channel: U+202E RIGHT-TO-LEFT OVERRIDE (reverses rendering in terminals/SIEMs), U+2028/U+2029 LINE/PARAGRAPH SEPARATOR (render as line breaks → row-injection), U+200B–200D ZERO WIDTH chars (invisible obfuscation), C1 controls U+0080–009F (terminal control semantics), and U+FEFF BOM all passed through.Fixed:
security.mdxstep 14b now normatively enumerates the minimum non-printable set with rationale for each class. Implementations MAY extend but MUST NOT narrow — ASCII-only is explicitly called out as non-conformant. Reference signer usesisNonPrintableCodepoint(); conformance tests unit-test every class plus codepoint-boundary-safe UTF-8 truncation for CJK and emoji.Admission-pressure — normative/non-normative split
security.mdxspecifies only the four categories (short-window ratio, medium-window ratio, long-window ratio, proportional ceiling) + MUST-configurable + alarm-names-triggering-clause. Concrete numbers moved to new Webhook Verifier Tuning Guide.Tuning guide contains:
Signer-side conformance — fixtures + reference implementation + CI harness
Fixtures (
webhook-hmac-sha256.jsonsigner_sideobject):action_valuesenum:reject-input-before-sign,sign-and-emitrejection_vectorscovers four shape-classes: top-level, plain-nested, array-contained (real-world AdCP shape), three-deeppositive_vectorshassigner-upstream-clean-inputso reject-everything signers cannot passReference signer (
tests/helpers/reference-webhook-signer.cjs, new):Module exports
findDuplicateKeyNames,hasDuplicateKeyInAnyObjectScope,isNonPrintableCodepoint,sanitizeKeyName,referenceSigner. Carries an explicit CONTRACT BOUNDARY comment — fixtures ARE the contract; this file is one implementation; downstream SDKs MUST match fixture behavior but MAY diverge in internal error shape. Also calls out that the tokenizer is a test-time shortcut — production signers MUST use their language's strict-parse escape hatch per step 14a.In-repo enforcement (
tests/webhook-hmac-signer-conformance.test.cjs, new):Exercises the reference signer against every
signer_sidevector. Asserts action, sanitized-keys surfaced, cap-at-4, positive-vectors-MUST-NOT-carry-error (ambiguous response shape prevention), signatures verify. Unit-testssanitizeKeyNameagainst every Unicode non-printable class and UTF-8 codepoint-boundary truncation.Test file split:
webhook-hmac-vectors.test.cjs(structural/signature) +webhook-hmac-signer-conformance.test.cjs(signer harness). 75 tests pass (52 + 23).Test plan
node --test tests/webhook-hmac-vectors.test.cjs tests/webhook-hmac-signer-conformance.test.cjs— 75 tests passnpm run test:unit && npm run typecheck— 587 tests, typecheck cleantests/helpers/reference-webhook-signer.cjsis the right explicit resolution of "fixtures are the contract, not the reference implementation"🤖 Generated with Claude Code