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>

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 && (
           <ConfirmPageContainerWarning warning={ethGasPriceWarning} />
         )}
-        {
-          ///: BEGIN:ONLY_INCLUDE_IN(blockaid)
-          <BlockaidBannerAlert
-            securityAlertResponse={txData?.securityAlertResponse}
-          />
-          ///: END:ONLY_INCLUDE_IN
-        }
-        {isSuspiciousResponse(txData?.securityProviderResponse) && (
-          <SecurityProviderBannerMessage
-            securityProviderResponse={txData.securityProviderResponse}
-          />
-        )}
         <ConfirmPageContainerSummary
           className={classnames({
             'confirm-page-container-summary--border':

ui/components/app/confirm-page-container/confirm-page-container-content/confirm-page-container-content.component.test.js

@@ -1,7 +1,6 @@
 import { fireEvent } from '@testing-library/react';
 import React from 'react';
 import configureMockStore from 'redux-mock-store';
-import { SECURITY_PROVIDER_MESSAGE_SEVERITY } from '../../../../../shared/constants/security-provider';
 import { TransactionType } from '../../../../../shared/constants/transaction';
 import { renderWithProvider } from '../../../../../test/lib/render-helpers';
 import {
@@ -51,13 +50,6 @@ describe('Confirm Page Container Content', () => {
       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(
-      <ConfirmPageContainerContent {...props} />,
-      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(
-      <ConfirmPageContainerContent {...props} />,
-      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(
       <ConfirmPageContainerContent
@@ -197,26 +155,4 @@ describe('Confirm Page Container Content', () => {
     );
     expect(getByRole('button', { name: 'Buy' })).toBeInTheDocument();
   });
-
-  it('should display security alert if present', () => {
-    const { getByText } = renderWithProvider(
-      <ConfirmPageContainerContent
-        {...props}
-        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',
-            },
-          },
-        }}
-      />,
-      store,
-    );
-    expect(getByText('This is a deceptive request')).toBeInTheDocument();
-  });
 });

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`] = `
 <div>
   <div
-    class="mm-box mm-banner-base mm-banner-alert mm-banner-alert--severity-danger mm-box--margin-4 mm-box--padding-3 mm-box--padding-left-2 mm-box--display-flex mm-box--gap-2 mm-box--background-color-error-muted mm-box--rounded-sm"
+    class="mm-box mm-banner-base mm-banner-alert mm-banner-alert--severity-danger mm-box--padding-3 mm-box--padding-left-2 mm-box--display-flex mm-box--gap-2 mm-box--background-color-error-muted mm-box--rounded-sm"
   >
     <span
       class="mm-box mm-icon mm-icon--size-lg mm-box--display-inline-block mm-box--color-error-default"

ui/components/app/security-provider-banner-alert/blockaid-banner-alert/__snapshots__/blockaid-banner-alert.test.js.snap

@@ -2,7 +2,7 @@
 
 exports[`Blockaid Banner Alert should render 'danger' UI when securityAlertResponse.result_type is 'Malicious 1`] = `
 <div
-  class="mm-box mm-banner-base mm-banner-alert mm-banner-alert--severity-danger mm-box--margin-4 mm-box--padding-3 mm-box--padding-left-2 mm-box--display-flex mm-box--gap-2 mm-box--background-color-error-muted mm-box--rounded-sm"
+  class="mm-box mm-banner-base mm-banner-alert mm-banner-alert--severity-danger mm-box--padding-3 mm-box--padding-left-2 mm-box--display-flex mm-box--gap-2 mm-box--background-color-error-muted mm-box--rounded-sm"
 >
   <span
     class="mm-box mm-icon mm-icon--size-lg mm-box--display-inline-block mm-box--color-error-default"
@@ -47,7 +47,7 @@ exports[`Blockaid Banner Alert should render 'danger' UI when securityAlertRespo
 
 exports[`Blockaid Banner Alert should render 'warning' UI when securityAlertResponse.result_type is 'Failed 1`] = `
 <div
-  class="mm-box mm-banner-base mm-banner-alert mm-banner-alert--severity-warning mm-box--margin-4 mm-box--padding-3 mm-box--padding-left-2 mm-box--display-flex mm-box--gap-2 mm-box--background-color-warning-muted mm-box--rounded-sm"
+  class="mm-box mm-banner-base mm-banner-alert mm-banner-alert--severity-warning mm-box--padding-3 mm-box--padding-left-2 mm-box--display-flex mm-box--gap-2 mm-box--background-color-warning-muted mm-box--rounded-sm"
 >
   <span
     class="mm-box mm-icon mm-icon--size-lg mm-box--display-inline-block mm-box--color-warning-default"
@@ -70,7 +70,7 @@ exports[`Blockaid Banner Alert should render 'warning' UI when securityAlertResp
 
 exports[`Blockaid Banner Alert should render 'warning' UI when securityAlertResponse.result_type is 'Warning 1`] = `
 <div
-  class="mm-box mm-banner-base mm-banner-alert mm-banner-alert--severity-warning mm-box--margin-4 mm-box--padding-3 mm-box--padding-left-2 mm-box--display-flex mm-box--gap-2 mm-box--background-color-warning-muted mm-box--rounded-sm"
+  class="mm-box mm-banner-base mm-banner-alert mm-banner-alert--severity-warning mm-box--padding-3 mm-box--padding-left-2 mm-box--display-flex mm-box--gap-2 mm-box--background-color-warning-muted mm-box--rounded-sm"
 >
   <span
     class="mm-box mm-icon mm-icon--size-lg mm-box--display-inline-block mm-box--color-warning-default"
@@ -116,7 +116,7 @@ exports[`Blockaid Banner Alert should render 'warning' UI when securityAlertResp
 exports[`Blockaid Banner Alert should render details when provided 1`] = `
 <div>
   <div
-    class="mm-box mm-banner-base mm-banner-alert mm-banner-alert--severity-warning mm-box--margin-4 mm-box--padding-3 mm-box--padding-left-2 mm-box--display-flex mm-box--gap-2 mm-box--background-color-warning-muted mm-box--rounded-sm"
+    class="mm-box mm-banner-base mm-banner-alert mm-banner-alert--severity-warning mm-box--padding-3 mm-box--padding-left-2 mm-box--display-flex mm-box--gap-2 mm-box--background-color-warning-muted mm-box--rounded-sm"
   >
     <span
       class="mm-box mm-icon mm-icon--size-lg mm-box--display-inline-block mm-box--color-warning-default"

ui/components/app/security-provider-banner-alert/blockaid-banner-alert/blockaid-banner-alert.js

@@ -44,7 +44,7 @@ const REASON_TO_TITLE_TKEY = Object.freeze({
   [BlockaidReason.rawSignatureFarming]: 'blockaidTitleSuspicious',
 });
 
-function BlockaidBannerAlert({ securityAlertResponse }) {
+function BlockaidBannerAlert({ securityAlertResponse, ...props }) {
   const t = useContext(I18nContext);
 
   if (!securityAlertResponse) {
@@ -63,13 +63,13 @@ function BlockaidBannerAlert({ securityAlertResponse }) {
 
   const description = t(REASON_TO_DESCRIPTION_TKEY[reason] || 'other');
 
-  const details = Boolean(features?.length) && (
+  const details = features?.length ? (
     <Text as="ul">
       {features.map((feature, i) => (
         <li key={`blockaid-detail-${i}`}>• {feature}</li>
       ))}
     </Text>
-  );
+  ) : null;
 
   const isFailedResultType = resultType === BlockaidResultType.Failed;
 
@@ -87,6 +87,7 @@ function BlockaidBannerAlert({ securityAlertResponse }) {
       provider={isFailedResultType ? null : SecurityProvider.Blockaid}
       severity={severity}
       title={title}
+      {...props}
     />
   );
 }

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 (
-    <BannerAlert title={title} severity={severity} margin={4}>
+    <BannerAlert title={title} severity={severity} {...props}>
       <Text marginTop={2}>{description}</Text>
 
       {details && (

ui/components/app/security-provider-banner-message/security-provider-banner-message.js

@@ -47,6 +47,7 @@ export default function SecurityProviderBannerMessage({
 
   return (
     <BannerAlert
+      className="security-provider-banner-message"
       marginTop={4}
       marginRight={4}
       marginLeft={4}

ui/components/app/signature-request-original/signature-request-original.component.js

@@ -157,6 +157,7 @@ export default class SignatureRequestOriginal extends Component {
           ///: BEGIN:ONLY_INCLUDE_IN(blockaid)
           <BlockaidBannerAlert
             securityAlertResponse={txData?.securityAlertResponse}
+            margin={4}
           />
           ///: END:ONLY_INCLUDE_IN
         }

ui/components/app/signature-request-siwe/signature-request-siwe.js

@@ -130,16 +130,12 @@ export default function SignatureRequestSIWE({ txData }) {
         <ConfirmPageContainerNavigation />
       </div>
       <SignatureRequestHeader txData={txData} />
-      <Header
+
-        fromAccount={fromAccount}
-        domain={origin}
-        isSIWEDomainValid={isSIWEDomainValid}
-        subjectMetadata={targetSubjectMetadata}
-      />
       {
         ///: BEGIN:ONLY_INCLUDE_IN(blockaid)
         <BlockaidBannerAlert
           securityAlertResponse={txData?.securityAlertResponse}
+          margin={4}
         />
         ///: END:ONLY_INCLUDE_IN
       }
@@ -148,6 +144,13 @@ export default function SignatureRequestSIWE({ txData }) {
           securityProviderResponse={txData.securityProviderResponse}
         />
       )}
+
+      <Header
+        fromAccount={fromAccount}
+        domain={origin}
+        isSIWEDomainValid={isSIWEDomainValid}
+        subjectMetadata={targetSubjectMetadata}
+      />
       <Message data={formatMessageParams(parsedMessage, t)} />
       {!isMatchingAddress && (
         <BannerAlert

ui/components/app/signature-request/signature-request.js

@@ -247,6 +247,9 @@ const SignatureRequest = ({ txData }) => {
           ///: BEGIN:ONLY_INCLUDE_IN(blockaid)
           <BlockaidBannerAlert
             securityAlertResponse={txData?.securityAlertResponse}
+            marginLeft={4}
+            marginRight={4}
+            marginBottom={4}
           />
           ///: END:ONLY_INCLUDE_IN
         }

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 (
     <div className="transaction-alerts">
+      {
+        ///: BEGIN:ONLY_INCLUDE_IN(blockaid)
+        <BlockaidBannerAlert
+          securityAlertResponse={txData?.securityAlertResponse}
+        />
+        ///: END:ONLY_INCLUDE_IN
+      }
+      {isSuspiciousResponse(txData?.securityProviderResponse) && (
+        <SecurityProviderBannerMessage
+          securityProviderResponse={txData.securityProviderResponse}
+        />
+      )}
+
       {supportsEIP1559 && hasSimulationError && (
         <SimulationErrorMessage
           userAcknowledgedGasMissing={userAcknowledgedGasMissing}
@@ -72,6 +92,7 @@ const TransactionAlerts = ({
 TransactionAlerts.propTypes = {
   userAcknowledgedGasMissing: PropTypes.bool,
   setUserAcknowledgedGasMissing: PropTypes.func,
+  txData: PropTypes.object,
 };
 
 export default TransactionAlerts;

ui/components/app/transaction-alerts/transaction-alerts.scss

@@ -26,4 +26,10 @@
     padding: 0;
     font-size: $font-size-h7;
   }
+
+  // We plan to deprecate SecurityProviderBannerMessage in favor of SecurityProviderBannerAlert. This override should be temporary
+  .security-provider-banner-message {
+    margin-left: 0;
+    margin-right: 0;
+  }
 }

ui/components/app/transaction-alerts/transaction-alerts.test.js

@@ -1,5 +1,6 @@
 import React from 'react';
 import { fireEvent } from '@testing-library/react';
+import { SECURITY_PROVIDER_MESSAGE_SEVERITY } from '../../../../shared/constants/security-provider';
 import { renderWithProvider } from '../../../../test/jest';
 import { submittedPendingTransactionsSelector } from '../../../selectors/transactions';
 import { useGasFeeContext } from '../../../contexts/gasFee';
@@ -29,6 +30,68 @@ function render({
 }
 
 describe('TransactionAlerts', () => {
+  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', () => {

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)
           <BlockaidBannerAlert
             securityAlertResponse={txData?.securityAlertResponse}
+            margin={4}
           />
           ///: END:ONLY_INCLUDE_IN
         }

ui/pages/confirm-transaction-base/confirm-transaction-base.component.js

@@ -452,6 +452,7 @@ export default class ConfirmTransactionBase extends Component {
     return (
       <div className="confirm-page-container-content__details">
         <TransactionAlerts
+          txData={txData}
           setUserAcknowledgedGasMissing={() =>
             this.setUserAcknowledgedGasMissing()
           }

ui/pages/token-allowance/token-allowance.js

@@ -314,18 +314,6 @@ export default function TokenAllowance({
       <Box>
         <ConfirmPageContainerNavigation />
       </Box>
-      {
-        ///: BEGIN:ONLY_INCLUDE_IN(blockaid)
-        <BlockaidBannerAlert
-          securityAlertResponse={txData?.securityAlertResponse}
-        />
-        ///: END:ONLY_INCLUDE_IN
-      }
-      {isSuspiciousResponse(txData?.securityProviderResponse) && (
-        <SecurityProviderBannerMessage
-          securityProviderResponse={txData.securityProviderResponse}
-        />
-      )}
       <Box
         paddingLeft={4}
         paddingRight={4}
@@ -367,6 +355,19 @@ export default function TokenAllowance({
         accountAddress={userAddress}
         chainId={fullTxData.chainId}
       />
+      {
+        ///: BEGIN:ONLY_INCLUDE_IN(blockaid)
+        <BlockaidBannerAlert
+          securityAlertResponse={txData?.securityAlertResponse}
+          margin={4}
+        />
+        ///: END:ONLY_INCLUDE_IN
+      }
+      {isSuspiciousResponse(txData?.securityProviderResponse) && (
+        <SecurityProviderBannerMessage
+          securityProviderResponse={txData.securityProviderResponse}
+        />
+      )}
       {warning && (
         <Box className="token-allowance-container__custom-nonce-warning">
           <ConfirmPageContainerWarning warning={warning} />