Daniel/reset password serverside #3198
Conversation
WalkthroughThe changes update various backend and frontend modules to enhance password reset and signup processes. In the backend, modifications include the addition of a new import for Suggested reviewers
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
backend/src/services/auth/auth-fns.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 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 (
|
![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
🔭 Outside diff range comments (1)
backend/src/server/routes/v1/password-router.ts (1)
95-101: 💡 Verification agent🧩 Analysis chain
Consider implementing constant time response for password reset email request
According to the PR objectives, a timing attack vulnerability was addressed in the email password reset endpoint by implementing a fixed delay. However, I don't see this implementation in the code. If not already implemented in the service layer, consider adding a fixed delay to this handler.
🏁 Script executed:
#!/bin/bash # Check if a fixed delay is implemented in the password service rg -A 10 "sendPasswordResetEmail" --glob "*.ts" | grep -E "delay|setTimeout|wait"Length of output: 426
Implement a fixed delay for the password reset request
It appears that no fixed delay mechanism (e.g., using
setTimeout, a promise-based wait, or similar) is implemented either in the endpoint handler or in the underlyingsendPasswordResetEmailservice, as verified by the search command output. This missing delay could leave the endpoint vulnerable to timing attacks. Please add a constant delay—either in the service layer or directly in the router—to ensure that the response time remains constant regardless of whether the email exists.
- Location:
backend/src/server/routes/v1/password-router.ts(lines 95-101)- Context: The delay should be implemented after calling
await server.services.password.sendPasswordResetEmail(req.body.email)and before returning the response.
🧹 Nitpick comments (12)
frontend/src/pages/auth/SignUpPage/SignUpPage.tsx (1)
41-48: Update code comments to reflect the new signup flowThe comments still describe a 5-step signup process with step 4 being "downloading a backup pdf", but this step has been removed from the actual implementation. The comments should be updated to match the new 4-step process.
/** - * Goes to the following step (out of 5) of the signup process. + * Goes to the following step (out of 4) of the signup process. * * Step 1 is submitting your email * Step 2 is Verifying your email with the code that you received * Step 3 is asking the final info. - * Step 4 is downloading a backup pdf - * Step 5 is inviting users + * Step 4 is inviting users */frontend/src/pages/user/PersonalSettingsPage/components/ChangePasswordSection/ChangePasswordSection.tsx (1)
83-83: Consider improving the post-password-change experience.After password change, the code redirects users to the login page. Consider keeping users logged in for a better experience, perhaps using token refresh or session extension mechanisms.
- window.location.href = "/login"; + // Refresh user session with new credentials instead of full redirect + // This could involve token refresh or other session management + // For now, we'll keep the user logged inbackend/src/services/auth/auth-fns.ts (1)
67-69: Consider validating token type for password reset authorization.Unlike
validateSignUpAuthorizationwhich verifies theauthTokenType(line 44), this function doesn't check the token type after decoding. Consider adding validation to ensure the token was specifically issued for password reset operations.const decodedToken = jwt.verify(AUTH_TOKEN_VALUE, appCfg.AUTH_SECRET) as AuthModeProviderSignUpTokenPayload; + if (decodedToken.authTokenType !== AuthTokenType.PASSWORD_RESET_TOKEN) throw new UnauthorizedError(); + return decodedToken;frontend/src/pages/auth/PasswordResetPage/components/InputBackupKeyStep.tsx (1)
36-36: Remove commented-out codeThere's a commented-out line
// setStep(3);that should be removed since it's not serving any purpose and could cause confusion for future developers.onComplete(privateKey); - // setStep(3);frontend/src/pages/auth/PasswordResetPage/components/ConfirmEmailStep.tsx (1)
33-44: Improve error handling and messagingThe error handling could be improved to provide more meaningful feedback to users:
- The error is just logged as "ERROR" in all caps, which isn't consistent with typical logging patterns
- There's no user-facing error message displayed
try { const response = await verifyPasswordResetCodeMutateAsync({ email, code: token }); onComplete(response.token, response.userEncryptionVersion); } catch (err) { - console.log("ERROR", err); + console.error("Failed to verify password reset code:", err); navigate({ to: "/email-not-verified" }); }frontend/src/pages/admin/SignUpPage/SignUpPage.tsx (1)
77-81: Consider more specific error messagesThe error message has been changed to a generic "Failed to create admin" which could make troubleshooting more difficult. Consider retaining more specific error information to help users understand what went wrong.
} catch (err) { console.log(err); createNotification({ type: "error", - text: "Failed to create admin" + text: err instanceof Error ? err.message : "Failed to create admin" }); }frontend/src/hooks/api/auth/queries.tsx (1)
311-321: Consider error handling for the newuseResetPasswordV2.
While the POST request is straightforward, it may be prudent to handle possible request failures more robustly (e.g., returning errors for invalid tokens, invalid password, etc.). This could be done viaonErrorinuseMutationor a try/catch block within the mutation function.useResetPasswordV2 = () => { return useMutation({ mutationFn: async (details: ResetPasswordV2DTO) => { try { await apiRequest.post("/api/v2/password/password-reset", details, { headers: { Authorization: `Bearer ${details.verificationToken}` } }); + } catch (err) { + // Possible place to provide user feedback or logging + throw err; } } }); };frontend/src/pages/auth/PasswordResetPage/PasswordResetPage.tsx (1)
19-23: Schema definition forformData.
Defining the schema via Zod forverificationToken,privateKey, anduserEncryptionVersionis concise and enforces early validation. Consider adding more constraints if the verification token or other fields demand specific formats (e.g., lengths).frontend/src/pages/auth/PasswordResetPage/components/EnterPasswordStep.tsx (1)
95-167: No inline error feedback for failed server requests.
Although the decryption/encryption pipeline and the password-based re-encryption logic are correct, there is no direct in-form error message if the server call fails (e.g., network error, invalid token, etc.). You may want to catch and display errors so users see immediate feedback.+ try { if (encryptionVersion === UserEncryptionVersion.V2) { await resetPasswordV2({ newPassword: data.password, verificationToken }); } else { // existing JSRP logic } + } catch (err) { + // Provide user-facing error message or call a toast notification, for example + console.error(err); + return; }backend/src/services/auth/auth-password-service.ts (3)
122-150: Good security measure but consider the concurrency impact.
Enforcing a minimum 8-second delay helps mitigate timing attacks. However, this synchronous delay might raise concerns on a high-traffic system. Consider verifying whether this blocking approach could lead to performance bottlenecks under heavy load.🧰 Tools
🪛 Biome (1.9.4)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
126-126: Optional chain suggestion.
Static analysis recommends using optional chaining:if (user?.isAccepted). This is a minor style preference; your existing null-checking is valid, though you may consider optional chaining for brevity.-if (user && user.isAccepted) { +if (user?.isAccepted) {🧰 Tools
🪛 Biome (1.9.4)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
159-163: Ensure consistent error status.
Throwing aBadRequestErrorhere is logically fine. However, consider whether a 404 or 401 code might convey the meaning more precisely (e.g., “No encryption data found” can hint at a missing resource). Keep the error semantics consistent across similar checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
backend/src/server/routes/v1/password-router.ts(2 hunks)backend/src/server/routes/v2/index.ts(2 hunks)backend/src/server/routes/v2/password-router.ts(1 hunks)backend/src/services/auth/auth-fns.ts(1 hunks)backend/src/services/auth/auth-password-service.ts(6 hunks)backend/src/services/auth/auth-password-type.ts(1 hunks)frontend/src/components/auth/DonwloadBackupPDFStep.tsx(0 hunks)frontend/src/components/utilities/checks/password/PasswordCheck.ts(1 hunks)frontend/src/hooks/api/auth/index.tsx(1 hunks)frontend/src/hooks/api/auth/queries.tsx(3 hunks)frontend/src/hooks/api/auth/types.ts(2 hunks)frontend/src/pages/admin/SignUpPage/SignUpPage.tsx(4 hunks)frontend/src/pages/admin/SignUpPage/components/DownloadBackupKeys/DownloadBackupKeys.tsx(0 hunks)frontend/src/pages/admin/SignUpPage/components/DownloadBackupKeys/index.tsx(0 hunks)frontend/src/pages/auth/PasswordResetPage/PasswordResetPage.tsx(1 hunks)frontend/src/pages/auth/PasswordResetPage/components/ConfirmEmailStep.tsx(1 hunks)frontend/src/pages/auth/PasswordResetPage/components/EnterPasswordStep.tsx(1 hunks)frontend/src/pages/auth/PasswordResetPage/components/InputBackupKeyStep.tsx(1 hunks)frontend/src/pages/auth/SignUpInvitePage/SignUpInvitePage.tsx(3 hunks)frontend/src/pages/auth/SignUpPage/SignUpPage.tsx(1 hunks)frontend/src/pages/auth/SignUpSsoPage/SignUpSsoPage.tsx(0 hunks)frontend/src/pages/auth/SignUpSsoPage/components/BackupPDFStep/BackupPDFStep.tsx(0 hunks)frontend/src/pages/auth/SignUpSsoPage/components/BackupPDFStep/index.tsx(0 hunks)frontend/src/pages/auth/SignUpSsoPage/components/UserInfoSSOStep/UserInfoSSOStep.tsx(4 hunks)frontend/src/pages/auth/VerifyEmailPage/VerifyEmailPage.tsx(1 hunks)frontend/src/pages/user/PersonalSettingsPage/components/ChangePasswordSection/ChangePasswordSection.tsx(3 hunks)frontend/src/pages/user/PersonalSettingsPage/components/PersonalGeneralTab/PersonalGeneralTab.tsx(1 hunks)
💤 Files with no reviewable changes (6)
- frontend/src/pages/auth/SignUpSsoPage/components/BackupPDFStep/index.tsx
- frontend/src/pages/auth/SignUpSsoPage/SignUpSsoPage.tsx
- frontend/src/pages/auth/SignUpSsoPage/components/BackupPDFStep/BackupPDFStep.tsx
- frontend/src/pages/admin/SignUpPage/components/DownloadBackupKeys/index.tsx
- frontend/src/pages/admin/SignUpPage/components/DownloadBackupKeys/DownloadBackupKeys.tsx
- frontend/src/components/auth/DonwloadBackupPDFStep.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/services/auth/auth-password-service.ts
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check API Changes
- GitHub Check: Check TS and Lint
- GitHub Check: Run integration test
- GitHub Check: Check Frontend Type and Lint check
🔇 Additional comments (42)
frontend/src/pages/auth/SignUpSsoPage/components/UserInfoSSOStep/UserInfoSSOStep.tsx (1)
5-5: Improved navigation approach using router instead of state management.The change from using a step state approach to router-based navigation is a clean improvement. This aligns with the PR objective of transitioning from React state to better state management approaches and simplifies the signup flow by removing the backup key download step.
Also applies to: 21-21, 66-66, 206-208
frontend/src/hooks/api/auth/index.tsx (1)
5-6: Nicely organized additions of new API hooks.The addition of
useResetPasswordV2anduseResetUserPasswordV2hooks maintains the existing export pattern while extending functionality to support the new v2 password reset flow.backend/src/server/routes/v2/index.ts (2)
6-6: Clean import addition.Import statement follows the existing pattern and naming convention.
16-16: Well-structured route registration.Registration of the password router follows the established pattern with appropriate prefix. The positioning within the file is logical, keeping it with other authentication-related registrations.
backend/src/services/auth/auth-password-type.ts (2)
16-19: Clear enum definition for password reset types.The enum clearly distinguishes between recovery (forgotten password) and logged-in reset scenarios, which aligns well with the PR objective of supporting password resets without emergency kits.
21-26: Well-structured DTO for password reset operations.The type definition includes all necessary fields and handles both reset scenarios appropriately. The optional
oldPasswordfield is a good choice for the logged-in reset flow where password verification may be needed.frontend/src/pages/auth/VerifyEmailPage/VerifyEmailPage.tsx (1)
75-77: Improved user guidance for password reset.The updated text provides clearer instructions about the password reset process, removing references to "emergency kit" which aligns with the PR's objective of enabling password resets without requiring such kits. The center-aligned, simplified messaging improves user experience.
frontend/src/components/utilities/checks/password/PasswordCheck.ts (2)
37-37:✅ Verification successful
Consider the implications of optional chaining in the password validator
The introduction of optional chaining (
pwd?.length) seems unnecessary sincepwdis explicitly typed as a non-optionalstringparameter and is directly passed from thepasswordvariable which is required in the interface.If there are legitimate cases where
pwdcould beundefined, this optional chaining would cause the validation to always fail sinceundefined >= 14evaluates tofalse.
🏁 Script executed:
#!/bin/bash # Check if there are any places in the codebase where the password validator is called with undefined or null rg -A 5 "validator\(.*null.*\)" --glob "*.ts*" rg -A 5 "validator\(.*undefined.*\)" --glob "*.ts*"Length of output: 102
Ensure Consistent Validator Behavior in PasswordCheck
The password validator’s use of optional chaining appears unnecessary. Since
pwdis explicitly typed as a non-optional string and our search did not reveal any cases where the validator is invoked withundefinedornull, it is safe to remove the optional chaining. This change will make the validator’s intent clearer and avoid any misinterpretation.
- File:
frontend/src/components/utilities/checks/password/PasswordCheck.ts- Line 37: Change from
tovalidator: (pwd: string) => pwd?.length >= 14,validator: (pwd: string) => pwd.length >= 14,
42-42:❓ Verification inconclusive
Same optional chaining concern as in the previous validator
The optional chaining pattern is repeated here. If this is intentional defensive programming, consider documenting why it's needed or refactoring the validator signature to explicitly accept potentially undefined values.
🏁 Script executed:
#!/bin/bash # Check how the password validators are typically called throughout the codebase rg "test\.validator\(" --glob "*.ts*"Length of output: 265
Optional Chaining on
pwdParameter Needs Clarification
At line 42 infrontend/src/components/utilities/checks/password/PasswordCheck.ts, the validator is defined as:validator: (pwd: string) => pwd?.length < 101,Since the parameter is typed as a string, using
pwd?.lengthsuggests defensive coding against a potential undefined value. However, review of the call sites shows that the validator is typically invoked with an explicit value (e.g., viaif (!test.validator(password))). Please consider one of the following approaches:
- Remove optional chaining: If the password is guaranteed to be a defined string, refactor the code to use
pwd.lengthto avoid ambiguity.- Refine the function signature: Update the parameter type to
pwd?: stringif it's possible for an undefined value to be passed, and add documentation explaining why this defensive pattern is needed.frontend/src/pages/user/PersonalSettingsPage/components/PersonalGeneralTab/PersonalGeneralTab.tsx (1)
10-17: LGTM! Good conditional rendering based on encryption versionThe conditional rendering of the
EmergencyKitSectiononly for users with encryption version V1 aligns well with the PR objective of maintaining backward compatibility while removing backup key downloads for V2 users.The fallback to
UserEncryptionVersion.V2when user data is not available is a good defensive programming practice.frontend/src/pages/auth/SignUpPage/SignUpPage.tsx (1)
74-74: Update the step number in the condition to reflect removal of backup PDF downloadThis change correctly adjusts the navigation logic after removing the DownloadBackupPDF step.
backend/src/server/routes/v1/password-router.ts (2)
117-120: LGTM! Good update to include encryption version in the responseThe response schema update properly adds the user encryption version, which is crucial information for the frontend to determine which password reset flow to use.
124-127: LGTM! Simplified return statementThe handler now directly returns the result of
verifyPasswordResetEmail, which is cleaner and more maintainable.frontend/src/hooks/api/auth/types.ts (3)
6-9: Well-structured enum for versioning user encryption.The
UserEncryptionVersionenum clearly defines the supported encryption versions, making it easy to perform version-specific logic throughout the codebase. This supports the PR's goal of adding backward compatibility for the password reset feature.
144-147: LGTM: Properly structured DTO for V2 password reset.This type correctly captures the essential fields needed for the new V2 password reset process - the new password and verification token. This is consistent with the token-based approach mentioned in the PR objectives.
149-152: LGTM: Clear DTO for logged-in user password changes.The separation between
ResetPasswordV2DTO(for recovery) andResetUserPasswordV2DTO(for logged-in users) creates a clear distinction between the two password reset flows, enhancing type safety in the codebase.frontend/src/pages/user/PersonalSettingsPage/components/ChangePasswordSection/ChangePasswordSection.tsx (2)
51-51: Good use of React Query hook for the API call.Using a mutation hook from React Query for the password reset operation provides automatic loading state management, error handling, and request deduplication.
63-74: Great implementation of conditional password reset logic based on encryption version.The code elegantly handles two different password reset flows based on the user's encryption version, fulfilling the PR's requirement to maintain backward compatibility while introducing the new V2 password reset functionality.
backend/src/server/routes/v2/password-router.ts (3)
10-29: Secure implementation of the recovery token password reset endpoint.The route properly:
- Applies rate limiting to prevent brute force attacks
- Validates the token from authorization header
- Only requires the new password in the request body
- Uses a dedicated service method for the reset operation
This satisfies the PR objective of providing a secure password reset workflow.
31-52: Well-designed logged-in user password reset endpoint.The implementation includes:
- Rate limiting protection
- Authentication verification through
verifyAuthmiddleware- Proper validation of both old and new passwords
- Clear separation between recovery and logged-in reset flows
This supports the PR objective of enhancing the password reset functionality.
1-53:✅ Verification successful
Verify handling of timing attacks for the email password reset endpoint.
The PR objectives mention addressing a timing attack vulnerability in the email password reset endpoint with a fixed delay of 8 seconds. I don't see this implementation in this file.
🏁 Script executed:
#!/bin/bash # Check for implementation of timing attack protection in the server code # Look for any code that implements fixed delays in password reset endpoints rg -A 3 -B 3 "setTimeout|delay|sleep" --type ts backend/src/Length of output: 44527
Timing Attack Mitigation Confirmed in Service Implementation
The 8‑second fixed delay to mitigate timing attacks isn’t applied directly in the router file. Instead, our verification shows that the delay is implemented within the password service (specifically in
backend/src/services/auth/auth-password-service.ts), where it ensures that each password reset request takes at least 8 seconds. The router appropriately delegates to this service. No further changes are needed here, but please confirm that all reset flows invoke this service so the protection applies uniformly.backend/src/services/auth/auth-fns.ts (1)
48-70: Comprehensive token validation for password reset.The
validatePasswordResetAuthorizationfunction performs thorough validation:
- Checks for token presence
- Validates the token type is "bearer"
- Verifies the JWT signature with proper error handling
This supports the security requirements for the password reset feature.
frontend/src/pages/auth/PasswordResetPage/components/InputBackupKeyStep.tsx (2)
24-41: LGTM! Good error handling implementationThe function correctly handles API calls and encryption/decryption operations with proper error handling. The form error is set appropriately when decryption fails.
1-87: Well-structured component using modern form handling patternsThis component is well-implemented using react-hook-form with Zod for validation. The form structure and error handling follow best practices, and the component has clear separation of concerns between UI rendering and business logic.
frontend/src/pages/auth/PasswordResetPage/components/ConfirmEmailStep.tsx (1)
1-54: Good implementation of the email confirmation stepThe component is well-structured and follows the single responsibility principle. It correctly extracts data from the URL and handles the verification process appropriately.
frontend/src/pages/auth/SignUpInvitePage/SignUpInvitePage.tsx (3)
389-390: Successfully removed backup key stepThe code has been properly updated to remove the backup key generation and download functionality, which aligns with the PR's objective to remove backup key downloads. The conditional rendering now only includes the email confirmation and main signup steps.
205-207: Updated navigation syntax correctlyThe navigation syntax has been updated to use the newer pattern with the
to:property, which is the correct approach.
183-185:❓ Verification inconclusive
Test token-based navigation behavior
Make sure to verify that the navigation to the SecretManager works correctly after the reorganization of the signup flow. Since the backup key step was removed, it's important to ensure that the user is still redirected to the correct location after signup.
🏁 Script executed:
#!/bin/bash # Check for any other occurrences of similar navigation patterns in the codebase echo "Searching for similar navigation patterns..." rg "navigate\(\{[\s\n]*to:" --type=tsx -A 1 -B 1Length of output: 177
I've noticed that the initial script failed because the file type "tsx" wasn't recognized. To gather the necessary information, please run the following shell script to search for occurrences of the navigation pattern within TSX files (which should include the token-based navigation behavior):
After running this script, please verify that the navigation logic correctly directs the user to the SecretManager after signup (especially given the removal of the backup key step). Once confirmed, you can update or remove the comment accordingly.
🏁 Script executed:
#!/bin/bash echo "Searching for navigation patterns using 'navigate(' in TSX files..." rg "navigate\(" -g "*.tsx" --sort-files -nLength of output: 24758
Verify Redirection Post-Signup in Token-Based Flow
Please ensure that after completing the signup flow on SignUpInvitePage (starting at lines 183–185), a user with a valid token is routed to the correct page (e.g., the SecretManager overview). Since the backup key step was removed, manually verify that the redirection logic in
completeSignupFlowbehaves correctly, matching the expected behavior seen in similar navigation patterns across the codebase.frontend/src/pages/admin/SignUpPage/SignUpPage.tsx (3)
85-85: Simplified conditional renderingThe conditional rendering has been simplified, which is a good improvement for readability.
98-188: Successfully streamlined signup flowThe signup flow has been appropriately streamlined from a multi-step process to a single form, which improves the user experience by making it more straightforward. The form structure and validation are well-implemented.
48-83:❓ Verification inconclusive
Verify removal of backup key logic is complete
Since the PR objective includes removing backup key downloads, ensure that all related functionality has been removed completely, not just the UI components. This includes checking if there are any leftover API calls or utility functions that were used solely for backup key generation.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to backup keys in the codebase echo "Searching for backup key references..." rg "backup(Key|PDF)" --type=tsxLength of output: 148
Below is a new shell script to re-run the search correctly (using a glob instead of the unrecognized file type):
#!/bin/bash echo "Searching for backup key references in all .tsx files using a glob..." rg "backup(Key|PDF)" -g "*.tsx"Backup Key Logic Removal Verification
Since the previous command failed due to an unrecognized file type, please run the above script. Verify manually that no output is returned—this confirms that all backup key logic (including API calls or utility functions) has been completely removed as intended.
frontend/src/hooks/api/auth/queries.tsx (4)
25-26: No issues with new DTO types.
The introduction ofResetPasswordV2DTOandResetUserPasswordV2DTOlooks consistent with the usage in this PR. Ensure these types remain aligned with the backend endpoint schemas as they evolve.
33-33: Ensure consistent usage ofUserEncryptionVersionacross the codebase.
UserEncryptionVersionis now being referenced in several new places. Verify all relevant areas (backend, components, error handlers) are updated to expect/support these enumerations properly.
253-256: Type-safe response inuseVerifyPasswordResetCode.
This explicit type definition for{ token, userEncryptionVersion }is clear and helps with type safety. Confirm that any calling code properly handles scenarios in whichuserEncryptionVersioncould be unexpectedly missing or invalid.
323-329: Verify whether authentication is required foruseResetUserPasswordV2.
TheBearertoken header is omitted here. If this endpoint intentionally does not require auth, that's acceptable. Otherwise, consider adding a similar header to ensure security consistency across both password reset endpoints.frontend/src/pages/auth/PasswordResetPage/PasswordResetPage.tsx (2)
1-17: Overall structure and enum-based step flow look good.
UsingStepsto guide the flow is a clean approach. It neatly separates the password reset process into logical steps. The imports and setup forreact-hook-formandzodare straightforward.
27-75: Effective step-handling for multi-stage password reset.
ConfirmEmailStepcaptures and setsverificationToken+userEncryptionVersion.- Logic branches to either
InputBackupKeyStepor directlyEnterNewPasswordStepbased on encryption version.- Navigation uses
react-routerupon completion.This is a well-structured approach. Just ensure that step transitions handle any edge cases (e.g., missing or invalid tokens) and gracefully degrade with user-friendly error feedback.
frontend/src/pages/auth/PasswordResetPage/components/EnterPasswordStep.tsx (2)
1-29: Check for potential oversharing of sensitive data.
The import ofcrypto, usage ofprivateKey, and handling of user passwords are necessary for encryption purposes. Just confirm that no logs or extraneous console statements leak these sensitive values anywhere in the code or dev tools.
169-326: Appropriate password policy enforcement with real-time checks.
passwordCheckfor length, characters, breach check, etc., appears comprehensive.- The UI feedback with icons is user-friendly.
- Good job using dynamic states to guide the user on the requirements.
Ensure that the
passwordCheckincludes any newly introduced constraints from your security team in future iterations (e.g., more granular checks or updated patterns).backend/src/services/auth/auth-password-service.ts (3)
17-17: Import of UserEncryption type is correct.
This import is duly used in the file for handling V2 encryption references.
20-23: New types for the V2 password reset.
The introduction ofResetPasswordV2TypeandTResetPasswordV2DTOis clear and aligns with the new password reset logic.
500-501: Export of new functions.
ExportingresetPasswordV2alongsidesetupPasswordensures external modules can access the new logic for V2 flows. Good addition.
frontend/src/pages/auth/PasswordResetPage/components/ConfirmEmailStep.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/src/services/auth/auth-password-service.ts (1)
126-126: Consider using optional chaining for better code safety.The static analysis tool suggests using optional chaining here.
- if (user && user.isAccepted) { + if (user?.isAccepted) {🧰 Tools
🪛 Biome (1.9.4)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/services/auth/auth-password-service.ts(6 hunks)frontend/src/pages/user/PersonalSettingsPage/components/ChangePasswordSection/ChangePasswordSection.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
backend/src/services/auth/auth-password-service.ts (2)
Learnt from: DanielHougaard
PR: Infisical/infisical#3198
File: backend/src/services/auth/auth-password-service.ts:187-271
Timestamp: 2025-03-07T00:55:56.068Z
Learning: The newly implemented resetPasswordV2 function in auth-password-service.ts handles password resets for the v2 user encryption architecture but should include session invalidation when passwords are changed.
Learnt from: DanielHougaard
PR: Infisical/infisical#3198
File: backend/src/services/auth/auth-password-service.ts:7-8
Timestamp: 2025-03-07T00:47:28.763Z
Learning: The encryption function name has a known typo and is consistently defined as "infisicalSymmetricEncypt" (missing 'r') throughout the Infisical codebase. This spelling should be maintained for consistency until a codebase-wide renaming effort is undertaken.
🪛 Biome (1.9.4)
backend/src/services/auth/auth-password-service.ts
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Check API Changes
- GitHub Check: Check TS and Lint
- GitHub Check: Run integration test
🔇 Additional comments (11)
frontend/src/pages/user/PersonalSettingsPage/components/ChangePasswordSection/ChangePasswordSection.tsx (5)
14-16: Good addition of imports for the new password reset flow.The imports for
useResetUserPasswordV2,UserEncryptionVersion, anduseNavigateare appropriate additions to support the new password reset functionality and improved navigation.
40-40: Good implementation of React Router's navigation.Using the
useNavigatehook from @tanstack/react-router is a better practice than directly manipulatingwindow.location.href.
53-53: Appropriate setup of the mutation hook.Destructuring the
mutateAsyncmethod from theuseResetUserPasswordV2hook allows for clean and convenient usage in the component.
64-76: Well-structured conditional password reset logic.The implementation properly handles different user encryption versions:
- For V2 users, it uses the new
resetPasswordV2function- For non-V2 users, it maintains backward compatibility with
attemptChangePasswordThis matches the PR objective of supporting password reset for v2 users while maintaining compatibility.
85-85: Improved navigation implementation.Replacing direct window location manipulation with React Router's navigation is a better practice for single-page applications.
backend/src/services/auth/auth-password-service.ts (6)
7-8: The encryption function import retains the known typo.Based on the learned context, the typo in
infisicalSymmetricEncypt(missing 'r') is consistent throughout the codebase and should be maintained until a codebase-wide renaming effort.
122-150: Good implementation of timing attack protection.The changes to
sendPasswordResetEmailinclude an excellent security improvement:
- Conditional email sending if user exists and is accepted
- A fixed 8-second request duration to mitigate timing attacks that could reveal whether an email exists
This addresses the timing attack vulnerability mentioned in the PR objectives.
🧰 Tools
🪛 Biome (1.9.4)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
160-163: Proper user encryption data validation.Adding a check for user encryption data existence and throwing a
BadRequestErrorwith a clear message improves error handling.
184-184: Enhanced response with encryption version information.Including the
userEncryptionVersionin the response is important for the frontend to determine which password reset flow to use.
187-267: Well-implemented password reset logic for V2 users.The
resetPasswordV2function is comprehensive and well-structured:
- Validates user existence and appropriate conditions
- Handles different reset types (LoggedInReset requiring old password verification)
- Properly decrypts and re-encrypts the user's private key with the new password
- Updates all necessary user encryption data
- Revokes all sessions after password change for security
I notice you've implemented session invalidation as previously suggested, which is an important security measure.
496-497: Export of new functionalities is properly handled.The
resetPasswordV2function is correctly exported, making it available for use in other modules.
|
Will be doing a complete application test and checking the auth part once again! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/services/auth/auth-password-service.ts (1)
122-148: Good security improvement to prevent timing attacks.The refactoring of
sendPasswordResetEmailto run the email sending process asynchronously in the background is an excellent security practice. This prevents timing attacks by ensuring consistent response times regardless of whether a user exists in the system.Consider an alternative to the 8-second delay mentioned in the previous comments:
void sendEmail().catch((err) => logger.error(err, "Failed to send password reset email")); + return { message: "If your email exists in our system, you will receive a password reset link shortly." };This approach provides immediate feedback to users while maintaining security, similar to how MongoDB and SendGrid handle password resets.
🧰 Tools
🪛 Biome (1.9.4)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/services/auth/auth-password-service.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
backend/src/services/auth/auth-password-service.ts (2)
Learnt from: DanielHougaard
PR: Infisical/infisical#3198
File: backend/src/services/auth/auth-password-service.ts:187-271
Timestamp: 2025-03-07T00:55:56.068Z
Learning: The newly implemented resetPasswordV2 function in auth-password-service.ts handles password resets for the v2 user encryption architecture but should include session invalidation when passwords are changed.
Learnt from: DanielHougaard
PR: Infisical/infisical#3198
File: backend/src/services/auth/auth-password-service.ts:7-8
Timestamp: 2025-03-07T00:47:28.763Z
Learning: The encryption function name has a known typo and is consistently defined as "infisicalSymmetricEncypt" (missing 'r') throughout the Infisical codebase. This spelling should be maintained for consistency until a codebase-wide renaming effort is undertaken.
🪛 Biome (1.9.4)
backend/src/services/auth/auth-password-service.ts
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check API Changes
- GitHub Check: Check TS and Lint
- GitHub Check: Run integration test
🔇 Additional comments (5)
backend/src/services/auth/auth-password-service.ts (5)
7-8: The encryption function name is correctly spelled as per codebase convention.The function is consistently defined with this spelling (
infisicalSymmetricEncyptmissing the 'r') throughout the Infisical codebase. Maintaining this spelling is appropriate until a codebase-wide renaming effort is undertaken.
181-182: Appropriate return value enhancement for encryption version.Including the user's encryption version in the response is a good addition that enables the client to determine the appropriate password reset flow.
184-264: Well-implemented password reset functionality with proper security measures.The
resetPasswordV2function implements a comprehensive and secure approach to password resets for the v2 user encryption architecture. Key strengths include:
- Thorough validation of user existence, password presence, and authentication methods
- Different handling for logged-in users vs. recovery token resets
- Proper decryption and re-encryption of the user's private key
- Complete session invalidation after password reset for security
The implementation correctly addresses the requirement to allow password resets without an emergency kit while maintaining security.
263-263: Excellent security practice for session management.Invalidating all user sessions after a password change is a critical security best practice. This ensures that any potentially compromised sessions are terminated when credentials change.
493-495: Function exports are properly updated.The
resetPasswordV2function is correctly added to the exported object, making it available for the new password reset routes.
|
Application testing looks good. Will do one more code check on auth tmrw. |
Description 📣
This PR introduces password resets without the need for having an emergency kit (only for v2 user encryption). It remains backwards compatible for users that aren't on the new v2 user encryption architecture.
Additionally I've also removed the backup key's downloads, as they are no longer needed.
Other:
a. I also moved us away from react state, and we now use react-hook-form instead.
Type ✨
Summary by CodeRabbit
New Features
Refactor
Bug Fixes