From 178c95ba21f80c9f44a207276a854759a65adc61 Mon Sep 17 00:00:00 2001 From: jiexi Date: Thu, 19 Sep 2024 10:58:36 -0700 Subject: [PATCH] feat: use networkClientId to resolve chainId in PPOM Middleware (#27263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Updates the PPOM Middleware to use the chainId for the dapp selected network instead of the globally selected network when processing a request [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27263?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/scripts/lib/ppom/ppom-middleware.test.ts | 112 ++++++++++--------- app/scripts/lib/ppom/ppom-middleware.ts | 17 ++- app/scripts/metamask-controller.js | 1 + 3 files changed, 72 insertions(+), 58 deletions(-) diff --git a/app/scripts/lib/ppom/ppom-middleware.test.ts b/app/scripts/lib/ppom/ppom-middleware.test.ts index a0188a96ae8a..f82d029aac51 100644 --- a/app/scripts/lib/ppom/ppom-middleware.test.ts +++ b/app/scripts/lib/ppom/ppom-middleware.test.ts @@ -8,7 +8,6 @@ import { BlockaidResultType, } from '../../../../shared/constants/security-provider'; import { flushPromises } from '../../../../test/lib/timer-helpers'; -import { mockNetworkState } from '../../../../test/stub/networks'; import { createPPOMMiddleware, PPOMMiddlewareRequest } from './ppom-middleware'; import { generateSecurityAlertId, @@ -34,6 +33,8 @@ const REQUEST_MOCK = { params: [], id: '', jsonrpc: '2.0' as const, + origin: 'test.com', + networkClientId: 'networkClientId', }; const createMiddleware = ( @@ -41,15 +42,9 @@ const createMiddleware = ( chainId?: Hex; error?: Error; securityAlertsEnabled?: boolean; - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - updateSecurityAlertResponse?: any; - } = { - updateSecurityAlertResponse: () => undefined, - }, + } = {}, ) => { - const { chainId, error, securityAlertsEnabled, updateSecurityAlertResponse } = - options; + const { chainId, error, securityAlertsEnabled } = options; const ppomController = {}; @@ -68,7 +63,9 @@ const createMiddleware = ( } const networkController = { - state: mockNetworkState({ chainId: chainId || CHAIN_IDS.MAINNET }), + getNetworkConfigurationByNetworkClientId: jest + .fn() + .mockReturnValue({ chainId: chainId || CHAIN_IDS.MAINNET }), }; const appStateController = { @@ -79,7 +76,9 @@ const createMiddleware = ( listAccounts: () => [{ address: INTERNAL_ACCOUNT_ADDRESS }], }; - return createPPOMMiddleware( + const updateSecurityAlertResponse = jest.fn(); + + const middleware = createPPOMMiddleware( // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any ppomController as any, @@ -96,6 +95,16 @@ const createMiddleware = ( accountsController as any, updateSecurityAlertResponse, ); + + return { + middleware, + ppomController, + preferenceController, + networkController, + appStateController, + accountsController, + updateSecurityAlertResponse, + }; }; describe('PPOMMiddleware', () => { @@ -121,12 +130,29 @@ describe('PPOMMiddleware', () => { }; }); - it('updates alert response after validating request', async () => { - const updateSecurityAlertResponse = jest.fn(); + it('gets the network configuration for the request networkClientId', async () => { + const { middleware, networkController } = createMiddleware(); - const middlewareFunction = createMiddleware({ - updateSecurityAlertResponse, - }); + const req = { + ...REQUEST_MOCK, + method: 'eth_sendTransaction', + securityAlertResponse: undefined, + }; + + await middleware(req, { ...JsonRpcResponseStruct.TYPE }, () => undefined); + + await flushPromises(); + + expect( + networkController.getNetworkConfigurationByNetworkClientId, + ).toHaveBeenCalledTimes(1); + expect( + networkController.getNetworkConfigurationByNetworkClientId, + ).toHaveBeenCalledWith('networkClientId'); + }); + + it('updates alert response after validating request', async () => { + const { middleware, updateSecurityAlertResponse } = createMiddleware(); const req = { ...REQUEST_MOCK, @@ -134,11 +160,7 @@ describe('PPOMMiddleware', () => { securityAlertResponse: undefined, }; - await middlewareFunction( - req, - { ...JsonRpcResponseStruct.TYPE }, - () => undefined, - ); + await middleware(req, { ...JsonRpcResponseStruct.TYPE }, () => undefined); await flushPromises(); @@ -151,7 +173,7 @@ describe('PPOMMiddleware', () => { }); it('adds loading response to confirmation requests while validation is in progress', async () => { - const middlewareFunction = createMiddleware(); + const { middleware } = createMiddleware(); const req: PPOMMiddlewareRequest<(string | { to: string })[]> = { ...REQUEST_MOCK, @@ -159,11 +181,7 @@ describe('PPOMMiddleware', () => { securityAlertResponse: undefined, }; - await middlewareFunction( - req, - { ...JsonRpcResponseStruct.TYPE }, - () => undefined, - ); + await middleware(req, { ...JsonRpcResponseStruct.TYPE }, () => undefined); expect(req.securityAlertResponse?.reason).toBe(BlockaidReason.inProgress); expect(req.securityAlertResponse?.result_type).toBe( @@ -172,7 +190,7 @@ describe('PPOMMiddleware', () => { }); it('does not do validation if the user has not enabled the preference', async () => { - const middlewareFunction = createMiddleware({ + const { middleware } = createMiddleware({ securityAlertsEnabled: false, }); @@ -183,7 +201,7 @@ describe('PPOMMiddleware', () => { }; // @ts-expect-error Passing in invalid input for testing purposes - await middlewareFunction(req, undefined, () => undefined); + await middleware(req, undefined, () => undefined); expect(req.securityAlertResponse).toBeUndefined(); expect(validateRequestWithPPOM).not.toHaveBeenCalled(); @@ -191,7 +209,7 @@ describe('PPOMMiddleware', () => { it('does not do validation if user is not on a supported network', async () => { isChainSupportedMock.mockResolvedValue(false); - const middlewareFunction = createMiddleware({ + const { middleware } = createMiddleware({ chainId: '0x2', }); @@ -201,18 +219,14 @@ describe('PPOMMiddleware', () => { securityAlertResponse: undefined, }; - await middlewareFunction( - req, - { ...JsonRpcResponseStruct.TYPE }, - () => undefined, - ); + await middleware(req, { ...JsonRpcResponseStruct.TYPE }, () => undefined); expect(req.securityAlertResponse).toBeUndefined(); expect(validateRequestWithPPOM).not.toHaveBeenCalled(); }); it('does not do validation when request is not for confirmation method', async () => { - const middlewareFunction = createMiddleware(); + const { middleware } = createMiddleware(); const req = { ...REQUEST_MOCK, @@ -220,18 +234,14 @@ describe('PPOMMiddleware', () => { securityAlertResponse: undefined, }; - await middlewareFunction( - req, - { ...JsonRpcResponseStruct.TYPE }, - () => undefined, - ); + await middleware(req, { ...JsonRpcResponseStruct.TYPE }, () => undefined); expect(req.securityAlertResponse).toBeUndefined(); expect(validateRequestWithPPOM).not.toHaveBeenCalled(); }); it('does not do validation when request is send to users own account', async () => { - const middlewareFunction = createMiddleware(); + const { middleware } = createMiddleware(); const req = { ...REQUEST_MOCK, @@ -240,18 +250,14 @@ describe('PPOMMiddleware', () => { securityAlertResponse: undefined, }; - await middlewareFunction( - req, - { ...JsonRpcResponseStruct.TYPE }, - () => undefined, - ); + await middleware(req, { ...JsonRpcResponseStruct.TYPE }, () => undefined); expect(req.securityAlertResponse).toBeUndefined(); expect(validateRequestWithPPOM).not.toHaveBeenCalled(); }); it('does not do validation for SIWE signature', async () => { - const middlewareFunction = createMiddleware({ + const { middleware } = createMiddleware({ securityAlertsEnabled: true, }); @@ -290,17 +296,17 @@ describe('PPOMMiddleware', () => { }); // @ts-expect-error Passing invalid input for testing purposes - await middlewareFunction(req, undefined, () => undefined); + await middleware(req, undefined, () => undefined); expect(req.securityAlertResponse).toBeUndefined(); expect(validateRequestWithPPOM).not.toHaveBeenCalled(); }); it('calls next method', async () => { - const middlewareFunction = createMiddleware(); + const { middleware } = createMiddleware(); const nextMock = jest.fn(); - await middlewareFunction( + await middleware( { ...REQUEST_MOCK, method: 'eth_sendTransaction' }, { ...JsonRpcResponseStruct.TYPE }, nextMock, @@ -315,7 +321,7 @@ describe('PPOMMiddleware', () => { const nextMock = jest.fn(); - const middlewareFunction = createMiddleware({ error }); + const { middleware } = createMiddleware({ error }); const req = { ...REQUEST_MOCK, @@ -323,7 +329,7 @@ describe('PPOMMiddleware', () => { securityAlertResponse: undefined, }; - await middlewareFunction(req, { ...JsonRpcResponseStruct.TYPE }, nextMock); + await middleware(req, { ...JsonRpcResponseStruct.TYPE }, nextMock); expect(req.securityAlertResponse).toStrictEqual( SECURITY_ALERT_RESPONSE_MOCK, diff --git a/app/scripts/lib/ppom/ppom-middleware.ts b/app/scripts/lib/ppom/ppom-middleware.ts index d623d4868af5..5b9107337a05 100644 --- a/app/scripts/lib/ppom/ppom-middleware.ts +++ b/app/scripts/lib/ppom/ppom-middleware.ts @@ -1,6 +1,9 @@ import { AccountsController } from '@metamask/accounts-controller'; import { PPOMController } from '@metamask/ppom-validator'; -import { NetworkController } from '@metamask/network-controller'; +import { + NetworkClientId, + NetworkController, +} from '@metamask/network-controller'; import { Json, JsonRpcParams, @@ -14,7 +17,6 @@ import { SIGNING_METHODS } from '../../../../shared/constants/transaction'; import PreferencesController from '../../controllers/preferences-controller'; import { AppStateController } from '../../controllers/app-state'; import { LOADING_SECURITY_ALERT_RESPONSE } from '../../../../shared/constants/security-provider'; -import { getProviderConfig } from '../../../../ui/ducks/metamask/metamask'; import { trace, TraceContext, TraceName } from '../../../../shared/lib/trace'; import { generateSecurityAlertId, @@ -33,6 +35,7 @@ const CONFIRMATION_METHODS = Object.freeze([ export type PPOMMiddlewareRequest< Params extends JsonRpcParams = JsonRpcParams, > = Required> & { + networkClientId: NetworkClientId; securityAlertResponse?: SecurityAlertResponse | undefined; traceContext?: TraceContext; }; @@ -78,9 +81,13 @@ export function createPPOMMiddleware< const securityAlertsEnabled = preferencesController.store.getState()?.securityAlertsEnabled; - const { chainId } = getProviderConfig({ - metamask: networkController.state, - }); + // This will always exist as the SelectedNetworkMiddleware + // adds networkClientId to the request before this middleware runs + const { chainId } = + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + networkController.getNetworkConfigurationByNetworkClientId( + req.networkClientId, + )!; const isSupportedChain = await isChainSupported(chainId); if ( diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 9eff0ed6d2be..c5fd4b834aed 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -5530,6 +5530,7 @@ export default class MetamaskController extends EventEmitter { engine.push(createTracingMiddleware()); + // PPOMMiddleware come after the SelectedNetworkMiddleware engine.push( createPPOMMiddleware( this.ppomController,