Skip to content

refactor(webhook): extract verifyHmac helper and createWebhookVerifier factory#1007

Merged
aaight merged 1 commit intodevfrom
refactor/webhook-signature-dedup
Mar 23, 2026
Merged

refactor(webhook): extract verifyHmac helper and createWebhookVerifier factory#1007
aaight merged 1 commit intodevfrom
refactor/webhook-signature-dedup

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Mar 23, 2026

Summary

  • Extracted verifyHmac() generic helper in src/webhook/signatureVerification.ts — replaces ~76 lines of near-identical HMAC verification code across 4 functions with a single timing-safe implementation parameterized by algorithm, encoding, and optional prefix
  • Added createWebhookVerifier() factory in src/router/webhookVerification.ts — replaces 4 near-identical verify*WebhookSignature async callback functions with a single factory parameterized by header name, platform, extractor, project finder, and verify function
  • All 4 per-platform public APIs are unchangedverifyGitHubSignature, verifyTrelloSignature, verifySentrySignature, verifyJiraSignature and their router callbacks keep identical signatures and behavior
  • New tests — 10 verifyHmac() edge-case tests added to signatureVerification.test.ts; 9 createWebhookVerifier() factory tests added to webhook-signature.test.ts

Key decisions

  • VerifyHmacOptions.prefix is optional (defaults to '') — allows Sentry (no prefix) and Trello (no prefix, base64) to use the same helper without forcing an artificial empty-string prefix everywhere
  • platform typed as WebhookPlatform literal union — preserves the narrow type expected by resolveWebhookSecret while keeping the factory generic
  • Sentry's extractIdentifier reads c.req.param('projectId') — the existing unique-URL approach is preserved; the factory's findProject uses direct p.id === projectId matching instead of body parsing
  • Standalone helper functions keptextractTrelloBoardId, buildTrelloCallbackUrl, extractJiraProjectKey remain exported as-is (used by tests and the verifier configs)

Test plan

  • All 37 existing signatureVerification.test.ts tests pass
  • All 60 existing webhook-signature.test.ts tests pass
  • 10 new verifyHmac() edge-case tests pass
  • 9 new createWebhookVerifier() factory tests pass
  • Full unit test suite: 6386 tests across 333 files — all passing
  • npm run typecheck — zero errors
  • npm run lint — zero errors

Card: https://trello.com/c/69c1829c1337276ace4155f5

🤖 Generated with Claude Code

🕵️ claude-code · claude-sonnet-4-6 · run details

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 23, 2026

Codecov Report

❌ Patch coverage is 99.18699% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/router/webhookVerification.ts 98.70% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Clean, well-executed deduplication refactor. The verifyHmac helper and createWebhookVerifier factory correctly consolidate ~76 lines of near-identical HMAC logic and ~110 lines of near-identical verifier-callback boilerplate into parameterized, reusable code. All public APIs maintain identical signatures and behavior. CI is green (all 7 checks pass).

Notes

Sentry behavioral change (benign): The old verifySentryWebhookSignature went directly from projectId (URL param) → resolveWebhookSecret() without calling loadProjectConfig(). The new factory always calls loadProjectConfig() and findProject() first. In practice this is equivalent — resolveWebhookSecret returns null for non-existent projects anyway, and loadProjectConfig() is cached — but it's a difference worth noting.

Sentry error message change (cosmetic): The old Sentry code returned 'Missing Sentry-Hook-Signature header' while the new factory returns 'Missing signature header'. Since this string is only used for logging/debugging, the normalization is fine.

Both changes are intentional consequences of the factory pattern and don't affect runtime behavior.

🕵️ claude-code · claude-opus-4-6 · run details

@aaight aaight merged commit 0cbca8a into dev Mar 23, 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.

2 participants