chore: CSRF protect forgot-password functionality #22361
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
|
✅ No security or compliance issues detected. Reviewed everything up to 7f9ee3b. Security Overview
Detected Code Changes
Reply to this PR with |
WalkthroughThe changes introduce CSRF protection to the password reset functionality. A new API route is added to generate and return a CSRF token, which is also set as a cookie. The password reset API handler is updated to validate the submitted CSRF token against the value stored in the cookie, returning a 403 error if validation fails. The password reset frontend is refactored to modularize UI components and now fetches and submits a CSRF token as part of the form submission, with logic to refresh the token on failure. Server-side logic is updated to remove unused CSRF-related imports. Estimated code review effort3 (~45 minutes) 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/web/app/api/auth/reset-password/route.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. apps/web/app/api/csrf/route.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ 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 (
|
Graphite Automations"Add foundation team as reviewer" took an action on this PR • (07/22/25)1 reviewer was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/api/auth/reset-password/route.ts(3 hunks)apps/web/app/api/csrf/route.ts(1 hunks)apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx(2 hunks)apps/web/server/lib/auth/forgot-password/[id]/getServerSideProps.tsx(0 hunks)
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
apps/web/app/api/auth/reset-password/route.tsapps/web/app/api/csrf/route.tsapps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/prisma/schema.prisma:1068-1071
Timestamp: 2025-07-16T05:10:22.891Z
Learning: In PR #22304 for Cal.com private link expiration features, the `maxUsageCount` field was intentionally set to default to 1 (non-nullable) as a breaking change, making all existing private links single-use after migration. This was a deliberate design decision by alishaz-polymath.
apps/web/app/api/auth/reset-password/route.ts (1)
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-21T13:54:11.770Z
Learning: Applies to backend/**/*.{ts,tsx} : Make sure the credential.key field is never returned back from tRPC endpoints or APIs.
apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx (2)
Learnt from: Anshumancanrock
PR: #22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.343Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
💤 Files with no reviewable changes (1)
- apps/web/server/lib/auth/forgot-password/[id]/getServerSideProps.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
apps/web/app/api/auth/reset-password/route.tsapps/web/app/api/csrf/route.tsapps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/prisma/schema.prisma:1068-1071
Timestamp: 2025-07-16T05:10:22.891Z
Learning: In PR #22304 for Cal.com private link expiration features, the `maxUsageCount` field was intentionally set to default to 1 (non-nullable) as a breaking change, making all existing private links single-use after migration. This was a deliberate design decision by alishaz-polymath.
apps/web/app/api/auth/reset-password/route.ts (1)
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-21T13:54:11.770Z
Learning: Applies to backend/**/*.{ts,tsx} : Make sure the credential.key field is never returned back from tRPC endpoints or APIs.
apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx (2)
Learnt from: Anshumancanrock
PR: #22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.343Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Security Check
🔇 Additional comments (3)
apps/web/app/api/auth/reset-password/route.ts (1)
28-37: CSRF token validation implementation looks goodThe implementation correctly:
- Retrieves the token from cookies
- Validates token presence and equality
- Deletes the token after successful validation to prevent reuse
apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx (2)
20-163: Excellent modular refactoringThe refactoring into separate components (
Success,Expired, andPasswordResetForm) improves code organization and maintainability. The CSRF token integration is well-implemented with automatic refresh on failure.
137-146: Password validation rules are comprehensiveThe password validation enforces:
- Minimum length of 7 characters
- At least one digit, one uppercase, and one lowercase letter
This provides good baseline security for password strength.
| @@ -10,6 +11,7 @@ import prisma from "@calcom/prisma"; | |||
| import { IdentityProvider } from "@calcom/prisma/enums"; | |||
|
|
|||
| const passwordResetRequestSchema = z.object({ | |||
| csrfToken: z.string().optional(), | |||
There was a problem hiding this comment.
Make CSRF token required in the schema
The CSRF token is marked as optional in the schema, but the validation logic treats it as required (returning 403 if missing). Update the schema to reflect this requirement.
const passwordResetRequestSchema = z.object({
- csrfToken: z.string().optional(),
+ csrfToken: z.string(),
password: z.string().refine(validPassword, () => ({
message: "Password does not meet the requirements",
})),
requestId: z.string(), // format doesn't matter.
});🤖 Prompt for AI Agents
In apps/web/app/api/auth/reset-password/route.ts at line 14, the csrfToken field
in the schema is currently optional but should be required to match the
validation logic that returns a 403 error if it is missing. Change the schema
definition for csrfToken from optional to required by removing the .optional()
modifier to ensure consistent validation.
| res.cookies.set("calcom.csrf_token", token, { | ||
| httpOnly: false, | ||
| secure: process.env.NODE_ENV === "production", | ||
| sameSite: "lax", | ||
| path: "/", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding an expiration time to the CSRF token cookie
The CSRF token cookie is set without an expiration time, making it a session cookie that persists until the browser is closed. For enhanced security, consider adding a reasonable expiration time (e.g., 1 hour) to limit the token's validity window.
res.cookies.set("calcom.csrf_token", token, {
httpOnly: false,
secure: process.env.NODE_ENV === "production",
sameSite: "lax",
path: "/",
+ maxAge: 3600, // 1 hour in seconds
});📝 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.
| res.cookies.set("calcom.csrf_token", token, { | |
| httpOnly: false, | |
| secure: process.env.NODE_ENV === "production", | |
| sameSite: "lax", | |
| path: "/", | |
| }); | |
| res.cookies.set("calcom.csrf_token", token, { | |
| httpOnly: false, | |
| secure: process.env.NODE_ENV === "production", | |
| sameSite: "lax", | |
| path: "/", | |
| maxAge: 3600, // 1 hour in seconds | |
| }); |
🤖 Prompt for AI Agents
In apps/web/app/api/csrf/route.ts around lines 9 to 14, the CSRF token cookie is
set without an expiration time, making it a session cookie. To enhance security,
add an expiration time by including the 'maxAge' property set to 3600 seconds (1
hour) in the cookie options object when calling res.cookies.set.
| useEffect(() => { | ||
| fetch("/api/csrf", { cache: "no-store" }) | ||
| .then((res) => res.json()) | ||
| .then(({ csrfToken }) => setValue("csrfToken", csrfToken)) | ||
| .catch(() => setValue("csrfToken", "")); | ||
| }, [setValue, refreshToken]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for CSRF token fetch failure
The CSRF token fetch catches errors but sets an empty string, which will cause the form submission to fail with a generic error. Consider showing a specific error message to the user or retrying the fetch.
useEffect(() => {
fetch("/api/csrf", { cache: "no-store" })
.then((res) => res.json())
.then(({ csrfToken }) => setValue("csrfToken", csrfToken))
- .catch(() => setValue("csrfToken", ""));
+ .catch(() => {
+ setValue("csrfToken", "");
+ setError("newPassword", {
+ type: "client",
+ message: t("csrf_token_fetch_failed")
+ });
+ });
}, [setValue, refreshToken]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx
around lines 81 to 86, the fetch call for the CSRF token catches errors but only
sets an empty string, which leads to generic form submission errors. Update the
error handling to either display a specific error message to the user indicating
the CSRF token fetch failed or implement a retry mechanism for the fetch. This
will improve user feedback and reduce confusion during form submission failures.
| {!success && <PasswordResetForm form={formMethods} requestId={requestId} />} | ||
| {success && <Success />} |
There was a problem hiding this comment.
While it's not true on all cases, I think it would be more readable and shorter using a ternary instead of two expressions:
{success ? <Success /> : <PasswordResetForm form={formMethods} requestId={requestId} />}
|
Hey @emrysal, can you address the comments on PR? So I'll approve it :) |
What does this PR do?
Summary by cubic
Added CSRF protection to the forgot password flow to prevent cross-site request forgery attacks.