Skip to content

fix: remove SECURITY_ALERTS_API_ENABLED flag #14221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .e2e.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,3 @@ export MM_TEST_ACCOUNT_SRP='word1 word... word12'
export MM_TEST_ACCOUNT_ADDRESS='0x...'
export MM_TEST_ACCOUNT_PRIVATE_KEY=''
export IS_TEST="true"
# Temporary mechanism to enable security alerts API prior to release.
export MM_SECURITY_ALERTS_API_ENABLED="true"
3 changes: 0 additions & 3 deletions .js.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,6 @@ export SECURITY_ALERTS_API_URL="https://security-alerts.api.cx.metamask.io"
# Enable Portfolio View
export PORTFOLIO_VIEW="true"


# Temporary mechanism to enable security alerts API prior to release.
export MM_SECURITY_ALERTS_API_ENABLED="true"
# Firebase
export GOOGLE_SERVICES_B64_ANDROID=""
export GOOGLE_SERVICES_B64_IOS=""
Expand Down
97 changes: 32 additions & 65 deletions app/lib/ppom/ppom-util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,6 @@ describe('PPOM Utils', () => {
securityAlertAPI.validateWithSecurityAlertsAPI,
);

const isSecurityAlertsEnabledMock = jest.mocked(
securityAlertAPI.isSecurityAlertsAPIEnabled,
);

const mockIsBlockaidFeatureEnabled = jest.mocked(isBlockaidFeatureEnabled);

const normalizeTransactionParamsMock = jest.mocked(
Expand Down Expand Up @@ -170,6 +166,12 @@ describe('PPOM Utils', () => {

normalizeTransactionParamsMock.mockImplementation((params) => params);
mockIsBlockaidFeatureEnabled.mockResolvedValue(true);

validateWithSecurityAlertsAPIMock.mockResolvedValue({
result_type: ResultType.Malicious,
reason: Reason.other,
source: SecurityAlertSource.API,
});
});

afterEach(() => {
Expand All @@ -186,9 +188,7 @@ describe('PPOM Utils', () => {
MockEngine.context.PreferencesController.state.securityAlertsEnabled =
false;
await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK);
expect(MockEngine.context.PPOMController?.usePPOM).toHaveBeenCalledTimes(
0,
);
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(0);
expect(spyTransactionAction).toHaveBeenCalledTimes(0);
});

Expand All @@ -205,9 +205,7 @@ describe('PPOM Utils', () => {
},
]);
await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK);
expect(MockEngine.context.PPOMController?.usePPOM).toHaveBeenCalledTimes(
0,
);
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(0);
expect(spyTransactionAction).toHaveBeenCalledTimes(0);
MockEngine.context.AccountsController.listAccounts = jest
.fn()
Expand All @@ -224,9 +222,7 @@ describe('PPOM Utils', () => {
.spyOn(NetworkControllerSelectors, 'selectChainId')
.mockReturnValue('0xfa');
await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK);
expect(MockEngine.context.PPOMController?.usePPOM).toHaveBeenCalledTimes(
0,
);
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(0);
expect(spyTransactionAction).toHaveBeenCalledTimes(0);
});

Expand All @@ -244,9 +240,7 @@ describe('PPOM Utils', () => {
},
CHAIN_ID_MOCK,
);
expect(MockEngine.context.PPOMController?.usePPOM).toHaveBeenCalledTimes(
0,
);
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(0);
expect(spyTransactionAction).toHaveBeenCalledTimes(0);
});

Expand All @@ -256,19 +250,10 @@ describe('PPOM Utils', () => {
'setTransactionSecurityAlertResponse',
);
await PPOMUtil.validateRequest(mockRequest);
expect(MockEngine.context.PPOMController?.usePPOM).toHaveBeenCalledTimes(
0,
);
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(0);
expect(spyTransactionAction).toHaveBeenCalledTimes(1);
});

it('should invoke PPOMController usePPOM if securityAlertsEnabled is true', async () => {
await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK);
expect(MockEngine.context.PPOMController?.usePPOM).toHaveBeenCalledTimes(
1,
);
});

it('should update transaction with validation result', async () => {
const spy = jest.spyOn(
TransactionActions,
Expand All @@ -290,17 +275,6 @@ describe('PPOM Utils', () => {
data: '0xabcd',
};

const validateMock = jest.fn();

const ppomMock = {
validateJsonRpc: validateMock,
};

MockEngine.context.PPOMController?.usePPOM.mockImplementation(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(callback: any) => callback(ppomMock),
);

normalizeTransactionParamsMock.mockReturnValue(
normalizedTransactionParamsMock,
);
Expand All @@ -312,11 +286,14 @@ describe('PPOM Utils', () => {
mockRequest.params[0],
);

expect(validateMock).toHaveBeenCalledTimes(1);
expect(validateMock).toHaveBeenCalledWith({
...mockRequest,
params: [normalizedTransactionParamsMock],
});
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(1);
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledWith(
CHAIN_ID_MOCK,
{
...mockRequest,
params: [normalizedTransactionParamsMock],
},
);
});

it('logs error if normalization fails', async () => {
Expand All @@ -336,17 +313,6 @@ describe('PPOM Utils', () => {
});

it('normalizes transaction request origin before validation', async () => {
const validateMock = jest.fn();

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,
Expand All @@ -360,19 +326,18 @@ describe('PPOM Utils', () => {
mockRequest.params[0],
);

expect(validateMock).toHaveBeenCalledTimes(1);
expect(validateMock).toHaveBeenCalledWith({
...mockRequest,
});
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(1);
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledWith(
CHAIN_ID_MOCK,
{
...mockRequest,
},
);
});

it('uses security alerts API if enabled', async () => {
isSecurityAlertsEnabledMock.mockReturnValue(true);

await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK);

expect(MockEngine.context.PPOMController?.usePPOM).not.toHaveBeenCalled();

expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(1);
expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledWith(
CHAIN_ID_MOCK,
Expand All @@ -381,8 +346,6 @@ describe('PPOM Utils', () => {
});

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

validateWithSecurityAlertsAPIMock.mockRejectedValue(
new Error('Test Error'),
);
Expand Down Expand Up @@ -410,6 +373,9 @@ describe('PPOM Utils', () => {
});

it('sets security alerts response to failed when security alerts API and controller PPOM throws', async () => {
validateWithSecurityAlertsAPIMock.mockRejectedValue(
new Error('Test Error'),
);
const spy = jest.spyOn(
TransactionActions,
'setTransactionSecurityAlertResponse',
Expand All @@ -427,6 +393,9 @@ describe('PPOM Utils', () => {
);

await PPOMUtil.validateRequest(mockRequest, CHAIN_ID_MOCK);

expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(1);
expect(MockEngine.context.PPOMController?.usePPOM).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledTimes(2);
expect(spy).toHaveBeenCalledWith(CHAIN_ID_MOCK, {
chainId: CHAIN_ID_MOCK,
Expand All @@ -441,8 +410,6 @@ describe('PPOM Utils', () => {
it.each([METHOD_SIGN_TYPED_DATA_V3, METHOD_SIGN_TYPED_DATA_V4])(
'sanitizes request params if method is %s',
async (method: string) => {
isSecurityAlertsEnabledMock.mockReturnValue(true);

const firstTwoParams = [
SIGN_TYPED_DATA_PARAMS_MOCK_1,
SIGN_TYPED_DATA_PARAMS_MOCK_2,
Expand Down
5 changes: 1 addition & 4 deletions app/lib/ppom/ppom-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
import { WALLET_CONNECT_ORIGIN } from '../../util/walletconnect';
import AppConstants from '../../core/AppConstants';
import {
isSecurityAlertsAPIEnabled,
validateWithSecurityAlertsAPI,
} from './security-alerts-api';
import { PPOMController } from '@metamask/ppom-validator';
Expand Down Expand Up @@ -111,9 +110,7 @@ async function validateRequest(req: PPOMRequest, transactionId?: string) {

const normalizedRequest = normalizeRequest(req);

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

securityAlertResponse = {
...securityAlertResponse,
Expand Down
4 changes: 0 additions & 4 deletions app/lib/ppom/security-alerts-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,6 @@ export interface SecurityAlertsAPIRequest {
params: unknown[];
}

export function isSecurityAlertsAPIEnabled() {
return process.env.MM_SECURITY_ALERTS_API_ENABLED === 'true';
}

export async function validateWithSecurityAlertsAPI(
chainId: string,
body: SecurityAlertsAPIRequest,
Expand Down
2 changes: 1 addition & 1 deletion bitrise.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2268,7 +2268,7 @@ app:
MM_PERMISSIONS_SETTINGS_V1_ENABLED: false
- opts:
is_expand: false
MM_SECURITY_ALERTS_API_ENABLED: true
MM_STABLECOIN_LENDING_UI_ENABLED: false
- opts:
is_expand: false
PROJECT_LOCATION: android
Expand Down
1 change: 0 additions & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ process.env.SEGMENT_REGULATIONS_ENDPOINT = 'TestRegulationsEndpoint';

process.env.MM_FOX_CODE = 'EXAMPLE_FOX_CODE';

process.env.MM_SECURITY_ALERTS_API_ENABLED = 'true';
process.env.PORTFOLIO_VIEW = 'true';
process.env.SECURITY_ALERTS_API_URL = 'https://example.com';

Expand Down
Loading