-
Notifications
You must be signed in to change notification settings - Fork 13
Prefer validating webhook events locally #71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
javorosas
commented
May 19, 2025
- Type fixes on webhook signature validation
- Implement local validation
There was a problem hiding this 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
cryptoas external dependency for bundling in Vite config - Expanded
validateSignatureto support string/Buffer/object payloads and perform local HMAC validation on Node and browser - Updated
CHANGELOG.mdwith 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
c0b4b1f to
391454e
Compare
There was a problem hiding this 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
cryptoas an external dependency - Refactor
validateSignatureto 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
cryptoas 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']
src/tools/webhooks.ts
Outdated
| .update(payloadString) | ||
| .digest('hex') | ||
| .toLowerCase(); | ||
| if (signature !== digest) { |
Copilot
AI
May 19, 2025
There was a problem hiding this comment.
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.
| if (signature !== digest) { | |
| const signatureBuffer = Buffer.from(signature, 'hex'); | |
| const digestBuffer = Buffer.from(digest, 'hex'); | |
| if ( | |
| signatureBuffer.length !== digestBuffer.length || | |
| !crypto.timingSafeEqual(signatureBuffer, digestBuffer) | |
| ) { |
There was a problem hiding this 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
cryptomodule in the Vite bundle. - Reworks
validateSignatureto perform HMAC checks locally, with fallbacks for React Native API calls. - Logs changes in
CHANGELOG.mdfor 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
src/tools/webhooks.ts
Outdated
| * @returns When the signature is valid, it returns the event object | ||
| */ | ||
| async validateSignature<T extends ApiEventType | '' = ''>(data: { | ||
| async validateSignature<T extends ApiEventType>(data: { |
Copilot
AI
May 19, 2025
There was a problem hiding this comment.
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.
| async validateSignature<T extends ApiEventType>(data: { | |
| async validateSignature<T extends ApiEventType = ''>(data: { |
There was a problem hiding this 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