From 3f5bc978ddac65249ad5380c3f59bf2a82705c22 Mon Sep 17 00:00:00 2001 From: Ariella Vu <20778143+digiwand@users.noreply.github.com> Date: Wed, 30 Aug 2023 07:05:53 -0700 Subject: [PATCH] Fix Blockaid and OpenSea BannerAlert placement for Token Allowance, Confirm Pages, SIWE, and Signature V3/V4 pages (#20530) * refactor: add SecurityProviderBannerAlert ...props * fix: mv security banner alerts on TokenAllowance * fix: mv security BannerAlert in confirm page https://github.com/MetaMask/MetaMask-planning/issues/1195 * fix: allow BlockaidBannerAlert null details * refactor: rm SecurityProviderBannerAlert margin * fix: SIWE security banner alert placement * fix: rm extra banner alert padding for sig v3 & v4 * fix: update SecurityProviderBannerAlert snapshot * fix: update BlockaidBannerAlert snapshot --------- Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com> --- ...onfirm-page-container-content.component.js | 20 ------ ...m-page-container-content.component.test.js | 64 ------------------- ...ecurity-provider-banner-alert.test.js.snap | 2 +- .../blockaid-banner-alert.test.js.snap | 8 +-- .../blockaid-banner-alert.js | 7 +- .../security-provider-banner-alert.js | 3 +- .../security-provider-banner-message.js | 1 + .../signature-request-original.component.js | 1 + .../signature-request-siwe.js | 15 +++-- .../signature-request/signature-request.js | 3 + .../transaction-alerts/transaction-alerts.js | 21 ++++++ .../transaction-alerts.scss | 6 ++ .../transaction-alerts.test.js | 63 ++++++++++++++++++ .../confirm-approve-content.component.js | 1 + .../confirm-transaction-base.component.js | 1 + ui/pages/token-allowance/token-allowance.js | 25 ++++---- 16 files changed, 130 insertions(+), 111 deletions(-) diff --git a/ui/components/app/confirm-page-container/confirm-page-container-content/confirm-page-container-content.component.js b/ui/components/app/confirm-page-container/confirm-page-container-content/confirm-page-container-content.component.js index f881c9029..81d07fcae 100644 --- a/ui/components/app/confirm-page-container/confirm-page-container-content/confirm-page-container-content.component.js +++ b/ui/components/app/confirm-page-container/confirm-page-container-content/confirm-page-container-content.component.js @@ -14,12 +14,6 @@ import { PageContainerFooter } from '../../../ui/page-container'; import { INSUFFICIENT_FUNDS_ERROR_KEY } from '../../../../helpers/constants/error-keys'; import { Severity } from '../../../../helpers/constants/design-system'; -import { isSuspiciousResponse } from '../../../../../shared/modules/security-provider.utils'; -///: BEGIN:ONLY_INCLUDE_IN(blockaid) -import BlockaidBannerAlert from '../../security-provider-banner-alert/blockaid-banner-alert/blockaid-banner-alert'; -///: END:ONLY_INCLUDE_IN -import SecurityProviderBannerMessage from '../../security-provider-banner-message/security-provider-banner-message'; - import { ConfirmPageContainerSummary, ConfirmPageContainerWarning } from '.'; export default class ConfirmPageContainerContent extends Component { @@ -67,7 +61,6 @@ export default class ConfirmPageContainerContent extends Component { ///: BEGIN:ONLY_INCLUDE_IN(build-main,build-beta,build-flask) openBuyCryptoInPdapp: PropTypes.func, ///: END:ONLY_INCLUDE_IN - txData: PropTypes.object, ///: BEGIN:ONLY_INCLUDE_IN(build-mmi) noteComponent: PropTypes.node, ///: END:ONLY_INCLUDE_IN @@ -209,7 +202,6 @@ export default class ConfirmPageContainerContent extends Component { ///: BEGIN:ONLY_INCLUDE_IN(build-main,build-beta,build-flask) openBuyCryptoInPdapp, ///: END:ONLY_INCLUDE_IN - txData, } = this.props; const { t } = this.context; @@ -227,18 +219,6 @@ export default class ConfirmPageContainerContent extends Component { {ethGasPriceWarning && ( )} - { - ///: BEGIN:ONLY_INCLUDE_IN(blockaid) - - ///: END:ONLY_INCLUDE_IN - } - {isSuspiciousResponse(txData?.securityProviderResponse) && ( - - )} { disabled: true, origin: 'http://localhost:4200', hideTitle: false, - txData: { - securityProviderResponse: { - flagAsDangerous: '?', - reason: 'Some reason...', - reason_header: 'Some reason header...', - }, - }, }; }); @@ -138,40 +130,6 @@ describe('Confirm Page Container Content', () => { expect(queryByText('Address Book Account 1')).not.toBeInTheDocument(); }); - it('should render SecurityProviderBannerMessage component properly', () => { - const { queryByText } = renderWithProvider( - , - store, - ); - - expect(queryByText('Request not verified')).toBeInTheDocument(); - expect( - queryByText( - 'Because of an error, this request was not verified by the security provider. Proceed with caution.', - ), - ).toBeInTheDocument(); - expect(queryByText('OpenSea')).toBeInTheDocument(); - }); - - it('should not render SecurityProviderBannerMessage component when flagAsDangerous is not malicious', () => { - props.txData.securityProviderResponse = { - flagAsDangerous: SECURITY_PROVIDER_MESSAGE_SEVERITY.NOT_MALICIOUS, - }; - - const { queryByText } = renderWithProvider( - , - store, - ); - - expect(queryByText('Request not verified')).toBeNull(); - expect( - queryByText( - 'Because of an error, this request was not verified by the security provider. Proceed with caution.', - ), - ).toBeNull(); - expect(queryByText('OpenSea')).toBeNull(); - }); - it('should show insufficient funds error for EIP-1559 network', () => { const { getByRole } = renderWithProvider( { ); expect(getByRole('button', { name: 'Buy' })).toBeInTheDocument(); }); - - it('should display security alert if present', () => { - const { getByText } = renderWithProvider( - , - store, - ); - expect(getByText('This is a deceptive request')).toBeInTheDocument(); - }); }); diff --git a/ui/components/app/security-provider-banner-alert/__snapshots__/security-provider-banner-alert.test.js.snap b/ui/components/app/security-provider-banner-alert/__snapshots__/security-provider-banner-alert.test.js.snap index 43c9bb879..f79956bbe 100644 --- a/ui/components/app/security-provider-banner-alert/__snapshots__/security-provider-banner-alert.test.js.snap +++ b/ui/components/app/security-provider-banner-alert/__snapshots__/security-provider-banner-alert.test.js.snap @@ -3,7 +3,7 @@ exports[`Security Provider Banner Alert should match snapshot 1`] = `
{features.map((feature, i) => (
  • • {feature}
    • ))} - ); + ) : null; const isFailedResultType = resultType === BlockaidResultType.Failed; @@ -87,6 +87,7 @@ function BlockaidBannerAlert({ securityAlertResponse }) { provider={isFailedResultType ? null : SecurityProvider.Blockaid} severity={severity} title={title} + {...props} /> ); } diff --git a/ui/components/app/security-provider-banner-alert/security-provider-banner-alert.js b/ui/components/app/security-provider-banner-alert/security-provider-banner-alert.js index d79b6eb35..b133e181d 100644 --- a/ui/components/app/security-provider-banner-alert/security-provider-banner-alert.js +++ b/ui/components/app/security-provider-banner-alert/security-provider-banner-alert.js @@ -33,11 +33,12 @@ function SecurityProviderBannerAlert({ provider, severity, title, + ...props }) { const t = useContext(I18nContext); return ( - + {description} {details && ( diff --git a/ui/components/app/security-provider-banner-message/security-provider-banner-message.js b/ui/components/app/security-provider-banner-message/security-provider-banner-message.js index fdaba451f..9dc2db6f9 100644 --- a/ui/components/app/security-provider-banner-message/security-provider-banner-message.js +++ b/ui/components/app/security-provider-banner-message/security-provider-banner-message.js @@ -47,6 +47,7 @@ export default function SecurityProviderBannerMessage({ return ( ///: END:ONLY_INCLUDE_IN } diff --git a/ui/components/app/signature-request-siwe/signature-request-siwe.js b/ui/components/app/signature-request-siwe/signature-request-siwe.js index 4332abbd3..7a43baa78 100644 --- a/ui/components/app/signature-request-siwe/signature-request-siwe.js +++ b/ui/components/app/signature-request-siwe/signature-request-siwe.js @@ -130,16 +130,12 @@ export default function SignatureRequestSIWE({ txData }) {
    -
    + { ///: BEGIN:ONLY_INCLUDE_IN(blockaid) ///: END:ONLY_INCLUDE_IN } @@ -148,6 +144,13 @@ export default function SignatureRequestSIWE({ txData }) { securityProviderResponse={txData.securityProviderResponse} /> )} + +
    {!isMatchingAddress && ( { ///: BEGIN:ONLY_INCLUDE_IN(blockaid) ///: END:ONLY_INCLUDE_IN } diff --git a/ui/components/app/transaction-alerts/transaction-alerts.js b/ui/components/app/transaction-alerts/transaction-alerts.js index 72b65c991..0738fdc10 100644 --- a/ui/components/app/transaction-alerts/transaction-alerts.js +++ b/ui/components/app/transaction-alerts/transaction-alerts.js @@ -11,9 +11,16 @@ import SimulationErrorMessage from '../../ui/simulation-error-message'; import { SEVERITIES } from '../../../helpers/constants/design-system'; import ZENDESK_URLS from '../../../helpers/constants/zendesk-url'; +import { isSuspiciousResponse } from '../../../../shared/modules/security-provider.utils'; +///: BEGIN:ONLY_INCLUDE_IN(blockaid) +import BlockaidBannerAlert from '../security-provider-banner-alert/blockaid-banner-alert/blockaid-banner-alert'; +///: END:ONLY_INCLUDE_IN +import SecurityProviderBannerMessage from '../security-provider-banner-message/security-provider-banner-message'; + const TransactionAlerts = ({ userAcknowledgedGasMissing, setUserAcknowledgedGasMissing, + txData, }) => { const { estimateUsed, hasSimulationError, supportsEIP1559, isNetworkBusy } = useGasFeeContext(); @@ -22,6 +29,19 @@ const TransactionAlerts = ({ return (
    + { + ///: BEGIN:ONLY_INCLUDE_IN(blockaid) + + ///: END:ONLY_INCLUDE_IN + } + {isSuspiciousResponse(txData?.securityProviderResponse) && ( + + )} + {supportsEIP1559 && hasSimulationError && ( { + it('should display security alert if present', () => { + const { getByText } = render({ + componentProps: { + txData: { + securityAlertResponse: { + resultType: 'Malicious', + reason: 'blur_farming', + description: + 'A SetApprovalForAll request was made on {contract}. We found the operator {operator} to be malicious', + args: { + contract: '0xa7206d878c5c3871826dfdb42191c49b1d11f466', + operator: '0x92a3b9773b1763efa556f55ccbeb20441962d9b2', + }, + }, + }, + }, + }); + expect(getByText('This is a deceptive request')).toBeInTheDocument(); + }); + + it('should render SecurityProviderBannerMessage component properly', () => { + const { queryByText } = render({ + componentProps: { + txData: { + securityProviderResponse: { + flagAsDangerous: '?', + reason: 'Some reason...', + reason_header: 'Some reason header...', + }, + }, + }, + }); + + expect(queryByText('Request not verified')).toBeInTheDocument(); + expect( + queryByText( + 'Because of an error, this request was not verified by the security provider. Proceed with caution.', + ), + ).toBeInTheDocument(); + expect(queryByText('OpenSea')).toBeInTheDocument(); + }); + + it('should not render SecurityProviderBannerMessage component when flagAsDangerous is not malicious', () => { + const { queryByText } = render({ + componentProps: { + txData: { + securityProviderResponse: { + flagAsDangerous: SECURITY_PROVIDER_MESSAGE_SEVERITY.NOT_MALICIOUS, + }, + }, + }, + }); + + expect(queryByText('Request not verified')).toBeNull(); + expect( + queryByText( + 'Because of an error, this request was not verified by the security provider. Proceed with caution.', + ), + ).toBeNull(); + expect(queryByText('OpenSea')).toBeNull(); + }); + describe('when supportsEIP1559 from useGasFeeContext is truthy', () => { describe('if hasSimulationError from useGasFeeContext is true', () => { it('informs the user that a simulation of the transaction failed', () => { diff --git a/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js b/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js index 817450231..39c105573 100644 --- a/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js +++ b/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js @@ -558,6 +558,7 @@ export default class ConfirmApproveContent extends Component { ///: BEGIN:ONLY_INCLUDE_IN(blockaid) ///: END:ONLY_INCLUDE_IN } diff --git a/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js index 0ee18509f..f610f6b92 100644 --- a/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js @@ -452,6 +452,7 @@ export default class ConfirmTransactionBase extends Component { return (
    this.setUserAcknowledgedGasMissing() } diff --git a/ui/pages/token-allowance/token-allowance.js b/ui/pages/token-allowance/token-allowance.js index 8a24c639f..53568cd21 100644 --- a/ui/pages/token-allowance/token-allowance.js +++ b/ui/pages/token-allowance/token-allowance.js @@ -314,18 +314,6 @@ export default function TokenAllowance({ - { - ///: BEGIN:ONLY_INCLUDE_IN(blockaid) - - ///: END:ONLY_INCLUDE_IN - } - {isSuspiciousResponse(txData?.securityProviderResponse) && ( - - )} + { + ///: BEGIN:ONLY_INCLUDE_IN(blockaid) + + ///: END:ONLY_INCLUDE_IN + } + {isSuspiciousResponse(txData?.securityProviderResponse) && ( + + )} {warning && (