From f82df2606c82ca735e2c156ce201b9b350fdb09c Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 17 Oct 2024 17:05:34 +0200 Subject: [PATCH 1/3] perf: Stop validating JSON for encrypted state --- packages/snaps-controllers/src/snaps/SnapController.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index a29da71243..2de3d1dd2d 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1759,7 +1759,9 @@ export class SnapController extends BaseController< */ async #decryptSnapState(snapId: SnapId, state: string) { try { - const parsed = parseJson(state); + // We assume that the state string here is valid JSON since we control serialization. + // This lets us skip JSON validation. + const parsed = JSON.parse(state) as EncryptionResult; const { salt, keyMetadata } = parsed; const useCache = this.#encryptor.isVaultUpdated(state); const { key } = await this.#getSnapEncryptionKey({ From dff5de7268878e5e3f55dbe9cc2f0da45114b32c Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Fri, 18 Oct 2024 11:08:50 +0200 Subject: [PATCH 2/3] Reduce JSON validation overhead when using manageState further --- packages/snaps-controllers/src/snaps/SnapController.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 2de3d1dd2d..576aa4d5d7 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -87,7 +87,6 @@ import { NpmSnapFileNames, OnNameLookupResponseStruct, getLocalizedSnapManifest, - parseJson, MAX_FILE_SIZE, } from '@metamask/snaps-utils'; import type { Json, NonEmptyArray, SemVerRange } from '@metamask/utils'; @@ -101,7 +100,6 @@ import { hasProperty, inMilliseconds, isNonEmptyArray, - isValidJson, isValidSemVerRange, satisfiesVersionRange, timeSince, @@ -1774,8 +1772,7 @@ export class SnapController extends BaseController< }); const decryptedState = await this.#encryptor.decryptWithKey(key, parsed); - assert(isValidJson(decryptedState)); - + // We assume this to be valid JSON, since all RPC requests from a Snap are validated and sanitized. return decryptedState as Record; } catch { throw rpcErrors.internal({ @@ -1866,7 +1863,8 @@ export class SnapController extends BaseController< } if (!encrypted) { - return parseJson(state); + // For performance reasons, we do not validate that the state is JSON, since we control serialization. + return JSON.parse(state); } const decrypted = await this.#decryptSnapState(snapId, state); From 81776dfb8d6c73a4c544f3858a2d2fbd6aa39378 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Fri, 18 Oct 2024 12:49:35 +0200 Subject: [PATCH 3/3] Short-circuit when cached encryption key already exists --- packages/snaps-controllers/coverage.json | 6 +++--- .../src/snaps/SnapController.ts | 17 ++++++++++++++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 5620fbae4d..c10c3a0f6a 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { - "branches": 92.67, + "branches": 92.73, "functions": 96.65, - "lines": 97.98, - "statements": 97.68 + "lines": 97.99, + "statements": 97.69 } diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 576aa4d5d7..f7e24db998 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1747,6 +1747,17 @@ export class SnapController extends BaseController< return { key: encryptionKey, salt }; } + /** + * Check if a given Snap has a cached encryption key stored in the runtime. + * + * @param snapId - The Snap ID. + * @returns True if the Snap has a cached encryption key, otherwise false. + */ + #hasCachedEncryptionKey(snapId: SnapId) { + const runtime = this.#getRuntimeExpect(snapId); + return runtime.encryptionKey !== null && runtime.encryptionSalt !== null; + } + /** * Decrypt the encrypted state for a given Snap. * @@ -1761,7 +1772,11 @@ export class SnapController extends BaseController< // This lets us skip JSON validation. const parsed = JSON.parse(state) as EncryptionResult; const { salt, keyMetadata } = parsed; - const useCache = this.#encryptor.isVaultUpdated(state); + + // We only cache encryption keys if they are already cached or if the encryption key is using the latest key derivation params. + const useCache = + this.#hasCachedEncryptionKey(snapId) || + this.#encryptor.isVaultUpdated(state); const { key } = await this.#getSnapEncryptionKey({ snapId, salt,