fix: securing with webhook signature #16
Conversation
WalkthroughThe WebhooksService class was updated to support HMAC signature generation for webhook payloads using a secret key. The constructor and method signatures were refined for stronger typing, and a new method for creating HMAC signatures was added. Webhook requests now include an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebhooksService
participant WebhookEndpoint
Client->>WebhooksService: sendWebhooksEvents(webhooks, payload)
loop for each webhook in webhooks
WebhooksService->>WebhooksService: createHmacSignature(payload)
WebhooksService->>WebhookEndpoint: POST payload + x-webhook-signature header
WebhookEndpoint-->>WebhooksService: Response
end
WebhooksService-->>Client: PromiseFulfilledResult[]
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces HMAC signing for webhook requests by generating a signature header using the node:crypto module.
- Introduces a helper method to generate HMAC signatures for payloads.
- Updates the payload type definitions for enhanced type safety.
- Adjusts the options type passed to the service to include a webhook secret key.
| logger: Logger; | ||
| }; | ||
|
|
||
| type WebhookOptions = LoaderOptions & { subscriptions: string[] & { secretKey: string } }; |
There was a problem hiding this comment.
The type definition for WebhookOptions seems off; combining subscriptions as string[] with an embedded secretKey is confusing. Consider separating secretKey from subscriptions, e.g., 'LoaderOptions & { secretKey: string, subscriptions: string[] }'.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
plugins/webhooks/src/modules/webhooks/service.ts (4)
47-52: Add validation for payload before signature creation.The current implementation doesn't validate that the payload is properly serializable before creating the signature. This could lead to unexpected errors if the payload contains circular references or non-serializable values.
createHmacSignature(payload: Record<string, unknown>) { + // Ensure payload is serializable to prevent unexpected errors + try { + const stringifiedPayload = JSON.stringify(payload); + return Crypto + .createHmac("sha256", this.options.secretKey) + .update(stringifiedPayload) + .digest("hex"); + } catch (error) { + this.logger.error("Failed to create signature for payload", error); + throw new Error("Failed to create signature: payload contains non-serializable values"); + } - return Crypto - .createHmac("sha256", this.options.secretKey) - .update(JSON.stringify(payload)) - .digest("hex"); }
67-67: Improve webhook signature header with timestamp.The current implementation only includes the signature hex digest. Industry standard practice is to also include a timestamp to prevent replay attacks. This allows receivers to verify that the webhook was sent within an acceptable time window.
Consider enhancing the signature header format to include a timestamp:
- "x-webhook-signature": this.createHmacSignature(payload), + const timestamp = Math.floor(Date.now() / 1000).toString(); + "x-webhook-signature": `t=${timestamp},v1=${this.createHmacSignature({ ...payload, timestamp })}`,This would require updating the
createHmacSignaturemethod signature and implementation to handle the enhanced payload, but it significantly improves security against replay attacks.
88-88: Apply consistent documentation for method parameters.You've updated the method signature with stronger typing, but it lacks JSDoc documentation that describes the parameters and return values. This would be helpful for developers using this API.
+ /** + * Sends webhook events to multiple webhook endpoints + * @param webhooks - Array of webhook models to send events to + * @param payload - The data payload to send with the webhook + * @returns Promise resolving to an array of webhook send responses + */ public async sendWebhooksEvents(webhooks: WebhookModel[], payload: Record<string, unknown>) {
123-125: Update test webhook method to use the same payload type.The
testWebhookSubscriptionmethod still uses a simple object{ test: true }as payload, while other methods now expectRecord<string, unknown>. This is technically compatible, but it would be better to be explicit for consistency.- const response = await this.send(testData, { - test: true, - }); + const response = await this.send(testData, { + test: true, + timestamp: new Date().toISOString(), + } as Record<string, unknown>);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/webhooks/src/modules/webhooks/service.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugins/webhooks/src/modules/webhooks/service.ts (1)
plugins/webhooks/src/common.ts (1)
WebhookModel(4-4)
🔇 Additional comments (1)
plugins/webhooks/src/modules/webhooks/service.ts (1)
5-5:✅ Verification successful
Consider using the crypto module more securely.
Using the Node.js crypto module is appropriate, but you should consider the impact on environments like browsers if this code needs to run there.
🏁 Script executed:
#!/bin/bash # Check if the service might be used in browser environments echo "Checking for browser usage indications..." rg -i "browser|isomorphic|universal" --type ts echo "Checking for package.json browser field..." cat package.json 2>/dev/null | grep -i "\"browser\""Length of output: 277
No browser usage detected; Node.js crypto is safe here
We’ve searched for any browser/isomorphic/universal indicators in the TypeScript code and nobrowserfield exists in package.json. This service lives on the server side, so importingnode:cryptois appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/webhooks/README.md (1)
32-32: Prefer formatting consistency and avoid hardcoding secrets
The snippet currently reads:secretKey:"Your-webhook-secret"
- Add a space after the colon and a trailing comma for uniform JSON style:
- secretKey:"Your-webhook-secret" + secretKey: "Your-webhook-secret",
- (Optional) Instead of hardcoding, demonstrate reading the secret from an environment variable to follow security best practices:
- secretKey: "Your-webhook-secret", + secretKey: process.env.WEBHOOK_SECRET_KEY,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/webhooks/README.md(1 hunks)plugins/webhooks/src/modules/webhooks/service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/webhooks/src/modules/webhooks/service.ts
|
@dwene can you approve this :) |
| this.options = options; | ||
| this.subscriptions = options.subscriptions; | ||
| if (!this.options.secretKey) { | ||
| this.logger.warn('No secretKey provided for webhook signatures. Webhook security will be compromised.'); |
There was a problem hiding this comment.
The wording here is a little extreme haha. Can we tweak this a bit?
No webhook secret key was found. Please provide a secretKey in the plugin options. A default will be used instead, but this may reduce the security of webhook signatures.
An attempt to build a webhook with a x-signature-header
Summary by CodeRabbit