From bb0c266f7ae9dedb753e4954b10d7693cc876d5f Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Tue, 19 Jul 2022 00:45:03 -0700 Subject: [PATCH] Send valid JSON-RPC notifications from executors (#639) `BaseSnapExecutor.notify()` was sending invalid [JSON-RPC notifications](https://www.jsonrpc.org/specification#notification) to the execution service over the command stream. This PR ensures that only valid notifications are sent. --- packages/controllers/jest.config.js | 6 +- .../services/AbstractExecutionService.test.ts | 112 ++++++++++++++++++ .../src/services/AbstractExecutionService.ts | 48 ++++++-- .../services/iframe/IframeExecutionService.ts | 11 +- .../execution-environments/jest.config.js | 2 +- .../src/common/BaseSnapExecutor.test.ts | 49 ++++---- .../src/common/BaseSnapExecutor.ts | 26 ++-- .../src/common/commands.ts | 2 +- 8 files changed, 203 insertions(+), 53 deletions(-) create mode 100644 packages/controllers/src/services/AbstractExecutionService.test.ts diff --git a/packages/controllers/jest.config.js b/packages/controllers/jest.config.js index 92b61bda6f..c87a0498d8 100644 --- a/packages/controllers/jest.config.js +++ b/packages/controllers/jest.config.js @@ -8,10 +8,10 @@ module.exports = { coveragePathIgnorePatterns: ['/node_modules/', '/mocks/', '/test/'], coverageThreshold: { global: { - branches: 72.56, + branches: 72.14, functions: 86.41, - lines: 85.02, - statements: 85.1, + lines: 85.06, + statements: 85.15, }, }, globals: { diff --git a/packages/controllers/src/services/AbstractExecutionService.test.ts b/packages/controllers/src/services/AbstractExecutionService.test.ts new file mode 100644 index 0000000000..5428f50a76 --- /dev/null +++ b/packages/controllers/src/services/AbstractExecutionService.test.ts @@ -0,0 +1,112 @@ +import { ControllerMessenger } from '@metamask/controllers'; +import { + ErrorMessageEvent, + ExecutionServiceMessenger, +} from './ExecutionService'; +import { NodeThreadExecutionService } from './node'; + +class MockExecutionService extends NodeThreadExecutionService { + constructor(messenger: ExecutionServiceMessenger) { + super({ + messenger, + setupSnapProvider: () => undefined, + }); + } + + getJobs() { + return this.jobs; + } +} + +describe('AbstractExecutionService', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('logs error for unrecognized notifications', async () => { + const consoleErrorSpy = jest.spyOn(console, 'error'); + + const controllerMessenger = new ControllerMessenger< + never, + ErrorMessageEvent + >(); + const service = new MockExecutionService( + controllerMessenger.getRestricted< + 'ExecutionService', + never, + ErrorMessageEvent['type'] + >({ + name: 'ExecutionService', + }), + ); + + await service.executeSnap({ + snapId: 'TestSnap', + sourceCode: ` + console.log('foo'); + `, + endowments: ['console'], + }); + + const { streams } = service.getJobs().values().next().value; + streams.command.emit('data', { + jsonrpc: '2.0', + method: 'foo', + }); + + await service.terminateAllSnaps(); + + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); + expect(consoleErrorSpy).toHaveBeenCalledWith( + new Error(`Received unexpected command stream notification "foo".`), + ); + }); + + it('logs error for malformed UnhandledError notification', async () => { + const consoleErrorSpy = jest.spyOn(console, 'error'); + + const controllerMessenger = new ControllerMessenger< + never, + ErrorMessageEvent + >(); + const service = new MockExecutionService( + controllerMessenger.getRestricted< + 'ExecutionService', + never, + ErrorMessageEvent['type'] + >({ + name: 'ExecutionService', + }), + ); + + await service.executeSnap({ + snapId: 'TestSnap', + sourceCode: ` + console.log('foo'); + `, + endowments: ['console'], + }); + + const { streams } = service.getJobs().values().next().value; + streams.command.emit('data', { + jsonrpc: '2.0', + method: 'UnhandledError', + params: 2, + }); + + streams.command.emit('data', { + jsonrpc: '2.0', + method: 'UnhandledError', + params: { error: null }, + }); + + await service.terminateAllSnaps(); + + const expectedError = new Error( + `Received malformed "UnhandledError" command stream notification.`, + ); + expect(consoleErrorSpy).toHaveBeenCalledTimes(2); + expect(consoleErrorSpy).toHaveBeenNthCalledWith(1, expectedError); + expect(consoleErrorSpy).toHaveBeenNthCalledWith(2, expectedError); + }); +}); diff --git a/packages/controllers/src/services/AbstractExecutionService.ts b/packages/controllers/src/services/AbstractExecutionService.ts index ab29450489..7a354b67a1 100644 --- a/packages/controllers/src/services/AbstractExecutionService.ts +++ b/packages/controllers/src/services/AbstractExecutionService.ts @@ -1,8 +1,13 @@ import { Duplex } from 'stream'; import ObjectMultiplex from '@metamask/object-multiplex'; -import { SnapExecutionData } from '@metamask/snap-types'; +import { ErrorJSON, SnapExecutionData } from '@metamask/snap-types'; import { SNAP_STREAM_NAMES } from '@metamask/execution-environments'; -import { Duration } from '@metamask/utils'; +import { + Duration, + isJsonRpcRequest, + isObject, + JsonRpcNotification, +} from '@metamask/utils'; import { JsonRpcEngine, // TODO: Replace with @metamask/utils version after bumping json-rpc-engine @@ -23,7 +28,7 @@ const controllerName = 'ExecutionService'; export type SetupSnapProvider = (snapId: string, stream: Duplex) => void; -type ExecutionServiceArgs = { +export type ExecutionServiceArgs = { setupSnapProvider: SetupSnapProvider; messenger: ExecutionServiceMessenger; terminationTimeout?: number; @@ -217,23 +222,40 @@ export abstract class AbstractExecutionService // Handle out-of-band errors, i.e. errors thrown from the snap outside of the req/res cycle. // Also keep track of outbound request/responses - const notificationHandler = (data: any) => { - if (data.id !== null && data.id !== undefined) { + const notificationHandler = ( + message: JsonRpcRequest | JsonRpcNotification, + ) => { + if (isJsonRpcRequest(message)) { return; } + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const snapId = this.jobToSnapMap.get(jobId)!; - if (data.result?.type === 'OutboundRequest') { + if (message.method === 'OutboundRequest') { this._messenger.publish('ExecutionService:outboundRequest', snapId); - } else if (data.result?.type === 'OutboundResponse') { + } else if (message.method === 'OutboundResponse') { this._messenger.publish('ExecutionService:outboundResponse', snapId); - } else if (data.error) { - this._messenger.publish( - 'ExecutionService:unhandledError', - snapId, - data.error, + } else if (message.method === 'UnhandledError') { + if (isObject(message.params) && message.params.error) { + this._messenger.publish( + 'ExecutionService:unhandledError', + snapId, + message.params.error as ErrorJSON, + ); + commandStream.removeListener('data', notificationHandler); + } else { + console.error( + new Error( + `Received malformed "${message.method}" command stream notification.`, + ), + ); + } + } else { + console.error( + new Error( + `Received unexpected command stream notification "${message.method}".`, + ), ); - commandStream.removeListener('data', notificationHandler); } }; commandStream.on('data', notificationHandler); diff --git a/packages/controllers/src/services/iframe/IframeExecutionService.ts b/packages/controllers/src/services/iframe/IframeExecutionService.ts index 05be322d03..49fe4e5ee8 100644 --- a/packages/controllers/src/services/iframe/IframeExecutionService.ts +++ b/packages/controllers/src/services/iframe/IframeExecutionService.ts @@ -5,27 +5,24 @@ import { import { Job, AbstractExecutionService, - SetupSnapProvider, + ExecutionServiceArgs, } from '../AbstractExecutionService'; -import { ExecutionServiceMessenger } from '../ExecutionService'; type IframeExecutionEnvironmentServiceArgs = { - setupSnapProvider: SetupSnapProvider; iframeUrl: URL; - messenger: ExecutionServiceMessenger; -}; +} & ExecutionServiceArgs; export class IframeExecutionService extends AbstractExecutionService { public iframeUrl: URL; constructor({ - setupSnapProvider, iframeUrl, messenger, + setupSnapProvider, }: IframeExecutionEnvironmentServiceArgs) { super({ - setupSnapProvider, messenger, + setupSnapProvider, }); this.iframeUrl = iframeUrl; } diff --git a/packages/execution-environments/jest.config.js b/packages/execution-environments/jest.config.js index 89d1c1a375..a181dd0504 100644 --- a/packages/execution-environments/jest.config.js +++ b/packages/execution-environments/jest.config.js @@ -6,7 +6,7 @@ module.exports = { coverageReporters: ['clover', 'json', 'lcov', 'text', 'json-summary'], coverageThreshold: { global: { - branches: 84.62, + branches: 85, functions: 92.38, lines: 86.18, statements: 86.38, diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts index 605d18cc2a..afa1caaa0a 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts @@ -465,7 +465,7 @@ describe('BaseSnapExecutor', () => { expect(await executor.readCommand()).toStrictEqual({ jsonrpc: '2.0', - result: { type: 'OutboundRequest' }, + method: 'OutboundRequest', }); const providerRequest = await executor.readRpc(); @@ -512,7 +512,7 @@ describe('BaseSnapExecutor', () => { expect(await executor.readCommand()).toStrictEqual({ jsonrpc: '2.0', - result: { type: 'OutboundResponse' }, + method: 'OutboundResponse', }); expect(await executor.readCommand()).toStrictEqual({ @@ -570,13 +570,16 @@ describe('BaseSnapExecutor', () => { expect(await executor.readCommand()).toStrictEqual({ jsonrpc: '2.0', - error: { - code: -32603, - data: { - stack: testError.stack, - snapName: 'local:foo', + method: 'UnhandledError', + params: { + error: { + code: -32603, + data: { + stack: testError.stack, + snapName: 'local:foo', + }, + message: testError.message, }, - message: testError.message, }, }); }); @@ -629,13 +632,16 @@ describe('BaseSnapExecutor', () => { expect(await executor.readCommand()).toStrictEqual({ jsonrpc: '2.0', - error: { - code: -32603, - data: { - stack: testError.stack, - snapName: 'local:foo', + method: 'UnhandledError', + params: { + error: { + code: -32603, + data: { + stack: testError.stack, + snapName: 'local:foo', + }, + message: testError.message, }, - message: testError.message, }, }); }); @@ -688,13 +694,16 @@ describe('BaseSnapExecutor', () => { expect(await executor.readCommand()).toStrictEqual({ jsonrpc: '2.0', - error: { - code: -32603, - data: { - stack: testError.stack, - snapName: 'local:foo', + method: 'UnhandledError', + params: { + error: { + code: -32603, + data: { + stack: testError.stack, + snapName: 'local:foo', + }, + message: testError.message, }, - message: testError.message, }, }); }); diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.ts b/packages/execution-environments/src/common/BaseSnapExecutor.ts index 4d5ac4d446..9329ecd6de 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.ts @@ -4,6 +4,7 @@ import { Duplex } from 'stream'; import { MetaMaskInpageProvider } from '@metamask/providers'; import { SnapProvider } from '@metamask/snap-types'; import { errorCodes, ethErrors, serializeError } from 'eth-rpc-errors'; +import { JsonRpcNotification } from '@metamask/utils'; import EEOpenRPCDocument from '../openrpc.json'; import { Endowments, @@ -68,6 +69,7 @@ export class BaseSnapExecutor { `No onRpcRequest handler exported for snap "${target}`, ); } + // We're capturing the handler in case someone modifies the data object before the call const handler = data.exports.onRpcRequest; return this.executeInSnapContext(target, () => @@ -85,11 +87,14 @@ export class BaseSnapExecutor { shouldIncludeStack: false, }); this.notify({ - error: { - ...serializedError, - data: { - ...data, - stack: constructedError?.stack, + method: 'UnhandledError', + params: { + error: { + ...serializedError, + data: { + ...data, + stack: constructedError?.stack, + }, }, }, }); @@ -144,7 +149,12 @@ export class BaseSnapExecutor { } } - protected notify(requestObject: Record) { + protected notify( + requestObject: Omit< + JsonRpcNotification | unknown[]>, + 'jsonrpc' + >, + ) { this.commandStream.write({ ...requestObject, jsonrpc: '2.0', @@ -276,11 +286,11 @@ export class BaseSnapExecutor { const originalRequest = provider.request; provider.request = async (args) => { - this.notify({ result: { type: 'OutboundRequest' } }); + this.notify({ method: 'OutboundRequest' }); try { return await originalRequest(args); } finally { - this.notify({ result: { type: 'OutboundResponse' } }); + this.notify({ method: 'OutboundResponse' }); } }; diff --git a/packages/execution-environments/src/common/commands.ts b/packages/execution-environments/src/common/commands.ts index 2598e2ea35..06cba5210b 100644 --- a/packages/execution-environments/src/common/commands.ts +++ b/packages/execution-environments/src/common/commands.ts @@ -71,7 +71,7 @@ export function getCommandMethodImplementations( throw new Error('request is not a proper JSON RPC Request'); } - return invokeSnapRpc(target, origin, request); + return (await invokeSnapRpc(target, origin, request)) ?? null; }, }; }