Skip to content

Commit 86d1bd3

Browse files
authored
fix: Add PPOM validation for deeplink requests (#22473)
<!-- 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** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR aims to add PPOM validation requests for deeplinks. ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Fixes: MetaMask/mobile-planning#2370 Fixes: #17358 ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **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** - [X] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [X] I’ve included tests if applicable - [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [X] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Integrates PPOM validation into deeplink transfer/approve flows, generating a securityAlertId and passing it to addTransaction, with refactors and tests. > > - **Deeplink handling (`app/components/Views/confirmations/utils/deeplink.ts`)**: > - Add `validateWithPPOM` to build a PPOM request (with uuid-based `securityAlertId`) and call `ppomUtil.validateRequest`. > - Pass returned `securityAlertResponse` to `addTransaction` for both native and ERC20 transfers. > - Refactor tx construction to derive `txParams` and `transactionType`; downgrade duplicate-request log from error to log. > - **Approve flow (`app/core/DeeplinkManager/TransactionManager/approveTransaction.ts`)**: > - Compute `chainId`/`networkClientId`, call `validateWithPPOM`, and include `securityAlertResponse` in `addTransaction`. > - **Tests**: > - Extend deeplink and approve tests to mock PPOM and UUID, assert PPOM validation payload, `securityAlertResponse`, and `networkClientId` wiring. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit daa4941. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 3602c24 commit 86d1bd3

File tree

4 files changed

+181
-34
lines changed

4 files changed

+181
-34
lines changed

app/components/Views/confirmations/utils/deeplink.test.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@ import { ETH_ACTIONS } from '../../../../constants/deeplinks';
44
import { selectConfirmationRedesignFlagsFromRemoteFeatureFlags } from '../../../../selectors/featureFlagController/confirmations';
55
import Engine from '../../../../core/Engine';
66
import { generateTransferData } from '../../../../util/transactions';
7+
import ppomUtil from '../../../../lib/ppom/ppom-util';
78

89
import {
910
addTransactionForDeeplink,
1011
isDeeplinkRedesignedConfirmationCompatible,
12+
validateWithPPOM,
1113
type DeeplinkRequest,
1214
} from './deeplink';
1315

@@ -53,6 +55,14 @@ jest.mock('../../../../util/transactions', () => ({
5355
generateTransferData: jest.fn(),
5456
}));
5557

58+
jest.mock('../../../../lib/ppom/ppom-util', () => ({
59+
validateRequest: jest.fn(),
60+
}));
61+
62+
jest.mock('uuid', () => ({
63+
v4: jest.fn(),
64+
}));
65+
5666
describe('isDeeplinkRedesignedConfirmationCompatible', () => {
5767
const enabledTransferFlags = {
5868
approve: false,
@@ -161,6 +171,9 @@ describe('addTransactionForDeeplink', () => {
161171
networkClientId: 'another-network',
162172
origin: ORIGIN_MOCK,
163173
type: TransactionType.simpleSend,
174+
securityAlertResponse: {
175+
securityAlertId: undefined,
176+
},
164177
},
165178
);
166179
});
@@ -184,6 +197,9 @@ describe('addTransactionForDeeplink', () => {
184197
networkClientId: 'mainnet',
185198
origin: ORIGIN_MOCK,
186199
type: TransactionType.simpleSend,
200+
securityAlertResponse: {
201+
securityAlertId: undefined,
202+
},
187203
},
188204
);
189205
});
@@ -245,6 +261,50 @@ describe('addTransactionForDeeplink', () => {
245261
networkClientId: 'mainnet',
246262
origin: ORIGIN_MOCK,
247263
type: TransactionType.tokenMethodTransfer,
264+
securityAlertResponse: {
265+
securityAlertId: undefined,
266+
},
267+
},
268+
);
269+
});
270+
});
271+
272+
describe('validateWithPPOM', () => {
273+
const mockPpomUtil = jest.mocked(ppomUtil);
274+
const mockUuid = jest.requireMock('uuid').v4;
275+
276+
const txParams = {
277+
from: '0x0b2C639c533813f4Aa9D7837CAf62653d097Ff85',
278+
to: '0x6D404AfE1a6A07Aa3CbcBf9Fd027671Df628ebFc',
279+
value: '0x3e8',
280+
};
281+
const origin = 'example.test-dapp.com';
282+
const chainId = '0x1';
283+
const networkClientId = 'mainnet';
284+
285+
beforeEach(() => {
286+
jest.clearAllMocks();
287+
mockUuid.mockReturnValue('test-uuid-123');
288+
});
289+
290+
it('calls ppomUtil.validateRequest with correct parameters', () => {
291+
validateWithPPOM({ txParams, origin, chainId, networkClientId });
292+
293+
expect(mockPpomUtil.validateRequest).toHaveBeenCalledWith(
294+
{
295+
id: 'test-uuid-123',
296+
jsonrpc: '2.0',
297+
method: 'eth_sendTransaction',
298+
origin: 'example.test-dapp.com',
299+
params: [txParams],
300+
},
301+
{
302+
transactionMeta: {
303+
chainId: '0x1',
304+
networkClientId: 'mainnet',
305+
txParams,
306+
},
307+
securityAlertId: 'test-uuid-123',
248308
},
249309
);
250310
});

app/components/Views/confirmations/utils/deeplink.ts

Lines changed: 71 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,13 @@ import {
22
CHAIN_IDS,
33
TransactionType,
44
type TransactionParams,
5+
type SecurityAlertResponse,
6+
type TransactionMeta,
57
} from '@metamask/transaction-controller';
68
import { Hex } from '@metamask/utils';
79
import { toHex } from '@metamask/controller-utils';
810
import { ParseOutput } from 'eth-url-parser';
11+
import { v4 as uuid } from 'uuid';
912

1013
import { selectConfirmationRedesignFlagsFromRemoteFeatureFlags } from '../../../../selectors/featureFlagController/confirmations';
1114
import { addTransaction } from '../../../../util/transaction-controller';
@@ -15,6 +18,7 @@ import { getGlobalNetworkClientId } from '../../../../util/networks/global-netwo
1518
import { ETH_ACTIONS } from '../../../../constants/deeplinks';
1619
import Engine from '../../../../core/Engine';
1720
import Logger from '../../../../util/Logger';
21+
import ppomUtil from '../../../../lib/ppom/ppom-util';
1822

1923
export type DeeplinkRequest = ParseOutput & { origin: string };
2024

@@ -68,7 +72,7 @@ export async function addTransactionForDeeplink({
6872

6973
// Temporary solution for preventing back to back deeplink requests
7074
if (isAddingDeeplinkTransaction) {
71-
Logger.error(new Error('Cannot add another deeplink transaction'));
75+
Logger.log('Cannot add another deeplink transaction');
7276
return;
7377
}
7478

@@ -92,36 +96,72 @@ export async function addTransactionForDeeplink({
9296
parameters?.address ?? '',
9397
);
9498

95-
if (function_name === ETH_ACTIONS.TRANSFER) {
96-
// ERC20 transfer
97-
const txParams: TransactionParams = {
98-
from,
99-
to,
100-
data: generateTransferData('transfer', {
101-
toAddress: checkSummedParamAddress,
102-
amount: toHex(parameters?.uint256 as string),
103-
}),
104-
};
105-
106-
await addTransaction(txParams, {
107-
networkClientId,
108-
origin,
109-
type: TransactionType.tokenMethodTransfer,
110-
});
111-
} else {
112-
// Native transfer
113-
const txParams: TransactionParams = {
114-
from,
115-
to,
116-
value: toHex(parameters?.value as string),
117-
};
118-
119-
await addTransaction(txParams, {
120-
networkClientId,
121-
origin,
122-
type: TransactionType.simpleSend,
123-
});
124-
}
99+
const isErc20Transfer = function_name === ETH_ACTIONS.TRANSFER;
100+
101+
const txParams: TransactionParams = isErc20Transfer
102+
? {
103+
from,
104+
to,
105+
data: generateTransferData('transfer', {
106+
toAddress: checkSummedParamAddress,
107+
amount: toHex(parameters?.uint256 as string),
108+
}),
109+
}
110+
: {
111+
from,
112+
to,
113+
value: toHex(parameters?.value as string),
114+
};
115+
116+
const transactionType = isErc20Transfer
117+
? TransactionType.tokenMethodTransfer
118+
: TransactionType.simpleSend;
119+
120+
const securityAlertResponse = validateWithPPOM({
121+
txParams,
122+
origin,
123+
chainId,
124+
networkClientId,
125+
});
126+
127+
await addTransaction(txParams, {
128+
networkClientId,
129+
origin,
130+
type: transactionType,
131+
securityAlertResponse,
132+
});
125133

126134
isAddingDeeplinkTransaction = false;
127135
}
136+
137+
export function validateWithPPOM({
138+
txParams,
139+
origin,
140+
chainId,
141+
networkClientId,
142+
}: {
143+
txParams: TransactionParams;
144+
origin: string;
145+
chainId: Hex;
146+
networkClientId: string;
147+
}) {
148+
const securityAlertId = uuid();
149+
ppomUtil.validateRequest(
150+
{
151+
id: securityAlertId,
152+
jsonrpc: '2.0',
153+
method: 'eth_sendTransaction',
154+
origin,
155+
params: [txParams],
156+
},
157+
{
158+
transactionMeta: {
159+
chainId,
160+
networkClientId,
161+
txParams,
162+
} as TransactionMeta,
163+
securityAlertId,
164+
},
165+
);
166+
return { securityAlertId } as SecurityAlertResponse;
167+
}

app/core/DeeplinkManager/TransactionManager/approveTransaction.test.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import approveTransaction from './approveTransaction';
88
import { addTransaction } from '../../../util/transaction-controller';
99
import { createMockInternalAccount } from '../../../util/test/accountsControllerTestUtils';
1010
import { toHex } from '@metamask/controller-utils';
11+
import { validateWithPPOM } from '../../../components/Views/confirmations/utils/deeplink';
1112

1213
const MOCK_SENDER_ADDRESS = '0xMockSenderAddress';
1314
const MOCK_TARGET_ADDRESS = '0xTargetAddress';
@@ -46,6 +47,7 @@ jest.mock('../../../util/transaction-controller', () => ({
4647
__esModule: true,
4748
addTransaction: jest.fn(),
4849
}));
50+
jest.mock('../../../components/Views/confirmations/utils/deeplink');
4951

5052
const mockEthUrl = {
5153
parameters: { uint256: '123', address: '0xMockAddress' },
@@ -85,11 +87,19 @@ describe('approveTransaction', () => {
8587
'showSimpleNotification',
8688
);
8789

90+
const spyValidateWithPPOM = validateWithPPOM as jest.Mock;
91+
92+
const spyFindNetworkClientIdByChainId = jest.spyOn(
93+
Engine.context.NetworkController,
94+
'findNetworkClientIdByChainId',
95+
);
96+
8897
beforeEach(() => {
8998
jest.clearAllMocks();
9099

91100
spyGetNetworkTypeById.mockReturnValue('fakeNetworkType');
92101
spyGenerateApproveData.mockReturnValue('fakeApproveData');
102+
spyFindNetworkClientIdByChainId.mockReturnValue('mockNetworkClientId');
93103
});
94104

95105
it('should call setProviderType with the correct network type', async () => {
@@ -151,7 +161,9 @@ describe('approveTransaction', () => {
151161
},
152162
{
153163
deviceConfirmedOn: 'metamask_mobile',
164+
networkClientId: 'mockNetworkClientId',
154165
origin: mockOrigin,
166+
securityAlertResponse: undefined,
155167
},
156168
);
157169
});
@@ -304,4 +316,30 @@ describe('approveTransaction', () => {
304316

305317
expect(spySetProviderType).toHaveBeenCalledWith(fakeNetworkType);
306318
});
319+
320+
it('calls validateWithPPOM with the correct parameters', async () => {
321+
spyGetAddress.mockResolvedValue('0xMockAddress');
322+
323+
await approveTransaction({
324+
// TODO: Replace "any" with type
325+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
326+
deeplinkManager: mockDeeplinkManager as any,
327+
// TODO: Replace "any" with type
328+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
329+
ethUrl: mockEthUrl as any,
330+
origin: mockOrigin,
331+
});
332+
333+
expect(spyValidateWithPPOM).toHaveBeenCalledWith({
334+
txParams: {
335+
to: MOCK_TARGET_ADDRESS,
336+
from: MOCK_SENDER_ADDRESS,
337+
value: '0x0',
338+
data: 'fakeApproveData',
339+
},
340+
origin: mockOrigin,
341+
chainId: toHex(mockEthUrl.chain_id),
342+
networkClientId: 'mockNetworkClientId',
343+
});
344+
});
307345
});

app/core/DeeplinkManager/TransactionManager/approveTransaction.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { generateApprovalData } from '../../../util/transactions';
33
import { ParseOutput } from 'eth-url-parser';
44
import { strings } from '../../../../locales/i18n';
55
import { getAddress } from '../../../util/address';
6+
import { validateWithPPOM } from '../../../components/Views/confirmations/utils/deeplink';
67
import { addTransaction } from '../../../util/transaction-controller';
78
import DeeplinkManager from '../DeeplinkManager';
89
import Engine from '../../Engine';
@@ -70,14 +71,22 @@ async function approveTransaction({
7071
data: generateApprovalData({ spender: spenderAddress, value }),
7172
};
7273

73-
const networkClientId = NetworkController.findNetworkClientIdByChainId(
74-
toHexOrFallback(chain_id as string),
75-
);
74+
const chainId = toHexOrFallback(chain_id as string);
75+
const networkClientId =
76+
NetworkController.findNetworkClientIdByChainId(chainId);
77+
78+
const securityAlertResponse = validateWithPPOM({
79+
txParams,
80+
origin,
81+
chainId,
82+
networkClientId,
83+
});
7684

7785
addTransaction(txParams, {
7886
deviceConfirmedOn: WalletDevice.MM_MOBILE,
7987
networkClientId,
8088
origin,
89+
securityAlertResponse,
8190
});
8291
}
8392

0 commit comments

Comments
 (0)