Skip to content

Commit

Permalink
feat: use networkClientId to resolve chainId in PPOM Middleware (#27263)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **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**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **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.
  • Loading branch information
jiexi committed Sep 19, 2024
1 parent 4a55b53 commit 178c95b
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 58 deletions.
112 changes: 59 additions & 53 deletions app/scripts/lib/ppom/ppom-middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -34,22 +33,18 @@ const REQUEST_MOCK = {
params: [],
id: '',
jsonrpc: '2.0' as const,
origin: 'test.com',
networkClientId: 'networkClientId',
};

const createMiddleware = (
options: {
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 = {};

Expand All @@ -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 = {
Expand All @@ -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,
Expand All @@ -96,6 +95,16 @@ const createMiddleware = (
accountsController as any,
updateSecurityAlertResponse,
);

return {
middleware,
ppomController,
preferenceController,
networkController,
appStateController,
accountsController,
updateSecurityAlertResponse,
};
};

describe('PPOMMiddleware', () => {
Expand All @@ -121,24 +130,37 @@ 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,
method: 'eth_sendTransaction',
securityAlertResponse: undefined,
};

await middlewareFunction(
req,
{ ...JsonRpcResponseStruct.TYPE },
() => undefined,
);
await middleware(req, { ...JsonRpcResponseStruct.TYPE }, () => undefined);

await flushPromises();

Expand All @@ -151,19 +173,15 @@ 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,
method: 'eth_sendTransaction',
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(
Expand All @@ -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,
});

Expand All @@ -183,15 +201,15 @@ 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();
});

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',
});

Expand All @@ -201,37 +219,29 @@ 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,
method: 'eth_someRequest',
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,
Expand All @@ -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,
});

Expand Down Expand Up @@ -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,
Expand All @@ -315,15 +321,15 @@ describe('PPOMMiddleware', () => {

const nextMock = jest.fn();

const middlewareFunction = createMiddleware({ error });
const { middleware } = createMiddleware({ error });

const req = {
...REQUEST_MOCK,
method: 'eth_sendTransaction',
securityAlertResponse: undefined,
};

await middlewareFunction(req, { ...JsonRpcResponseStruct.TYPE }, nextMock);
await middleware(req, { ...JsonRpcResponseStruct.TYPE }, nextMock);

expect(req.securityAlertResponse).toStrictEqual(
SECURITY_ALERT_RESPONSE_MOCK,
Expand Down
17 changes: 12 additions & 5 deletions app/scripts/lib/ppom/ppom-middleware.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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,
Expand All @@ -33,6 +35,7 @@ const CONFIRMATION_METHODS = Object.freeze([
export type PPOMMiddlewareRequest<
Params extends JsonRpcParams = JsonRpcParams,
> = Required<JsonRpcRequest<Params>> & {
networkClientId: NetworkClientId;
securityAlertResponse?: SecurityAlertResponse | undefined;
traceContext?: TraceContext;
};
Expand Down Expand Up @@ -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 (
Expand Down
1 change: 1 addition & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -5530,6 +5530,7 @@ export default class MetamaskController extends EventEmitter {

engine.push(createTracingMiddleware());

// PPOMMiddleware come after the SelectedNetworkMiddleware
engine.push(
createPPOMMiddleware(
this.ppomController,
Expand Down

0 comments on commit 178c95b

Please sign in to comment.