Skip to content

Conversation

@javorosas
Copy link
Member

  • Type fixes on webhook signature validation
  • Implement local validation

@javorosas javorosas self-assigned this May 19, 2025
Copy link
Contributor

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

Implement local webhook signature validation and adjust types for improved developer experience

  • Added crypto as external dependency for bundling in Vite config
  • Expanded validateSignature to support string/Buffer/object payloads and perform local HMAC validation on Node and browser
  • Updated CHANGELOG.md with version 4.8.3 entry

Reviewed Changes

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

File Description
vite.config.ts Added crypto to external so native module isn’t bundled
src/tools/webhooks.ts Broadened payload types, implemented Node/browser local checks
CHANGELOG.md Documented new release with local webhook validation and fixes
Files not reviewed (1)
  • package.json: Language not supported
Comments suppressed due to low confidence (1)

src/tools/webhooks.ts:74

  • The new local validation branches (Node, React Native, browser) aren’t covered by existing tests. Add unit tests for each environment to ensure signature validation behaves as expected.
    // Validated locally

@javorosas javorosas force-pushed the FAC-1299/update/validate-signatures-locally branch from c0b4b1f to 391454e Compare May 19, 2025 13:44
@javorosas javorosas requested a review from Copilot May 19, 2025 13:44
Copy link
Contributor

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 enhances webhook signature handling by validating signatures locally in Node and browser environments, adjusts build configuration to accommodate crypto usage, and updates types and the changelog accordingly.

  • Update Vite config to treat crypto as an external dependency
  • Refactor validateSignature to support local HMAC verification for Node and browsers (React Native still proxies to the API)
  • Fix payload type definitions and document changes in the changelog

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
vite.config.ts Added 'crypto' to the external array to avoid bundling core crypto in Vite builds
src/tools/webhooks.ts Expanded validateSignature to accept string/Buffer, perform local HMAC in Node and browser, and adjusted type signature
CHANGELOG.md Added new 4.8.3 entry covering local validation and type fixes
Files not reviewed (1)
  • package.json: Language not supported
Comments suppressed due to low confidence (3)

src/tools/webhooks.ts:74

  • The new local validation branches for Node and browser are not covered by existing tests; add unit tests to verify each environment's logic.
// Validated locally

src/tools/webhooks.ts:84

  • [nitpick] The error message 'Invalid payload type' is generic; include the actual type received to make debugging easier.
throw new Error('Invalid payload type')

vite.config.ts:11

  • [nitpick] Marking crypto as external may omit a polyfill in browser builds; verify that client-side code still has access to the Web Crypto API or provide a fallback.
external: ['stream', 'crypto']

.update(payloadString)
.digest('hex')
.toLowerCase();
if (signature !== digest) {
Copy link

Copilot AI May 19, 2025

Choose a reason for hiding this comment

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

Using a direct string comparison for HMAC verification can be vulnerable to timing attacks; use crypto.timingSafeEqual on Buffer representations of the signatures.

Suggested change
if (signature !== digest) {
const signatureBuffer = Buffer.from(signature, 'hex');
const digestBuffer = Buffer.from(digest, 'hex');
if (
signatureBuffer.length !== digestBuffer.length ||
!crypto.timingSafeEqual(signatureBuffer, digestBuffer)
) {

Copilot uses AI. Check for mistakes.
@javorosas javorosas requested a review from Copilot May 19, 2025 14:00
Copy link
Contributor

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

Introduces local signature validation for webhook events in Node and web environments, updates type definitions, and adjusts build config.

  • Externalizes the crypto module in the Vite bundle.
  • Reworks validateSignature to perform HMAC checks locally, with fallbacks for React Native API calls.
  • Logs changes in CHANGELOG.md for version 4.8.3.

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
vite.config.ts Added crypto to Rollup external dependencies for bundling.
src/tools/webhooks.ts Reimplemented validateSignature with local HMAC in Node/browser and refined payload typing.
CHANGELOG.md Documented new local webhook validation and type fixes.
Files not reviewed (1)
  • package.json: Language not supported
Comments suppressed due to low confidence (2)

src/tools/webhooks.ts:84

  • [nitpick] The error message is generic; including the actual payload type or expected types could make debugging easier.
throw new Error('Invalid payload type');

src/tools/webhooks.ts:74

  • Consider adding unit tests to cover the new local validation logic across Node and browser branches to ensure the HMAC comparison works as intended.
// Validated locally

* @returns When the signature is valid, it returns the event object
*/
async validateSignature<T extends ApiEventType | '' = ''>(data: {
async validateSignature<T extends ApiEventType>(data: {
Copy link

Copilot AI May 19, 2025

Choose a reason for hiding this comment

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

Removing the default generic (| '' = '') may break consumers that relied on an empty-string fallback type; consider preserving backward compatibility or bumping the major version.

Suggested change
async validateSignature<T extends ApiEventType>(data: {
async validateSignature<T extends ApiEventType = ''>(data: {

Copilot uses AI. Check for mistakes.
@javorosas javorosas requested a review from Copilot May 19, 2025 14:57
Copy link
Contributor

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 introduces local webhook signature validation to improve security and performance by avoiding remote API calls, with accompanying type fixes and dependency adjustments.

  • Added "crypto" to external dependencies in vite.config.ts for local cryptographic processing.
  • Refactored Webhooks.validateSignature in src/tools/webhooks.ts to implement local validation logic for Node and browser environments, while retaining API-based validation for React Native.
  • Updated CHANGELOG.md to document the local validation improvements and type fixes.

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
vite.config.ts Added "crypto" to the external modules list to support local signature validation.
src/tools/webhooks.ts Refactored the signature validation function to include local validation logic and adjusted type definitions accordingly.
CHANGELOG.md Updated release notes to describe the new local webhook validation behavior and type adjustments.
Files not reviewed (1)
  • package.json: Language not supported

@javorosas javorosas merged commit 7295fb6 into main May 20, 2025
@javorosas javorosas deleted the FAC-1299/update/validate-signatures-locally branch May 20, 2025 08:45
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