Skip to content

fix: properly surface Treezor user rejection errors #8490

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.

Sign up for GitHub

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

Open
Olivier-BB wants to merge 1 commit into
base: main
Choose a base branch
from
+277 −35
Open

fix: properly surface Treezor user rejection errors #8490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions packages/keyring-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
"@metamask/keyring-api": "^21.6.0",
"@metamask/keyring-internal-api": "^10.0.0",
"@metamask/messenger": "^1.1.1",
"@metamask/rpc-errors": "^7.0.2",
"@metamask/utils": "^11.9.0",
"async-mutex": "^0.5.0",
"ethereumjs-wallet": "^1.0.1",
Expand Down
117 changes: 88 additions & 29 deletions packages/keyring-controller/src/KeyringController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import type {
MessengerEvents,
MockAnyNamespace,
} from '@metamask/messenger';
import { errorCodes } from '@metamask/rpc-errors';
import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english';
import { bytesToHex, isValidHexAddress } from '@metamask/utils';
import type { Hex } from '@metamask/utils';
Expand All @@ -32,10 +33,7 @@ import MockEncryptor, {
SALT,
} from '../tests/mocks/mockEncryptor';
import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring';
import {
HardwareWalletError,
MockHardwareKeyring,
} from '../tests/mocks/mockHardwareKeyring';
import { MockHardwareKeyring } from '../tests/mocks/mockHardwareKeyring';
import { MockKeyring } from '../tests/mocks/mockKeyring';
import MockShallowKeyring from '../tests/mocks/mockShallowKeyring';
import { buildMockTransaction } from '../tests/mocks/mockTransaction';
Expand Down Expand Up @@ -1797,6 +1795,29 @@ describe('KeyringController', () => {
).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound);
});
});

it('normalizes cancellation-like signMessage errors to 4001', async () => {
await withController(async ({ controller, initialState }) => {
const account = initialState.keyrings[0].accounts[0];
const keyring = (await controller.getKeyringForAccount(
account,
)) as EthKeyring;
jest
.spyOn(keyring, 'signMessage')
.mockRejectedValue(new Error('Action canceled by user'));

await expect(
controller.signMessage({
data: '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0',
from: account,
}),
).rejects.toMatchObject({
code: errorCodes.provider.userRejectedRequest,
message:
'MetaMask Tx Signature: User denied transaction signature.',
});
});
});
});

describe('when the keyring for the given address does not support signMessage', () => {
Expand Down Expand Up @@ -2224,6 +2245,38 @@ describe('KeyringController', () => {
).rejects.toThrow(/^Keyring Controller signTypedMessage:/u);
});
});

it('normalizes cancellation-like signTypedMessage errors to 4001', async () => {
await withController(async ({ controller, initialState }) => {
const account = initialState.keyrings[0].accounts[0];
const keyring = (await controller.getKeyringForAccount(
account,
)) as EthKeyring;
jest
.spyOn(keyring, 'signTypedData')
.mockRejectedValue(new Error('failure_actioncancelled'));

await expect(
controller.signTypedMessage(
{
data: [
{
name: 'Message',
type: 'string',
value: 'Hi, Alice!',
},
],
from: account,
},
SignTypedDataVersion.V1,
),
).rejects.toMatchObject({
code: errorCodes.provider.userRejectedRequest,
message:
'MetaMask Tx Signature: User denied transaction signature.',
});
});
});
});

describe('when the keyring for the given address does not support signTypedMessage', () => {
Expand Down Expand Up @@ -2351,6 +2404,26 @@ describe('KeyringController', () => {
}).rejects.toThrow('tx.sign is not a function');
});
});

it('normalizes cancellation-like signTransaction errors to 4001', async () => {
await withController(async ({ controller, initialState }) => {
const account = initialState.keyrings[0].accounts[0];
const keyring = (await controller.getKeyringForAccount(
account,
)) as EthKeyring;
jest
.spyOn(keyring, 'signTransaction')
.mockRejectedValue(new Error('Action cancelled by user'));

await expect(
controller.signTransaction(buildMockTransaction(), account),
).rejects.toMatchObject({
code: errorCodes.provider.userRejectedRequest,
message:
'MetaMask Tx Signature: User denied transaction signature.',
});
});
});
});

describe('when the keyring for the given address does not support signTransaction', () => {
Expand Down Expand Up @@ -5344,7 +5417,7 @@ describe('KeyringController', () => {

describe('error handling', () => {
describe('when hardware wallet throws custom error', () => {
it('should preserve hardware wallet error in originalError property', async () => {
it('normalizes hardware user-rejection errors to 4001', async () => {
const mockHardwareKeyringBuilder = keyringBuilderFactory(
MockHardwareKeyring as unknown as KeyringClass,
);
Expand Down Expand Up @@ -5384,7 +5457,11 @@ describe('KeyringController', () => {
{ data: JSON.stringify(typedData), from: hardwareAddress },
SignTypedDataVersion.V4,
),
).rejects.toThrow(KeyringControllerError);
).rejects.toMatchObject({
code: errorCodes.provider.userRejectedRequest,
message:
'MetaMask Tx Signature: User denied transaction signature.',
});

// Verify the error details by catching it explicitly
let caughtError: unknown;
Expand All @@ -5397,29 +5474,11 @@ describe('KeyringController', () => {
caughtError = error;
}

// Verify the error is a KeyringControllerError (wrapped by signTypedMessage)
expect(caughtError).toBeInstanceOf(KeyringControllerError);

const keyringError = caughtError as KeyringControllerError;

// Verify the error message contains information about the hardware wallet error
expect(keyringError.message).toContain(
'Keyring Controller signTypedMessage',
);
expect(keyringError.message).toContain('HardwareWalletError');
expect(keyringError.message).toContain(
'User rejected the request on hardware device',
);

// Verify the original hardware wallet error is preserved in originalError
expect(keyringError.cause).toBeInstanceOf(HardwareWalletError);
expect(keyringError.cause?.message).toBe(
'User rejected the request on hardware device',
);
expect(keyringError.cause?.name).toBe('HardwareWalletError');
expect((keyringError.cause as HardwareWalletError).code).toBe(
'USER_REJECTED',
);
expect(caughtError).toMatchObject({
code: errorCodes.provider.userRejectedRequest,
message:
'MetaMask Tx Signature: User denied transaction signature.',
});
},
);
});
Expand Down
79 changes: 76 additions & 3 deletions packages/keyring-controller/src/KeyringController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type {
import type { EthKeyring } from '@metamask/keyring-internal-api';
import type { Keyring, KeyringClass } from '@metamask/keyring-utils';
import type { Messenger } from '@metamask/messenger';
import { providerErrors } from '@metamask/rpc-errors';
import type { Eip1024EncryptedData, Hex, Json } from '@metamask/utils';
import {
add0x,
Expand Down Expand Up @@ -1367,7 +1368,11 @@ export class KeyringController<
);
}

return await keyring.signMessage(address, messageParams.data);
try {
return await keyring.signMessage(address, messageParams.data);
} catch (error) {
throw this.#normalizeSigningRejectionError(error);
}
}

/**
Expand Down Expand Up @@ -1428,7 +1433,11 @@ export class KeyringController<

const normalizedData = normalize(messageParams.data) as Hex;

return await keyring.signPersonalMessage(address, normalizedData);
try {
return await keyring.signPersonalMessage(address, normalizedData);
} catch (error) {
throw this.#normalizeSigningRejectionError(error);
}
}

/**
Expand Down Expand Up @@ -1477,6 +1486,11 @@ export class KeyringController<
{ version },
);
} catch (error) {
const normalizedError = this.#normalizeSigningRejectionError(error);
if (normalizedError !== error) {
throw normalizedError;
}

const errorMessage =
error instanceof Error
? `${error.name}: ${error.message}`
Expand Down Expand Up @@ -1510,7 +1524,66 @@ export class KeyringController<
);
}

return await keyring.signTransaction(address, transaction, opts);
try {
return await keyring.signTransaction(address, transaction, opts);
} catch (error) {
throw this.#normalizeSigningRejectionError(error);
}
}

#normalizeSigningRejectionError(error: unknown): unknown {
if (!this.#isSigningUserRejectedError(error)) {
return error;
}

const errorData = isObject(error)
? (error.data as Json | undefined)
: undefined;

return providerErrors.userRejectedRequest({
message: 'MetaMask Tx Signature: User denied transaction signature.',
data: errorData,
});
}

#isSigningUserRejectedError(
error: unknown,
visited = new Set<unknown>(),
): boolean {
if (!error || visited.has(error)) {
return false;
}
visited.add(error);

if (typeof error === 'string') {
return /(?:\buser rejected\b|\baction cancelled\b|\bcancelled\b|\bcanceled\b|failure_actioncancelled)/iu.test(
error,
);
}

if (typeof error !== 'object') {
return false;
}

const errorObject = error as {
code?: unknown;
message?: unknown;
stack?: unknown;
cause?: unknown;
originalError?: unknown;
};

if (errorObject.code === 4001) {
return true;
}

return (
this.#isSigningUserRejectedError(errorObject.code, visited) ||
this.#isSigningUserRejectedError(errorObject.message, visited) ||
this.#isSigningUserRejectedError(errorObject.stack, visited) ||
this.#isSigningUserRejectedError(errorObject.cause, visited) ||
this.#isSigningUserRejectedError(errorObject.originalError, visited)
);
}

/**
Expand Down
49 changes: 49 additions & 0 deletions packages/transaction-controller/src/TransactionController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3253,10 +3253,12 @@ describe('TransactionController', () => {
*
* @param controller - The controller instance.
* @param expectedError - The expected error message.
* @param expectedErrorCode - The expected persisted error code.
*/
async function expectTransactionToFail(
controller: TransactionController,
expectedError: string,
expectedErrorCode?: number,
): Promise<void> {
const { result } = await controller.addTransaction(
{
Expand All @@ -3274,6 +3276,11 @@ describe('TransactionController', () => {
expect(txParams.from).toBe(ACCOUNT_MOCK);
expect(txParams.to).toBe(ACCOUNT_MOCK);
expect(status).toBe(TransactionStatus.failed);
if (expectedErrorCode !== undefined) {
expect(controller.state.transactions[0].error).toMatchObject({
code: expectedErrorCode,
});
}
}

it('if signing error', async () => {
Expand Down Expand Up @@ -3308,6 +3315,48 @@ describe('TransactionController', () => {
await expectTransactionToFail(controller, 'No sign method defined');
});

it('normalizes "cancelled" signing errors to userRejectedRequest', async () => {
const { controller } = setupController({
options: {
sign: () => {
throw new Error('Action cancelled by user');
},
},
messengerOptions: {
addTransactionApprovalRequest: {
state: 'approved',
},
},
});

await expectTransactionToFail(
controller,
'MetaMask Tx Signature: User denied transaction signature.',
errorCodes.provider.userRejectedRequest,
);
});

it('normalizes "canceled" signing errors to userRejectedRequest', async () => {
const { controller } = setupController({
options: {
sign: () => {
throw new Error('Action canceled by user');
},
},
messengerOptions: {
addTransactionApprovalRequest: {
state: 'approved',
},
},
});

await expectTransactionToFail(
controller,
'MetaMask Tx Signature: User denied transaction signature.',
errorCodes.provider.userRejectedRequest,
);
});

it('if unexpected status', async () => {
const { controller } = setupController({
messengerOptions: {
Expand Down
Loading
Loading