Skip to content

test(webhook-signature): add regression coverage for WEBHOOK_SIGNATURE_HEADER failure handling #1051 - #1170

Open
Chulexino wants to merge 2 commits into
RevoraOrg:masterfrom
Chulexino:fix/1051-webhook-signature-regression
Open

Chulexino wants to merge 2 commits into
RevoraOrg:masterfrom
Chulexino:fix/1051-webhook-signature-regression

Conversation

@Chulexino

Copy link
Copy Markdown

Overview

This PR adds a focused regression suite for the WEBHOOK_SIGNATURE_HEADER failure and empty-result paths in src/lib/webhookSignature.ts (issue #1051), so a silent behaviour change is caught in CI instead of reaching webhook receivers. Test-only change — the implementation file is byte-identical; the existing public contract is pinned, not modified.

Related Issue

Closes #1051

Changes

🧪 Webhook Signature Regression Suite

  • [ADD] src/lib/webhookSignature.regression.test.ts

    • Pins the empty-result exit at webhookSignature.ts:156 — extractSignatureFromHeaders returns undefined for absent, unset, empty-array, null, non-string and mixed-case header keys — plus the neighbouring normal path (candidate priority order, every supported header name, empty string returned verbatim).
    • Pins the parseExpiryTimestamp undefined branches at :214 (unset), :218 (invalid Date), :221 (NaN) and :234 (out-of-contract runtime types), together with the boundary rules: the 1e11 seconds/milliseconds cut-off (inclusive on the ms side), 0 kept distinct from "unset", and the anchored ^\d+$ numeric-string branch.
    • Pins the missing-signature failure contract: deterministic, non-leaking MISSING_SIGNATURE for empty/unset/empty-array/empty-string/custom-name-absent headers, ordered after the payload-size check and before the timestamp/replay check, and thrown by assertValidWebhookSignature.
    • Pins the verifyWebhookPayloadDualKey expiry coupling: unparseable / NaN nextSecretExpiry fails open (documented and deliberate), a passed deadline flips nextKeyExpired, Infinity never expires, and no nextSecret keeps the current key only.
    • Pins verifyWebhookPayload malformed signature-container boundaries (fail closed), including an equal-character-length multi-byte value that makes crypto.timingSafeEqual throw — the defensive catch absorbs it, so there is no unhandled RangeError.
    • Header-smuggling defence: only the first array entry is considered, so a later duplicate can never rescue an unusable first entry.
  • [ADD] docs/webhook-signature-header-regression.md

    • Security assumptions and abuse/failure paths, the deliberately pinned behaviours (fail-open expiry, Infinity, negative deadlines), the exercised cases with measured results, and residual risk for reviewers.
  • [MODIFY] package.json

    • Adds test:coverage:webhook-signature — the focused + surrounding suites with a --coverageThreshold of 95% on statements/lines/functions/branches for src/lib/webhookSignature.ts.

Verification Results

npx jest src/lib/webhookSignature.regression.test.ts
✅ 50/50 passed

npm run test:coverage:webhook-signature
✅ 190/190 passed (3 suites)
✅ src/lib/webhookSignature.ts — 100% statements, 97.29% branches, 100% functions, 100% lines (95% gate met, exit 0)

npx jest src/lib/webhookSignature.test.ts src/lib/webhookSignature.regression.test.ts \
  src/middleware/webhookAuth.test.ts src/services/webhookService.test.ts \
  src/services/__tests__/outboxDispatcher.test.ts src/services/__tests__/outboxHmacRotationService.test.ts --coverage=false
✅ 334/334 passed (6 suites)

npx eslint src/lib/webhookSignature.regression.test.ts
✅ 0 problems

npx tsc --noEmit
✅ Type-error count unchanged by this PR (250 before and after; none in the webhookSignature files)

npm run validate:alert-mappings
✅ OK: All 22 known alerts have mapping entries.
Acceptance Criteria Status
Cover the named behavior with focused automated tests, including the relevant success and failure paths ✅ 50 tests across the :156, :214, :218, :221, :234 evidence lines and their neighbouring normal paths
Preserve the existing public contract unless the change includes an explicit compatibility plan ✅ Test-only — src/lib/webhookSignature.ts is untouched (+686/-0 across 3 files)
Make error and boundary behavior observable and deterministic ✅ Exact MISSING_SIGNATURE codes/messages, fixed failure ordering, explicit boundary tables (1e11, 0 vs unset, Infinity)
Run the focused test file and the surrounding suite ✅ 50/50 focused, 334/334 surrounding (6 suites)
Run the repository's configured lint, type, build, or contract checks ✅ eslint clean on the new file, tsc count unchanged, alert mappings OK — the pre-existing repo-wide build/lint/audit failures are documented in docs/webhook-signature-header-regression.md §5
Include the exercised cases and results in the pull request description ✅ This section, plus the per-evidence-line results table in the docs

Security notes

  • A stripped signature header never means "skip verification" — MISSING_SIGNATURE is returned, never valid: true, and the failure payload carries no key material (asserted).
  • An oversized body is rejected before signature handling, and a missing timestamp cannot be used to dodge replay protection because the signature check still runs first.
  • Mis-wired callers that build header maps with mixed-case keys fail closed and deterministically.
  • Pinned, deliberately unchanged (follow-ups in the docs): an unparseable nextSecretExpiry fails open, Infinity means never-expiring, and negative numeric deadlines take the seconds branch.

…E_HEADER failure handling (RevoraOrg#1051)

Pins the explicit empty-result / failure branches named in issue RevoraOrg#1051 so a
silent behaviour change fails CI instead of reaching webhook receivers.
Test-only change: src/lib/webhookSignature.ts and its public contract are
untouched.

Exercised cases (50 new tests):
- :156 extractSignatureFromHeaders undefined result (absent, unset, empty
  array, null, non-string, mixed-case key) plus the neighbouring normal path
  (candidate priority, every supported header name, empty string verbatim)
- :214/:218/:221/:234 parseExpiryTimestamp undefined branches, the 1e11
  seconds/ms cut-off (inclusive on the ms side), 0 vs unset, anchored numeric
  string regex
- verifyWebhook missing-signature contract: deterministic, non-leaking
  MISSING_SIGNATURE, ordered after the payload-size check and before the
  timestamp/replay check
- verifyWebhookPayloadDualKey expiry coupling (pinned fail-open, expired flag,
  Infinity boundary)
- verifyWebhookPayload malformed signature-container boundaries (fail closed,
  equal-length multi-byte value absorbed by the timingSafeEqual guard)

Results:
- npx jest src/lib/webhookSignature.regression.test.ts -> 50/50 passed
- npm run test:coverage:webhook-signature (new script) -> 190/190 passed;
  src/lib/webhookSignature.ts = 100% stmts / 97.29% branches / 100% funcs /
  100% lines against a 95% gate
- surrounding suite (webhookSignature, webhookAuth, webhookService,
  outboxDispatcher, outboxHmacRotationService) -> 334/334 passed
- eslint on the new file: 0 problems; tsc error count unchanged (250 before and
  after, none in the webhookSignature files)

Pre-existing and reproduced with this branch stashed: the
src/routes/health.test.ts:1126 failure and the npm run audit:ci advisories.

Security assumptions, abuse paths and residual risk:
docs/webhook-signature-header-regression.md
@drips-wave

drips-wave Bot commented Sep 27, 2026

Copy link
Copy Markdown

@Chulexino Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

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.

Add regression coverage for WEBHOOK_SIGNATURE_HEADER failure handling

2 participants