[gopi-701]

Fixes #479

Problem

validateWebhookSignature() compares HMAC-SHA256 digests using ===, which is not constant-time. String comparison short-circuits at the first mismatched character, allowing an attacker to recover the signature byte-by-byte via timing measurements (CWE-208).

Root Cause

In lib/utils/razorpay-utils.js, line 102:

return expectedSignature === signature;  // vulnerable

Solution

Replace with crypto.timingSafeEqual() which performs constant-time comparison:

const a = Buffer.from(expectedSignature, 'hex');
const b = Buffer.from(signature, 'hex');
if (a.length !== b.length) return false;
return crypto.timingSafeEqual(a, b);

Testing

• All 379 tests pass
• Added 2 new tests:
  • Timing-safe comparison with correct and wrong signatures
  • Rejection of signatures with different lengths

Notes

• The secondary issue (fixed IV in AES-GCM) is already fixed on main
• No breaking changes — same API, same return values

[ratomirjovanovic]

This replaces === with crypto.timingSafeEqual in validateWebhookSignature, which is the right fix for #479. Two things need changing first. The new tests are nested inside an existing test, so Mocha never runs them. And decoding the incoming signature with Buffer.from(signature, 'hex') makes the check accept strings that the old === rejected. Also, #465, #469 and #473 are still open for the same issue, so the maintainers may want to settle on one of them.

[ratomirjovanovic]

Buffer.from(str, 'hex') is lenient. It stops at the first non-hex character and drops a trailing odd nibble, so with this change all of these now return true for a correct HMAC, where the old === returned false:

• the signature in upper case
• the signature followed by any non-hex text, e.g. sig + "zz"
• the signature followed by one extra hex character, e.g. sig + "f"

None of these let anyone forge a signature, but a verification function shouldn't start accepting values it rejected before. Separately, isDefined() only rejects undefined, so a null signature now throws ERR_INVALID_ARG_TYPE from Buffer.from where it used to return false.

If you compare the hex strings as bytes, you keep the exact semantics of === and still get a constant-time comparison:

const a = Buffer.from(expectedSignature, 'utf8');
const b = Buffer.from(String(signature), 'utf8');
if (a.length !== b.length) return false;
return crypto.timingSafeEqual(a, b);

Since validatePaymentVerification returns this function's result, the same change also covers payment signature checks. A test with correctSignature + "zz" and one with correctSignature.toUpperCase() would pin this behavior.

[ratomirjovanovic]

These two it(...) blocks are inside the callback of the existing it('validateWebhookSignature', ...), because that test's closing }); was moved down to line 136. Mocha doesn't run an it registered from inside a running test: it gets added to the root suite after that suite has already run, and nothing reports it. I checked this with mocha 9: an outer test containing two failing inner its passes, and the inner tests never show up in the spec reporter. So the 379 passing tests don't include either of the new ones.

Fix: close the existing test after its assert.ok(...) (line 107), then remove the extra }); at line 136. Also, the first new test is the same as the existing one with a different wrong signature, so it doesn't check that a constant-time comparison is used. You could drop it, or spy on crypto.timingSafeEqual and assert that it gets called.

[ratomirjovanovic]

Thanks for the update. The last commit (33c1dcb) still has unresolved merge conflict markers (<<<<<<< HEAD, =======, >>>>>>> HEAD@{1}) in lib/utils/razorpay-utils.js (line 110) and test/utils/razorpay-utils.spec.js (lines 124 and 154), so the module no longer parses and the tests can't run. The ======= side, with the length and hex checks before timingSafeEqual, looks like the version you meant to keep. The commit also changes fail_existing in lib/types/customers.d.ts, which seems unrelated to this fix. One small thing on that version: /^[0-9a-fA-F]+$/ still accepts an upper-case signature, which === rejected; /^[0-9a-f]+$/ would keep the old behaviour exactly.

[gopi-701]

Thanks @ratomirjovanovic for the detailed review.

Addressed all of your points:

• Resolved the merge conflicts in lib/utils/razorpay-utils.js and test/utils/razorpay-utils.spec.js (tests now parse and run)
• Moved the timing-safe tests to top-level so Mocha actually executes them
• Added strict lowercase hex + length validation before crypto.timingSafeEqual to avoid accepting signatures that the previous === would have rejected
• Removed unrelated changes

Ready for re-review when you get a chance.

[ratomirjovanovic]

Thanks, the conflict markers are gone and the lowercase hex and length checks look right.

Two things are still open:

1. The two new tests are still nested inside the existing it('validateWebhookSignature', ...) block, because the closing }); after its assert.ok(...) is missing (around line 108 of test/utils/razorpay-utils.spec.js). Mocha does not run an it that is registered inside a running test, so neither new case executes, and Subscription Verfication ends up nested too. Adding }); before the first new it should fix it.
2. The fail_existing change in lib/types/customers.d.ts is still part of the diff.

One smaller point: the new typeof secret !== 'string' check throws for a Buffer secret, which worked before since createHmac accepts it. If that is intentional, it may be worth a changelog note; otherwise dropping the secret part of that check keeps existing callers working. The second typeof signature !== 'string' check after the HMAC is now unreachable and can go.

[gopi-701]

Thanks again for catching that. Moved the two timing-safe tests to top-level so Mocha runs them properly.

[gopi-701]

Addressed the latest feedback:

• Compare the hex strings as UTF-8 byte buffers with timingSafeEqual (keeps exact === semantics, avoids Buffer.from(hex) leniency for mixed case/trailing chars), while still rejecting different lengths early
• Removed the unrelated fail_existing type change
• Kept backward compatibility for non-string secrets by coercing with String() as before; the redundant post-HMAC string check is removed

Tests are top-level and the conflicts are resolved. Ready for re-review.

[ratomirjovanovic]

Thanks, this looks right now. The two new tests run at the top level, the fail_existing change is gone, and comparing the hex strings as UTF-8 buffers after the length and lowercase-hex check keeps the old === semantics while making the comparison constant-time.

One small, non-blocking point on the tests: wrongSignature in the first new test is 61 characters, so the length check rejects it before timingSafeEqual runs. A wrong signature of the right length (for example the correct one with its last character changed) would exercise the comparison itself, and an upper-case copy of the correct signature would pin the case handling we discussed earlier.