Dispatcher webhook signatures have no timestamp or nonce — signed payloads are permanently replayable

[prodbycorne]

Overview

Two different, inconsistent signing schemes exist in this codebase for conceptually the same thing (a signed outbound webhook payload):

src/services/webhookSignature.js, used by webhookDispatcher.js for every pool.* event delivery:

function sign(secret, body) {
  ...
  const digest = crypto.createHmac('sha256', secret).update(payload).digest('hex');
  return `sha256=${digest}`;
}

This signature is a pure HMAC over the request body with no timestamp, nonce, or any other binding to a specific point in time. Anyone who ever observes one valid (body, X-SmartDrop-Signature) pair for a given webhook secret — for example, a subscriber's own logging/monitoring system that stores raw request bodies and headers, a compromised intermediate proxy, or a passive network observer if TLS were ever misconfigured/terminated somewhere unexpected — can replay that exact request to the same subscriber endpoint indefinitely, and the subscriber's own signature verification (using this exact library, since it's the reference implementation subscribers would naturally copy) would report it as perfectly valid every time, because nothing in the signature ever expires or becomes stale.

Compare this to src/services/webhook.js, used for price.alert deliveries (alertsService.fire() → webhook.deliver()), which does correctly bind a timestamp into the signed material:

function signPayload(secret, payload, timestamp = Date.now()) {
  const body = payloadBody(payload);
  return crypto.createHmac('sha256', secret).update(`${timestamp}.${body}`).digest('hex');
}

and exposes a verifySignature(secret, payload, signatureHeader, timestamp) that a subscriber could use to reject stale/replayed requests (though note: even here, nothing in this codebase's own subscriber-facing documentation or test suite demonstrates enforcing a maximum-age check on timestamp — the function accepts any timestamp value the caller passes in for verification, so the replay-prevention is only as good as whatever the subscriber independently chooses to enforce around it).

The practical result: subscribers to pool lifecycle events (pool.created, pool.assets_locked, etc. — the primary, documented webhook use case per the README) get a signature scheme with zero replay protection built in or even representable, while subscribers to price alerts get a meaningfully better (if still incompletely enforced) scheme — for no principled reason, since both are the same kind of "SmartDrop signs an outbound webhook" operation and should share one hardened implementation.

Requirements

• Unify webhook signing on a single scheme, used by both webhookDispatcher.js and alertsService/webhook.js, that includes a timestamp bound into the HMAC input (following the pattern already proven out in webhook.js's signPayload).
• Send the timestamp as its own header (as webhook.js already does via X-SmartDrop-Timestamp) alongside the signature for webhookDispatcher.js's deliveries too (currently buildHeaders in webhookDispatcher.js sends only X-SmartDrop-Signature, with no timestamp header at all).
• Update webhookSignature.js's verify() to require and check a timestamp, rejecting signatures older than a configurable maximum age (new env var, e.g. WEBHOOK_SIGNATURE_MAX_AGE_SECONDS, default 300) — matching standard practice (e.g. Stripe's webhook signature scheme, which this design otherwise resembles).
• Update the README's webhook documentation with the exact signing algorithm, the new timestamp header, and the recommended subscriber-side verification pseudocode including the replay-window check, so third-party bounty contributors integrating with this system get it right the first time.
• Since this is a breaking change to an already-documented (if still evolving) signature format, bump whatever the codebase uses to signal payload/signature schema version (add one if none exists, e.g. an X-SmartDrop-Signature-Version header) so subscribers built against the old scheme can detect the change explicitly rather than silently failing verification.

Acceptance Criteria

• webhookDispatcher.js includes a timestamp (bound into the HMAC, not just appended as an unsigned header) with every delivered webhook.
• webhookSignature.verify() rejects a structurally-valid signature whose bound timestamp is older than WEBHOOK_SIGNATURE_MAX_AGE_SECONDS.
• A captured, previously-valid (body, signature, timestamp) tuple replayed after the max-age window is correctly rejected by verify().
• services/webhook.js's alert-delivery signing and webhookDispatcher.js's pool-event signing now share one implementation (no more parallel, divergent schemes).
• README documents the unified scheme, the new headers, and the replay-window guarantee with example subscriber-side verification code.
• Existing tests (test/webhookSignature.test.js, test/webhookDispatcher.test.js) are updated to reflect the new required timestamp and continue to pass.

Additional Notes

More precise references

• src/services/webhookSignature.js:7-14 (sign): confirmed pure HMAC-SHA256 over payload alone (typeof body === 'string' ? body : JSON.stringify(body)), no timestamp input at all — signature is a deterministic function of (secret, body) only, exactly as the issue states.
• src/services/webhookSignature.js:16-30 (verify): confirmed it recomputes sign(secret, body) and compares via crypto.timingSafeEqual after a length check — correctly constant-time, but has literally no concept of a timestamp parameter to check staleness against, confirming there's nothing to even wire a max-age check into without a signature-format change.
• src/services/webhookDispatcher.js:27-35 (buildHeaders): confirmed only three headers plus signature are sent — X-SmartDrop-Event, X-SmartDrop-Delivery, X-SmartDrop-Signature — no timestamp header at all, confirming the issue's claim precisely.
• src/services/webhook.js:11-17 (signPayload) and :19-26 (buildSignatureHeaders): confirmed the alert-delivery path DOES bind ${timestamp}.${body} into the HMAC input and sends X-SmartDrop-Timestamp as a header (line 24) — confirmed as the superior reference pattern, exactly as the issue describes.
• src/services/webhook.js:28-37 (verifySignature): confirmed it takes a timestamp parameter and uses it to recompute the expected signature, but there is no max-age check anywhere in this function — it will happily verify a signature against a timestamp value from a year ago, exactly as the issue's own text flags ("nothing in this codebase's own... test suite demonstrates enforcing a maximum-age check"). This means even the "better" of the two existing schemes doesn't actually close the replay window on its own — it merely makes replay-prevention representable to a well-behaved subscriber who independently chooses to reject old timestamps, which the issue is careful to point out is not the same as replay prevention being enforced.
• src/services/webhookDispatcher.js:145-153 (deliverToWebhook) and :155-178 (dispatch): confirmed this is the single call site that would need to plumb a timestamp into buildHeaders, and that dispatch()'s Promise.all(targets.map(...)) means all targets for a given event share one occurredAt/timestamp if the timestamp is computed once per dispatch() call rather than once per individual delivery attempt — worth deciding explicitly whether the signed timestamp should be "time of dispatch" (shared across all targets, simpler) or "time of this specific HTTP attempt" (recomputed per retry, which matters since a retried delivery happens meaningfully later than the original attempt and per-attempt freshness is arguably more correct for anti-replay purposes — a retry that's already 10 minutes old signing with the original dispatch timestamp would itself immediately fail its own max-age check when finally delivered, which is a real, non-hypothetical interaction with the existing retry/backoff system in this same file).

Additional edge cases

• The retry interaction above is the most concrete new failure mode: webhookDispatcher.js's backoffMs/shouldRetry logic (lines 13-25) can schedule a retry base * factor^(attempts-1) ms later — with config.webhooks.maxAttempts and exponential backoff, a delivery could legitimately be retried well past a 300-second (5-minute) default WEBHOOK_SIGNATURE_MAX_AGE_SECONDS window. If the timestamp is fixed at original-dispatch-time, later legitimate retries would sign with an already-stale timestamp and get rejected by the subscriber's own max-age check — meaning this fix, if implemented naively, could break the existing, currently-working retry mechanism for any subscriber who actually implements the max-age check being newly documented/recommended. The fix must recompute the timestamp fresh on every individual delivery attempt (inside attempt(), not once in dispatch()), which the implementation sketch below reflects.
• A signature-version header (X-SmartDrop-Signature-Version) needs a clear rollout story: existing subscribers verifying with the old body-only HMAC will see their verification silently break the moment this ships (their old verify()-equivalent code checks a body-only signature; the new signed material is ${timestamp}.${body}, a different digest entirely) — this is explicitly called out in the issue as a breaking change, but worth being concrete: is there a transition period where both old and new signatures are sent simultaneously (e.g. two headers) to let subscribers migrate, or is it a hard cutover? The issue's acceptance criteria imply a hard cutover (no dual-signature transition mentioned) — worth raising as a product/rollout decision in the PR description rather than assuming.
• Clock skew between the SmartDrop server and a subscriber's server is a real, non-adversarial cause of false-positive rejections under a max-age check — worth a note in the README's recommended subscriber-side verification pseudocode about tolerating some symmetric skew (e.g. rejecting only if abs(now - timestamp) > maxAge, not just now - timestamp > maxAge, to also guard against a timestamp claiming to be from the future, which webhook.js's current signPayload/verifySignature doesn't address either since it accepts any timestamp value the caller provides).

Implementation sketch

1. Create a single shared module (e.g. promote webhook.js's signPayload/verifySignature logic into webhookSignature.js, or vice versa — pick one canonical location) exposing sign(secret, body, timestamp = Date.now()) returning sha256=<hmac of "${timestamp}.${body}">, and verify(secret, body, signatureHeader, timestampHeader, { maxAgeSeconds }) that: rejects if timestampHeader is missing/non-numeric; rejects if Math.abs(Date.now() - Number(timestampHeader)) > maxAgeSeconds * 1000 (symmetric skew check per the edge case above); recomputes the expected signature bound to the provided timestamp and compares via timingSafeEqual.
2. Update webhookDispatcher.js's buildHeaders to accept and sign a timestamp: buildHeaders(secret, body, eventType, deliveryId, timestamp) computed fresh via Date.now() at the top of attempt() (not passed down from dispatch()), and add 'X-SmartDrop-Timestamp': String(timestamp) and 'X-SmartDrop-Signature-Version': '2' to the returned headers object.
3. Update alertsService/webhook.js's callers to use the same shared sign/verify, removing the parallel implementation; add maxAgeSeconds enforcement to whatever currently calls verifySignature (if anything in this codebase actually verifies its own outbound signatures anywhere — if not, this is purely a subscriber-facing contract change and the enforcement lives in the README's example code, not in this codebase's own runtime).
4. Add WEBHOOK_SIGNATURE_MAX_AGE_SECONDS (default 300) to config.js, threaded into the shared verify()'s default options.
5. README: rewrite the webhook section's signing documentation with the unified header set (X-SmartDrop-Signature, X-SmartDrop-Timestamp, X-SmartDrop-Signature-Version), example subscriber verification code in JS showing the symmetric-skew check, and an explicit migration note for existing subscribers about the breaking change and its version header.

Test/reproduction plan

• sign(secret, body, ts) + verify(secret, body, sig, ts, { maxAgeSeconds: 300 }) immediately after signing → true.
• Same signature/timestamp pair verified after mocking Date.now() to be 301 seconds later → false (replay rejected).
• A signature with a timestamp claiming to be 301 seconds in the future → false (future-timestamp rejected, closing the skew-direction gap noted above).
• Simulate a delivery retried after backoff pushes it past maxAgeSeconds from the original dispatch time; assert the retry computes a fresh timestamp at attempt-time (not the original), so a legitimately-late retry still produces a currently-valid signature — this is the regression test proving the retry-interaction edge case above is actually handled, not just described.
• test/webhookSignature.test.js and test/webhookDispatcher.test.js updated to assert the new headers are present and required; confirm old tests asserting the previous 3-header shape are updated, not left passing against stale expectations.
• README example verification code, if feasible, run as an actual small test (copy-pasted into a test file) against real signed output from this codebase's sign(), to prove the documented subscriber-side snippet is correct and not just illustrative pseudocode that quietly doesn't work.

Cross-references

• Admin API key comparison uses non-constant-time string equality — timing side-channel #89 (constant-time admin key comparison) and Log redaction only matches top-level-ish substring keys — misses arrays, nested structures, and secrets embedded in URLs #94 (log redaction) both touch cryptographic-secret hygiene in this codebase; the unified signing module this issue creates is a good candidate to also house the constantTimeEquals helper proposed in Admin API key comparison uses non-constant-time string equality — timing side-channel #89's notes, consolidating all raw-secret-comparison logic in one place as Admin API key comparison uses non-constant-time string equality — timing side-channel #89's own audit requirement asks for.
• Webhook test endpoint can be abused as a blind SSRF timing/status oracle to probe internal network services #96 (webhook test SSRF oracle) shares the exact same webhookDispatcher.js file and attempt()/buildHeaders() functions this issue modifies — sequence carefully or land together to avoid merge conflicts on this file; also worth confirming the new timestamp header itself doesn't inadvertently become a new information-leak vector for Webhook test endpoint can be abused as a blind SSRF timing/status oracle to probe internal network services #96's timing-oracle concern (it shouldn't, since the timestamp reflects SmartDrop's own clock/dispatch time, not anything about the target).
• Fixed-window rate limiter allows boundary bursts and trusts req.ip without configuring Express trust proxy #90 (rate limiter) — no direct code interaction, but both issues touch config.js additions (WEBHOOK_SIGNATURE_MAX_AGE_SECONDS here, TRUST_PROXY_HOPS there) — trivial to land independently, just noting for anyone reviewing config-surface growth across this batch.

[Cyber-Mitch]

I'm a Software engineer with of 7 years of engineering experience, i write javascript codes, review and test. I will complete and submit the task within 2 days.