diff --git a/app/scripts/lib/ppom/ppom-middleware.test.ts b/app/scripts/lib/ppom/ppom-middleware.test.ts index 8977c00aa3d7..16317fef0380 100644 --- a/app/scripts/lib/ppom/ppom-middleware.test.ts +++ b/app/scripts/lib/ppom/ppom-middleware.test.ts @@ -7,7 +7,6 @@ import { BlockaidReason, BlockaidResultType, } from '../../../../shared/constants/security-provider'; -import { flushPromises } from '../../../../test/lib/timer-helpers'; import { mockNetworkState } from '../../../../test/stub/networks'; import { createPPOMMiddleware, PPOMMiddlewareRequest } from './ppom-middleware'; import { @@ -105,7 +104,6 @@ const createMiddleware = ( }; describe('PPOMMiddleware', () => { - const validateRequestWithPPOMMock = jest.mocked(validateRequestWithPPOM); const generateSecurityAlertIdMock = jest.mocked(generateSecurityAlertId); const handlePPOMErrorMock = jest.mocked(handlePPOMError); const isChainSupportedMock = jest.mocked(isChainSupported); @@ -114,7 +112,6 @@ describe('PPOMMiddleware', () => { beforeEach(() => { jest.resetAllMocks(); - validateRequestWithPPOMMock.mockResolvedValue(SECURITY_ALERT_RESPONSE_MOCK); generateSecurityAlertIdMock.mockReturnValue(SECURITY_ALERT_ID_MOCK); handlePPOMErrorMock.mockReturnValue(SECURITY_ALERT_RESPONSE_MOCK); isChainSupportedMock.mockResolvedValue(true); @@ -129,38 +126,13 @@ describe('PPOMMiddleware', () => { }; }); - it('updates alert response after validating request', async () => { + it('adds checking chain response to confirmation requests while validation is in progress', async () => { const updateSecurityAlertResponse = jest.fn(); const middlewareFunction = createMiddleware({ updateSecurityAlertResponse, }); - const req = { - ...REQUEST_MOCK, - method: 'eth_sendTransaction', - securityAlertResponse: undefined, - }; - - await middlewareFunction( - req, - { ...JsonRpcResponseStruct.TYPE }, - () => undefined, - ); - - await flushPromises(); - - expect(updateSecurityAlertResponse).toHaveBeenCalledTimes(1); - expect(updateSecurityAlertResponse).toHaveBeenCalledWith( - req.method, - SECURITY_ALERT_ID_MOCK, - SECURITY_ALERT_RESPONSE_MOCK, - ); - }); - - it('adds loading response to confirmation requests while validation is in progress', async () => { - const middlewareFunction = createMiddleware(); - const req: PPOMMiddlewareRequest<(string | { to: string })[]> = { ...REQUEST_MOCK, method: 'eth_sendTransaction', @@ -173,7 +145,9 @@ describe('PPOMMiddleware', () => { () => undefined, ); - expect(req.securityAlertResponse?.reason).toBe(BlockaidReason.inProgress); + expect(req.securityAlertResponse?.reason).toBe( + BlockaidReason.checkingChain, + ); expect(req.securityAlertResponse?.result_type).toBe( BlockaidResultType.Loading, ); @@ -197,50 +171,6 @@ describe('PPOMMiddleware', () => { expect(validateRequestWithPPOM).not.toHaveBeenCalled(); }); - it('does not do validation if unable to get the chainId from the network provider config', async () => { - isChainSupportedMock.mockResolvedValue(false); - const middlewareFunction = createMiddleware({ - chainId: null, - }); - - const req = { - ...REQUEST_MOCK, - method: 'eth_sendTransaction', - securityAlertResponse: undefined, - }; - - await middlewareFunction( - req, - { ...JsonRpcResponseStruct.TYPE }, - () => undefined, - ); - - expect(req.securityAlertResponse).toBeUndefined(); - expect(validateRequestWithPPOM).not.toHaveBeenCalled(); - }); - - it('does not do validation if user is not on a supported network', async () => { - isChainSupportedMock.mockResolvedValue(false); - const middlewareFunction = createMiddleware({ - chainId: '0x2', - }); - - const req = { - ...REQUEST_MOCK, - method: 'eth_sendTransaction', - securityAlertResponse: undefined, - }; - - await middlewareFunction( - req, - { ...JsonRpcResponseStruct.TYPE }, - () => undefined, - ); - - expect(req.securityAlertResponse).toBeUndefined(); - expect(validateRequestWithPPOM).not.toHaveBeenCalled(); - }); - it('does not do validation when request is not for confirmation method', async () => { const middlewareFunction = createMiddleware(); diff --git a/app/scripts/lib/ppom/ppom-middleware.ts b/app/scripts/lib/ppom/ppom-middleware.ts index ebfdbe3f04d7..44c7a8a965c4 100644 --- a/app/scripts/lib/ppom/ppom-middleware.ts +++ b/app/scripts/lib/ppom/ppom-middleware.ts @@ -13,17 +13,16 @@ import { MESSAGE_TYPE } from '../../../../shared/constants/app'; import { SIGNING_METHODS } from '../../../../shared/constants/transaction'; import { PreferencesController } from '../../controllers/preferences-controller'; import { AppStateController } from '../../controllers/app-state-controller'; -import { LOADING_SECURITY_ALERT_RESPONSE } from '../../../../shared/constants/security-provider'; +import { SECURITY_ALERT_RESPONSE_CHECKING_CHAIN } from '../../../../shared/constants/security-provider'; // eslint-disable-next-line import/no-restricted-paths import { getProviderConfig } from '../../../../ui/ducks/metamask/metamask'; import { trace, TraceContext, TraceName } from '../../../../shared/lib/trace'; import { generateSecurityAlertId, handlePPOMError, - isChainSupported, validateRequestWithPPOM, } from './ppom-util'; -import { SecurityAlertResponse } from './types'; +import { SecurityAlertResponse, UpdateSecurityAlertResponse } from './types'; const CONFIRMATION_METHODS = Object.freeze([ 'eth_sendRawTransaction', @@ -64,11 +63,7 @@ export function createPPOMMiddleware< networkController: NetworkController, appStateController: AppStateController, accountsController: AccountsController, - updateSecurityAlertResponse: ( - method: string, - signatureAlertId: string, - securityAlertResponse: SecurityAlertResponse, - ) => void, + updateSecurityAlertResponse: UpdateSecurityAlertResponse, ) { return async ( req: PPOMMiddlewareRequest, @@ -88,9 +83,7 @@ export function createPPOMMiddleware< if ( !securityAlertsEnabled || - !CONFIRMATION_METHODS.includes(req.method) || - // Do not move this call above this check because it will result in unnecessary calls - !(await isChainSupported(chainId)) + !CONFIRMATION_METHODS.includes(req.method) ) { return; } @@ -122,27 +115,22 @@ export function createPPOMMiddleware< request: req, securityAlertId, chainId, - }).then((securityAlertResponse) => { - updateSecurityAlertResponse( - req.method, - securityAlertId, - securityAlertResponse, - ); + updateSecurityAlertResponse, }), ); - const loadingSecurityAlertResponse: SecurityAlertResponse = { - ...LOADING_SECURITY_ALERT_RESPONSE, + const securityAlertResponseCheckingChain: SecurityAlertResponse = { + ...SECURITY_ALERT_RESPONSE_CHECKING_CHAIN, securityAlertId, }; if (SIGNING_METHODS.includes(req.method)) { appStateController.addSignatureSecurityAlertResponse( - loadingSecurityAlertResponse, + securityAlertResponseCheckingChain, ); } - req.securityAlertResponse = loadingSecurityAlertResponse; + req.securityAlertResponse = securityAlertResponseCheckingChain; } catch (error) { req.securityAlertResponse = handlePPOMError( error, diff --git a/app/scripts/lib/ppom/ppom-util.test.ts b/app/scripts/lib/ppom/ppom-util.test.ts index ea62c3b88533..8acb6dd9788c 100644 --- a/app/scripts/lib/ppom/ppom-util.test.ts +++ b/app/scripts/lib/ppom/ppom-util.test.ts @@ -10,9 +10,12 @@ import { SignatureController, SignatureRequest, } from '@metamask/signature-controller'; +import { Hex } from '@metamask/utils'; import { BlockaidReason, BlockaidResultType, + LOADING_SECURITY_ALERT_RESPONSE, + SECURITY_ALERT_RESPONSE_CHAIN_NOT_SUPPORTED, SecurityAlertSource, } from '../../../../shared/constants/security-provider'; import { AppStateController } from '../../controllers/app-state-controller'; @@ -32,7 +35,7 @@ jest.mock('@metamask/transaction-controller', () => ({ const SECURITY_ALERT_ID_MOCK = '1234-5678'; const TRANSACTION_ID_MOCK = '123'; -const CHAIN_ID_MOCK = '0x1'; +const CHAIN_ID_MOCK = '0x1' as Hex; const REQUEST_MOCK = { method: 'eth_signTypedData_v4', @@ -45,6 +48,7 @@ const SECURITY_ALERT_RESPONSE_MOCK: SecurityAlertResponse = { result_type: 'success', reason: 'success', source: SecurityAlertSource.Local, + securityAlertId: SECURITY_ALERT_ID_MOCK, }; const TRANSACTION_PARAMS_MOCK_1: TransactionParams = { @@ -110,6 +114,15 @@ describe('PPOM Utils', () => { ); let isSecurityAlertsEnabledMock: jest.SpyInstance; + const updateSecurityAlertResponseMock = jest.fn(); + + const validateRequestWithPPOMOptionsBase = { + request: REQUEST_MOCK, + securityAlertId: SECURITY_ALERT_ID_MOCK, + chainId: CHAIN_ID_MOCK, + updateSecurityAlertResponse: updateSecurityAlertResponseMock, + }; + beforeEach(() => { jest.resetAllMocks(); jest.spyOn(console, 'error').mockImplementation(() => undefined); @@ -119,7 +132,7 @@ describe('PPOM Utils', () => { }); describe('validateRequestWithPPOM', () => { - it('returns response from validation with PPOM instance via controller', async () => { + it('updates response from validation with PPOM instance via controller', async () => { const ppom = createPPOMMock(); const ppomController = createPPOMControllerMock(); @@ -129,23 +142,39 @@ describe('PPOM Utils', () => { (callback) => callback(ppom as any) as any, ); - const response = await validateRequestWithPPOM({ + await validateRequestWithPPOM({ + ...validateRequestWithPPOMOptionsBase, ppomController, - request: REQUEST_MOCK, - securityAlertId: SECURITY_ALERT_ID_MOCK, - chainId: CHAIN_ID_MOCK, - }); - - expect(response).toStrictEqual({ - ...SECURITY_ALERT_RESPONSE_MOCK, - securityAlertId: SECURITY_ALERT_ID_MOCK, }); + expect(updateSecurityAlertResponseMock).toHaveBeenCalledWith( + REQUEST_MOCK.method, + SECURITY_ALERT_ID_MOCK, + { + ...SECURITY_ALERT_RESPONSE_MOCK, + securityAlertId: SECURITY_ALERT_ID_MOCK, + }, + ); expect(ppom.validateJsonRpc).toHaveBeenCalledTimes(1); expect(ppom.validateJsonRpc).toHaveBeenCalledWith(REQUEST_MOCK); }); - it('returns error response if validation with PPOM instance throws', async () => { + it('updates securityAlertResponse with loading state', async () => { + const ppomController = createPPOMControllerMock(); + + await validateRequestWithPPOM({ + ...validateRequestWithPPOMOptionsBase, + ppomController, + }); + + expect(updateSecurityAlertResponseMock).toHaveBeenCalledWith( + REQUEST_MOCK.method, + SECURITY_ALERT_ID_MOCK, + LOADING_SECURITY_ALERT_RESPONSE, + ); + }); + + it('updates error response if validation with PPOM instance throws', async () => { const ppom = createPPOMMock(); const ppomController = createPPOMControllerMock(); @@ -157,37 +186,41 @@ describe('PPOM Utils', () => { callback(ppom as any) as any, ); - const response = await validateRequestWithPPOM({ + await validateRequestWithPPOM({ + ...validateRequestWithPPOMOptionsBase, ppomController, - request: REQUEST_MOCK, - securityAlertId: SECURITY_ALERT_ID_MOCK, - chainId: CHAIN_ID_MOCK, }); - expect(response).toStrictEqual({ - result_type: BlockaidResultType.Errored, - reason: BlockaidReason.errored, - description: 'Test Error: Test error message', - }); + expect(updateSecurityAlertResponseMock).toHaveBeenCalledWith( + validateRequestWithPPOMOptionsBase.request.method, + SECURITY_ALERT_ID_MOCK, + { + result_type: BlockaidResultType.Errored, + reason: BlockaidReason.errored, + description: 'Test Error: Test error message', + }, + ); }); - it('returns error response if controller throws', async () => { + it('updates error response if controller throws', async () => { const ppomController = createPPOMControllerMock(); ppomController.usePPOM.mockRejectedValue(createErrorMock()); - const response = await validateRequestWithPPOM({ + await validateRequestWithPPOM({ + ...validateRequestWithPPOMOptionsBase, ppomController, - request: REQUEST_MOCK, - securityAlertId: SECURITY_ALERT_ID_MOCK, - chainId: CHAIN_ID_MOCK, }); - expect(response).toStrictEqual({ - result_type: BlockaidResultType.Errored, - reason: BlockaidReason.errored, - description: 'Test Error: Test error message', - }); + expect(updateSecurityAlertResponseMock).toHaveBeenCalledWith( + validateRequestWithPPOMOptionsBase.request.method, + SECURITY_ALERT_ID_MOCK, + { + result_type: BlockaidResultType.Errored, + reason: BlockaidReason.errored, + description: 'Test Error: Test error message', + }, + ); }); it('normalizes request if method is eth_sendTransaction', async () => { @@ -209,10 +242,9 @@ describe('PPOM Utils', () => { }; await validateRequestWithPPOM({ + ...validateRequestWithPPOMOptionsBase, ppomController, request, - securityAlertId: SECURITY_ALERT_ID_MOCK, - chainId: CHAIN_ID_MOCK, }); expect(ppom.validateJsonRpc).toHaveBeenCalledTimes(1); @@ -226,6 +258,23 @@ describe('PPOM Utils', () => { TRANSACTION_PARAMS_MOCK_1, ); }); + + it('updates response indicating chain is not supported', async () => { + const ppomController = {} as PPOMController; + const CHAIN_ID_UNSUPPORTED_MOCK = '0x2'; + + await validateRequestWithPPOM({ + ...validateRequestWithPPOMOptionsBase, + ppomController, + chainId: CHAIN_ID_UNSUPPORTED_MOCK, + }); + + expect(updateSecurityAlertResponseMock).toHaveBeenCalledWith( + validateRequestWithPPOMOptionsBase.request.method, + SECURITY_ALERT_ID_MOCK, + SECURITY_ALERT_RESPONSE_CHAIN_NOT_SUPPORTED, + ); + }); }); describe('generateSecurityAlertId', () => { @@ -318,10 +367,9 @@ describe('PPOM Utils', () => { const ppomController = createPPOMControllerMock(); await validateRequestWithPPOM({ + ...validateRequestWithPPOMOptionsBase, ppomController, request, - securityAlertId: SECURITY_ALERT_ID_MOCK, - chainId: CHAIN_ID_MOCK, }); expect(ppomController.usePPOM).not.toHaveBeenCalled(); @@ -345,10 +393,9 @@ describe('PPOM Utils', () => { .mockRejectedValue(new Error('Test Error')); await validateRequestWithPPOM({ + ...validateRequestWithPPOMOptionsBase, ppomController, request, - securityAlertId: SECURITY_ALERT_ID_MOCK, - chainId: CHAIN_ID_MOCK, }); expect(ppomController.usePPOM).toHaveBeenCalledTimes(1); diff --git a/app/scripts/lib/ppom/ppom-util.ts b/app/scripts/lib/ppom/ppom-util.ts index e8c54ee5eb53..b0bf6b03b4d9 100644 --- a/app/scripts/lib/ppom/ppom-util.ts +++ b/app/scripts/lib/ppom/ppom-util.ts @@ -11,12 +11,14 @@ import { SignatureController } from '@metamask/signature-controller'; import { BlockaidReason, BlockaidResultType, + LOADING_SECURITY_ALERT_RESPONSE, + SECURITY_ALERT_RESPONSE_CHAIN_NOT_SUPPORTED, SECURITY_PROVIDER_SUPPORTED_CHAIN_IDS_FALLBACK_LIST, SecurityAlertSource, } from '../../../../shared/constants/security-provider'; import { SIGNING_METHODS } from '../../../../shared/constants/transaction'; import { AppStateController } from '../../controllers/app-state-controller'; -import { SecurityAlertResponse } from './types'; +import { SecurityAlertResponse, UpdateSecurityAlertResponse } from './types'; import { getSecurityAlertsAPISupportedChainIds, isSecurityAlertsAPIEnabled, @@ -37,18 +39,36 @@ type PPOMRequest = Omit & { method: typeof METHOD_SEND_TRANSACTION; params: [TransactionParams]; }; + export async function validateRequestWithPPOM({ ppomController, request, securityAlertId, chainId, + updateSecurityAlertResponse: updateSecurityResponse, }: { ppomController: PPOMController; request: JsonRpcRequest; securityAlertId: string; chainId: Hex; -}): Promise { + updateSecurityAlertResponse: UpdateSecurityAlertResponse; +}) { try { + if (!(await isChainSupported(chainId))) { + await updateSecurityResponse( + request.method, + securityAlertId, + SECURITY_ALERT_RESPONSE_CHAIN_NOT_SUPPORTED, + ); + return; + } + + await updateSecurityResponse( + request.method, + securityAlertId, + LOADING_SECURITY_ALERT_RESPONSE, + ); + const normalizedRequest = normalizePPOMRequest(request); const ppomResponse = isSecurityAlertsAPIEnabled() @@ -58,13 +78,13 @@ export async function validateRequestWithPPOM({ normalizedRequest, chainId, ); - - return { - ...ppomResponse, - securityAlertId, - }; + await updateSecurityResponse(request.method, securityAlertId, ppomResponse); } catch (error: unknown) { - return handlePPOMError(error, 'Error validating JSON RPC using PPOM: '); + await updateSecurityResponse( + request.method, + securityAlertId, + handlePPOMError(error, 'Error validating JSON RPC using PPOM: '), + ); } } @@ -97,12 +117,15 @@ export async function updateSecurityAlertResponse({ ); if (isSignatureRequest) { - appStateController.addSignatureSecurityAlertResponse(securityAlertResponse); + appStateController.addSignatureSecurityAlertResponse({ + ...securityAlertResponse, + securityAlertId, + }); } else { - transactionController.updateSecurityAlertResponse( - confirmation.id, - securityAlertResponse, - ); + transactionController.updateSecurityAlertResponse(confirmation.id, { + ...securityAlertResponse, + securityAlertId, + } as SecurityAlertResponse); } } diff --git a/app/scripts/lib/ppom/types.ts b/app/scripts/lib/ppom/types.ts index 6188d644aa12..57dd4a9e533d 100644 --- a/app/scripts/lib/ppom/types.ts +++ b/app/scripts/lib/ppom/types.ts @@ -10,3 +10,9 @@ export type SecurityAlertResponse = { securityAlertId?: string; source?: SecurityAlertSource; }; + +export type UpdateSecurityAlertResponse = ( + method: string, + securityAlertId: string, + securityAlertResponse: SecurityAlertResponse, +) => Promise; diff --git a/app/scripts/lib/transaction/util.test.ts b/app/scripts/lib/transaction/util.test.ts index 16077e0f08ed..fbbee025381b 100644 --- a/app/scripts/lib/transaction/util.test.ts +++ b/app/scripts/lib/transaction/util.test.ts @@ -16,7 +16,6 @@ import { BlockaidReason, BlockaidResultType, } from '../../../../shared/constants/security-provider'; -import { SecurityAlertResponse } from '../ppom/types'; import { flushPromises } from '../../../../test/lib/timer-helpers'; import { createMockInternalAccount } from '../../../../test/jest/mocks'; import { @@ -79,11 +78,6 @@ const TRANSACTION_REQUEST_MOCK: AddTransactionRequest = { internalAccounts: [], } as unknown as AddTransactionRequest; -const SECURITY_ALERT_RESPONSE_MOCK: SecurityAlertResponse = { - result_type: BlockaidResultType.Malicious, - reason: BlockaidReason.maliciousDomain, -}; - function createTransactionControllerMock() { return { addTransaction: jest.fn(), @@ -406,10 +400,6 @@ describe('Transaction Utils', () => { describe('validates using security provider', () => { it('adds loading response to request options', async () => { - validateRequestWithPPOMMock.mockResolvedValue( - SECURITY_ALERT_RESPONSE_MOCK, - ); - await addTransaction({ ...request, securityAlertsEnabled: true, @@ -425,36 +415,13 @@ describe('Transaction Utils', () => { ).toHaveBeenCalledWith(TRANSACTION_PARAMS_MOCK, { ...TRANSACTION_OPTIONS_MOCK, securityAlertResponse: { - reason: BlockaidReason.inProgress, + reason: BlockaidReason.checkingChain, result_type: BlockaidResultType.Loading, securityAlertId: SECURITY_ALERT_ID_MOCK, }, }); }); - it('updates response after validation', async () => { - validateRequestWithPPOMMock.mockResolvedValue( - SECURITY_ALERT_RESPONSE_MOCK, - ); - - await addTransaction({ - ...request, - securityAlertsEnabled: true, - chainId: '0x1', - }); - - await flushPromises(); - - expect(request.updateSecurityAlertResponse).toHaveBeenCalledTimes(1); - expect(request.updateSecurityAlertResponse).toHaveBeenCalledWith( - 'eth_sendTransaction', - SECURITY_ALERT_ID_MOCK, - SECURITY_ALERT_RESPONSE_MOCK, - ); - - expect(validateRequestWithPPOMMock).toHaveBeenCalledTimes(1); - }); - it('unless blockaid is disabled', async () => { await addTransaction({ ...request, @@ -505,29 +472,6 @@ describe('Transaction Utils', () => { expect(validateRequestWithPPOMMock).toHaveBeenCalledTimes(0); }); - it('unless chain is not supported', async () => { - isChainSupportedMock.mockResolvedValue(false); - - await addTransaction({ - ...request, - securityAlertsEnabled: true, - chainId: '0xF', - }); - - expect( - request.transactionController.addTransaction, - ).toHaveBeenCalledTimes(1); - - expect( - request.transactionController.addTransaction, - ).toHaveBeenCalledWith( - TRANSACTION_PARAMS_MOCK, - TRANSACTION_OPTIONS_MOCK, - ); - - expect(validateRequestWithPPOMMock).toHaveBeenCalledTimes(0); - }); - it('unless transaction type is swap', async () => { const swapRequest = { ...request }; swapRequest.transactionOptions.type = TransactionType.swap; diff --git a/app/scripts/lib/transaction/util.ts b/app/scripts/lib/transaction/util.ts index 8b71e33119f8..0bbf93afd8a8 100644 --- a/app/scripts/lib/transaction/util.ts +++ b/app/scripts/lib/transaction/util.ts @@ -16,12 +16,14 @@ import { PPOMController } from '@metamask/ppom-validator'; import { generateSecurityAlertId, handlePPOMError, - isChainSupported, validateRequestWithPPOM, } from '../ppom/ppom-util'; -import { SecurityAlertResponse } from '../ppom/types'; import { - LOADING_SECURITY_ALERT_RESPONSE, + SecurityAlertResponse, + UpdateSecurityAlertResponse, +} from '../ppom/types'; +import { + SECURITY_ALERT_RESPONSE_CHECKING_CHAIN, SECURITY_PROVIDER_EXCLUDED_TRANSACTION_TYPES, } from '../../../../shared/constants/security-provider'; import { endTrace, TraceName } from '../../../../shared/lib/trace'; @@ -38,11 +40,7 @@ type BaseAddTransactionRequest = { selectedAccount: InternalAccount; transactionParams: TransactionParams; transactionController: TransactionController; - updateSecurityAlertResponse: ( - method: string, - securityAlertId: string, - securityAlertResponse: SecurityAlertResponse, - ) => void; + updateSecurityAlertResponse: UpdateSecurityAlertResponse; userOperationController: UserOperationController; internalAccounts: InternalAccount[]; }; @@ -239,18 +237,12 @@ async function validateSecurity(request: AddTransactionRequest) { const { type } = transactionOptions; - const isCurrentChainSupported = await isChainSupported(chainId); - const typeIsExcludedFromPPOM = SECURITY_PROVIDER_EXCLUDED_TRANSACTION_TYPES.includes( type as TransactionType, ); - if ( - !securityAlertsEnabled || - !isCurrentChainSupported || - typeIsExcludedFromPPOM - ) { + if (!securityAlertsEnabled || typeIsExcludedFromPPOM) { return; } @@ -290,21 +282,16 @@ async function validateSecurity(request: AddTransactionRequest) { request: ppomRequest, securityAlertId, chainId, - }).then((securityAlertResponse) => { - updateSecurityAlertResponse( - ppomRequest.method, - securityAlertId, - securityAlertResponse, - ); + updateSecurityAlertResponse, }); - const loadingSecurityAlertResponse: SecurityAlertResponse = { - ...LOADING_SECURITY_ALERT_RESPONSE, + const securityAlertResponseCheckingChain: SecurityAlertResponse = { + ...SECURITY_ALERT_RESPONSE_CHECKING_CHAIN, securityAlertId, }; request.transactionOptions.securityAlertResponse = - loadingSecurityAlertResponse; + securityAlertResponseCheckingChain; } catch (error) { handlePPOMError(error, 'Error validating JSON RPC using PPOM: '); } diff --git a/shared/constants/security-provider.ts b/shared/constants/security-provider.ts index 082f68aa88de..c7c4a8df3b2c 100644 --- a/shared/constants/security-provider.ts +++ b/shared/constants/security-provider.ts @@ -32,7 +32,7 @@ export enum BlockaidReason { approvalFarming = 'approval_farming', /** Malicious signature on Blur order */ blurFarming = 'blur_farming', - /** A known malicous site invoked that transaction */ + /** A known malicious site invoked that transaction */ maliciousDomain = 'malicious_domain', /** Malicious signature on a Permit order */ permitFarming = 'permit_farming', @@ -57,6 +57,8 @@ export enum BlockaidReason { errored = 'Error', notApplicable = 'NotApplicable', inProgress = 'validation_in_progress', + checkingChain = 'CheckingChain', + chainNotSupported = 'ChainNotSupported', } export enum BlockaidResultType { @@ -117,6 +119,17 @@ export const LOADING_SECURITY_ALERT_RESPONSE: SecurityAlertResponse = { reason: BlockaidReason.inProgress, }; +export const SECURITY_ALERT_RESPONSE_CHECKING_CHAIN: SecurityAlertResponse = { + result_type: BlockaidResultType.Loading, + reason: BlockaidReason.checkingChain, +}; + +export const SECURITY_ALERT_RESPONSE_CHAIN_NOT_SUPPORTED: SecurityAlertResponse = + { + result_type: BlockaidResultType.Benign, + reason: BlockaidReason.chainNotSupported, + }; + export enum SecurityAlertSource { /** Validation performed remotely using the Security Alerts API. */ API = 'api', diff --git a/test/e2e/tests/confirmations/signatures/malicious-signatures.spec.ts b/test/e2e/tests/confirmations/signatures/malicious-signatures.spec.ts index fc8a6d0ab240..328ad7811e1b 100644 --- a/test/e2e/tests/confirmations/signatures/malicious-signatures.spec.ts +++ b/test/e2e/tests/confirmations/signatures/malicious-signatures.spec.ts @@ -10,6 +10,10 @@ import { withRedesignConfirmationFixtures, } from '../helpers'; import { TestSuiteArguments } from '../transactions/shared'; +import { + BlockaidReason, + BlockaidResultType, +} from '../../../../../shared/constants/security-provider'; import { assertSignatureRejectedMetrics, openDappAndTriggerSignature, @@ -80,6 +84,8 @@ describe('Malicious Confirmation Signature - Bad Domain @no-mmi', function (this alert_visualized: [], alert_visualized_count: 0, }, + securityAlertReason: BlockaidReason.notApplicable, + securityAlertResponse: BlockaidResultType.NotApplicable, }); }, mockSignatureRejected, @@ -130,6 +136,8 @@ describe('Malicious Confirmation Signature - Bad Domain @no-mmi', function (this alert_visualized: ['requestFrom'], alert_visualized_count: 1, }, + securityAlertReason: BlockaidReason.notApplicable, + securityAlertResponse: BlockaidResultType.NotApplicable, }); }, mockSignatureRejected, diff --git a/test/e2e/tests/confirmations/signatures/signature-helpers.ts b/test/e2e/tests/confirmations/signatures/signature-helpers.ts index 5242be3f3c20..a4c5e9309249 100644 --- a/test/e2e/tests/confirmations/signatures/signature-helpers.ts +++ b/test/e2e/tests/confirmations/signatures/signature-helpers.ts @@ -8,6 +8,10 @@ import { unlockWallet, } from '../../../helpers'; import { Driver } from '../../../webdriver/driver'; +import { + BlockaidReason, + BlockaidResultType, +} from '../../../../../shared/constants/security-provider'; export const WALLET_ADDRESS = '0x5CfE73b6021E818B776b421B1c4Db2474086a7e1'; export const WALLET_ETH_BALANCE = '25'; @@ -31,6 +35,8 @@ type AssertSignatureMetricsOptions = { location?: string; expectedProps?: Record; withAnonEvents?: boolean; + securityAlertReason?: string; + securityAlertResponse?: string; }; type SignatureEventProperty = { @@ -40,7 +46,7 @@ type SignatureEventProperty = { environment_type: 'background'; locale: 'en'; security_alert_reason: string; - security_alert_response: 'NotApplicable'; + security_alert_response: string; signature_type: string; eip712_primary_type?: string; ui_customizations?: string[]; @@ -59,11 +65,15 @@ const signatureAnonProperties = { * @param signatureType * @param primaryType * @param uiCustomizations + * @param securityAlertReason + * @param securityAlertResponse */ function getSignatureEventProperty( signatureType: string, primaryType: string, uiCustomizations: string[], + securityAlertReason: string = BlockaidReason.checkingChain, + securityAlertResponse: string = BlockaidResultType.Loading, ): SignatureEventProperty { const signatureEventProperty: SignatureEventProperty = { account_type: 'MetaMask', @@ -72,8 +82,8 @@ function getSignatureEventProperty( chain_id: '0x539', environment_type: 'background', locale: 'en', - security_alert_reason: 'NotApplicable', - security_alert_response: 'NotApplicable', + security_alert_reason: securityAlertReason, + security_alert_response: securityAlertResponse, ui_customizations: uiCustomizations, }; @@ -90,15 +100,15 @@ function assertSignatureRequestedMetrics( signatureEventProperty: SignatureEventProperty, withAnonEvents = false, ) { - assertEventPropertiesMatch(events, 'Signature Requested', { - ...signatureEventProperty, - security_alert_reason: 'NotApplicable', - }); + assertEventPropertiesMatch( + events, + 'Signature Requested', + signatureEventProperty, + ); if (withAnonEvents) { assertEventPropertiesMatch(events, 'Signature Requested Anon', { ...signatureEventProperty, - security_alert_reason: 'NotApplicable', ...signatureAnonProperties, }); } @@ -111,12 +121,16 @@ export async function assertSignatureConfirmedMetrics({ primaryType = '', uiCustomizations = ['redesigned_confirmation'], withAnonEvents = false, + securityAlertReason, + securityAlertResponse, }: AssertSignatureMetricsOptions) { const events = await getEventPayloads(driver, mockedEndpoints); const signatureEventProperty = getSignatureEventProperty( signatureType, primaryType, uiCustomizations, + securityAlertReason, + securityAlertResponse, ); assertSignatureRequestedMetrics( @@ -148,12 +162,16 @@ export async function assertSignatureRejectedMetrics({ location, expectedProps = {}, withAnonEvents = false, + securityAlertReason, + securityAlertResponse, }: AssertSignatureMetricsOptions) { const events = await getEventPayloads(driver, mockedEndpoints); const signatureEventProperty = getSignatureEventProperty( signatureType, primaryType, uiCustomizations, + securityAlertReason, + securityAlertResponse, ); assertSignatureRequestedMetrics( @@ -201,14 +219,44 @@ function assertEventPropertiesMatch( expectedProperties: object, ) { const event = events.find((e) => e.event === eventName); + + const actualProperties = { ...event.properties }; + const expectedProps = { ...expectedProperties }; + + compareSecurityAlertResponse(actualProperties, expectedProps, eventName); + assert(event, `${eventName} event not found`); assert.deepStrictEqual( - event.properties, - expectedProperties, + actualProperties, + expectedProps, `${eventName} event properties do not match`, ); } +function compareSecurityAlertResponse( + actualProperties: Record, + expectedProperties: Record, + eventName: string, +) { + if ( + expectedProperties.security_alert_response && + (expectedProperties.security_alert_response === 'loading' || + expectedProperties.security_alert_response === 'Benign') + ) { + if ( + actualProperties.security_alert_response !== 'loading' && + actualProperties.security_alert_response !== 'Benign' + ) { + assert.fail( + `${eventName} event properties do not match: security_alert_response is ${actualProperties.security_alert_response}`, + ); + } + // Remove the property from both objects to avoid comparison + delete actualProperties.security_alert_response; + delete expectedProperties.security_alert_response; + } +} + export async function clickHeaderInfoBtn(driver: Driver) { await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); diff --git a/test/e2e/tests/confirmations/signatures/siwe.spec.ts b/test/e2e/tests/confirmations/signatures/siwe.spec.ts index 2a7b43ba1099..889122eb22b8 100644 --- a/test/e2e/tests/confirmations/signatures/siwe.spec.ts +++ b/test/e2e/tests/confirmations/signatures/siwe.spec.ts @@ -11,6 +11,10 @@ import { withRedesignConfirmationFixtures, } from '../helpers'; import { TestSuiteArguments } from '../transactions/shared'; +import { + BlockaidReason, + BlockaidResultType, +} from '../../../../../shared/constants/security-provider'; import { assertAccountDetailsMetrics, assertHeaderInfoBalance, @@ -60,6 +64,8 @@ describe('Confirmation Signature - SIWE @no-mmi', function (this: Suite) { 'redesigned_confirmation', 'sign_in_with_ethereum', ], + securityAlertReason: BlockaidReason.notApplicable, + securityAlertResponse: BlockaidResultType.NotApplicable, }); }, mockSignatureApproved, @@ -95,6 +101,8 @@ describe('Confirmation Signature - SIWE @no-mmi', function (this: Suite) { 'sign_in_with_ethereum', ], location: 'confirmation', + securityAlertReason: BlockaidReason.notApplicable, + securityAlertResponse: BlockaidResultType.NotApplicable, }); }, mockSignatureRejected, diff --git a/test/e2e/tests/metrics/signature-approved.spec.js b/test/e2e/tests/metrics/signature-approved.spec.js index c6990820af8b..2ea84d281b50 100644 --- a/test/e2e/tests/metrics/signature-approved.spec.js +++ b/test/e2e/tests/metrics/signature-approved.spec.js @@ -1,4 +1,5 @@ const { strict: assert } = require('assert'); + const { defaultGanacheOptions, switchToNotificationWindow, @@ -47,6 +48,16 @@ async function mockSegment(mockServer) { ]; } +const expectedEventPropertiesBase = { + account_type: 'MetaMask', + category: 'inpage_provider', + locale: 'en', + chain_id: '0x539', + environment_type: 'background', + security_alert_reason: 'CheckingChain', + security_alert_response: 'loading', +}; + describe('Signature Approved Event @no-mmi', function () { it('Successfully tracked for signTypedData_v4', async function () { await withFixtures( @@ -76,31 +87,21 @@ describe('Signature Approved Event @no-mmi', function () { const events = await getEventPayloads(driver, mockedEndpoints); assert.deepStrictEqual(events[0].properties, { - account_type: 'MetaMask', + ...expectedEventPropertiesBase, signature_type: 'eth_signTypedData_v4', - category: 'inpage_provider', - locale: 'en', - chain_id: '0x539', eip712_primary_type: 'Mail', - environment_type: 'background', - security_alert_reason: 'NotApplicable', - security_alert_response: 'NotApplicable', }); assert.deepStrictEqual(events[1].properties, { - account_type: 'MetaMask', + ...expectedEventPropertiesBase, signature_type: 'eth_signTypedData_v4', - category: 'inpage_provider', - locale: 'en', - chain_id: '0x539', eip712_primary_type: 'Mail', - environment_type: 'background', - security_alert_reason: 'NotApplicable', - security_alert_response: 'NotApplicable', + security_alert_response: 'Benign', }); }, ); }); + it('Successfully tracked for signTypedData_v3', async function () { await withFixtures( { @@ -127,29 +128,21 @@ describe('Signature Approved Event @no-mmi', function () { await validateContractDetails(driver); await clickSignOnSignatureConfirmation({ driver }); const events = await getEventPayloads(driver, mockedEndpoints); + assert.deepStrictEqual(events[0].properties, { - account_type: 'MetaMask', + ...expectedEventPropertiesBase, signature_type: 'eth_signTypedData_v3', - category: 'inpage_provider', - locale: 'en', - chain_id: '0x539', - environment_type: 'background', - security_alert_reason: 'NotApplicable', - security_alert_response: 'NotApplicable', }); + assert.deepStrictEqual(events[1].properties, { - account_type: 'MetaMask', + ...expectedEventPropertiesBase, signature_type: 'eth_signTypedData_v3', - category: 'inpage_provider', - locale: 'en', - chain_id: '0x539', - environment_type: 'background', - security_alert_reason: 'NotApplicable', - security_alert_response: 'NotApplicable', + security_alert_response: 'Benign', }); }, ); }); + it('Successfully tracked for signTypedData', async function () { await withFixtures( { @@ -175,29 +168,21 @@ describe('Signature Approved Event @no-mmi', function () { await switchToNotificationWindow(driver); await clickSignOnSignatureConfirmation({ driver }); const events = await getEventPayloads(driver, mockedEndpoints); + assert.deepStrictEqual(events[0].properties, { - account_type: 'MetaMask', + ...expectedEventPropertiesBase, signature_type: 'eth_signTypedData', - category: 'inpage_provider', - locale: 'en', - chain_id: '0x539', - environment_type: 'background', - security_alert_reason: 'NotApplicable', - security_alert_response: 'NotApplicable', }); + assert.deepStrictEqual(events[1].properties, { - account_type: 'MetaMask', + ...expectedEventPropertiesBase, signature_type: 'eth_signTypedData', - category: 'inpage_provider', - locale: 'en', - chain_id: '0x539', - environment_type: 'background', - security_alert_reason: 'NotApplicable', - security_alert_response: 'NotApplicable', + security_alert_response: 'Benign', }); }, ); }); + it('Successfully tracked for personalSign', async function () { await withFixtures( { @@ -223,25 +208,16 @@ describe('Signature Approved Event @no-mmi', function () { await switchToNotificationWindow(driver); await clickSignOnSignatureConfirmation({ driver }); const events = await getEventPayloads(driver, mockedEndpoints); + assert.deepStrictEqual(events[0].properties, { - account_type: 'MetaMask', + ...expectedEventPropertiesBase, signature_type: 'personal_sign', - category: 'inpage_provider', - locale: 'en', - chain_id: '0x539', - environment_type: 'background', - security_alert_reason: 'NotApplicable', - security_alert_response: 'NotApplicable', }); + assert.deepStrictEqual(events[1].properties, { - account_type: 'MetaMask', + ...expectedEventPropertiesBase, signature_type: 'personal_sign', - category: 'inpage_provider', - locale: 'en', - chain_id: '0x539', - environment_type: 'background', - security_alert_reason: 'NotApplicable', - security_alert_response: 'NotApplicable', + security_alert_response: 'Benign', }); }, );