Skip to content

Commit

Permalink
fix: add source when local PPOM fails (#12460)
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**

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.

<!--
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?
-->

## **Related issues**

Fixes: MetaMask/mobile-planning#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)

<!-- 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.
  • Loading branch information
vinistevam authored Nov 29, 2024
1 parent 9b2bcd5 commit a6e7175
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 10 deletions.
57 changes: 54 additions & 3 deletions app/lib/ppom/ppom-util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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,
});
});
});
});
});
19 changes: 12 additions & 7 deletions app/lib/ppom/ppom-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,19 @@ async function validateWithController(
ppomController: PPOMController,
request: PPOMRequest,
): Promise<SecurityAlertResponse> {
const response = (await ppomController.usePPOM((ppom) =>
ppom.validateJsonRpc(request as unknown as Record<string, unknown>),
)) as SecurityAlertResponse;
try{
const response = (await ppomController.usePPOM((ppom) =>
ppom.validateJsonRpc(request as unknown as Record<string, unknown>),
)) 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(
Expand Down

0 comments on commit a6e7175

Please sign in to comment.