Skip to content

Commit

Permalink
perf: Stop validating JSON for stored state (#2845)
Browse files Browse the repository at this point in the history
Removes some validation checks when storing and retrieving state from
`snap_manageState`. Since we have input sanitization on the Snap side,
we can improve performance significantly when retrieving state by
parsing it without additional validation.

Also improves performance slightly by not using `isVaultUpdated` as the
sole check for caching (as that function can be somewhat expensive to
call).
  • Loading branch information
FrederikBolding authored Oct 22, 2024
1 parent 2c5a1e5 commit ec811b7
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 10 deletions.
6 changes: 3 additions & 3 deletions packages/snaps-controllers/coverage.json
Original file line number Diff line number Diff line change
@@ -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
}
29 changes: 22 additions & 7 deletions packages/snaps-controllers/src/snaps/SnapController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ import {
NpmSnapFileNames,
OnNameLookupResponseStruct,
getLocalizedSnapManifest,
parseJson,
MAX_FILE_SIZE,
} from '@metamask/snaps-utils';
import type { Json, NonEmptyArray, SemVerRange } from '@metamask/utils';
Expand All @@ -101,7 +100,6 @@ import {
hasProperty,
inMilliseconds,
isNonEmptyArray,
isValidJson,
isValidSemVerRange,
satisfiesVersionRange,
timeSince,
Expand Down Expand Up @@ -1749,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.
*
Expand All @@ -1759,9 +1768,15 @@ export class SnapController extends BaseController<
*/
async #decryptSnapState(snapId: SnapId, state: string) {
try {
const parsed = parseJson<EncryptionResult>(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);

// 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,
Expand All @@ -1772,8 +1787,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<string, Json>;
} catch {
throw rpcErrors.internal({
Expand Down Expand Up @@ -1864,7 +1878,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);
Expand Down

0 comments on commit ec811b7

Please sign in to comment.