Alert Webhook improvements #1703
Conversation
WalkthroughThe pull request introduces a series of changes across multiple modules to enhance the webhook and alert functionality. Updates include adding a version property to webhook schemas (with distinct defaults for different contexts), a new route for testing webhooks, modifications in the alert delivery service to handle versioned payloads, and adjustments to the export of utility functions. Additionally, new webhook schemas and a dedicated webhook module with signature verification and error handling are added, alongside enhanced documentation on alert setup and usage. Changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/webapp/app/v3/services/alerts/performTaskRunAlerts.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. packages/trigger-sdk/src/v3/index.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. apps/webapp/app/v3/services/alerts/deliverAlert.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
packages/trigger-sdk/src/v3/webhooks.ts (3)
7-12: Use more descriptive error messages for better debugging support.Currently, the
WebhookErrorclass only includes a generic message. You could provide an optional rendering of relevant context (e.g., missing header name, request info) to enhance diagnostic clarity without exposing sensitive information.
70-80: Check for strict content-type handling.When deriving the signature from a
Request, you may want to verify theContent-Typeheader (e.g.,application/json) to avoid confusion if the payload is unexpectedly encoded in another format.
85-87: Extend or revise the array-header error message.The message “Signature header cannot be an array” is valid, but consider clarifying which header is invalid and possibly log the raw header for debugging, ensuring sensitive data is masked.
packages/core/src/v3/schemas/webhooks.ts (1)
97-117: Consider DRY approach for environment, organization, and project shapes.Although you’ve extracted
deploymentCommonProperties, you might also unify or alias these environment/org/project schemas for both run and deployment objects, reducing repetition and potential drift.apps/webapp/app/v3/services/alerts/deliverAlert.server.ts (2)
316-412: Reduce repetitive code for v1/v2 payload construction.In the switch statement for run alerts, the logic for “v1” and “v2” shares many similarities. Extracting the shared fields into a helper can reduce repetition.
+ // Example: A small helper function returning a base payload object: + function buildBaseRunPayload(alert: FoundAlert) { ... } switch (webhookProperties.data.version) { case "v1": - const payload = { /* large object with many fields */ }; + const payload = { + ...buildBaseRunPayload(alert), + // v1-specific fields + }; ... case "v2": - const payload: RunFailedWebhook = { /* large object with many fields */ }; + const payload: RunFailedWebhook = { + ...buildBaseRunPayload(alert), + // v2-specific fields + }; ... }
909-948: Validate retry requirements for non-200 responses.Currently, if the request fails, the code throws an error and logs it. Confirm whether you need a retry mechanism for transient network failures (e.g., 502 status or timeouts).
apps/webapp/app/models/projectAlert.server.ts (1)
7-7: LGTM! Consider adding documentation.The version property is correctly defined as optional with a default value of "v1". Consider adding a comment to document the purpose of this property and its supported values.
+ /** Version of the webhook payload format. Defaults to "v1". */ version: z.string().optional().default("v1"),apps/webapp/app/routes/internal.webhooks.tester.ts (1)
41-44: Improve error handling.Consider logging the error stack trace for better debugging.
if (err instanceof Error) { - logger.error("[Webhook Internal Test] Error processing webhook:", { message: err.message }); + logger.error("[Webhook Internal Test] Error processing webhook:", { + message: err.message, + stack: err.stack + }); return json({ error: err.message }, { status: 400 }); }docs/troubleshooting-alerts.mdx (1)
6-9: Improve grammar and readability.Consider the following improvements:
- Remove redundancy in the list of events:
-We support receiving alerts for the following events: -- Run fails -- Deployment fails -- Deployment succeeds +We support alerts for: +- Run failures +- Deployment failures +- Deployment successes
- Add a comma for better readability:
-For the alert webhooks you can use the SDK to parse them. +For the alert webhooks, you can use the SDK to parse them.Also applies to: 41-41
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: This phrase is duplicated. You should probably use “fails Deployment” only once.
Context: ... alerts for the following events: - Run fails - Deployment fails - Deployment succeeds ## How to setup alerts <Step...(PHRASE_REPETITION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/webapp/app/models/projectAlert.server.ts(1 hunks)apps/webapp/app/routes/internal.webhooks.tester.ts(1 hunks)apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts(1 hunks)apps/webapp/app/v3/services/alerts/deliverAlert.server.ts(8 hunks)apps/webapp/app/v3/services/completeAttempt.server.ts(1 hunks)docs/troubleshooting-alerts.mdx(2 hunks)packages/core/src/v3/schemas/index.ts(1 hunks)packages/core/src/v3/schemas/webhooks.ts(1 hunks)packages/trigger-sdk/src/v3/index.ts(1 hunks)packages/trigger-sdk/src/v3/webhooks.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/troubleshooting-alerts.mdx
[grammar] ~7-~7: This phrase is duplicated. You should probably use “fails Deployment” only once.
Context: ... alerts for the following events: - Run fails - Deployment fails - Deployment succeeds ## How to setup alerts <Step...
(PHRASE_REPETITION)
[uncategorized] ~41-~41: Possible missing comma found.
Context: ...eps> ## Alert webhooks For the alert webhooks you can use the SDK to parse them. Here...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (10)
packages/trigger-sdk/src/v3/webhooks.ts (2)
49-53: Add reminder about test coverage.The
constructEventmethod is central to verifying and parsing incoming requests. Ensure it’s covered by unit or integration tests to validate real-world edge cases (e.g., empty payload, array header, incorrect signature).
153-155: Ensure no collisions or length mismatches in signature comparison.The
timingSafeEqualfunction correctly protects against timing attacks for equal-length signatures. However, confirm that the upstream logic never truncates or modifies the signature string in a way that might bypass length checks.packages/core/src/v3/schemas/webhooks.ts (2)
1-5: Schema coverage looks great.The usage of Zod for typed validation is well-structured. The reference to
RuntimeEnvironmentTypeSchema,RunStatus, andTaskRunErrorensures consistency across the codebase.
181-200: Expand tested union types with direct references.The discriminated union approach is solid. To improve test coverage, consider writing tests that feed each possible
typepayload (run failed, deployment success, deployment failed) intoWebhook.parseto ensure end-to-end reliability.apps/webapp/app/v3/services/alerts/deliverAlert.server.ts (1)
356-561: Double-check the webhookVersion property for “v2” payloads.When constructing the “v2” payload, you're setting
webhookVersion: "v1". This may be confusing or unintentional if you plan to track version differences downstream.packages/core/src/v3/schemas/index.ts (1)
13-13: LGTM!The new export statement for the webhooks module aligns with the PR objectives and follows the established pattern of module exports.
packages/trigger-sdk/src/v3/index.ts (1)
15-15: LGTM!The new export statement for the webhooks module aligns with the PR objectives and follows the established pattern of module exports.
docs/troubleshooting-alerts.mdx (1)
41-96: LGTM! The webhook example is well-implemented.The webhook implementation includes:
- Proper request method validation
- Comprehensive error handling
- Type-safe event processing
- Clear logging
🧰 Tools
🪛 LanguageTool
[uncategorized] ~41-~41: Possible missing comma found.
Context: ...eps> ## Alert webhooks For the alert webhooks you can use the SDK to parse them. Here...(AI_HYDRA_LEO_MISSING_COMMA)
apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts (1)
103-103: Verify the impact of hardcoding webhook version to "v2".The hardcoded version "v2" might affect existing webhook integrations. Please ensure that:
- The webhook handler maintains backward compatibility with "v1" payloads.
- Existing webhook integrations are notified about the version change.
Run the following script to check for existing webhook channels:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check for existing webhook channels that might be affected by the version change. # Test: Search for webhook channel creation. Expect: Existing webhook channels. ast-grep --pattern $'type: "WEBHOOK"'Length of output: 38
Action Required: Manual Verification for Webhook Version Impact
The automated search using the provided script did not yield any results for existing webhook channels. This lack of output makes it difficult to infer whether existing integrations are affected or if they are implemented using a different pattern.
- Manual Check Needed: Please review the webhook handler code to ensure that it supports backward compatibility with "v1" payloads.
- Notification Validation: Confirm that any consumers of webhook integrations have been properly notified regarding the switch to "v2".
- Further Investigation: If the integration setup is more complex or uses a different mechanism to register webhook channels, a manual code inspection or a targeted search for alternative patterns may be necessary.
apps/webapp/app/v3/services/completeAttempt.server.ts (1)
741-778: LGTM! Well-documented OOM error detection.The function is now exported, making it reusable across modules. The implementation thoroughly handles various OOM error cases with clear documentation.
🧰 Tools
🪛 Biome (1.9.4)
[error] 766-766: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🦋 Changeset detectedLatest commit: d033444 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
apps/webapp/app/v3/services/alerts/deliverAlert.server.ts (2)
206-206: Consider adding a deprecation notice period.The task run attempt alerts are being deprecated immediately. Consider adding a deprecation notice period to allow users to migrate to the new alert types.
Also applies to: 309-309, 665-665
316-412: Verify webhook version handling.The webhook version handling looks good but let's verify a few things:
- The
webhookVersionfield in v2 payloads is hardcoded to "v1". Is this intentional?- The error message for unknown versions could be more descriptive.
Apply this diff to improve error handling:
- throw new Error(`Unknown webhook version: ${webhookProperties.data.version}`); + throw new Error( + `Unsupported webhook version: ${webhookProperties.data.version}. ` + + `Supported versions are: v1, v2` + );Also applies to: 435-508, 519-604
.changeset/slow-olives-fix.md (1)
5-16: Enhance the documentation with error handling examples.The documentation would benefit from:
- Examples of error handling when the signature verification fails
- Description of the webhook payload structure for each event type
- Instructions for securely storing the webhook secret
packages/trigger-sdk/src/v3/webhooks.ts (3)
63-109: Add additional error handling for edge cases.While the error handling is good, consider handling these additional edge cases:
- Empty payload
- Malformed JSON before parsing
- Maximum payload size limit
Here's a suggested implementation:
async function constructEvent( request: ConstructEventOptions | Request, secret: string ): Promise<Webhook> { let payload: string; let signature: string; + const MAX_PAYLOAD_SIZE = 5 * 1024 * 1024; // 5MB limit if (request instanceof Request) { if (!secret) { throw new WebhookError("Secret is required when passing a Request object"); } + const contentLength = request.headers.get("content-length"); + if (contentLength && parseInt(contentLength) > MAX_PAYLOAD_SIZE) { + throw new WebhookError("Payload size exceeds limit"); + } + const signatureHeader = request.headers.get(SIGNATURE_HEADER_NAME); if (!signatureHeader) { throw new WebhookError("No signature header found"); } signature = signatureHeader; payload = await request.text(); } else { payload = request.payload.toString(); if (Array.isArray(request.header)) { throw new WebhookError("Signature header cannot be an array"); } signature = request.header.toString(); } + if (!payload.trim()) { + throw new WebhookError("Empty payload"); + } + // Verify the signature const isValid = await verifySignature(payload, signature, secret); if (!isValid) { throw new WebhookError("Invalid signature"); } // Parse and validate the payload try { + // Check for valid JSON structure before parsing + if (!payload.startsWith("{") && !payload.startsWith("[")) { + throw new WebhookError("Malformed JSON payload"); + } const jsonPayload = JSON.parse(payload); const parsedPayload = Webhook.parse(jsonPayload); return parsedPayload; } catch (error) { if (error instanceof Error) { throw new WebhookError(`Webhook parsing failed: ${error.message}`); } throw new WebhookError("Webhook parsing failed"); } }
111-158: Add input validation for signature and secret.Consider adding validation for:
- Minimum secret length
- Expected signature format (hex)
Here's a suggested implementation:
async function verifySignature( payload: string, signature: string, secret: string ): Promise<boolean> { try { if (!secret) { throw new WebhookError("Secret is required for signature verification"); } + // Validate minimum secret length (32 bytes recommended for HMAC-SHA256) + if (secret.length < 32) { + throw new WebhookError("Secret must be at least 32 characters long"); + } + + // Validate signature format (hex) + if (!/^[a-f0-9]+$/i.test(signature)) { + throw new WebhookError("Invalid signature format"); + } + // Convert the payload and secret to buffers const hashPayload = Buffer.from(payload, "utf-8"); const hmacSecret = Buffer.from(secret, "utf-8");
160-171: Consider using crypto.timingSafeEqual.While the current implementation is correct, Node.js's
crypto.timingSafeEqualis a battle-tested alternative that's specifically designed for this purpose.Here's a suggested implementation:
-// Timing-safe comparison to prevent timing attacks -function timingSafeEqual(a: string, b: string): boolean { - if (a.length !== b.length) { - return false; - } - - let result = 0; - for (let i = 0; i < a.length; i++) { - result |= a.charCodeAt(i) ^ b.charCodeAt(i); - } - return result === 0; -} +import { timingSafeEqual as cryptoTimingSafeEqual } from "crypto"; + +// Timing-safe comparison to prevent timing attacks +function timingSafeEqual(a: string, b: string): boolean { + if (a.length !== b.length) { + return false; + } + return cryptoTimingSafeEqual(Buffer.from(a), Buffer.from(b)); +}Note: If you need to maintain compatibility with non-Node.js platforms, the current implementation is a good fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.changeset/slow-olives-fix.md(1 hunks)apps/webapp/app/v3/services/alerts/deliverAlert.server.ts(10 hunks)apps/webapp/app/v3/services/alerts/performDeploymentAlerts.server.ts(1 hunks)apps/webapp/app/v3/services/alerts/performTaskRunAlerts.server.ts(1 hunks)packages/trigger-sdk/package.json(1 hunks)packages/trigger-sdk/src/imports/uncrypto-cjs.cts(1 hunks)packages/trigger-sdk/src/imports/uncrypto.ts(1 hunks)packages/trigger-sdk/src/v3/index.ts(1 hunks)packages/trigger-sdk/src/v3/webhooks.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/trigger-sdk/src/imports/uncrypto.ts
- packages/trigger-sdk/src/imports/uncrypto-cjs.cts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/trigger-sdk/src/v3/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/webapp/app/v3/services/alerts/performTaskRunAlerts.server.ts (1)
52-52: LGTM!The addition of
channelTypeto the alert payload aligns with the webhook improvements, ensuring consistent alert delivery across different channel types.apps/webapp/app/v3/services/alerts/performDeploymentAlerts.server.ts (1)
52-52: LGTM!The addition of
channelTypeto the alert payload is consistent with the webhook improvements and matches the changes inperformTaskRunAlerts.server.ts.apps/webapp/app/v3/services/alerts/deliverAlert.server.ts (3)
9-16: LGTM!The new imports from
@trigger.dev/core/v3support the webhook improvements by providing strongly-typed webhook payloads.
909-948: LGTM! Robust webhook delivery implementation.The generic
#deliverWebhookmethod provides a type-safe way to deliver webhooks with proper signature verification and error handling. The 5-second timeout is a good practice to prevent hanging connections.
1088-1112: LGTM! Rate limiting for non-webhook channels.The rate limiting logic is well-implemented:
- Only applies to non-webhook channels
- Includes proper error handling
- Logs rate limit events for monitoring
packages/trigger-sdk/src/v3/webhooks.ts (1)
27-61: Well-documented interface with clear examples!The Webhooks interface and its implementation are well-structured with:
- Clear JSDoc documentation
- Practical usage examples
- Proper type definitions
| @@ -56,6 +56,7 @@ | |||
| "slug": "^6.0.0", | |||
| "terminal-link": "^3.0.0", | |||
| "ulid": "^2.3.0", | |||
| "uncrypto": "^0.1.3", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the stability and security of the uncrypto package.
The package is at version 0.1.3 which suggests it's in early development. Since it's used for security-critical webhook signature verification, we should ensure it's stable and secure.
Additionally, consider pinning the exact version (0.1.3 instead of ^0.1.3) to avoid potential breaking changes, as 0.x versions may not follow semantic versioning strictly.
- "uncrypto": "^0.1.3",
+ "uncrypto": "0.1.3",🏁 Script executed:
#!/bin/bash
# Description: Check package details and security advisories for uncrypto
# Check npm for latest versions and download stats
curl -s https://registry.npmjs.org/uncrypto | jq '{latest: .["dist-tags"].latest, versions: .versions, time: .time}'
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "uncrypto") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 16219
🌐 Web query:
What is the latest version of uncrypto? Is it actively maintained and secure for production use?
💡 Result:
The latest version of uncrypto is 0.1.3, as shown in its package.json file[5]. This cross-runtime cryptographic library provides a unified API for Web Crypto and Node.js Crypto modules[1][4].
Maintenance Status
- The GitHub repository shows active maintenance with recent commits (as of June 2023)[7] and clear contribution guidelines[1].
- Development requirements include modern tooling (Node.js v15+, pnpm)[1].
- Automated testing via Vitest is implemented[5].
Security Considerations
-
Underlying Security:
- Uses platform-native crypto implementations (Web Crypto API/Node.js Crypto)[4][7]
- Inherits security from battle-tested systems like CommonCrypto on macOS/iOS[4]
-
Production Readiness:
- Implements secure key handling by keeping secrets outside JavaScript execution context[4]
- Aligns with Node.js' stabilized Permissions Model for defense-in-depth security[8]
- Requires secure contexts (HTTPS/localhost) in browsers[1]
-
Limitations:
- Version 0.x indicates pre-1.0 status (caution advised for critical systems)[5]
- No recent security audits mentioned in available documentation
While not explicitly marked "production-ready," its design leverages secure patterns from modern JavaScript ecosystems[1][8]. For non-critical use cases requiring cross-platform crypto operations, it appears suitable when used with standard security precautions like HTTPS and updated dependencies[4][8].
Citations:
- 1: https://github.com/unjs/uncrypto
- 2: https://community.letsencrypt.org/t/making-sure-auto-renewal-is-enabled-with-certbot/205256
- 3: https://www.timesys.com/security/manufacturing-protection-provisioning-iot-device-secrets/
- 4: https://webkit.org/blog/7790/update-on-web-cryptography/
- 5: https://github.com/unjs/uncrypto/blob/main/package.json
- 6: https://status.solana.com
- 7: easy creation and validation of JWTs unjs/uncrypto#17
- 8: https://openjsf.org/blog/heres-a-summary-of-the-key-updates
- 9: http://www.uniindia.com/unco-coin-listing-on-january-20th-2025-transforming-meme-coins-with-ethereum/business-wire-india/news/3369063.html
- 10: http://status.base.org
Uncrypto Dependency Verification & Version Pinning
The new dependency uncrypto at version ^0.1.3 has been verified:
- Latest Version & Maintenance: The npm registry confirms the latest version is 0.1.3, and the repository shows active maintenance with recent activity.
- Security Status: No vulnerabilities have been reported in the security advisories for this package.
- Recommendation: As this package (being in its 0.x phase) might introduce breaking changes, it's advisable to pin the version explicitly to "0.1.3" rather than using the caret range. This minimizes any unexpected impacts on our webhook signature verification code.
Actionable Change:
- "uncrypto": "^0.1.3",
+ "uncrypto": "0.1.3",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "uncrypto": "^0.1.3", | |
| "uncrypto": "0.1.3", |
| /** Header name used for webhook signatures */ | ||
| const SIGNATURE_HEADER_NAME = "x-trigger-signature-hmacsha256"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the security header naming convention.
The header name x-trigger-signature-hmacsha256 follows good practices by:
- Using the
x-prefix for custom headers - Including the hash algorithm in the name
Let's verify if this header name is consistently used across the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Check for consistent usage of the signature header name
rg -i "trigger.*signature" --type tsLength of output: 372
Inconsistent header naming found in the codebase.
- The constant
SIGNATURE_HEADER_NAMEis defined as"x-trigger-signature-hmacsha256"inpackages/trigger-sdk/src/v3/webhooks.tsand used as such inapps/webapp/app/v3/services/alerts/deliverAlert.server.ts. - However, in
apps/webapp/app/services/runs/deliverRunSubscription.server.ts, it is referenced as"X-Trigger-Signature-256", which deviates from the defined convention. - Action Required: Update the header name in
deliverRunSubscription.server.tsto either import and useSIGNATURE_HEADER_NAMEor change it to"x-trigger-signature-hmacsha256"for consistency and adherence to common security practices.
| if (request instanceof Request) { | ||
| if (!secret) { | ||
| throw new WebhookError("Secret is required when passing a Request object"); | ||
| } | ||
|
|
||
| const signatureHeader = request.headers.get(SIGNATURE_HEADER_NAME); | ||
| if (!signatureHeader) { | ||
| throw new WebhookError("No signature header found"); | ||
| } | ||
| signature = signatureHeader; | ||
|
|
||
| payload = await request.text(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security headers validation.
Consider validating additional security headers to prevent replay attacks:
x-trigger-timestampfor request freshnessx-trigger-idempotency-keyfor idempotency
Here's a suggested implementation:
if (request instanceof Request) {
if (!secret) {
throw new WebhookError("Secret is required when passing a Request object");
}
+ // Validate timestamp to prevent replay attacks
+ const timestamp = request.headers.get("x-trigger-timestamp");
+ if (!timestamp) {
+ throw new WebhookError("No timestamp header found");
+ }
+ const requestTime = parseInt(timestamp);
+ if (isNaN(requestTime)) {
+ throw new WebhookError("Invalid timestamp");
+ }
+ const now = Date.now() / 1000;
+ if (Math.abs(now - requestTime) > 300) { // 5 minutes tolerance
+ throw new WebhookError("Request too old");
+ }
+
+ // Validate idempotency key
+ const idempotencyKey = request.headers.get("x-trigger-idempotency-key");
+ if (!idempotencyKey) {
+ throw new WebhookError("No idempotency key found");
+ }
+
const signatureHeader = request.headers.get(SIGNATURE_HEADER_NAME);
if (!signatureHeader) {
throw new WebhookError("No signature header found");
}
signature = signatureHeader;
payload = await request.text();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (request instanceof Request) { | |
| if (!secret) { | |
| throw new WebhookError("Secret is required when passing a Request object"); | |
| } | |
| const signatureHeader = request.headers.get(SIGNATURE_HEADER_NAME); | |
| if (!signatureHeader) { | |
| throw new WebhookError("No signature header found"); | |
| } | |
| signature = signatureHeader; | |
| payload = await request.text(); | |
| if (request instanceof Request) { | |
| if (!secret) { | |
| throw new WebhookError("Secret is required when passing a Request object"); | |
| } | |
| // Validate timestamp to prevent replay attacks | |
| const timestamp = request.headers.get("x-trigger-timestamp"); | |
| if (!timestamp) { | |
| throw new WebhookError("No timestamp header found"); | |
| } | |
| const requestTime = parseInt(timestamp); | |
| if (isNaN(requestTime)) { | |
| throw new WebhookError("Invalid timestamp"); | |
| } | |
| const now = Date.now() / 1000; | |
| if (Math.abs(now - requestTime) > 300) { // 5 minutes tolerance | |
| throw new WebhookError("Request too old"); | |
| } | |
| // Validate idempotency key | |
| const idempotencyKey = request.headers.get("x-trigger-idempotency-key"); | |
| if (!idempotencyKey) { | |
| throw new WebhookError("No idempotency key found"); | |
| } | |
| const signatureHeader = request.headers.get(SIGNATURE_HEADER_NAME); | |
| if (!signatureHeader) { | |
| throw new WebhookError("No signature header found"); | |
| } | |
| signature = signatureHeader; | |
| payload = await request.text(); | |
| } |
| import { Webhook } from "@trigger.dev/core/v3"; | ||
| import { subtle } from "../imports/uncrypto.js"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the crypto implementation choice.
Based on the past review discussion about crypto module choices, we should verify that the uncrypto.js implementation provides the necessary compatibility across different platforms (Node.js, Bun, Deno, CF Workers, Vercel Edge).
Let me help verify the implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check the uncrypto.js implementation
# Test: Look for the uncrypto.js file and its implementation
fd -t f "uncrypto.js" | xargs catLength of output: 33
I'll run a new shell script to explicitly check for the existence and content of packages/trigger-sdk/src/imports/uncrypto.js to better understand the crypto implementation reference.
🏁 Script executed:
#!/bin/bash
# Check for uncrypto.js at the expected relative location
FILE="packages/trigger-sdk/src/imports/uncrypto.js"
if [ -f "$FILE" ]; then
echo "File found: $FILE"
# Display file size and first 50 lines for a quick review
wc -c "$FILE"
head -n 50 "$FILE"
else
echo "File not found: $FILE"
fiLength of output: 240
It appears that the file referenced by the import—../imports/uncrypto.js—is not present in the repository, which means that the crypto implementation under review isn’t available as expected. Before concluding, could you please confirm if the file was intentionally removed or moved to a different location? If it was meant to be removed or replaced by another module, kindly update the import and corresponding usage accordingly.
Improved the alert webhooks data and added an SDK function to verify and parse them (similar to Stripe SDK).
If the signature you provide matches the one from the dashboard when you create the webhook, you will get a nicely typed object back for these three types:
Summary by CodeRabbit
New Features
channelTypeadded to alert creation processes for better data structure.Documentation