fix: require current password before changing password#3572
Open
shivv23 wants to merge 2 commits into
Open
Conversation
- Validate currentPassword by decrypting account config (same pattern as unlock) - Add currentPassword field to the Settings UI modal - Update MessageChangePassword type with currentPassword arg - Add i18n key for current password label
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Settings password-change flow now collects current and new passwords, labels the current-password field, and sends both values to the background action. The background action verifies the current password, re-encrypts stored account data with the new password, updates state, and persists the result. ChangesPassword change verification
Sequence Diagram(s)sequenceDiagram
participant SettingsScreen
participant changePassword
participant state
participant saveToStorage
SettingsScreen->>changePassword: currentPassword and password
changePassword->>state: read account configs
changePassword->>changePassword: verify currentPassword and re-encrypt data
changePassword->>state: update accounts and stored password
changePassword->>saveToStorage: persist updated state
changePassword-->>SettingsScreen: {}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/screens/Settings/index.tsx`:
- Around line 50-53: The error handling in Settings/index.tsx is showing raw
backend text through toast.error in the catch block, which bypasses
react-i18next. Update the password-change flow to translate stable backend error
codes (including the new invalid current password case) into
t("change_password.errors...") keys before toasting, and keep the existing catch
logic in the change password handler from exposing e.message directly.
- Line 21: The currentPassword field in Settings/index.tsx is being handled in a
way that can both alter the user’s input and leave stale state behind. Update
the form flow so the verification logic in the submit handler uses the exact
currentPassword value without trimming or otherwise modifying it, and make the
field controlled like the other form inputs. Also clear formData.currentPassword
inside closeModal() and on any reset path so a previous value cannot be reused
on the next modal open; use the existing closeModal and submit/verification
logic as the touchpoints for the fix.
In `@src/extension/background-script/actions/settings/changePassword.ts`:
- Around line 29-36: The changePassword flow in the account re-encryption logic
is only shallow-copying the accounts map, so mutating each entry’s config and
other secret fields can leak into live state before the whole loop succeeds.
Update the logic in changePassword to deep-clone each account object before
decrypting/re-encrypting fields, then write the encrypted values back on the
cloned object and only replace the original state after all per-account updates
complete; apply this consistently for the config, nostrPrivateKey, and mnemonic
handling in the same function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa0709a8-27a1-4eee-81b3-df0bfb67c81a
📒 Files selected for processing (4)
src/app/screens/Settings/index.tsxsrc/extension/background-script/actions/settings/changePassword.tssrc/i18n/locales/en/translation.jsonsrc/types.ts
- Remove .trim() on currentPassword so passwords with leading/trailing spaces work - Add value prop to currentPassword TextField (make it controlled) - Move formData reset into closeModal() and remove duplicate from updateAccountPassword - Deep-clone each account entry in changePassword before re-encrypting fields
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the changes you have made in this PR
Require the user to enter their current password before allowing a password change. Previously, anyone with access to an unlocked extension could change the password without re-authentication.
The fix decrypts the stored account config with the provided current password (same pattern used in
unlock.ts) before accepting the new password.Link this PR to an issue
Fixes #2865
Type of change
fix: Bug fix (non-breaking change which fixes an issue)How has this been tested?
yarn tsc:compile— 0 errorsyarn lint:js— 0 errors, 0 warningsyarn test:unit— all 75 suites / 168 tests passChecklist
Summary by CodeRabbit