Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions app/core/BackupVault/backupVault.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,32 @@ describe('backupVault file', () => {
expect(response).toEqual(mockedSuccessResponse);
});

it('should still succeed if reading the existing backup throws (e.g. Android Keystore key invalidation)', async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Triggered by project rule: Unit Testing Guidelines

Reviewed by Cursor Bugbot for commit 43ff1dd. Configure here.

const newVault = 'new-vault';

// Simulate a stale entry already in the keychain
await setInternetCredentials(
VAULT_BACKUP_KEY,
VAULT_BACKUP_KEY,
dummyPassword,
);

// Simulate Android Keystore throwing on the read
(getInternetCredentials as jest.Mock).mockImplementationOnce(() => {
throw new Error('Android Keystore key permanently invalidated');
});

const keyringState: KeyringControllerState = {
vault: newVault,
keyrings: [],
isUnlocked: false,
};

const response = await backupVault(keyringState);

expect(response).toEqual({ success: true, vault: newVault });
});

it('should reset vault before backup', async () => {
const mockedSuccessResponse = { success: true };

Expand Down
16 changes: 15 additions & 1 deletion app/core/BackupVault/backupVault.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,21 @@ export async function backupVault(

try {
// Does a primary backup exist?
const existingBackup = await getInternetCredentials(VAULT_BACKUP_KEY);
// Wrapped in its own try/catch because Android Keystore key invalidation
// (e.g. biometric enrollment change, Android 16 behavioural change) causes
// getInternetCredentials to throw rather than return false/undefined.
// If the read fails we treat it as "no existing backup" so the fresh
// backup can still be written below.
let existingBackup;
try {
existingBackup = await getInternetCredentials(VAULT_BACKUP_KEY);
} catch (readError) {
Logger.log(
readError,
'backupVault: failed to read existing backup, proceeding with fresh backup',
);
await clearAllVaultBackups();
Comment thread
tommasini marked this conversation as resolved.
Outdated
}

// An existing backup exists, backup it to the temp key
if (existingBackup && existingBackup.password) {
Expand Down
Loading