From a6e71750616d37490aa57ae04a1b2faf4b2e1b34 Mon Sep 17 00:00:00 2001 From: Vinicius Stevam <45455812+vinistevam@users.noreply.github.com> Date: Fri, 29 Nov 2024 14:51:03 +0000 Subject: [PATCH] fix: add source when local PPOM fails (#12460) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR fixes an issue where the source property was not being set correctly when an error occurred during local PPOM validation. Previously, if the local PPOM failed, the error handling mechanism would add an error message but omit the source property, making it unclear whether the error originated from the API or the local PPOM. **Changes introduced:** - Modified error handling for the local PPOM validation to ensure that the source property is correctly set to "local" when an error occurs during local validation. - Ensured that the source property is properly set to "api" in case of an API failure. ## **Related issues** Fixes: https://github.com/MetaMask/mobile-planning/issues/2058 ## **Manual testing steps** 1. Go to the live test dapp 2. Trigger a send transaction with suggested gas values (ie Send Legacy or send EIP1559) 3. Check Segment Obs: We need to block the network calls for `https://security-alerts.api.cx.metamask.io/` and `https://static.cx.metamask.io/api/v1/confirmations/ppom/ppom_version.json` in the Chrome dev tools > network tab. ## **Screenshots/Recordings** [security-alerts-local.webm](https://github.com/user-attachments/assets/f1fbfe8c-b7ca-4056-99ff-06ab5a9cba6a) ### **Before** ### **After** ## **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. --- app/lib/ppom/ppom-util.test.ts | 57 ++++++++++++++++++++++++++++++++-- app/lib/ppom/ppom-util.ts | 19 +++++++----- 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/app/lib/ppom/ppom-util.test.ts b/app/lib/ppom/ppom-util.test.ts index 95241217c88..27340f43e23 100644 --- a/app/lib/ppom/ppom-util.test.ts +++ b/app/lib/ppom/ppom-util.test.ts @@ -13,6 +13,12 @@ import { RpcEndpointType, } from '@metamask/network-controller'; import { NETWORKS_CHAIN_ID } from '../../constants/network'; +import { + Reason, + ResultType, + SecurityAlertSource, +} from '../../components/Views/confirmations/components/BlockaidBanner/BlockaidBanner.types'; +import Logger from '../../util/Logger'; const CHAIN_ID_MOCK = '0x1'; @@ -176,8 +182,8 @@ describe('PPOM Utils', () => { MockEngine.context.PreferencesController.state.securityAlertsEnabled = false; await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK); - expect(MockEngine.context.PPOMController?.usePPOM).toBeCalledTimes(0); - expect(spyTransactionAction).toBeCalledTimes(0); + expect(MockEngine.context.PPOMController?.usePPOM).toHaveBeenCalledTimes(0); + expect(spyTransactionAction).toHaveBeenCalledTimes(0); }); it('should not validate if request is send to users own account ', async () => { @@ -307,6 +313,22 @@ describe('PPOM Utils', () => { }); }); + it('logs error if normalization fails', async () => { + const error = new Error('Test Error'); + normalizeTransactionParamsMock.mockImplementation(() => { + throw error; + }); + + const spyLogger = jest.spyOn(Logger, 'log'); + + await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK); + + expect(spyLogger).toHaveBeenCalledTimes(1); + expect(spyLogger).toHaveBeenCalledWith( + `Error validating JSON RPC using PPOM: ${error}`, + ); + }); + it('normalizes transaction request origin before validation', async () => { const validateMock = jest.fn(); @@ -385,5 +407,34 @@ describe('PPOM Utils', () => { await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK); expect(spy).toHaveBeenCalledTimes(2); }); + + it('sets security alerts response to failed when security alerts API and controller PPOM throws', async () => { + const spy = jest.spyOn( + TransactionActions, + 'setTransactionSecurityAlertResponse', + ); + + const validateMock = new Error('Test Error'); + + const ppomMock = { + validateJsonRpc: validateMock, + }; + + MockEngine.context.PPOMController?.usePPOM.mockImplementation( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (callback: any) => callback(ppomMock), + ); + + await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK); + expect(spy).toHaveBeenCalledTimes(2); + expect(spy).toHaveBeenCalledWith(CHAIN_ID_MOCK, { + chainId: CHAIN_ID_MOCK, + req: mockRequest, + result_type: ResultType.Failed, + reason: Reason.failed, + description: 'Validating the confirmation failed by throwing error.', + source: SecurityAlertSource.Local, + }); + }); }); -}); +}); \ No newline at end of file diff --git a/app/lib/ppom/ppom-util.ts b/app/lib/ppom/ppom-util.ts index 86f291b3543..3a7716eb87f 100644 --- a/app/lib/ppom/ppom-util.ts +++ b/app/lib/ppom/ppom-util.ts @@ -155,14 +155,19 @@ async function validateWithController( ppomController: PPOMController, request: PPOMRequest, ): Promise { - const response = (await ppomController.usePPOM((ppom) => - ppom.validateJsonRpc(request as unknown as Record), - )) as SecurityAlertResponse; + try{ + const response = (await ppomController.usePPOM((ppom) => + ppom.validateJsonRpc(request as unknown as Record), + )) as SecurityAlertResponse; - return { - ...response, - source: SecurityAlertSource.Local, - }; + return { + ...response, + source: SecurityAlertSource.Local, + }; + } catch (e) { + Logger.log(`Error validating request with PPOM: ${e}`); + return {...SECURITY_ALERT_RESPONSE_FAILED, source: SecurityAlertSource.Local,}; + } } async function validateWithAPI(