Constant-time webhook signatures in razorpay-node: when the fix accepts more than the bug did

The problem

Razorpay signs every webhook with HMAC-SHA256. The SDK's validateWebhookSignature(body, signature, secret) recomputes the HMAC and compares it with the signature from the request header. Issue #479 points at the comparison:

var expectedSignature = crypto.createHmac('sha256', secret).update(body).digest('hex');
return expectedSignature === signature;

A string comparison with === can stop at the first byte that differs, so in principle the response time leaks how much of a guessed signature is correct (CWE-208). Whether that is measurable over a network is debatable, but comparing MACs in constant time is cheap and is the standard practice. The same function also backs validatePaymentVerification, which ends with return validateWebhookSignature(payload, signature, secret), so payment signature checks share the comparison.

The proposed fix

The first version of the pull request decoded both values from hex and compared the bytes with Node's constant-time function:

// Use constant-time comparison to prevent timing attacks (CWE-208).
const a = Buffer.from(expectedSignature, 'hex');
const b = Buffer.from(signature, 'hex');
if (a.length !== b.length) return false;
return crypto.timingSafeEqual(a, b);

timingSafeEqual is the right tool. The question is what the code feeds into it.

What I found in review

Hex decoding is lenient

Buffer.from(str, 'hex') does not validate its input. It is case-insensitive, it stops at the first character that is not hex, and it silently drops a trailing odd nibble. Different strings therefore decode to the same bytes, and the check started accepting values that === had rejected. From the review:

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.

There was a second behavior change. The input guard isDefined() only rejects undefined, so a null signature reached Buffer.from and threw ERR_INVALID_ARG_TYPE where the old code returned false.

The smallest fix keeps the exact semantics of === by comparing the hex strings as bytes:

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);

The early length check only leaks the length, and the length of a SHA-256 hex digest is public anyway: 64 characters.

The new tests never ran

The two new it(...) blocks were added inside the callback of the existing it('validateWebhookSignature', ...), because that test's closing }); had moved down. Mocha collects tests before it runs them; an it registered from inside a running test is never executed and nothing reports it. I checked it with Mocha 9: an outer test containing two failing inner tests passes, and the inner tests never appear in the output. So the PR's "379 passing" did not include either new test.

I also pointed out that pull requests #465, #469 and #473 were already open for the same issue, so the maintainers may want to settle on one.

What happened next

The author pushed several commits. One of them contained unresolved merge conflict markers (<<<<<<< HEAD, =======, >>>>>>> HEAD@{1}) in both the library and the test file, which means the module no longer parsed, and it also changed an unrelated fail_existing type in lib/types/customers.d.ts. I flagged both, and noted that the regular expression in that version, /^[0-9a-fA-F]+$/, still accepted an upper-case signature.

The current head validates the input before decoding:

if (signature.length !== expectedSignature.length || !/^[0-9a-f]+$/.test(signature)) {
  return false;
}

Why it matters

Security fixes get less scrutiny than they deserve because the intent is obviously good. Here the intent was right and the core call was right, but the surrounding conversion widened what the function accepts, and the test that was supposed to prove the fix was silently skipped. Neither shows up in a green CI run.

Takeaways

• When you harden a comparison, keep the accepted input set identical. Compare the exact bytes you used to compare, or validate the format first.
• Buffer.from(value, 'hex') is a decoder, not a validator.
• In Mocha, a nested it is a test that does not exist. Count the tests the reporter prints, not the ones you wrote.
• Read the diff, not the summary comment. "Addressed all of your points" and the diff disagreed here.