Skip to content

Conversation

@Yaniv-Nimni
Copy link
Contributor

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves webhook signature verification security by replacing string comparison with timing-safe comparison to prevent timing attacks.

  • Replaces direct string comparison with crypto.timingSafeEqual() for webhook signature verification
  • Adds proper buffer handling for base64 signature comparison
  • Includes test coverage for the updated webhook verification method

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
resources/webhooks.ts Updates verify method to use timing-safe comparison with buffer conversion
tests/webhooks.spec.ts Adds test case for webhook signature verification functionality

Comment on lines +48 to +49

return crypto.timingSafeEqual(hmac.digest(), signatureBuffer)
Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comparison will fail because hmac.digest() returns a Buffer while signatureBuffer is created from base64. Both should use the same encoding - either both as base64 strings or both as raw Buffers. Consider using hmac.digest('base64') or converting signatureBuffer to raw bytes.

Suggested change
return crypto.timingSafeEqual(hmac.digest(), signatureBuffer)
const computedSignatureBuffer = Buffer.from(hmac.digest('base64'), 'base64')
return crypto.timingSafeEqual(computedSignatureBuffer, signatureBuffer)

Copilot uses AI. Check for mistakes.
@ilyamerman ilyamerman changed the base branch from main to yanivs-webhook-pr August 5, 2025 13:23
@ilyamerman ilyamerman merged commit 02336dc into unit-finance:yanivs-webhook-pr Aug 5, 2025
0 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants