From 17d10e8b1092605121aef9b320923dde7798bbd4 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 18 May 2026 21:21:35 +0200 Subject: [PATCH] refactor(wallet-cli): Parameterise RpcHandler with struct-validated dispatch Adds a `defineHandler(paramsStruct, run)` helper plus a generic `RpcHandler` / `RpcHandlerDefinition` type so each daemon RPC method owns its params struct. `rpc-socket-server` now validates params via `superstruct.validate` once per request and returns `-32602 invalidParams` on shape mismatch, so handler bodies can trust their input. Rewrites `getStatus` and `call` against the new shape and replaces the `wallet.messenger.call as any` cast with a single labelled `RpcDispatcher` bind. Tests updated; coverage stays at 100%. Closes #8777. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/wallet-cli/package.json | 1 + .../src/daemon/daemon-entry.test.ts | 69 ++++++++++-------- .../wallet-cli/src/daemon/daemon-entry.ts | 67 +++++++++++------ .../src/daemon/rpc-socket-server.test.ts | 72 ++++++++++++++++--- .../src/daemon/rpc-socket-server.ts | 19 ++++- .../src/daemon/socket-integration.test.ts | 33 +++++++-- packages/wallet-cli/src/daemon/types.ts | 63 ++++++++++++++-- yarn.lock | 1 + 8 files changed, 251 insertions(+), 74 deletions(-) diff --git a/packages/wallet-cli/package.json b/packages/wallet-cli/package.json index 000c5b4a7e..7bb0d5ad6a 100644 --- a/packages/wallet-cli/package.json +++ b/packages/wallet-cli/package.json @@ -49,6 +49,7 @@ "@inquirer/confirm": "^6.0.11", "@metamask/remote-feature-flag-controller": "^4.2.0", "@metamask/rpc-errors": "^7.0.2", + "@metamask/superstruct": "^3.1.0", "@metamask/utils": "^11.9.0", "@metamask/wallet": "^0.0.0", "@oclif/core": "^4.10.5", diff --git a/packages/wallet-cli/src/daemon/daemon-entry.test.ts b/packages/wallet-cli/src/daemon/daemon-entry.test.ts index 6b81d63da2..dd9566af9f 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.test.ts @@ -1,3 +1,4 @@ +import { validate } from '@metamask/superstruct'; import { mkdirSync } from 'node:fs'; import { appendFile, readFile, rm, writeFile } from 'node:fs/promises'; @@ -490,7 +491,7 @@ describe('daemon-entry', () => { const callArgs = mockStartRpcSocketServer.mock.calls[0][0]; const { handlers } = callArgs; - const status = (await handlers.getStatus(null)) as { + const status = (await handlers.getStatus.run(null)) as { pid: number; uptime: number; }; @@ -720,13 +721,18 @@ describe('daemon-entry', () => { describe('call handler', () => { /** - * Import the daemon entry and extract the `call` handler from the - * handlers map, along with the mock wallet for assertions. + * Import the daemon entry and extract the `call` handler definition from + * the handlers map, along with the mock wallet for assertions. * - * @returns The call handler function and mock wallet result. + * @returns The call handler definition and mock wallet result. */ async function setupCallHandler(): Promise<{ - callHandler: (params: unknown) => Promise; + callHandler: { + paramsStruct: import('@metamask/superstruct').Struct< + [string, ...unknown[]] + >; + run: (params: [string, ...unknown[]]) => Promise; + }; result: MockCreateWalletResult; }> { const result = createMockWallet(); @@ -736,20 +742,25 @@ describe('daemon-entry', () => { await importDaemonEntry(); const callArgs = mockStartRpcSocketServer.mock.calls[0][0]; - const callHandler = callArgs.handlers.call as ( - params: unknown, - ) => Promise; + const callHandler = callArgs.handlers.call as unknown as { + paramsStruct: import('@metamask/superstruct').Struct< + [string, ...unknown[]] + >; + run: (params: [string, ...unknown[]]) => Promise; + }; return { callHandler, result }; } - it('registers a call handler', async () => { + it('registers a call handler definition', async () => { mockCreateWallet.mockResolvedValue(createMockWallet()); mockStartRpcSocketServer.mockResolvedValue(createMockHandle()); await importDaemonEntry(); const callArgs = mockStartRpcSocketServer.mock.calls[0][0]; - expect(typeof callArgs.handlers.call).toBe('function'); + const callDefinition = callArgs.handlers.call; + expect(callDefinition).toHaveProperty('paramsStruct'); + expect(typeof callDefinition.run).toBe('function'); }); it('forwards action and args to messenger.call', async () => { @@ -757,7 +768,7 @@ describe('daemon-entry', () => { const mockCall = result.wallet.messenger.call as jest.Mock; mockCall.mockReturnValue({ accounts: [] }); - const callResult = await callHandler([ + const callResult = await callHandler.run([ 'Controller:action', 'arg1', 'arg2', @@ -776,7 +787,7 @@ describe('daemon-entry', () => { const mockCall = result.wallet.messenger.call as jest.Mock; mockCall.mockReturnValue('ok'); - await callHandler(['Controller:action']); + await callHandler.run(['Controller:action']); expect(mockCall).toHaveBeenCalledWith('Controller:action'); }); @@ -786,7 +797,7 @@ describe('daemon-entry', () => { const mockCall = result.wallet.messenger.call as jest.Mock; mockCall.mockResolvedValue({ async: true }); - const callResult = await callHandler(['Controller:asyncAction']); + const callResult = await callHandler.run(['Controller:asyncAction']); expect(callResult).toStrictEqual({ async: true }); }); @@ -798,33 +809,29 @@ describe('daemon-entry', () => { throw new Error('A handler for Unknown:action has not been registered'); }); - await expect(callHandler(['Unknown:action'])).rejects.toThrow( + await expect(callHandler.run(['Unknown:action'])).rejects.toThrow( 'A handler for Unknown:action has not been registered', ); }); - it('throws when params is null', async () => { + it.each([ + ['null', null], + ['empty array', []], + ['non-string first element', [42]], + ['non-array', { foo: 'bar' }], + ])('paramsStruct rejects invalid params (%s)', async (_label, value) => { const { callHandler } = await setupCallHandler(); - - await expect(callHandler(null)).rejects.toThrow( - 'Expected params to be an array with an action name', - ); - }); - - it('throws when params is an empty array', async () => { - const { callHandler } = await setupCallHandler(); - - await expect(callHandler([])).rejects.toThrow( - 'Expected params to be an array with an action name', - ); + const [error] = validate(value, callHandler.paramsStruct); + expect(error).toBeDefined(); }); - it('throws when action name is not a string', async () => { + it('paramsStruct accepts a non-empty array starting with a string', async () => { const { callHandler } = await setupCallHandler(); - - await expect(callHandler([42])).rejects.toThrow( - 'Expected params to be an array with an action name', + const [error] = validate( + ['Controller:action', 1, 'two'], + callHandler.paramsStruct, ); + expect(error).toBeUndefined(); }); }); }); diff --git a/packages/wallet-cli/src/daemon/daemon-entry.ts b/packages/wallet-cli/src/daemon/daemon-entry.ts index 8a27f7df40..9f48be1778 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.ts @@ -1,3 +1,4 @@ +import { define, literal } from '@metamask/superstruct'; import type { Json } from '@metamask/utils'; import type { Wallet } from '@metamask/wallet'; import { mkdirSync } from 'node:fs'; @@ -8,10 +9,33 @@ import { pingDaemon } from './daemon-client'; import { getDaemonPaths } from './paths'; import { startRpcSocketServer } from './rpc-socket-server'; import type { RpcSocketServerHandle } from './rpc-socket-server'; -import type { DaemonStatusInfo, RpcHandlerMap } from './types'; +import { defineHandler } from './types'; +import type { + DaemonStatusInfo, + RpcDispatcher, + RpcHandlerMap, +} from './types'; import { isErrorWithCode, isProcessAlive, readPidFile } from './utils'; import { createWallet } from './wallet-factory'; +/** + * Params struct for the `call` RPC method. `params` must be a non-empty array + * whose first element is the messenger action name; remaining elements are + * positional action arguments forwarded as-is to `messenger.call`. + */ +const callParamsStruct = define<[string, ...Json[]]>('CallParams', (value) => { + if (!Array.isArray(value)) { + return 'Expected an array'; + } + if (value.length === 0) { + return 'Expected a non-empty array'; + } + if (typeof value[0] !== 'string') { + return 'Expected the first element to be a string action name'; + } + return true; +}); + const startTime = Date.now(); main().catch((error: unknown) => { @@ -101,28 +125,29 @@ async function main(): Promise { })); const constructedWallet = wallet; + // Arbitrary messenger dispatch is intentional: the CLI exposes the full + // messenger surface over a Unix socket inside the per-user oclif data + // directory. The dataDir/socket are chmodded to 0o700/0o600 below so + // only the owning user can open them, but there is no in-process + // auth check beyond that filesystem-permission barrier. The messenger is + // strongly typed by action name; we narrow it once here to the + // RpcDispatcher shape the `call` handler needs. + const dispatch = constructedWallet.messenger.call.bind( + constructedWallet.messenger, + ) as unknown as RpcDispatcher; + const handlers: RpcHandlerMap = { - getStatus: async (): Promise => ({ - pid: process.pid, - uptime: Math.floor((Date.now() - startTime) / 1000), + getStatus: defineHandler( + literal(null), + async (): Promise => ({ + pid: process.pid, + uptime: Math.floor((Date.now() - startTime) / 1000), + }), + ), + call: defineHandler(callParamsStruct, async (params) => { + const [action, ...args] = params; + return await dispatch(action, ...args); }), - // Arbitrary messenger dispatch is intentional: the CLI exposes the full - // messenger surface over a Unix socket inside the per-user oclif data - // directory. The dataDir/socket are chmodded to 0o700/0o600 below so - // only the owning user can open them, but there is no in-process - // auth check beyond that filesystem-permission barrier. - call: async (params) => { - if (!Array.isArray(params) || typeof params[0] !== 'string') { - throw new Error('Expected params to be an array with an action name'); - } - const [action, ...args] = params as [string, ...Json[]]; - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- The messenger is strongly typed; we bypass it here to dispatch arbitrary action names from RPC. - const result = (constructedWallet.messenger as any).call( - action, - ...args, - ); - return (result instanceof Promise ? await result : result) as Json; - }, }; handle = await startRpcSocketServer({ diff --git a/packages/wallet-cli/src/daemon/rpc-socket-server.test.ts b/packages/wallet-cli/src/daemon/rpc-socket-server.test.ts index 2fcea6ff95..33fc3e3fbe 100644 --- a/packages/wallet-cli/src/daemon/rpc-socket-server.test.ts +++ b/packages/wallet-cli/src/daemon/rpc-socket-server.test.ts @@ -1,10 +1,26 @@ +import { any, literal } from '@metamask/superstruct'; import { EventEmitter } from 'node:events'; import { unlink } from 'node:fs/promises'; import { createServer } from 'node:net'; import type { Server, Socket } from 'node:net'; import { startRpcSocketServer } from './rpc-socket-server'; -import type { RpcHandlerMap } from './types'; +import type { RpcHandlerDefinition, RpcHandlerMap } from './types'; + +/** + * Wrap a `jest.fn` as an {@link RpcHandlerDefinition} so existing tests can + * register a handler without writing out the `{ paramsStruct, run }` shape. + * Defaults to `any()` so the struct guard never rejects the test inputs. + * + * @param run - The mocked handler function. + * @returns A handler definition with an `any()` paramsStruct. + */ +function asHandler(run: jest.Mock): RpcHandlerDefinition { + return { + paramsStruct: any(), + run: run as unknown as RpcHandlerDefinition['run'], + }; +} jest.mock('node:fs/promises'); jest.mock('node:net'); @@ -172,7 +188,7 @@ describe('startRpcSocketServer', () => { it('dispatches valid request to handler and returns result', async () => { const { simulateConnection } = createMockServer(); const handlers: RpcHandlerMap = { - getStatus: jest.fn().mockResolvedValue({ status: 'ok' }), + getStatus: asHandler(jest.fn().mockResolvedValue({ status: 'ok' })), }; await startRpcSocketServer({ @@ -200,7 +216,7 @@ describe('startRpcSocketServer', () => { it('returns null result when handler returns undefined', async () => { const { simulateConnection } = createMockServer(); const handlers: RpcHandlerMap = { - noop: jest.fn().mockResolvedValue(undefined), + noop: asHandler(jest.fn().mockResolvedValue(undefined)), }; await startRpcSocketServer({ @@ -315,7 +331,9 @@ describe('startRpcSocketServer', () => { it('returns -32603 when handler throws an Error', async () => { const { simulateConnection } = createMockServer(); const handlers: RpcHandlerMap = { - failing: jest.fn().mockRejectedValue(new Error('handler failed')), + failing: asHandler( + jest.fn().mockRejectedValue(new Error('handler failed')), + ), }; await startRpcSocketServer({ @@ -341,7 +359,7 @@ describe('startRpcSocketServer', () => { const { simulateConnection } = createMockServer(); const rpcError = { code: -32001, message: 'custom rpc' }; const handlers: RpcHandlerMap = { - failing: jest.fn().mockRejectedValue(rpcError), + failing: asHandler(jest.fn().mockRejectedValue(rpcError)), }; await startRpcSocketServer({ @@ -364,7 +382,7 @@ describe('startRpcSocketServer', () => { it('returns Internal error when handler throws a non-Error value', async () => { const { simulateConnection } = createMockServer(); const handlers: RpcHandlerMap = { - failing: jest.fn().mockRejectedValue('string error'), + failing: asHandler(jest.fn().mockRejectedValue('string error')), }; await startRpcSocketServer({ @@ -489,7 +507,7 @@ describe('startRpcSocketServer', () => { it('accumulates partial data across multiple events', async () => { const { simulateConnection } = createMockServer(); const handlers: RpcHandlerMap = { - test: jest.fn().mockResolvedValue('ok'), + test: asHandler(jest.fn().mockResolvedValue('ok')), }; await startRpcSocketServer({ @@ -568,7 +586,7 @@ describe('startRpcSocketServer', () => { const circular: Record = {}; circular.self = circular; const handlers: RpcHandlerMap = { - bad: jest.fn().mockResolvedValue(circular), + bad: asHandler(jest.fn().mockResolvedValue(circular)), }; await startRpcSocketServer({ @@ -641,10 +659,46 @@ describe('startRpcSocketServer', () => { jest.useRealTimers(); }); + it('returns -32602 when params fail the registered struct', async () => { + const { simulateConnection } = createMockServer(); + const run = jest.fn(); + const handlers: RpcHandlerMap = { + // Struct that only accepts the literal value `'expected'`. + strict: { + paramsStruct: literal('expected'), + run: run as unknown as RpcHandlerMap[string]['run'], + }, + }; + + await startRpcSocketServer({ + socketPath: '/tmp/test.sock', + handlers, + }); + + const socket = createMockSocket(); + simulateConnection(socket); + sendRequest(socket, { + jsonrpc: '2.0', + id: '1', + method: 'strict', + params: ['something else'], + }); + + await flushPromises(); + + expect(getResponse(socket).error).toStrictEqual( + expect.objectContaining({ + code: -32602, + message: expect.stringContaining('Invalid params for strict'), + }), + ); + expect(run).not.toHaveBeenCalled(); + }); + it('wraps thrown object with code but no message as internal error', async () => { const { simulateConnection } = createMockServer(); const handlers: RpcHandlerMap = { - failing: jest.fn().mockRejectedValue({ code: 42 }), + failing: asHandler(jest.fn().mockRejectedValue({ code: 42 })), }; await startRpcSocketServer({ diff --git a/packages/wallet-cli/src/daemon/rpc-socket-server.ts b/packages/wallet-cli/src/daemon/rpc-socket-server.ts index 07c8a3ed7b..8b6cba8baa 100644 --- a/packages/wallet-cli/src/daemon/rpc-socket-server.ts +++ b/packages/wallet-cli/src/daemon/rpc-socket-server.ts @@ -1,4 +1,5 @@ import { rpcErrors } from '@metamask/rpc-errors'; +import { validate as validateStruct } from '@metamask/superstruct'; import type { JsonRpcId, JsonRpcParams, @@ -240,7 +241,23 @@ async function handleRequest( }; } - const result = await handler(coerceHandlerParams(params)); + const [structError, validatedParams] = validateStruct( + coerceHandlerParams(params), + handler.paramsStruct, + ); + if (structError !== undefined) { + return { + jsonrpc: '2.0', + id, + error: rpcErrors + .invalidParams({ + message: `Invalid params for ${method}: ${structError.message}`, + }) + .serialize(), + }; + } + + const result = await handler.run(validatedParams); return { jsonrpc: '2.0', id, result: result ?? null }; } catch (error) { log(`RPC handler "${method}" failed: ${String(error)}`); diff --git a/packages/wallet-cli/src/daemon/socket-integration.test.ts b/packages/wallet-cli/src/daemon/socket-integration.test.ts index 0327ed918d..69e77c7227 100644 --- a/packages/wallet-cli/src/daemon/socket-integration.test.ts +++ b/packages/wallet-cli/src/daemon/socket-integration.test.ts @@ -1,9 +1,28 @@ +import { any } from '@metamask/superstruct'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { pingDaemon, sendCommand } from './daemon-client'; import { startRpcSocketServer } from './rpc-socket-server'; import type { RpcSocketServerHandle } from './rpc-socket-server'; +import type { RpcHandlerDefinition } from './types'; + +/** + * Wrap a plain async function as an {@link RpcHandlerDefinition} with an + * `any()` paramsStruct so integration tests don't need to spell out the full + * definition shape for each handler. + * + * @param run - The async handler implementation. + * @returns A handler definition usable in an {@link RpcHandlerMap}. + */ +function handlerDefinition( + run: (params: unknown) => Promise, +): RpcHandlerDefinition { + return { + paramsStruct: any(), + run: run as unknown as RpcHandlerDefinition['run'], + }; +} /** * End-to-end integration tests for the daemon's IPC layer: real @@ -52,7 +71,7 @@ describe('socket integration', () => { await startServer({ socketPath, handlers: { - getStatus: async () => ({ pid: 42, uptime: 7 }), + getStatus: handlerDefinition(async () => ({ pid: 42, uptime: 7 })), }, }); @@ -72,7 +91,7 @@ describe('socket integration', () => { await startServer({ socketPath, handlers: { - getStatus: async () => ({ pid: 1, uptime: 0 }), + getStatus: handlerDefinition(async () => ({ pid: 1, uptime: 0 })), }, }); @@ -89,9 +108,9 @@ describe('socket integration', () => { await startServer({ socketPath, handlers: { - boom: async () => { + boom: handlerDefinition(async () => { throw new Error('handler exploded'); - }, + }), }, }); @@ -114,7 +133,7 @@ describe('socket integration', () => { await startServer({ socketPath, handlers: { - getStatus: async () => ({ pid: 1, uptime: 0 }), + getStatus: handlerDefinition(async () => ({ pid: 1, uptime: 0 })), }, }); @@ -134,7 +153,7 @@ describe('socket integration', () => { await startServer({ socketPath, handlers: { - echo: async (params) => ({ params }), + echo: handlerDefinition(async (params) => ({ params })), }, }); @@ -183,7 +202,7 @@ describe('socket integration', () => { await startServer({ socketPath, handlers: { - getStatus: async () => ({ pid: 1, uptime: 0 }), + getStatus: handlerDefinition(async () => ({ pid: 1, uptime: 0 })), }, }); diff --git a/packages/wallet-cli/src/daemon/types.ts b/packages/wallet-cli/src/daemon/types.ts index 8e9e8be50e..250c2f19b6 100644 --- a/packages/wallet-cli/src/daemon/types.ts +++ b/packages/wallet-cli/src/daemon/types.ts @@ -1,16 +1,69 @@ +import type { Struct } from '@metamask/superstruct'; import type { Json } from '@metamask/utils'; /** - * A function that handles a JSON-RPC method call. + * A function that handles a JSON-RPC method call after its params have been + * validated by the corresponding {@link RpcHandlerDefinition.paramsStruct}. + */ +export type RpcHandler = ( + params: TParams, +) => Promise; + +/** + * Definition for a single JSON-RPC method: the struct that validates + * incoming `params` plus the handler that runs once `params` is known to + * match. * - * The `params` argument will be `null` if the client did not provide params. + * The server (see `rpc-socket-server.ts`) validates the raw `params` against + * `paramsStruct` before invoking `run`, so each handler body can trust the + * shape of its input without re-checking. */ -export type RpcHandler = (params: Json) => Promise; +export type RpcHandlerDefinition = { + paramsStruct: Struct; + run: RpcHandler; +}; /** - * A map of RPC method names to their handler functions. + * A map of RPC method names to their handler definitions. + * + * `TParams` is widened to `any` here so definitions with different narrow + * params types (e.g. `null` vs. a tuple) can coexist in the same map. The + * runtime struct guard validates `params` before each `run` invocation, so the + * widening cannot let an unvalidated value reach a handler body. + */ +export type RpcHandlerMap = Record< + string, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + RpcHandlerDefinition +>; + +/** + * Bundle a params struct with the handler that runs once `params` is + * validated. The server invokes `run` only after `paramsStruct` accepts the + * value, so `run` can trust the type of its argument. + * + * @param paramsStruct - Struct that validates `params` for this method. + * @param run - Handler invoked with the validated params. + * @returns An {@link RpcHandlerDefinition} suitable for an {@link RpcHandlerMap}. + */ +export function defineHandler( + paramsStruct: Struct, + run: RpcHandler, +): RpcHandlerDefinition { + return { paramsStruct, run }; +} + +/** + * Typed wrapper around `wallet.messenger.call` used by the `call` RPC. + * + * The messenger is strongly typed by action name; the daemon exposes the full + * messenger surface over the socket and dispatches by string, so we narrow it + * to a single, documented escape hatch instead of casting at each call site. */ -export type RpcHandlerMap = Record; +export type RpcDispatcher = ( + action: string, + ...args: Json[] +) => Json | Promise; /** * Resolved paths for daemon state files. diff --git a/yarn.lock b/yarn.lock index be0f14d051..08135d79ad 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5820,6 +5820,7 @@ __metadata: "@metamask/auto-changelog": "npm:^6.1.0" "@metamask/remote-feature-flag-controller": "npm:^4.2.0" "@metamask/rpc-errors": "npm:^7.0.2" + "@metamask/superstruct": "npm:^3.1.0" "@metamask/utils": "npm:^11.9.0" "@metamask/wallet": "npm:^0.0.0" "@oclif/core": "npm:^4.10.5"