fix: use timing-safe HMAC comparison in Nodeless callback controller#584
Conversation
…cation and guard against missing NODELESS_WEBHOOK_SECRET
🦋 Changeset detectedLatest commit: c48661a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Anshumancanrock
changed the title
|
coverage: 64.19% (+0.08%) from 64.107% — Anshumancanrock:fix/nodeless-hmac-timing-safe-comparison into cameri:main |
|
hii @cameri , could you please review this PR? Thanks ! |
|
@copilot please review ! |
There was a problem hiding this comment.
Pull request overview
Improves security and robustness of the Nodeless webhook callback handler by switching to constant-time HMAC comparison and handling missing configuration without crashing.
Changes:
- Replace plain string comparison with
crypto.timingSafeEqualusingBuffercomparisons and signature format/length validation. - Add a 500 response path when
NODELESS_WEBHOOK_SECRETis unset to avoid runtime exceptions. - Add unit tests covering signature length mismatch, signature mismatch, and missing secret configuration; include a patch changeset.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/controllers/callbacks/nodeless-callback-controller.ts |
Implements timing-safe HMAC verification, validates signature format/length, and guards missing webhook secret. |
test/unit/controllers/callbacks/nodeless-callback-controller.spec.ts |
Adds unit tests for new verification/error paths (wrong length, mismatch, missing secret). |
.changeset/huge-trains-nail.md |
Records the patch-level change for release notes/versioning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
hii @cameri , Should I apply the same conditional route registration to the other payment processors (OpenNode, LNbits, Zebedee) ? They still have the in-controller check. |
|
@Anshumancanrock yes but in a separate ticket |
Move the processor-identity guard from the OpenNode, LNbits, and Zebedee callback controllers to the route layer. Each callback endpoint is now only registered when settings.payments.processor matches the processor, matching the pattern established for Nodeless in PR cameri#584. This eliminates unnecessary attack surface and removes redundant in-controller checks across all three remaining payment processors.
Description
The Nodeless callback controller was using a plain
!==string comparison to verify webhook HMAC signatures. This PR fixes it to usecrypto.timingSafeEqualwith proper Buffer comparison, matching how the OpenNode callback already handles it.Also adds a guard for when
NODELESS_WEBHOOK_SECRETis not set (previously this would throw aTypeErrorfromcreateHmac)Related Issue
Fixes #581
Motivation and Context
The OpenNode callback controller uses
timingSafeEqualfor its HMAC check, but the Nodeless controller was using!==. This is inconsistent and goes against the standard practice of using constant-time comparison for secret values.Also, if
NODELESS_WEBHOOK_SECRETwas unset (e.g. on a relay using a different payment processor), any POST to/callbacks/nodelesswould crash with aTypeErrorinstead of returning a clean error response and removes the valid HMAC value from the signature mismatch log (the old code logged{ expected, actual }, leaking the correct signature).How Has This Been Tested?
returns 403 when callback signature has wrong lengthreturns 403 when callback signature is a valid-length hex string but does not matchreturns 500 when NODELESS_WEBHOOK_SECRET is not configuredtsc --noEmit)Screenshots (if appropriate):
N/A
Types of changes
Checklist: