Skip to content

Commit

Permalink
feat: support security alerts API (#25544)
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 enables the use of the Security Alerts API to validate dApp
requests, with a fallback to local PPOM validation if the API request
fails.

#### Environment Variables

Add the following variables to `.metamaskrc`:

```shell
SECURITY_ALERTS_API_URL='http://localhost:3000'
SECURITY_ALERTS_API_ENABLED='true'
```

#### Additional Changes
Introduces the security_alert_source property to transaction and
signature events, indicating api or local as the source.
#### Related Repository
Refer to the [Security Alerts API
repository](https://github.com/consensys-vertical-apps/va-mmcx-security-alerts-api)
for more details.

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

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25544?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#2514
MetaMask/MetaMask-planning#2515

## **Manual testing steps**

1. Test blockaid regression

2. add the envs 
```shell
SECURITY_ALERTS_API_URL='https://security-alerts.dev-api.cx.metamask.io'
SECURITY_ALERTS_API_ENABLED='true'
```
- Go to test dapp and trigger on of the malicious signatures
- To verify in chrome go to dev tools > network. Search for
`security-alerts` and find the call to the API service.

Existing PPOM logic should function as before, even with the above
environment variables added, due to the fallback to the controller in
the event of an error.

## **Screenshots/Recordings**

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


![image](https://github.com/MetaMask/metamask-extension/assets/45455812/ace14a9e-32e4-4489-a9a0-c648128674bc)

### **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
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.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-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
vinistevam authored Jul 5, 2024
1 parent a935093 commit fe23ae0
Show file tree
Hide file tree
Showing 18 changed files with 334 additions and 65 deletions.
5 changes: 5 additions & 0 deletions .metamaskrc.dist
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,10 @@ BLOCKAID_PUBLIC_KEY=

ENABLE_CONFIRMATION_REDESIGN=

; URL of security alerts API used to validate dApp requests
; SECURITY_ALERTS_API_URL='http://localhost:3000'
; Temporary mechanism to enable security alerts API prior to release
; SECURITY_ALERTS_API_ENABLED='true'

; Enables the Settings Page - Developer Options
; ENABLE_SETTINGS_PAGE_DEV_OPTIONS=true
1 change: 1 addition & 0 deletions app/scripts/lib/ppom/ppom-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export function createPPOMMiddleware<
ppomController,
request: req,
securityAlertId,
chainId,
}).then((securityAlertResponse) => {
updateSecurityAlertResponse(
req.method,
Expand Down
82 changes: 76 additions & 6 deletions app/scripts/lib/ppom/ppom-util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { Message } from '@metamask/message-manager';
import {
BlockaidReason,
BlockaidResultType,
SecurityAlertSource,
} from '../../../../shared/constants/security-provider';
import { AppStateController } from '../../controllers/app-state';
import {
Expand All @@ -19,6 +20,7 @@ import {
validateRequestWithPPOM,
} from './ppom-util';
import { SecurityAlertResponse } from './types';
import * as securityAlertAPI from './security-alerts-api';

jest.mock('@metamask/transaction-controller', () => ({
...jest.requireActual('@metamask/transaction-controller'),
Expand All @@ -27,6 +29,7 @@ jest.mock('@metamask/transaction-controller', () => ({

const SECURITY_ALERT_ID_MOCK = '1234-5678';
const TRANSACTION_ID_MOCK = '123';
const CHAIN_ID_MOCK = '0x1';

const REQUEST_MOCK = {
method: 'eth_signTypedData_v4',
Expand All @@ -36,6 +39,7 @@ const REQUEST_MOCK = {
const SECURITY_ALERT_RESPONSE_MOCK: SecurityAlertResponse = {
result_type: 'success',
reason: 'success',
source: SecurityAlertSource.Local,
};

const TRANSACTION_PARAMS_MOCK_1: TransactionParams = {
Expand Down Expand Up @@ -95,10 +99,14 @@ describe('PPOM Utils', () => {
const normalizeTransactionParamsMock = jest.mocked(
normalizeTransactionParams,
);
let isSecurityAlertsEnabledMock: jest.SpyInstance;

beforeEach(() => {
jest.resetAllMocks();
jest.spyOn(console, 'error').mockImplementation(() => undefined);
isSecurityAlertsEnabledMock = jest
.spyOn(securityAlertAPI, 'isSecurityAlertsAPIEnabled')
.mockReturnValue(false);
});

describe('validateRequestWithPPOM', () => {
Expand All @@ -109,15 +117,14 @@ describe('PPOM Utils', () => {
ppom.validateJsonRpc.mockResolvedValue(SECURITY_ALERT_RESPONSE_MOCK);

ppomController.usePPOM.mockImplementation(
(callback) =>
// eslint-disable-next-line @typescript-eslint/no-explicit-any
callback(ppom as any) as any,
(callback) => callback(ppom as any) as any,
);

const response = await validateRequestWithPPOM({
ppomController,
request: REQUEST_MOCK,
securityAlertId: SECURITY_ALERT_ID_MOCK,
chainId: CHAIN_ID_MOCK,
});

expect(response).toStrictEqual({
Expand Down Expand Up @@ -145,6 +152,7 @@ describe('PPOM Utils', () => {
ppomController,
request: REQUEST_MOCK,
securityAlertId: SECURITY_ALERT_ID_MOCK,
chainId: CHAIN_ID_MOCK,
});

expect(response).toStrictEqual({
Expand All @@ -163,6 +171,7 @@ describe('PPOM Utils', () => {
ppomController,
request: REQUEST_MOCK,
securityAlertId: SECURITY_ALERT_ID_MOCK,
chainId: CHAIN_ID_MOCK,
});

expect(response).toStrictEqual({
Expand Down Expand Up @@ -194,6 +203,7 @@ describe('PPOM Utils', () => {
ppomController,
request,
securityAlertId: SECURITY_ALERT_ID_MOCK,
chainId: CHAIN_ID_MOCK,
});

expect(ppom.validateJsonRpc).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -238,8 +248,7 @@ describe('PPOM Utils', () => {
securityAlertId: SECURITY_ALERT_ID_MOCK,
securityAlertResponse: SECURITY_ALERT_RESPONSE_MOCK,
signatureController,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
transactionController: {} as any,
transactionController: {} as unknown as TransactionController,
});

expect(
Expand All @@ -261,7 +270,7 @@ describe('PPOM Utils', () => {
},
},
],
} as any);
} as unknown as TransactionController['state']);

await updateSecurityAlertResponse({
appStateController: {} as any,
Expand All @@ -281,4 +290,65 @@ describe('PPOM Utils', () => {
).toHaveBeenCalledWith(TRANSACTION_ID_MOCK, SECURITY_ALERT_RESPONSE_MOCK);
});
});

describe('validateWithAPI', () => {
const request = {
...REQUEST_MOCK,
method: 'eth_sendTransaction',
params: [TRANSACTION_PARAMS_MOCK_1],
};

it('uses security alerts API if enabled', async () => {
isSecurityAlertsEnabledMock.mockReturnValue(true);
normalizeTransactionParamsMock.mockReturnValue(TRANSACTION_PARAMS_MOCK_1);
const validateWithSecurityAlertsAPIMock = jest
.spyOn(securityAlertAPI, 'validateWithSecurityAlertsAPI')
.mockResolvedValue(SECURITY_ALERT_RESPONSE_MOCK);

const ppom = createPPOMMock();
const ppomController = createPPOMControllerMock();

await validateRequestWithPPOM({
ppomController,
request,
securityAlertId: SECURITY_ALERT_ID_MOCK,
chainId: CHAIN_ID_MOCK,
});

expect(ppomController.usePPOM).not.toHaveBeenCalled();
expect(ppom.validateJsonRpc).not.toHaveBeenCalled();

expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(1);
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledWith(
CHAIN_ID_MOCK,
request,
);
});

it('uses controller if security alerts API throws', async () => {
isSecurityAlertsEnabledMock.mockReturnValue(true);
normalizeTransactionParamsMock.mockReturnValue(TRANSACTION_PARAMS_MOCK_1);

const ppomController = createPPOMControllerMock();

const validateWithSecurityAlertsAPIMock = jest
.spyOn(securityAlertAPI, 'validateWithSecurityAlertsAPI')
.mockRejectedValue(new Error('Test Error'));

await validateRequestWithPPOM({
ppomController,
request,
securityAlertId: SECURITY_ALERT_ID_MOCK,
chainId: CHAIN_ID_MOCK,
});

expect(ppomController.usePPOM).toHaveBeenCalledTimes(1);

expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(1);
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledWith(
CHAIN_ID_MOCK,
request,
);
});
});
});
76 changes: 52 additions & 24 deletions app/scripts/lib/ppom/ppom-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,22 @@ import {
TransactionParams,
normalizeTransactionParams,
} from '@metamask/transaction-controller';
import { JsonRpcRequest } from '@metamask/utils';
import { Hex, JsonRpcRequest } from '@metamask/utils';
import { v4 as uuid } from 'uuid';
import { PPOM } from '@blockaid/ppom_release';
import { SignatureController } from '@metamask/signature-controller';
import {
BlockaidReason,
BlockaidResultType,
SecurityAlertSource,
} from '../../../../shared/constants/security-provider';
import { SIGNING_METHODS } from '../../../../shared/constants/transaction';
import { AppStateController } from '../../controllers/app-state';
import { SecurityAlertResponse } from './types';
import {
isSecurityAlertsAPIEnabled,
validateWithSecurityAlertsAPI,
} from './security-alerts-api';

const { sentry } = global;

Expand All @@ -29,17 +34,26 @@ export async function validateRequestWithPPOM({
ppomController,
request,
securityAlertId,
chainId,
}: {
ppomController: PPOMController;
request: JsonRpcRequest;
securityAlertId: string;
chainId: Hex;
}): Promise<SecurityAlertResponse> {
try {
return await ppomController.usePPOM(async (ppom: PPOM) => {
return await usePPOM(request, securityAlertId, ppom);
});
} catch (error) {
return handlePPOMError(error, 'Error validateRequestWithPPOM#usePPOM: ');
const normalizedRequest = normalizePPOMRequest(request);

const ppomResponse = isSecurityAlertsAPIEnabled()
? await validateWithAPI(ppomController, chainId, normalizedRequest)
: await validateWithController(ppomController, normalizedRequest);

return {
...ppomResponse,
securityAlertId,
};
} catch (error: unknown) {
return handlePPOMError(error, 'Error validating JSON RPC using PPOM: ');
}
}

Expand Down Expand Up @@ -97,24 +111,6 @@ export function handlePPOMError(
};
}

async function usePPOM(
request: JsonRpcRequest,
securityAlertId: string,
ppom: PPOM,
): Promise<SecurityAlertResponse> {
try {
const normalizedRequest = normalizePPOMRequest(request);
const ppomResponse = await ppom.validateJsonRpc(normalizedRequest);

return {
...ppomResponse,
securityAlertId,
};
} catch (error: unknown) {
return handlePPOMError(error, 'Error validating JSON RPC using PPOM: ');
}
}

function normalizePPOMRequest(request: JsonRpcRequest): JsonRpcRequest {
if (request.method !== METHOD_SEND_TRANSACTION) {
return request;
Expand Down Expand Up @@ -178,3 +174,35 @@ async function findConfirmationBySecurityAlertId(
await new Promise((resolve) => setTimeout(resolve, 100));
}
}

async function validateWithController(
ppomController: PPOMController,
request: JsonRpcRequest,
): Promise<SecurityAlertResponse> {
const response = (await ppomController.usePPOM((ppom: PPOM) =>
ppom.validateJsonRpc(request),
)) as SecurityAlertResponse;

return {
...response,
source: SecurityAlertSource.Local,
};
}

async function validateWithAPI(
ppomController: PPOMController,
chainId: string,
request: JsonRpcRequest,
): Promise<SecurityAlertResponse> {
try {
const response = await validateWithSecurityAlertsAPI(chainId, request);

return {
...response,
source: SecurityAlertSource.API,
};
} catch (error: unknown) {
handlePPOMError(error, `Error validating request with security alerts API`);
return await validateWithController(ppomController, request);
}
}
89 changes: 89 additions & 0 deletions app/scripts/lib/ppom/security-alerts-api.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import {
BlockaidReason,
BlockaidResultType,
} from '../../../../shared/constants/security-provider';
import {
isSecurityAlertsAPIEnabled,
validateWithSecurityAlertsAPI,
} from './security-alerts-api';

const CHAIN_ID_MOCK = '0x1';

const REQUEST_MOCK = {
method: 'eth_sendTransaction',
params: [
{
from: '0x123',
to: '0x456',
value: '0x123',
},
],
};

const RESPONSE_MOCK = {
result_type: BlockaidResultType.Errored,
reason: BlockaidReason.maliciousDomain,
description: 'Test Description',
};

describe('Security Alerts API', () => {
const fetchMock = jest.fn();

beforeEach(() => {
jest.resetAllMocks();

global.fetch = fetchMock;

fetchMock.mockResolvedValue({
ok: true,
json: async () => RESPONSE_MOCK,
});

process.env.SECURITY_ALERTS_API_URL = 'https://example.com';
});

describe('validateWithSecurityAlertsAPI', () => {
it('sends POST request', async () => {
const response = await validateWithSecurityAlertsAPI(
CHAIN_ID_MOCK,
REQUEST_MOCK,
);

expect(response).toEqual(RESPONSE_MOCK);

expect(fetchMock).toHaveBeenCalledTimes(1);
expect(fetchMock).toHaveBeenCalledWith(
`https://example.com/validate/${CHAIN_ID_MOCK}`,
expect.any(Object),
);
});

it('throws an error if response is not ok', async () => {
fetchMock.mockResolvedValue({ ok: false, status: 567 });

const responsePromise = validateWithSecurityAlertsAPI(
CHAIN_ID_MOCK,
REQUEST_MOCK,
);

await expect(responsePromise).rejects.toThrow(
'Security alerts API request failed with status: 567',
);
});

it('throws an error if SECURITY_ALERTS_API_URL is not set', async () => {
delete process.env.SECURITY_ALERTS_API_URL;

await expect(
validateWithSecurityAlertsAPI(CHAIN_ID_MOCK, REQUEST_MOCK),
).rejects.toThrow('Security alerts API URL is not set');
});

it('throws an error if SECURITY_ALERTS_API_ENABLED is false', () => {
process.env.SECURITY_ALERTS_API_ENABLED = 'false';

const isEnabled = isSecurityAlertsAPIEnabled();
expect(isEnabled).toBe(false);
});
});
});
Loading

0 comments on commit fe23ae0

Please sign in to comment.