fix: wrap vault backup initial read in try/catch to survive Android Keystore key invalidation#29752
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 43ff1dd. Configure here.
| expect(response).toEqual(mockedSuccessResponse); | ||
| }); | ||
|
|
||
| it('should still succeed if reading the existing backup throws (e.g. Android Keystore key invalidation)', async () => { |
There was a problem hiding this comment.
Test name uses banned "should" prefix
Low Severity
The new test name 'should still succeed if reading the existing backup throws...' uses the word "should", which is banned per the unit testing guidelines ("NEVER use 'should' in test names — this is a hard rule with zero exceptions"). An action-oriented alternative like 'completes backup when reading existing backup throws due to Keystore key invalidation' would conform to the rule.
Triggered by project rule: Unit Testing Guidelines
Reviewed by Cursor Bugbot for commit 43ff1dd. Configure here.
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
No E2E tags are warranted for this change. Performance Test Selection: |
|





Description
Reason for the change:
On Android, when a user changes their biometric enrollment (or due to an Android 16 Keystore behavioral change), the Keystore key backing the vault backup entry can be permanently invalidated. In that state,
getInternetCredentials(VAULT_BACKUP_KEY)throws an exception rather than returningfalse/undefined. Because that call sat inside the outertry/catchinbackupVault, the thrown error was caught at the top-level catch, which logged "Vault backup failed" to Sentry and returned{ success: false }— even though the underlying vault was fine and a fresh backup could have been written successfully.Solution:
Wrap the initial
getInternetCredentialsread in its own innertry/catch. If the read throws (key invalidated), we:Logger.log(no Sentry noise)clearAllVaultBackups()This makes
backupVaultresilient to stale Android Keystore state and prevents a false "Vault backup failed" from ever surfacing when the real backup operation can succeed.Changelog
CHANGELOG entry: Fixed an issue on Android where vault backup could silently fail after a biometric enrollment change or Keystore key invalidation
Related issues
No issue: bug surfaced via Sentry report (
extra.message: "Vault backup failed") — Android Keystore key invalidation causinggetInternetCredentialsto throw insidebackupVaultManual testing steps
Screenshots/Recordings
Before
N/A
After
N/A
Pre-merge author checklist
Performance checks (if applicable)
trace()for usage andaddTokenfor an exampleFor performance guidelines and tooling, see the Performance Guide.
Pre-merge reviewer checklist
Made with Cursor
Note
Low Risk
Low risk, localized change that only alters error handling around reading existing keychain credentials and adds a unit test for the throwing path. Main risk is potentially masking unexpected read failures by proceeding to overwrite the backup.
Overview
Makes
backupVaultresilient to Android Keystore/keychain read exceptions by wrapping the initialgetInternetCredentials(VAULT_BACKUP_KEY)call in an inner try/catch and proceeding as if no existing backup was found (while logging viaLogger.log).Adds a unit test ensuring vault backup still reports success when the existing-backup read throws (simulating Android Keystore key invalidation).
Reviewed by Cursor Bugbot for commit 6a6edf9. Bugbot is set up for automated code reviews on this repo. Configure here.