feat: Throw when encountering unexpected RPC method hooks

app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js

@@ -1,23 +1,21 @@
 import { permissionRpcMethods } from '@metamask/permission-controller';
 import { selectHooks } from '@metamask/snaps-rpc-methods';
+import { hasProperty } from '@metamask/utils';
 import { ethErrors } from 'eth-rpc-errors';
-import { flatten } from 'lodash';
 import { UNSUPPORTED_RPC_METHODS } from '../../../../shared/constants/network';
 import localHandlers from './handlers';
 
 const allHandlers = [...localHandlers, ...permissionRpcMethods.handlers];
 
 const handlerMap = allHandlers.reduce((map, handler) => {
  for (const methodName of handler.methodNames) {
- map.set(methodName, handler);
+ map[methodName] = handler;
  }
  return map;
-}, new Map());
+}, {});
 
-const expectedHookNames = Array.from(
- new Set(
- flatten(allHandlers.map(({ hookNames }) => Object.keys(hookNames))),
- ).values(),
+const expectedHookNames = new Set(
+ allHandlers.flatMap(({ hookNames }) => Object.getOwnPropertyNames(hookNames)),
 );
 
 /**
@@ -26,28 +24,20 @@ const expectedHookNames = Array.from(
  * Handlers consume functions that hook into the background, and only depend
  * on their signatures, not e.g. controller internals.
  *
- * @param {Record<string, unknown>} hooks - Required "hooks" into our
+ * @param  {Record<string, (...args: unknown[]) => unknown | Promise<unknown>>} hooks - Required "hooks" into our
  * controllers.
- * @returns {(req: object, res: object, next: Function, end: Function) => void}
+ * @returns {import('json-rpc-engine').JsonRpcMiddleware<unknown, unknown>} The method middleware function.
  */
 export function createMethodMiddleware(hooks) {
- // Fail immediately if we forgot to provide any expected hooks.
- const missingHookNames = expectedHookNames.filter(
- (hookName) => !Object.hasOwnProperty.call(hooks, hookName),
- );
- if (missingHookNames.length > 0) {
- throw new Error(
- `Missing expected hooks:\n\n${missingHookNames.join('\n')}\n`,
- );
- }
+ assertExpectedHook(hooks);
 
  return async function methodMiddleware(req, res, next, end) {
  // Reject unsupported methods.
  if (UNSUPPORTED_RPC_METHODS.has(req.method)) {
  return end(ethErrors.rpc.methodNotSupported());
  }
 
- const handler = handlerMap.get(req.method);
+ const handler = handlerMap[req.method];
  if (handler) {
  const { implementation, hookNames } = handler;
  try {
@@ -63,10 +53,42 @@ export function createMethodMiddleware(hooks) {
  if (process.env.METAMASK_DEBUG) {
  console.error(error);
  }
- return end(error);
+ return end(
+ error instanceof Error
+ ? error
+ : ethErrors.rpc.internal({ data: error }),
+ );
  }
  }
 
  return next();
  };
 }
+
+/**
+ * Asserts that the hooks object only has all expected hooks and no extraneous ones.
+ *
+ * @param {Record<string, unknown>} hooks - Required "hooks" into our controllers.
+ */
+function assertExpectedHook(hooks) {
+ const missingHookNames = [];
+ expectedHookNames.forEach((hookName) => {
+ if (!hasProperty(hooks, hookName)) {
+ missingHookNames.push(hookName);
+ }
+ });
+ if (missingHookNames.length > 0) {
+ throw new Error(
+ `Missing expected hooks:\n\n${missingHookNames.join('\n')}\n`,
+ );
+ }
+
+ const extraneousHookNames = Object.getOwnPropertyNames(hooks).filter(
+ (hookName) => !expectedHookNames.has(hookName),
+ );
+ if (extraneousHookNames.length > 0) {
+ throw new Error(
+ `Received unexpected hooks:\n\n${extraneousHookNames.join('\n')}\n`,
+ );
+ }
+}

app/scripts/lib/rpc-method-middleware/createMethodMiddleware.test.js

@@ -0,0 +1,150 @@
+import { JsonRpcEngine } from 'json-rpc-engine';
+import {
+ assertIsJsonRpcFailure,
+ assertIsJsonRpcSuccess,
+} from '@metamask/utils';
+import { createMethodMiddleware } from '.';
+
+jest.mock('@metamask/permission-controller', () => ({
+ permissionRpcMethods: { handlers: [] },
+}));
+
+jest.mock('./handlers', () => [
+ {
+ implementation: (req, res, _next, end, hooks) => {
+ if (Array.isArray(req.params)) {
+ switch (req.params[0]) {
+ case 1:
+ res.result = hooks.hook1();
+ break;
+ case 2:
+ res.result = hooks.hook2();
+ break;
+ case 3:
+ return end(new Error('test error'));
+ default:
+ throw new Error(`unexpected param "${req.params[0]}"`);
+ }
+ }
+ return end();
+ },
+ hookNames: { hook1: true, hook2: true },
+ methodNames: ['method1', 'method2'],
+ },
+]);
+
+describe('createMethodMiddleware', () => {
+ const method1 = 'method1';
+
+ const getDefaultHooks = () => ({
+ hook1: () => 42,
+ hook2: () => 99,
+ });
+
+ it('should return a function', () => {
+ const middleware = createMethodMiddleware(getDefaultHooks());
+ expect(typeof middleware).toBe('function');
+ });
+
+ it('should throw an error if a required hook is missing', () => {
+ const hooks = { hook1: () => 42 };
+
+ // @ts-expect-error Intentional destructive testing
+ expect(() => createMethodMiddleware(hooks)).toThrow(
+ 'Missing expected hooks',
+ );
+ });
+
+ it('should throw an error if an extraneous hook is provided', () => {
+ const hooks = {
+ ...getDefaultHooks(),
+ extraneousHook: () => 100,
+ };
+
+ expect(() => createMethodMiddleware(hooks)).toThrow(
+ 'Received unexpected hooks',
+ );
+ });
+
+ it('should call the handler for the matching method (uses hook1)', async () => {
+ const middleware = createMethodMiddleware(getDefaultHooks());
+ const engine = new JsonRpcEngine();
+ engine.push(middleware);
+
+ const response = await engine.handle({
+ jsonrpc: '2.0',
+ id: 1,
+ method: method1,
+ params: [1],
+ });
+ assertIsJsonRpcSuccess(response);
+
+ expect(response.result).toBe(42);
+ });
+
+ it('should call the handler for the matching method (uses hook2)', async () => {
+ const middleware = createMethodMiddleware(getDefaultHooks());
+ const engine = new JsonRpcEngine();
+ engine.push(middleware);
+
+ const response = await engine.handle({
+ jsonrpc: '2.0',
+ id: 1,
+ method: method1,
+ params: [2],
+ });
+ assertIsJsonRpcSuccess(response);
+
+ expect(response.result).toBe(99);
+ });
+
+ it('should not call the handler for a non-matching method', async () => {
+ const middleware = createMethodMiddleware(getDefaultHooks());
+ const engine = new JsonRpcEngine();
+ engine.push(middleware);
+
+ const response = await engine.handle({
+ jsonrpc: '2.0',
+ id: 1,
+ method: 'nonMatchingMethod',
+ });
+ assertIsJsonRpcFailure(response);
+
+ expect(response.error).toMatchObject({
+ message: expect.stringMatching(
+ /Response has no error or result for request/u,
+ ),
+ });
+ });
+
+ it('should reject unsupported methods', async () => {
+ const middleware = createMethodMiddleware(getDefaultHooks());
+ const engine = new JsonRpcEngine();
+ engine.push(middleware);
+
+ const response = await engine.handle({
+ jsonrpc: '2.0',
+ id: 1,
+ method: 'eth_signTransaction',
+ });
+ assertIsJsonRpcFailure(response);
+
+ expect(response.error.message).toBe('Method not supported.');
+ });
+
+ it('should handle errors thrown by the implementation', async () => {
+ const middleware = createMethodMiddleware(getDefaultHooks());
+ const engine = new JsonRpcEngine();
+ engine.push(middleware);
+
+ const response = await engine.handle({
+ jsonrpc: '2.0',
+ id: 1,
+ method: method1,
+ params: [3],
+ });
+ assertIsJsonRpcFailure(response);
+
+ expect(response.error.message).toBe('test error');
+ });
+});

app/scripts/lib/rpc-method-middleware/handlers/index.ts

@@ -1,4 +1,3 @@
-import { MESSAGE_TYPE } from '../../../../../shared/constants/app';
 import addEthereumChain from './add-ethereum-chain';
 import ethAccounts from './eth-accounts';
 import getProviderState from './get-provider-state';
@@ -17,15 +16,7 @@ import mmiSetAccountAndNetwork from './institutional/mmi-set-account-and-network
 import mmiOpenAddHardwareWallet from './institutional/mmi-open-add-hardware-wallet';
 ///: END:ONLY_INCLUDE_IF
 
-type MessageType = (typeof MESSAGE_TYPE)[keyof typeof MESSAGE_TYPE];
-
-type HandlerInterface = {
- methodNames: MessageType[];
- implementation: unknown;
- hookNames?: { [key: string]: boolean };
-};
-
-const handlers: HandlerInterface[] = [
+const handlers = [
  addEthereumChain,
  ethAccounts,
  getProviderState,

app/scripts/metamask-controller.js

@@ -5005,16 +5005,6 @@ export default class MetamaskController extends EventEmitter {
  endApprovalFlow: this.approvalController.endFlow.bind(
  this.approvalController,
  ),
- setApprovalFlowLoadingText:
- this.approvalController.setFlowLoadingText.bind(
- this.approvalController,
- ),
- showApprovalSuccess: this.approvalController.success.bind(
- this.approvalController,
- ),
- showApprovalError: this.approvalController.error.bind(
- this.approvalController,
- ),
  sendMetrics: this.metaMetricsController.trackEvent.bind(
  this.metaMetricsController,
  ),
@@ -5076,18 +5066,11 @@ export default class MetamaskController extends EventEmitter {
  this.networkController,
  ),
  findNetworkConfigurationBy: this.findNetworkConfigurationBy.bind(this),
- getNetworkClientIdForDomain:
- this.selectedNetworkController.getNetworkClientIdForDomain.bind(
- this.selectedNetworkController,
- ),
  setNetworkClientIdForDomain:
  this.selectedNetworkController.setNetworkClientIdForDomain.bind(
  this.selectedNetworkController,
  ),
 
- getUseRequestQueue: this.preferencesController.getUseRequestQueue.bind(
- this.preferencesController,
- ),
  getProviderConfig: () => this.networkController.state.providerConfig,
  setProviderType: (type) => {
  return this.networkController.setProviderType(type);

app/scripts/metamask-controller.test.js

@@ -83,8 +83,15 @@ const createLoggerMiddlewareMock = () => (req, res, next) => {
  }
  next();
 };
-
 jest.mock('./lib/createLoggerMiddleware', () => createLoggerMiddlewareMock);
+
+const rpcMethodMiddlewareMock = {
+ createMethodMiddleware: () => (_req, _res, next, _end) => {
+ next();
+ },
+};
+jest.mock('./lib/rpc-method-middleware', () => rpcMethodMiddlewareMock);
+
 jest.mock(
  './controllers/preferences',
  () =>