From d82ae6474182de1f09d5bfa6592e42eadd8e28fe Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 11 Jun 2025 19:29:53 +0200 Subject: [PATCH 01/44] refactor!: drop uncached encryption support --- .../src/KeyringController.ts | 410 +++++++++++------- 1 file changed, 252 insertions(+), 158 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 65b3954f873..ee5e463e219 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -250,16 +250,8 @@ export type KeyringControllerOptions = { keyringBuilders?: { (): EthKeyring; type: string }[]; messenger: KeyringControllerMessenger; state?: { vault?: string; keyringsMetadata?: KeyringMetadata[] }; -} & ( - | { - cacheEncryptionKey: true; - encryptor?: ExportableKeyEncryptor; - } - | { - cacheEncryptionKey?: false; - encryptor?: GenericEncryptor | ExportableKeyEncryptor; - } -); + encryptor?: ExportableKeyEncryptor; +}; /** * A keyring object representation. @@ -321,12 +313,26 @@ export type SerializedKeyring = { metadata?: KeyringMetadata; }; +/** + * Cached encryption key used to encrypt/decrypt the vault. + */ +type CachedEncryptionKey = { + /** + * The exported encryption key string. + */ + exported: string; + /** + * The salt used to derive the encryption key. + */ + salt?: string; +}; + /** * State/data that can be updated during a `withKeyring` operation. */ type SessionState = { keyrings: SerializedKeyring[]; - password?: string; + encryptionKey?: CachedEncryptionKey; }; /** @@ -427,6 +433,31 @@ export type ExportableKeyEncryptor = * @returns The encryption key. */ importKey: (key: string) => Promise; + /** + * Exports the encryption key as a string. + * + * @param key - The encryption key to export. + * @returns The exported key string. + */ + exportKey: (key: EncryptionKey) => Promise; + /** + * Derives an encryption key from a password. + * + * @param password - The password to derive the key from. + * @param salt - The salt to use for key derivation. + * @param exportable - Whether the key should be exportable or not. + * @param options - Optional key derivation options. + * @returns The derived encryption key. + */ + keyFromPassword: ( + password: string, + salt: string, + exportable?: boolean, + ) => Promise; + /** + * Generates a random salt for key derivation. + */ + generateSalt: typeof encryptorUtils.generateSalt; }; export type KeyringSelector = @@ -639,15 +670,13 @@ export class KeyringController extends BaseController< readonly #keyringBuilders: { (): EthKeyring; type: string }[]; - readonly #encryptor: GenericEncryptor | ExportableKeyEncryptor; - - readonly #cacheEncryptionKey: boolean; + readonly #encryptor: ExportableKeyEncryptor; #keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; #unsupportedKeyrings: SerializedKeyring[]; - #password?: string; + #encryptionKey?: CachedEncryptionKey; #qrKeyringStateListener?: ( state: ReturnType, @@ -691,17 +720,11 @@ export class KeyringController extends BaseController< ? keyringBuilders.concat(defaultKeyringBuilders) : defaultKeyringBuilders; + assertIsExportableKeyEncryptor(encryptor); this.#encryptor = encryptor; this.#keyrings = []; this.#unsupportedKeyrings = []; - // This option allows the controller to cache an exported key - // for use in decrypting and encrypting data without password - this.#cacheEncryptionKey = Boolean(options.cacheEncryptionKey); - if (this.#cacheEncryptionKey) { - assertIsExportableKeyEncryptor(encryptor); - } - this.#registerMessageHandlers(); } @@ -1167,7 +1190,7 @@ export class KeyringController extends BaseController< return this.#withRollback(async () => { this.#unsubscribeFromQRKeyringsEvents(); - this.#password = undefined; + this.#encryptionKey = undefined; await this.#clearKeyrings(); this.update((state) => { @@ -1419,17 +1442,7 @@ export class KeyringController extends BaseController< return this.#persistOrRollback(async () => { assertIsValidPassword(password); - - this.#password = password; - // We need to clear encryption key and salt from state - // to force the controller to re-encrypt the vault using - // the new password. - if (this.#cacheEncryptionKey) { - this.update((state) => { - delete state.encryptionKey; - delete state.encryptionSalt; - }); - } + await this.#deriveEncryptionKey(password); }); } @@ -1438,19 +1451,13 @@ export class KeyringController extends BaseController< * using the given encryption key and salt. * * @param encryptionKey - Key to unlock the keychain. - * @param encryptionSalt - Salt to unlock the keychain. * @returns Promise resolving when the operation completes. */ - async submitEncryptionKey( - encryptionKey: string, - encryptionSalt: string, - ): Promise { + async submitEncryptionKey(encryptionKey: string): Promise { const { newMetadata } = await this.#withRollback(async () => { - const result = await this.#unlockKeyrings( - undefined, - encryptionKey, - encryptionSalt, - ); + const result = await this.#unlockKeyrings({ + exportedEncryptionKey: encryptionKey, + }); this.#setUnlocked(); return result; }); @@ -1479,7 +1486,7 @@ export class KeyringController extends BaseController< */ async submitPassword(password: string): Promise { const { newMetadata } = await this.#withRollback(async () => { - const result = await this.#unlockKeyrings(password); + const result = await this.#unlockKeyrings({ password }); this.#setUnlocked(); return result; }); @@ -2091,13 +2098,68 @@ export class KeyringController extends BaseController< delete state.encryptionSalt; }); - this.#password = password; + await this.#deriveEncryptionKey(password); await this.#clearKeyrings(); await this.#createKeyringWithFirstAccount(keyring.type, keyring.opts); this.#setUnlocked(); } + /** + * Derive the vault encryption key from the provided password, and + * assign it to the class variable for later use with cryptographic + * functions. + * + * When the controller has a vault in its state, the key is derived + * using the salt from the vault. If the vault is empty, a new salt + * is generated and used to derive the key. + * + * @param password - The password to use for decryption or derivation. + */ + async #deriveEncryptionKey(password: string): Promise { + this.#assertControllerMutexIsLocked(); + const { vault } = this.state; + + if (typeof password !== 'string') { + throw new TypeError(KeyringControllerError.WrongPasswordType); + } + + let salt: string; + if (vault) { + salt = JSON.parse(vault).salt; + } else { + salt = this.#encryptor.generateSalt(); + } + + this.#encryptionKey = { + salt, + exported: await this.#encryptor.exportKey( + await this.#encryptor.keyFromPassword(password, salt, true), + ), + }; + } + + /** + * Use the provided encryption key and salt to set the + * encryptionKey class variable. This method is used + * when the user provides an encryption key and salt + * to unlock the keychain, instead of using a password. + * + * @param encryptionKey - The encryption key to use. + * @param encryptionSalt - The salt to use for the encryption key. + */ + async #useEncryptionKey( + encryptionKey: string, + encryptionSalt: string, + ): Promise { + this.#assertControllerMutexIsLocked(); + + this.#encryptionKey = { + salt: encryptionSalt, + exported: encryptionKey, + }; + } + /** * Internal non-exclusive method to verify the seed phrase. * @@ -2203,7 +2265,7 @@ export class KeyringController extends BaseController< async #getSessionState(): Promise { return { keyrings: await this.#getSerializedKeyrings(), - password: this.#password, + encryptionKey: this.#encryptionKey, }; } @@ -2237,79 +2299,138 @@ export class KeyringController extends BaseController< return { keyrings, newMetadata }; } + // async #unlockKeyrings( + // password: string | undefined, + // encryptionKey?: string, + // encryptionSalt?: string, + // ): Promise<{ + // keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; + // newMetadata: boolean; + // }> { + // return this.#withVaultLock(async () => { + // const encryptedVault = this.state.vault; + // if (!encryptedVault) { + // throw new Error(KeyringControllerError.VaultError); + // } + // + // let vault; + // const updatedState: Partial = {}; + // + // if (this.#cacheEncryptionKey) { + // assertIsExportableKeyEncryptor(this.#encryptor); + // + // if (password) { + // const result = await this.#encryptor.decryptWithDetail( + // password, + // encryptedVault, + // ); + // vault = result.vault; + // this.#password = password; + // + // updatedState.encryptionKey = result.exportedKeyString; + // updatedState.encryptionSalt = result.salt; + // } else { + // const parsedEncryptedVault = JSON.parse(encryptedVault); + // + // if (encryptionSalt !== parsedEncryptedVault.salt) { + // throw new Error(KeyringControllerError.ExpiredCredentials); + // } + // + // if (typeof encryptionKey !== 'string') { + // throw new TypeError(KeyringControllerError.WrongPasswordType); + // } + // + // const key = await this.#encryptor.importKey(encryptionKey); + // vault = await this.#encryptor.decryptWithKey( + // key, + // parsedEncryptedVault, + // ); + // + // // This call is required on the first call because encryptionKey + // // is not yet inside the memStore + // updatedState.encryptionKey = encryptionKey; + // // we can safely assume that encryptionSalt is defined here + // // because we compare it with the salt from the vault + // // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + // updatedState.encryptionSalt = encryptionSalt!; + // } + // } else { + // if (typeof password !== 'string') { + // throw new TypeError(KeyringControllerError.WrongPasswordType); + // } + // + // vault = await this.#encryptor.decrypt(password, encryptedVault); + // this.#password = password; + // } + // + // if (!isSerializedKeyringsArray(vault)) { + // throw new Error(KeyringControllerError.VaultDataError); + // } + // + // const { keyrings, newMetadata } = + // await this.#restoreSerializedKeyrings(vault); + // + // const updatedKeyrings = await this.#getUpdatedKeyrings(); + // + // this.update((state) => { + // state.keyrings = updatedKeyrings; + // if (updatedState.encryptionKey || updatedState.encryptionSalt) { + // state.encryptionKey = updatedState.encryptionKey; + // state.encryptionSalt = updatedState.encryptionSalt; + // } + // }); + // + // return { keyrings, newMetadata }; + // }); + // } + // + /** * Unlock Keyrings, decrypting the vault and deserializing all * keyrings contained in it, using a password or an encryption key with salt. * - * @param password - The keyring controller password. - * @param encryptionKey - An exported key string to unlock keyrings with. - * @param encryptionSalt - The salt used to encrypt the vault. + * @param credentials - The credentials to unlock the keyrings. * @returns A promise resolving to the deserialized keyrings array. */ async #unlockKeyrings( - password: string | undefined, - encryptionKey?: string, - encryptionSalt?: string, + credentials: + | { + password: string; + } + | { + exportedEncryptionKey: string; + }, ): Promise<{ keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; newMetadata: boolean; }> { return this.#withVaultLock(async () => { - const encryptedVault = this.state.vault; - if (!encryptedVault) { + if (!this.state.vault) { throw new Error(KeyringControllerError.VaultError); } + const parsedEncryptedVault = JSON.parse(this.state.vault); - let vault; - const updatedState: Partial = {}; - - if (this.#cacheEncryptionKey) { - assertIsExportableKeyEncryptor(this.#encryptor); - - if (password) { - const result = await this.#encryptor.decryptWithDetail( - password, - encryptedVault, - ); - vault = result.vault; - this.#password = password; - - updatedState.encryptionKey = result.exportedKeyString; - updatedState.encryptionSalt = result.salt; - } else { - const parsedEncryptedVault = JSON.parse(encryptedVault); - - if (encryptionSalt !== parsedEncryptedVault.salt) { - throw new Error(KeyringControllerError.ExpiredCredentials); - } - - if (typeof encryptionKey !== 'string') { - throw new TypeError(KeyringControllerError.WrongPasswordType); - } - - const key = await this.#encryptor.importKey(encryptionKey); - vault = await this.#encryptor.decryptWithKey( - key, - parsedEncryptedVault, - ); - - // This call is required on the first call because encryptionKey - // is not yet inside the memStore - updatedState.encryptionKey = encryptionKey; - // we can safely assume that encryptionSalt is defined here - // because we compare it with the salt from the vault - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - updatedState.encryptionSalt = encryptionSalt!; - } + if ('password' in credentials) { + await this.#deriveEncryptionKey(credentials.password); } else { - if (typeof password !== 'string') { - throw new TypeError(KeyringControllerError.WrongPasswordType); - } + const { exportedEncryptionKey } = credentials; + await this.#useEncryptionKey( + exportedEncryptionKey, + parsedEncryptedVault.salt, + ); + } - vault = await this.#encryptor.decrypt(password, encryptedVault); - this.#password = password; + const encryptionKey = this.#encryptionKey?.exported; + if (!encryptionKey) { + throw new Error(KeyringControllerError.MissingCredentials); } + const key = await this.#encryptor.importKey(encryptionKey); + const vault = await this.#encryptor.decryptWithKey( + key, + parsedEncryptedVault, + ); + if (!isSerializedKeyringsArray(vault)) { throw new Error(KeyringControllerError.VaultDataError); } @@ -2321,10 +2442,8 @@ export class KeyringController extends BaseController< this.update((state) => { state.keyrings = updatedKeyrings; - if (updatedState.encryptionKey || updatedState.encryptionSalt) { - state.encryptionKey = updatedState.encryptionKey; - state.encryptionSalt = updatedState.encryptionSalt; - } + state.encryptionKey = encryptionKey; + state.encryptionSalt = parsedEncryptedVault.salt; }); return { keyrings, newMetadata }; @@ -2341,72 +2460,44 @@ export class KeyringController extends BaseController< // Ensure no duplicate accounts are persisted. await this.#assertNoDuplicateAccounts(); - const { encryptionKey, encryptionSalt, vault } = this.state; - // READ THIS CAREFULLY: - // We do check if the vault is still considered up-to-date, if not, we would not re-use the - // cached key and we will re-generate a new one (based on the password). - // - // This helps doing seamless updates of the vault. Useful in case we change some cryptographic - // parameters to the KDF. - const useCachedKey = - encryptionKey && vault && this.#encryptor.isVaultUpdated?.(vault); - - if (!this.#password && !encryptionKey) { + if (!this.#encryptionKey) { throw new Error(KeyringControllerError.MissingCredentials); } const serializedKeyrings = await this.#getSerializedKeyrings(); if ( - !serializedKeyrings.some((keyring) => keyring.type === KeyringTypes.hd) + !serializedKeyrings.some( + (keyring) => keyring.type === (KeyringTypes.hd as string), + ) ) { throw new Error(KeyringControllerError.NoHdKeyring); } - const updatedState: Partial = {}; - - if (this.#cacheEncryptionKey) { - assertIsExportableKeyEncryptor(this.#encryptor); - - if (useCachedKey) { - const key = await this.#encryptor.importKey(encryptionKey); - const vaultJSON = await this.#encryptor.encryptWithKey( - key, - serializedKeyrings, - ); - vaultJSON.salt = encryptionSalt; - updatedState.vault = JSON.stringify(vaultJSON); - } else if (this.#password) { - const { vault: newVault, exportedKeyString } = - await this.#encryptor.encryptWithDetail( - this.#password, - serializedKeyrings, - ); - - updatedState.vault = newVault; - updatedState.encryptionKey = exportedKeyString; - } - } else { - assertIsValidPassword(this.#password); - updatedState.vault = await this.#encryptor.encrypt( - this.#password, - serializedKeyrings, - ); - } - - if (!updatedState.vault) { - throw new Error(KeyringControllerError.MissingVaultData); + const key = await this.#encryptor.importKey(this.#encryptionKey.exported); + const encryptedVault = await this.#encryptor.encryptWithKey( + key, + serializedKeyrings, + ); + if (this.#encryptionKey.salt) { + // We need to include the salt used to derive + // the encryption key, to be able to derive it + // from password again. + encryptedVault.salt = this.#encryptionKey.salt; } + const updatedState: Partial = { + vault: JSON.stringify(encryptedVault), + encryptionKey: this.#encryptionKey.exported, + encryptionSalt: this.#encryptionKey.salt, + }; const updatedKeyrings = await this.#getUpdatedKeyrings(); this.update((state) => { state.vault = updatedState.vault; state.keyrings = updatedKeyrings; - if (updatedState.encryptionKey) { - state.encryptionKey = updatedState.encryptionKey; - state.encryptionSalt = JSON.parse(updatedState.vault as string).salt; - } + state.encryptionKey = updatedState.encryptionKey; + state.encryptionSalt = updatedState.encryptionSalt; }); return true; @@ -2421,7 +2512,7 @@ export class KeyringController extends BaseController< #isNewEncryptionAvailable(): boolean { const { vault } = this.state; - if (!vault || !this.#password || !this.#encryptor.isVaultUpdated) { + if (!vault || !this.#encryptor.isVaultUpdated) { return false; } @@ -2730,13 +2821,16 @@ export class KeyringController extends BaseController< ): Promise { return this.#withControllerLock(async ({ releaseLock }) => { const currentSerializedKeyrings = await this.#getSerializedKeyrings(); - const currentPassword = this.#password; + const currentEncryptionKey = this.#encryptionKey?.exported; + const currentEncryptionSalt = this.#encryptionKey?.salt; try { return await callback({ releaseLock }); } catch (e) { - // Keyrings and password are restored to their previous state - this.#password = currentPassword; + // Keyrings and encryption credentials are restored to their previous state + if (currentEncryptionKey && currentEncryptionSalt) { + this.#useEncryptionKey(currentEncryptionKey, currentEncryptionSalt); + } await this.#restoreSerializedKeyrings(currentSerializedKeyrings); throw e; From 95b41a50d71973fbcc45f7ecd683b5a67566c851 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 11 Jun 2025 19:31:13 +0200 Subject: [PATCH 02/44] remove commented code --- .../src/KeyringController.ts | 86 ------------------- 1 file changed, 86 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index ee5e463e219..467bbc64528 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2299,92 +2299,6 @@ export class KeyringController extends BaseController< return { keyrings, newMetadata }; } - // async #unlockKeyrings( - // password: string | undefined, - // encryptionKey?: string, - // encryptionSalt?: string, - // ): Promise<{ - // keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; - // newMetadata: boolean; - // }> { - // return this.#withVaultLock(async () => { - // const encryptedVault = this.state.vault; - // if (!encryptedVault) { - // throw new Error(KeyringControllerError.VaultError); - // } - // - // let vault; - // const updatedState: Partial = {}; - // - // if (this.#cacheEncryptionKey) { - // assertIsExportableKeyEncryptor(this.#encryptor); - // - // if (password) { - // const result = await this.#encryptor.decryptWithDetail( - // password, - // encryptedVault, - // ); - // vault = result.vault; - // this.#password = password; - // - // updatedState.encryptionKey = result.exportedKeyString; - // updatedState.encryptionSalt = result.salt; - // } else { - // const parsedEncryptedVault = JSON.parse(encryptedVault); - // - // if (encryptionSalt !== parsedEncryptedVault.salt) { - // throw new Error(KeyringControllerError.ExpiredCredentials); - // } - // - // if (typeof encryptionKey !== 'string') { - // throw new TypeError(KeyringControllerError.WrongPasswordType); - // } - // - // const key = await this.#encryptor.importKey(encryptionKey); - // vault = await this.#encryptor.decryptWithKey( - // key, - // parsedEncryptedVault, - // ); - // - // // This call is required on the first call because encryptionKey - // // is not yet inside the memStore - // updatedState.encryptionKey = encryptionKey; - // // we can safely assume that encryptionSalt is defined here - // // because we compare it with the salt from the vault - // // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - // updatedState.encryptionSalt = encryptionSalt!; - // } - // } else { - // if (typeof password !== 'string') { - // throw new TypeError(KeyringControllerError.WrongPasswordType); - // } - // - // vault = await this.#encryptor.decrypt(password, encryptedVault); - // this.#password = password; - // } - // - // if (!isSerializedKeyringsArray(vault)) { - // throw new Error(KeyringControllerError.VaultDataError); - // } - // - // const { keyrings, newMetadata } = - // await this.#restoreSerializedKeyrings(vault); - // - // const updatedKeyrings = await this.#getUpdatedKeyrings(); - // - // this.update((state) => { - // state.keyrings = updatedKeyrings; - // if (updatedState.encryptionKey || updatedState.encryptionSalt) { - // state.encryptionKey = updatedState.encryptionKey; - // state.encryptionSalt = updatedState.encryptionSalt; - // } - // }); - // - // return { keyrings, newMetadata }; - // }); - // } - // - /** * Unlock Keyrings, decrypting the vault and deserializing all * keyrings contained in it, using a password or an encryption key with salt. From cf465cecc62687eb366a99cdd8f9223c714ccfba Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 12 Jun 2025 12:08:17 +0200 Subject: [PATCH 03/44] update tests --- packages/keyring-controller/jest.config.js | 6 +- .../src/KeyringController.test.ts | 1361 +++++++---------- .../src/KeyringController.ts | 39 +- packages/keyring-controller/src/constants.ts | 1 + .../tests/mocks/mockEncryptor.ts | 19 +- 5 files changed, 609 insertions(+), 817 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 568a60b2b46..b419b2a2e9a 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 94.31, + branches: 94.01, functions: 100, - lines: 98.79, - statements: 98.8, + lines: 98.76, + statements: 98.77, }, }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 08e726c34fb..e4df88b9dcd 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -38,6 +38,7 @@ import { } from './KeyringController'; import MockEncryptor, { MOCK_ENCRYPTION_KEY, + MOCK_KEY, } from '../tests/mocks/mockEncryptor'; import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring'; import { MockKeyring } from '../tests/mocks/mockKeyring'; @@ -64,6 +65,8 @@ const uint8ArraySeed = new Uint8Array( seedWords.split(' ').map((word) => wordlist.indexOf(word)), ).buffer, ); +const mockVault = + '{"data":"{\\"tag\\":{\\"key\\":{\\"password\\":\\"password123\\",\\"salt\\":\\"salt\\"},\\"iv\\":\\"iv\\"},\\"value\\":[{\\"type\\":\\"HD Key Tree\\",\\"data\\":{\\"mnemonic\\":[119,97,114,114,105,111,114,32,108,97,110,103,117,97,103,101,32,106,111,107,101,32,98,111,110,117,115,32,117,110,102,97,105,114,32,97,114,116,105,115,116,32,107,97,110,103,97,114,111,111,32,99,105,114,99,108,101,32,101,120,112,97,110,100,32,104,111,112,101,32,109,105,100,100,108,101,32,103,97,117,103,101],\\"numberOfAccounts\\":1,\\"hdPath\\":\\"m/44\'/60\'/0\'/0\\"},\\"metadata\\":{\\"id\\":\\"01JXEFM7DAX2VJ0YFR4ESNY3GQ\\",\\"name\\":\\"\\"}}]}","iv":"iv","salt":"salt"}'; const privateKey = '1e4e6a4c0c077f4ae8ddfbf372918e61dd0fb4a4cfa592cb16e7546d505e68fc'; const password = 'password123'; @@ -82,7 +85,6 @@ describe('KeyringController', () => { () => new KeyringController({ messenger: buildKeyringControllerMessenger(), - cacheEncryptionKey: true, }), ).not.toThrow(); }); @@ -90,10 +92,9 @@ describe('KeyringController', () => { it('should throw error if cacheEncryptionKey is true and encryptor does not support key export', () => { expect( () => - // @ts-expect-error testing an invalid encryptor new KeyringController({ messenger: buildKeyringControllerMessenger(), - cacheEncryptionKey: true, + // @ts-expect-error testing an invalid encryptor encryptor: { encrypt: jest.fn(), decrypt: jest.fn() }, }), ).toThrow(KeyringControllerError.UnsupportedEncryptionKeyExport); @@ -133,11 +134,11 @@ describe('KeyringController', () => { { skipVaultCreation: true, state: { - vault: 'my vault', + vault: mockVault, }, }, async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: '', @@ -177,11 +178,11 @@ describe('KeyringController', () => { { skipVaultCreation: true, state: { - vault: 'my vault', + vault: mockVault, }, }, async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: 'HD Key Tree', data: '', @@ -283,10 +284,10 @@ describe('KeyringController', () => { it('should throw an error if there is no primary keyring', async () => { await withController( - { skipVaultCreation: true, state: { vault: 'my vault' } }, + { skipVaultCreation: true, state: { vault: mockVault } }, async ({ controller, encryptor }) => { jest - .spyOn(encryptor, 'decrypt') + .spyOn(encryptor, 'decryptWithKey') .mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]); await controller.submitPassword('123'); @@ -547,254 +548,181 @@ describe('KeyringController', () => { }); describe('createNewVaultAndRestore', () => { - [false, true].map((cacheEncryptionKey) => - describe(`when cacheEncryptionKey is ${cacheEncryptionKey}`, () => { - it('should create new vault and restore', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller, initialState }) => { - const initialVault = controller.state.vault; - const initialKeyrings = controller.state.keyrings; - await controller.createNewVaultAndRestore( - password, - uint8ArraySeed, - ); - expect(controller.state).not.toBe(initialState); - expect(controller.state.vault).toBeDefined(); - expect(controller.state.vault).toStrictEqual(initialVault); - expect(controller.state.keyrings).toHaveLength( - initialKeyrings.length, - ); - // new keyring metadata should be generated - expect(controller.state.keyrings).not.toStrictEqual( - initialKeyrings, - ); - }, - ); - }); - - it('should call encryptor.encrypt with the same keyrings if old seedWord is used', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller, encryptor }) => { - const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - const serializedKeyring = await controller.withKeyring( - { type: 'HD Key Tree' }, - async ({ keyring }) => keyring.serialize(), - ); - const currentSeedWord = - await controller.exportSeedPhrase(password); + it('should create new vault and restore', async () => { + await withController(async ({ controller, initialState }) => { + const initialVault = controller.state.vault; + const initialKeyrings = controller.state.keyrings; + await controller.createNewVaultAndRestore(password, uint8ArraySeed); + expect(controller.state).not.toBe(initialState); + expect(controller.state.vault).toBeDefined(); + expect(controller.state.vault).toStrictEqual(initialVault); + expect(controller.state.keyrings).toHaveLength(initialKeyrings.length); + // new keyring metadata should be generated + expect(controller.state.keyrings).not.toStrictEqual(initialKeyrings); + }); + }); + + it('should call encryptor.encryptWithKey with the same keyrings if old seedWord is used', async () => { + await withController(async ({ controller, encryptor }) => { + const encryptSpy = jest.spyOn(encryptor, 'encryptWithKey'); + jest.spyOn(encryptor, 'importKey').mockResolvedValueOnce(MOCK_KEY); + const serializedKeyring = await controller.withKeyring( + { type: 'HD Key Tree' }, + async ({ keyring }) => keyring.serialize(), + ); + const currentSeedWord = await controller.exportSeedPhrase(password); - await controller.createNewVaultAndRestore( - password, - currentSeedWord, - ); + await controller.createNewVaultAndRestore(password, currentSeedWord); - expect(encryptSpy).toHaveBeenCalledWith(password, [ - { - data: serializedKeyring, - type: 'HD Key Tree', - metadata: { - id: expect.any(String), - name: '', - }, - }, - ]); + expect(encryptSpy).toHaveBeenCalledWith(MOCK_KEY, [ + { + data: serializedKeyring, + type: 'HD Key Tree', + metadata: { + id: expect.any(String), + name: '', }, - ); - }); + }, + ]); + }); + }); - it('should throw error if creating new vault and restore without password', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller }) => { - await expect( - controller.createNewVaultAndRestore('', uint8ArraySeed), - ).rejects.toThrow(KeyringControllerError.InvalidEmptyPassword); - }, - ); - }); + it('should throw error if creating new vault and restore without password', async () => { + await withController(async ({ controller }) => { + await expect( + controller.createNewVaultAndRestore('', uint8ArraySeed), + ).rejects.toThrow(KeyringControllerError.InvalidEmptyPassword); + }); + }); - it('should throw error if creating new vault and restoring without seed phrase', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller }) => { - await expect( - controller.createNewVaultAndRestore( - password, - // @ts-expect-error invalid seed phrase - '', - ), - ).rejects.toThrow( - 'Eth-Hd-Keyring: Deserialize method cannot be called with an opts value for numberOfAccounts and no menmonic', - ); - }, - ); - }); + it('should throw error if creating new vault and restoring without seed phrase', async () => { + await withController(async ({ controller }) => { + await expect( + controller.createNewVaultAndRestore( + password, + // @ts-expect-error invalid seed phrase + '', + ), + ).rejects.toThrow( + 'Eth-Hd-Keyring: Deserialize method cannot be called with an opts value for numberOfAccounts and no menmonic', + ); + }); + }); - cacheEncryptionKey && - it('should set encryptionKey and encryptionSalt in state', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller }) => { - await controller.createNewVaultAndRestore( - password, - uint8ArraySeed, - ); - expect(controller.state.encryptionKey).toBeDefined(); - expect(controller.state.encryptionSalt).toBeDefined(); - }, - ); - }); - }), - ); + it('should set encryptionKey and encryptionSalt in state', async () => { + await withController(async ({ controller }) => { + await controller.createNewVaultAndRestore(password, uint8ArraySeed); + expect(controller.state.encryptionKey).toBeDefined(); + expect(controller.state.encryptionSalt).toBeDefined(); + }); + }); }); describe('createNewVaultAndKeychain', () => { - [false, true].map((cacheEncryptionKey) => - describe(`when cacheEncryptionKey is ${cacheEncryptionKey}`, () => { - describe('when there is no existing vault', () => { - it('should create new vault, mnemonic and keychain', async () => { - await withController( - { cacheEncryptionKey, skipVaultCreation: true }, - async ({ controller }) => { - await controller.createNewVaultAndKeychain(password); - - const currentSeedPhrase = - await controller.exportSeedPhrase(password); - - expect(currentSeedPhrase.length).toBeGreaterThan(0); - expect( - isValidHexAddress( - controller.state.keyrings[0].accounts[0] as Hex, - ), - ).toBe(true); - expect(controller.state.vault).toBeDefined(); - }, - ); - }); + describe('when there is no existing vault', () => { + it('should create new vault, mnemonic and keychain', async () => { + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await controller.createNewVaultAndKeychain(password); - cacheEncryptionKey && - it('should set encryptionKey and encryptionSalt in state', async () => { - await withController( - { cacheEncryptionKey, skipVaultCreation: true }, - async ({ controller }) => { - await controller.createNewVaultAndKeychain(password); + const currentSeedPhrase = + await controller.exportSeedPhrase(password); - expect(controller.state.encryptionKey).toBeDefined(); - expect(controller.state.encryptionSalt).toBeDefined(); - }, - ); - }); + expect(currentSeedPhrase.length).toBeGreaterThan(0); + expect( + isValidHexAddress( + controller.state.keyrings[0].accounts[0] as Hex, + ), + ).toBe(true); + expect(controller.state.vault).toBeDefined(); + }, + ); + }); - it('should set default state', async () => { - await withController( - { cacheEncryptionKey, skipVaultCreation: true }, - async ({ controller }) => { - await controller.createNewVaultAndKeychain(password); - - expect(controller.state.keyrings).not.toStrictEqual([]); - const keyring = controller.state.keyrings[0]; - expect(keyring.accounts).not.toStrictEqual([]); - expect(keyring.type).toBe('HD Key Tree'); - expect(controller.state.vault).toBeDefined(); - }, - ); - }); + it('should set encryptionKey and encryptionSalt in state', async () => { + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await controller.createNewVaultAndKeychain(password); - it('should throw error if password is of wrong type', async () => { - await withController( - { cacheEncryptionKey, skipVaultCreation: true }, - async ({ controller }) => { - await expect( - controller.createNewVaultAndKeychain( - // @ts-expect-error invalid password - 123, - ), - ).rejects.toThrow(KeyringControllerError.WrongPasswordType); - }, - ); - }); + expect(controller.state.encryptionKey).toBeDefined(); + expect(controller.state.encryptionSalt).toBeDefined(); + }, + ); + }); - it('should throw error if the first account is not found on the keyring', async () => { - jest.spyOn(HdKeyring.prototype, 'getAccounts').mockReturnValue([]); - await withController( - { cacheEncryptionKey, skipVaultCreation: true }, - async ({ controller }) => { - await expect( - controller.createNewVaultAndKeychain(password), - ).rejects.toThrow(KeyringControllerError.NoFirstAccount); - }, - ); - }); + it('should set default state', async () => { + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await controller.createNewVaultAndKeychain(password); - !cacheEncryptionKey && - it('should not set encryptionKey and encryptionSalt in state', async () => { - await withController( - { skipVaultCreation: true }, - async ({ controller }) => { - await controller.createNewVaultAndKeychain(password); + expect(controller.state.keyrings).not.toStrictEqual([]); + const keyring = controller.state.keyrings[0]; + expect(keyring.accounts).not.toStrictEqual([]); + expect(keyring.type).toBe('HD Key Tree'); + expect(controller.state.vault).toBeDefined(); + }, + ); + }); - expect(controller.state).not.toHaveProperty('encryptionKey'); - expect(controller.state).not.toHaveProperty('encryptionSalt'); - }, - ); - }); - }); + it('should throw error if password is of wrong type', async () => { + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect( + controller.createNewVaultAndKeychain( + // @ts-expect-error invalid password + 123, + ), + ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + }, + ); + }); - describe('when there is an existing vault', () => { - it('should not create a new vault or keychain', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller, initialState }) => { - const initialSeedWord = - await controller.exportSeedPhrase(password); - expect(initialSeedWord).toBeDefined(); - const initialVault = controller.state.vault; - - await controller.createNewVaultAndKeychain(password); - - const currentSeedWord = - await controller.exportSeedPhrase(password); - expect(initialState).toStrictEqual(controller.state); - expect(initialSeedWord).toBe(currentSeedWord); - expect(initialVault).toStrictEqual(controller.state.vault); - }, - ); - }); + it('should throw error if the first account is not found on the keyring', async () => { + jest.spyOn(HdKeyring.prototype, 'getAccounts').mockReturnValue([]); + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect( + controller.createNewVaultAndKeychain(password), + ).rejects.toThrow(KeyringControllerError.NoFirstAccount); + }, + ); + }); + }); - cacheEncryptionKey && - it('should set encryptionKey and encryptionSalt in state', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller }) => { - await controller.setLocked(); - expect(controller.state.encryptionKey).toBeUndefined(); - expect(controller.state.encryptionSalt).toBeUndefined(); + describe('when there is an existing vault', () => { + it('should not create a new vault or keychain', async () => { + await withController(async ({ controller, initialState }) => { + const initialSeedWord = await controller.exportSeedPhrase(password); + expect(initialSeedWord).toBeDefined(); + const initialVault = controller.state.vault; - await controller.createNewVaultAndKeychain(password); + await controller.createNewVaultAndKeychain(password); - expect(controller.state.encryptionKey).toBeDefined(); - expect(controller.state.encryptionSalt).toBeDefined(); - }, - ); - }); + const currentSeedWord = await controller.exportSeedPhrase(password); + expect(initialState).toStrictEqual(controller.state); + expect(initialSeedWord).toBe(currentSeedWord); + expect(initialVault).toStrictEqual(controller.state.vault); + }); + }); - !cacheEncryptionKey && - it('should not set encryptionKey and encryptionSalt in state', async () => { - await withController( - { skipVaultCreation: false, cacheEncryptionKey }, - async ({ controller }) => { - await controller.createNewVaultAndKeychain(password); + it('should set encryptionKey and encryptionSalt in state', async () => { + await withController(async ({ controller }) => { + await controller.setLocked(); + expect(controller.state.encryptionKey).toBeUndefined(); + expect(controller.state.encryptionSalt).toBeUndefined(); - expect(controller.state).not.toHaveProperty('encryptionKey'); - expect(controller.state).not.toHaveProperty('encryptionSalt'); - }, - ); - }); + await controller.createNewVaultAndKeychain(password); + + expect(controller.state.encryptionKey).toBeDefined(); + expect(controller.state.encryptionSalt).toBeDefined(); }); - }), - ); + }); + }); }); describe('setLocked', () => { @@ -1174,10 +1102,10 @@ describe('KeyringController', () => { it('should throw an error if there is no keyring', async () => { await withController( - { skipVaultCreation: true, state: { vault: 'my vault' } }, + { skipVaultCreation: true, state: { vault: mockVault } }, async ({ controller, encryptor }) => { jest - .spyOn(encryptor, 'decrypt') + .spyOn(encryptor, 'decryptWithKey') .mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]); await controller.submitPassword('123'); @@ -2579,579 +2507,453 @@ describe('KeyringController', () => { }); describe('changePassword', () => { - [false, true].map((cacheEncryptionKey) => - describe(`when cacheEncryptionKey is ${cacheEncryptionKey}`, () => { - it('should encrypt the vault with the new password', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller, encryptor }) => { - const newPassword = 'new-password'; - const spiedEncryptionFn = jest.spyOn( - encryptor, - cacheEncryptionKey ? 'encryptWithDetail' : 'encrypt', - ); - - await controller.changePassword(newPassword); - - // we pick the first argument of the first call - expect(spiedEncryptionFn.mock.calls[0][0]).toBe(newPassword); - }, - ); - }); + it('should encrypt the vault with the new password', async () => { + await withController(async ({ controller, encryptor }) => { + const newPassword = 'new-password'; + const keyFromPasswordSpy = jest.spyOn(encryptor, 'keyFromPassword'); + const spiedEncryptionFn = jest.spyOn(encryptor, 'encryptWithKey'); + + await controller.changePassword(newPassword); + + // we pick the first argument of the first call + expect(keyFromPasswordSpy).toHaveBeenCalledWith( + newPassword, + controller.state.encryptionSalt, + true, + undefined, + ); + }); + }); - it('should throw error if `isUnlocked` is false', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller }) => { - await controller.setLocked(); + it('should throw error if `isUnlocked` is false', async () => { + await withController(async ({ controller }) => { + await controller.setLocked(); - await expect(async () => - controller.changePassword(''), - ).rejects.toThrow(KeyringControllerError.ControllerLocked); - }, - ); - }); + await expect(async () => controller.changePassword('')).rejects.toThrow( + KeyringControllerError.ControllerLocked, + ); + }); + }); - it('should throw error if the new password is an empty string', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller }) => { - await expect(controller.changePassword('')).rejects.toThrow( - KeyringControllerError.InvalidEmptyPassword, - ); - }, - ); - }); + it('should throw error if the new password is an empty string', async () => { + await withController(async ({ controller }) => { + await expect(controller.changePassword('')).rejects.toThrow( + KeyringControllerError.InvalidEmptyPassword, + ); + }); + }); - it('should throw error if the new password is undefined', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller }) => { - await expect( - // @ts-expect-error we are testing wrong input - controller.changePassword(undefined), - ).rejects.toThrow(KeyringControllerError.WrongPasswordType); - }, - ); - }); + it('should throw error if the new password is undefined', async () => { + await withController(async ({ controller }) => { + await expect( + // @ts-expect-error we are testing wrong input + controller.changePassword(undefined), + ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + }); + }); - it('should throw error when the controller is locked', async () => { - await withController(async ({ controller }) => { - await controller.setLocked(); + it('should throw error when the controller is locked', async () => { + await withController(async ({ controller }) => { + await controller.setLocked(); - await expect(async () => - controller.changePassword('whatever'), - ).rejects.toThrow(KeyringControllerError.ControllerLocked); - }); - }); - }), - ); + await expect(async () => + controller.changePassword('whatever'), + ).rejects.toThrow(KeyringControllerError.ControllerLocked); + }); + }); }); describe('submitPassword', () => { - [false, true].map((cacheEncryptionKey) => - describe(`when cacheEncryptionKey is ${cacheEncryptionKey}`, () => { - it('should submit password and decrypt', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller, initialState }) => { - await controller.submitPassword(password); - expect(controller.state).toStrictEqual(initialState); - }, - ); - }); + it('should submit password and decrypt', async () => { + await withController(async ({ controller, initialState }) => { + await controller.submitPassword(password); + expect(controller.state).toStrictEqual(initialState); + }); + }); - it('should emit KeyringController:unlock event', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller, messenger }) => { - const listener = sinon.spy(); - messenger.subscribe('KeyringController:unlock', listener); - await controller.submitPassword(password); - expect(listener.called).toBe(true); - }, - ); - }); + it('should emit KeyringController:unlock event', async () => { + await withController(async ({ controller, messenger }) => { + const listener = sinon.spy(); + messenger.subscribe('KeyringController:unlock', listener); + await controller.submitPassword(password); + expect(listener.called).toBe(true); + }); + }); - it('should unlock also with unsupported keyrings', async () => { - await withController( + it('should unlock also with unsupported keyrings', async () => { + await withController( + { + skipVaultCreation: true, + state: { vault: mockVault }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { - cacheEncryptionKey, - skipVaultCreation: true, - state: { vault: 'my vault' }, + type: 'UnsupportedKeyring', + data: '0x1234', }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: 'UnsupportedKeyring', - data: '0x1234', - }, - ]); + ]); - await controller.submitPassword(password); + await controller.submitPassword(password); - expect(controller.state.isUnlocked).toBe(true); - }, - ); - }); + expect(controller.state.isUnlocked).toBe(true); + }, + ); + }); - it('should throw error if vault unlocked has an unexpected shape', async () => { - await withController( + it('should throw error if vault unlocked has an unexpected shape', async () => { + await withController( + { + skipVaultCreation: true, + state: { vault: mockVault }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { - cacheEncryptionKey, - skipVaultCreation: true, - state: { vault: 'my vault' }, + foo: 'bar', }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - foo: 'bar', - }, - ]); + ]); - await expect(controller.submitPassword(password)).rejects.toThrow( - KeyringControllerError.VaultDataError, - ); - }, + await expect(controller.submitPassword(password)).rejects.toThrow( + KeyringControllerError.VaultDataError, ); - }); + }, + ); + }); - it('should throw error if vault is missing', async () => { - await withController( - { skipVaultCreation: true }, - async ({ controller }) => { - await expect(controller.submitPassword(password)).rejects.toThrow( - KeyringControllerError.VaultError, - ); - }, + it('should throw error if vault is missing', async () => { + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect(controller.submitPassword(password)).rejects.toThrow( + KeyringControllerError.VaultError, ); - }); + }, + ); + }); - it('should unlock succesfully when the controller is instantiated with an existing `keyringsMetadata`', async () => { - // @ts-expect-error HdKeyring is not yet compatible with Keyring type. - stubKeyringClassWithAccount(HdKeyring, '0x123'); - await withController( + it('should unlock succesfully when the controller is instantiated with an existing `keyringsMetadata`', async () => { + // @ts-expect-error HdKeyring is not yet compatible with Keyring type. + stubKeyringClassWithAccount(HdKeyring, '0x123'); + await withController( + { + state: { vault: mockVault }, + skipVaultCreation: true, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { - cacheEncryptionKey, - state: { vault: 'my vault' }, - skipVaultCreation: true, - }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - metadata: { - id: '123', - name: '', - }, - }, - ]); + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: '123', + name: '', + }, + }, + ]); - await controller.submitPassword(password); + await controller.submitPassword(password); - expect(controller.state.keyrings).toStrictEqual([ - { - type: KeyringTypes.hd, - accounts: ['0x123'], - metadata: { - id: '123', - name: '', - }, - }, - ]); + expect(controller.state.keyrings).toStrictEqual([ + { + type: KeyringTypes.hd, + accounts: ['0x123'], + metadata: { + id: '123', + name: '', + }, }, - ); - }); + ]); + }, + ); + }); - cacheEncryptionKey && - it('should generate new metadata when there is no metadata in the vault and cacheEncryptionKey is enabled', async () => { - const hdKeyringSerializeSpy = jest.spyOn( - HdKeyring.prototype, - 'serialize', - ); - await withController( - { - cacheEncryptionKey: true, - state: { - vault: 'my vault', - }, - skipVaultCreation: true, + it('should generate new metadata when there is no metadata in the vault', async () => { + const hdKeyringSerializeSpy = jest.spyOn( + HdKeyring.prototype, + 'serialize', + ); + await withController( + { + state: { + vault: mockVault, + }, + skipVaultCreation: true, + }, + async ({ controller, encryptor }) => { + const encryptWithKeySpy = jest.spyOn(encryptor, 'encryptWithKey'); + jest.spyOn(encryptor, 'importKey').mockResolvedValue('imported key'); + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], }, - async ({ controller, encryptor }) => { - const encryptWithKeySpy = jest.spyOn( - encryptor, - 'encryptWithKey', - ); - jest - .spyOn(encryptor, 'importKey') - // @ts-expect-error we are assigning a mock value - .mockResolvedValue('imported key'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]); - hdKeyringSerializeSpy.mockResolvedValue({ - // @ts-expect-error we are assigning a mock value - accounts: ['0x123'], - }); + }, + ]); + hdKeyringSerializeSpy.mockResolvedValue({ + // @ts-expect-error we are assigning a mock value + accounts: ['0x123'], + }); - await controller.submitPassword(password); + await controller.submitPassword(password); - expect(controller.state.keyrings).toStrictEqual([ - { - type: KeyringTypes.hd, - accounts: expect.any(Array), - metadata: { - id: expect.any(String), - name: '', - }, - }, - ]); - expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - metadata: { - id: expect.any(String), - name: '', - }, - }, - ]); + expect(controller.state.keyrings).toStrictEqual([ + { + type: KeyringTypes.hd, + accounts: expect.any(Array), + metadata: { + id: expect.any(String), + name: '', }, - ); - }); - - !cacheEncryptionKey && - it('should generate new metadata when there is no metadata in the vault and cacheEncryptionKey is disabled', async () => { - const hdKeyringSerializeSpy = jest.spyOn( - HdKeyring.prototype, - 'serialize', - ); - await withController( - { - cacheEncryptionKey: false, - state: { - vault: 'my vault', - }, - skipVaultCreation: true, + }, + ]); + expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], }, - async ({ controller, encryptor }) => { - const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]); - hdKeyringSerializeSpy.mockResolvedValue({ - // @ts-expect-error we are assigning a mock value - accounts: ['0x123'], - }); - - await controller.submitPassword(password); - - expect(controller.state.keyrings).toStrictEqual([ - { - type: KeyringTypes.hd, - accounts: expect.any(Array), - metadata: { - id: expect.any(String), - name: '', - }, - }, - ]); - expect(encryptSpy).toHaveBeenCalledWith(password, [ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - metadata: { - id: expect.any(String), - name: '', - }, - }, - ]); + metadata: { + id: expect.any(String), + name: '', }, - ); - }); + }, + ]); + }, + ); + }); - it('should unlock the wallet if the state has a duplicate account and the encryption parameters are outdated', async () => { - stubKeyringClassWithAccount(MockKeyring, '0x123'); - // @ts-expect-error HdKeyring is not yet compatible with Keyring type. - stubKeyringClassWithAccount(HdKeyring, '0x123'); - await withController( + it('should unlock the wallet if the state has a duplicate account and the encryption parameters are outdated', async () => { + stubKeyringClassWithAccount(MockKeyring, '0x123'); + // @ts-expect-error HdKeyring is not yet compatible with Keyring type. + stubKeyringClassWithAccount(HdKeyring, '0x123'); + await withController( + { + skipVaultCreation: true, + state: { vault: mockVault }, + keyringBuilders: [keyringBuilderFactory(MockKeyring)], + }, + async ({ controller, encryptor, messenger }) => { + const unlockListener = jest.fn(); + messenger.subscribe('KeyringController:unlock', unlockListener); + jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { - skipVaultCreation: true, - cacheEncryptionKey, - state: { vault: 'my vault' }, - keyringBuilders: [keyringBuilderFactory(MockKeyring)], + type: KeyringTypes.hd, + data: {}, }, - async ({ controller, encryptor, messenger }) => { - const unlockListener = jest.fn(); - messenger.subscribe('KeyringController:unlock', unlockListener); - jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: {}, - }, - { - type: MockKeyring.type, - data: {}, - }, - ]); + { + type: MockKeyring.type, + data: {}, + }, + ]); - await controller.submitPassword(password); + await controller.submitPassword(password); - expect(controller.state.isUnlocked).toBe(true); - expect(unlockListener).toHaveBeenCalledTimes(1); - }, - ); - }); + expect(controller.state.isUnlocked).toBe(true); + expect(unlockListener).toHaveBeenCalledTimes(1); + }, + ); + }); - it('should unlock the wallet also if encryption parameters are outdated and the vault upgrade fails', async () => { - await withController( + it('should unlock the wallet also if encryption parameters are outdated and the vault upgrade fails', async () => { + await withController( + { + skipVaultCreation: true, + state: { vault: mockVault }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); + jest + .spyOn(encryptor, 'encryptWithKey') + .mockRejectedValue(new Error()); + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { - skipVaultCreation: true, - cacheEncryptionKey, - state: { vault: 'my vault' }, + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); - jest.spyOn(encryptor, 'encrypt').mockRejectedValue(new Error()); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]); + ]); - await controller.submitPassword(password); + await controller.submitPassword(password); - expect(controller.state.isUnlocked).toBe(true); - }, - ); - }); + expect(controller.state.isUnlocked).toBe(true); + }, + ); + }); - it('should unlock the wallet discarding existing duplicate accounts', async () => { - stubKeyringClassWithAccount(MockKeyring, '0x123'); - // @ts-expect-error HdKeyring is not yet compatible with Keyring type. - stubKeyringClassWithAccount(HdKeyring, '0x123'); - await withController( + it('should unlock the wallet discarding existing duplicate accounts', async () => { + stubKeyringClassWithAccount(MockKeyring, '0x123'); + // @ts-expect-error HdKeyring is not yet compatible with Keyring type. + stubKeyringClassWithAccount(HdKeyring, '0x123'); + await withController( + { + skipVaultCreation: true, + state: { vault: mockVault }, + keyringBuilders: [keyringBuilderFactory(MockKeyring)], + }, + async ({ controller, encryptor, messenger }) => { + const unlockListener = jest.fn(); + messenger.subscribe('KeyringController:unlock', unlockListener); + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { - skipVaultCreation: true, - cacheEncryptionKey, - state: { vault: 'my vault' }, - keyringBuilders: [keyringBuilderFactory(MockKeyring)], + type: KeyringTypes.hd, + data: {}, }, - async ({ controller, encryptor, messenger }) => { - const unlockListener = jest.fn(); - messenger.subscribe('KeyringController:unlock', unlockListener); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: {}, - }, - { - type: MockKeyring.type, - data: {}, - }, - ]); - - await controller.submitPassword(password); - - expect(controller.state.keyrings).toHaveLength(1); // Second keyring will be skipped as "unsupported". - expect(unlockListener).toHaveBeenCalledTimes(1); + { + type: MockKeyring.type, + data: {}, }, - ); - }); - - cacheEncryptionKey && - it('should upgrade the vault encryption if the key encryptor has different parameters', async () => { - await withController( - { - skipVaultCreation: true, - cacheEncryptionKey, - state: { vault: 'my vault' }, - }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); - const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]); + ]); - await controller.submitPassword(password); + await controller.submitPassword(password); - expect(encryptSpy).toHaveBeenCalledTimes(1); - }, - ); - }); + expect(controller.state.keyrings).toHaveLength(1); // Second keyring will be skipped as "unsupported". + expect(unlockListener).toHaveBeenCalledTimes(1); + }, + ); + }); - cacheEncryptionKey && - it('should not upgrade the vault encryption if the key encryptor has the same parameters', async () => { - await withController( - { - skipVaultCreation: true, - cacheEncryptionKey, - state: { vault: 'my vault' }, + it('should upgrade the vault encryption if the key encryptor has different parameters', async () => { + await withController( + { + skipVaultCreation: true, + state: { vault: mockVault }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); + jest.spyOn(encryptor, 'generateSalt').mockReturnValue('new salt'); + const keyFromPasswordSpy = jest.spyOn(encryptor, 'keyFromPassword'); + const encryptSpy = jest.spyOn(encryptor, 'encryptWithKey'); + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); - const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]); + }, + ]); - await controller.submitPassword(password); + await controller.submitPassword(password); - expect(encryptSpy).not.toHaveBeenCalled(); - }, - ); - }); + expect(keyFromPasswordSpy).toHaveBeenCalledWith( + password, + 'new salt', + true, + undefined, + ); + expect(encryptSpy).toHaveBeenCalledTimes(1); + }, + ); + }); - !cacheEncryptionKey && - it('should upgrade the vault encryption if the generic encryptor has different parameters', async () => { - await withController( - { - skipVaultCreation: true, - cacheEncryptionKey, - state: { vault: 'my vault' }, + it('should not upgrade the vault encryption if the key encryptor has the same parameters', async () => { + await withController( + { + skipVaultCreation: true, + state: { vault: mockVault }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); + const encryptSpy = jest.spyOn(encryptor, 'encrypt'); + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); - const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]); + }, + ]); - await controller.submitPassword(password); + await controller.submitPassword(password); - expect(encryptSpy).toHaveBeenCalledTimes(1); - }, - ); - }); + expect(encryptSpy).not.toHaveBeenCalled(); + }, + ); + }); - it('should not upgrade the vault encryption if the encryptor has the same parameters and the keyring has metadata', async () => { - await withController( + it('should not upgrade the vault encryption if the encryptor has the same parameters and the keyring has metadata', async () => { + await withController( + { + skipVaultCreation: true, + state: { vault: mockVault }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); + const encryptSpy = jest.spyOn(encryptor, 'encrypt'); + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { - skipVaultCreation: true, - cacheEncryptionKey, - state: { vault: 'my vault' }, + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: '123', + name: '', + }, }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); - const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - metadata: { - id: '123', - name: '', - }, - }, - ]); + ]); - await controller.submitPassword(password); + await controller.submitPassword(password); - expect(encryptSpy).not.toHaveBeenCalled(); - }, - ); - }); + expect(encryptSpy).not.toHaveBeenCalled(); + }, + ); + }); - !cacheEncryptionKey && - it('should throw error if password is of wrong type', async () => { - await withController( - { cacheEncryptionKey }, - async ({ controller }) => { - await expect( - // @ts-expect-error we are testing the case of a user using - // the wrong password type - controller.submitPassword(12341234), - ).rejects.toThrow(KeyringControllerError.WrongPasswordType); - }, - ); - }); + it('should throw error if password is of wrong type', async () => { + await withController(async ({ controller }) => { + await expect( + // @ts-expect-error we are testing the case of a user using + // the wrong password type + controller.submitPassword(12341234), + ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + }); + }); - cacheEncryptionKey && - it('should set encryptionKey and encryptionSalt in state', async () => { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - withController({ cacheEncryptionKey }, async ({ controller }) => { - await controller.submitPassword(password); - expect(controller.state.encryptionKey).toBeDefined(); - expect(controller.state.encryptionSalt).toBeDefined(); - }); - }); + it('should set encryptionKey and encryptionSalt in state', async () => { + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + withController(async ({ controller }) => { + await controller.submitPassword(password); + expect(controller.state.encryptionKey).toBeDefined(); + expect(controller.state.encryptionSalt).toBeDefined(); + }); + }); - it('should throw error when using the wrong password', async () => { - await withController( - { - skipVaultCreation: true, - cacheEncryptionKey, - state: { vault: 'my vault' }, - }, - async ({ controller }) => { - await expect( - controller.submitPassword('wrong password'), - ).rejects.toThrow('Incorrect password.'); - }, - ); - }); - }), - ); + it('should throw error when using the wrong password', async () => { + await withController( + { + skipVaultCreation: true, + state: { vault: mockVault }, + }, + async ({ controller, encryptor }) => { + jest + .spyOn(encryptor, 'decryptWithKey') + .mockRejectedValue(new Error('Incorrect password.')); + await expect( + controller.submitPassword('wrong password'), + ).rejects.toThrow('Incorrect password.'); + }, + ); + }); }); describe('submitEncryptionKey', () => { it('should submit encryption key and decrypt', async () => { - await withController( - { cacheEncryptionKey: true }, - async ({ controller, initialState }) => { - await controller.submitEncryptionKey( - MOCK_ENCRYPTION_KEY, - initialState.encryptionSalt as string, - ); - expect(controller.state).toStrictEqual(initialState); - }, - ); + await withController(async ({ controller, initialState }) => { + await controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY); + expect(controller.state).toStrictEqual(initialState); + }); }); it('should unlock also with unsupported keyrings', async () => { await withController( { - cacheEncryptionKey: true, skipVaultCreation: true, state: { vault: JSON.stringify({ data: '0x123', salt: 'my salt' }), @@ -3160,18 +2962,15 @@ describe('KeyringController', () => { encryptionSalt: 'my salt', }, }, - async ({ controller, initialState, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: 'UnsupportedKeyring', data: '0x1234', }, ]); - await controller.submitEncryptionKey( - MOCK_ENCRYPTION_KEY, - initialState.encryptionSalt as string, - ); + await controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY); expect(controller.state.isUnlocked).toBe(true); }, @@ -3185,7 +2984,6 @@ describe('KeyringController', () => { }); await withController( { - cacheEncryptionKey: true, skipVaultCreation: true, state: { vault: JSON.stringify({ data: '0x123', salt: 'my salt' }), @@ -3194,23 +2992,17 @@ describe('KeyringController', () => { encryptionSalt: 'my salt', }, }, - async ({ controller, initialState, encryptor }) => { + async ({ controller, encryptor }) => { const encryptWithKeySpy = jest.spyOn(encryptor, 'encryptWithKey'); - jest - .spyOn(encryptor, 'importKey') - // @ts-expect-error we are assigning a mock value - .mockResolvedValue('imported key'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'importKey').mockResolvedValue('imported key'); + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: '0x123', }, ]); - await controller.submitEncryptionKey( - MOCK_ENCRYPTION_KEY, - initialState.encryptionSalt as string, - ); + await controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY); expect(controller.state.isUnlocked).toBe(true); expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [ @@ -3230,50 +3022,29 @@ describe('KeyringController', () => { }); it('should throw error if vault unlocked has an unexpected shape', async () => { - await withController( - { cacheEncryptionKey: true }, - async ({ controller, initialState, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ - { - foo: 'bar', - }, - ]); - - await expect( - controller.submitEncryptionKey( - MOCK_ENCRYPTION_KEY, - initialState.encryptionSalt as string, - ), - ).rejects.toThrow(KeyringControllerError.VaultDataError); - }, - ); - }); + await withController(async ({ controller, initialState, encryptor }) => { + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ + { + foo: 'bar', + }, + ]); - it('should throw error if encryptionSalt is different from the one in the vault', async () => { - await withController( - { cacheEncryptionKey: true }, - async ({ controller }) => { - await expect( - controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY, '0x1234'), - ).rejects.toThrow(KeyringControllerError.ExpiredCredentials); - }, - ); + await expect( + controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY), + ).rejects.toThrow(KeyringControllerError.VaultDataError); + }); }); it('should throw error if encryptionKey is of an unexpected type', async () => { - await withController( - { cacheEncryptionKey: true }, - async ({ controller, initialState }) => { - await expect( - controller.submitEncryptionKey( - // @ts-expect-error we are testing the case of a user using - // the wrong encryptionKey type - 12341234, - initialState.encryptionSalt as string, - ), - ).rejects.toThrow(KeyringControllerError.WrongPasswordType); - }, - ); + await withController(async ({ controller, initialState }) => { + await expect( + controller.submitEncryptionKey( + // @ts-expect-error we are testing the case of a user using + // the wrong encryptionKey type + 12341234, + ), + ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + }); }); }); @@ -3339,10 +3110,10 @@ describe('KeyringController', () => { it('should throw an error if there is no primary keyring', async () => { await withController( - { skipVaultCreation: true, state: { vault: 'my vault' } }, + { skipVaultCreation: true, state: { vault: mockVault } }, async ({ controller, encryptor }) => { jest - .spyOn(encryptor, 'decrypt') + .spyOn(encryptor, 'decryptWithKey') .mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]); await controller.submitPassword('123'); @@ -3744,7 +3515,6 @@ describe('KeyringController', () => { { // @ts-expect-error QRKeyring is not yet compatible with Keyring type. keyringBuilders: [keyringBuilderFactory(QRKeyring)], - cacheEncryptionKey: true, }, (args) => args, ); @@ -4361,8 +4131,6 @@ describe('KeyringController', () => { 'KeyringController:qrKeyringStateChange', listener, ); - const salt = signProcessKeyringController.state - .encryptionSalt as string; // We ensure there is a QRKeyring before locking await signProcessKeyringController.getOrAddQRKeyring(); // Locking the keyring will dereference the QRKeyring @@ -4370,7 +4138,6 @@ describe('KeyringController', () => { // ..and unlocking it should add a new instance of QRKeyring await signProcessKeyringController.submitEncryptionKey( MOCK_ENCRYPTION_KEY, - salt, ); // We call `getQRKeyring` instead of `getOrAddQRKeyring` so that // we are able to test if the subscription to the internal QR keyring diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 467bbc64528..eb24bcdf468 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -453,6 +453,9 @@ export type ExportableKeyEncryptor = password: string, salt: string, exportable?: boolean, + // setting this to unknown as currently each client has different + // key derivation options + keyDerivationOptions?: unknown, ) => Promise; /** * Generates a random salt for key derivation. @@ -1497,6 +1500,12 @@ export class KeyringController extends BaseController< // can attempt to upgrade the vault. await this.#withRollback(async () => { if (newMetadata || this.#isNewEncryptionAvailable()) { + await this.#deriveEncryptionKey(password, { + // If the vault is being upgraded, we want to ignore the metadata + // that is already in the vault, so we can effectively + // re-encrypt the vault with the new encryption config. + ignoreVautKeyMetadata: true, + }); await this.#updateVault(); } }); @@ -2116,7 +2125,12 @@ export class KeyringController extends BaseController< * * @param password - The password to use for decryption or derivation. */ - async #deriveEncryptionKey(password: string): Promise { + async #deriveEncryptionKey( + password: string, + options: { ignoreVautKeyMetadata: boolean } = { + ignoreVautKeyMetadata: false, + }, + ): Promise { this.#assertControllerMutexIsLocked(); const { vault } = this.state; @@ -2124,18 +2138,22 @@ export class KeyringController extends BaseController< throw new TypeError(KeyringControllerError.WrongPasswordType); } - let salt: string; - if (vault) { - salt = JSON.parse(vault).salt; + let salt: string, keyMetadata: unknown; + if (vault && !options.ignoreVautKeyMetadata) { + const parsedVault = JSON.parse(vault); + salt = parsedVault.salt; + keyMetadata = parsedVault.keyMetadata; } else { salt = this.#encryptor.generateSalt(); } + const exportedEncryptionKey = await this.#encryptor.exportKey( + await this.#encryptor.keyFromPassword(password, salt, true, keyMetadata), + ); + this.#encryptionKey = { salt, - exported: await this.#encryptor.exportKey( - await this.#encryptor.keyFromPassword(password, salt, true), - ), + exported: exportedEncryptionKey, }; } @@ -2154,6 +2172,13 @@ export class KeyringController extends BaseController< ): Promise { this.#assertControllerMutexIsLocked(); + if ( + typeof encryptionKey !== 'string' || + typeof encryptionSalt !== 'string' + ) { + throw new TypeError(KeyringControllerError.WrongEncryptionKeyType); + } + this.#encryptionKey = { salt: encryptionSalt, exported: encryptionKey, diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index d914a3d6f74..e6237d8caae 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -3,6 +3,7 @@ export enum KeyringControllerError { KeyringNotFound = 'KeyringController - Keyring not found.', UnsafeDirectKeyringAccess = 'KeyringController - Returning keyring instances is unsafe', WrongPasswordType = 'KeyringController - Password must be of type string.', + WrongEncryptionKeyType = 'KeyringController - Password must be of type string.', InvalidEmptyPassword = 'KeyringController - Password cannot be empty.', NoFirstAccount = 'KeyringController - First Account not found.', DuplicatedAccount = 'KeyringController - The account you are trying to import is a duplicate', diff --git a/packages/keyring-controller/tests/mocks/mockEncryptor.ts b/packages/keyring-controller/tests/mocks/mockEncryptor.ts index 034d9e32d1d..9a7798c4e7b 100644 --- a/packages/keyring-controller/tests/mocks/mockEncryptor.ts +++ b/packages/keyring-controller/tests/mocks/mockEncryptor.ts @@ -14,7 +14,7 @@ export const MOCK_ENCRYPTION_SALT = export const MOCK_HARDCODED_KEY = 'key'; export const MOCK_HEX = '0xabcdef0123456789'; // eslint-disable-next-line no-restricted-globals -const MOCK_KEY = Buffer.alloc(32); +export const MOCK_KEY = Buffer.alloc(32); const INVALID_PASSWORD_ERROR = 'Incorrect password.'; export default class MockEncryptor implements ExportableKeyEncryptor { @@ -65,20 +65,19 @@ export default class MockEncryptor implements ExportableKeyEncryptor { } async decryptWithKey(key: unknown, text: string) { - return this.decrypt(key as string, text); + return JSON.parse(this.cacheVal || '') ?? {}; } - async keyFromPassword(_password: string) { - return MOCK_KEY; + async keyFromPassword(_password: string, _salt?: string) { + return JSON.parse(MOCK_ENCRYPTION_KEY); } async importKey(key: string) { - if (key === '{}') { - throw new TypeError( - `Failed to execute 'importKey' on 'SubtleCrypto': The provided value is not of type '(ArrayBuffer or ArrayBufferView or JsonWebKey)'.`, - ); - } - return null; + return JSON.parse(key); + } + + async exportKey(key: unknown) { + return JSON.stringify(key); } async updateVault(_vault: string, _password: string) { From 383bed2fe111fd0c387ef4403292bcef70b0ffe2 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 12 Jun 2025 12:23:30 +0200 Subject: [PATCH 04/44] fix lint --- packages/keyring-controller/src/KeyringController.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index eb24bcdf468..9f4eb954095 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2124,6 +2124,8 @@ export class KeyringController extends BaseController< * is generated and used to derive the key. * * @param password - The password to use for decryption or derivation. + * @param options - Options for the key derivation. + * @param options.ignoreVautKeyMetadata - Whether to ignore the vault key metadata */ async #deriveEncryptionKey( password: string, From 22ba2d5f31179fa964d571192912ceecc6739268 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 12 Jun 2025 12:23:37 +0200 Subject: [PATCH 05/44] update changelog --- packages/keyring-controller/CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 38f4b66ca6b..218867a10a6 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING:** The `cacheEncryptionKey` parameter has been removed from the `KeyringController` constructor options ([#5963](https://github.com/MetaMask/core/pull/5963)) + - This parameter was previously used to enable encryption key in-memory caching, but it is no longer needed as the controller now always uses the latest encryption key. +- **BREAKING:** The `submitEncryptionKey` method does not accept an `encryptionSalt` argument anymore ([#5963](https://github.com/MetaMask/core/pull/5963)) + - The encryption salt is now always taken from the vault. +- **BREAKING:** The `KeyringController` constructor now requries an encryptor supporting the `keyFromPassword`, `exportKey` and `generateSalt` methods ([#5963](https://github.com/MetaMask/core/pull/5963)) + ## [22.0.2] ### Fixed From 811d0790d31ea34627cbc2b1579f467c44e60ee8 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 12 Jun 2025 12:34:57 +0200 Subject: [PATCH 06/44] make `#useEncryptionKey` sync --- packages/keyring-controller/src/KeyringController.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 9f4eb954095..cab81c10bf8 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2168,10 +2168,7 @@ export class KeyringController extends BaseController< * @param encryptionKey - The encryption key to use. * @param encryptionSalt - The salt to use for the encryption key. */ - async #useEncryptionKey( - encryptionKey: string, - encryptionSalt: string, - ): Promise { + #useEncryptionKey(encryptionKey: string, encryptionSalt: string): void { this.#assertControllerMutexIsLocked(); if ( @@ -2355,7 +2352,7 @@ export class KeyringController extends BaseController< await this.#deriveEncryptionKey(credentials.password); } else { const { exportedEncryptionKey } = credentials; - await this.#useEncryptionKey( + this.#useEncryptionKey( exportedEncryptionKey, parsedEncryptedVault.salt, ); From 27657e3625f6702e8bef1322f648e13a0ba1ff9d Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 12 Jun 2025 12:47:54 +0200 Subject: [PATCH 07/44] remove unused vars --- eslint-warning-thresholds.json | 7 +++---- packages/keyring-controller/src/KeyringController.test.ts | 6 ++---- packages/keyring-controller/src/constants.ts | 2 +- packages/keyring-controller/tests/mocks/mockEncryptor.ts | 2 +- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index 847cca2288b..dfd30a0ca83 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -213,10 +213,10 @@ }, "packages/keyring-controller/src/KeyringController.test.ts": { "import-x/namespace": 14, - "jest/no-conditional-in-test": 8 + "jest/no-conditional-in-test": 7 }, "packages/keyring-controller/src/KeyringController.ts": { - "@typescript-eslint/no-unsafe-enum-comparison": 4, + "@typescript-eslint/no-unsafe-enum-comparison": 3, "@typescript-eslint/no-unused-vars": 1 }, "packages/keyring-controller/tests/mocks/mockKeyring.ts": { @@ -260,7 +260,6 @@ "import-x/order": 1 }, "packages/multichain/src/adapters/caip-permission-adapter-eth-accounts.ts": { - "@typescript-eslint/no-unsafe-enum-comparison": 1, "jsdoc/tag-lines": 5 }, "packages/multichain/src/adapters/caip-permission-adapter-permittedChains.test.ts": { @@ -325,7 +324,7 @@ "jsdoc/require-returns": 1 }, "packages/multichain/src/scope/supported.ts": { - "@typescript-eslint/no-unsafe-enum-comparison": 6 + "@typescript-eslint/no-unsafe-enum-comparison": 3 }, "packages/multichain/src/scope/validation.ts": { "jsdoc/tag-lines": 2 diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index e4df88b9dcd..b523b7ff172 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -2511,11 +2511,9 @@ describe('KeyringController', () => { await withController(async ({ controller, encryptor }) => { const newPassword = 'new-password'; const keyFromPasswordSpy = jest.spyOn(encryptor, 'keyFromPassword'); - const spiedEncryptionFn = jest.spyOn(encryptor, 'encryptWithKey'); await controller.changePassword(newPassword); - // we pick the first argument of the first call expect(keyFromPasswordSpy).toHaveBeenCalledWith( newPassword, controller.state.encryptionSalt, @@ -3022,7 +3020,7 @@ describe('KeyringController', () => { }); it('should throw error if vault unlocked has an unexpected shape', async () => { - await withController(async ({ controller, initialState, encryptor }) => { + await withController(async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { foo: 'bar', @@ -3036,7 +3034,7 @@ describe('KeyringController', () => { }); it('should throw error if encryptionKey is of an unexpected type', async () => { - await withController(async ({ controller, initialState }) => { + await withController(async ({ controller }) => { await expect( controller.submitEncryptionKey( // @ts-expect-error we are testing the case of a user using diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index e6237d8caae..eab9969fec9 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -3,7 +3,7 @@ export enum KeyringControllerError { KeyringNotFound = 'KeyringController - Keyring not found.', UnsafeDirectKeyringAccess = 'KeyringController - Returning keyring instances is unsafe', WrongPasswordType = 'KeyringController - Password must be of type string.', - WrongEncryptionKeyType = 'KeyringController - Password must be of type string.', + WrongEncryptionKeyType = 'KeyringController - Encryption key must be of type string.', InvalidEmptyPassword = 'KeyringController - Password cannot be empty.', NoFirstAccount = 'KeyringController - First Account not found.', DuplicatedAccount = 'KeyringController - The account you are trying to import is a duplicate', diff --git a/packages/keyring-controller/tests/mocks/mockEncryptor.ts b/packages/keyring-controller/tests/mocks/mockEncryptor.ts index 9a7798c4e7b..3edcd539f58 100644 --- a/packages/keyring-controller/tests/mocks/mockEncryptor.ts +++ b/packages/keyring-controller/tests/mocks/mockEncryptor.ts @@ -64,7 +64,7 @@ export default class MockEncryptor implements ExportableKeyEncryptor { }; } - async decryptWithKey(key: unknown, text: string) { + async decryptWithKey(_key: unknown, _text: string) { return JSON.parse(this.cacheVal || '') ?? {}; } From 307119cdc74af8a9cb413f70922c1bf88acfb383 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 12 Jun 2025 13:01:41 +0200 Subject: [PATCH 08/44] revert eslint threshold changes --- eslint-warning-thresholds.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index dfd30a0ca83..6e6ae4f1d14 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -260,6 +260,7 @@ "import-x/order": 1 }, "packages/multichain/src/adapters/caip-permission-adapter-eth-accounts.ts": { + "@typescript-eslint/no-unsafe-enum-comparison": 1, "jsdoc/tag-lines": 5 }, "packages/multichain/src/adapters/caip-permission-adapter-permittedChains.test.ts": { @@ -324,7 +325,7 @@ "jsdoc/require-returns": 1 }, "packages/multichain/src/scope/supported.ts": { - "@typescript-eslint/no-unsafe-enum-comparison": 3 + "@typescript-eslint/no-unsafe-enum-comparison": 6 }, "packages/multichain/src/scope/validation.ts": { "jsdoc/tag-lines": 2 From 62318ccef7b608bfc111f1b70220021bff1f9975 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 12 Jun 2025 13:28:02 +0200 Subject: [PATCH 09/44] fix seedless-onboarding-controller tests --- .../src/KeyringController.ts | 178 +++++++++--------- .../src/SeedlessOnboardingController.test.ts | 79 +++++--- .../src/SeedlessOnboardingController.ts | 15 +- .../src/types.ts | 11 +- .../tests/mocks/vaultEncryptor.ts | 9 +- 5 files changed, 170 insertions(+), 122 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index cab81c10bf8..a4fb7674deb 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -374,94 +374,96 @@ export type GenericEncryptor = { * An encryptor interface that supports encrypting and decrypting * serializable data with a password, and exporting and importing keys. */ -export type ExportableKeyEncryptor = - GenericEncryptor & { - /** - * Encrypts the given object with the given encryption key. - * - * @param key - The encryption key to encrypt with. - * @param object - The object to encrypt. - * @returns The encryption result. - */ - encryptWithKey: ( - key: EncryptionKey, - object: Json, - ) => Promise; - /** - * Encrypts the given object with the given password, and returns the - * encryption result and the exported key string. - * - * @param password - The password to encrypt with. - * @param object - The object to encrypt. - * @param salt - The optional salt to use for encryption. - * @returns The encrypted string and the exported key string. - */ - encryptWithDetail: ( - password: string, - object: Json, - salt?: string, - ) => Promise; - /** - * Decrypts the given encrypted string with the given encryption key. - * - * @param key - The encryption key to decrypt with. - * @param encryptedString - The encrypted string to decrypt. - * @returns The decrypted object. - */ - decryptWithKey: ( - key: EncryptionKey, - encryptedString: string, - ) => Promise; - /** - * Decrypts the given encrypted string with the given password, and returns - * the decrypted object and the salt and exported key string used for - * encryption. - * - * @param password - The password to decrypt with. - * @param encryptedString - The encrypted string to decrypt. - * @returns The decrypted object and the salt and exported key string used for - * encryption. - */ - decryptWithDetail: ( - password: string, - encryptedString: string, - ) => Promise; - /** - * Generates an encryption key from exported key string. - * - * @param key - The exported key string. - * @returns The encryption key. - */ - importKey: (key: string) => Promise; - /** - * Exports the encryption key as a string. - * - * @param key - The encryption key to export. - * @returns The exported key string. - */ - exportKey: (key: EncryptionKey) => Promise; - /** - * Derives an encryption key from a password. - * - * @param password - The password to derive the key from. - * @param salt - The salt to use for key derivation. - * @param exportable - Whether the key should be exportable or not. - * @param options - Optional key derivation options. - * @returns The derived encryption key. - */ - keyFromPassword: ( - password: string, - salt: string, - exportable?: boolean, - // setting this to unknown as currently each client has different - // key derivation options - keyDerivationOptions?: unknown, - ) => Promise; - /** - * Generates a random salt for key derivation. - */ - generateSalt: typeof encryptorUtils.generateSalt; - }; +export type ExportableKeyEncryptor< + EncryptionKey = unknown, + SupportedKeyDerivationParams = unknown, +> = GenericEncryptor & { + /** + * Encrypts the given object with the given encryption key. + * + * @param key - The encryption key to encrypt with. + * @param object - The object to encrypt. + * @returns The encryption result. + */ + encryptWithKey: ( + key: EncryptionKey, + object: Json, + ) => Promise; + /** + * Encrypts the given object with the given password, and returns the + * encryption result and the exported key string. + * + * @param password - The password to encrypt with. + * @param object - The object to encrypt. + * @param salt - The optional salt to use for encryption. + * @returns The encrypted string and the exported key string. + */ + encryptWithDetail: ( + password: string, + object: Json, + salt?: string, + ) => Promise; + /** + * Decrypts the given encrypted string with the given encryption key. + * + * @param key - The encryption key to decrypt with. + * @param encryptedString - The encrypted string to decrypt. + * @returns The decrypted object. + */ + decryptWithKey: ( + key: EncryptionKey, + encryptedString: string, + ) => Promise; + /** + * Decrypts the given encrypted string with the given password, and returns + * the decrypted object and the salt and exported key string used for + * encryption. + * + * @param password - The password to decrypt with. + * @param encryptedString - The encrypted string to decrypt. + * @returns The decrypted object and the salt and exported key string used for + * encryption. + */ + decryptWithDetail: ( + password: string, + encryptedString: string, + ) => Promise; + /** + * Generates an encryption key from exported key string. + * + * @param key - The exported key string. + * @returns The encryption key. + */ + importKey: (key: string) => Promise; + /** + * Exports the encryption key as a string. + * + * @param key - The encryption key to export. + * @returns The exported key string. + */ + exportKey: (key: EncryptionKey) => Promise; + /** + * Derives an encryption key from a password. + * + * @param password - The password to derive the key from. + * @param salt - The salt to use for key derivation. + * @param exportable - Whether the key should be exportable or not. + * @param options - Optional key derivation options. + * @returns The derived encryption key. + */ + keyFromPassword: ( + password: string, + salt: string, + exportable?: boolean, + // setting this to unknown as currently each client has different + // key derivation options + keyDerivationOptions?: SupportedKeyDerivationParams, + ) => Promise; + /** + * Generates a random salt for key derivation. + */ + generateSalt: typeof encryptorUtils.generateSalt; +}; export type KeyringSelector = | { diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 074a0589324..052044c7794 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -1,6 +1,9 @@ import { keccak256AndHexify } from '@metamask/auth-network-utils'; import type { Messenger } from '@metamask/base-controller'; -import type { EncryptionKey } from '@metamask/browser-passworder'; +import type { + EncryptionKey, + KeyDerivationOptions, +} from '@metamask/browser-passworder'; import { encrypt, decrypt, @@ -8,6 +11,9 @@ import { encryptWithDetail, decryptWithKey as decryptWithKeyBrowserPassworder, importKey as importKeyBrowserPassworder, + exportKey as exportKeyBrowserPassworder, + generateSalt as generateSaltBrowserPassworder, + keyFromPassword as keyFromPasswordBrowserPassworder, } from '@metamask/browser-passworder'; import { TOPRFError, @@ -99,27 +105,34 @@ const MOCK_AUTH_PUB_KEY = 'A09CwPHdl/qo2AjBOHen5d4QORaLedxOrSdgReq8IhzQ'; const MOCK_AUTH_PUB_KEY_OUTDATED = 'Ao2sa8imX7SD4KE4fJLoJ/iBufmaBxSFygG1qUhW2qAb'; -type WithControllerCallback = ({ - controller, - initialState, - encryptor, - messenger, -}: { - controller: SeedlessOnboardingController; - encryptor: VaultEncryptor; - initialState: SeedlessOnboardingControllerState; - messenger: SeedlessOnboardingControllerMessenger; - baseMessenger: Messenger; - toprfClient: ToprfSecureBackup; -}) => Promise | ReturnValue; - -type WithControllerOptions = Partial< - SeedlessOnboardingControllerOptions +type WithControllerCallback = + ({ + controller, + initialState, + encryptor, + messenger, + }: { + controller: SeedlessOnboardingController< + EKey, + SupportedKeyDerivationOptions + >; + encryptor: VaultEncryptor; + initialState: SeedlessOnboardingControllerState; + messenger: SeedlessOnboardingControllerMessenger; + baseMessenger: Messenger; + toprfClient: ToprfSecureBackup; + }) => Promise | ReturnValue; + +type WithControllerOptions = Partial< + SeedlessOnboardingControllerOptions >; -type WithControllerArgs = - | [WithControllerCallback] - | [WithControllerOptions, WithControllerCallback]; +type WithControllerArgs = + | [WithControllerCallback] + | [ + WithControllerOptions, + WithControllerCallback, + ]; /** * Get the default vault encryptor for the Seedless Onboarding Controller. @@ -139,6 +152,9 @@ function getDefaultSeedlessOnboardingVaultEncryptor() { payload: unknown, ) => Promise, importKey: importKeyBrowserPassworder, + exportKey: exportKeyBrowserPassworder, + generateSalt: generateSaltBrowserPassworder, + keyFromPassword: keyFromPasswordBrowserPassworder, }; } @@ -162,13 +178,20 @@ function createMockVaultEncryptor() { * @returns Whatever the callback returns. */ async function withController( - ...args: WithControllerArgs + ...args: WithControllerArgs< + ReturnValue, + EncryptionKey | webcrypto.CryptoKey, + KeyDerivationOptions + > ) { const [{ ...rest }, fn] = args.length === 2 ? args : [{}, args[0]]; const encryptor = new MockVaultEncryptor(); const { messenger, baseMessenger } = mockSeedlessOnboardingMessenger(); - const controller = new SeedlessOnboardingController({ + const controller = new SeedlessOnboardingController< + EncryptionKey | webcrypto.CryptoKey, + KeyDerivationOptions + >({ encryptor, messenger, network: Web3AuthNetwork.Devnet, @@ -313,9 +336,12 @@ function mockChangeEncKey( * @param seedPhrase - The mock seed phrase. * @param keyringId - The mock keyring id. */ -async function mockCreateToprfKeyAndBackupSeedPhrase( +async function mockCreateToprfKeyAndBackupSeedPhrase< + EKey, + SupportedKeyDerivationOptions, +>( toprfClient: ToprfSecureBackup, - controller: SeedlessOnboardingController, + controller: SeedlessOnboardingController, password: string, seedPhrase: Uint8Array, keyringId: string, @@ -462,7 +488,10 @@ describe('SeedlessOnboardingController', () => { describe('constructor', () => { it('should be able to instantiate', () => { const { messenger } = mockSeedlessOnboardingMessenger(); - const controller = new SeedlessOnboardingController({ + const controller = new SeedlessOnboardingController< + EncryptionKey | webcrypto.CryptoKey, + KeyDerivationOptions + >({ messenger, encryptor: getDefaultSeedlessOnboardingVaultEncryptor(), }); diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index b33cfadc010..41511dbd490 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -111,12 +111,18 @@ const seedlessOnboardingMetadata: StateMetadata extends BaseController< +export class SeedlessOnboardingController< + EncryptionKey, + SupportedKeyDerivationOptions, +> extends BaseController< typeof controllerName, SeedlessOnboardingControllerState, SeedlessOnboardingControllerMessenger > { - readonly #vaultEncryptor: VaultEncryptor; + readonly #vaultEncryptor: VaultEncryptor< + EncryptionKey, + SupportedKeyDerivationOptions + >; readonly #controllerOperationMutex = new Mutex(); @@ -147,7 +153,10 @@ export class SeedlessOnboardingController extends BaseController< encryptor, toprfKeyDeriver, network = Web3AuthNetwork.Mainnet, - }: SeedlessOnboardingControllerOptions) { + }: SeedlessOnboardingControllerOptions< + EncryptionKey, + SupportedKeyDerivationOptions + >) { super({ name: controllerName, metadata: seedlessOnboardingMetadata, diff --git a/packages/seedless-onboarding-controller/src/types.ts b/packages/seedless-onboarding-controller/src/types.ts index 5bff5bc7f91..40a572a87a4 100644 --- a/packages/seedless-onboarding-controller/src/types.ts +++ b/packages/seedless-onboarding-controller/src/types.ts @@ -157,8 +157,8 @@ export type SeedlessOnboardingControllerMessenger = RestrictedMessenger< /** * Encryptor interface for encrypting and decrypting seedless onboarding vault. */ -export type VaultEncryptor = Omit< - ExportableKeyEncryptor, +export type VaultEncryptor = Omit< + ExportableKeyEncryptor, 'encryptWithKey' >; @@ -189,7 +189,10 @@ export type ToprfKeyDeriver = { * @param state - The initial state to set on this controller. * @param encryptor - The encryptor to use for encrypting and decrypting seedless onboarding vault. */ -export type SeedlessOnboardingControllerOptions = { +export type SeedlessOnboardingControllerOptions< + EncryptionKey, + SupportedKeyDerivationOptions, +> = { messenger: SeedlessOnboardingControllerMessenger; /** @@ -202,7 +205,7 @@ export type SeedlessOnboardingControllerOptions = { * * @default browser-passworder @link https://github.com/MetaMask/browser-passworder */ - encryptor: VaultEncryptor; + encryptor: VaultEncryptor; /** * Optional key derivation interface for the TOPRF client. diff --git a/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts b/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts index e3568755c45..6e1a3157a0c 100644 --- a/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts +++ b/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts @@ -9,7 +9,8 @@ import { webcrypto } from 'node:crypto'; import type { VaultEncryptor } from '../../src/types'; export default class MockVaultEncryptor - implements VaultEncryptor + implements + VaultEncryptor { DEFAULT_DERIVATION_PARAMS: KeyDerivationOptions = { algorithm: 'PBKDF2', @@ -85,7 +86,7 @@ export default class MockVaultEncryptor async keyFromPassword( password: string, - salt: string = this.DEFAULT_SALT, + salt: string, exportable: boolean = true, opts: KeyDerivationOptions = this.DEFAULT_DERIVATION_PARAMS, ) { @@ -217,4 +218,8 @@ export default class MockVaultEncryptor const result = await this.decryptWithKey(key, payload); return result; } + + generateSalt(): string { + return this.DEFAULT_SALT; + } } From 5f563b5020f8f41f34f60fcc291fe97bf393ed6d Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 12 Jun 2025 13:37:52 +0200 Subject: [PATCH 10/44] update changelogs --- packages/keyring-controller/CHANGELOG.md | 5 +++++ packages/keyring-controller/src/KeyringController.test.ts | 2 +- packages/seedless-onboarding-controller/CHANGELOG.md | 4 ++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 218867a10a6..0549c174b1d 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -15,6 +15,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The encryption salt is now always taken from the vault. - **BREAKING:** The `KeyringController` constructor now requries an encryptor supporting the `keyFromPassword`, `exportKey` and `generateSalt` methods ([#5963](https://github.com/MetaMask/core/pull/5963)) +### Added + +- Added optional `SupportedKeyDerivationOptions` type parameter to the `ExportableKeyEncryptor` type ([#5963](https://github.com/MetaMask/core/pull/5963)) + - This type parameter allows specifying the key derivation options supported by the injected encryptor. + ## [22.0.2] ### Fixed diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index b523b7ff172..c37f59e7440 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -3041,7 +3041,7 @@ describe('KeyringController', () => { // the wrong encryptionKey type 12341234, ), - ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + ).rejects.toThrow(KeyringControllerError.WrongEncryptionKeyType); }); }); }); diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index 305bd5c110f..c55237e0ac5 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING:** `SeedlessOnboardingController` now requires an additional `SupportedKeyDerivationOptions` type parameter ([#5963](https://github.com/MetaMask/core/pull/5963)) + ### Added - Added `PrivateKey sync` feature to the controller. ([#5948](https://github.com/MetaMask/core/pull/5948)) From 86b6397809cf99194381fa3ec7421850ebd23c3e Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 12 Jun 2025 13:43:56 +0200 Subject: [PATCH 11/44] fix changelog validation --- packages/keyring-controller/CHANGELOG.md | 10 +++++----- packages/seedless-onboarding-controller/CHANGELOG.md | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 0549c174b1d..1ee1b745397 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Added optional `SupportedKeyDerivationOptions` type parameter to the `ExportableKeyEncryptor` type ([#5963](https://github.com/MetaMask/core/pull/5963)) + - This type parameter allows specifying the key derivation options supported by the injected encryptor. + ### Changed - **BREAKING:** The `cacheEncryptionKey` parameter has been removed from the `KeyringController` constructor options ([#5963](https://github.com/MetaMask/core/pull/5963)) @@ -15,11 +20,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The encryption salt is now always taken from the vault. - **BREAKING:** The `KeyringController` constructor now requries an encryptor supporting the `keyFromPassword`, `exportKey` and `generateSalt` methods ([#5963](https://github.com/MetaMask/core/pull/5963)) -### Added - -- Added optional `SupportedKeyDerivationOptions` type parameter to the `ExportableKeyEncryptor` type ([#5963](https://github.com/MetaMask/core/pull/5963)) - - This type parameter allows specifying the key derivation options supported by the injected encryptor. - ## [22.0.2] ### Fixed diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index c55237e0ac5..4f75c20e6a5 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -7,10 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Changed - -- **BREAKING:** `SeedlessOnboardingController` now requires an additional `SupportedKeyDerivationOptions` type parameter ([#5963](https://github.com/MetaMask/core/pull/5963)) - ### Added - Added `PrivateKey sync` feature to the controller. ([#5948](https://github.com/MetaMask/core/pull/5948)) @@ -20,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - renamed `fetchAllSeedPhrases` method to `fetchAllSecretData` and updated the return value to `Record`. - added new error message, `MissingKeyringId` which will throw if no `keyringId` is provided during seed phrase (Mnemonic) backup. +### Changed + +- **BREAKING:** `SeedlessOnboardingController` now requires an additional `SupportedKeyDerivationOptions` type parameter ([#5963](https://github.com/MetaMask/core/pull/5963)) + ## [1.0.0] ### Added From f3af65b3da5cf2ee8ac300b78e3a449513d80647 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 12 Jun 2025 14:38:46 +0200 Subject: [PATCH 12/44] remove `CachedEncryptionKey.salt` optionality --- packages/keyring-controller/src/KeyringController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index a4fb7674deb..a0e6f0c9fbf 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -324,7 +324,7 @@ type CachedEncryptionKey = { /** * The salt used to derive the encryption key. */ - salt?: string; + salt: string; }; /** From 6e24cbb969059e5925f2acc94bcd49e831b43517 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 13 Jun 2025 10:52:28 +0200 Subject: [PATCH 13/44] (@mcmire): refactor changelog --- packages/keyring-controller/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 1ee1b745397..e7a1e7a6b4a 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -14,11 +14,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** The `KeyringController` constructor now requires an encryptor supporting the `keyFromPassword`, `exportKey` and `generateSalt` methods ([#5963](https://github.com/MetaMask/core/pull/5963)) + +### Removed + - **BREAKING:** The `cacheEncryptionKey` parameter has been removed from the `KeyringController` constructor options ([#5963](https://github.com/MetaMask/core/pull/5963)) - This parameter was previously used to enable encryption key in-memory caching, but it is no longer needed as the controller now always uses the latest encryption key. - **BREAKING:** The `submitEncryptionKey` method does not accept an `encryptionSalt` argument anymore ([#5963](https://github.com/MetaMask/core/pull/5963)) - The encryption salt is now always taken from the vault. -- **BREAKING:** The `KeyringController` constructor now requries an encryptor supporting the `keyFromPassword`, `exportKey` and `generateSalt` methods ([#5963](https://github.com/MetaMask/core/pull/5963)) ## [22.0.2] From 13f36eb893f3d7993d8fa108be2f5b05405716c8 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 13 Jun 2025 10:53:30 +0200 Subject: [PATCH 14/44] fix typo --- packages/keyring-controller/src/KeyringController.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index a0e6f0c9fbf..a403dbfee4e 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1506,7 +1506,7 @@ export class KeyringController extends BaseController< // If the vault is being upgraded, we want to ignore the metadata // that is already in the vault, so we can effectively // re-encrypt the vault with the new encryption config. - ignoreVautKeyMetadata: true, + ignoreVaultKeyMetadata: true, }); await this.#updateVault(); } @@ -2127,12 +2127,12 @@ export class KeyringController extends BaseController< * * @param password - The password to use for decryption or derivation. * @param options - Options for the key derivation. - * @param options.ignoreVautKeyMetadata - Whether to ignore the vault key metadata + * @param options.ignoreVaultKeyMetadata - Whether to ignore the vault key metadata */ async #deriveEncryptionKey( password: string, - options: { ignoreVautKeyMetadata: boolean } = { - ignoreVautKeyMetadata: false, + options: { ignoreVaultKeyMetadata: boolean } = { + ignoreVaultKeyMetadata: false, }, ): Promise { this.#assertControllerMutexIsLocked(); @@ -2143,7 +2143,7 @@ export class KeyringController extends BaseController< } let salt: string, keyMetadata: unknown; - if (vault && !options.ignoreVautKeyMetadata) { + if (vault && !options.ignoreVaultKeyMetadata) { const parsedVault = JSON.parse(vault); salt = parsedVault.salt; keyMetadata = parsedVault.keyMetadata; From 169346d2611c30a06cc75338b344e1ad62232fa0 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 18 Jun 2025 11:15:51 +0200 Subject: [PATCH 15/44] update changelog --- packages/seedless-onboarding-controller/CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index ddfe7af986e..fdb8871c247 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** `SeedlessOnboardingController` now requires an additional `SupportedKeyDerivationOptions` type parameter ([#5963](https://github.com/MetaMask/core/pull/5963)) - Refresh and revoke token handling ([#5917](https://github.com/MetaMask/core/pull/5917)) - **BREAKING:** `authenticate` need extra `refreshToken` and `revokeToken` params, persist refresh token in state and store revoke token temporarily for user in next step - `createToprfKeyAndBackupSeedPhrase`, `fetchAllSecretData` store revoke token in vault @@ -28,10 +29,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `submitPassword` revoke refresh token and replace with new one after password submit to prevent malicious use if refresh token leak in persisted state - Removed `recoveryRatelimitCache` from the controller state. ([#5976](https://github.com/MetaMask/core/pull/5976)). -### Changed - -- **BREAKING:** `SeedlessOnboardingController` now requires an additional `SupportedKeyDerivationOptions` type parameter ([#5963](https://github.com/MetaMask/core/pull/5963)) - ## [1.0.0] ### Added From f037800d2258f98d1b1f6f8dd7d925e1e21ccadb Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 19 Jun 2025 12:48:21 +0200 Subject: [PATCH 16/44] refactor: rename class variable to instance variable --- packages/keyring-controller/src/KeyringController.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index a403dbfee4e..63df81bfe50 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2118,7 +2118,7 @@ export class KeyringController extends BaseController< /** * Derive the vault encryption key from the provided password, and - * assign it to the class variable for later use with cryptographic + * assign it to the instance variable for later use with cryptographic * functions. * * When the controller has a vault in its state, the key is derived @@ -2163,7 +2163,7 @@ export class KeyringController extends BaseController< /** * Use the provided encryption key and salt to set the - * encryptionKey class variable. This method is used + * encryptionKey instance variable. This method is used * when the user provides an encryption key and salt * to unlock the keychain, instead of using a password. * @@ -2283,7 +2283,7 @@ export class KeyringController extends BaseController< } /** - * Get a snapshot of session data held by class variables. + * Get a snapshot of session data held by instance variables. * * @returns An object with serialized keyrings, keyrings metadata, * and the user password. From c716ec20ab6dc92529ca995304ec80ffa440c51d Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 22 Jul 2025 11:59:45 +0200 Subject: [PATCH 17/44] restore `encryptionSalt` method argument --- packages/keyring-controller/jest.config.js | 2 +- .../src/KeyringController.test.ts | 58 ++++++++++++++++--- .../src/KeyringController.ts | 20 +++++-- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index b419b2a2e9a..a2c27be2575 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,7 +17,7 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 94.01, + branches: 94.11, functions: 100, lines: 98.76, statements: 98.77, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index c37f59e7440..11430b9b315 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -38,6 +38,7 @@ import { } from './KeyringController'; import MockEncryptor, { MOCK_ENCRYPTION_KEY, + MOCK_ENCRYPTION_SALT, MOCK_KEY, } from '../tests/mocks/mockEncryptor'; import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring'; @@ -2944,7 +2945,10 @@ describe('KeyringController', () => { describe('submitEncryptionKey', () => { it('should submit encryption key and decrypt', async () => { await withController(async ({ controller, initialState }) => { - await controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY); + await controller.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + MOCK_ENCRYPTION_SALT, + ); expect(controller.state).toStrictEqual(initialState); }); }); @@ -2954,10 +2958,13 @@ describe('KeyringController', () => { { skipVaultCreation: true, state: { - vault: JSON.stringify({ data: '0x123', salt: 'my salt' }), + vault: JSON.stringify({ + data: '0x123', + salt: MOCK_ENCRYPTION_SALT, + }), // @ts-expect-error we want to force the controller to have an // encryption salt equal to the one in the vault - encryptionSalt: 'my salt', + encryptionSalt: MOCK_ENCRYPTION_SALT, }, }, async ({ controller, encryptor }) => { @@ -2968,7 +2975,10 @@ describe('KeyringController', () => { }, ]); - await controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY); + await controller.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + MOCK_ENCRYPTION_SALT, + ); expect(controller.state.isUnlocked).toBe(true); }, @@ -2984,10 +2994,13 @@ describe('KeyringController', () => { { skipVaultCreation: true, state: { - vault: JSON.stringify({ data: '0x123', salt: 'my salt' }), + vault: JSON.stringify({ + data: '0x123', + salt: MOCK_ENCRYPTION_SALT, + }), // @ts-expect-error we want to force the controller to have an // encryption salt equal to the one in the vault - encryptionSalt: 'my salt', + encryptionSalt: MOCK_ENCRYPTION_SALT, }, }, async ({ controller, encryptor }) => { @@ -3000,7 +3013,10 @@ describe('KeyringController', () => { }, ]); - await controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY); + await controller.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + MOCK_ENCRYPTION_SALT, + ); expect(controller.state.isUnlocked).toBe(true); expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [ @@ -3028,7 +3044,10 @@ describe('KeyringController', () => { ]); await expect( - controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY), + controller.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + MOCK_ENCRYPTION_SALT, + ), ).rejects.toThrow(KeyringControllerError.VaultDataError); }); }); @@ -3040,10 +3059,32 @@ describe('KeyringController', () => { // @ts-expect-error we are testing the case of a user using // the wrong encryptionKey type 12341234, + MOCK_ENCRYPTION_SALT, + ), + ).rejects.toThrow(KeyringControllerError.WrongEncryptionKeyType); + }); + }); + + it('should throw error if encryptionSalt is of an unexpected type', async () => { + await withController(async ({ controller }) => { + await expect( + controller.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + // @ts-expect-error we are testing the case of a user using + // the wrong encryptionSalt type + 12341234, ), ).rejects.toThrow(KeyringControllerError.WrongEncryptionKeyType); }); }); + + it('should throw error if encryptionSalt is different from the one in the vault', async () => { + await withController(async ({ controller }) => { + await expect( + controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY, '0x1234'), + ).rejects.toThrow(KeyringControllerError.ExpiredCredentials); + }); + }); }); describe('verifySeedPhrase', () => { @@ -4136,6 +4177,7 @@ describe('KeyringController', () => { // ..and unlocking it should add a new instance of QRKeyring await signProcessKeyringController.submitEncryptionKey( MOCK_ENCRYPTION_KEY, + MOCK_ENCRYPTION_SALT, ); // We call `getQRKeyring` instead of `getOrAddQRKeyring` so that // we are able to test if the subscription to the internal QR keyring diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 63df81bfe50..75e1978aa3e 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1456,12 +1456,17 @@ export class KeyringController extends BaseController< * using the given encryption key and salt. * * @param encryptionKey - Key to unlock the keychain. + * @param encryptionSalt - Salt to unlock the keychain. * @returns Promise resolving when the operation completes. */ - async submitEncryptionKey(encryptionKey: string): Promise { + async submitEncryptionKey( + encryptionKey: string, + encryptionSalt: string, + ): Promise { const { newMetadata } = await this.#withRollback(async () => { const result = await this.#unlockKeyrings({ exportedEncryptionKey: encryptionKey, + encryptionKeySalt: encryptionSalt, }); this.#setUnlocked(); return result; @@ -2180,6 +2185,11 @@ export class KeyringController extends BaseController< throw new TypeError(KeyringControllerError.WrongEncryptionKeyType); } + const { vault } = this.state; + if (vault && JSON.parse(vault).salt !== encryptionSalt) { + throw new Error(KeyringControllerError.ExpiredCredentials); + } + this.#encryptionKey = { salt: encryptionSalt, exported: encryptionKey, @@ -2339,6 +2349,7 @@ export class KeyringController extends BaseController< } | { exportedEncryptionKey: string; + encryptionKeySalt: string; }, ): Promise<{ keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; @@ -2348,15 +2359,13 @@ export class KeyringController extends BaseController< if (!this.state.vault) { throw new Error(KeyringControllerError.VaultError); } - const parsedEncryptedVault = JSON.parse(this.state.vault); if ('password' in credentials) { await this.#deriveEncryptionKey(credentials.password); } else { - const { exportedEncryptionKey } = credentials; this.#useEncryptionKey( - exportedEncryptionKey, - parsedEncryptedVault.salt, + credentials.exportedEncryptionKey, + credentials.encryptionKeySalt, ); } @@ -2365,6 +2374,7 @@ export class KeyringController extends BaseController< throw new Error(KeyringControllerError.MissingCredentials); } + const parsedEncryptedVault = JSON.parse(this.state.vault); const key = await this.#encryptor.importKey(encryptionKey); const vault = await this.#encryptor.decryptWithKey( key, From 3d3a3abd666769ad9ee2cbf02d483545682c7b90 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 22 Jul 2025 12:14:14 +0200 Subject: [PATCH 18/44] rename `ignoreVaultKeyMetadata` to `useVaultKeyMetadata` --- packages/keyring-controller/src/KeyringController.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 75e1978aa3e..4380f69879b 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1511,7 +1511,7 @@ export class KeyringController extends BaseController< // If the vault is being upgraded, we want to ignore the metadata // that is already in the vault, so we can effectively // re-encrypt the vault with the new encryption config. - ignoreVaultKeyMetadata: true, + useVaultKeyMetadata: false, }); await this.#updateVault(); } @@ -2132,12 +2132,12 @@ export class KeyringController extends BaseController< * * @param password - The password to use for decryption or derivation. * @param options - Options for the key derivation. - * @param options.ignoreVaultKeyMetadata - Whether to ignore the vault key metadata + * @param options.useVaultKeyMetadata - Whether to use the vault key metadata */ async #deriveEncryptionKey( password: string, - options: { ignoreVaultKeyMetadata: boolean } = { - ignoreVaultKeyMetadata: false, + options: { useVaultKeyMetadata: boolean } = { + useVaultKeyMetadata: true, }, ): Promise { this.#assertControllerMutexIsLocked(); @@ -2148,7 +2148,7 @@ export class KeyringController extends BaseController< } let salt: string, keyMetadata: unknown; - if (vault && !options.ignoreVaultKeyMetadata) { + if (vault && options.useVaultKeyMetadata) { const parsedVault = JSON.parse(vault); salt = parsedVault.salt; keyMetadata = parsedVault.keyMetadata; From e529a0995a8d37dcae129543bd62c7c2d7c737fc Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 22 Jul 2025 12:18:38 +0200 Subject: [PATCH 19/44] remove salt conditional assignment --- packages/keyring-controller/jest.config.js | 2 +- packages/keyring-controller/src/KeyringController.ts | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index a2c27be2575..477b3659e77 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,7 +17,7 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 94.11, + branches: 94.08, functions: 100, lines: 98.76, statements: 98.77, diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 4380f69879b..7d980017eb6 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2429,12 +2429,10 @@ export class KeyringController extends BaseController< key, serializedKeyrings, ); - if (this.#encryptionKey.salt) { - // We need to include the salt used to derive - // the encryption key, to be able to derive it - // from password again. - encryptedVault.salt = this.#encryptionKey.salt; - } + // We need to include the salt used to derive + // the encryption key, to be able to derive it + // from password again. + encryptedVault.salt = this.#encryptionKey.salt; const updatedState: Partial = { vault: JSON.stringify(encryptedVault), encryptionKey: this.#encryptionKey.exported, From b543a75f84070906a7dbdfcceae8bb0c80d41582 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 22 Jul 2025 12:57:13 +0200 Subject: [PATCH 20/44] refactor encryption key rollback --- packages/keyring-controller/jest.config.js | 2 +- packages/keyring-controller/src/KeyringController.ts | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 477b3659e77..d1a2f2e845c 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,7 +17,7 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 94.08, + branches: 93.97, functions: 100, lines: 98.76, statements: 98.77, diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 7d980017eb6..a9cf93a58a8 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -36,7 +36,7 @@ import { Mutex } from 'async-mutex'; import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; -import { isEqual } from 'lodash'; +import { cloneDeep, isEqual } from 'lodash'; // When generating a ULID within the same millisecond, monotonicFactory provides some guarantees regarding sort order. import { ulid } from 'ulid'; @@ -2769,16 +2769,13 @@ export class KeyringController extends BaseController< ): Promise { return this.#withControllerLock(async ({ releaseLock }) => { const currentSerializedKeyrings = await this.#getSerializedKeyrings(); - const currentEncryptionKey = this.#encryptionKey?.exported; - const currentEncryptionSalt = this.#encryptionKey?.salt; + const currentEncryptionKey = cloneDeep(this.#encryptionKey); try { return await callback({ releaseLock }); } catch (e) { // Keyrings and encryption credentials are restored to their previous state - if (currentEncryptionKey && currentEncryptionSalt) { - this.#useEncryptionKey(currentEncryptionKey, currentEncryptionSalt); - } + this.#encryptionKey = currentEncryptionKey; await this.#restoreSerializedKeyrings(currentSerializedKeyrings); throw e; From 4cbfca612147a182c2acaa4372da694dbd375e70 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 22 Jul 2025 13:19:50 +0200 Subject: [PATCH 21/44] remove cacheEncryptionKey references --- packages/keyring-controller/src/KeyringController.test.ts | 2 +- packages/keyring-controller/src/KeyringController.ts | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 11430b9b315..c5663bfe44b 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -90,7 +90,7 @@ describe('KeyringController', () => { ).not.toThrow(); }); - it('should throw error if cacheEncryptionKey is true and encryptor does not support key export', () => { + it('should throw error if encryptor does not support key export', () => { expect( () => new KeyringController({ diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index a9cf93a58a8..a48df12b11a 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -93,8 +93,7 @@ export type KeyringControllerState = { keyrings: KeyringObject[]; /** * The encryption key derived from the password and used to encrypt - * the vault. This is only stored if the `cacheEncryptionKey` option - * is enabled. + * the vault. */ encryptionKey?: string; /** @@ -693,7 +692,6 @@ export class KeyringController extends BaseController< * @param options - Initial options used to configure this controller * @param options.encryptor - An optional object for defining encryption schemes. * @param options.keyringBuilders - Set a new name for account. - * @param options.cacheEncryptionKey - Whether to cache or not encryption key. * @param options.messenger - A restricted messenger. * @param options.state - Initial state to set on this controller. */ From 434c779722b76b5d21d8530436d68429cf9fd7bd Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 23 Jul 2025 14:54:54 +0200 Subject: [PATCH 22/44] fix lint --- packages/keyring-controller/src/KeyringController.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 687231f15bd..2336b531ade 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -37,7 +37,6 @@ import { keyringBuilderFactory, } from './KeyringController'; import MockEncryptor, { - DECRYPTION_ERROR, MOCK_ENCRYPTION_KEY, MOCK_KEY, SALT, From 069cac9043902f2303464d856dab1001826327df Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 23 Jul 2025 14:55:03 +0200 Subject: [PATCH 23/44] fix seedless-onboarding-controller tests --- .../src/SeedlessOnboardingController.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 4364547e04f..f731398a7bd 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -393,7 +393,7 @@ function mockChangeEncKey( * @param newPassword - The new password. */ async function mockChangePassword( - controller: SeedlessOnboardingController, + controller: SeedlessOnboardingController, toprfClient: ToprfSecureBackup, oldPassword: string, newPassword: string, From c18a35eba751216bffde96bb593ff43632c60ee5 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 23 Jul 2025 14:58:12 +0200 Subject: [PATCH 24/44] change `MockEncryptor.keyFromPassword` signature --- packages/keyring-controller/tests/mocks/mockEncryptor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/tests/mocks/mockEncryptor.ts b/packages/keyring-controller/tests/mocks/mockEncryptor.ts index e9f6703866d..40a58e1250b 100644 --- a/packages/keyring-controller/tests/mocks/mockEncryptor.ts +++ b/packages/keyring-controller/tests/mocks/mockEncryptor.ts @@ -98,7 +98,7 @@ export default class MockEncryptor implements ExportableKeyEncryptor { return data.value; } - async keyFromPassword(_password: string, _salt?: string) { + async keyFromPassword(_password: string, _salt: string) { return JSON.parse(MOCK_ENCRYPTION_KEY); } From 7b919d75dd7fc146ca48edddf5a6988e2d927f88 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 23 Jul 2025 15:00:54 +0200 Subject: [PATCH 25/44] fix seedless-onboarding-controller changelog --- packages/seedless-onboarding-controller/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index a4c8efb157a..c0bffdf5719 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING:** `SeedlessOnboardingController` now requires an additional `SupportedKeyDerivationOptions` type parameter ([#5963](https://github.com/MetaMask/core/pull/5963)) + ## [2.4.0] ### Fixed @@ -71,7 +75,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **BREAKING:** `SeedlessOnboardingController` now requires an additional `SupportedKeyDerivationOptions` type parameter ([#5963](https://github.com/MetaMask/core/pull/5963)) - Refresh and revoke token handling ([#5917](https://github.com/MetaMask/core/pull/5917)) - **BREAKING:** `authenticate` need extra `refreshToken` and `revokeToken` params, persist refresh token in state and store revoke token temporarily for user in next step - `createToprfKeyAndBackupSeedPhrase`, `fetchAllSecretData` store revoke token in vault From 9e906a5fb24b0943e32b03d8a6f657901f01bb5e Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 23 Jul 2025 18:05:21 +0200 Subject: [PATCH 26/44] rename `#encryptionKey.exported` to `#encryptionKey.serialized` --- .../src/KeyringController.ts | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 9fbfb9e837a..8edac9c03e2 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -317,9 +317,9 @@ export type SerializedKeyring = { */ type CachedEncryptionKey = { /** - * The exported encryption key string. + * The serialized encryption key. */ - exported: string; + serialized: string; /** * The salt used to derive the encryption key. */ @@ -390,12 +390,12 @@ export type ExportableKeyEncryptor< ) => Promise; /** * Encrypts the given object with the given password, and returns the - * encryption result and the exported key string. + * encryption result and the serialized key string. * * @param password - The password to encrypt with. * @param object - The object to encrypt. * @param salt - The optional salt to use for encryption. - * @returns The encrypted string and the exported key string. + * @returns The encrypted string and the serialized key string. */ encryptWithDetail: ( password: string, @@ -415,12 +415,12 @@ export type ExportableKeyEncryptor< ) => Promise; /** * Decrypts the given encrypted string with the given password, and returns - * the decrypted object and the salt and exported key string used for + * the decrypted object and the salt and serialized key string used for * encryption. * * @param password - The password to decrypt with. * @param encryptedString - The encrypted string to decrypt. - * @returns The decrypted object and the salt and exported key string used for + * @returns The decrypted object and the salt and serialized key string used for * encryption. */ decryptWithDetail: ( @@ -428,9 +428,9 @@ export type ExportableKeyEncryptor< encryptedString: string, ) => Promise; /** - * Generates an encryption key from exported key string. + * Generates an encryption key from a serialized key. * - * @param key - The exported key string. + * @param key - The serialized key string. * @returns The encryption key. */ importKey: (key: string) => Promise; @@ -438,7 +438,7 @@ export type ExportableKeyEncryptor< * Exports the encryption key as a string. * * @param key - The encryption key to export. - * @returns The exported key string. + * @returns The serialized key string. */ exportKey: (key: EncryptionKey) => Promise; /** @@ -1509,8 +1509,8 @@ export class KeyringController extends BaseController< this.#assertIsUnlocked(); return await this.#withControllerLock(async () => { - assertIsEncryptionKeySet(this.#encryptionKey?.exported); - return this.#encryptionKey.exported; + assertIsEncryptionKeySet(this.#encryptionKey?.serialized); + return this.#encryptionKey.serialized; }); } @@ -2183,13 +2183,13 @@ export class KeyringController extends BaseController< salt = this.#encryptor.generateSalt(); } - const exportedEncryptionKey = await this.#encryptor.exportKey( + const serializedEncryptionKey = await this.#encryptor.exportKey( await this.#encryptor.keyFromPassword(password, salt, true, keyMetadata), ); this.#encryptionKey = { salt, - exported: exportedEncryptionKey, + serialized: serializedEncryptionKey, }; } @@ -2219,7 +2219,7 @@ export class KeyringController extends BaseController< this.#encryptionKey = { salt: encryptionSalt, - exported: encryptionKey, + serialized: encryptionKey, }; } @@ -2397,7 +2397,7 @@ export class KeyringController extends BaseController< ); } - const encryptionKey = this.#encryptionKey?.exported; + const encryptionKey = this.#encryptionKey?.serialized; if (!encryptionKey) { throw new Error(KeyringControllerError.MissingCredentials); } @@ -2451,7 +2451,9 @@ export class KeyringController extends BaseController< throw new Error(KeyringControllerError.NoHdKeyring); } - const key = await this.#encryptor.importKey(this.#encryptionKey.exported); + const key = await this.#encryptor.importKey( + this.#encryptionKey.serialized, + ); const encryptedVault = await this.#encryptor.encryptWithKey( key, serializedKeyrings, @@ -2462,7 +2464,7 @@ export class KeyringController extends BaseController< encryptedVault.salt = this.#encryptionKey.salt; const updatedState: Partial = { vault: JSON.stringify(encryptedVault), - encryptionKey: this.#encryptionKey.exported, + encryptionKey: this.#encryptionKey.serialized, encryptionSalt: this.#encryptionKey.salt, }; From 93e72f5f5e8950918028945890ff790654e19a7a Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 23 Jul 2025 18:10:46 +0200 Subject: [PATCH 27/44] rename `#useEncryptionKey()` and `encryptionSalt` --- .../src/KeyringController.ts | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 8edac9c03e2..de533101d11 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1469,17 +1469,17 @@ export class KeyringController extends BaseController< * consistency with the vault salt. * * @param encryptionKey - Key to unlock the keychain. - * @param encryptionSalt - Optional salt to unlock the keychain. + * @param keyDerivationSalt - Optional salt to unlock the keychain. * @returns Promise resolving when the operation completes. */ async submitEncryptionKey( encryptionKey: string, - encryptionSalt?: string, + keyDerivationSalt?: string, ): Promise { const { newMetadata } = await this.#withRollback(async () => { const result = await this.#unlockKeyrings({ - exportedEncryptionKey: encryptionKey, - encryptionKeySalt: encryptionSalt, + encryptionKey, + keyDerivationSalt, }); this.#setUnlocked(); return result; @@ -2194,31 +2194,30 @@ export class KeyringController extends BaseController< } /** - * Use the provided encryption key and salt to set the - * encryptionKey instance variable. This method is used - * when the user provides an encryption key and salt + * Set the the `#encryptionKey` instance variable. + * This method is used when the user provides an encryption key and salt * to unlock the keychain, instead of using a password. * * @param encryptionKey - The encryption key to use. - * @param encryptionSalt - The salt to use for the encryption key. + * @param keyDerivationSalt - The salt to use for the encryption key. */ - #useEncryptionKey(encryptionKey: string, encryptionSalt: string): void { + #setEncryptionKey(encryptionKey: string, keyDerivationSalt: string): void { this.#assertControllerMutexIsLocked(); if ( typeof encryptionKey !== 'string' || - typeof encryptionSalt !== 'string' + typeof keyDerivationSalt !== 'string' ) { throw new TypeError(KeyringControllerError.WrongEncryptionKeyType); } const { vault } = this.state; - if (vault && JSON.parse(vault).salt !== encryptionSalt) { + if (vault && JSON.parse(vault).salt !== keyDerivationSalt) { throw new Error(KeyringControllerError.ExpiredCredentials); } this.#encryptionKey = { - salt: encryptionSalt, + salt: keyDerivationSalt, serialized: encryptionKey, }; } @@ -2375,8 +2374,8 @@ export class KeyringController extends BaseController< password: string; } | { - exportedEncryptionKey: string; - encryptionKeySalt?: string; + encryptionKey: string; + keyDerivationSalt?: string; }, ): Promise<{ keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; @@ -2391,9 +2390,9 @@ export class KeyringController extends BaseController< if ('password' in credentials) { await this.#deriveEncryptionKey(credentials.password); } else { - this.#useEncryptionKey( - credentials.exportedEncryptionKey, - credentials.encryptionKeySalt || parsedEncryptedVault.salt, + this.#setEncryptionKey( + credentials.encryptionKey, + credentials.keyDerivationSalt || parsedEncryptedVault.salt, ); } From 2803e22045f0816ece208b22684711119aff9d4e Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 3 Nov 2025 11:31:06 +0100 Subject: [PATCH 28/44] fix `decryptWithKey` type --- packages/keyring-controller/CHANGELOG.md | 4 ++++ packages/keyring-controller/src/KeyringController.ts | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 3037534679f..a418c822f1b 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **BREAKING:** The `KeyringController` constructor now requires an encryptor supporting the `keyFromPassword`, `exportKey` and `generateSalt` methods ([#5963](https://github.com/MetaMask/core/pull/5963)) +### Fixed + +- Fixed incorrect type for `decryptWithKey` method of `ExportableKeyEncryptor` ([#5963](https://github.com/MetaMask/core/pull/5963)) + ### Removed - **BREAKING:** The `cacheEncryptionKey` parameter has been removed from the `KeyringController` constructor options ([#5963](https://github.com/MetaMask/core/pull/5963)) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 29efc18cff4..c5a0258d52e 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -412,12 +412,12 @@ export type ExportableKeyEncryptor< * Decrypts the given encrypted string with the given encryption key. * * @param key - The encryption key to decrypt with. - * @param encryptedString - The encrypted string to decrypt. + * @param encryptedObject - The encrypted string to decrypt. * @returns The decrypted object. */ decryptWithKey: ( key: EncryptionKey, - encryptedString: string, + encryptedObject: encryptorUtils.EncryptionResult, ) => Promise; /** * Decrypts the given encrypted string with the given password, and returns From 348860ff8c6868b254978e41bafda2429f5b6d95 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 4 Nov 2025 10:00:27 +0100 Subject: [PATCH 29/44] fix `mockEncryptor` --- .../keyring-controller/tests/mocks/mockEncryptor.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/keyring-controller/tests/mocks/mockEncryptor.ts b/packages/keyring-controller/tests/mocks/mockEncryptor.ts index 40a58e1250b..f5f20e3b487 100644 --- a/packages/keyring-controller/tests/mocks/mockEncryptor.ts +++ b/packages/keyring-controller/tests/mocks/mockEncryptor.ts @@ -42,7 +42,8 @@ export default class MockEncryptor implements ExportableKeyEncryptor { async decrypt(password: string, text: string): Promise { const { salt } = JSON.parse(text); const key = deriveKey(password, salt); - return await this.decryptWithKey(key, text); + const payload = JSON.parse(text); + return await this.decryptWithKey(key, payload); } async encryptWithDetail( @@ -68,8 +69,9 @@ export default class MockEncryptor implements ExportableKeyEncryptor { ): Promise { const { salt } = JSON.parse(text); const key = deriveKey(password, salt); + const payload = JSON.parse(text); return { - vault: await this.decryptWithKey(key, text), + vault: await this.decryptWithKey(key, payload), salt, exportedKeyString: JSON.stringify(key), }; @@ -86,7 +88,10 @@ export default class MockEncryptor implements ExportableKeyEncryptor { }; } - async decryptWithKey(key: unknown, ciphertext: string): Promise { + async decryptWithKey( + key: unknown, + ciphertext: EncryptionResult, + ): Promise { // This conditional assignment is required because sometimes the keyring // controller passes in the parsed object instead of the string. const ciphertextObj = From fdb985904f16cade3ff528cee6279ea2a414b3af Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 4 Nov 2025 10:25:42 +0100 Subject: [PATCH 30/44] fix `SeedlessOnboardingController` vault encryptor types --- .../src/SeedlessOnboardingController.test.ts | 5 +---- .../tests/mocks/vaultEncryptor.ts | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index aa638f3e901..3ec55f5d7b5 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -675,10 +675,7 @@ describe('SeedlessOnboardingController', () => { newRefreshToken: 'newRefreshToken', }); const { messenger } = mockSeedlessOnboardingMessenger(); - const controller = new SeedlessOnboardingController< - EncryptionKey | webcrypto.CryptoKey, - KeyDerivationOptions - >({ + const controller = new SeedlessOnboardingController({ messenger, encryptor: getDefaultSeedlessOnboardingVaultEncryptor(), refreshJWTToken: mockRefreshJWTToken, diff --git a/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts b/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts index 6e1a3157a0c..b424037cd14 100644 --- a/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts +++ b/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts @@ -152,7 +152,7 @@ export default class MockVaultEncryptor async decryptWithKey( encryptionKey: EncryptionKey | webcrypto.CryptoKey, - payload: string, + payload: EncryptionResult, ) { let encData: EncryptionResult; if (typeof payload === 'string') { From dd446299c8aaec919c7c083c1612d68dd5fa05bb Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 4 Nov 2025 10:55:25 +0100 Subject: [PATCH 31/44] add type parameters to KeyringController --- packages/keyring-controller/CHANGELOG.md | 13 +-- .../src/KeyringController.ts | 84 +++++++------------ 2 files changed, 38 insertions(+), 59 deletions(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index a418c822f1b..6592a4bebba 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -9,21 +9,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Added optional `SupportedKeyDerivationOptions` type parameter to the `ExportableKeyEncryptor` type ([#5963](https://github.com/MetaMask/core/pull/5963)) - - This type parameter allows specifying the key derivation options supported by the injected encryptor. +- Added optional `EncryptionKey` and `SupportedKeyDerivationOptions` type parameters to the `KeyringController, `ExportableKeyEncryptor` and `KeyringControllerOptions` type ([#5963](https://github.com/MetaMask/core/pull/5963)) + - This type parameter allows specifying the key derivation options supported by the injected encryptor, defaulting to `@metamask/browser-passworder` types. ### Changed - **BREAKING:** The `KeyringController` constructor now requires an encryptor supporting the `keyFromPassword`, `exportKey` and `generateSalt` methods ([#5963](https://github.com/MetaMask/core/pull/5963)) -### Fixed - -- Fixed incorrect type for `decryptWithKey` method of `ExportableKeyEncryptor` ([#5963](https://github.com/MetaMask/core/pull/5963)) - ### Removed - **BREAKING:** The `cacheEncryptionKey` parameter has been removed from the `KeyringController` constructor options ([#5963](https://github.com/MetaMask/core/pull/5963)) - This parameter was previously used to enable encryption key in-memory caching, but it is no longer needed as the controller now always uses the latest encryption key. +- **BREAKING:** The `GenericEncryptor` type has been removed ([#5963](https://github.com/MetaMask/core/pull/5963)) + +### Fixed + +- Fixed incorrect type for `decryptWithKey` method of `ExportableKeyEncryptor` ([#5963](https://github.com/MetaMask/core/pull/5963)) ## [24.0.0] diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index c5a0258d52e..22e5c5874e4 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1,7 +1,7 @@ import type { TypedTransaction, TypedTxData } from '@ethereumjs/tx'; import { isValidPrivate, getBinarySize } from '@ethereumjs/util'; import { BaseController } from '@metamask/base-controller'; -import * as encryptorUtils from '@metamask/browser-passworder'; +import type * as encryptorUtils from '@metamask/browser-passworder'; import { HdKeyring } from '@metamask/eth-hd-keyring'; import { normalize as ethNormalize } from '@metamask/eth-sig-util'; import SimpleKeyring from '@metamask/eth-simple-keyring'; @@ -251,11 +251,17 @@ export type KeyringControllerMessenger = Messenger< KeyringControllerEvents >; -export type KeyringControllerOptions = { +export type KeyringControllerOptions< + EncryptionKey = encryptorUtils.EncryptionKey | CryptoKey, + SupportedKeyDerivationOptions = encryptorUtils.KeyDerivationOptions, +> = { keyringBuilders?: { (): EthKeyring; type: string }[]; messenger: KeyringControllerMessenger; state?: { vault?: string; keyringsMetadata?: KeyringMetadata[] }; - encryptor?: ExportableKeyEncryptor; + encryptor: ExportableKeyEncryptor< + EncryptionKey, + SupportedKeyDerivationOptions + >; }; /** @@ -341,10 +347,13 @@ type SessionState = { }; /** - * A generic encryptor interface that supports encrypting and decrypting - * serializable data with a password. + * An encryptor interface that supports encrypting and decrypting + * serializable data with a password, and exporting and importing keys. */ -export type GenericEncryptor = { +export type ExportableKeyEncryptor< + EncryptionKey = encryptorUtils.EncryptionKey | CryptoKey, + SupportedKeyDerivationParams = encryptorUtils.KeyDerivationOptions, +> = { /** * Encrypts the given object with the given password. * @@ -373,16 +382,6 @@ export type GenericEncryptor = { vault: string, targetDerivationParams?: encryptorUtils.KeyDerivationOptions, ) => boolean; -}; - -/** - * An encryptor interface that supports encrypting and decrypting - * serializable data with a password, and exporting and importing keys. - */ -export type ExportableKeyEncryptor< - EncryptionKey = unknown, - SupportedKeyDerivationParams = unknown, -> = GenericEncryptor & { /** * Encrypts the given object with the given encryption key. * @@ -460,8 +459,6 @@ export type ExportableKeyEncryptor< password: string, salt: string, exportable?: boolean, - // setting this to unknown as currently each client has different - // key derivation options keyDerivationOptions?: SupportedKeyDerivationParams, ) => Promise; /** @@ -543,30 +540,6 @@ function assertHasUint8ArrayMnemonic( } } -/** - * Assert that the provided encryptor supports - * encryption and encryption key export. - * - * @param encryptor - The encryptor to check. - * @throws If the encryptor does not support key encryption. - */ -function assertIsExportableKeyEncryptor( - encryptor: GenericEncryptor | ExportableKeyEncryptor, -): asserts encryptor is ExportableKeyEncryptor { - if ( - !( - 'importKey' in encryptor && - typeof encryptor.importKey === 'function' && - 'decryptWithKey' in encryptor && - typeof encryptor.decryptWithKey === 'function' && - 'encryptWithKey' in encryptor && - typeof encryptor.encryptWithKey === 'function' - ) - ) { - throw new Error(KeyringControllerError.UnsupportedEncryptionKeyExport); - } -} - /** * Assert that the provided password is a valid non-empty string. * @@ -682,7 +655,10 @@ function normalize(address: string): string | undefined { * with the internal keyring controller and handling certain complex operations that involve the * keyrings. */ -export class KeyringController extends BaseController< +export class KeyringController< + EncryptionKey = encryptorUtils.EncryptionKey | CryptoKey, + SupportedKeyDerivationOptions = encryptorUtils.KeyDerivationOptions, +> extends BaseController< typeof name, KeyringControllerState, KeyringControllerMessenger @@ -693,7 +669,10 @@ export class KeyringController extends BaseController< readonly #keyringBuilders: { (): EthKeyring; type: string }[]; - readonly #encryptor: ExportableKeyEncryptor; + readonly #encryptor: ExportableKeyEncryptor< + EncryptionKey, + SupportedKeyDerivationOptions + >; #keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; @@ -710,13 +689,13 @@ export class KeyringController extends BaseController< * @param options.messenger - A restricted messenger. * @param options.state - Initial state to set on this controller. */ - constructor(options: KeyringControllerOptions) { - const { - encryptor = encryptorUtils, - keyringBuilders, - messenger, - state, - } = options; + constructor( + options: KeyringControllerOptions< + EncryptionKey, + SupportedKeyDerivationOptions + >, + ) { + const { encryptor, keyringBuilders, messenger, state } = options; super({ name, @@ -763,7 +742,6 @@ export class KeyringController extends BaseController< ? keyringBuilders.concat(defaultKeyringBuilders) : defaultKeyringBuilders; - assertIsExportableKeyEncryptor(encryptor); this.#encryptor = encryptor; this.#keyrings = []; this.#unsupportedKeyrings = []; @@ -1936,7 +1914,7 @@ export class KeyringController extends BaseController< throw new TypeError(KeyringControllerError.WrongPasswordType); } - let salt: string, keyMetadata: unknown; + let salt: string, keyMetadata: SupportedKeyDerivationOptions | undefined; if (vault && options.useVaultKeyMetadata) { const parsedVault = JSON.parse(vault); salt = parsedVault.salt; From fe9b7e4cbe877a13013f37c42e2aad8e228c2b45 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 4 Nov 2025 10:56:22 +0100 Subject: [PATCH 32/44] set `SeedlessOnboardingController` type parameters as optional --- packages/seedless-onboarding-controller/CHANGELOG.md | 3 ++- .../src/SeedlessOnboardingController.ts | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index 753950970d6..a7c9e396520 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -9,7 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **BREAKING:** `SeedlessOnboardingController` now requires an additional `SupportedKeyDerivationOptions` type parameter ([#5963](https://github.com/MetaMask/core/pull/5963)) +- `SeedlessOnboardingController` now accepts an optional `SupportedKeyDerivationOptions` type parameter ([#5963](https://github.com/MetaMask/core/pull/5963)) + - The type parameter can be used to specify which key derivation algorithms are supported by the controller instance. ## [6.0.0] diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index e5ccbb64e35..28f2eae2998 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -5,6 +5,7 @@ import type { RecoverEncryptionKeyResult, SEC1EncodedPublicKey, } from '@metamask/toprf-secure-backup'; +import type * as encryptionUtils from '@metamask/browser-passworder'; import { ToprfSecureBackup, TOPRFErrorCode, @@ -59,7 +60,6 @@ import { deserializeVaultData, serializeVaultData, } from './utils'; - const log = createModuleLogger(projectLogger, controllerName); /** @@ -226,8 +226,8 @@ const seedlessOnboardingMetadata: StateMetadata extends BaseController< typeof controllerName, SeedlessOnboardingControllerState, From dfd6a4003ca7b701cd131539488b5b9171cdcd24 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 4 Nov 2025 10:58:38 +0100 Subject: [PATCH 33/44] add CHANGELOG note on `encryptor` constructor option --- packages/keyring-controller/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 6592a4bebba..9142a5aeada 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - **BREAKING:** The `KeyringController` constructor now requires an encryptor supporting the `keyFromPassword`, `exportKey` and `generateSalt` methods ([#5963](https://github.com/MetaMask/core/pull/5963)) + - The `encryptor` constructor option was previously optional and defaulted to an instance of `@metamask/browser-passworder`. ### Removed From a1c72c9b088105a4d0fb416f30ea05e73b8be6c0 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 4 Nov 2025 11:03:39 +0100 Subject: [PATCH 34/44] fix changelog typo --- packages/keyring-controller/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 9142a5aeada..73874a6c22f 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Added optional `EncryptionKey` and `SupportedKeyDerivationOptions` type parameters to the `KeyringController, `ExportableKeyEncryptor` and `KeyringControllerOptions` type ([#5963](https://github.com/MetaMask/core/pull/5963)) +- Added optional `EncryptionKey` and `SupportedKeyDerivationOptions` type parameters to the `KeyringController`, `ExportableKeyEncryptor` and `KeyringControllerOptions` types ([#5963](https://github.com/MetaMask/core/pull/5963)) - This type parameter allows specifying the key derivation options supported by the injected encryptor, defaulting to `@metamask/browser-passworder` types. ### Changed From 7ed1598f4dfb944e6093564345e56878965f5c9a Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 4 Nov 2025 11:07:44 +0100 Subject: [PATCH 35/44] remove KeyringController encryptor test --- packages/keyring-controller/jest.config.js | 4 ++-- .../keyring-controller/src/KeyringController.test.ts | 12 +----------- .../src/SeedlessOnboardingController.ts | 1 + 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 89b32b350ef..d4a4b6c1778 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -19,8 +19,8 @@ module.exports = merge(baseConfig, { global: { branches: 93.49, functions: 100, - lines: 98.64, - statements: 98.65, + lines: 98.63, + statements: 98.64, }, }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index f4548896028..e5300595ff0 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -101,21 +101,11 @@ describe('KeyringController', () => { () => new KeyringController({ messenger: buildKeyringControllerMessenger(), + encryptor: new MockEncryptor(), }), ).not.toThrow(); }); - it('should throw error if encryptor does not support key export', () => { - expect( - () => - new KeyringController({ - messenger: buildKeyringControllerMessenger(), - // @ts-expect-error testing an invalid encryptor - encryptor: { encrypt: jest.fn(), decrypt: jest.fn() }, - }), - ).toThrow(KeyringControllerError.UnsupportedEncryptionKeyExport); - }); - it('allows overwriting the built-in Simple keyring builder', async () => { const mockSimpleKeyringBuilder = // todo: keyring types are mismatched, this should be fixed in they keyrings themselves diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 28f2eae2998..31074fecb3c 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -60,6 +60,7 @@ import { deserializeVaultData, serializeVaultData, } from './utils'; + const log = createModuleLogger(projectLogger, controllerName); /** From 842f6030eb0ef3ae53dc0f3b0b766c7ecbcca5ff Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 4 Nov 2025 11:17:57 +0100 Subject: [PATCH 36/44] fix `SeedlessOnboardingController` lint --- .../src/SeedlessOnboardingController.test.ts | 2 +- .../src/SeedlessOnboardingController.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 3ec55f5d7b5..fa45428ed6c 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -438,7 +438,7 @@ function mockChangeEncKey( * @param newPassword - The new password. */ async function mockChangePassword( - controller: SeedlessOnboardingController, + controller: SeedlessOnboardingController, toprfClient: ToprfSecureBackup, oldPassword: string, newPassword: string, diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 31074fecb3c..06cae4bb098 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -1,11 +1,11 @@ import { keccak256AndHexify } from '@metamask/auth-network-utils'; import { BaseController, type StateMetadata } from '@metamask/base-controller'; +import type * as encryptionUtils from '@metamask/browser-passworder'; import type { KeyPair, RecoverEncryptionKeyResult, SEC1EncodedPublicKey, } from '@metamask/toprf-secure-backup'; -import type * as encryptionUtils from '@metamask/browser-passworder'; import { ToprfSecureBackup, TOPRFErrorCode, From 4419d7e4ba492da65a1d65ca952856052e68cb9c Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 4 Nov 2025 15:10:34 +0100 Subject: [PATCH 37/44] bump `@metamask/browser-passworder` from `^4.3.0` to `^6.0.0` --- packages/keyring-controller/package.json | 2 +- .../package.json | 2 +- yarn.lock | 33 +++++-------------- 3 files changed, 10 insertions(+), 27 deletions(-) diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index f2a601569ac..f22ca838af8 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -49,7 +49,6 @@ "dependencies": { "@ethereumjs/util": "^9.1.0", "@metamask/base-controller": "^9.0.0", - "@metamask/browser-passworder": "^4.3.0", "@metamask/eth-hd-keyring": "^13.0.0", "@metamask/eth-sig-util": "^8.2.0", "@metamask/eth-simple-keyring": "^11.0.0", @@ -69,6 +68,7 @@ "@lavamoat/allow-scripts": "^3.0.4", "@lavamoat/preinstall-always-fail": "^2.1.0", "@metamask/auto-changelog": "^3.4.4", + "@metamask/browser-passworder": "^6.0.0", "@metamask/keyring-utils": "^3.1.0", "@metamask/scure-bip39": "^2.1.1", "@types/jest": "^27.4.1", diff --git a/packages/seedless-onboarding-controller/package.json b/packages/seedless-onboarding-controller/package.json index 8dfb5027c95..ce63dd21e7a 100644 --- a/packages/seedless-onboarding-controller/package.json +++ b/packages/seedless-onboarding-controller/package.json @@ -61,7 +61,7 @@ "@lavamoat/allow-scripts": "^3.0.4", "@lavamoat/preinstall-always-fail": "^2.1.0", "@metamask/auto-changelog": "^3.4.4", - "@metamask/browser-passworder": "^4.3.0", + "@metamask/browser-passworder": "^6.0.0", "@metamask/keyring-controller": "^24.0.0", "@types/elliptic": "^6", "@types/jest": "^27.4.1", diff --git a/yarn.lock b/yarn.lock index 700bcfaeb58..204507cba13 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3023,12 +3023,12 @@ __metadata: languageName: unknown linkType: soft -"@metamask/browser-passworder@npm:^4.3.0": - version: 4.3.0 - resolution: "@metamask/browser-passworder@npm:4.3.0" +"@metamask/browser-passworder@npm:^6.0.0": + version: 6.0.0 + resolution: "@metamask/browser-passworder@npm:6.0.0" dependencies: - "@metamask/utils": "npm:^8.2.0" - checksum: 10/8ba5c50cd6274b0cc0f90a1ee16b960ee150f14c29083f3515f4abe018a28ead32c21f5f4a62a6e27a946b1228adc2ff1f195e71e38782fa39fa8fff116173e6 + "@metamask/utils": "npm:^11.0.1" + checksum: 10/44b31729ccd52e62df27a948fbea2320a11861d6465128b1626a723fc47e25f9e3a8c3bdc09329f3f270b5d9e023107cb439cfb6961a3614c6daa58ea4524f00 languageName: node linkType: hard @@ -3972,7 +3972,7 @@ __metadata: "@lavamoat/preinstall-always-fail": "npm:^2.1.0" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^9.0.0" - "@metamask/browser-passworder": "npm:^4.3.0" + "@metamask/browser-passworder": "npm:^6.0.0" "@metamask/eth-hd-keyring": "npm:^13.0.0" "@metamask/eth-sig-util": "npm:^8.2.0" "@metamask/eth-simple-keyring": "npm:^11.0.0" @@ -4774,7 +4774,7 @@ __metadata: "@metamask/auth-network-utils": "npm:^0.3.0" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^9.0.0" - "@metamask/browser-passworder": "npm:^4.3.0" + "@metamask/browser-passworder": "npm:^6.0.0" "@metamask/keyring-controller": "npm:^24.0.0" "@metamask/messenger": "npm:^0.3.0" "@metamask/toprf-secure-backup": "npm:^0.10.0" @@ -5048,7 +5048,7 @@ __metadata: languageName: unknown linkType: soft -"@metamask/superstruct@npm:^3.0.0, @metamask/superstruct@npm:^3.1.0, @metamask/superstruct@npm:^3.2.1": +"@metamask/superstruct@npm:^3.1.0, @metamask/superstruct@npm:^3.2.1": version: 3.2.1 resolution: "@metamask/superstruct@npm:3.2.1" checksum: 10/9e29380f2cf8b129283ccb2b568296d92682b705109ba62dbd7739ffd6a1982fe38c7228cdcf3cbee94dbcdd5fcc1c846ab9d1dd3582167154f914422fcff547 @@ -5259,23 +5259,6 @@ __metadata: languageName: node linkType: hard -"@metamask/utils@npm:^8.2.0": - version: 8.5.0 - resolution: "@metamask/utils@npm:8.5.0" - dependencies: - "@ethereumjs/tx": "npm:^4.2.0" - "@metamask/superstruct": "npm:^3.0.0" - "@noble/hashes": "npm:^1.3.1" - "@scure/base": "npm:^1.1.3" - "@types/debug": "npm:^4.1.7" - debug: "npm:^4.3.4" - pony-cause: "npm:^2.1.10" - semver: "npm:^7.5.4" - uuid: "npm:^9.0.1" - checksum: 10/68a42a55f7dc750b75467fb7c05a496c20dac073a2753e0f4d9642c4d8dcb3f9ddf51a09d30337e11637f1777f3dfe22e15b5159dbafb0fdb7bd8c9236056153 - languageName: node - linkType: hard - "@metamask/utils@npm:^9.0.0": version: 9.3.0 resolution: "@metamask/utils@npm:9.3.0" From 607c80f848f21fe6c73b443dbeb7bfdd58a9e582 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 4 Nov 2025 15:38:04 +0100 Subject: [PATCH 38/44] rename `ExportableKeyEncryptor` to `Encryptor` --- packages/keyring-controller/CHANGELOG.md | 4 ++-- packages/keyring-controller/src/KeyringController.ts | 12 +++--------- packages/seedless-onboarding-controller/src/types.ts | 4 ++-- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 73874a6c22f..74b574cfd61 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -14,14 +14,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **BREAKING:** The `KeyringController` constructor now requires an encryptor supporting the `keyFromPassword`, `exportKey` and `generateSalt` methods ([#5963](https://github.com/MetaMask/core/pull/5963)) +- **BREAKING:** The `KeyringController` constructor options now require an encryptor ([#5963](https://github.com/MetaMask/core/pull/5963)) - The `encryptor` constructor option was previously optional and defaulted to an instance of `@metamask/browser-passworder`. +- **BREAKING:** The `GenericEncryptor` and `ExportableKeyEncryptor` types have been merged into a single `Encryptor` type ([#5963](https://github.com/MetaMask/core/pull/5963)) ### Removed - **BREAKING:** The `cacheEncryptionKey` parameter has been removed from the `KeyringController` constructor options ([#5963](https://github.com/MetaMask/core/pull/5963)) - This parameter was previously used to enable encryption key in-memory caching, but it is no longer needed as the controller now always uses the latest encryption key. -- **BREAKING:** The `GenericEncryptor` type has been removed ([#5963](https://github.com/MetaMask/core/pull/5963)) ### Fixed diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 22e5c5874e4..a0d0634a3a6 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -258,10 +258,7 @@ export type KeyringControllerOptions< keyringBuilders?: { (): EthKeyring; type: string }[]; messenger: KeyringControllerMessenger; state?: { vault?: string; keyringsMetadata?: KeyringMetadata[] }; - encryptor: ExportableKeyEncryptor< - EncryptionKey, - SupportedKeyDerivationOptions - >; + encryptor: Encryptor; }; /** @@ -350,7 +347,7 @@ type SessionState = { * An encryptor interface that supports encrypting and decrypting * serializable data with a password, and exporting and importing keys. */ -export type ExportableKeyEncryptor< +export type Encryptor< EncryptionKey = encryptorUtils.EncryptionKey | CryptoKey, SupportedKeyDerivationParams = encryptorUtils.KeyDerivationOptions, > = { @@ -669,10 +666,7 @@ export class KeyringController< readonly #keyringBuilders: { (): EthKeyring; type: string }[]; - readonly #encryptor: ExportableKeyEncryptor< - EncryptionKey, - SupportedKeyDerivationOptions - >; + readonly #encryptor: Encryptor; #keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; diff --git a/packages/seedless-onboarding-controller/src/types.ts b/packages/seedless-onboarding-controller/src/types.ts index 59c1f6257cb..a091fc396b3 100644 --- a/packages/seedless-onboarding-controller/src/types.ts +++ b/packages/seedless-onboarding-controller/src/types.ts @@ -2,7 +2,7 @@ import type { ControllerGetStateAction, ControllerStateChangeEvent, } from '@metamask/base-controller'; -import type { ExportableKeyEncryptor } from '@metamask/keyring-controller'; +import type { Encryptor } from '@metamask/keyring-controller'; import type { Messenger } from '@metamask/messenger'; import type { KeyPair, NodeAuthTokens } from '@metamask/toprf-secure-backup'; import type { MutexInterface } from 'async-mutex'; @@ -227,7 +227,7 @@ export type SeedlessOnboardingControllerMessenger = Messenger< * Encryptor interface for encrypting and decrypting seedless onboarding vault. */ export type VaultEncryptor = Omit< - ExportableKeyEncryptor, + Encryptor, 'encryptWithKey' >; From 317c0a9f06bf501976546031aefbffc24a3708cc Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 6 Nov 2025 00:31:50 +0100 Subject: [PATCH 39/44] use `decryptWithDetail` instead of `keyFromPassword` for existing vault --- .../src/KeyringController.ts | 20 +++++++++++-------- .../tests/mocks/mockEncryptor.ts | 4 ++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index a0d0634a3a6..89f3fab1382 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1908,19 +1908,23 @@ export class KeyringController< throw new TypeError(KeyringControllerError.WrongPasswordType); } - let salt: string, keyMetadata: SupportedKeyDerivationOptions | undefined; + let serializedEncryptionKey: string, salt: string; if (vault && options.useVaultKeyMetadata) { - const parsedVault = JSON.parse(vault); - salt = parsedVault.salt; - keyMetadata = parsedVault.keyMetadata; + // The `decryptWithDetail` method is being used here instead of + // `keyFromPassword` + `exportKey` to let the encryptor handle + // any legacy encryption formats and metadata that might be + // present (or absent) in the vault. + const { exportedKeyString, salt: existingSalt } = + await this.#encryptor.decryptWithDetail(password, vault); + serializedEncryptionKey = exportedKeyString; + salt = existingSalt; } else { salt = this.#encryptor.generateSalt(); + serializedEncryptionKey = await this.#encryptor.exportKey( + await this.#encryptor.keyFromPassword(password, salt, true), + ); } - const serializedEncryptionKey = await this.#encryptor.exportKey( - await this.#encryptor.keyFromPassword(password, salt, true, keyMetadata), - ); - this.#encryptionKey = { salt, serialized: serializedEncryptionKey, diff --git a/packages/keyring-controller/tests/mocks/mockEncryptor.ts b/packages/keyring-controller/tests/mocks/mockEncryptor.ts index f5f20e3b487..5c931ec18eb 100644 --- a/packages/keyring-controller/tests/mocks/mockEncryptor.ts +++ b/packages/keyring-controller/tests/mocks/mockEncryptor.ts @@ -9,7 +9,7 @@ import type { import type { Json } from '@metamask/utils'; import { isEqual } from 'lodash'; -import type { ExportableKeyEncryptor } from '../../src/KeyringController'; +import type { Encryptor } from '../../src/KeyringController'; export const PASSWORD = 'password123'; export const SALT = 'salt'; @@ -28,7 +28,7 @@ function deriveKey(password: string, salt: string) { }; } -export default class MockEncryptor implements ExportableKeyEncryptor { +export default class MockEncryptor implements Encryptor { async encrypt(password: string, dataObj: Json): Promise { const salt = this.generateSalt(); const key = deriveKey(password, salt); From e641b371118fb7692b7917e185c90078799224fb Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 6 Nov 2025 01:14:56 +0100 Subject: [PATCH 40/44] skip metadata and salt when changing password --- packages/keyring-controller/src/KeyringController.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 89f3fab1382..d63c455a27b 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1451,7 +1451,9 @@ export class KeyringController< return this.#persistOrRollback(async () => { assertIsValidPassword(password); - await this.#deriveEncryptionKey(password); + await this.#deriveEncryptionKey(password, { + useVaultKeyMetadata: false, + }); }); } From 2e88eaeaca5a289be104f49e1aa7bfa2db6a41d0 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 6 Nov 2025 01:16:30 +0100 Subject: [PATCH 41/44] add `createVault()` test helper function --- .../src/KeyringController.test.ts | 387 ++++++++++-------- 1 file changed, 213 insertions(+), 174 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index e5300595ff0..d72fd55f7db 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -33,6 +33,7 @@ import type { KeyringControllerOptions, KeyringControllerActions, KeyringMetadata, + SerializedKeyring, } from './KeyringController'; import { AccountImportStrategy, @@ -84,11 +85,49 @@ const uint8ArraySeed = new Uint8Array( const privateKey = '1e4e6a4c0c077f4ae8ddfbf372918e61dd0fb4a4cfa592cb16e7546d505e68fc'; const password = 'password123'; -const freshVault = - '{"data":"{\\"tag\\":{\\"key\\":{\\"password\\":\\"password123\\",\\"salt\\":\\"salt\\"},\\"iv\\":\\"iv\\"},\\"value\\":[{\\"type\\":\\"HD Key Tree\\",\\"data\\":{\\"mnemonic\\":[119,97,114,114,105,111,114,32,108,97,110,103,117,97,103,101,32,106,111,107,101,32,98,111,110,117,115,32,117,110,102,97,105,114,32,97,114,116,105,115,116,32,107,97,110,103,97,114,111,111,32,99,105,114,99,108,101,32,101,120,112,97,110,100,32,104,111,112,101,32,109,105,100,100,108,101,32,103,97,117,103,101],\\"numberOfAccounts\\":1,\\"hdPath\\":\\"m/44\'/60\'/0\'/0\\"},\\"metadata\\":{\\"id\\":\\"01JXEFM7DAX2VJ0YFR4ESNY3GQ\\",\\"name\\":\\"\\"}}]}","iv":"iv","salt":"salt"}'; const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin }; +const defaultKeyrings: SerializedKeyring[] = [ + { + type: 'HD Key Tree', + data: { + mnemonic: [ + 119, 97, 114, 114, 105, 111, 114, 32, 108, 97, 110, 103, 117, 97, 103, + 101, 32, 106, 111, 107, 101, 32, 98, 111, 110, 117, 115, 32, 117, 110, + 102, 97, 105, 114, 32, 97, 114, 116, 105, 115, 116, 32, 107, 97, 110, + 103, 97, 114, 111, 111, 32, 99, 105, 114, 99, 108, 101, 32, 101, 120, + 112, 97, 110, 100, 32, 104, 111, 112, 101, 32, 109, 105, 100, 100, 108, + 101, 32, 103, 97, 117, 103, 101, + ], + numberOfAccounts: 1, + hdPath: "m/44'/60'/0'/0", + }, + metadata: { id: '01JXEFM7DAX2VJ0YFR4ESNY3GQ', name: '' }, + }, +]; + +const defaultCredentials = { password, salt: 'salt' }; + +/** + * Build a vault string with the given keyrings. + * This vault can be used with the MockEncryptor to test KeyringController + * with controlled keyrings. + * + * @param keyrings - The keyrings to include in the vault. + * @returns The vault string. + */ +function createVault(keyrings: SerializedKeyring[] = defaultKeyrings): string { + return JSON.stringify({ + data: JSON.stringify({ + tag: { key: defaultCredentials, iv: 'iv' }, + value: keyrings, + }), + iv: 'iv', + salt: 'salt', + }); +} + describe('KeyringController', () => { afterEach(() => { sinon.restore(); @@ -138,28 +177,26 @@ describe('KeyringController', () => { { skipVaultCreation: true, state: { - vault: freshVault, + vault: createVault([ + { + type: KeyringTypes.hd, + data: '', + metadata: { id: 'hd', name: '' }, + }, + { + type: 'Unsupported', + data: '', + metadata: { id: 'unsupported', name: '' }, + }, + { + type: KeyringTypes.hd, + data: '', + metadata: { id: 'hd2', name: '' }, + }, + ]), }, }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: '', - metadata: { id: 'hd', name: '' }, - }, - { - type: 'Unsupported', - data: '', - metadata: { id: 'unsupported', name: '' }, - }, - { - type: KeyringTypes.hd, - data: '', - metadata: { id: 'hd2', name: '' }, - }, - ]); - + async ({ controller }) => { await controller.submitPassword(password); expect(controller.state.keyrings).toHaveLength(2); @@ -182,29 +219,27 @@ describe('KeyringController', () => { { skipVaultCreation: true, state: { - vault: freshVault, + vault: createVault([ + { + type: 'HD Key Tree', + data: '', + metadata: { id: 'hd', name: '' }, + }, + { + type: 'HD Key Tree', + data: '', + metadata: { id: 'hd2', name: '' }, + }, + // This keyring was already unsupported + // (no metadata, and is at the end of the array) + { + type: MockKeyring.type, + data: 'unsupported', + }, + ]), }, }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: 'HD Key Tree', - data: '', - metadata: { id: 'hd', name: '' }, - }, - { - type: 'HD Key Tree', - data: '', - metadata: { id: 'hd2', name: '' }, - }, - // This keyring was already unsupported - // (no metadata, and is at the end of the array) - { - type: MockKeyring.type, - data: 'unsupported', - }, - ]); - + async ({ controller }) => { await controller.submitPassword(password); expect(controller.state.keyrings).toHaveLength(2); @@ -288,12 +323,12 @@ describe('KeyringController', () => { it('should throw an error if there is no primary keyring', async () => { await withController( - { skipVaultCreation: true, state: { vault: freshVault } }, - async ({ controller, encryptor }) => { - jest - .spyOn(encryptor, 'decryptWithKey') - .mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]); - await controller.submitPassword('123'); + { + skipVaultCreation: true, + state: { vault: createVault([{ type: 'Unsupported', data: '' }]) }, + }, + async ({ controller }) => { + await controller.submitPassword(password); await expect(controller.addNewAccount()).rejects.toThrow( 'No HD keyring found', @@ -1122,12 +1157,12 @@ describe('KeyringController', () => { it('should throw an error if there is no keyring', async () => { await withController( - { skipVaultCreation: true, state: { vault: freshVault } }, - async ({ controller, encryptor }) => { - jest - .spyOn(encryptor, 'decryptWithKey') - .mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]); - await controller.submitPassword('123'); + { + skipVaultCreation: true, + state: { vault: createVault([{ type: 'Unsupported', data: '' }]) }, + }, + async ({ controller }) => { + await controller.submitPassword(password); await expect( controller.getKeyringForAccount( @@ -2538,7 +2573,6 @@ describe('KeyringController', () => { newPassword, controller.state.encryptionSalt, true, - undefined, ); }); }); @@ -2602,16 +2636,16 @@ describe('KeyringController', () => { await withController( { skipVaultCreation: true, - state: { vault: freshVault }, + state: { + vault: createVault([ + { + type: 'UnsupportedKeyring', + data: '0x1234', + }, + ]), + }, }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: 'UnsupportedKeyring', - data: '0x1234', - }, - ]); - + async ({ controller }) => { await controller.submitPassword(password); expect(controller.state.isUnlocked).toBe(true); @@ -2623,15 +2657,16 @@ describe('KeyringController', () => { await withController( { skipVaultCreation: true, - state: { vault: freshVault }, + state: { + vault: createVault([ + { + // @ts-expect-error we are testing invalid vault shape + foo: 'bar', + }, + ]), + }, }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - foo: 'bar', - }, - ]); - + async ({ controller }) => { await expect(controller.submitPassword(password)).rejects.toThrow( KeyringControllerError.VaultDataError, ); @@ -2654,23 +2689,23 @@ describe('KeyringController', () => { stubKeyringClassWithAccount(HdKeyring, '0x123'); await withController( { - state: { vault: freshVault }, + state: { + vault: createVault([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: '123', + name: '', + }, + }, + ]), + }, skipVaultCreation: true, }, - async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - metadata: { - id: '123', - name: '', - }, - }, - ]); - + async ({ controller }) => { await controller.submitPassword(password); expect(controller.state.keyrings).toStrictEqual([ @@ -2688,6 +2723,14 @@ describe('KeyringController', () => { }); it('should generate new metadata when there is no metadata in the vault', async () => { + const vault = createVault([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + }, + ]); const hdKeyringSerializeSpy = jest.spyOn( HdKeyring.prototype, 'serialize', @@ -2695,21 +2738,12 @@ describe('KeyringController', () => { await withController( { state: { - vault: freshVault, + vault, }, skipVaultCreation: true, }, async ({ controller, encryptor }) => { const encryptWithKeySpy = jest.spyOn(encryptor, 'encryptWithKey'); - jest.spyOn(encryptor, 'importKey').mockResolvedValue('imported key'); - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]); hdKeyringSerializeSpy.mockResolvedValue({ // @ts-expect-error we are assigning a mock value accounts: ['0x123'], @@ -2727,7 +2761,7 @@ describe('KeyringController', () => { }, }, ]); - expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [ + expect(encryptWithKeySpy).toHaveBeenCalledWith(defaultCredentials, [ { type: KeyringTypes.hd, data: { @@ -2749,23 +2783,24 @@ describe('KeyringController', () => { await withController( { skipVaultCreation: true, - state: { vault: freshVault }, + state: { + vault: createVault([ + { + type: KeyringTypes.hd, + data: {}, + }, + { + type: MockKeyring.type, + data: {}, + }, + ]), + }, keyringBuilders: [keyringBuilderFactory(MockKeyring)], }, async ({ controller, encryptor, messenger }) => { const unlockListener = jest.fn(); messenger.subscribe('KeyringController:unlock', unlockListener); jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: {}, - }, - { - type: MockKeyring.type, - data: {}, - }, - ]); await controller.submitPassword(password); @@ -2779,21 +2814,22 @@ describe('KeyringController', () => { await withController( { skipVaultCreation: true, - state: { vault: freshVault }, + state: { + vault: createVault([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + }, + ]), + }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); jest .spyOn(encryptor, 'encryptWithKey') .mockRejectedValue(new Error()); - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]); await controller.submitPassword(password); @@ -2808,22 +2844,23 @@ describe('KeyringController', () => { await withController( { skipVaultCreation: true, - state: { vault: freshVault }, + state: { + vault: createVault([ + { + type: KeyringTypes.hd, + data: {}, + }, + { + type: MockKeyring.type, + data: {}, + }, + ]), + }, keyringBuilders: [keyringBuilderFactory(MockKeyring)], }, - async ({ controller, encryptor, messenger }) => { + async ({ controller, messenger }) => { const unlockListener = jest.fn(); messenger.subscribe('KeyringController:unlock', unlockListener); - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: {}, - }, - { - type: MockKeyring.type, - data: {}, - }, - ]); await controller.submitPassword(password); @@ -2837,21 +2874,22 @@ describe('KeyringController', () => { await withController( { skipVaultCreation: true, - state: { vault: freshVault }, + state: { + vault: createVault([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + }, + ]), + }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); jest.spyOn(encryptor, 'generateSalt').mockReturnValue('new salt'); const keyFromPasswordSpy = jest.spyOn(encryptor, 'keyFromPassword'); const encryptSpy = jest.spyOn(encryptor, 'encryptWithKey'); - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]); await controller.submitPassword(password); @@ -2859,7 +2897,6 @@ describe('KeyringController', () => { password, 'new salt', true, - undefined, ); expect(encryptSpy).toHaveBeenCalledTimes(1); }, @@ -2870,19 +2907,20 @@ describe('KeyringController', () => { await withController( { skipVaultCreation: true, - state: { vault: freshVault }, + state: { + vault: createVault([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + }, + ]), + }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]); await controller.submitPassword(password); @@ -2895,23 +2933,24 @@ describe('KeyringController', () => { await withController( { skipVaultCreation: true, - state: { vault: freshVault }, + state: { + vault: createVault([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: '123', + name: '', + }, + }, + ]), + }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - metadata: { - id: '123', - name: '', - }, - }, - ]); await controller.submitPassword(password); @@ -2944,7 +2983,7 @@ describe('KeyringController', () => { await withController( { skipVaultCreation: true, - state: { vault: freshVault }, + state: { vault: createVault() }, }, async ({ controller, encryptor }) => { jest @@ -3235,12 +3274,12 @@ describe('KeyringController', () => { it('should throw an error if there is no primary keyring', async () => { await withController( - { skipVaultCreation: true, state: { vault: freshVault } }, - async ({ controller, encryptor }) => { - jest - .spyOn(encryptor, 'decryptWithKey') - .mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]); - await controller.submitPassword('123'); + { + skipVaultCreation: true, + state: { vault: createVault([{ type: 'Unsupported', data: '' }]) }, + }, + async ({ controller }) => { + await controller.submitPassword(password); await expect(controller.verifySeedPhrase()).rejects.toThrow( KeyringControllerError.KeyringNotFound, @@ -4079,7 +4118,7 @@ describe('KeyringController', () => { it('includes expected state in debug snapshots', async () => { await withController( // Skip vault creation and use static vault to get deterministic state snapshot - { skipVaultCreation: true, state: { vault: freshVault } }, + { skipVaultCreation: true, state: { vault: createVault() } }, ({ controller }) => { expect( deriveStateFromMetadata( @@ -4099,7 +4138,7 @@ describe('KeyringController', () => { it('includes expected state in state logs', async () => { await withController( // Skip vault creation and use static vault to get deterministic state snapshot - { skipVaultCreation: true, state: { vault: freshVault } }, + { skipVaultCreation: true, state: { vault: createVault() } }, ({ controller }) => { expect( deriveStateFromMetadata( @@ -4120,7 +4159,7 @@ describe('KeyringController', () => { it('persists expected state', async () => { await withController( // Skip vault creation and use static vault to get deterministic state snapshot - { skipVaultCreation: true, state: { vault: freshVault } }, + { skipVaultCreation: true, state: { vault: createVault() } }, ({ controller }) => { expect( deriveStateFromMetadata( @@ -4140,7 +4179,7 @@ describe('KeyringController', () => { it('exposes expected state to UI', async () => { await withController( // Skip vault creation and use static vault to get deterministic state snapshot - { skipVaultCreation: true, state: { vault: freshVault } }, + { skipVaultCreation: true, state: { vault: createVault() } }, ({ controller }) => { expect( deriveStateFromMetadata( From ae32c9487f4edc902bbe8a141b0746c044e8681a Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 6 Nov 2025 01:26:15 +0100 Subject: [PATCH 42/44] bump jest threshold --- packages/keyring-controller/jest.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index d4a4b6c1778..c36bbb28363 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,7 +17,7 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 93.49, + branches: 94.52, functions: 100, lines: 98.63, statements: 98.64, From 21f1c91c92983b21c142af21f46fcde6bc9ec882 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 6 Nov 2025 02:25:27 +0100 Subject: [PATCH 43/44] fix: ignore existing vault when recreating first keyring --- .../src/KeyringController.test.ts | 17 ++++++++++++++++ .../src/KeyringController.ts | 20 ++++++++++++------- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index d72fd55f7db..68e6696599e 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -599,6 +599,23 @@ describe('KeyringController', () => { }); }); + it('should create new vault with a different password', async () => { + await withController(async ({ controller, initialState }) => { + const initialKeyrings = controller.state.keyrings; + + await controller.createNewVaultAndRestore( + 'new-password', + uint8ArraySeed, + ); + + expect(controller.state).not.toBe(initialState); + expect(controller.state.vault).toBeDefined(); + expect(controller.state.keyrings).toHaveLength(initialKeyrings.length); + // new keyring metadata should be generated + expect(controller.state.keyrings).not.toStrictEqual(initialKeyrings); + }); + }); + it('should call encryptor.encryptWithKey with the same keyrings if old seedWord is used', async () => { await withController(async ({ controller, encryptor }) => { const encryptSpy = jest.spyOn(encryptor, 'encryptWithKey'); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index d63c455a27b..4f093ebb313 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1452,7 +1452,7 @@ export class KeyringController< return this.#persistOrRollback(async () => { assertIsValidPassword(password); await this.#deriveEncryptionKey(password, { - useVaultKeyMetadata: false, + ignoreExistingVault: true, }); }); } @@ -1532,7 +1532,7 @@ export class KeyringController< // If the vault is being upgraded, we want to ignore the metadata // that is already in the vault, so we can effectively // re-encrypt the vault with the new encryption config. - useVaultKeyMetadata: false, + ignoreExistingVault: true, }); await this.#updateVault(); } @@ -1877,7 +1877,9 @@ export class KeyringController< delete state.encryptionSalt; }); - await this.#deriveEncryptionKey(password); + await this.#deriveEncryptionKey(password, { + ignoreExistingVault: true, + }); await this.#clearKeyrings(); await this.#createKeyringWithFirstAccount(keyring.type, keyring.opts); @@ -1893,14 +1895,18 @@ export class KeyringController< * using the salt from the vault. If the vault is empty, a new salt * is generated and used to derive the key. * + * If `options.ignoreExistingVault` is set to `false`, the existing + * vault is completely ignored: the new key won't be able to decrypt + * the existing vault, and should be used to re-encrypt it. + * * @param password - The password to use for decryption or derivation. * @param options - Options for the key derivation. - * @param options.useVaultKeyMetadata - Whether to use the vault key metadata + * @param options.ignoreExistingVault - Whether to use the existing vault salt and key metadata */ async #deriveEncryptionKey( password: string, - options: { useVaultKeyMetadata: boolean } = { - useVaultKeyMetadata: true, + options: { ignoreExistingVault: boolean } = { + ignoreExistingVault: false, }, ): Promise { this.#assertControllerMutexIsLocked(); @@ -1911,7 +1917,7 @@ export class KeyringController< } let serializedEncryptionKey: string, salt: string; - if (vault && options.useVaultKeyMetadata) { + if (vault && !options.ignoreExistingVault) { // The `decryptWithDetail` method is being used here instead of // `keyFromPassword` + `exportKey` to let the encryptor handle // any legacy encryption formats and metadata that might be From e958bbade6ad74303422023fcf8b85daa573d91b Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 6 Nov 2025 11:46:21 +0100 Subject: [PATCH 44/44] add type parameter for `EncryptionResult` --- packages/keyring-controller/CHANGELOG.md | 4 +- .../src/KeyringController.ts | 37 ++++++++++++++++--- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 74b574cfd61..951992dc02b 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -9,8 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Added optional `EncryptionKey` and `SupportedKeyDerivationOptions` type parameters to the `KeyringController`, `ExportableKeyEncryptor` and `KeyringControllerOptions` types ([#5963](https://github.com/MetaMask/core/pull/5963)) - - This type parameter allows specifying the key derivation options supported by the injected encryptor, defaulting to `@metamask/browser-passworder` types. +- Added optional `EncryptionKey`, `SupportedKeyDerivationOptions` and `EncryptionResult` type parameters to the `KeyringController`, `ExportableKeyEncryptor` and `KeyringControllerOptions` types ([#5963](https://github.com/MetaMask/core/pull/5963)) + - This type parameter allows specifying the encryption key, key derivation options and encryption result types supported by the injected encryptor, defaulting to `@metamask/browser-passworder` types. ### Changed diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 4f093ebb313..dc91522664f 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -254,11 +254,17 @@ export type KeyringControllerMessenger = Messenger< export type KeyringControllerOptions< EncryptionKey = encryptorUtils.EncryptionKey | CryptoKey, SupportedKeyDerivationOptions = encryptorUtils.KeyDerivationOptions, + EncryptionResult extends + EncryptionResultConstraint = DefaultEncryptionResult, > = { keyringBuilders?: { (): EthKeyring; type: string }[]; messenger: KeyringControllerMessenger; state?: { vault?: string; keyringsMetadata?: KeyringMetadata[] }; - encryptor: Encryptor; + encryptor: Encryptor< + EncryptionKey, + SupportedKeyDerivationOptions, + EncryptionResult + >; }; /** @@ -343,6 +349,18 @@ type SessionState = { encryptionKey?: CachedEncryptionKey; }; +export type EncryptionResultConstraint = { + salt?: string; + keyMetadata?: SupportedKeyMetadata; +}; + +export type DefaultEncryptionResult = { + data: string; + iv: string; + salt?: string; + keyMetadata?: SupportedKeyMetadata; +}; + /** * An encryptor interface that supports encrypting and decrypting * serializable data with a password, and exporting and importing keys. @@ -350,6 +368,8 @@ type SessionState = { export type Encryptor< EncryptionKey = encryptorUtils.EncryptionKey | CryptoKey, SupportedKeyDerivationParams = encryptorUtils.KeyDerivationOptions, + EncryptionResult extends + EncryptionResultConstraint = DefaultEncryptionResult, > = { /** * Encrypts the given object with the given password. @@ -389,7 +409,7 @@ export type Encryptor< encryptWithKey: ( key: EncryptionKey, object: Json, - ) => Promise; + ) => Promise; /** * Encrypts the given object with the given password, and returns the * encryption result and the serialized key string. @@ -413,7 +433,7 @@ export type Encryptor< */ decryptWithKey: ( key: EncryptionKey, - encryptedObject: encryptorUtils.EncryptionResult, + encryptedObject: EncryptionResult, ) => Promise; /** * Decrypts the given encrypted string with the given password, and returns @@ -655,6 +675,8 @@ function normalize(address: string): string | undefined { export class KeyringController< EncryptionKey = encryptorUtils.EncryptionKey | CryptoKey, SupportedKeyDerivationOptions = encryptorUtils.KeyDerivationOptions, + EncryptionResult extends + EncryptionResultConstraint = DefaultEncryptionResult, > extends BaseController< typeof name, KeyringControllerState, @@ -666,7 +688,11 @@ export class KeyringController< readonly #keyringBuilders: { (): EthKeyring; type: string }[]; - readonly #encryptor: Encryptor; + readonly #encryptor: Encryptor< + EncryptionKey, + SupportedKeyDerivationOptions, + EncryptionResult + >; #keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; @@ -686,7 +712,8 @@ export class KeyringController< constructor( options: KeyringControllerOptions< EncryptionKey, - SupportedKeyDerivationOptions + SupportedKeyDerivationOptions, + EncryptionResult >, ) { const { encryptor, keyringBuilders, messenger, state } = options;