m/metamask-extension
1
0
Fork 0
mirror of https://github.com/kremalicious/metamask-extension.git synced 2024-12-22 17:33:23 +01:00
Code

Stop storing request and response objects in the permission activity log (#14485)

Browse Source 
We currently store the JSON-RPC request and response objects in the permission activity log. The utility of doing this was always rather dubious, but never problematic. Until now.

In Flask, as the restricted methods have expanded in number, user secrets may be included on JSON-RPC message objects. This PR removes these properties from the permission activity log, and adds a migration which does the same to existing log objects. We don't interact with the log objects anywhere in our codebase, but we don't want unexpected properties to cause errors in the future should any log objects be retained.

This PR also updates relevant tests and test data. It makes a minor functional change to how a request is designated as a success or failure, but this should not change any behavior in practice.
This commit is contained in:
Erik Marks 2022-04-21 08:44:15 -07:00 committed by GitHub
parent 488d64ae8b
commit cef95f8733
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 341 additions and 104 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

75
.storybook/test-data.js
View File

@ -1248,20 +1248,7 @@ const state = {
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://metamask.io',
request: {
method: 'eth_accounts',
params: [],
jsonrpc: '2.0',
id: 522690215,
origin: 'https://metamask.io',
tabId: 5,
},
requestTime: 1602643170686,
response: {
id: 522690215,
jsonrpc: '2.0',
result: [],
},
responseTime: 1602643170688,
success: true,
},
@ -1270,20 +1257,7 @@ const state = {
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://widget.getacute.io',
request: {
method: 'eth_accounts',
params: [],
jsonrpc: '2.0',
id: 1620464600,
origin: 'https://widget.getacute.io',
tabId: 5,
},
requestTime: 1602643172935,
response: {
id: 1620464600,
jsonrpc: '2.0',
result: [],
},
responseTime: 1602643172935,
success: true,
},
@ -1292,19 +1266,7 @@ const state = {
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
request: {
method: 'eth_accounts',
jsonrpc: '2.0',
id: 4279100021,
origin: 'https://app.uniswap.org',
tabId: 5,
},
requestTime: 1620710669962,
response: {
id: 4279100021,
jsonrpc: '2.0',
result: [],
},
responseTime: 1620710669963,
success: true,
},
@ -1313,19 +1275,7 @@ const state = {
method: 'eth_requestAccounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
request: {
method: 'eth_requestAccounts',
jsonrpc: '2.0',
id: 4279100022,
origin: 'https://app.uniswap.org',
tabId: 5,
},
requestTime: 1620710686872,
response: {
id: 4279100022,
jsonrpc: '2.0',
result: ['0x64a845a5b02460acf8a3d84503b0d68d028b4bb4'],
},
responseTime: 1620710693187,
success: true,
},
@ -1334,19 +1284,7 @@ const state = {
method: 'eth_requestAccounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
request: {
method: 'eth_requestAccounts',
jsonrpc: '2.0',
id: 4279100023,
origin: 'https://app.uniswap.org',
tabId: 5,
},
requestTime: 1620710693204,
response: {
id: 4279100023,
jsonrpc: '2.0',
result: ['0x64a845a5b02460acf8a3d84503b0d68d028b4bb4'],
},
responseTime: 1620710693213,
success: true,
},
@ -1355,20 +1293,7 @@ const state = {
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
request: {
method: 'eth_accounts',
params: [],
jsonrpc: '2.0',
id: 4279100034,
origin: 'https://app.uniswap.org',
tabId: 5,
},
requestTime: 1620710712072,
response: {
id: 4279100034,
jsonrpc: '2.0',
result: ['0x64a845a5b02460acf8a3d84503b0d68d028b4bb4'],
},
responseTime: 1620710712075,
success: true,
},

10
app/scripts/controllers/permissions/permission-log.js
View File

@ -1,5 +1,4 @@
import { ObservableStore } from '@metamask/obs-store';
import stringify from 'fast-safe-stringify';
import { CaveatTypes } from '../../../../shared/constants/permissions';
import {
LOG_IGNORE_METHODS,
@ -158,9 +157,7 @@ export class PermissionLogController {
? LOG_METHOD_TYPES.internal
: LOG_METHOD_TYPES.restricted,
origin: request.origin,
request: stringify(request, null, 2),
requestTime: Date.now(),
response: null,
responseTime: null,
success: null,
};
@ -181,9 +178,12 @@ export class PermissionLogController {
return;
}
entry.response = stringify(response, null, 2);
// The JSON-RPC 2.0 specification defines "success" by the presence of
// either the "result" or "error" property. The specification forbids
// both properties from being present simultaneously, and our JSON-RPC
// stack is spec-compliant at the time of writing.
entry.success = Object.hasOwnProperty.call(response, 'result');
entry.responseTime = time;
entry.success = !response.error;
}
/**

35
app/scripts/controllers/permissions/permission-log.test.js
View File

@ -1,6 +1,5 @@
import nanoid from 'nanoid';
import { useFakeTimers } from 'sinon';
import stringify from 'fast-safe-stringify';
import { constants, getters, noop } from '../../../../test/mocks/permissions';
import { PermissionLogController } from './permission-log';
import { LOG_LIMIT, LOG_METHOD_TYPES } from './enums';
@ -67,7 +66,7 @@ describe('PermissionLogController', () => {
req = RPC_REQUESTS.test_method(SUBJECTS.a.origin);
req.id = REQUEST_IDS.a;
res = { foo: 'bar' };
res = { result: 'bar' };
logMiddleware({ ...req }, res);
@ -143,11 +142,17 @@ describe('PermissionLogController', () => {
false,
);
// validate final state
// Validate final state
expect(entry1).toStrictEqual(log[0]);
expect(entry2).toStrictEqual(log[1]);
expect(entry3).toStrictEqual(log[2]);
expect(entry4).toStrictEqual(log[3]);
// Regression test: ensure "response" and "request" properties
// are not present
log.forEach((entry) =>
expect('request' in entry && 'response' in entry).toBe(false),
);
});
it('handles responses added out of order', () => {
@ -163,15 +168,15 @@ describe('PermissionLogController', () => {
// get make requests
req.id = id1;
const res1 = { foo: id1 };
const res1 = { result: id1 };
logMiddleware({ ...req }, { ...res1 }, getSavedMockNext(handlerArray));
req.id = id2;
const res2 = { foo: id2 };
const res2 = { result: id2 };
logMiddleware({ ...req }, { ...res2 }, getSavedMockNext(handlerArray));
req.id = id3;
const res3 = { foo: id3 };
const res3 = { result: id3 };
logMiddleware({ ...req }, { ...res3 }, getSavedMockNext(handlerArray));
// verify log state
@ -181,10 +186,10 @@ describe('PermissionLogController', () => {
const entry2 = log[1];
const entry3 = log[2];
// all entries should be in correct order, without responses
expect(entry1).toMatchObject({ id: id1, response: null });
expect(entry2).toMatchObject({ id: id2, response: null });
expect(entry3).toMatchObject({ id: id3, response: null });
// all entries should be in correct order
expect(entry1).toMatchObject({ id: id1, responseTime: null });
expect(entry2).toMatchObject({ id: id2, responseTime: null });
expect(entry3).toMatchObject({ id: id3, responseTime: null });
// call response handlers
for (const i of [1, 2, 0]) {
@ -226,7 +231,7 @@ describe('PermissionLogController', () => {
it('handles a lack of response', () => {
let req = RPC_REQUESTS.test_method(SUBJECTS.a.origin);
req.id = REQUEST_IDS.a;
let res = { foo: 'bar' };
let res = { result: 'bar' };
// noop for next handler prevents recording of response
logMiddleware({ ...req }, res, noop);
@ -270,7 +275,7 @@ describe('PermissionLogController', () => {
let log = permLog.getActivityLog();
expect(log).toHaveLength(0);
const res = { foo: 'bar' };
const res = { result: 'bar' };
const req1 = RPC_REQUESTS.metamask_sendDomainMetadata(
SUBJECTS.c.origin,
'foobar',
@ -288,7 +293,7 @@ describe('PermissionLogController', () => {
it('enforces log limit', () => {
const req = RPC_REQUESTS.test_method(SUBJECTS.a.origin);
const res = { foo: 'bar' };
const res = { result: 'bar' };
// max out log
let lastId;
@ -647,19 +652,15 @@ function validateActivityEntry(entry, req, res, methodType, success) {
expect(entry.method).toStrictEqual(req.method);
expect(entry.origin).toStrictEqual(req.origin);
expect(entry.methodType).toStrictEqual(methodType);
expect(entry.request).toStrictEqual(stringify(req, null, 2));
expect(Number.isInteger(entry.requestTime)).toBe(true);
if (res) {
expect(Number.isInteger(entry.responseTime)).toBe(true);
expect(entry.requestTime <= entry.responseTime).toBe(true);
expect(entry.success).toStrictEqual(success);
expect(entry.response).toStrictEqual(stringify(res, null, 2));
} else {
expect(entry.requestTime > 0).toBe(true);
expect(entry).toMatchObject({
response: null,
responseTime: null,
success: null,
});

40
app/scripts/migrations/070.js Normal file
View File

@ -0,0 +1,40 @@
import { cloneDeep } from 'lodash';
const version = 70;
/**
* Removes the `request` and `response` properties from
* `PermissionLogController.permissionActivityLog` objects.
*/
export default {
version,
async migrate(originalVersionedData) {
const versionedData = cloneDeep(originalVersionedData);
versionedData.meta.version = version;
const state = versionedData.data;
const newState = transformState(state);
versionedData.data = newState;
return versionedData;
},
};
function transformState(state) {
if (Array.isArray(state?.PermissionLogController?.permissionActivityLog)) {
const {
PermissionLogController: { permissionActivityLog },
} = state;
// mutate activity log entries in place
permissionActivityLog.forEach((logEntry) => {
if (
logEntry &&
typeof logEntry === 'object' &&
!Array.isArray(logEntry)
) {
delete logEntry.request;
delete logEntry.response;
}
});
}
return state;
}

273
app/scripts/migrations/070.test.js Normal file
View File

@ -0,0 +1,273 @@
import migration70 from './070';
describe('migration #70', () => {
it('should update the version metadata', async () => {
const oldStorage = {
meta: {
version: 69,
},
data: {},
};
const newStorage = await migration70.migrate(oldStorage);
expect(newStorage.meta).toStrictEqual({
version: 70,
});
});
it('should migrate all data', async () => {
const oldStorage = {
meta: {
version: 69,
},
data: {
FooController: { a: 'b' },
PermissionLogController: {
permissionActivityLog: [
{
id: 522690215,
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://metamask.io',
request: {
method: 'eth_accounts',
params: [],
jsonrpc: '2.0',
id: 522690215,
origin: 'https://metamask.io',
tabId: 5,
},
requestTime: 1602643170686,
response: {
id: 522690215,
jsonrpc: '2.0',
result: [],
},
responseTime: 1602643170688,
success: true,
},
{
id: 1620464600,
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://widget.getacute.io',
request: {
method: 'eth_accounts',
params: [],
jsonrpc: '2.0',
id: 1620464600,
origin: 'https://widget.getacute.io',
tabId: 5,
},
requestTime: 1602643172935,
response: {
id: 1620464600,
jsonrpc: '2.0',
result: [],
},
responseTime: 1602643172935,
success: true,
},
{
id: 4279100021,
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
request: {
method: 'eth_accounts',
jsonrpc: '2.0',
id: 4279100021,
origin: 'https://app.uniswap.org',
tabId: 5,
},
requestTime: 1620710669962,
response: {
id: 4279100021,
jsonrpc: '2.0',
result: [],
},
responseTime: 1620710669963,
success: true,
},
{
id: 4279100022,
method: 'eth_requestAccounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
request: {
method: 'eth_requestAccounts',
jsonrpc: '2.0',
id: 4279100022,
origin: 'https://app.uniswap.org',
tabId: 5,
},
requestTime: 1620710686872,
response: {
id: 4279100022,
jsonrpc: '2.0',
result: ['0x64a845a5b02460acf8a3d84503b0d68d028b4bb4'],
},
responseTime: 1620710693187,
success: true,
},
{
id: 4279100023,
method: 'eth_requestAccounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
request: {
method: 'eth_requestAccounts',
jsonrpc: '2.0',
id: 4279100023,
origin: 'https://app.uniswap.org',
tabId: 5,
},
requestTime: 1620710693204,
response: {
id: 4279100023,
jsonrpc: '2.0',
result: ['0x64a845a5b02460acf8a3d84503b0d68d028b4bb4'],
},
responseTime: 1620710693213,
success: true,
},
{
id: 4279100034,
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
request: {
method: 'eth_accounts',
params: [],
jsonrpc: '2.0',
id: 4279100034,
origin: 'https://app.uniswap.org',
tabId: 5,
},
requestTime: 1620710712072,
response: {
id: 4279100034,
jsonrpc: '2.0',
result: ['0x64a845a5b02460acf8a3d84503b0d68d028b4bb4'],
},
responseTime: 1620710712075,
success: true,
},
],
},
},
};
const newStorage = await migration70.migrate(oldStorage);
expect(newStorage).toStrictEqual({
meta: {
version: 70,
},
data: {
FooController: { a: 'b' },
PermissionLogController: {
permissionActivityLog: [
{
id: 522690215,
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://metamask.io',
requestTime: 1602643170686,
responseTime: 1602643170688,
success: true,
},
{
id: 1620464600,
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://widget.getacute.io',
requestTime: 1602643172935,
responseTime: 1602643172935,
success: true,
},
{
id: 4279100021,
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
requestTime: 1620710669962,
responseTime: 1620710669963,
success: true,
},
{
id: 4279100022,
method: 'eth_requestAccounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
requestTime: 1620710686872,
responseTime: 1620710693187,
success: true,
},
{
id: 4279100023,
method: 'eth_requestAccounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
requestTime: 1620710693204,
responseTime: 1620710693213,
success: true,
},
{
id: 4279100034,
method: 'eth_accounts',
methodType: 'restricted',
origin: 'https://app.uniswap.org',
requestTime: 1620710712072,
responseTime: 1620710712075,
success: true,
},
],
},
},
});
});
it('should handle missing PermissionLogController', async () => {
const oldStorage = {
meta: {
version: 69,
},
data: {
FooController: { a: 'b' },
},
};
const newStorage = await migration70.migrate(oldStorage);
expect(newStorage).toStrictEqual({
meta: {
version: 70,
},
data: {
FooController: { a: 'b' },
},
});
});
it('should handle missing PermissionLogController.permissionActivityLog', async () => {
const oldStorage = {
meta: {
version: 69,
},
data: {
FooController: { a: 'b' },
PermissionLogController: {},
},
};
const newStorage = await migration70.migrate(oldStorage);
expect(newStorage).toStrictEqual({
meta: {
version: 70,
},
data: {
FooController: { a: 'b' },
PermissionLogController: {},
},
});
});
});

2
app/scripts/migrations/index.js
View File

@ -73,6 +73,7 @@ import m066 from './066';
import m067 from './067';
import m068 from './068';
import m069 from './069';
import m070 from './070';
const migrations = [
m002,
@ -143,6 +144,7 @@ const migrations = [
m067,
m068,
m069,
m070,
];
export default migrations;

3
lavamoat/browserify/beta/policy.json
View File

@ -585,8 +585,7 @@
"console.log": true,
"document.createElement": true,
"document.head.appendChild": true,
"fetch": true,
"removeEventListener": true
"fetch": true
},
"packages": {
"@ethereumjs/tx": true,

3
lavamoat/browserify/flask/policy.json
View File

@ -585,8 +585,7 @@
"console.log": true,
"document.createElement": true,
"document.head.appendChild": true,
"fetch": true,
"removeEventListener": true
"fetch": true
},
"packages": {
"@ethereumjs/tx": true,

3
lavamoat/browserify/main/policy.json
View File

@ -585,8 +585,7 @@
"console.log": true,
"document.createElement": true,
"document.head.appendChild": true,
"fetch": true,
"removeEventListener": true
"fetch": true
},
"packages": {
"@ethereumjs/tx": true,

1
package.json
View File

@ -174,7 +174,6 @@
"ethjs-query": "^0.3.4",
"extension-port-stream": "^2.0.0",
"fast-json-patch": "^2.2.1",
"fast-safe-stringify": "^2.0.7",
"fuse.js": "^3.2.0",
"globalthis": "^1.0.1",
"human-standard-collectible-abi": "^1.0.2",