Skip to content
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

fix: Clicking on report link does not add external_link_clicked to si… #29969

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
71 changes: 70 additions & 1 deletion test/integration/confirmations/signatures/permit.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import * as backgroundConnection from '../../../../ui/store/background-connectio
import { integrationTestRender } from '../../../lib/render-helpers';
import mockMetaMaskState from '../../data/integration-init-state.json';
import { createMockImplementation } from '../../helpers';
import { getMetaMaskStateWithUnapprovedPermitSign } from './signature-helpers';
import { tEn } from '../../../lib/i18n-helpers';
import {
getMetamaskStateWithMaliciousPermit,
getMetaMaskStateWithUnapprovedPermitSign,
} from './signature-helpers';

jest.mock('../../../../ui/store/background-connection', () => ({
...jest.requireActual('../../../../ui/store/background-connection'),
Expand Down Expand Up @@ -250,4 +254,69 @@ describe('Permit Confirmation', () => {
scope.done();
expect(scope.isDone()).toBe(true);
});

it('displays the malicious banner', async () => {
const account =
mockMetaMaskState.internalAccounts.accounts[
mockMetaMaskState.internalAccounts
.selectedAccount as keyof typeof mockMetaMaskState.internalAccounts.accounts
];

const mockedMetaMaskState = getMetamaskStateWithMaliciousPermit(
account.address,
);

await act(async () => {
await integrationTestRender({
preloadedState: mockedMetaMaskState,
backgroundConnection: backgroundConnectionMocked,
});
});

const headingText = tEn('blockaidTitleDeceptive') as string;
const bodyText = tEn('blockaidDescriptionApproveFarming') as string;
expect(await screen.findByText(headingText)).toBeInTheDocument();
expect(await screen.findByText(bodyText)).toBeInTheDocument();
});

it('tracks external link clicked property in signature rejected event', async () => {
const account =
mockMetaMaskState.internalAccounts.accounts[
mockMetaMaskState.internalAccounts
.selectedAccount as keyof typeof mockMetaMaskState.internalAccounts.accounts
];

const mockedMetaMaskState = getMetamaskStateWithMaliciousPermit(
account.address,
);

await act(async () => {
await integrationTestRender({
preloadedState: mockedMetaMaskState,
backgroundConnection: backgroundConnectionMocked,
});
});

fireEvent.click(await screen.findByTestId('disclosure'));
expect(
await screen.findByTestId('alert-provider-report-link'),
).toBeInTheDocument();

fireEvent.click(await screen.findByTestId('alert-provider-report-link'));

fireEvent.click(await screen.findByTestId('confirm-footer-cancel-button'));

expect(
mockedBackgroundConnection.submitRequestToBackground,
).toHaveBeenCalledWith(
'updateEventFragment',
expect.arrayContaining([
expect.objectContaining({
properties: expect.objectContaining({
external_link_clicked: 'security_alert_support_link',
}),
}),
]),
);
});
});
45 changes: 44 additions & 1 deletion test/integration/confirmations/signatures/signature-helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const SEAPORT_DATA = `{"types":{"OrderComponents":[{"name":"offerer","type":"add

const TRADE_ORDER_DATA = `{"types":{"ERC721Order":[{"type":"uint8","name":"direction"},{"type":"address","name":"maker"},{"type":"address","name":"taker"},{"type":"uint256","name":"expiry"},{"type":"uint256","name":"nonce"},{"type":"address","name":"erc20Token"},{"type":"uint256","name":"erc20TokenAmount"},{"type":"Fee[]","name":"fees"},{"type":"address","name":"erc721Token"},{"type":"uint256","name":"erc721TokenId"},{"type":"Property[]","name":"erc721TokenProperties"}],"Fee":[{"type":"address","name":"recipient"},{"type":"uint256","name":"amount"},{"type":"bytes","name":"feeData"}],"Property":[{"type":"address","name":"propertyValidator"},{"type":"bytes","name":"propertyData"}],"EIP712Domain":[{"name":"name","type":"string"},{"name":"version","type":"string"},{"name":"chainId","type":"uint256"},{"name":"verifyingContract","type":"address"}]},"domain":{"name":"ZeroEx","version":"1.0.0","chainId":"0x1","verifyingContract":"0xdef1c0ded9bec7f1a1670819833240f027b25eff"},"primaryType":"ERC721Order","message":{"direction":"0","maker":"0x8eeee1781fd885ff5ddef7789486676961873d12","taker":"0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826","expiry":"2524604400","nonce":"100131415900000000000000000000000000000083840314483690155566137712510085002484","erc20Token":"0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2","erc20TokenAmount":"42000000000000","fees":[],"erc721Token":"0x8a90CAb2b38dba80c64b7734e58Ee1dB38B8992e","erc721TokenId":"2516","erc721TokenProperties":[]}}`;

const pendingPermitId = '48a75190-45ca-11ef-9001-f3886ec2397c';

const getPermitData = (permitType: string, accountAddress: string) => {
switch (permitType) {
case 'Permit':
Expand Down Expand Up @@ -48,7 +50,6 @@ export const getMetaMaskStateWithUnapprovedPermitSign = (
| 'TradeOrder',
) => {
const data = getPermitData(permitType, accountAddress);
const pendingPermitId = '48a75190-45ca-11ef-9001-f3886ec2397c';
const pendingPermitTime = new Date().getTime();
const messageParams = getMessageParams(accountAddress, data);

Expand Down Expand Up @@ -91,6 +92,48 @@ export const getMetaMaskStateWithUnapprovedPermitSign = (
};
};

export const getMetamaskStateWithMaliciousPermit = (accountAddress: string) => {
const state = getMetaMaskStateWithUnapprovedPermitSign(
accountAddress,
'Permit',
);
const unapprovedTypedMessage = {
[pendingPermitId]: {
...state.unapprovedTypedMessages[pendingPermitId],
securityAlertResponse: {
block: 7596565,
result_type: 'Malicious',
reason: 'permit_farming',
description:
'permit_farming to spender 0x1661f1b207629e4f385da89cff535c8e5eb23ee3, classification: A known malicious address is involved in the transaction',
features: ['A known malicious address is involved in the transaction'],
source: 'api',
securityAlertId: 'ba944b14-aa65-45b5-ae92-f305cdba64c1',
},
},
};

state.unapprovedTypedMessages = {
...unapprovedTypedMessage,
};

return {
...state,
signatureSecurityAlertResponses: {
'ba944b14-aa65-45b5-ae92-f305cdba64c1': {
block: 7596565,
result_type: 'Malicious',
reason: 'permit_farming',
description:
'permit_farming to spender 0x1661f1b207629e4f385da89cff535c8e5eb23ee3, classification: A known malicious address is involved in the transaction',
features: ['A known malicious address is involved in the transaction'],
source: 'api',
securityAlertId: 'ba944b14-aa65-45b5-ae92-f305cdba64c1',
},
},
};
};

export const verifyDetails = (element: Element, expectedValues: string[]) => {
expectedValues.forEach((value) => {
expect(element).toHaveTextContent(value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -491,4 +491,52 @@ describe('Contract Interaction Confirmation', () => {
expect(await screen.findByText(headingText)).toBeInTheDocument();
expect(await screen.findByText(bodyText)).toBeInTheDocument();
});

it('tracks external link clicked in transaction metrics', async () => {
const account =
mockMetaMaskState.internalAccounts.accounts[
mockMetaMaskState.internalAccounts
.selectedAccount as keyof typeof mockMetaMaskState.internalAccounts.accounts
];

const mockedMetaMaskState =
getMetaMaskStateWithMaliciousUnapprovedContractInteraction(
account.address,
);

await act(async () => {
await integrationTestRender({
preloadedState: mockedMetaMaskState,
backgroundConnection: backgroundConnectionMocked,
});
});

fireEvent.click(await screen.findByTestId('disclosure'));
expect(
await screen.findByTestId('alert-provider-report-link'),
).toBeInTheDocument();

fireEvent.click(await screen.findByTestId('alert-provider-report-link'));

fireEvent.click(await screen.findByTestId('confirm-footer-cancel-button'));

let updateTransactionEventFragment;

await waitFor(() => {
updateTransactionEventFragment =
mockedBackgroundConnection.submitRequestToBackground.mock.calls?.find(
(call) =>
call[0] === 'updateEventFragment' &&
JSON.stringify(call[1]).includes(
JSON.stringify({
properties: {
external_link_clicked: 'security_alert_support_link',
},
}),
),
);

expect(updateTransactionEventFragment).toBeDefined();
});
});
Comment on lines +525 to +541
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to change this to use jest matchers as well?

});
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ function ReportLink({
<Text marginTop={1} display={Display.Flex}>
{t('somethingDoesntLookRight', [
<ButtonLink
data-testid="alert-provider-report-link"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We avoid adding data-testid in extension, in case its possible to query using button text it may be better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this suggestion. I do like using findByText to also test text expectations simultaneously

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But text can change, making tests flaky and prone to failure due to content updates. Coupling tests to specific text, especially when supporting multiple languages is not recommended. Why do we not want to use test-ids? it supports testing and makes things stable.

key={`security-provider-button-supporturl-${provider}`}
size={ButtonLinkSize.Inherit}
href={reportUrl ?? ZENDESK_URLS.SUPPORT_URL}
Expand Down
14 changes: 14 additions & 0 deletions ui/pages/confirmations/components/confirm/title/title.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,28 @@ import { useIsNFT } from '../info/approve/hooks/use-is-nft';
import { useTokenTransactionData } from '../info/hooks/useTokenTransactionData';
import { getIsRevokeSetApprovalForAll } from '../info/utils';
import { getIsRevokeDAIPermit } from '../utils';
import { useSignatureEventFragment } from '../../../hooks/useSignatureEventFragment';
import { useTransactionEventFragment } from '../../../hooks/useTransactionEventFragment';
import { useCurrentSpendingCap } from './hooks/useCurrentSpendingCap';

function ConfirmBannerAlert({ ownerId }: { ownerId: string }) {
const { generalAlerts } = useAlerts(ownerId);
const { updateSignatureEventFragment } = useSignatureEventFragment();
const { updateTransactionEventFragment } = useTransactionEventFragment();

if (generalAlerts.length === 0) {
return null;
}

const onClickSupportLink = () => {
const properties = {
properties: {
external_link_clicked: 'security_alert_support_link',
},
};
updateSignatureEventFragment(properties);
updateTransactionEventFragment(properties, ownerId);
};
return (
<Box marginTop={3}>
{generalAlerts.map((alert) => (
Expand All @@ -45,6 +58,7 @@ function ConfirmBannerAlert({ ownerId }: { ownerId: string }) {
details={alert.alertDetails}
reportUrl={alert.reportUrl}
children={alert.content}
onClickSupportLink={onClickSupportLink}
/>
</Box>
))}
Expand Down
Loading