From 3dfdd043d7b78623a88464eb7f231a28e17fc815 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 20 Jan 2025 14:50:20 +0100 Subject: [PATCH] Improve state size validation (#3025) This PR contains a couple changes related to JSON size validation in the `snap_manageState` and `snap_setState` methods: - The maximum size was reduced from 100 MB to 64 MB. This is to because `window.postMessage` breaks when sending payloads that are too large. This is technically a breaking change, but I'm not sure if it should be considered as one, given that the execution environment would throw regardless, if the new state is too big? - `snap_setState` now properly validates the size of the new state, like `snap_manageState` does. --- packages/snaps-rpc-methods/jest.config.js | 4 +- .../src/permitted/setState.test.ts | 111 +++++++++++++++++- .../src/permitted/setState.ts | 16 ++- .../src/restricted/manageState.test.ts | 12 +- .../src/restricted/manageState.ts | 22 +--- 5 files changed, 140 insertions(+), 25 deletions(-) diff --git a/packages/snaps-rpc-methods/jest.config.js b/packages/snaps-rpc-methods/jest.config.js index fafe368501..0b93d167c8 100644 --- a/packages/snaps-rpc-methods/jest.config.js +++ b/packages/snaps-rpc-methods/jest.config.js @@ -10,10 +10,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 93.97, + branches: 94.89, functions: 98.05, lines: 98.67, - statements: 98.25, + statements: 98.34, }, }, }); diff --git a/packages/snaps-rpc-methods/src/permitted/setState.test.ts b/packages/snaps-rpc-methods/src/permitted/setState.test.ts index 4af9e9703f..5861bcb471 100644 --- a/packages/snaps-rpc-methods/src/permitted/setState.test.ts +++ b/packages/snaps-rpc-methods/src/permitted/setState.test.ts @@ -1,7 +1,11 @@ import { JsonRpcEngine } from '@metamask/json-rpc-engine'; import { errorCodes } from '@metamask/rpc-errors'; import type { SetStateResult } from '@metamask/snaps-sdk'; -import type { JsonRpcRequest, PendingJsonRpcResponse } from '@metamask/utils'; +import type { + Json, + JsonRpcRequest, + PendingJsonRpcResponse, +} from '@metamask/utils'; import { setStateHandler, type SetStateParameters, set } from './setState'; @@ -396,6 +400,111 @@ describe('snap_setState', () => { }, }); }); + + it('throws if the new state is not JSON serialisable', async () => { + const { implementation } = setStateHandler; + + const getSnapState = jest.fn().mockReturnValue(null); + const updateSnapState = jest.fn().mockReturnValue(null); + const getUnlockPromise = jest.fn().mockResolvedValue(undefined); + const hasPermission = jest.fn().mockReturnValue(true); + + const hooks = { + getSnapState, + updateSnapState, + getUnlockPromise, + hasPermission, + }; + + const engine = new JsonRpcEngine(); + + engine.push((request, response, next, end) => { + const result = implementation( + request as JsonRpcRequest, + response as PendingJsonRpcResponse, + next, + end, + hooks, + ); + + result?.catch(end); + }); + + const response = await engine.handle({ + jsonrpc: '2.0', + id: 1, + method: 'snap_setState', + params: { + value: { + // @ts-expect-error - BigInt is not JSON serializable. + foo: 1n as Json, + }, + }, + }); + + expect(response).toStrictEqual({ + jsonrpc: '2.0', + id: 1, + error: { + code: errorCodes.rpc.invalidParams, + message: + 'Invalid params: At path: value -- Expected a value of type `JSON`, but received: `[object Object]`.', + stack: expect.any(String), + }, + }); + }); + + it('throws if the new state exceeds the size limit', async () => { + const { implementation } = setStateHandler; + + const getSnapState = jest.fn().mockReturnValue(null); + const updateSnapState = jest.fn().mockReturnValue(null); + const getUnlockPromise = jest.fn().mockResolvedValue(undefined); + const hasPermission = jest.fn().mockReturnValue(true); + + const hooks = { + getSnapState, + updateSnapState, + getUnlockPromise, + hasPermission, + }; + + const engine = new JsonRpcEngine(); + + engine.push((request, response, next, end) => { + const result = implementation( + request as JsonRpcRequest, + response as PendingJsonRpcResponse, + next, + end, + hooks, + ); + + result?.catch(end); + }); + + const response = await engine.handle({ + jsonrpc: '2.0', + id: 1, + method: 'snap_setState', + params: { + value: { + foo: 'foo'.repeat(21_500_000), // 64.5 MB + }, + }, + }); + + expect(response).toStrictEqual({ + jsonrpc: '2.0', + id: 1, + error: { + code: errorCodes.rpc.invalidParams, + message: + 'Invalid params: The new state must not exceed 64 MB in size.', + stack: expect.any(String), + }, + }); + }); }); }); diff --git a/packages/snaps-rpc-methods/src/permitted/setState.ts b/packages/snaps-rpc-methods/src/permitted/setState.ts index 5227d69391..55317a57b3 100644 --- a/packages/snaps-rpc-methods/src/permitted/setState.ts +++ b/packages/snaps-rpc-methods/src/permitted/setState.ts @@ -13,6 +13,7 @@ import { } from '@metamask/superstruct'; import type { PendingJsonRpcResponse, JsonRpcRequest } from '@metamask/utils'; import { + getJsonSize, hasProperty, isObject, assert, @@ -20,7 +21,10 @@ import { type Json, } from '@metamask/utils'; -import { manageStateBuilder } from '../restricted/manageState'; +import { + manageStateBuilder, + STORAGE_SIZE_LIMIT, +} from '../restricted/manageState'; import type { MethodHooksObject } from '../utils'; import { FORBIDDEN_KEYS, StateKeyStruct } from '../utils'; @@ -142,6 +146,16 @@ async function setStateImplementation( } const newState = await getNewState(key, value, encrypted, getSnapState); + + const size = getJsonSize(newState); + if (size > STORAGE_SIZE_LIMIT) { + throw rpcErrors.invalidParams({ + message: `Invalid params: The new state must not exceed ${ + STORAGE_SIZE_LIMIT / 1_000_000 + } MB in size.`, + }); + } + await updateSnapState(newState, encrypted); response.result = null; } catch (error) { diff --git a/packages/snaps-rpc-methods/src/restricted/manageState.test.ts b/packages/snaps-rpc-methods/src/restricted/manageState.test.ts index a6b2cfc45b..d1b6a0bf4d 100644 --- a/packages/snaps-rpc-methods/src/restricted/manageState.test.ts +++ b/packages/snaps-rpc-methods/src/restricted/manageState.test.ts @@ -333,7 +333,7 @@ describe('snap_manageState', () => { }, }), ).rejects.toThrow( - 'Invalid snap_manageState "updateState" parameter: The new state must be a plain object.', + 'Invalid snap_manageState "newState" parameter: The new state must be a plain object.', ); expect(updateSnapState).not.toHaveBeenCalledWith( @@ -375,7 +375,7 @@ describe('snap_manageState', () => { }, }), ).rejects.toThrow( - 'Invalid snap_manageState "updateState" parameter: The new state must be JSON serializable.', + 'Invalid snap_manageState "newState" parameter: The new state must be JSON serializable.', ); expect(updateSnapState).not.toHaveBeenCalledWith( @@ -459,7 +459,7 @@ describe('snap_manageState', () => { 'snap_manageState', ), ).toThrow( - 'Invalid snap_manageState "updateState" parameter: The new state must be a plain object.', + 'Invalid snap_manageState "newState" parameter: The new state must be a plain object.', ); }); @@ -478,7 +478,7 @@ describe('snap_manageState', () => { 'snap_manageState', ), ).toThrow( - 'Invalid snap_manageState "updateState" parameter: The new state must be JSON serializable.', + 'Invalid snap_manageState "newState" parameter: The new state must be JSON serializable.', ); }); @@ -498,7 +498,9 @@ describe('snap_manageState', () => { MOCK_SMALLER_STORAGE_SIZE_LIMIT, ), ).toThrow( - `Invalid snap_manageState "updateState" parameter: The new state must not exceed ${MOCK_SMALLER_STORAGE_SIZE_LIMIT} bytes in size.`, + `Invalid snap_manageState "newState" parameter: The new state must not exceed ${ + MOCK_SMALLER_STORAGE_SIZE_LIMIT / 1_000_000 + } MB in size.`, ); }); }); diff --git a/packages/snaps-rpc-methods/src/restricted/manageState.ts b/packages/snaps-rpc-methods/src/restricted/manageState.ts index a8bfe4a81b..a99848ce3a 100644 --- a/packages/snaps-rpc-methods/src/restricted/manageState.ts +++ b/packages/snaps-rpc-methods/src/restricted/manageState.ts @@ -107,7 +107,7 @@ export const manageStateBuilder = Object.freeze({ methodHooks, } as const); -export const STORAGE_SIZE_LIMIT = 104857600; // In bytes (100MB) +export const STORAGE_SIZE_LIMIT = 64_000_000; // In bytes (64 MB) type GetEncryptionKeyArgs = { snapId: string; @@ -256,11 +256,7 @@ export function getValidatedParams( if (operation === ManageStateOperation.UpdateState) { if (!isObject(newState)) { throw rpcErrors.invalidParams({ - message: `Invalid ${method} "updateState" parameter: The new state must be a plain object.`, - data: { - receivedNewState: - typeof newState === 'undefined' ? 'undefined' : newState, - }, + message: `Invalid ${method} "newState" parameter: The new state must be a plain object.`, }); } @@ -270,21 +266,15 @@ export function getValidatedParams( size = getJsonSize(newState); } catch { throw rpcErrors.invalidParams({ - message: `Invalid ${method} "updateState" parameter: The new state must be JSON serializable.`, - data: { - receivedNewState: - typeof newState === 'undefined' ? 'undefined' : newState, - }, + message: `Invalid ${method} "newState" parameter: The new state must be JSON serializable.`, }); } if (size > storageSizeLimit) { throw rpcErrors.invalidParams({ - message: `Invalid ${method} "updateState" parameter: The new state must not exceed ${storageSizeLimit} bytes in size.`, - data: { - receivedNewState: - typeof newState === 'undefined' ? 'undefined' : newState, - }, + message: `Invalid ${method} "newState" parameter: The new state must not exceed ${ + storageSizeLimit / 1_000_000 + } MB in size.`, }); } }