fix: use constant-time comparison for HMAC verification (#522)

[shunkica]

Replace non-constant-time === operator with crypto.timingSafeEqual() to prevent timing side-channel attacks on HMAC signature verification.

Fixes #522

Summary by CodeRabbit

• Bug Fixes
  • Strengthened the security of signature verification mechanisms.

[coderabbitai]

Walkthrough

Security fix in HmacSha1.verifySignature replacing direct string comparison with constant-time comparison using crypto.timingSafeEqual() to prevent timing attacks. Error handling added via try/catch for buffer length mismatches.

Changes

Cohort / File(s)	Summary
HMAC Signature Verification Security Fix src/signature-algorithms.ts	Replaced === operator with crypto.timingSafeEqual() for base64-decoded buffers in signature verification; wrapped in try/catch to handle length-mismatch errors gracefully

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Single file modified with focused change to one function
• Straightforward replacement of comparison method with established security API
• Error handling pattern is standard practice
• No API or control flow changes beyond internal logic

Poem

🐰 Hop, hop, hooray! The signatures are safe,
No timing whispers through the gate,
With constant-time checks, secrets don't slip,
Each comparison steady as a rabbit's hip,
HMAC's now secure from attacks so sly,
The vault stays locked 'neath the digital sky! 🔐

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "fix: use constant-time comparison for HMAC verification (#522)" directly and clearly describes the main change in the changeset. According to the raw summary, the modification replaces the non-constant-time === operator with crypto.timingSafeEqual() in HmacSha1.verifySignature, which is exactly what the title indicates. The title is concise, specific enough for scanning history, and contains no misleading or vague terminology.
Linked Issues Check	✅ Passed	The code changes directly satisfy all coding-related objectives from issue #522. The raw summary confirms that the equality check in HmacSha1.verifySignature was changed from res === signatureValue to crypto.timingSafeEqual() on base64-decoded buffers, addressing the requirement to use constant-time comparison for HMAC verification in src/signature-algorithms.ts. The implementation includes proper error handling via try/catch for length-mismatch scenarios, aligning with cryptographic best practices and CWE-208 without altering the public API.
Out of Scope Changes Check	✅ Passed	The changeset is entirely within scope and focused on the stated objectives. The only modification is in src/signature-algorithms.ts where the HMAC verification method was updated to use crypto.timingSafeEqual() instead of ===, which directly addresses the security issue described in #522. No alterations to exported or public entities are noted, and no unrelated changes appear to have been introduced.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 73db72d and ef08b40.

📒 Files selected for processing (1)

• src/signature-algorithms.ts (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.

Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.

📚 Learning: 2025-10-22T21:50:05.454Z

Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.

Applied to files:

• src/signature-algorithms.ts

🧬 Code graph analysis (1)

src/signature-algorithms.ts (1)

example/example.js (1)

• res (25-25)

🔇 Additional comments (1)

src/signature-algorithms.ts (1)

146-156: Security fix verified and complete. No additional action required.

The implementation correctly addresses CWE-208 timing attacks in HmacSha1 verification. Verification confirms:

• Only HmacSha1 uses manual comparison requiring the fix (RSA algorithms use built-in crypto.createVerify() which handles constant-time comparison internally)
• No other HMAC variants exist in the codebase (only HmacSha1)
• Test coverage validates the fix: test/hmac-tests.spec.ts includes 3 tests covering valid signatures, invalid keys, and create-validate roundtrips

The crypto.timingSafeEqual() implementation with base64-to-buffer conversion and try-catch exception handling is correct and complete.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}