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.

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

Jump to bottom

BREAKING: Add JSON-RPC handler permission #905

Merged
merged 9 commits into from Nov 24, 2022
Merged

BREAKING: Add JSON-RPC handler permission #905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f3c9a73
Add JSON-RPC handler permission
Mrtenz Nov 3, 2022
de9b9f0
Check entry point endowment permissions in Snaps controller
Mrtenz Nov 4, 2022
64476db
Fix mock
Mrtenz Nov 4, 2022
95a0090
Check permission origin
Mrtenz Nov 10, 2022
2a4a052
Fix some rebase issues and tests
Mrtenz Nov 23, 2022
5fc8018
Remove package.json resolutions
Mrtenz Nov 23, 2022
e21508d
Fix tests
Mrtenz Nov 23, 2022
bd9635a
Add more tests
Mrtenz Nov 23, 2022
3d069ae
Fix review comments
Mrtenz Nov 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/snaps-controllers/jest.config.js
View file
Open in desktop
Expand Up @@ -7,8 +7,8 @@ module.exports = deepmerge(baseConfig, {
global: {
branches: 88.74,
functions: 97.6,
lines: 96.74,
statements: 96.74,
lines: 97.1,
statements: 97.1,
},
},
projects: [
Expand Down
1 change: 1 addition & 0 deletions packages/snaps-controllers/package.json
View file
Open in desktop
Expand Up @@ -41,6 +41,7 @@
"@metamask/snaps-execution-environments": "^0.24.1",
"@metamask/snaps-types": "^0.24.1",
"@metamask/snaps-utils": "^0.24.1",
"@metamask/subject-metadata-controller": "^1.0.0",
"@metamask/utils": "^3.3.1",
"@xstate/fsm": "^2.0.0",
"concat-stream": "^2.0.0",
Expand Down
14 changes: 7 additions & 7 deletions packages/snaps-controllers/src/multichain/MultiChainController.test.ts
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ describe('MultiChainController', () => {
},
},
});
expect(rootMessenger.call).toHaveBeenCalledTimes(11);
expect(rootMessenger.call).toHaveBeenCalledTimes(12);

snapController.destroy();
await executionService.terminateAllSnaps();
Expand Down Expand Up @@ -99,7 +99,7 @@ describe('MultiChainController', () => {
MOCK_SNAP_ID,
);

expect(rootMessenger.call).toHaveBeenCalledTimes(21);
expect(rootMessenger.call).toHaveBeenCalledTimes(23);

snapController.destroy();
await executionService.terminateAllSnaps();
Expand Down Expand Up @@ -153,7 +153,7 @@ describe('MultiChainController', () => {
},
});

expect(rootMessenger.call).toHaveBeenCalledTimes(9);
expect(rootMessenger.call).toHaveBeenCalledTimes(10);

snapController.destroy();
await executionService.terminateAllSnaps();
Expand Down Expand Up @@ -255,9 +255,9 @@ describe('MultiChainController', () => {
},
});

expect(rootMessenger.call).toHaveBeenCalledTimes(17);
expect(rootMessenger.call).toHaveBeenCalledTimes(19);
expect(rootMessenger.call).toHaveBeenNthCalledWith(
15,
17,
'ApprovalController:addRequest',
{
id: expect.any(String),
Expand Down Expand Up @@ -326,7 +326,7 @@ describe('MultiChainController', () => {
).rejects.toThrow(
'No installed snaps found for any requested namespace.',
);
expect(rootMessenger.call).toHaveBeenCalledTimes(9);
expect(rootMessenger.call).toHaveBeenCalledTimes(10);

snapController.destroy();
await executionService.terminateAllSnaps();
Expand Down Expand Up @@ -359,7 +359,7 @@ describe('MultiChainController', () => {
});

expect(result).toEqual(['eip155:1:foo']);
expect(rootMessenger.call).toHaveBeenCalledTimes(15);
expect(rootMessenger.call).toHaveBeenCalledTimes(17);

snapController.destroy();
await executionService.terminateAllSnaps();
Expand Down
194 changes: 159 additions & 35 deletions packages/snaps-controllers/src/snaps/SnapController.test.ts
View file
Open in desktop
Expand Up @@ -45,12 +45,16 @@ import {
getSnapControllerWithEES,
getSnapControllerWithEESOptions,
MOCK_BLOCK_NUMBER,
MOCK_DAPP_SUBJECT_METADATA,
MOCK_DAPPS_RPC_ORIGINS_PERMISSION,
MOCK_NAMESPACES,
MOCK_RPC_ORIGINS_PERMISSION,
MOCK_SNAP_SUBJECT_METADATA,
PERSISTED_MOCK_KEYRING_SNAP,
sleep,
} from '../test-utils';
import { delay } from '../utils';
import { SnapEndowments } from './endowments';
import { handlerEndowments, SnapEndowments } from './endowments';
import { SnapControllerState, SNAP_APPROVAL_UPDATE } from './SnapController';

const { subtle } = new Crypto();
Expand Down Expand Up @@ -433,8 +437,8 @@ describe('SnapController', () => {

rootMessenger.registerActionHandler(
'PermissionController:hasPermission',
() => {
return false;
(_origin, permission) => {
return permission === SnapEndowments.Rpc;
},
);

Expand Down Expand Up @@ -878,7 +882,9 @@ describe('SnapController', () => {

rootMessenger.registerActionHandler(
'PermissionController:hasPermission',
() => false,
(_origin, permission) => {
return permission === SnapEndowments.Rpc;
},
);

await snapController.startSnap(snap.id);
Expand Down Expand Up @@ -1286,7 +1292,9 @@ describe('SnapController', () => {

rootMessenger.registerActionHandler(
'PermissionController:hasPermission',
() => false,
(_origin, permission) => {
return permission === SnapEndowments.Rpc;
},
);

await snapController.startSnap(snap.id);
Expand Down Expand Up @@ -1315,34 +1323,143 @@ describe('SnapController', () => {
await service.terminateAllSnaps();
});

describe('handleRequest', () => {
it.each(Object.keys(handlerEndowments) as HandlerType[])(
'throws if the snap does not have permission for the handler',
async (handler) => {
const rootMessenger = getControllerMessenger();
const messenger = getSnapControllerMessenger(rootMessenger);
const snapController = getSnapController(
getSnapControllerOptions({
messenger,
state: {
snaps: getPersistedSnapsState(),
},
}),
);

rootMessenger.registerActionHandler(
'PermissionController:hasPermission',
() => false,
);

const snap = snapController.getExpect(MOCK_SNAP_ID);
await expect(
snapController.handleRequest({
snapId: snap.id,
origin: 'foo.com',
handler,
request: { jsonrpc: '2.0', method: 'test' },
}),
).rejects.toThrow(
`Snap "${snap.id}" is not permitted to use "${handlerEndowments[handler]}".`,
);

snapController.destroy();
},
);

it('throws if the snap does not have permission to handle JSON-RPC requests from dapps', async () => {
const rootMessenger = getControllerMessenger();
const messenger = getSnapControllerMessenger(rootMessenger);
const snapController = getSnapController(
getSnapControllerOptions({
messenger,
state: {
snaps: getPersistedSnapsState(),
},
}),
);

rootMessenger.registerActionHandler(
'PermissionController:getPermissions',
() => ({
// Permission to receive JSON-RPC requests from other Snaps.
[SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION,
}),
);

rootMessenger.registerActionHandler(
'SubjectMetadataController:getSubjectMetadata',
() => MOCK_DAPP_SUBJECT_METADATA,
);

const snap = snapController.getExpect(MOCK_SNAP_ID);
await expect(
snapController.handleRequest({
snapId: snap.id,
origin: MOCK_ORIGIN,
handler: HandlerType.OnRpcRequest,
request: { jsonrpc: '2.0', method: 'test' },
}),
).rejects.toThrow(
`Snap "${snap.id}" is not permitted to handle JSON-RPC requests from "${MOCK_ORIGIN}".`,
);

snapController.destroy();
});

it('throws if the snap does not have permission to handle JSON-RPC requests from snaps', async () => {
const rootMessenger = getControllerMessenger();
const messenger = getSnapControllerMessenger(rootMessenger);
const snapController = getSnapController(
getSnapControllerOptions({
messenger,
state: {
snaps: getPersistedSnapsState(),
},
}),
);

rootMessenger.registerActionHandler(
'PermissionController:getPermissions',
() => ({
// Permission to receive JSON-RPC requests from dapps.
[SnapEndowments.Rpc]: MOCK_DAPPS_RPC_ORIGINS_PERMISSION,
}),
);

rootMessenger.registerActionHandler(
'SubjectMetadataController:getSubjectMetadata',
() => MOCK_SNAP_SUBJECT_METADATA,
);

const snap = snapController.getExpect(MOCK_SNAP_ID);
await expect(
snapController.handleRequest({
snapId: snap.id,
origin: MOCK_SNAP_ID,
handler: HandlerType.OnRpcRequest,
request: { jsonrpc: '2.0', method: 'test' },
}),
).rejects.toThrow(
`Snap "${snap.id}" is not permitted to handle JSON-RPC requests from "${MOCK_SNAP_ID}".`,
);

snapController.destroy();
});
});

describe('getRpcRequestHandler', () => {
it('handlers populate the "jsonrpc" property if missing', async () => {
const snapId = 'fooSnap';
const rootMessenger = getControllerMessenger();
const options = getSnapControllerWithEESOptions({
rootMessenger,
state: {
snaps: {
[snapId]: {
enabled: true,
id: snapId,
status: SnapStatus.Running,
} as any,
},
snaps: getPersistedSnapsState(),
},
});
const [snapController, service] = getSnapControllerWithEES(options);

const mockMessageHandler = jest.fn();
const spyOnMessengerCall = jest
.spyOn(options.messenger, 'call')
.mockImplementation((method, ..._args: unknown[]) => {
if (method === 'ExecutionService:handleRpcRequest') {
return mockMessageHandler as any;
}
return true;
});
rootMessenger.registerActionHandler(
'PermissionController:hasPermission',
(_origin, permission) => {
return permission === SnapEndowments.Rpc;
},
);

await snapController.handleRequest({
snapId,
snapId: MOCK_SNAP_ID,
origin: 'foo.com',
handler: HandlerType.OnRpcRequest,
request: {
Expand All @@ -1353,10 +1470,10 @@ describe('SnapController', () => {
},
});

expect(spyOnMessengerCall).toHaveBeenCalledTimes(2);
expect(spyOnMessengerCall).toHaveBeenCalledWith(
expect(rootMessenger.call).toHaveBeenCalledTimes(7);
expect(rootMessenger.call).toHaveBeenCalledWith(
'ExecutionService:handleRpcRequest',
snapId,
MOCK_SNAP_ID,
{
origin: 'foo.com',
handler: HandlerType.OnRpcRequest,
Expand Down Expand Up @@ -1403,7 +1520,8 @@ describe('SnapController', () => {
});

it('handlers will throw if there are too many pending requests before a snap has started', async () => {
const messenger = getSnapControllerMessenger();
const rootMessenger = getControllerMessenger();
const messenger = getSnapControllerMessenger(rootMessenger);
const fakeSnap = getPersistedSnapObject({ status: SnapStatus.Stopped });
const snapId = fakeSnap.id;
const snapController = getSnapController(
Expand All @@ -1422,14 +1540,10 @@ describe('SnapController', () => {
resolveExecutePromise = res;
});

jest
.spyOn(messenger, 'call')
.mockImplementation((method, ..._args: unknown[]) => {
if (method === 'ExecutionService:executeSnap') {
return deferredExecutePromise;
}
return true;
});
rootMessenger.registerActionHandler(
'ExecutionService:executeSnap',
async () => deferredExecutePromise,
);

// Fill up the request queue
const finishPromise = Promise.all([
Expand Down Expand Up @@ -2073,6 +2187,11 @@ describe('SnapController', () => {
}),
);

rootMessenger.registerActionHandler(
'PermissionController:getPermissions',
() => ({}),
);

jest
.spyOn(snapController as any, 'fetchSnap')
.mockImplementationOnce(() => {
Expand Down Expand Up @@ -2155,6 +2274,11 @@ describe('SnapController', () => {
getSnapControllerOptions({ messenger }),
);

rootMessenger.registerActionHandler(
'PermissionController:getPermissions',
() => ({}),
);

const fetchSnapMock = jest
.spyOn(controller as any, 'fetchSnap')
.mockImplementationOnce(async () =>
Expand Down