From 79773dab95640b0268ebb766c9a54adc958e4a9d Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Wed, 11 Sep 2024 11:01:57 -0700 Subject: [PATCH] Mobile,Desktop: Fix unable to change incorrect decryption password if the same as the master password (#11026) --- .eslintignore | 1 + .gitignore | 1 + .../EncryptionConfigScreen/utils.test.ts | 88 +++++++++++++++++++ .../EncryptionConfigScreen/utils.ts | 9 +- 4 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 packages/lib/components/EncryptionConfigScreen/utils.test.ts diff --git a/.eslintignore b/.eslintignore index 5d47d6656..dad96e30e 100644 --- a/.eslintignore +++ b/.eslintignore @@ -917,6 +917,7 @@ packages/lib/commands/historyForward.js packages/lib/commands/index.js packages/lib/commands/openMasterPasswordDialog.js packages/lib/commands/synchronize.js +packages/lib/components/EncryptionConfigScreen/utils.test.js packages/lib/components/EncryptionConfigScreen/utils.js packages/lib/components/shared/NoteList/getEmptyFolderMessage.js packages/lib/components/shared/config/config-shared.js diff --git a/.gitignore b/.gitignore index c2736ad23..58e0340b8 100644 --- a/.gitignore +++ b/.gitignore @@ -894,6 +894,7 @@ packages/lib/commands/historyForward.js packages/lib/commands/index.js packages/lib/commands/openMasterPasswordDialog.js packages/lib/commands/synchronize.js +packages/lib/components/EncryptionConfigScreen/utils.test.js packages/lib/components/EncryptionConfigScreen/utils.js packages/lib/components/shared/NoteList/getEmptyFolderMessage.js packages/lib/components/shared/config/config-shared.js diff --git a/packages/lib/components/EncryptionConfigScreen/utils.test.ts b/packages/lib/components/EncryptionConfigScreen/utils.test.ts new file mode 100644 index 000000000..c71f1460c --- /dev/null +++ b/packages/lib/components/EncryptionConfigScreen/utils.test.ts @@ -0,0 +1,88 @@ +import MasterKey from '../../models/MasterKey'; +import EncryptionService, { EncryptionMethod } from '../../services/e2ee/EncryptionService'; +import { MasterKeyEntity } from '../../services/e2ee/types'; +import { setEncryptionEnabled } from '../../services/synchronizer/syncInfoUtils'; +import { setupDatabaseAndSynchronizer, switchClient } from '../../testing/test-utils'; +import { usePasswordChecker } from './utils'; +import { renderHook } from '@testing-library/react-hooks'; + +interface WrappedPasswordCheckerProps { + masterKeys: MasterKeyEntity[]; + activeMasterKeyId: string; + masterPassword: string; + passwords: Record; +} + +const useWrappedPasswordChecker = ({ + masterKeys = [], + activeMasterKeyId = '', + masterPassword = '', + passwords = {}, +}: WrappedPasswordCheckerProps) => usePasswordChecker( + masterKeys, + activeMasterKeyId, + masterPassword, + passwords, +); + +describe('EncryptionConfigScreen/utils', () => { + beforeEach(async () => { + await setupDatabaseAndSynchronizer(0); + await switchClient(0); + setEncryptionEnabled(true); + }); + + test('should not mark keys as master password keys if the master password is incorrect for that key', async () => { + const makeMasterKey = async (password: string) => { + const result = await EncryptionService.instance().generateMasterKey(password, { + encryptionMethod: EncryptionMethod.SJCL4, + }); + return await MasterKey.save(result); + }; + + const activeMasterKey = await makeMasterKey('master-password'); + const secondaryMasterKey = await makeMasterKey('some other password'); + const masterKeys = [ + activeMasterKey, + secondaryMasterKey, + ]; + + const initialProps = { + masterKeys, + activeMasterKeyId: activeMasterKey.id, + masterPassword: 'master-password', + passwords: { + [activeMasterKey.id]: 'master-password', + [secondaryMasterKey.id]: 'some other password', + }, + }; + const hook = renderHook(useWrappedPasswordChecker, { + initialProps, + }); + + // Different password from the master password should cause the secondary key + // to be marked as not using the master password. + await hook.waitFor(() => { + expect(hook.result.current.masterPasswordKeys).toMatchObject({ + [activeMasterKey.id]: true, + [secondaryMasterKey.id]: false, + }); + }); + + // Same password as the master password but fails to decrypt: Should not be marked + // as using the master password. + hook.rerender({ + ...initialProps, + passwords: { + ...initialProps.passwords, + [secondaryMasterKey.id]: 'primary', + }, + }); + await hook.waitFor(() => { + expect(hook.result.current.masterPasswordKeys).toMatchObject({ + [activeMasterKey.id]: true, + [secondaryMasterKey.id]: false, + }); + }); + }); +}); diff --git a/packages/lib/components/EncryptionConfigScreen/utils.ts b/packages/lib/components/EncryptionConfigScreen/utils.ts index cce4549dc..49560e47a 100644 --- a/packages/lib/components/EncryptionConfigScreen/utils.ts +++ b/packages/lib/components/EncryptionConfigScreen/utils.ts @@ -160,15 +160,20 @@ export const usePasswordChecker = (masterKeys: MasterKeyEntity[], activeMasterKe const newPasswordChecks: PasswordChecks = {}; const newMasterPasswordKeys: PasswordChecks = {}; + const masterPasswordOk = masterPassword ? await masterPasswordIsValid(masterPassword, masterKeys.find(mk => mk.id === activeMasterKeyId)) : true; + newPasswordChecks['master'] = masterPasswordOk; + for (let i = 0; i < masterKeys.length; i++) { const mk = masterKeys[i]; const password = await findMasterKeyPassword(EncryptionService.instance(), mk, passwords); const ok = password ? await EncryptionService.instance().checkMasterKeyPassword(mk, password) : false; newPasswordChecks[mk.id] = ok; - newMasterPasswordKeys[mk.id] = password === masterPassword; + + // Even if the password matches the master password, it isn't a master password key if the + // master password can't decrypt this key. + newMasterPasswordKeys[mk.id] = password === masterPassword && (ok || !masterPasswordOk); } - newPasswordChecks['master'] = masterPassword ? await masterPasswordIsValid(masterPassword, masterKeys.find(mk => mk.id === activeMasterKeyId)) : true; if (event.cancelled) return;