Mobile: Fixes #14975: Add missing validations for setting master password and encryption passwords#14983
Mobile: Fixes #14975: Add missing validations for setting master password and encryption passwords#14983mrjo118 wants to merge 25 commits intolaurent22:devfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughUpdated the mobile encryption configuration to validate entered master passwords against the active master key (via Changes
Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app-mobile/components/screens/encryption-config.tsx (1)
177-178: Consider handling the case where the active master key is not found.
props.masterKeys.find(...)may returnundefinedifactiveMasterKeyIddoesn't match any key in the array. WhilstmasterPasswordIsValidfalls back togetDefaultMasterKey()internally, this silent fallback could validate against a different key than the user expects, potentially masking a misconfiguration.Suggestion: explicit handling
-} else if (!(await masterPasswordIsValid(password, props.masterKeys.find(mk => mk.id === props.activeMasterKeyId)))) { +} else { + const activeMasterKey = props.masterKeys.find(mk => mk.id === props.activeMasterKeyId); + if (!activeMasterKey) { + // Falls back to getDefaultMasterKey() inside masterPasswordIsValid - this is expected behaviour + } + if (!(await masterPasswordIsValid(password, activeMasterKey))) { + throw new Error(_('Invalid password. Please try again. If you have forgotten your password you will need to reset it.')); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-mobile/components/screens/encryption-config.tsx` around lines 177 - 178, The code calls masterPasswordIsValid(..., props.masterKeys.find(mk => mk.id === props.activeMasterKeyId)) which can pass undefined when the active master key isn't found; instead explicitly resolve the active master key first (e.g., const activeKey = props.masterKeys.find(...)), and if activeKey is undefined throw a clear, specific error (or surface a UI message) before calling masterPasswordIsValid so you don't silently fall back to getDefaultMasterKey(); update the block around masterPasswordIsValid and props.activeMasterKeyId/props.masterKeys to perform this explicit check and error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-mobile/components/screens/encryption-config.tsx`:
- Around line 107-113: The onSave handler calls masterPasswordIsValid(password,
mk) which throws Error('Password is empty') when password is falsy, so clicking
Save with an empty field causes an unhandled rejection; fix by guarding the
empty-password case or catching exceptions: in the onSave function (the handler
that currently awaits masterPasswordIsValid) first validate that password is
non-empty and show the alert, or wrap the await masterPasswordIsValid(...) call
in a try/catch and show the same _('Password is invalid. Please try again.')
alert on error, then only call onSavePasswordClick(mk, inputPasswords) when
validation succeeds.
---
Nitpick comments:
In `@packages/app-mobile/components/screens/encryption-config.tsx`:
- Around line 177-178: The code calls masterPasswordIsValid(...,
props.masterKeys.find(mk => mk.id === props.activeMasterKeyId)) which can pass
undefined when the active master key isn't found; instead explicitly resolve the
active master key first (e.g., const activeKey = props.masterKeys.find(...)),
and if activeKey is undefined throw a clear, specific error (or surface a UI
message) before calling masterPasswordIsValid so you don't silently fall back to
getDefaultMasterKey(); update the block around masterPasswordIsValid and
props.activeMasterKeyId/props.masterKeys to perform this explicit check and
error path.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ba2f231-1804-49c9-b817-efe2c7842e1e
📒 Files selected for processing (1)
packages/app-mobile/components/screens/encryption-config.tsx
As per #14975, it is possible to face issues with items being unable to be decrypted when entering incorrect encryption passwords on the mobile app, due to lack of necessary validations which exist on the desktop app.
This PR addresses these issues by adding the relevant validations. This includes:
Other issues observed during testing, which are not addressed in this PR:
This fixes #14975
Testing
See video demonstrating the main flows (creating a master key locally, entering master password for syncing with an existing target, and validating password is validated when disabling and re-enabling encryption after a profile has been synced):
1NdPaKxKmp.mp4
Part 2 - testing the scenario where a user has setup encryption with an incorrect password before setting up sync:
https://github.com/user-attachments/assets/f80c522b-6d99-4e63-944b-b6d41e98d8da