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

Remove eth-keyring-controller from DecryptMessageController #20808

Merged
merged 3 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
diff --git a/dist/KeyringController.d.ts b/dist/KeyringController.d.ts
index ccfd5ca9accf782b0a612ab1666501898cf2abb9..f772552156491b308f2aa29a734a5138237d951e 100644
index d6d70eea2de56310b49fa9dcf1e90afae3ddb202..4518aed8e03376d76e5ab6ef0971529e65563fd3 100644
--- a/dist/KeyringController.d.ts
+++ b/dist/KeyringController.d.ts
@@ -1,10 +1,11 @@
Expand All @@ -10,13 +10,12 @@ index ccfd5ca9accf782b0a612ab1666501898cf2abb9..f772552156491b308f2aa29a734a5138
import { BaseControllerV2 } from '@metamask/base-controller';
import type { PersonalMessageParams, TypedMessageParams } from '@metamask/message-manager';
import type { PreferencesController } from '@metamask/preferences-controller';
-import { type Hex, type Keyring, type Json } from '@metamask/utils';
+import type { Hex, Keyring, Json } from '@metamask/utils';
import type { Eip1024EncryptedData, Hex, Keyring, Json } from '@metamask/utils';
+import type { KeyringController as EthKeyringController } from '@metamask/eth-keyring-controller';
import type { Patch } from 'immer';
declare const name = "KeyringController";
/**
@@ -135,6 +136,10 @@ export declare class KeyringController extends BaseControllerV2<typeof name, Key
@@ -155,6 +156,10 @@ export declare class KeyringController extends BaseControllerV2<typeof name, Key
* @param opts.state - Initial state to set on this controller.
*/
constructor({ removeIdentity, syncIdentities, updateIdentities, setSelectedAddress, setAccountLabel, encryptor, keyringBuilders, cacheEncryptionKey, messenger, state, }: KeyringControllerOptions);
Expand All @@ -28,12 +27,12 @@ index ccfd5ca9accf782b0a612ab1666501898cf2abb9..f772552156491b308f2aa29a734a5138
* Adds a new account to the default (first) HD seed phrase keyring.
*
diff --git a/dist/KeyringController.js b/dist/KeyringController.js
index 9e241ba3b5445a053e3b20ee70a579118b7b2934..eb1f7bce9d3607017b39e087db227e8a7dadd849 100644
index bce9884ea54bf8e24274dee3876db95ed862abf0..471a06b30f2275de821c33b7d94cb1de85b074c7 100644
--- a/dist/KeyringController.js
+++ b/dist/KeyringController.js
@@ -153,6 +153,12 @@ class KeyringController extends base_controller_1.BaseControllerV2 {
this.setSelectedAddress = setSelectedAddress;
@@ -154,6 +154,12 @@ class KeyringController extends base_controller_1.BaseControllerV2 {
this.setAccountLabel = setAccountLabel;
__classPrivateFieldGet(this, _KeyringController_instances, "m", _KeyringController_registerMessageHandlers).call(this);
}
+ /**
+ * Gets the internal keyring controller.
Expand Down
70 changes: 61 additions & 9 deletions app/scripts/controllers/decrypt-message.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ const messageMock = {
type: 'testType',
rawSig: undefined,
} as any as AbstractMessage;
const messageDataMock =
'0x7b2276657273696f6e223a227832353531392d7873616c736132302d706f6c7931333035222c226e6f6e6365223a226b45586143524c3045646142766f77756e35675979357175784a4a6967304548222c22657068656d5075626c69634b6579223a224863334636506d314734385a567955424763365866537839682b77784b6958587238456a51434253466e553d222c2263697068657274657874223a22546a41556b68554a5968656e7a2f655a6e57454a2b31456c7861354f77765939613830507a62746c7a7a48746934634175525941227d';

const mockExtState = {};

Expand Down Expand Up @@ -52,7 +54,7 @@ const createDecryptMessageManagerMock = <T>() =>
},
} as any as jest.Mocked<T>);

describe('EncryptionPublicKeyController', () => {
describe('DecryptMessageController', () => {
let decryptMessageController: DecryptMessageController;

const decryptMessageManagerConstructorMock =
Expand All @@ -65,6 +67,19 @@ describe('EncryptionPublicKeyController', () => {
const decryptMessageManagerMock =
createDecryptMessageManagerMock<DecryptMessageManager>();

const mockMessengerAction = (
action: string,
callback: (actionName: string, ...args: any[]) => any,
) => {
messengerMock.call.mockImplementation((actionName, ...rest) => {
if (actionName === action) {
return callback(actionName, ...rest);
}

return Promise.resolve();
});
};

beforeEach(() => {
jest.resetAllMocks();

Expand Down Expand Up @@ -116,12 +131,18 @@ describe('EncryptionPublicKeyController', () => {
it('should decrypt message', async () => {
const messageToDecrypt = {
...messageMock,
data: '0x7b22666f6f223a22626172227d',
data: messageDataMock,
};
const decryptMessageActionCallbackMock = jest
.fn()
.mockReturnValue('decryptedMessage');
decryptMessageManagerMock.approveMessage.mockResolvedValue(
messageToDecrypt,
);
keyringControllerMock.decryptMessage.mockResolvedValue('decryptedMessage');
mockMessengerAction(
'KeyringController:decryptMessage',
decryptMessageActionCallbackMock,
);
getStateMock.mockReturnValue(mockExtState);

const result = await decryptMessageController.decryptMessage(
Expand All @@ -132,8 +153,9 @@ describe('EncryptionPublicKeyController', () => {
expect(decryptMessageManagerMock.approveMessage).toBeCalledWith(
messageToDecrypt,
);
expect(keyringControllerMock.decryptMessage).toBeCalledTimes(1);
expect(keyringControllerMock.decryptMessage).toBeCalledWith(
expect(decryptMessageActionCallbackMock).toBeCalledTimes(1);
expect(decryptMessageActionCallbackMock).toBeCalledWith(
'KeyringController:decryptMessage',
messageToDecrypt,
);
expect(decryptMessageManagerMock.setMessageStatusAndResult).toBeCalledTimes(
Expand All @@ -147,15 +169,31 @@ describe('EncryptionPublicKeyController', () => {
expect(result).toBe(mockExtState);
});

it('should throw when decrypting invalid message', async () => {
const messageToDecrypt = {
...messageMock,
data: '0x7b2022666f6f223a202262617222207d',
};
decryptMessageManagerMock.approveMessage.mockResolvedValue(
messageToDecrypt,
);

expect(
decryptMessageController.decryptMessage(messageToDecrypt),
).rejects.toThrow('Invalid encrypted data.');
});

it('should cancel decrypt request', async () => {
const messageToDecrypt = {
...messageMock,
data: '0x7b22666f6f223a22626172227d',
data: messageDataMock,
};
decryptMessageManagerMock.approveMessage.mockResolvedValue(
messageToDecrypt,
);
keyringControllerMock.decryptMessage.mockRejectedValue(new Error('error'));
mockMessengerAction('KeyringController:decryptMessage', async () => {
throw new Error('error');
});
getStateMock.mockReturnValue(mockExtState);

return expect(
Expand All @@ -166,10 +204,13 @@ describe('EncryptionPublicKeyController', () => {
it('should decrypt message inline', async () => {
const messageToDecrypt = {
...messageMock,
data: '0x7b22666f6f223a22626172227d',
data: messageDataMock,
};
decryptMessageManagerMock.getMessage.mockReturnValue(messageToDecrypt);
keyringControllerMock.decryptMessage.mockResolvedValue('decryptedMessage');
mockMessengerAction(
'KeyringController:decryptMessage',
async () => 'decryptedMessage',
);
getStateMock.mockReturnValue(mockExtState);

const result = await decryptMessageController.decryptMessageInline(
Expand All @@ -184,6 +225,17 @@ describe('EncryptionPublicKeyController', () => {
expect(result).toBe(mockExtState);
});

it('should throw when decrypting invalid message inline', async () => {
const messageToDecrypt = {
...messageMock,
data: '0x7b2022666f6f223a202262617222207d',
};

expect(
decryptMessageController.decryptMessageInline(messageToDecrypt),
).rejects.toThrow('Invalid encrypted data.');
});

it('should be able to cancel decrypt message', async () => {
decryptMessageManagerMock.rejectMessage.mockResolvedValue(messageMock);
getStateMock.mockReturnValue(mockExtState);
Expand Down
59 changes: 49 additions & 10 deletions app/scripts/controllers/decrypt-message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
DecryptMessageParams,
DecryptMessageParamsMetamask,
} from '@metamask/message-manager';
import { KeyringController } from '@metamask/eth-keyring-controller';
import {
AbstractMessage,
AbstractMessageManager,
Expand All @@ -25,6 +24,8 @@ import {
} from '@metamask/approval-controller';
import { ApprovalType, ORIGIN_METAMASK } from '@metamask/controller-utils';
import { Patch } from 'immer';
import type { KeyringControllerDecryptMessageAction } from '@metamask/keyring-controller';
import { Eip1024EncryptedData, hasProperty, isObject } from '@metamask/utils';
import { MetaMetricsEventCategory } from '../../../shared/constants/metametrics';
import { stripHexPrefix } from '../../../shared/modules/hexstring-utils';

Expand All @@ -35,6 +36,37 @@ const stateMetadata = {
unapprovedDecryptMsgCount: { persist: false, anonymous: false },
};

/**
* Type guard that checks for the presence of the required properties
* for EIP-1024 encrypted data.
*
* See: https://eips.ethereum.org/EIPS/eip-1024
*
* @param message - The message to check.
* @param message.from - The sender of the message.
* @param message.data - The EIP-1024 encrypted data.
* @returns Whether the message is an EIP-1024 encrypted message.
*/
export const isEIP1024EncryptedMessage = (message: {
mikesposito marked this conversation as resolved.
Show resolved Hide resolved
from: string;
data: unknown;
}): message is {
from: string;
data: Eip1024EncryptedData;
} => {
if (
isObject(message.data) &&
hasProperty(message.data, 'version') &&
hasProperty(message.data, 'nonce') &&
hasProperty(message.data, 'ephemPublicKey') &&
hasProperty(message.data, 'ciphertext')
) {
return true;
}

return false;
};

export const getDefaultState = () => ({
unapprovedDecryptMsgs: {},
unapprovedDecryptMsgCount: 0,
Expand Down Expand Up @@ -70,7 +102,11 @@ export type DecryptMessageControllerActions = GetDecryptMessageState;

export type DecryptMessageControllerEvents = DecryptMessageStateChange;

type AllowedActions = AddApprovalRequest | AcceptRequest | RejectRequest;
type AllowedActions =
| AddApprovalRequest
| AcceptRequest
| RejectRequest
| KeyringControllerDecryptMessageAction;

export type DecryptMessageControllerMessenger = RestrictedControllerMessenger<
typeof controllerName,
Expand All @@ -82,7 +118,6 @@ export type DecryptMessageControllerMessenger = RestrictedControllerMessenger<

export type DecryptMessageControllerOptions = {
getState: () => any;
keyringController: KeyringController;
messenger: DecryptMessageControllerMessenger;
metricsEvent: (payload: any, options?: any) => void;
};
Expand All @@ -99,8 +134,6 @@ export default class DecryptMessageController extends BaseControllerV2<

private _getState: () => any;

private _keyringController: KeyringController;

private _metricsEvent: (payload: any, options?: any) => void;

private _decryptMessageManager: DecryptMessageManager;
Expand All @@ -110,13 +143,11 @@ export default class DecryptMessageController extends BaseControllerV2<
*
* @param options - The controller options.
* @param options.getState - Callback to retrieve all user state.
* @param options.keyringController - An instance of a keyring controller used to decrypt message
* @param options.messenger - A reference to the messaging system.
* @param options.metricsEvent - A function for emitting a metric event.
*/
constructor({
getState,
keyringController,
metricsEvent,
messenger,
}: DecryptMessageControllerOptions) {
Expand All @@ -127,7 +158,6 @@ export default class DecryptMessageController extends BaseControllerV2<
state: getDefaultState(),
});
this._getState = getState;
this._keyringController = keyringController;
this._metricsEvent = metricsEvent;

this.hub = new EventEmitter();
Expand Down Expand Up @@ -216,7 +246,12 @@ export default class DecryptMessageController extends BaseControllerV2<
await this._decryptMessageManager.approveMessage(messageParams);

cleanMessageParams.data = this._parseMessageData(cleanMessageParams.data);
const rawMessage = await this._keyringController.decryptMessage(
if (!isEIP1024EncryptedMessage(cleanMessageParams)) {
throw new Error('Invalid encrypted data.');
mikesposito marked this conversation as resolved.
Show resolved Hide resolved
}

const rawMessage = await this.messagingSystem.call(
'KeyringController:decryptMessage',
cleanMessageParams,
);

Expand All @@ -243,7 +278,11 @@ export default class DecryptMessageController extends BaseControllerV2<
async decryptMessageInline(messageParams: DecryptMessageParamsMetamask) {
const messageId = messageParams.metamaskId as string;
messageParams.data = this._parseMessageData(messageParams.data);
const rawMessage = await this._keyringController.decryptMessage(
if (!isEIP1024EncryptedMessage(messageParams)) {
throw new Error('Invalid encrypted data.');
}
const rawMessage = await this.messagingSystem.call(
'KeyringController:decryptMessage',
messageParams,
);

Expand Down
2 changes: 1 addition & 1 deletion app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -1373,13 +1373,13 @@ export default class MetamaskController extends EventEmitter {
this.networkController.lookupNetwork();
this.decryptMessageController = new DecryptMessageController({
getState: this.getState.bind(this),
keyringController: this.keyringController,
messenger: this.controllerMessenger.getRestricted({
name: 'DecryptMessageController',
allowedActions: [
`${this.approvalController.name}:addRequest`,
`${this.approvalController.name}:acceptRequest`,
`${this.approvalController.name}:rejectRequest`,
`${this.coreKeyringController.name}:decryptMessage`,
],
}),
metricsEvent: this.metaMetricsController.trackEvent.bind(
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@
"request@^2.88.2": "patch:request@npm%3A2.88.2#./.yarn/patches/request-npm-2.88.2-f4a57c72c4.patch",
"request@^2.85.0": "patch:request@npm%3A2.88.2#./.yarn/patches/request-npm-2.88.2-f4a57c72c4.patch",
"lavamoat-core@^14.2.0": "patch:lavamoat-core@npm%3A14.2.0#./.yarn/patches/lavamoat-core-npm-14.2.0-c453f4f755.patch",
"@metamask/keyring-controller@^7.2.0": "patch:@metamask/keyring-controller@npm%3A7.2.0#~/.yarn/patches/@metamask-keyring-controller-npm-7.2.0-fcc0c7893b.patch",
"@metamask/keyring-controller@^7.4.0": "patch:@metamask/keyring-controller@npm%3A7.4.0#~/.yarn/patches/@metamask-keyring-controller-npm-7.4.0-5dd5df31c7.patch",
"@metamask/signature-controller@^5.3.0": "patch:@metamask/signature-controller@npm%3A5.3.0#./.yarn/patches/@metamask-signature-controller-npm-5.3.0-225628460b.patch",
"[email protected]": "^7.5.4",
"[email protected]": "^7.5.4"
Expand Down Expand Up @@ -252,7 +252,7 @@
"@metamask/gas-fee-controller": "^6.0.1",
"@metamask/jazzicon": "^2.0.0",
"@metamask/key-tree": "^9.0.0",
"@metamask/keyring-controller": "^7.2.0",
"@metamask/keyring-controller": "^7.4.0",
"@metamask/logo": "^3.1.1",
"@metamask/message-manager": "^7.3.0",
"@metamask/metamask-eth-abis": "^3.0.0",
Expand Down
18 changes: 9 additions & 9 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4285,9 +4285,9 @@ __metadata:
languageName: node
linkType: hard

"@metamask/keyring-controller@npm:7.2.0":
version: 7.2.0
resolution: "@metamask/keyring-controller@npm:7.2.0"
"@metamask/keyring-controller@npm:7.4.0":
version: 7.4.0
resolution: "@metamask/keyring-controller@npm:7.4.0"
dependencies:
"@keystonehq/metamask-airgapped-keyring": "npm:^0.13.1"
"@metamask/base-controller": "npm:^3.2.1"
Expand All @@ -4301,13 +4301,13 @@ __metadata:
immer: "npm:^9.0.6"
peerDependencies:
"@metamask/preferences-controller": ^4.4.0
checksum: 3535d698f68f732658787943eaff3ebabdc1e3d0e6bce731cff413b0fb98d19fb127841e525d9031fa96b50e75840686da9cd0615ab6772914e2b18fe1797df1
checksum: 17b3f24d57dc79689ee6c0159f931ba71b10cbb728cecce6595ec69c46bc706289e90e22b9e7709fef8719ff99594adccd842a3a687fcc651a55cd873f2bc219
languageName: node
linkType: hard

"@metamask/keyring-controller@patch:@metamask/keyring-controller@npm%3A7.2.0#~/.yarn/patches/@metamask-keyring-controller-npm-7.2.0-fcc0c7893b.patch":
version: 7.2.0
resolution: "@metamask/keyring-controller@patch:@metamask/keyring-controller@npm%3A7.2.0#~/.yarn/patches/@metamask-keyring-controller-npm-7.2.0-fcc0c7893b.patch::version=7.2.0&hash=6f51dc"
"@metamask/keyring-controller@patch:@metamask/keyring-controller@npm%3A7.4.0#~/.yarn/patches/@metamask-keyring-controller-npm-7.4.0-5dd5df31c7.patch":
version: 7.4.0
resolution: "@metamask/keyring-controller@patch:@metamask/keyring-controller@npm%3A7.4.0#~/.yarn/patches/@metamask-keyring-controller-npm-7.4.0-5dd5df31c7.patch::version=7.4.0&hash=4a7ca3"
dependencies:
"@keystonehq/metamask-airgapped-keyring": "npm:^0.13.1"
"@metamask/base-controller": "npm:^3.2.1"
Expand All @@ -4321,7 +4321,7 @@ __metadata:
immer: "npm:^9.0.6"
peerDependencies:
"@metamask/preferences-controller": ^4.4.0
checksum: b3f0ffac12077c19bef8d978237b24ebe40c2bbee1363ee5cfdf7a09b600540981d3bdbac96845645cd976e0bdb55f56f80442a2797e1538abb00744194edf17
checksum: d6965c9e3f7fe00481ed544fae172aa04f8212e570ccea4c1d16f52f42c775759df59a1009c52937e1f6c6a764c16b80b89c457a4765009ff9bb1d454aab42ab
languageName: node
linkType: hard

Expand Down Expand Up @@ -24221,7 +24221,7 @@ __metadata:
"@metamask/gas-fee-controller": "npm:^6.0.1"
"@metamask/jazzicon": "npm:^2.0.0"
"@metamask/key-tree": "npm:^9.0.0"
"@metamask/keyring-controller": "npm:^7.2.0"
"@metamask/keyring-controller": "npm:^7.4.0"
"@metamask/logo": "npm:^3.1.1"
"@metamask/message-manager": "npm:^7.3.0"
"@metamask/metamask-eth-abis": "npm:^3.0.0"
Expand Down