feat: harden webhook zod validation for reliability#3354
feat: harden webhook zod validation for reliability#3354alihani714 wants to merge 1 commit intotriggerdotdev:mainfrom
Conversation
|
|
Hi @alihani714, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe change consolidates webhook schema definitions in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 Massive removal of exported schemas and types breaks multiple consumers
This PR removes the following exports that are actively used across the codebase: Webhook (discriminated union), AlertWebhookDeploymentSuccessObject, AlertWebhookDeploymentFailedObject, AlertWebhookErrorGroupObject, DeployError, RunFailedWebhook, DeploymentSuccessWebhook, DeploymentFailedWebhook, ErrorWebhook, and the AlertWebhookRunFailedObject type alias. These are imported and used in:
packages/trigger-sdk/src/v3/webhooks.ts:1— importsWebhookand callsWebhook.parse()at line 101apps/webapp/app/v3/services/alerts/deliverAlert.server.ts:11-14— importsDeploymentFailedWebhook,DeploymentSuccessWebhook,RunFailedWebhookas typesapps/webapp/app/v3/services/alerts/errorGroupWebhook.server.ts:2— importsErrorWebhooktypeapps/webapp/test/errorGroupWebhook.test.ts:2andapps/webapp/test/webhookErrorAlerts.test.ts:2— importWebhookfor test validation
While TypeScript CI should catch the compilation failures, the semantic impact is worth noting: the entire SDK webhook verification flow (webhooks.constructEvent()) is broken since it depends on Webhook.parse(). This is a breaking change for the published @trigger.dev/sdk package, and per packages/core/CLAUDE.md, changes to @trigger.dev/core affect both the customer-facing SDK and the server-side webapp.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /** File path where the task is defined */ | ||
| filePath: z.string(), | ||
| /** Name of the exported task function */ | ||
| id: idWithPrefix(ID_PATTERNS.TASK), |
There was a problem hiding this comment.
🔴 task.id regex pattern rejects all valid task identifiers
The TASK pattern /^task_[a-zA-Z0-9]+$/ requires a task_ prefix, but the actual value populated in webhook payloads is alert.taskRun.taskIdentifier (deliverAlert.server.ts:397), which is a user-defined slug like "process-payment", "email-sequence", etc. These identifiers contain hyphens and have no task_ prefix, so any call to AlertWebhookRunFailedObject.parse() or .safeParse() on a real webhook payload will fail validation.
| id: idWithPrefix(ID_PATTERNS.TASK), | |
| id: z.string().min(1), |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const ID_PATTERNS = { | ||
| RUN: /^run_[a-zA-Z0-9]+$/, | ||
| TASK: /^task_[a-zA-Z0-9]+$/, | ||
| ENV: /^env_[a-zA-Z0-9]+$/, | ||
| ORG: /^org_[a-zA-Z0-9]+$/, | ||
| PROJECT: /^proj_[a-zA-Z0-9]+$/, | ||
| }; |
There was a problem hiding this comment.
🔴 environment.id, organization.id, and project.id regex patterns reject all valid database CUIDs
The ID patterns ^env_[a-zA-Z0-9]+$, ^org_[a-zA-Z0-9]+$, and ^proj_[a-zA-Z0-9]+$ require specific prefixes, but the actual values used in webhook payloads are Prisma CUIDs (e.g., clxxxxxxxxxxxxxxxxxx) with no prefix. In deliverAlert.server.ts:428, environment.id is set to alert.environment.id (a raw CUID). Similarly, organization.id is alert.project.organizationId (line 434) and project.id is alert.project.id (line 439). All Prisma model primary keys use @default(cuid()) per the schema. Any validation of a real server-generated webhook payload against this schema will fail for all three of these fields.
| const ID_PATTERNS = { | |
| RUN: /^run_[a-zA-Z0-9]+$/, | |
| TASK: /^task_[a-zA-Z0-9]+$/, | |
| ENV: /^env_[a-zA-Z0-9]+$/, | |
| ORG: /^org_[a-zA-Z0-9]+$/, | |
| PROJECT: /^proj_[a-zA-Z0-9]+$/, | |
| }; | |
| const ID_PATTERNS = { | |
| RUN: /^run_[a-zA-Z0-9]+$/, | |
| }; | |
| const idWithPrefix = (pattern: RegExp) => z.string().regex(pattern, "Invalid ID format"); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| environment: z.object({ | ||
| /** Environment ID */ | ||
| id: z.string(), | ||
| /** Environment type */ | ||
| id: idWithPrefix(ID_PATTERNS.ENV), | ||
| type: RuntimeEnvironmentTypeSchema, | ||
| /** Environment slug */ | ||
| slug: z.string(), | ||
| /** Environment branch name */ | ||
| branchName: z.string().optional(), | ||
| }), |
There was a problem hiding this comment.
🟡 Removed branchName from environment schema breaks webhook API contract
The branchName optional field was removed from the environment object in the schema, but the server still sends it in webhook payloads at deliverAlert.server.ts:431 (branchName: alert.environment.branchName ?? undefined). Zod's default behavior strips unknown keys, so consumers who validate with .parse() will silently lose the branchName field. This is a breaking change to the webhook API contract — consumers who relied on branchName (e.g., to identify preview environment branches) will no longer receive it after validation.
| environment: z.object({ | |
| /** Environment ID */ | |
| id: z.string(), | |
| /** Environment type */ | |
| id: idWithPrefix(ID_PATTERNS.ENV), | |
| type: RuntimeEnvironmentTypeSchema, | |
| /** Environment slug */ | |
| slug: z.string(), | |
| /** Environment branch name */ | |
| branchName: z.string().optional(), | |
| }), | |
| environment: z.object({ | |
| id: idWithPrefix(ID_PATTERNS.ENV), | |
| type: RuntimeEnvironmentTypeSchema, | |
| slug: z.string(), | |
| branchName: z.string().optional(), | |
| }), |
Was this helpful? React with 👍 or 👎 to provide feedback.
| /** Associated tags */ | ||
| tags: z.array(z.string()), | ||
| /** Error information */ | ||
| tags: z.array(z.string().max(50)).max(20), |
There was a problem hiding this comment.
🚩 Tags validation constraints may reject existing valid payloads
The new schema adds .max(50) on individual tag strings and .max(20) on the tags array (tags: z.array(z.string().max(50)).max(20)), whereas the old schema had no such constraints (tags: z.array(z.string())). If any existing webhook consumers or server paths produce payloads with more than 20 tags or tags longer than 50 characters, validation would fail. This should be verified against the actual tag limits enforced elsewhere in the system (e.g., at task trigger time) to ensure consistency.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This PR implements stricter Zod validation for webhook schemas to improve reliability and prevent malformed data injection. Specifically, it adds ID prefix validation (run_, task_, etc.), URL sanitization, and data consistency checks for Git metadata.