From 32f958f4846133f6b9be6f33f1a01448d45255ee Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 11 Oct 2022 15:31:14 +0200 Subject: [PATCH 1/6] Improve execution environment type validation --- .../execution-environments/jest.config.js | 8 +- packages/execution-environments/package.json | 3 +- .../src/common/BaseSnapExecutor.test.ts | 31 +-- .../src/common/BaseSnapExecutor.ts | 30 ++- .../src/common/commands.test.ts | 12 +- .../src/common/commands.ts | 67 +++---- .../src/common/sortParams.ts | 4 +- .../src/common/validation.ts | 184 ++++++++++++++++++ yarn.lock | 1 + 9 files changed, 257 insertions(+), 83 deletions(-) diff --git a/packages/execution-environments/jest.config.js b/packages/execution-environments/jest.config.js index 504d65031a..e6ccba9652 100644 --- a/packages/execution-environments/jest.config.js +++ b/packages/execution-environments/jest.config.js @@ -6,10 +6,10 @@ module.exports = { coverageReporters: ['clover', 'json', 'lcov', 'text', 'json-summary'], coverageThreshold: { global: { - branches: 82.88, - functions: 92.48, - lines: 85.84, - statements: 86.02, + branches: 83.51, + functions: 92.64, + lines: 86.37, + statements: 86.54, }, }, moduleFileExtensions: ['js', 'json', 'jsx', 'ts', 'tsx', 'node'], diff --git a/packages/execution-environments/package.json b/packages/execution-environments/package.json index 187909e47f..01172d40bf 100644 --- a/packages/execution-environments/package.json +++ b/packages/execution-environments/package.json @@ -41,7 +41,8 @@ "eth-rpc-errors": "^4.0.3", "pump": "^3.0.0", "ses": "^0.15.15", - "stream-browserify": "^3.0.0" + "stream-browserify": "^3.0.0", + "superstruct": "^0.16.5" }, "devDependencies": { "@lavamoat/allow-scripts": "^2.0.3", diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts index 5652faae29..2b840283ac 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts @@ -1,9 +1,14 @@ // eslint-disable-next-line import/no-unassigned-import import 'ses'; import { Duplex, DuplexOptions, EventEmitter, Readable } from 'stream'; -import { Json, JsonRpcResponse } from '@metamask/utils'; +import { + assertIsJsonRpcSuccess, + Json, + JsonRpcParams, + JsonRpcRequest, + JsonRpcResponse, +} from '@metamask/utils'; import { HandlerType } from '@metamask/snap-utils'; -import { JsonRpcRequest } from '../__GENERATED__/openrpc'; import { BaseSnapExecutor } from './BaseSnapExecutor'; const FAKE_ORIGIN = 'origin:foo'; @@ -106,7 +111,7 @@ class TestSnapExecutor extends BaseSnapExecutor { }); } - public writeCommand(message: JsonRpcRequest): Promise { + public writeCommand(message: JsonRpcRequest): Promise { return new Promise((resolve, reject) => this.commandLeft.write(message, (error) => { if (error) { @@ -117,8 +122,8 @@ class TestSnapExecutor extends BaseSnapExecutor { ); } - public readCommand(): Promise { - const promise = new Promise((resolve) => + public readCommand(): Promise> { + const promise = new Promise>((resolve) => this.commandListeners.push(resolve), ); @@ -153,10 +158,14 @@ class TestSnapExecutor extends BaseSnapExecutor { ); } - public readRpc(): Promise<{ name: string; data: JsonRpcRequest }> { - const promise = new Promise<{ name: string; data: JsonRpcRequest }>( - (resolve) => this.rpcListeners.push(resolve), - ); + public readRpc(): Promise<{ + name: string; + data: JsonRpcRequest; + }> { + const promise = new Promise<{ + name: string; + data: JsonRpcRequest; + }>((resolve) => this.rpcListeners.push(resolve)); TestSnapExecutor.flushReads(this.rpcBuffer, this.rpcListeners); @@ -360,8 +369,8 @@ describe('BaseSnapExecutor', () => { }), ); - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const handle = getHandleResult!.result; + assertIsJsonRpcSuccess(getHandleResult); + const handle = getHandleResult.result; await executor.writeCommand({ jsonrpc: '2.0', diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.ts b/packages/execution-environments/src/common/BaseSnapExecutor.ts index dc0bc88c18..467f71cd3d 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.ts @@ -9,6 +9,11 @@ import { isValidJson, JsonRpcNotification, assert, + isJsonRpcRequest, + JsonRpcId, + JsonRpcRequest, + JsonRpcParams, + Json, } from '@metamask/utils'; import { HandlerType, @@ -16,13 +21,6 @@ import { SnapExportsParameters, } from '@metamask/snap-utils'; import EEOpenRPCDocument from '../openrpc.json'; -import { - Endowments, - JSONRPCID, - JsonRpcRequest, - Target, -} from '../__GENERATED__/openrpc'; -import { isJsonRpcRequest } from '../__GENERATED__/openrpc.guard'; import { createEndowments } from './endowments'; import { getCommandMethodImplementations, @@ -52,10 +50,10 @@ const fallbackError = { export type InvokeSnapArgs = SnapExportsParameters[0]; export type InvokeSnap = ( - target: Target, + target: string, handler: HandlerType, args: InvokeSnapArgs | undefined, -) => Promise; +) => Promise; export class BaseSnapExecutor { private snapData: Map; @@ -118,16 +116,12 @@ export class BaseSnapExecutor { }); } - private async onCommandRequest(message: JsonRpcRequest) { + private async onCommandRequest(message: JsonRpcRequest) { if (!isJsonRpcRequest(message)) { - throw new Error('Command stream received a non Json Rpc Request'); - } - const { id, method, params } = message; - - if (id === undefined) { - throw new Error('Notifications not supported'); + throw new Error('Command stream received a non-JSON-RPC request.'); } + const { id, method, params } = message; if (method === 'rpc.discover') { this.respond(id, { result: EEOpenRPCDocument, @@ -185,7 +179,7 @@ export class BaseSnapExecutor { }); } - protected respond(id: JSONRPCID, requestObject: Record) { + protected respond(id: JsonRpcId, requestObject: Record) { if (!isValidJson(requestObject) || !isObject(requestObject)) { throw new Error('JSON-RPC responses must be JSON serializable objects.'); } @@ -208,7 +202,7 @@ export class BaseSnapExecutor { protected async startSnap( snapName: string, sourceCode: string, - _endowments?: Endowments, + _endowments?: string[], ): Promise { console.log(`starting snap '${snapName}' in worker`); if (this.snapPromiseErrorHandler) { diff --git a/packages/execution-environments/src/common/commands.test.ts b/packages/execution-environments/src/common/commands.test.ts index ece1cd0416..68594d00f4 100644 --- a/packages/execution-environments/src/common/commands.test.ts +++ b/packages/execution-environments/src/common/commands.test.ts @@ -74,7 +74,7 @@ describe('getCommandMethodImplementations', () => { 'endowment1', 'endowment2', ]); - }).rejects.toThrow('snapName is not a string'); + }).rejects.toThrow('Snap name is not a string.'); }); it('the executeSnap method will throw an Error if the sourceCode is not a string', async () => { @@ -91,7 +91,7 @@ describe('getCommandMethodImplementations', () => { 'endowment1', 'endowment2', ]); - }).rejects.toThrow('sourceCode is not a string'); + }).rejects.toThrow('Source code is not a string.'); }); it('the executeSnap method will throw an Error if it is not passed a proper Endowments object', async () => { @@ -105,7 +105,7 @@ describe('getCommandMethodImplementations', () => { ); await expect(async () => { await methodsObj.executeSnap('foo', 'code', ['endowment1', 2 as any]); - }).rejects.toThrow('endowment is not a proper Endowments object'); + }).rejects.toThrow('Endowments is not an array of strings.'); }); it('the snapRpc method will invoke the invokeSnapRpc function', async () => { @@ -152,7 +152,7 @@ describe('getCommandMethodImplementations', () => { 'bar', rpcRequest as any, ); - }).rejects.toThrow('target is not a string'); + }).rejects.toThrow('Target is not a string.'); }); it('the snapRpc method will throw an error if the origin is not a string', async () => { @@ -172,7 +172,7 @@ describe('getCommandMethodImplementations', () => { 2 as any, rpcRequest as any, ); - }).rejects.toThrow('origin is not a string'); + }).rejects.toThrow('Origin is not a string.'); }); it('the snapRpc method will throw an error if the request is not a JSON RPC request', async () => { @@ -192,6 +192,6 @@ describe('getCommandMethodImplementations', () => { 'bar', rpcRequest as any, ); - }).rejects.toThrow('request is not a proper JSON RPC Request'); + }).rejects.toThrow('Request is not a proper JSON-RPC request.'); }); }); diff --git a/packages/execution-environments/src/common/commands.ts b/packages/execution-environments/src/common/commands.ts index 43b56edf43..6329e3e3ea 100644 --- a/packages/execution-environments/src/common/commands.ts +++ b/packages/execution-environments/src/common/commands.ts @@ -1,14 +1,19 @@ -import { JsonRpcRequest, assertExhaustive } from '@metamask/utils'; +import { + JsonRpcRequest, + assertExhaustive, + JsonRpcParams, + assert, +} from '@metamask/utils'; import { HandlerType } from '@metamask/snap-utils'; +import { InvokeSnap, InvokeSnapArgs } from './BaseSnapExecutor'; import { ExecuteSnap, - Origin, + isEndowmentsArray, + isJsonRpcRequestWithoutId, Ping, SnapRpc, Terminate, -} from '../__GENERATED__/openrpc'; -import { isEndowments, isJsonRpcRequest } from '../__GENERATED__/openrpc.guard'; -import { InvokeSnap, InvokeSnapArgs } from './BaseSnapExecutor'; +} from './validation'; export type CommandMethodsMapping = { ping: Ping; @@ -27,9 +32,9 @@ export type CommandMethodsMapping = { * @returns The formatted arguments. */ function getHandlerArguments( - origin: Origin, + origin: string, handler: HandlerType, - request: JsonRpcRequest, + request: JsonRpcRequest, ): InvokeSnapArgs { switch (handler) { case HandlerType.OnTransaction: { @@ -86,51 +91,31 @@ export function getCommandMethodImplementations( }, executeSnap: async (snapName, sourceCode, endowments) => { - if (typeof snapName !== 'string') { - throw new Error('snapName is not a string'); - } - - if (typeof sourceCode !== 'string') { - throw new Error('sourceCode is not a string'); - } - - if (endowments !== undefined) { - if (!isEndowments(endowments)) { - throw new Error('endowment is not a proper Endowments object'); - } - } + assert(typeof snapName === 'string', 'Snap name is not a string.'); + assert(typeof sourceCode === 'string', 'Source code is not a string.'); + assert( + !endowments || isEndowmentsArray(endowments), + 'Endowments is not an array of strings.', + ); await startSnap(snapName as string, sourceCode as string, endowments); return 'OK'; }, snapRpc: async (target, handler, origin, request) => { - if (typeof target !== 'string') { - throw new Error('target is not a string'); - } - - if (typeof origin !== 'string') { - throw new Error('origin is not a string'); - } - - if (!isJsonRpcRequest(request)) { - throw new Error('request is not a proper JSON RPC Request'); - } - - if (!isHandler(handler)) { - throw new Error('Incorrect handler type.'); - } + assert(typeof target === 'string', 'Target is not a string.'); + assert(typeof origin === 'string', 'Origin is not a string.'); + assert( + isJsonRpcRequestWithoutId(request), + 'Request is not a proper JSON-RPC request.', + ); + assert(isHandler(handler), 'Incorrect handler type.'); return ( (await invokeSnap( target, handler, - getHandlerArguments( - origin, - handler, - // Specifically casting to other JsonRpcRequest type here on purpose, to stop using the OpenRPC type. - request as JsonRpcRequest, - ), + getHandlerArguments(origin, handler, request), )) ?? null ); }, diff --git a/packages/execution-environments/src/common/sortParams.ts b/packages/execution-environments/src/common/sortParams.ts index 4b7a0ba6c1..b945f6948f 100644 --- a/packages/execution-environments/src/common/sortParams.ts +++ b/packages/execution-environments/src/common/sortParams.ts @@ -1,6 +1,6 @@ // original source sortParamKeys from: https://github.com/etclabscore/sig.tools/blob/master/src/postMessageServer/postMessageServer.ts#L75-L77 -import { JSONRPCParams } from '../__GENERATED__/openrpc'; +import { JsonRpcParams } from '@metamask/utils'; /** * Deterministically sort JSON-RPC parameter keys. This makes it possible to @@ -19,7 +19,7 @@ import { JSONRPCParams } from '../__GENERATED__/openrpc'; */ export const sortParamKeys = ( method: { params: { name: string }[] }, - params?: JSONRPCParams, + params?: JsonRpcParams, ) => { if (!params) { return []; diff --git a/packages/execution-environments/src/common/validation.ts b/packages/execution-environments/src/common/validation.ts index 3cd1ec0803..fe4f17ba31 100644 --- a/packages/execution-environments/src/common/validation.ts +++ b/packages/execution-environments/src/common/validation.ts @@ -1,5 +1,26 @@ import { HandlerType } from '@metamask/snap-utils'; import { SnapKeyring } from '@metamask/snap-types'; +import { + array, + assign, + enums, + Infer, + is, + literal, + object, + omit, + optional, + string, + unknown, +} from 'superstruct'; +import { + Json, + JsonRpcIdStruct, + JsonRpcRequest, + JsonRpcRequestStruct, + JsonRpcSuccess, + JsonRpcSuccessStruct, +} from '@metamask/utils'; const VALIDATION_FUNCTIONS = { [HandlerType.OnRpcRequest]: validateFunctionExport, @@ -41,3 +62,166 @@ export function validateExport(type: HandlerType, snapExport: unknown) { const validationFunction = VALIDATION_FUNCTIONS[type]; return validationFunction?.(snapExport) ?? false; } + +export const JsonRpcRequestWithoutIdStruct = assign( + omit(JsonRpcRequestStruct, ['id']), + object({ + id: optional(JsonRpcIdStruct), + }), +); + +export type JsonRpcRequestWithoutId = Infer< + typeof JsonRpcRequestWithoutIdStruct +>; + +/** + * Check if the given value is a JSON-RPC request with an optional ID. + * + * @param value - The value to check. + * @returns Whether the value is a JSON-RPC request with an optional ID. + */ +export function isJsonRpcRequestWithoutId( + value: unknown, +): value is JsonRpcRequestWithoutId { + return is(value, JsonRpcRequestWithoutIdStruct); +} + +export const EndowmentStruct = string(); +export type Endowment = Infer; + +/** + * Check if the given value is an endowment. + * + * @param value - The value to check. + * @returns Whether the value is an endowment. + */ +export function isEndowment(value: unknown): value is Endowment { + return is(value, EndowmentStruct); +} + +/** + * Check if the given value is an array of endowments. + * + * @param value - The value to check. + * @returns Whether the value is an array of endowments. + */ +export function isEndowmentsArray(value: unknown): value is Endowment[] { + return Array.isArray(value) && value.every(isEndowment); +} + +const OkStruct = literal('OK'); + +const PingRequestStruct = assign( + JsonRpcRequestStruct, + object({ + method: literal('ping'), + params: optional(unknown()), + }), +); + +const TerminateRequestStruct = assign( + JsonRpcRequestStruct, + object({ + method: literal('terminate'), + params: optional(unknown()), + }), +); + +const ExecuteSnapRequestStruct = assign( + JsonRpcRequestStruct, + object({ + method: literal('executeSnap'), + params: object({ + snapName: string(), + sourceCode: string(), + endowments: optional(array(EndowmentStruct)), + }), + }), +); + +const SnapRpcRequestStruct = assign( + JsonRpcRequestStruct, + object({ + method: literal('snapRpc'), + params: object({ + target: string(), + handler: enums(Object.values(HandlerType)), + origin: string(), + request: JsonRpcRequestStruct, + }), + }), +); + +export type PingRequest = Infer; +export type TerminateRequest = Infer; +export type ExecuteSnapRequest = Infer; +export type SnapRpcRequest = Infer; + +export type Request = + | PingRequest + | TerminateRequest + | ExecuteSnapRequest + | SnapRpcRequest; + +const OkResponseStruct = assign( + JsonRpcSuccessStruct, + object({ + result: OkStruct, + }), +); + +const SnapRpcResponse = JsonRpcSuccessStruct; + +export type OkResponse = Infer; +export type SnapRpcResponse = Infer; + +export type Response = OkResponse | SnapRpcResponse; + +// UnionToIntersection = A & B +type UnionToIntersection = ( + U extends unknown ? (arg: U) => 0 : never +) extends (arg: infer I) => 0 + ? I + : never; + +// LastInUnion = B +type LastInUnion = UnionToIntersection< + U extends unknown ? (x: U) => 0 : never +> extends (x: infer L) => 0 + ? L + : never; + +// UnionToTuple = [A, B] +type UnionToTuple> = [T] extends [never] + ? [] + : [Last, ...UnionToTuple>]; + +type InterfaceToTuple< + T, + U = UnionToTuple, + V extends any[] = [], +> = U extends [infer F, ...infer Rest] + ? F extends keyof T + ? InterfaceToTuple + : never + : V; + +type RequestParams = RequestType extends JsonRpcRequest< + infer Params +> + ? Params extends Record + ? InterfaceToTuple + : Params extends unknown[] + ? [...Params] + : [] + : []; + +type RequestFunction< + RequestType extends Request, + ResponseType extends JsonRpcSuccess, +> = (...args: RequestParams) => Promise; + +export type Ping = RequestFunction; +export type Terminate = RequestFunction; +export type ExecuteSnap = RequestFunction; +export type SnapRpc = RequestFunction; diff --git a/yarn.lock b/yarn.lock index 3ebc1c5cb4..673242524e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2876,6 +2876,7 @@ __metadata: rimraf: ^3.0.2 ses: ^0.15.15 stream-browserify: ^3.0.0 + superstruct: ^0.16.5 ts-auto-guard: ^2.3.0 ts-jest: ^29.0.0 ts-loader: ^9.3.1 From 74c2589e0d422913034bd146a8652907ee5cce2f Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Wed, 12 Oct 2022 12:31:24 +0200 Subject: [PATCH 2/6] Simplify code --- .../src/common/BaseSnapExecutor.test.ts | 117 +++++++++++++++ .../src/common/BaseSnapExecutor.ts | 67 ++++++++- .../src/common/commands.test.ts | 108 -------------- .../src/common/commands.ts | 37 +---- .../src/common/sortParams.test.ts | 6 +- .../src/common/sortParams.ts | 21 ++- .../src/common/validation.ts | 138 ++++++------------ 7 files changed, 239 insertions(+), 255 deletions(-) diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts index 2b840283ac..c05017709d 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts @@ -1544,4 +1544,121 @@ describe('BaseSnapExecutor', () => { }, }); }); + + describe('executeSnap', () => { + it.each([ + { + snapName: 1, + code: 'module.exports.onRpcRequest = () => 1;', + endowments: [], + }, + { + snapName: FAKE_SNAP_NAME, + code: 1, + endowments: [], + }, + { + snapName: FAKE_SNAP_NAME, + code: 'module.exports.onRpcRequest = () => 1;', + endowments: ['foo', 1], + }, + [1, 'module.exports.onRpcRequest = () => 1;', []], + [FAKE_SNAP_NAME, 1, []], + [FAKE_SNAP_NAME, 'module.exports.onRpcRequest = () => 1;', ['foo', 1]], + ])( + 'throws an error if the request arguments are invalid', + async (params) => { + const executor = new TestSnapExecutor(); + + await executor.writeCommand({ + jsonrpc: '2.0', + id: 1, + method: 'executeSnap', + params, + }); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + id: 1, + error: { + code: -32600, + data: expect.any(Object), + message: expect.any(String), + stack: expect.any(String), + }, + }); + }, + ); + }); + + describe('snapRpc', () => { + it.each([ + { + snapName: 1, + method: ON_RPC_REQUEST, + origin: FAKE_ORIGIN, + request: { jsonrpc: '2.0', method: '', params: [] }, + }, + { + snapName: FAKE_SNAP_NAME, + method: 1, + origin: FAKE_ORIGIN, + request: { jsonrpc: '2.0', method: '', params: [] }, + }, + { + snapName: FAKE_SNAP_NAME, + method: ON_RPC_REQUEST, + origin: 1, + request: { jsonrpc: '2.0', method: '', params: [] }, + }, + { + snapName: FAKE_SNAP_NAME, + method: ON_RPC_REQUEST, + origin: FAKE_ORIGIN, + request: 1, + }, + [ + 1, + ON_RPC_REQUEST, + FAKE_ORIGIN, + { jsonrpc: '2.0', method: '', params: [] }, + ], + [ + FAKE_SNAP_NAME, + 1, + FAKE_ORIGIN, + { jsonrpc: '2.0', method: '', params: [] }, + ], + [ + FAKE_SNAP_NAME, + ON_RPC_REQUEST, + 1, + { jsonrpc: '2.0', method: '', params: [] }, + ], + [FAKE_SNAP_NAME, ON_RPC_REQUEST, FAKE_ORIGIN, 1], + ])( + 'throws an error if the request arguments are invalid', + async (params) => { + const executor = new TestSnapExecutor(); + + await executor.writeCommand({ + jsonrpc: '2.0', + id: 1, + method: 'snapRpc', + params, + }); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + id: 1, + error: { + code: -32600, + data: expect.any(Object), + message: expect.any(String), + stack: expect.any(String), + }, + }); + }, + ); + }); }); diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.ts b/packages/execution-environments/src/common/BaseSnapExecutor.ts index 467f71cd3d..f822071e40 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.ts @@ -14,12 +14,14 @@ import { JsonRpcRequest, JsonRpcParams, Json, + hasProperty, } from '@metamask/utils'; import { HandlerType, SNAP_EXPORT_NAMES, SnapExportsParameters, } from '@metamask/snap-utils'; +import { validate } from 'superstruct'; import EEOpenRPCDocument from '../openrpc.json'; import { createEndowments } from './endowments'; import { @@ -30,7 +32,13 @@ import { removeEventListener, addEventListener } from './globalEvents'; import { sortParamKeys } from './sortParams'; import { constructError, withTeardown } from './utils'; import { wrapKeyring } from './keyring'; -import { validateExport } from './validation'; +import { + ExecuteSnapRequestArgumentsStruct, + PingRequestArgumentsStruct, + SnapRpcRequestArgumentsStruct, + TerminateRequestStruct, + validateExport, +} from './validation'; type EvaluationData = { stop: () => void; @@ -47,7 +55,9 @@ const fallbackError = { message: 'Execution Environment Error', }; -export type InvokeSnapArgs = SnapExportsParameters[0]; +// TODO: `KeyringParameters` expects a `chainId` for certain methods, but we're +// not providing it in `getHandlerArguments`, resulting in type errors. +export type InvokeSnapArgs = Omit; export type InvokeSnap = ( target: string, @@ -55,6 +65,30 @@ export type InvokeSnap = ( args: InvokeSnapArgs | undefined, ) => Promise; +/** + * The supported methods in the execution environment. The validator checks the + * incoming JSON-RPC request, and the `params` property is used for sorting the + * parameters, if they are an object. + */ +const EXECUTION_ENVIRONMENT_METHODS = { + ping: { + validator: PingRequestArgumentsStruct, + params: [], + }, + executeSnap: { + validator: ExecuteSnapRequestArgumentsStruct, + params: ['snapName', 'sourceCode', 'endowments'], + }, + terminate: { + validator: TerminateRequestStruct, + params: [], + }, + snapRpc: { + validator: SnapRpcRequestArgumentsStruct, + params: ['target', 'handler', 'origin', 'request'], + }, +}; + export class BaseSnapExecutor { private snapData: Map; @@ -129,11 +163,7 @@ export class BaseSnapExecutor { return; } - const methodObject = EEOpenRPCDocument.methods.find( - (m) => m.name === method, - ); - - if (!methodObject || !(this.methods as any)[method]) { + if (!hasProperty(EXECUTION_ENVIRONMENT_METHODS, method)) { this.respond(id, { error: ethErrors.rpc .methodNotFound({ @@ -146,8 +176,29 @@ export class BaseSnapExecutor { return; } + const methodObject = + EXECUTION_ENVIRONMENT_METHODS[ + method as keyof typeof EXECUTION_ENVIRONMENT_METHODS + ]; + // support params by-name and by-position - const paramsAsArray = sortParamKeys(methodObject, params); + const paramsAsArray = sortParamKeys(methodObject.params, params); + + const [error] = validate(paramsAsArray, methodObject.validator); + if (error) { + this.respond(id, { + error: ethErrors.rpc + .invalidRequest({ + message: error.message, + data: { + method, + params: paramsAsArray, + }, + }) + .serialize(), + }); + return; + } try { const result = await (this.methods as any)[method](...paramsAsArray); diff --git a/packages/execution-environments/src/common/commands.test.ts b/packages/execution-environments/src/common/commands.test.ts index 68594d00f4..7be66f33ec 100644 --- a/packages/execution-environments/src/common/commands.test.ts +++ b/packages/execution-environments/src/common/commands.test.ts @@ -60,54 +60,6 @@ describe('getCommandMethodImplementations', () => { ).toStrictEqual('OK'); }); - it('the executeSnap method will throw an Error if the snapName is not a string', async () => { - const startSnap = jest.fn(); - const invokeSnapRpc = jest.fn(); - const onTerminate = jest.fn(); - const methodsObj = getCommandMethodImplementations( - startSnap, - invokeSnapRpc, - onTerminate, - ); - await expect(async () => { - await methodsObj.executeSnap(1 as any, 'code', [ - 'endowment1', - 'endowment2', - ]); - }).rejects.toThrow('Snap name is not a string.'); - }); - - it('the executeSnap method will throw an Error if the sourceCode is not a string', async () => { - const startSnap = jest.fn(); - const invokeSnapRpc = jest.fn(); - const onTerminate = jest.fn(); - const methodsObj = getCommandMethodImplementations( - startSnap, - invokeSnapRpc, - onTerminate, - ); - await expect(async () => { - await methodsObj.executeSnap('foo', 2 as any, [ - 'endowment1', - 'endowment2', - ]); - }).rejects.toThrow('Source code is not a string.'); - }); - - it('the executeSnap method will throw an Error if it is not passed a proper Endowments object', async () => { - const startSnap = jest.fn(); - const invokeSnapRpc = jest.fn(); - const onTerminate = jest.fn(); - const methodsObj = getCommandMethodImplementations( - startSnap, - invokeSnapRpc, - onTerminate, - ); - await expect(async () => { - await methodsObj.executeSnap('foo', 'code', ['endowment1', 2 as any]); - }).rejects.toThrow('Endowments is not an array of strings.'); - }); - it('the snapRpc method will invoke the invokeSnapRpc function', async () => { const startSnap = jest.fn(); const invokeSnapRpc = jest.fn(); @@ -134,64 +86,4 @@ describe('getCommandMethodImplementations', () => { }, ); }); - - it('the snapRpc method will throw an error if the target is not a string', async () => { - const startSnap = jest.fn(); - const invokeSnapRpc = jest.fn(); - const onTerminate = jest.fn(); - const methodsObj = getCommandMethodImplementations( - startSnap, - invokeSnapRpc, - onTerminate, - ); - const rpcRequest = { jsonrpc: '2.0', method: 'hello' }; - await expect(async () => { - await methodsObj.snapRpc( - 2 as any, - HandlerType.OnRpcRequest, - 'bar', - rpcRequest as any, - ); - }).rejects.toThrow('Target is not a string.'); - }); - - it('the snapRpc method will throw an error if the origin is not a string', async () => { - const startSnap = jest.fn(); - const invokeSnapRpc = jest.fn(); - const onTerminate = jest.fn(); - const methodsObj = getCommandMethodImplementations( - startSnap, - invokeSnapRpc, - onTerminate, - ); - const rpcRequest = { jsonrpc: '2.0', method: 'hello' }; - await expect(async () => { - await methodsObj.snapRpc( - 'foo', - HandlerType.OnRpcRequest, - 2 as any, - rpcRequest as any, - ); - }).rejects.toThrow('Origin is not a string.'); - }); - - it('the snapRpc method will throw an error if the request is not a JSON RPC request', async () => { - const startSnap = jest.fn(); - const invokeSnapRpc = jest.fn(); - const onTerminate = jest.fn(); - const methodsObj = getCommandMethodImplementations( - startSnap, - invokeSnapRpc, - onTerminate, - ); - const rpcRequest = { method: 'hello' }; - await expect(async () => { - await methodsObj.snapRpc( - 'foo', - HandlerType.OnRpcRequest, - 'bar', - rpcRequest as any, - ); - }).rejects.toThrow('Request is not a proper JSON-RPC request.'); - }); }); diff --git a/packages/execution-environments/src/common/commands.ts b/packages/execution-environments/src/common/commands.ts index 6329e3e3ea..854b2bbe8c 100644 --- a/packages/execution-environments/src/common/commands.ts +++ b/packages/execution-environments/src/common/commands.ts @@ -1,15 +1,9 @@ -import { - JsonRpcRequest, - assertExhaustive, - JsonRpcParams, - assert, -} from '@metamask/utils'; +import { assertExhaustive } from '@metamask/utils'; import { HandlerType } from '@metamask/snap-utils'; import { InvokeSnap, InvokeSnapArgs } from './BaseSnapExecutor'; import { ExecuteSnap, - isEndowmentsArray, - isJsonRpcRequestWithoutId, + JsonRpcRequestWithoutId, Ping, SnapRpc, Terminate, @@ -34,7 +28,7 @@ export type CommandMethodsMapping = { function getHandlerArguments( origin: string, handler: HandlerType, - request: JsonRpcRequest, + request: JsonRpcRequestWithoutId, ): InvokeSnapArgs { switch (handler) { case HandlerType.OnTransaction: { @@ -54,16 +48,6 @@ function getHandlerArguments( } } -/** - * Typeguard to ensure a handler is part of the HandlerType. - * - * @param handler - The handler to pass the request to. - * @returns A boolean. - */ -function isHandler(handler: string): handler is HandlerType { - return Object.values(HandlerType).includes(handler as HandlerType); -} - /** * Gets an object mapping internal, "command" JSON-RPC method names to their * implementations. @@ -91,26 +75,11 @@ export function getCommandMethodImplementations( }, executeSnap: async (snapName, sourceCode, endowments) => { - assert(typeof snapName === 'string', 'Snap name is not a string.'); - assert(typeof sourceCode === 'string', 'Source code is not a string.'); - assert( - !endowments || isEndowmentsArray(endowments), - 'Endowments is not an array of strings.', - ); - await startSnap(snapName as string, sourceCode as string, endowments); return 'OK'; }, snapRpc: async (target, handler, origin, request) => { - assert(typeof target === 'string', 'Target is not a string.'); - assert(typeof origin === 'string', 'Origin is not a string.'); - assert( - isJsonRpcRequestWithoutId(request), - 'Request is not a proper JSON-RPC request.', - ); - assert(isHandler(handler), 'Incorrect handler type.'); - return ( (await invokeSnap( target, diff --git a/packages/execution-environments/src/common/sortParams.test.ts b/packages/execution-environments/src/common/sortParams.test.ts index 20e894bdc8..c92c85bc6d 100644 --- a/packages/execution-environments/src/common/sortParams.test.ts +++ b/packages/execution-environments/src/common/sortParams.test.ts @@ -2,18 +2,18 @@ import { sortParamKeys } from './sortParams'; describe('getSortedParams', () => { it('will return an empty array if params is undefined', () => { - const method = { params: [{ name: 'foo' }, { name: 'bar' }] }; + const method = ['foo', 'bar']; expect(sortParamKeys(method, undefined)).toStrictEqual([]); }); it('will return the params if provided with a params argument', () => { - const method = { params: [{ name: 'foo' }, { name: 'bar' }] }; + const method = ['foo', 'bar']; const params = ['dog', 'cat']; expect(sortParamKeys(method, params)).toStrictEqual(['dog', 'cat']); }); it('will return a by position-sorted array of params', () => { - const method = { params: [{ name: 'bar' }, { name: 'foo' }] }; + const method = ['bar', 'foo']; const params = { foo: 'val1', bar: 'val2' }; expect(sortParamKeys(method, params)).toStrictEqual(['val2', 'val1']); }); diff --git a/packages/execution-environments/src/common/sortParams.ts b/packages/execution-environments/src/common/sortParams.ts index b945f6948f..53f4a2679b 100644 --- a/packages/execution-environments/src/common/sortParams.ts +++ b/packages/execution-environments/src/common/sortParams.ts @@ -9,8 +9,7 @@ import { JsonRpcParams } from '@metamask/utils'; * * The order is defined by the `method` parameter. * - * @param method - The JSON-RPC method format. - * @param method.params - The parameters of the JSON-RPC method, which + * @param methodParams - The parameters of the JSON-RPC method, which * determines the ordering for the parameters. * @param params - JSON-RPC parameters as object or array. * @returns The values for the sorted keys. If `params` is not provided, this @@ -18,7 +17,7 @@ import { JsonRpcParams } from '@metamask/utils'; * `params`. */ export const sortParamKeys = ( - method: { params: { name: string }[] }, + methodParams: string[], params?: JsonRpcParams, ) => { if (!params) { @@ -29,15 +28,13 @@ export const sortParamKeys = ( return params; } - const methodParamsOrder: { [k: string]: number } = method.params - .map((param) => param.name) - .reduce( - (paramsOrderObj, paramsName, i) => ({ - ...paramsOrderObj, - [paramsName]: i, - }), - {}, - ); + const methodParamsOrder: { [k: string]: number } = methodParams.reduce( + (paramsOrderObj, paramsName, i) => ({ + ...paramsOrderObj, + [paramsName]: i, + }), + {}, + ); return Object.entries(params) .sort( diff --git a/packages/execution-environments/src/common/validation.ts b/packages/execution-environments/src/common/validation.ts index fe4f17ba31..fe25fb69ca 100644 --- a/packages/execution-environments/src/common/validation.ts +++ b/packages/execution-environments/src/common/validation.ts @@ -10,13 +10,15 @@ import { object, omit, optional, + record, string, + tuple, + union, unknown, } from 'superstruct'; import { Json, JsonRpcIdStruct, - JsonRpcRequest, JsonRpcRequestStruct, JsonRpcSuccess, JsonRpcSuccessStruct, @@ -111,57 +113,46 @@ export function isEndowmentsArray(value: unknown): value is Endowment[] { const OkStruct = literal('OK'); -const PingRequestStruct = assign( - JsonRpcRequestStruct, - object({ - method: literal('ping'), - params: optional(unknown()), - }), +export const PingRequestArgumentsStruct = optional( + union([literal(undefined), array()]), ); -const TerminateRequestStruct = assign( - JsonRpcRequestStruct, - object({ - method: literal('terminate'), - params: optional(unknown()), - }), -); - -const ExecuteSnapRequestStruct = assign( - JsonRpcRequestStruct, - object({ - method: literal('executeSnap'), - params: object({ - snapName: string(), - sourceCode: string(), - endowments: optional(array(EndowmentStruct)), +export const TerminateRequestStruct = union([literal(undefined), array()]); + +export const ExecuteSnapRequestArgumentsStruct = tuple([ + string(), + string(), + optional(array(EndowmentStruct)), +]); + +export const SnapRpcRequestArgumentsStruct = tuple([ + string(), + enums(Object.values(HandlerType)), + string(), + assign( + JsonRpcRequestWithoutIdStruct, + object({ + params: optional(record(string(), unknown())), }), - }), -); + ), +]); -const SnapRpcRequestStruct = assign( - JsonRpcRequestStruct, - object({ - method: literal('snapRpc'), - params: object({ - target: string(), - handler: enums(Object.values(HandlerType)), - origin: string(), - request: JsonRpcRequestStruct, - }), - }), -); +export type PingRequestArguments = Infer; +export type TerminateRequestArguments = Infer; -export type PingRequest = Infer; -export type TerminateRequest = Infer; -export type ExecuteSnapRequest = Infer; -export type SnapRpcRequest = Infer; +export type ExecuteSnapRequestArguments = Infer< + typeof ExecuteSnapRequestArgumentsStruct +>; -export type Request = - | PingRequest - | TerminateRequest - | ExecuteSnapRequest - | SnapRpcRequest; +export type SnapRpcRequestArguments = Infer< + typeof SnapRpcRequestArgumentsStruct +>; + +export type RequestArguments = + | PingRequestArguments + | TerminateRequestArguments + | ExecuteSnapRequestArguments + | SnapRpcRequestArguments; const OkResponseStruct = assign( JsonRpcSuccessStruct, @@ -177,51 +168,18 @@ export type SnapRpcResponse = Infer; export type Response = OkResponse | SnapRpcResponse; -// UnionToIntersection = A & B -type UnionToIntersection = ( - U extends unknown ? (arg: U) => 0 : never -) extends (arg: infer I) => 0 - ? I - : never; - -// LastInUnion = B -type LastInUnion = UnionToIntersection< - U extends unknown ? (x: U) => 0 : never -> extends (x: infer L) => 0 - ? L - : never; - -// UnionToTuple = [A, B] -type UnionToTuple> = [T] extends [never] - ? [] - : [Last, ...UnionToTuple>]; - -type InterfaceToTuple< - T, - U = UnionToTuple, - V extends any[] = [], -> = U extends [infer F, ...infer Rest] - ? F extends keyof T - ? InterfaceToTuple - : never - : V; - -type RequestParams = RequestType extends JsonRpcRequest< - infer Params -> - ? Params extends Record - ? InterfaceToTuple - : Params extends unknown[] - ? [...Params] - : [] - : []; +type RequestParams = + Params extends undefined ? [] : Params; type RequestFunction< - RequestType extends Request, + Args extends RequestArguments, ResponseType extends JsonRpcSuccess, -> = (...args: RequestParams) => Promise; +> = (...args: RequestParams) => Promise; -export type Ping = RequestFunction; -export type Terminate = RequestFunction; -export type ExecuteSnap = RequestFunction; -export type SnapRpc = RequestFunction; +export type Ping = RequestFunction; +export type Terminate = RequestFunction; +export type ExecuteSnap = RequestFunction< + ExecuteSnapRequestArguments, + OkResponse +>; +export type SnapRpc = RequestFunction; From 13538d93ba5c67d87ccb2dc8304bde28aba9168f Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Wed, 12 Oct 2022 12:35:58 +0200 Subject: [PATCH 3/6] Remove unused type generation --- packages/execution-environments/package.json | 6 +- yarn.lock | 147 +------------------ 2 files changed, 4 insertions(+), 149 deletions(-) diff --git a/packages/execution-environments/package.json b/packages/execution-environments/package.json index 01172d40bf..975fb64ac4 100644 --- a/packages/execution-environments/package.json +++ b/packages/execution-environments/package.json @@ -23,11 +23,9 @@ "lint": "yarn lint:eslint && yarn lint:misc --check", "clean": "rimraf '*.tsbuildinfo' 'dist/*' 'src/__GENERATED__/' 'coverage/*' '__test__/*'", "build:clean": "yarn clean && yarn build", - "build:pre-tsc": "yarn build:typings", "build:post-tsc": "webpack --mode production && concat -o ./dist/webpack/node-process/bundle.js ./dist/webpack/node-process/lockdown.umd.min.js ./dist/webpack/node-process/bundle.js && concat -o ./dist/webpack/node-thread/bundle.js ./dist/webpack/node-thread/lockdown.umd.min.js ./dist/webpack/node-thread/bundle.js", "build:tsc": "tsc --project tsconfig.local.json", - "build": "yarn build:pre-tsc && yarn build:tsc && yarn build:post-tsc", - "build:typings": "open-rpc-typings -d ./src/openrpc.json --output-ts=./src/__GENERATED__/ --name-ts=openrpc && ts-auto-guard ./src/__GENERATED__/openrpc.ts --export-all", + "build": "yarn build:tsc && yarn build:post-tsc", "auto-changelog-init": "auto-changelog init", "publish:package": "../../scripts/publish-package.sh" }, @@ -51,7 +49,6 @@ "@metamask/eslint-config-jest": "^9.0.0", "@metamask/eslint-config-nodejs": "^9.0.0", "@metamask/eslint-config-typescript": "^9.0.1", - "@open-rpc/typings": "^1.12.1", "@types/jest": "^27.5.1", "@types/node": "^17.0.36", "concat": "^1.0.3", @@ -73,7 +70,6 @@ "prettier": "^2.3.2", "prettier-plugin-packagejson": "^2.2.11", "rimraf": "^3.0.2", - "ts-auto-guard": "^2.3.0", "ts-jest": "^29.0.0", "ts-loader": "^9.3.1", "typescript": "^4.4.0", diff --git a/yarn.lock b/yarn.lock index 673242524e..fab0126f9c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2514,41 +2514,6 @@ __metadata: languageName: node linkType: hard -"@json-schema-spec/json-pointer@npm:^0.1.2": - version: 0.1.2 - resolution: "@json-schema-spec/json-pointer@npm:0.1.2" - checksum: 2a691ffc11f1a266ca4d0c9e2c99791679d580f343ef69746fad623d1abcf4953adde987890e41f906767d7729604c0182341e9012388b73a44d5b21fb296453 - languageName: node - linkType: hard - -"@json-schema-tools/dereferencer@npm:1.5.1": - version: 1.5.1 - resolution: "@json-schema-tools/dereferencer@npm:1.5.1" - dependencies: - "@json-schema-tools/reference-resolver": ^1.2.1 - "@json-schema-tools/traverse": ^1.7.5 - fast-safe-stringify: ^2.0.7 - checksum: 3ac85af3944b392e8fb3bbe6b0c25a2024fca016f3b5f12f0bbbd6074652c7ee0f55b1f0cdc1b005e4b22e35ca3b7a75e63b329e09c2906c2eb0355aea8dfc1d - languageName: node - linkType: hard - -"@json-schema-tools/meta-schema@npm:^1.6.10": - version: 1.6.19 - resolution: "@json-schema-tools/meta-schema@npm:1.6.19" - checksum: 0105281416ee79fe027b10b952eec02e21466a7a0ffffc5e280d7d32ec8776536fd420456d0b4f605ecb1bef04efe3e35e75687aa3b1a256e8825078d4843233 - languageName: node - linkType: hard - -"@json-schema-tools/reference-resolver@npm:^1.2.1": - version: 1.2.3 - resolution: "@json-schema-tools/reference-resolver@npm:1.2.3" - dependencies: - "@json-schema-spec/json-pointer": ^0.1.2 - isomorphic-fetch: ^3.0.0 - checksum: 0d423cfa8ad44a68c41b71b6fcbc08106a9591ea02e0b57b01271e27e8fc1d52243830218a9675e3b8ecc4d381da33888b0367e94b09c1c071f689b2a58c62ed - languageName: node - linkType: hard - "@json-schema-tools/referencer@npm:^1.0.4": version: 1.0.5 resolution: "@json-schema-tools/referencer@npm:1.0.5" @@ -2558,7 +2523,7 @@ __metadata: languageName: node linkType: hard -"@json-schema-tools/titleizer@npm:1.0.6, @json-schema-tools/titleizer@npm:^1.0.5": +"@json-schema-tools/titleizer@npm:^1.0.5": version: 1.0.6 resolution: "@json-schema-tools/titleizer@npm:1.0.6" dependencies: @@ -2582,7 +2547,7 @@ __metadata: languageName: node linkType: hard -"@json-schema-tools/traverse@npm:^1.7.5, @json-schema-tools/traverse@npm:^1.7.8, @json-schema-tools/traverse@npm:^1.8.0": +"@json-schema-tools/traverse@npm:^1.7.8, @json-schema-tools/traverse@npm:^1.8.0": version: 1.8.1 resolution: "@json-schema-tools/traverse@npm:1.8.1" checksum: aaf4ea22ea8974d60a9526169093de6ad992cd20caed340549a2b0cd8b93573c0933280f6b25c6f6f3db49d5c19fb0efbf4fb87ad712b945cc6e04b980c954b6 @@ -2850,7 +2815,6 @@ __metadata: "@metamask/snap-types": ^0.22.3 "@metamask/snap-utils": ^0.22.3 "@metamask/utils": ^3.1.0 - "@open-rpc/typings": ^1.12.1 "@types/jest": ^27.5.1 "@types/node": ^17.0.36 concat: ^1.0.3 @@ -2877,7 +2841,6 @@ __metadata: ses: ^0.15.15 stream-browserify: ^3.0.0 superstruct: ^0.16.5 - ts-auto-guard: ^2.3.0 ts-jest: ^29.0.0 ts-loader: ^9.3.1 typescript: ^4.4.0 @@ -3487,46 +3450,6 @@ __metadata: languageName: node linkType: hard -"@open-rpc/meta-schema@npm:1.14.2": - version: 1.14.2 - resolution: "@open-rpc/meta-schema@npm:1.14.2" - checksum: 63d3672aeea6941a96522ab2b7a9f95c4b3216ea6d7b700bdbb0ca0595b0ad28e439818d22788991a36403953e39cda9f8c7a99ed5c18258b0a45b815ecfaf8b - languageName: node - linkType: hard - -"@open-rpc/schema-utils-js@npm:1.15.0": - version: 1.15.0 - resolution: "@open-rpc/schema-utils-js@npm:1.15.0" - dependencies: - "@json-schema-tools/dereferencer": 1.5.1 - "@json-schema-tools/meta-schema": ^1.6.10 - "@json-schema-tools/reference-resolver": ^1.2.1 - "@open-rpc/meta-schema": 1.14.2 - ajv: ^6.10.0 - detect-node: ^2.0.4 - fast-safe-stringify: ^2.0.7 - fs-extra: ^9.0.0 - is-url: ^1.2.4 - isomorphic-fetch: ^3.0.0 - checksum: fc2295999f37dd55d4e1d18808d83969a8b26aaf39c8327cb57bd388ff87496e8732fb03126b52fe75a5cacc10b4ef77d95719ed468971090d7112d2ba9aa94d - languageName: node - linkType: hard - -"@open-rpc/typings@npm:^1.12.1": - version: 1.12.1 - resolution: "@open-rpc/typings@npm:1.12.1" - dependencies: - "@json-schema-tools/titleizer": 1.0.6 - "@json-schema-tools/transpiler": ^1.10.2 - "@open-rpc/schema-utils-js": 1.15.0 - commander: ^6.0.0 - fs-extra: ^10.0.0 - bin: - open-rpc-typings: build/cli.js - checksum: 6b9d98d54e13e8ad4f9368651ae0adf3ba14e8b854ee79897f42243b45430f7cc1182ec8d1c40f8b82cc15042894c580e068435ef39d39535d8f4292655475ee - languageName: node - linkType: hard - "@peculiar/asn1-schema@npm:^2.1.6": version: 2.1.8 resolution: "@peculiar/asn1-schema@npm:2.1.8" @@ -5411,13 +5334,6 @@ __metadata: languageName: node linkType: hard -"at-least-node@npm:^1.0.0": - version: 1.0.0 - resolution: "at-least-node@npm:1.0.0" - checksum: 463e2f8e43384f1afb54bc68485c436d7622acec08b6fad269b421cb1d29cebb5af751426793d0961ed243146fe4dc983402f6d5a51b720b277818dbf6f2e49e - languageName: node - linkType: hard - "atob@npm:^2.1.2": version: 2.1.2 resolution: "atob@npm:2.1.2" @@ -6812,13 +6728,6 @@ __metadata: languageName: node linkType: hard -"commander@npm:^6.0.0": - version: 6.2.1 - resolution: "commander@npm:6.2.1" - checksum: d7090410c0de6bc5c67d3ca41c41760d6d268f3c799e530aafb73b7437d1826bbf0d2a3edac33f8b57cc9887b4a986dce307fa5557e109be40eadb7c43b21742 - languageName: node - linkType: hard - "commander@npm:^7.0.0": version: 7.2.0 resolution: "commander@npm:7.2.0" @@ -7486,13 +7395,6 @@ __metadata: languageName: node linkType: hard -"detect-node@npm:^2.0.4": - version: 2.0.5 - resolution: "detect-node@npm:2.0.5" - checksum: 1a53096f6c3d1c4a82db2e97440d7a38eff7d8e635a5613ac1fff870135b709c5a4e6af789cda17bd6f4b6f036d30a913a0fe2dec125b565f5f7767e3e401c46 - languageName: node - linkType: hard - "detective@npm:^5.2.0": version: 5.2.0 resolution: "detective@npm:5.2.0" @@ -9546,29 +9448,6 @@ __metadata: languageName: node linkType: hard -"fs-extra@npm:^10.0.0": - version: 10.0.0 - resolution: "fs-extra@npm:10.0.0" - dependencies: - graceful-fs: ^4.2.0 - jsonfile: ^6.0.1 - universalify: ^2.0.0 - checksum: 5285a3d8f34b917cf2b66af8c231a40c1623626e9d701a20051d3337be16c6d7cac94441c8b3732d47a92a2a027886ca93c69b6a4ae6aee3c89650d2a8880c0a - languageName: node - linkType: hard - -"fs-extra@npm:^9.0.0": - version: 9.1.0 - resolution: "fs-extra@npm:9.1.0" - dependencies: - at-least-node: ^1.0.0 - graceful-fs: ^4.2.0 - jsonfile: ^6.0.1 - universalify: ^2.0.0 - checksum: ba71ba32e0faa74ab931b7a0031d1523c66a73e225de7426e275e238e312d07313d2da2d33e34a52aa406c8763ade5712eb3ec9ba4d9edce652bcacdc29e6b20 - languageName: node - linkType: hard - "fs-minipass@npm:^2.0.0, fs-minipass@npm:^2.1.0": version: 2.1.0 resolution: "fs-minipass@npm:2.1.0" @@ -9971,7 +9850,7 @@ __metadata: languageName: node linkType: hard -"graceful-fs@npm:^4.0.0, graceful-fs@npm:^4.1.11, graceful-fs@npm:^4.1.2, graceful-fs@npm:^4.1.6, graceful-fs@npm:^4.2.0, graceful-fs@npm:^4.2.3, graceful-fs@npm:^4.2.4, graceful-fs@npm:^4.2.6, graceful-fs@npm:^4.2.9": +"graceful-fs@npm:^4.0.0, graceful-fs@npm:^4.1.11, graceful-fs@npm:^4.1.2, graceful-fs@npm:^4.1.6, graceful-fs@npm:^4.2.3, graceful-fs@npm:^4.2.4, graceful-fs@npm:^4.2.6, graceful-fs@npm:^4.2.9": version: 4.2.10 resolution: "graceful-fs@npm:4.2.10" checksum: 3f109d70ae123951905d85032ebeae3c2a5a7a997430df00ea30df0e3a6c60cf6689b109654d6fdacd28810a053348c4d14642da1d075049e6be1ba5216218da @@ -11976,19 +11855,6 @@ __metadata: languageName: node linkType: hard -"jsonfile@npm:^6.0.1": - version: 6.1.0 - resolution: "jsonfile@npm:6.1.0" - dependencies: - graceful-fs: ^4.1.6 - universalify: ^2.0.0 - dependenciesMeta: - graceful-fs: - optional: true - checksum: 7af3b8e1ac8fe7f1eccc6263c6ca14e1966fcbc74b618d3c78a0a2075579487547b94f72b7a1114e844a1e15bb00d440e5d1720bfc4612d790a6f285d5ea8354 - languageName: node - linkType: hard - "jsonify@npm:~0.0.0": version: 0.0.0 resolution: "jsonify@npm:0.0.0" @@ -17117,13 +16983,6 @@ __metadata: languageName: node linkType: hard -"universalify@npm:^2.0.0": - version: 2.0.0 - resolution: "universalify@npm:2.0.0" - checksum: 2406a4edf4a8830aa6813278bab1f953a8e40f2f63a37873ffa9a3bc8f9745d06cc8e88f3572cb899b7e509013f7f6fcc3e37e8a6d914167a5381d8440518c44 - languageName: node - linkType: hard - "unset-value@npm:^1.0.0": version: 1.0.0 resolution: "unset-value@npm:1.0.0" From 8a7c04b622c578c28be2b292c2d9f7e820959672 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 31 Oct 2022 09:22:50 +0100 Subject: [PATCH 4/6] Validate handler arguments --- .../execution-environments/jest.config.js | 8 +- .../src/common/commands.test.ts | 27 +++++++ .../src/common/commands.ts | 10 ++- .../src/common/validation.test.ts | 75 +++++++++++++++++++ .../src/common/validation.ts | 43 +++++++---- yarn.lock | 61 --------------- 6 files changed, 143 insertions(+), 81 deletions(-) create mode 100644 packages/execution-environments/src/common/validation.test.ts diff --git a/packages/execution-environments/jest.config.js b/packages/execution-environments/jest.config.js index e6ccba9652..b842133215 100644 --- a/packages/execution-environments/jest.config.js +++ b/packages/execution-environments/jest.config.js @@ -6,10 +6,10 @@ module.exports = { coverageReporters: ['clover', 'json', 'lcov', 'text', 'json-summary'], coverageThreshold: { global: { - branches: 83.51, - functions: 92.64, - lines: 86.37, - statements: 86.54, + branches: 83.79, + functions: 92.48, + lines: 86.53, + statements: 86.7, }, }, moduleFileExtensions: ['js', 'json', 'jsx', 'ts', 'tsx', 'node'], diff --git a/packages/execution-environments/src/common/commands.test.ts b/packages/execution-environments/src/common/commands.test.ts index 7be66f33ec..a70b8de8e0 100644 --- a/packages/execution-environments/src/common/commands.test.ts +++ b/packages/execution-environments/src/common/commands.test.ts @@ -1,9 +1,36 @@ import { HandlerType } from '@metamask/snap-utils'; +import { MOCK_ORIGIN } from '@metamask/snap-utils/test-utils'; import { CommandMethodsMapping, getCommandMethodImplementations, + getHandlerArguments, } from './commands'; +describe('getHandlerArguments', () => { + it('validates the request params for the OnTransaction handler', () => { + expect(() => + getHandlerArguments(MOCK_ORIGIN, HandlerType.OnTransaction, { + id: 1, + jsonrpc: '2.0', + method: 'foo', + params: {}, + }), + ).toThrow('Invalid request params'); + }); + + it('throws for invalid handler types', () => { + expect(() => + // @ts-expect-error Invalid handler type. + getHandlerArguments(MOCK_ORIGIN, 'foo', { + id: 1, + jsonrpc: '2.0', + method: 'foo', + params: {}, + }), + ).toThrow('Invalid branch reached. Should be detected during compilation.'); + }); +}); + describe('getCommandMethodImplementations', () => { it('will return an object with ping, executeSnap, snapRpc and terminate methods', () => { const startSnap = jest.fn(); diff --git a/packages/execution-environments/src/common/commands.ts b/packages/execution-environments/src/common/commands.ts index 854b2bbe8c..c5b607faaa 100644 --- a/packages/execution-environments/src/common/commands.ts +++ b/packages/execution-environments/src/common/commands.ts @@ -2,6 +2,7 @@ import { assertExhaustive } from '@metamask/utils'; import { HandlerType } from '@metamask/snap-utils'; import { InvokeSnap, InvokeSnapArgs } from './BaseSnapExecutor'; import { + assertIsOnTransactionRequestArguments, ExecuteSnap, JsonRpcRequestWithoutId, Ping, @@ -16,7 +17,6 @@ export type CommandMethodsMapping = { snapRpc: SnapRpc; }; -// TODO: Add validation in cases. /** * Formats the arguments for the given handler. * @@ -25,14 +25,18 @@ export type CommandMethodsMapping = { * @param request - The request object. * @returns The formatted arguments. */ -function getHandlerArguments( +export function getHandlerArguments( origin: string, handler: HandlerType, request: JsonRpcRequestWithoutId, ): InvokeSnapArgs { + // `request` is already validated by the time this function is called. + switch (handler) { case HandlerType.OnTransaction: { - const { transaction, chainId } = request.params as Record; + assertIsOnTransactionRequestArguments(request.params); + + const { transaction, chainId } = request.params; return { transaction, chainId, diff --git a/packages/execution-environments/src/common/validation.test.ts b/packages/execution-environments/src/common/validation.test.ts new file mode 100644 index 0000000000..244cc199d5 --- /dev/null +++ b/packages/execution-environments/src/common/validation.test.ts @@ -0,0 +1,75 @@ +import { + assertIsOnTransactionRequestArguments, + isEndowment, + isEndowmentsArray, +} from './validation'; + +describe('isEndowment', () => { + it.each(['foo', 'bar', 'baz'])('returns true for %s', (value) => { + expect(isEndowment(value)).toBe(true); + }); + + it.each([true, false, null, undefined, 0, 1, [], {}])( + 'returns false for %s', + (value) => { + expect(isEndowment(value)).toBe(false); + }, + ); +}); + +describe('isEndowmentsArray', () => { + it.each([ + { array: ['foo', 'bar', 'baz'] }, + { array: ['foo', 'bar', 'baz', 'qux'] }, + ])('returns true for a valid endowments array', ({ array }) => { + expect(isEndowmentsArray(array)).toBe(true); + }); + + it.each([ + { array: [true, false, null, undefined, 0, 1, [], {}] }, + { array: ['foo', 'bar', 'baz', 0] }, + { array: ['foo', 'bar', 'baz', true] }, + { array: ['foo', 'bar', 'baz', false] }, + { array: ['foo', 'bar', 'baz', null] }, + { array: ['foo', 'bar', 'baz', undefined] }, + { array: ['foo', 'bar', 'baz', []] }, + { array: ['foo', 'bar', 'baz', {}] }, + ])('returns false for an invalid endowments array', (value) => { + expect(isEndowmentsArray(value)).toBe(false); + }); +}); + +describe('assertIsOnTransactionRequestArguments', () => { + it.each([ + { transaction: {}, chainId: 'eip155:1' }, + { + transaction: { foo: 'bar' }, + chainId: 'bip122:000000000019d6689c085ae165831e93', + }, + { transaction: { bar: 'baz' }, chainId: 'eip155:2' }, + ])('does not throw for a valid transaction params object', (args) => { + expect(() => assertIsOnTransactionRequestArguments(args)).not.toThrow(); + }); + + it.each([ + true, + false, + null, + undefined, + 0, + 1, + '', + 'foo', + [], + {}, + { transaction: { foo: 'bar' }, chainId: 1 }, + { transaction: { foo: 'bar' }, chainId: '0x1', foo: 'bar' }, + ])( + 'throws if the value is not a valid transaction params object', + (value) => { + expect(() => assertIsOnTransactionRequestArguments(value as any)).toThrow( + 'Invalid request params:', + ); + }, + ); +}); diff --git a/packages/execution-environments/src/common/validation.ts b/packages/execution-environments/src/common/validation.ts index fe25fb69ca..eace8c372b 100644 --- a/packages/execution-environments/src/common/validation.ts +++ b/packages/execution-environments/src/common/validation.ts @@ -1,4 +1,4 @@ -import { HandlerType } from '@metamask/snap-utils'; +import { assertStruct, ChainIdStruct, HandlerType } from '@metamask/snap-utils'; import { SnapKeyring } from '@metamask/snap-types'; import { array, @@ -22,6 +22,7 @@ import { JsonRpcRequestStruct, JsonRpcSuccess, JsonRpcSuccessStruct, + JsonStruct, } from '@metamask/utils'; const VALIDATION_FUNCTIONS = { @@ -76,18 +77,6 @@ export type JsonRpcRequestWithoutId = Infer< typeof JsonRpcRequestWithoutIdStruct >; -/** - * Check if the given value is a JSON-RPC request with an optional ID. - * - * @param value - The value to check. - * @returns Whether the value is a JSON-RPC request with an optional ID. - */ -export function isJsonRpcRequestWithoutId( - value: unknown, -): value is JsonRpcRequestWithoutId { - return is(value, JsonRpcRequestWithoutIdStruct); -} - export const EndowmentStruct = string(); export type Endowment = Infer; @@ -154,6 +143,34 @@ export type RequestArguments = | ExecuteSnapRequestArguments | SnapRpcRequestArguments; +export const OnTransactionRequestArgumentsStruct = object({ + // TODO: Improve `transaction` type. + transaction: record(string(), JsonStruct), + chainId: ChainIdStruct, +}); + +export type OnTransactionRequestArguments = Infer< + typeof OnTransactionRequestArgumentsStruct +>; + +/** + * Asserts that the given value is a valid {@link OnTransactionRequestArguments} + * object. + * + * @param value - The value to validate. + * @throws If the value is not a valid {@link OnTransactionRequestArguments} + * object. + */ +export function assertIsOnTransactionRequestArguments( + value: unknown, +): asserts value is OnTransactionRequestArguments { + assertStruct( + value, + OnTransactionRequestArgumentsStruct, + 'Invalid request params', + ); +} + const OkResponseStruct = assign( JsonRpcSuccessStruct, object({ diff --git a/yarn.lock b/yarn.lock index fab0126f9c..d1ebfdb1f2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2514,46 +2514,6 @@ __metadata: languageName: node linkType: hard -"@json-schema-tools/referencer@npm:^1.0.4": - version: 1.0.5 - resolution: "@json-schema-tools/referencer@npm:1.0.5" - dependencies: - "@json-schema-tools/traverse": ^1.7.8 - checksum: 99b09adc1e068d07d2a7d6fefc2c20471c53715a512dc85b6e288a5a001fdc508d1d260484eb4df64d90f7927efa11d2665ace20a56ae81e4f842611e2cd2041 - languageName: node - linkType: hard - -"@json-schema-tools/titleizer@npm:^1.0.5": - version: 1.0.6 - resolution: "@json-schema-tools/titleizer@npm:1.0.6" - dependencies: - "@json-schema-tools/traverse": ^1.7.8 - checksum: 32bfcd0e6b212dd605772f605bc06455698d176c518cb11aa68fd6a7992ed179bea220c1d325be310b7d071c7e500d10bab5700eb8f7eabff4717cae0dca149f - languageName: node - linkType: hard - -"@json-schema-tools/transpiler@npm:^1.10.2": - version: 1.10.2 - resolution: "@json-schema-tools/transpiler@npm:1.10.2" - dependencies: - "@json-schema-tools/referencer": ^1.0.4 - "@json-schema-tools/titleizer": ^1.0.5 - "@json-schema-tools/traverse": ^1.8.0 - lodash.camelcase: ^4.3.0 - lodash.deburr: ^4.1.0 - lodash.snakecase: ^4.1.1 - lodash.trim: ^4.5.1 - checksum: a34d8a39da958823ad508aa31fb19f950a35220942f2268fdd2d39998080a3cb037708fec29a60e90ecf48d0e154b880b98d396a58a396f1cdef996f0e798a60 - languageName: node - linkType: hard - -"@json-schema-tools/traverse@npm:^1.7.8, @json-schema-tools/traverse@npm:^1.8.0": - version: 1.8.1 - resolution: "@json-schema-tools/traverse@npm:1.8.1" - checksum: aaf4ea22ea8974d60a9526169093de6ad992cd20caed340549a2b0cd8b93573c0933280f6b25c6f6f3db49d5c19fb0efbf4fb87ad712b945cc6e04b980c954b6 - languageName: node - linkType: hard - "@keystonehq/base-eth-keyring@npm:^0.7.1": version: 0.7.1 resolution: "@keystonehq/base-eth-keyring@npm:0.7.1" @@ -12239,13 +12199,6 @@ __metadata: languageName: node linkType: hard -"lodash.deburr@npm:^4.1.0": - version: 4.1.0 - resolution: "lodash.deburr@npm:4.1.0" - checksum: 6e2012315c20a4d8ed4f1884ed4b8e6b0093c6355a87bfd95ecf25a5243c8c88d747d67375d52cb87ebc99d090935ed8dc3814c8e661e3275a6dbe02b68efc99 - languageName: node - linkType: hard - "lodash.memoize@npm:4.x": version: 4.1.2 resolution: "lodash.memoize@npm:4.1.2" @@ -12267,20 +12220,6 @@ __metadata: languageName: node linkType: hard -"lodash.snakecase@npm:^4.1.1": - version: 4.1.1 - resolution: "lodash.snakecase@npm:4.1.1" - checksum: 1685ed3e83dda6eae5a4dcaee161a51cd210aabb3e1c09c57150e7dd8feda19e4ca0d27d0631eabe8d0f4eaa51e376da64e8c018ae5415417c5890d42feb72a8 - languageName: node - linkType: hard - -"lodash.trim@npm:^4.5.1": - version: 4.5.1 - resolution: "lodash.trim@npm:4.5.1" - checksum: 64b08e97d94d4c7620159371e6fe6cbb706514a41d737db2f189d9ec738305eb08cb772a9bbd2459e90f1c22f96174ec1047ceb8272f2f6040cb5bd63d8f9f2b - languageName: node - linkType: hard - "lodash.truncate@npm:^4.4.2": version: 4.4.2 resolution: "lodash.truncate@npm:4.4.2" From 1425abe187bff96b8e3e8a3697556657cb7a40c6 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 31 Oct 2022 12:20:04 +0100 Subject: [PATCH 5/6] Fix PR comments --- .../src/common/BaseSnapExecutor.ts | 12 ++++++------ .../execution-environments/src/common/validation.ts | 9 +++++++-- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.ts b/packages/execution-environments/src/common/BaseSnapExecutor.ts index f822071e40..497faf9cd8 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.ts @@ -36,7 +36,7 @@ import { ExecuteSnapRequestArgumentsStruct, PingRequestArgumentsStruct, SnapRpcRequestArgumentsStruct, - TerminateRequestStruct, + TerminateRequestArgumentsStruct, validateExport, } from './validation'; @@ -72,19 +72,19 @@ export type InvokeSnap = ( */ const EXECUTION_ENVIRONMENT_METHODS = { ping: { - validator: PingRequestArgumentsStruct, + struct: PingRequestArgumentsStruct, params: [], }, executeSnap: { - validator: ExecuteSnapRequestArgumentsStruct, + struct: ExecuteSnapRequestArgumentsStruct, params: ['snapName', 'sourceCode', 'endowments'], }, terminate: { - validator: TerminateRequestStruct, + struct: TerminateRequestArgumentsStruct, params: [], }, snapRpc: { - validator: SnapRpcRequestArgumentsStruct, + struct: SnapRpcRequestArgumentsStruct, params: ['target', 'handler', 'origin', 'request'], }, }; @@ -184,7 +184,7 @@ export class BaseSnapExecutor { // support params by-name and by-position const paramsAsArray = sortParamKeys(methodObject.params, params); - const [error] = validate(paramsAsArray, methodObject.validator); + const [error] = validate(paramsAsArray, methodObject.struct); if (error) { this.respond(id, { error: ethErrors.rpc diff --git a/packages/execution-environments/src/common/validation.ts b/packages/execution-environments/src/common/validation.ts index eace8c372b..b3d393bf17 100644 --- a/packages/execution-environments/src/common/validation.ts +++ b/packages/execution-environments/src/common/validation.ts @@ -106,7 +106,10 @@ export const PingRequestArgumentsStruct = optional( union([literal(undefined), array()]), ); -export const TerminateRequestStruct = union([literal(undefined), array()]); +export const TerminateRequestArgumentsStruct = union([ + literal(undefined), + array(), +]); export const ExecuteSnapRequestArgumentsStruct = tuple([ string(), @@ -127,7 +130,9 @@ export const SnapRpcRequestArgumentsStruct = tuple([ ]); export type PingRequestArguments = Infer; -export type TerminateRequestArguments = Infer; +export type TerminateRequestArguments = Infer< + typeof TerminateRequestArgumentsStruct +>; export type ExecuteSnapRequestArguments = Infer< typeof ExecuteSnapRequestArgumentsStruct From 9dd2f91eb1c37b4bc2433ecadfdaa5ed05313ff3 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 31 Oct 2022 12:39:31 +0100 Subject: [PATCH 6/6] Improve error message and add test --- .../src/common/BaseSnapExecutor.test.ts | 35 +++++++++++++++++-- .../src/common/BaseSnapExecutor.ts | 4 +-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts index c05017709d..7a63bc1444 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts @@ -1581,7 +1581,7 @@ describe('BaseSnapExecutor', () => { jsonrpc: '2.0', id: 1, error: { - code: -32600, + code: -32602, data: expect.any(Object), message: expect.any(String), stack: expect.any(String), @@ -1652,7 +1652,7 @@ describe('BaseSnapExecutor', () => { jsonrpc: '2.0', id: 1, error: { - code: -32600, + code: -32602, data: expect.any(Object), message: expect.any(String), stack: expect.any(String), @@ -1661,4 +1661,35 @@ describe('BaseSnapExecutor', () => { }, ); }); + + describe('onCommandRequest', () => { + it('throws a human-readable error if the request arguments are invalid', async () => { + const executor = new TestSnapExecutor(); + const params = { + snapName: 1, + method: ON_RPC_REQUEST, + origin: FAKE_ORIGIN, + request: { jsonrpc: '2.0', method: '', params: [] }, + }; + + await executor.writeCommand({ + jsonrpc: '2.0', + id: 1, + method: 'snapRpc', + params, + }); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + id: 1, + error: { + code: -32602, + data: expect.any(Object), + message: + 'Invalid parameters for method "snapRpc": At path: 0 -- Expected a string, but received: 1.', + stack: expect.any(String), + }, + }); + }); + }); }); diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.ts b/packages/execution-environments/src/common/BaseSnapExecutor.ts index 497faf9cd8..a9d926cb59 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.ts @@ -188,8 +188,8 @@ export class BaseSnapExecutor { if (error) { this.respond(id, { error: ethErrors.rpc - .invalidRequest({ - message: error.message, + .invalidParams({ + message: `Invalid parameters for method "${method}": ${error.message}.`, data: { method, params: paramsAsArray,