From 504f8727413c09220fd9ad8cf0eef60166329291 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Mon, 18 Jul 2022 13:33:15 +0200 Subject: [PATCH] Fix error serialization issues (#637) * Fix originalError serialization * Update coverage * Fix tests * Add tests * Simplify fix * Revert back to original error handling * Only include stack once --- .../node/NodeProcessExecutionService.test.ts | 7 +- .../node/NodeThreadExecutionService.test.ts | 7 +- .../execution-environments/jest.config.js | 8 +- .../src/common/BaseSnapExecutor.test.ts | 179 +++++++++++++++++- .../src/common/BaseSnapExecutor.ts | 21 +- 5 files changed, 199 insertions(+), 23 deletions(-) diff --git a/packages/controllers/src/services/node/NodeProcessExecutionService.test.ts b/packages/controllers/src/services/node/NodeProcessExecutionService.test.ts index e395800e8d..3be5d28898 100644 --- a/packages/controllers/src/services/node/NodeProcessExecutionService.test.ts +++ b/packages/controllers/src/services/node/NodeProcessExecutionService.test.ts @@ -198,8 +198,11 @@ describe('NodeProcessExecutionService', () => { // eslint-disable-next-line jest/prefer-strict-equal expect(await unhandledErrorPromise).toEqual({ code: -32603, - data: { snapName: 'TestSnap' }, - message: 'Unhandled promise rejection in snap.', + data: { + snapName: 'TestSnap', + stack: expect.any(String), + }, + message: 'random error inside', }); await service.terminateAllSnaps(); diff --git a/packages/controllers/src/services/node/NodeThreadExecutionService.test.ts b/packages/controllers/src/services/node/NodeThreadExecutionService.test.ts index ca10de6cf1..7deb3577a9 100644 --- a/packages/controllers/src/services/node/NodeThreadExecutionService.test.ts +++ b/packages/controllers/src/services/node/NodeThreadExecutionService.test.ts @@ -198,8 +198,11 @@ describe('NodeThreadExecutionService', () => { // eslint-disable-next-line jest/prefer-strict-equal expect(await unhandledErrorPromise).toEqual({ code: -32603, - data: { snapName: 'TestSnap' }, - message: 'Unhandled promise rejection in snap.', + data: { + snapName: 'TestSnap', + stack: expect.any(String), + }, + message: 'random error inside', }); await service.terminateAllSnaps(); diff --git a/packages/execution-environments/jest.config.js b/packages/execution-environments/jest.config.js index f8a3268ca1..89d1c1a375 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: 84.67, - functions: 89.52, - lines: 84.91, - statements: 85.14, + branches: 84.62, + functions: 92.38, + lines: 86.18, + statements: 86.38, }, }, moduleFileExtensions: ['js', 'json', 'jsx', 'ts', 'tsx', 'node'], diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts index 82c23e00f8..605d18cc2a 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts @@ -1,6 +1,6 @@ // eslint-disable-next-line import/no-unassigned-import import 'ses'; -import { Duplex, DuplexOptions, Readable } from 'stream'; +import { Duplex, DuplexOptions, EventEmitter, Readable } from 'stream'; import { JsonRpcResponse } from '@metamask/utils'; import { JsonRpcRequest } from '../__GENERATED__/openrpc'; import { BaseSnapExecutor } from './BaseSnapExecutor'; @@ -522,6 +522,183 @@ describe('BaseSnapExecutor', () => { }); }); + it('notifies execution service of out of band errors via unhandledrejection', async () => { + const CODE = ` + module.exports.onRpcRequest = async () => 'foo'; + `; + const executor = new TestSnapExecutor(); + const emitter = new EventEmitter(); + + jest + .spyOn(window, 'addEventListener') + .mockImplementation((type, listener) => + emitter.on(type, listener as any), + ); + + await executor.writeCommand({ + jsonrpc: '2.0', + id: 1, + method: 'executeSnap', + params: [FAKE_SNAP_NAME, CODE, []], + }); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + id: 1, + result: 'OK', + }); + + await executor.writeCommand({ + jsonrpc: '2.0', + id: 2, + method: 'snapRpc', + params: [ + FAKE_SNAP_NAME, + FAKE_ORIGIN, + { jsonrpc: '2.0', method: '', params: [] }, + ], + }); + + expect(await executor.readCommand()).toStrictEqual({ + id: 2, + jsonrpc: '2.0', + result: 'foo', + }); + + const testError = new Error('test error'); + emitter.emit('unhandledrejection', { reason: testError }); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + error: { + code: -32603, + data: { + stack: testError.stack, + snapName: 'local:foo', + }, + message: testError.message, + }, + }); + }); + + it('notifies execution service of out of band errors via unhandledrejection when event is error', async () => { + const CODE = ` + module.exports.onRpcRequest = async () => 'foo'; + `; + const executor = new TestSnapExecutor(); + const emitter = new EventEmitter(); + + jest + .spyOn(window, 'addEventListener') + .mockImplementation((type, listener) => + emitter.on(type, listener as any), + ); + + await executor.writeCommand({ + jsonrpc: '2.0', + id: 1, + method: 'executeSnap', + params: [FAKE_SNAP_NAME, CODE, []], + }); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + id: 1, + result: 'OK', + }); + + await executor.writeCommand({ + jsonrpc: '2.0', + id: 2, + method: 'snapRpc', + params: [ + FAKE_SNAP_NAME, + FAKE_ORIGIN, + { jsonrpc: '2.0', method: '', params: [] }, + ], + }); + + expect(await executor.readCommand()).toStrictEqual({ + id: 2, + jsonrpc: '2.0', + result: 'foo', + }); + + const testError = new Error('test error'); + emitter.emit('unhandledrejection', testError); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + error: { + code: -32603, + data: { + stack: testError.stack, + snapName: 'local:foo', + }, + message: testError.message, + }, + }); + }); + + it('notifies execution service of out of band errors via error', async () => { + const CODE = ` + module.exports.onRpcRequest = async () => 'foo'; + `; + const executor = new TestSnapExecutor(); + const emitter = new EventEmitter(); + + jest + .spyOn(window, 'addEventListener') + .mockImplementation((type, listener) => + emitter.on(type, listener as any), + ); + + await executor.writeCommand({ + jsonrpc: '2.0', + id: 1, + method: 'executeSnap', + params: [FAKE_SNAP_NAME, CODE, []], + }); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + id: 1, + result: 'OK', + }); + + await executor.writeCommand({ + jsonrpc: '2.0', + id: 2, + method: 'snapRpc', + params: [ + FAKE_SNAP_NAME, + FAKE_ORIGIN, + { jsonrpc: '2.0', method: '', params: [] }, + ], + }); + + expect(await executor.readCommand()).toStrictEqual({ + id: 2, + jsonrpc: '2.0', + result: 'foo', + }); + + const testError = new Error('test error'); + emitter.emit('error', { error: testError }); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + error: { + code: -32603, + data: { + stack: testError.stack, + snapName: 'local:foo', + }, + message: testError.message, + }, + }); + }); + it('blocks Snaps from escaping confinement by using unbound this', async () => { const PAYLOAD = ` console.error("Hack the planet"); diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.ts b/packages/execution-environments/src/common/BaseSnapExecutor.ts index 9515e81e10..4d5ac4d446 100644 --- a/packages/execution-environments/src/common/BaseSnapExecutor.ts +++ b/packages/execution-environments/src/common/BaseSnapExecutor.ts @@ -78,25 +78,18 @@ export class BaseSnapExecutor { ); } - private errorHandler( - reason: string, - originalError: unknown, - data: Record, - ) { - const error = new Error(reason); - - const _originalError: Error | undefined = constructError(originalError); - - const serializedError = serializeError(error, { + private errorHandler(error: unknown, data: Record) { + const constructedError = constructError(error); + const serializedError = serializeError(constructedError, { + fallbackError, shouldIncludeStack: false, }); - this.notify({ error: { ...serializedError, data: { ...data, - originalError: _originalError, + stack: constructedError?.stack, }, }, }); @@ -189,11 +182,11 @@ export class BaseSnapExecutor { } this.snapErrorHandler = (error: ErrorEvent) => { - this.errorHandler('Uncaught error in snap.', error.error, { snapName }); + this.errorHandler(error.error, { snapName }); }; this.snapPromiseErrorHandler = (error: PromiseRejectionEvent) => { - this.errorHandler('Unhandled promise rejection in snap.', error.reason, { + this.errorHandler(error instanceof Error ? error : error.reason, { snapName, }); };