Conversation
6bc17a9 to
46e9f7f
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive password reset functionality across authentication and profile management areas. It enables users to handle password operations through both email-based reset flows and profile-based password management, with different interfaces for Google OAuth versus email/password users.
Key changes include:
- Added password change/setup forms in the profile section with user type detection
- Implemented password reset confirmation handling in the auth callback flow
- Created reusable auth service for password operations
- Enhanced existing password reset email functionality with improved UX
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/AuthCallbackPage.tsx | Added password recovery event detection and reset form rendering |
| src/lib/auth-service.ts | Created service for password operations including update and verification |
| src/components/Profile/ProfileForm.tsx | Added security section with conditional password forms based on user type |
| src/components/Profile/PasswordSetupForm.tsx | New form for Google OAuth users to set up passwords |
| src/components/Profile/PasswordChangeForm.tsx | New form for email users to change existing passwords |
| src/components/Auth/SignInForm.tsx | Enhanced password reset UX with better error handling |
| src/components/Auth/ResetPasswordForm.tsx | New form for handling password reset confirmation |
| public/locales/*/profile.json | Added security-related translation keys |
| public/locales/*/auth.json | Added password validation and reset-related translations |
| PRPs/reset-password-implementation.md | Implementation documentation and requirements |
| export async function verifyCurrentPassword(email: string, currentPassword: string): Promise<void> { | ||
| const { error } = await supabase.auth.signInWithPassword({ | ||
| email: email, | ||
| password: currentPassword, | ||
| }); | ||
|
|
||
| if (error) { | ||
| console.error("Error verifying current password:", error); | ||
| if (error.message?.toLowerCase().includes("invalid login credentials")) { | ||
| throw new Error("Current password is incorrect"); | ||
| } | ||
| throw new Error(`Failed to verify current password: ${error.message}`); | ||
| } |
There was a problem hiding this comment.
The verifyCurrentPassword function attempts a sign-in to verify the password, which could create a new session. This approach may interfere with the user's current session and could be exploited for session hijacking. Consider using a dedicated password verification endpoint or implementing a different verification strategy that doesn't create a new session.
| export async function verifyCurrentPassword(email: string, currentPassword: string): Promise<void> { | |
| const { error } = await supabase.auth.signInWithPassword({ | |
| email: email, | |
| password: currentPassword, | |
| }); | |
| if (error) { | |
| console.error("Error verifying current password:", error); | |
| if (error.message?.toLowerCase().includes("invalid login credentials")) { | |
| throw new Error("Current password is incorrect"); | |
| } | |
| throw new Error(`Failed to verify current password: ${error.message}`); | |
| } | |
| /** | |
| * This function is unsafe because it attempts to verify the password by signing in, | |
| * which can create a new session and interfere with the user's current session. | |
| * Do not use this function. Instead, implement password verification on a secure backend endpoint. | |
| */ | |
| export async function verifyCurrentPassword(email: string, currentPassword: string): Promise<void> { | |
| throw new Error( | |
| "verifyCurrentPassword is unsafe and has been disabled. " + | |
| "Implement password verification on a secure backend endpoint that does not create a new session." | |
| ); |
| data: { subscription }, | ||
| } = supabase.auth.onAuthStateChange((event, _session) => { | ||
| if (event === "PASSWORD_RECOVERY") { | ||
| console.log("Password recovery event detected"); |
There was a problem hiding this comment.
Console.log statements should be removed from production code. Consider using a proper logging service or removing this debug statement.
| console.log("Password recovery event detected"); |
| setIsSubmitting(true); | ||
| try { | ||
| // First verify the current password | ||
| await verifyCurrentPassword(user.email, values.currentPassword); |
There was a problem hiding this comment.
Using verifyCurrentPassword could create a new session while the user is already authenticated. This may cause session conflicts or security issues. Consider alternative verification methods that don't involve creating new authentication sessions.
| await verifyCurrentPassword(user.email, values.currentPassword); | |
| await checkPassword(user.email, values.currentPassword); |
No description provided.