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

Security provider check (OpenSea) #16584

Merged
merged 19 commits into from
Jan 23, 2023
Merged
Changes from 18 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
13 changes: 13 additions & 0 deletions app/scripts/controllers/transactions/index.js
Original file line number Diff line number Diff line change
@@ -150,6 +150,7 @@ export default class TransactionController extends EventEmitter {
this.getDeviceModel = opts.getDeviceModel;
this.getAccountType = opts.getAccountType;
this.getTokenStandardAndDetails = opts.getTokenStandardAndDetails;
this.securityProviderRequest = opts.securityProviderRequest;

this.memStore = new ObservableStore({});

@@ -336,6 +337,7 @@ export default class TransactionController extends EventEmitter {
);

const initialTxMeta = await this.addUnapprovedTransaction(
opts.method,
txParams,
opts.origin,
undefined,
@@ -765,6 +767,7 @@ export default class TransactionController extends EventEmitter {
* actionId is fix used for making this action idempotent to deal with scenario when
* action is invoked multiple times with same parameters in MV3 due to service worker re-activation.
*
* @param txMethodType
* @param txParams
* @param origin
* @param transactionType
@@ -773,6 +776,7 @@ export default class TransactionController extends EventEmitter {
* @returns {txMeta}
*/
blackdevelopa marked this conversation as resolved.
Show resolved Hide resolved
async addUnapprovedTransaction(
txMethodType,
txParams,
origin,
transactionType,
@@ -855,6 +859,15 @@ export default class TransactionController extends EventEmitter {
? addHexPrefix(txMeta.txParams.value)
: '0x0';

if (txMethodType && this.securityProviderRequest) {
const securityProviderResponse = await this.securityProviderRequest(
txMeta,
txMethodType,
);

txMeta.securityProviderResponse = securityProviderResponse;
}

this.addTransaction(txMeta);
this.emit('newUnapprovedTx', txMeta);

62 changes: 54 additions & 8 deletions app/scripts/controllers/transactions/index.test.js
Original file line number Diff line number Diff line change
@@ -96,6 +96,7 @@ describe('Transaction Controller', function () {
getEIP1559GasFeeEstimates: () => undefined,
getAccountType: () => 'MetaMask',
getDeviceModel: () => 'N/A',
securityProviderRequest: () => undefined,
});
txController.nonceTracker.getNonceLock = () =>
Promise.resolve({ nextNonce: 0, releaseLock: noop });
@@ -362,7 +363,7 @@ describe('Transaction Controller', function () {
});

it('should add an unapproved transaction and return a valid txMeta', async function () {
const txMeta = await txController.addUnapprovedTransaction({
const txMeta = await txController.addUnapprovedTransaction(undefined, {
from: selectedAddress,
to: recipientAddress,
});
@@ -386,6 +387,7 @@ describe('Transaction Controller', function () {

it('should add only 1 unapproved transaction when called twice with same actionId', async function () {
await txController.addUnapprovedTransaction(
undefined,
{
from: selectedAddress,
to: recipientAddress,
@@ -398,6 +400,7 @@ describe('Transaction Controller', function () {
const transactionCount1 =
txController.txStateManager.getTransactions().length;
await txController.addUnapprovedTransaction(
undefined,
{
from: selectedAddress,
to: recipientAddress,
@@ -414,6 +417,7 @@ describe('Transaction Controller', function () {

it('should add multiple transactions when called with different actionId', async function () {
await txController.addUnapprovedTransaction(
undefined,
{
from: selectedAddress,
to: recipientAddress,
@@ -426,6 +430,7 @@ describe('Transaction Controller', function () {
const transactionCount1 =
txController.txStateManager.getTransactions().length;
await txController.addUnapprovedTransaction(
undefined,
{
from: selectedAddress,
to: recipientAddress,
@@ -447,7 +452,7 @@ describe('Transaction Controller', function () {
done();
});
txController
.addUnapprovedTransaction({
.addUnapprovedTransaction(undefined, {
from: selectedAddress,
to: recipientAddress,
})
@@ -466,7 +471,7 @@ describe('Transaction Controller', function () {
networkStore.putState('loading');
await assert.rejects(
() =>
txController.addUnapprovedTransaction({
txController.addUnapprovedTransaction(undefined, {
from: selectedAddress,
to: '0x0d1d4e623D10F9FBA5Db95830F7d3839406C6AF2',
}),
@@ -510,7 +515,7 @@ describe('Transaction Controller', function () {
});

it('should add an cancel transaction and return a valid txMeta', async function () {
const txMeta = await txController.addUnapprovedTransaction({
const txMeta = await txController.addUnapprovedTransaction(undefined, {
from: selectedAddress,
to: recipientAddress,
});
@@ -528,7 +533,7 @@ describe('Transaction Controller', function () {
});

it('should add only 1 cancel transaction when called twice with same actionId', async function () {
const txMeta = await txController.addUnapprovedTransaction({
const txMeta = await txController.addUnapprovedTransaction(undefined, {
from: selectedAddress,
to: recipientAddress,
});
@@ -551,7 +556,7 @@ describe('Transaction Controller', function () {
});

it('should add multiple transactions when called with different actionId', async function () {
const txMeta = await txController.addUnapprovedTransaction({
const txMeta = await txController.addUnapprovedTransaction(undefined, {
from: selectedAddress,
to: recipientAddress,
});
@@ -1279,7 +1284,7 @@ describe('Transaction Controller', function () {
});

it('should add only 1 speedup transaction when called twice with same actionId', async function () {
const txMeta = await txController.addUnapprovedTransaction({
const txMeta = await txController.addUnapprovedTransaction(undefined, {
from: selectedAddress,
to: recipientAddress,
});
@@ -1302,7 +1307,7 @@ describe('Transaction Controller', function () {
});

it('should add multiple transactions when called with different actionId', async function () {
const txMeta = await txController.addUnapprovedTransaction({
const txMeta = await txController.addUnapprovedTransaction(undefined, {
from: selectedAddress,
to: recipientAddress,
});
blackdevelopa marked this conversation as resolved.
Show resolved Hide resolved
@@ -1323,6 +1328,47 @@ describe('Transaction Controller', function () {
txController.txStateManager.getTransactions().length;
assert.equal(transactionCount1 + 1, transactionCount2);
});

it('should add multiple transactions when called with different actionId and txMethodType defined', async function () {
const txMeta = await txController.addUnapprovedTransaction(
'eth_sendTransaction',
{
from: selectedAddress,
to: recipientAddress,
},
);
await txController.approveTransaction(txMeta.id);
await txController.createSpeedUpTransaction(
txMeta.id,
{},
{ actionId: 12345 },
);
const transactionCount1 =
txController.txStateManager.getTransactions().length;
await txController.createSpeedUpTransaction(
txMeta.id,
{},
{ actionId: 11111 },
);
const transactionCount2 =
txController.txStateManager.getTransactions().length;
assert.equal(transactionCount1 + 1, transactionCount2);
});

it('should call securityProviderRequest and have flagAsDangerous inside txMeta', async function () {
const txMeta = await txController.addUnapprovedTransaction(
'eth_sendTransaction',
{
from: selectedAddress,
to: recipientAddress,
},
);

assert.ok(
'securityProviderResponse' in txMeta,
'should have a securityProviderResponse',
);
});
blackdevelopa marked this conversation as resolved.
Show resolved Hide resolved
});

describe('#signTransaction', function () {
15 changes: 12 additions & 3 deletions app/scripts/lib/message-manager.js
Original file line number Diff line number Diff line change
@@ -29,8 +29,9 @@ export default class MessageManager extends EventEmitter {
*
* @param {object} opts - Controller options
* @param {Function} opts.metricsEvent - A function for emitting a metric event.
* @param {Function} opts.securityProviderRequest - A function for verifying a message, whether it is malicious or not.
*/
constructor({ metricsEvent }) {
constructor({ metricsEvent, securityProviderRequest }) {
super();
this.memStore = new ObservableStore({
unapprovedMsgs: {},
@@ -46,6 +47,7 @@ export default class MessageManager extends EventEmitter {

this.messages = [];
this.metricsEvent = metricsEvent;
this.securityProviderRequest = securityProviderRequest;
}

/**
@@ -80,7 +82,7 @@ export default class MessageManager extends EventEmitter {
* @returns {promise} after signature has been
*/
async addUnapprovedMessageAsync(msgParams, req) {
const msgId = this.addUnapprovedMessage(msgParams, req);
const msgId = await this.addUnapprovedMessage(msgParams, req);
return await new Promise((resolve, reject) => {
// await finished
this.once(`${msgId}:finished`, (data) => {
@@ -118,7 +120,7 @@ export default class MessageManager extends EventEmitter {
* @param {object} [req] - The original request object where the origin may be specified
* @returns {number} The id of the newly created message.
*/
addUnapprovedMessage(msgParams, req) {
async addUnapprovedMessage(msgParams, req) {
// add origin from request
if (req) {
msgParams.origin = req.origin;
@@ -136,6 +138,13 @@ export default class MessageManager extends EventEmitter {
};
this.addMsg(msgData);

const securityProviderResponse = await this.securityProviderRequest(
msgData,
msgData.type,
);

msgData.securityProviderResponse = securityProviderResponse;

// signal update
this.emit('update');
return msgId;
64 changes: 37 additions & 27 deletions app/scripts/lib/personal-message-manager.js
Original file line number Diff line number Diff line change
@@ -36,8 +36,9 @@ export default class PersonalMessageManager extends EventEmitter {
*
* @param options
* @param options.metricsEvent
* @param options.securityProviderRequest
*/
constructor({ metricsEvent }) {
constructor({ metricsEvent, securityProviderRequest }) {
super();
this.memStore = new ObservableStore({
unapprovedPersonalMsgs: {},
@@ -53,6 +54,7 @@ export default class PersonalMessageManager extends EventEmitter {

this.messages = [];
this.metricsEvent = metricsEvent;
this.securityProviderRequest = securityProviderRequest;
}

/**
@@ -96,31 +98,32 @@ export default class PersonalMessageManager extends EventEmitter {
);
return;
}
blackdevelopa marked this conversation as resolved.
Show resolved Hide resolved
const msgId = this.addUnapprovedMessage(msgParams, req);
this.once(`${msgId}:finished`, (data) => {
switch (data.status) {
case 'signed':
resolve(data.rawSig);
return;
case 'rejected':
reject(
ethErrors.provider.userRejectedRequest(
'MetaMask Message Signature: User denied message signature.',
),
);
return;
case 'errored':
reject(new Error(`MetaMask Message Signature: ${data.error}`));
return;
default:
reject(
new Error(
`MetaMask Message Signature: Unknown problem: ${JSON.stringify(
msgParams,
)}`,
),
);
}
this.addUnapprovedMessage(msgParams, req).then((msgId) => {
this.once(`${msgId}:finished`, (data) => {
switch (data.status) {
case 'signed':
resolve(data.rawSig);
return;
case 'rejected':
reject(
ethErrors.provider.userRejectedRequest(
'MetaMask Message Signature: User denied message signature.',
),
);
return;
case 'errored':
reject(new Error(`MetaMask Message Signature: ${data.error}`));
return;
default:
reject(
new Error(
`MetaMask Message Signature: Unknown problem: ${JSON.stringify(
msgParams,
)}`,
),
);
}
});
});
});
}
@@ -134,7 +137,7 @@ export default class PersonalMessageManager extends EventEmitter {
* @param {object} [req] - The original request object possibly containing the origin
* @returns {number} The id of the newly created PersonalMessage.
*/
addUnapprovedMessage(msgParams, req) {
async addUnapprovedMessage(msgParams, req) {
log.debug(
`PersonalMessageManager addUnapprovedMessage: ${JSON.stringify(
msgParams,
@@ -162,6 +165,13 @@ export default class PersonalMessageManager extends EventEmitter {
};
this.addMsg(msgData);

const securityProviderResponse = await this.securityProviderRequest(
msgData,
msgData.type,
);

msgData.securityProviderResponse = securityProviderResponse;

// signal update
this.emit('update');
return msgId;
Loading