Skip to content

feat(signing): add createWebhookVerifier factory for secure-by-default webhook verification#928

Merged
bokelley merged 2 commits into
mainfrom
claude/issue-926-createwebhookverifier-factory
Apr 25, 2026
Merged

feat(signing): add createWebhookVerifier factory for secure-by-default webhook verification#928
bokelley merged 2 commits into
mainfrom
claude/issue-926-createwebhookverifier-factory

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Closes #926

Summary

  • Adds createWebhookVerifier(options) factory to src/lib/signing/webhook-verifier.ts — mirrors createExpressVerifier for the webhook profile
  • Exports createWebhookVerifier and CreateWebhookVerifierOptions from @adcp/client/signing/server (and the aggregate @adcp/client/signing barrel)
  • verifyWebhookSignature is unchanged — required replayStore/revocationStore stay required on the low-level function
  • Adds 3 regression tests; the key one asserts replay is rejected when the caller omits explicit stores (proving the default store is shared across calls, not constructed per-call)
  • Changeset included (minor — new additive exports, no breaking changes)

Why a factory and not optional params on verifyWebhookSignature

The initial proposal in #926 suggested making replayStore/revocationStore optional directly on verifyWebhookSignature. Pre-triage expert review (security + code) flagged that as a blocker: defaulting stores inside a per-request function would silently defeat replay protection for any caller who constructs a new options object per request (the natural pattern for a stateless function). The createExpressVerifier precedent is structurally correct because it's a factory — stores live in closure scope. This PR follows the same pattern.

What was tested

  • npm run format:check — passes
  • node --test test/webhook-verifier-factory.test.js — 3/3 pass
  • node --test test/lib/webhook-signing-vectors.test.js test/webhook-verifier-error-codes.test.js — 34/34 pass (no regressions)
  • npm run build:lib — emits correctly; pre-existing tsconfig.lib.json TypeScript config warnings are repo-wide and unrelated to this change

Pre-PR review

  • code-reviewer: approved — no blockers; 3 nits addressed (test fixture guard, multi-replica JSDoc strength, revocationStore static-nature note)
  • security-reviewer: approved — factory pattern correctly fixes the replay-protection gap; Omit<> constraint prevents stale-store overwrite via spread; multi-replica MUST language strengthened per feedback

Nits noted (not fixed — out of scope)

  • createExpressVerifier in middleware.ts could similarly strengthen its multi-replica JSDoc (separate PR)

Session: https://claude.ai/code/session_01UWbckodGXcJVoH1Z1VpLjv


Generated by Claude Code

…t webhook verification

Mirrors createExpressVerifier: stores are instantiated once at creation time and
captured in closure scope so replay dedup works across all requests on the same
verifier instance. Fixes the request-signing / webhook-signing ergonomics parity
gap raised in #926.

Closes #926
Session: https://claude.ai/code/session_01UWbckodGXcJVoH1Z1VpLjv
Comment thread test/webhook-verifier-factory.test.js Fixed
@bokelley bokelley marked this pull request as ready for review April 25, 2026 00:20
… multi-replica JSDoc

- test/webhook-verifier-factory.test.js: remove unused `verifyWebhookSignature`
  import (github-code-quality bot)
- middleware.ts: tighten replayStore/revocationStore JSDoc to MUST language,
  matching the wording used on createWebhookVerifier so both factories carry the
  same multi-replica warning

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley merged commit 79abb02 into main Apr 25, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Webhook verifier should default replay store to in-memory (parity with request-signing)

2 participants