fix: Add a webhook for WorkOS, update middleware to allow webhooks#3048
fix: Add a webhook for WorkOS, update middleware to allow webhooks#3048
Conversation
perkinsjr
requested review from
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe pull request introduces an optional environment variable ( Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![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 (5)
apps/dashboard/pages/api/v1/workos/webhooks.ts (5)
16-17: Remove console.log statements before productionConsole statements should be removed from production code as they can expose sensitive information in logs.
- console.info("payload",payload); - console.info(req.headers);
7-8: Consider handling all HTTP methods explicitlyThe current implementation only checks for POST requests but doesn't explicitly return an error for other methods.
export default async (req: NextApiRequest, res: NextApiResponse) => { if (req.method === 'POST') { + } else { + return res.status(405).json({ error: "Method not allowed" }); } };
63-102: Improve error handling in alertSlack functionThe current implementation swallows errors from the Slack notification. Consider at least logging the error more descriptively or propagating it up to be handled by the caller.
async function alertSlack(email: string): Promise<void> { const url = process.env.SLACK_WEBHOOK_URL_SIGNUP; if (!url) { + console.warn("SLACK_WEBHOOK_URL_SIGNUP not configured"); return; } const domain = email.split("@").at(-1); if (!domain) { + console.warn(`Could not extract domain from email: ${email}`); return; } if (freeDomains.includes(domain)) { + console.debug(`Skipping Slack notification for free email domain: ${domain}`); return; } await fetch(url, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ text: `${email} signed up`, blocks: [ { type: "section", fields: [ { type: "mrkdwn", text: `${email} signed up`, }, { type: "mrkdwn", text: `<https://${domain}>`, }, ], }, ], }), }).catch((err: Error) => { - console.error(err); + console.error(`Failed to send Slack notification: ${err.message}`); + // Consider whether you want to throw the error to propagate it + // throw new Error(`Slack notification failed: ${err.message}`); }); }
40-42: Add email format validationConsider validating the email format before processing it, as invalid email formats could cause issues downstream.
if (!webhookData.email) { return res.status(400).json({ Error: "No email address found" }); } +// Simple regex for basic email validation +const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; +if (!emailRegex.test(webhookData.email)) { + return res.status(400).json({ Error: "Invalid email format" }); +}
65-74: Consider extracting email domain validation to a separate utility functionThe domain validation logic could be useful elsewhere in the codebase.
+/** + * Checks if an email is from a business domain (not a free email provider) + */ +function isBusinessEmail(email: string): boolean { + const domain = email.split("@").at(-1); + if (!domain) return false; + return !freeDomains.includes(domain); +} async function alertSlack(email: string): Promise<void> { const url = process.env.SLACK_WEBHOOK_URL_SIGNUP; if (!url) { return; } const domain = email.split("@").at(-1); if (!domain) { return; } - if (freeDomains.includes(domain)) { + if (!isBusinessEmail(email)) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/lib/env.ts(1 hunks)apps/dashboard/middleware.ts(1 hunks)apps/dashboard/pages/api/v1/clerk/webhooks.ts(0 hunks)apps/dashboard/pages/api/v1/workos/webhooks.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/pages/api/v1/clerk/webhooks.ts
🧰 Additional context used
🧬 Code Definitions (1)
apps/dashboard/pages/api/v1/workos/webhooks.ts (1)
apps/dashboard/lib/env.ts (1)
env(3-52)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/lib/env.ts (1)
46-46: LGTM: New environment variable for WorkOS webhookThe addition of
WORKOS_WEBHOOK_SECRETas an optional environment variable is correctly implemented and appropriately placed with other WorkOS-related configurations.apps/dashboard/middleware.ts (1)
26-28: Webhook paths correctly added to public routesThe addition of these webhook paths to the
publicPathsarray ensures they'll be accessible without authentication, which is necessary for external services to call these endpoints.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard/pages/api/v1/workos/webhooks.ts (1)
21-29:⚠️ Potential issueMissing try/catch around WorkOS webhook construction
The webhook construction is missing error handling. If the signature validation fails, it will throw an exception that isn't currently caught, potentially causing unhandled promise rejections.
const workos = new WorkOS(WORKOS_API_KEY); - const webhook = await workos.webhooks.constructEvent({ - payload: payload, - sigHeader: sigHeader, - secret: WORKOS_WEBHOOK_SECRET, - }); - - if (!webhook) { - return res.status(400).json({ Error: "Invalid payload" }); - } + try { + const webhook = await workos.webhooks.constructEvent({ + payload: payload, + sigHeader: sigHeader, + secret: WORKOS_WEBHOOK_SECRET, + }); + + if (!webhook) { + return res.status(400).json({ Error: "Invalid payload" }); + }
🧹 Nitpick comments (3)
apps/dashboard/pages/api/v1/workos/webhooks.ts (3)
72-97: Improve error handling in the Slack notificationThe current implementation catches errors but only logs them. Consider returning the error or at least adding more context to the log.
await fetch(url, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ text: `${email} signed up`, blocks: [ { type: "section", fields: [ { type: "mrkdwn", text: `${email} signed up`, }, { type: "mrkdwn", text: `<https://${domain}>`, }, ], }, ], }), - }).catch((err: Error) => { - console.error(err); - }); + }).catch((err: Error) => { + console.error("Failed to send Slack notification:", err); + });
64-67: Improve domain extraction robustnessThe current domain extraction assumes the email format is valid and contains an "@" symbol. Add validation to handle malformed emails.
- const domain = email.split("@").at(-1); - if (!domain) { + // Ensure the email format is valid before extracting domain + if (!email.includes("@")) { + console.warn(`Invalid email format: ${email}`); return; } + const domain = email.split("@").at(-1); + if (!domain) { + console.warn(`Could not extract domain from: ${email}`); + return; + }
16-18: Improve error message for missing payload or signatureThe current error message "Nope" is not descriptive enough. Consider providing a more informative error message.
if (!payload || !sigHeader) { - return res.status(400).json({ Error: "Nope" }); + return res.status(400).json({ Error: "Missing payload or signature header" }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/pages/api/v1/workos/webhooks.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/dashboard/pages/api/v1/workos/webhooks.ts (1)
apps/dashboard/lib/env.ts (1)
env(3-52)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Agent Local / test_agent_local
🔇 Additional comments (3)
apps/dashboard/pages/api/v1/workos/webhooks.ts (3)
9-14: Good job validating environment variablesThe code properly checks for all required environment variables before proceeding, which prevents runtime errors.
39-54: Well-structured try-catch block for operationsThe code correctly wraps the Resend operations and Slack notification in a try-catch block, providing appropriate error handling.
68-70: Good filtering of free email domainsThe implementation correctly filters out notifications for free email domains, which helps reduce noise in the Slack channel.
2 participants
What does this PR do?
A few tune ups for WorkOS
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Removed