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

feat: Add domain binding SIWE redesign alert row #25281

Merged
merged 55 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
04318ca
refactor: update SignatureRequestType siwe type to SIWEMessage
digiwand Jun 13, 2024
1fda0b5
TEMP: prep to add domain binding alert in sign redesign SIWE
digiwand Jun 13, 2024
68e8b29
Merge branch 'develop' into feat-sign-refactor-alerts
digiwand Jun 14, 2024
979ad8f
feat: add domain binding SIWE alert
digiwand Jun 14, 2024
661de18
fix: isSIWESignatureRequest request?
digiwand Jun 14, 2024
2a9dbeb
refactor: revert msgParams destruct
digiwand Jun 14, 2024
35efdfc
chore: rm unused comment
digiwand Jun 14, 2024
2f58e32
fix: msgParams destruct and alert title/reason
digiwand Jun 18, 2024
fbcf497
fix: lint useSignatureAlerts
digiwand Jun 19, 2024
f1c0ac7
Merge branch 'develop' into feat-sign-refactor-alerts
digiwand Jun 19, 2024
dfa35a1
refactor: useSignatureAlerts -> useDomainMismatchAlerts
digiwand Jun 20, 2024
5e8fb5b
refactor: useDomainMismatchAlerts -> useDomainMismatchAlert
digiwand Jun 20, 2024
9ed03d9
fix: disable SIWE redesign
digiwand Jun 20, 2024
78363ec
refactor: useDomainMismatchAlert rm useMemo and directly return
digiwand Jun 20, 2024
ca059ab
style: export default function useDomainMismatchAlerts
digiwand Jun 20, 2024
44bd4e8
style: rn isSIWEDomainValid -> isValidDomain
digiwand Jun 20, 2024
8bb125b
refactor: use currentConfirmationSelector instead of useCurrentConfir…
digiwand Jun 20, 2024
c21feb2
refactor: use currentConfirmationSelector instead of useCurrentConfir…
digiwand Jun 20, 2024
f779176
refactor: force return boolean isSIWESignatureRequest
digiwand Jun 20, 2024
07b8a97
refactor: mv useDomainMismatchAlert and update useConfirmationAlerts …
digiwand Jun 20, 2024
abc8365
style: rn confirmAlertModalTitleSignIn -> alertReasonSignIn
digiwand Jun 21, 2024
7825774
style: rn -> alertMessageSignInDomainMismatch
digiwand Jun 21, 2024
8fcaec9
refactor: restore useDomainMismatchAlert useMemo
digiwand Jun 21, 2024
60a6493
yarn lint:fix
digiwand Jun 21, 2024
30f7405
fix: should return alert if !isValidDomain
digiwand Jun 21, 2024
a07296d
test: add useDomainMismatchAlert tests
digiwand Jun 21, 2024
1f18f72
yarn lint:fix
digiwand Jun 21, 2024
c195dde
test: split useDomainMismatchAlert msgParams and SIWE tests
digiwand Jun 21, 2024
1069915
test: update useDomainMismatchAlert mock states
digiwand Jun 21, 2024
f8ddcbb
yarn lint:fix
digiwand Jun 21, 2024
245c5c6
Merge branch 'develop' into feat-sign-refactor-alerts
digiwand Jun 21, 2024
c45f50e
fix: lint PersonalInfo SIWESign statement null
digiwand Jun 21, 2024
3763396
fix: no early return with useMemo
digiwand Jun 21, 2024
a467e42
Merge branch 'develop' into feat-sign-refactor-alerts
digiwand Jun 21, 2024
4ecafc7
fix: rm superfluous msgParams check
digiwand Jun 21, 2024
5ae550d
fix: useDomainMismatchAlert useMemo logic
digiwand Jun 21, 2024
f87de03
refactor: update useDomainMismatchAlert useMemo logic
digiwand Jun 21, 2024
9d22e20
fix: mock personal_sign parsedMessage
digiwand Jun 21, 2024
be02ff6
Merge branch 'develop' into feat-sign-refactor-alerts
digiwand Jun 21, 2024
323f1f8
fix: use {} in case !currentConfirmation
digiwand Jun 21, 2024
8397115
fix: useDomainMismatchAlerts tests
digiwand Jun 21, 2024
ea6aeda
style: readd a comment to address SIWE redesign
digiwand Jun 21, 2024
bd39c5b
yarn lint:fix
digiwand Jun 21, 2024
780f5b7
fix: update: test
digiwand Jun 23, 2024
834f68a
Merge branch 'develop' into feat-sign-refactor-alerts
digiwand Jun 23, 2024
3771748
refactor: reduce useDomainMismatchAlert dep
digiwand Jun 25, 2024
ffda4d1
style: update conditional
digiwand Jun 25, 2024
a98fbcd
fix: lint newline
digiwand Jun 25, 2024
e817fec
yarn lint:fix
digiwand Jun 25, 2024
51df149
Merge branch 'develop' into feat-sign-refactor-alerts
digiwand Jun 26, 2024
c2e8a8c
Merge branch 'develop' into feat-sign-refactor-alerts
digiwand Jun 26, 2024
e78bdd0
chore: mv useDomainMismatchAlert
digiwand Jun 26, 2024
95f28be
chore: pluarlize useDomainMismatchAlerts filepath
digiwand Jun 26, 2024
ec7f458
Merge branch 'develop' into feat-sign-refactor-alerts
digiwand Jun 26, 2024
52502c2
Merge branch 'develop' into feat-sign-refactor-alerts
digiwand Jun 27, 2024
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
6 changes: 6 additions & 0 deletions app/_locales/en/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 17 additions & 10 deletions test/data/confirmations/personal_sign.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,20 @@ export const signatureRequestSIWE = {
siwe: {
isSIWEMessage: true,
parsedMessage: {
domain: 'metamask.github.io',
address: '0x935e73edb9ff52e23bac7f7e049a1ecd06d05477',
chainId: 1,
domain: 'metamask.github.io',
expirationTime: null,
issuedAt: '2021-09-30T16:25:24.000Z',
nonce: '32891757',
notBefore: '2022-03-17T12:45:13.610Z',
requestId: 'some_id',
scheme: null,
statement:
'I accept the MetaMask Terms of Service: https://community.metamask.io/tos',
uri: 'https://metamask.github.io',
version: '1',
chainId: 1,
nonce: '32891757',
issuedAt: '2021-09-30T16:25:24.000Z',
resources: null,
},
},
},
Expand All @@ -67,17 +72,19 @@ export const SignatureRequestSIWEWithResources = {
siwe: {
isSIWEMessage: true,
parsedMessage: {
domain: 'metamask.github.io',
address: '0x935e73edb9ff52e23bac7f7e043a1ecd06d05477',
statement:
'I accept the MetaMask Terms of Service: https://community.metamask.io/tos',
uri: 'https://metamask.github.io',
version: '1',
chainId: 1,
nonce: '32891757',
domain: 'metamask.github.io',
expirationTime: null,
issuedAt: '2021-09-30T16:25:24.000Z',
nonce: '32891757',
notBefore: '2022-03-17T12:45:13.610Z',
requestId: 'some_id',
scheme: null,
statement:
'I accept the MetaMask Terms of Service: https://community.metamask.io/tos',
uri: 'https://metamask.github.io',
version: '1',
resources: [
'ipfs://Qme7ss3ARVgxv6rXqVPiikMJ8u2NLgmgszg13pYrDKEoiu',
'https://example.com/my-web2-claim.json',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,30 @@ exports[`SIWESignInfo renders correctly for SIWE signature request 1`] = `
</p>
</div>
</div>
<div
class="mm-box confirm-info-row mm-box--margin-top-2 mm-box--margin-bottom-2 mm-box--padding-right-2 mm-box--padding-left-2 mm-box--display-flex mm-box--flex-direction-row mm-box--flex-wrap-wrap mm-box--justify-content-space-between mm-box--color-text-default mm-box--rounded-lg"
style="overflow-wrap: anywhere; min-height: 24px;"
>
<div
class="mm-box mm-box--display-flex mm-box--flex-direction-row mm-box--justify-content-center mm-box--align-items-center"
>
<p
class="mm-box mm-text mm-text--body-md-medium mm-box--color-inherit"
>
Request ID
</p>
</div>
<div
class="mm-box mm-box--display-flex mm-box--gap-2 mm-box--flex-wrap-wrap mm-box--align-items-center"
>
<p
class="mm-box mm-text mm-text--body-md mm-box--color-inherit"
style="white-space: pre-wrap;"
>
some_id
</p>
</div>
</div>
</div>
`;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const SIWESignInfo: React.FC = () => {
return (
<>
<ConfirmInfoRow label={t('message')}>
<ConfirmInfoRowText text={statement} />
<ConfirmInfoRowText text={statement || ''} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<ConfirmInfoRowText text={statement || ''} />
<ConfirmInfoRowText text={statement ?? ''} />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh nice! I can update this in a subsequent alert PR to not need re-reviews

</ConfirmInfoRow>
<ConfirmInfoRow label={t('siweURI')}>
<ConfirmInfoRowText text={domain} />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
import { ApprovalType } from '@metamask/controller-utils';
import { Severity } from '../../../../../helpers/constants/design-system';
import { renderHookWithProvider } from '../../../../../../test/lib/render-helpers';
import mockState from '../../../../../../test/data/mock-state.json';
import useDomainMismatchAlert from './useDomainMismatchAlerts';

const MOCK_ORIGIN = 'https://example-dapp.example';
const MOCK_SUSPICIOUS_DOMAIN = 'http://suspicious.example';
const MOCK_ADDRESS = '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc';

const mockSiwe = {
isSIWEMessage: true,
parsedMessage: {
domain: MOCK_SUSPICIOUS_DOMAIN,
address: MOCK_ADDRESS,
statement:
'Click to sign in and accept the Terms of Service: https://community.metamask.io/tos',
uri: 'http://localhost:8080',
version: '1',
nonce: 'STMt6KQMwwdOXE306',
chainId: 1,
issuedAt: '2023-03-18T21:40:40.823Z',
resources: [
'ipfs://Qme7ss3ARVgxv6rXqVPiikMJ8u2NLgmgszg13pYrDKEoiu',
'https://example.com/my-web2-claim.json',
],
},
};

const mockCurrentConfirmation = {
id: '1',
status: 'unapproved',
time: new Date().getTime(),
type: ApprovalType.PersonalSign,
msgParams: {
from: MOCK_ADDRESS,
data: '0x6c6f63616c686f73743a383038302077616e747320796f7520746f207369676e20696e207769746820796f757220457468657265756d206163636f756e743a0a3078466232433135303034333433393034653566343038323537386334653865313131303563463765330a0a436c69636b20746f207369676e20696e20616e642061636365707420746865205465726d73206f6620536572766963653a2068747470733a2f2f636f6d6d756e6974792e6d6574616d61736b2e696f2f746f730a0a5552493a20687474703a2f2f6c6f63616c686f73743a383038300a56657273696f6e3a20310a436861696e2049443a20310a4e6f6e63653a2053544d74364b514d7777644f58453330360a4973737565642041743a20323032322d30332d31385432313a34303a34302e3832335a0a5265736f75726365733a0a2d20697066733a2f2f516d653773733341525667787636725871565069696b4d4a3875324e4c676d67737a673133705972444b456f69750a2d2068747470733a2f2f6578616d706c652e636f6d2f6d792d776562322d636c61696d2e6a736f6e',
origin: MOCK_ORIGIN,
siwe: mockSiwe,
},
};

const mockExpectedState = {
...mockState,
metamask: {
...mockState.metamask,
unapprovedPersonalMsgs: {
'1': { ...mockCurrentConfirmation },
},
pendingApprovals: {
'1': {
...mockCurrentConfirmation,
// origin: MOCK_ORIGIN,
requestData: {},
requestState: null,
expectsResult: false,
},
},
preferences: { redesignedConfirmationsEnabled: true },
},
confirm: { currentConfirmation: mockCurrentConfirmation },
};

describe('useDomainMismatchAlert', () => {
beforeAll(() => {
process.env.ENABLE_CONFIRMATION_REDESIGN = 'true';
});

afterAll(() => {
process.env.ENABLE_CONFIRMATION_REDESIGN = 'false';
});

describe('returns an empty array', () => {
it('when there is no current confirmation', () => {
const { result } = renderHookWithProvider(
() => useDomainMismatchAlert(),
mockState,
);
expect(result.current).toEqual([]);
});

it('when the current confirmation is not a SIWE request', () => {
const { result } = renderHookWithProvider(
() => useDomainMismatchAlert(),
{
...mockExpectedState,
confirm: {
currentConfirmation: {
...mockCurrentConfirmation,
msgParams: {
...mockCurrentConfirmation.msgParams,
siwe: {
isSIWEMessage: false,
parsedMessage: mockSiwe.parsedMessage,
},
},
},
},
},
);
expect(result.current).toEqual([]);
});

it('when the SIWE domain matches origin', () => {
const originalDomain = mockSiwe.parsedMessage.domain;
mockSiwe.parsedMessage.domain = MOCK_ORIGIN;

const { result } = renderHookWithProvider(
() => useDomainMismatchAlert(),
mockExpectedState,
);
expect(result.current).toEqual([]);

mockSiwe.parsedMessage.domain = originalDomain;
});
});

it('returns an alert when the SIWE domain does not match the origin', () => {
const alertResponseExpected = {
field: 'requestFrom',
key: 'requestFrom',
message:
'The site making the request is not the site you’re signing into. This could be an attempt to steal your login credentials.',
reason: 'Suspicious sign-in request',
severity: Severity.Danger,
};
const { result } = renderHookWithProvider(
() => useDomainMismatchAlert(),
mockExpectedState,
);

expect(result.current).toHaveLength(1);
expect(result.current[0]).toStrictEqual(alertResponseExpected);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { useMemo } from 'react';
import { useSelector } from 'react-redux';
import {
isValidSIWEOrigin,
WrappedSIWERequest,
} from '@metamask/controller-utils';

import { Alert } from '../../../../../ducks/confirm-alerts/confirm-alerts';
import { Severity } from '../../../../../helpers/constants/design-system';
import { useI18nContext } from '../../../../../hooks/useI18nContext';
import { currentConfirmationSelector } from '../../../../../selectors';

import { SignatureRequestType } from '../../../types/confirm';
import { isSIWESignatureRequest } from '../../../utils';

export default function useDomainMismatchAlerts(): Alert[] {
const t = useI18nContext();

const currentConfirmation = useSelector(
currentConfirmationSelector,
) as SignatureRequestType;
const { msgParams } = currentConfirmation || {};

const isSIWE = isSIWESignatureRequest(currentConfirmation);
const isInvalidSIWEDomain =
isSIWE && !isValidSIWEOrigin(msgParams as WrappedSIWERequest);
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be useful to include above 3 lines also inside useMemo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had this originally and then @matthewwalsh0 brought up a good point to reduce the # of observed dependencies


const alerts = useMemo(() => {
if (!isInvalidSIWEDomain) {
return [];
}

return [
{
field: 'requestFrom',
key: 'requestFrom',
message: t('alertMessageSignInDomainMismatch'),
reason: t('alertReasonSignIn'),
severity: Severity.Danger,
},
] as Alert[];
}, [isInvalidSIWEDomain, t]);

return alerts;
}
12 changes: 10 additions & 2 deletions ui/pages/confirmations/hooks/useConfirmationAlerts.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { useMemo } from 'react';
import { Alert } from '../../../ducks/confirm-alerts/confirm-alerts';
import useBlockaidAlerts from './alerts/useBlockaidAlerts';
import useDomainMismatchAlerts from './alerts/signatures/useDomainMismatchAlerts';
import { useInsufficientBalanceAlerts } from './alerts/transactions/useInsufficientBalanceAlerts';
import { useGasEstimateFailedAlerts } from './alerts/transactions/useGasEstimateFailedAlerts';
import { usePendingTransactionAlerts } from './alerts/transactions/usePendingTransactionAlerts';
Expand All @@ -10,6 +11,12 @@ import { useGasTooLowAlerts } from './alerts/transactions/useGasTooLowAlerts';
import { useNoGasPriceAlerts } from './alerts/transactions/useNoGasPriceAlerts';
import { useNetworkBusyAlerts } from './alerts/transactions/useNetworkBusyAlerts';

function useSignatureAlerts(): Alert[] {
const domainMismatchAlerts = useDomainMismatchAlerts();
matthewwalsh0 marked this conversation as resolved.
Show resolved Hide resolved

return useMemo(() => [...domainMismatchAlerts], [domainMismatchAlerts]);
}

function useTransactionAlerts(): Alert[] {
const gasEstimateFailedAlerts = useGasEstimateFailedAlerts();
const gasFeeLowAlerts = useGasFeeLowAlerts();
Expand Down Expand Up @@ -46,10 +53,11 @@ function useTransactionAlerts(): Alert[] {

export default function useConfirmationAlerts(): Alert[] {
const blockaidAlerts = useBlockaidAlerts();
const signatureAlerts = useSignatureAlerts();
const transactionAlerts = useTransactionAlerts();

return useMemo(
() => [...blockaidAlerts, ...transactionAlerts],
[blockaidAlerts, transactionAlerts],
() => [...blockaidAlerts, ...signatureAlerts, ...transactionAlerts],
[blockaidAlerts, signatureAlerts, transactionAlerts],
);
}
4 changes: 4 additions & 0 deletions ui/pages/confirmations/hooks/useCurrentConfirmation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ const useCurrentConfirmation = () => {
if (
!redesignedConfirmationsEnabled ||
(!isCorrectTransactionType && !isCorrectApprovalType) ||
/**
* @todo remove isSIWE check when we want to enable SIWE in redesigned confirmations
* @see {@link https://github.com/MetaMask/metamask-extension/issues/24617}
*/
isSIWE
) {
return { currentConfirmation: undefined };
Expand Down
18 changes: 3 additions & 15 deletions ui/pages/confirmations/types/confirm.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { ApprovalControllerState } from '@metamask/approval-controller';
import { SIWEMessage } from '@metamask/controller-utils';

import {
TransactionMeta,
TransactionType,
Expand Down Expand Up @@ -27,21 +29,7 @@ export type SignatureRequestType = {
data: string | TypedSignDataV1Type;
version?: string;
signatureMethod?: string;
siwe?: {
isSIWEMessage: boolean;
parsedMessage: null | {
domain: string;
address: string;
statement: string;
uri: string;
version: string;
chainId: number;
nonce: string;
issuedAt: string;
requestId?: string;
resources?: string[];
};
};
siwe?: SIWEMessage;
};
type: TransactionType;
custodyId?: string;
Expand Down
2 changes: 1 addition & 1 deletion ui/pages/confirmations/utils/confirm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export const parseSanitizeTypedDataMessage = (dataToParse: string) => {
};

export const isSIWESignatureRequest = (request: SignatureRequestType) =>
request.msgParams?.siwe?.isSIWEMessage;
Boolean(request?.msgParams?.siwe?.isSIWEMessage);

export const isPermitSignatureRequest = (request: SignatureRequestType) => {
if (
Expand Down