Skip to content

Enforce JSON-RPC response size limits #2201

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

Merged
merged 4 commits into from
Feb 21, 2024
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
8 changes: 4 additions & 4 deletions packages/snaps-execution-environments/coverage.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"branches": 81.04,
"functions": 90.13,
"lines": 90.73,
"statements": 90.11
"branches": 80.66,
"functions": 90.19,
"lines": 90.83,
"statements": 90.2
}
Original file line number Diff line number Diff line change
Expand Up @@ -2007,6 +2007,44 @@ describe('BaseSnapExecutor', () => {
});
});

it('throws when trying to respond with value that is too large', async () => {
const CODE = `
module.exports.onRpcRequest = () => '1'.repeat(100_000_000);
`;

const executor = new TestSnapExecutor();
await executor.executeSnap(1, MOCK_SNAP_ID, CODE, []);

expect(await executor.readCommand()).toStrictEqual({
jsonrpc: '2.0',
id: 1,
result: 'OK',
});

await executor.writeCommand({
jsonrpc: '2.0',
id: 2,
method: 'snapRpc',
params: [
MOCK_SNAP_ID,
HandlerType.OnRpcRequest,
MOCK_ORIGIN,
{ jsonrpc: '2.0', method: '', params: [] },
],
});

expect(await executor.readCommand()).toStrictEqual({
jsonrpc: '2.0',
id: 2,
error: {
code: -32603,
message:
'JSON-RPC responses must be JSON serializable objects smaller than 64 MB.',
stack: expect.any(String),
},
});
});

it('throws when receiving an invalid RPC request', async () => {
const executor = new TestSnapExecutor();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ import type {
Json,
} from '@metamask/utils';
import {
isObject,
isValidJson,
assert,
isJsonRpcRequest,
hasProperty,
Expand All @@ -49,6 +47,7 @@ import {
sanitizeRequestArguments,
proxyStreamProvider,
withTeardown,
isValidResponse,
} from './utils';
import {
ExecuteSnapRequestArgumentsStruct,
Expand Down Expand Up @@ -301,27 +300,27 @@ export class BaseSnapExecutor {
});
}

async #notify(requestObject: Omit<JsonRpcNotification, 'jsonrpc'>) {
if (!isValidJson(requestObject) || !isObject(requestObject)) {
async #notify(notification: Omit<JsonRpcNotification, 'jsonrpc'>) {
if (!isValidResponse(notification)) {
throw rpcErrors.internal(
'JSON-RPC notifications must be JSON serializable objects',
'JSON-RPC notifications must be JSON serializable objects smaller than 64 MB.',
);
}

await this.#write({
...requestObject,
...notification,
jsonrpc: '2.0',
});
}

async #respond(id: JsonRpcId, requestObject: Record<string, unknown>) {
if (!isValidJson(requestObject) || !isObject(requestObject)) {
async #respond(id: JsonRpcId, response: Record<string, unknown>) {
if (!isValidResponse(response)) {
// Instead of throwing, we directly respond with an error.
// This prevents an issue where we wouldn't respond when errors were non-serializable
await this.#write({
error: serializeError(
rpcErrors.internal(
'JSON-RPC responses must be JSON serializable objects.',
'JSON-RPC responses must be JSON serializable objects smaller than 64 MB.',
),
),
id,
Expand All @@ -331,7 +330,7 @@ export class BaseSnapExecutor {
}

await this.#write({
...requestObject,
...response,
id,
jsonrpc: '2.0',
});
Expand Down
20 changes: 20 additions & 0 deletions packages/snaps-execution-environments/src/common/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
BLOCKED_RPC_METHODS,
assertEthereumOutboundRequest,
assertSnapOutboundRequest,
isValidResponse,
} from './utils';

describe('assertSnapOutboundRequest', () => {
Expand Down Expand Up @@ -77,3 +78,22 @@ describe('assertEthereumOutboundRequest', () => {
);
});
});

describe('isValidResponse', () => {
it('returns false if the value is not an object', () => {
// @ts-expect-error Intentionally using bad params
expect(isValidResponse('foo')).toBe(false);
});

it('returns false if the value is not JSON serializable', () => {
expect(isValidResponse({ foo: BigInt(0) })).toBe(false);
});

it('returns false if the value is too large', () => {
expect(isValidResponse({ foo: '1'.repeat(100_000_000) })).toBe(false);
});

it('returns true if the value is a valid JSON object', () => {
expect(isValidResponse({ foo: 'bar' })).toBe(true);
});
});
34 changes: 33 additions & 1 deletion packages/snaps-execution-environments/src/common/utils.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,21 @@
import type { StreamProvider } from '@metamask/providers';
import type { RequestArguments } from '@metamask/providers/dist/BaseProvider';
import { rpcErrors } from '@metamask/rpc-errors';
import { assert, assertStruct, getSafeJson, JsonStruct } from '@metamask/utils';
import {
assert,
assertStruct,
getJsonSize,
getSafeJson,
isObject,
JsonStruct,
} from '@metamask/utils';

import { log } from '../logging';

// 64 MB - we chose this number because it is the size limit for postMessage
// between the extension and the dapp enforced by Chrome.
const MAX_RESPONSE_JSON_SIZE = 64_000_000;

/**
* Make proxy for Promise and handle the teardown process properly.
* If the teardown is called in the meanwhile, Promise result will not be
Expand Down Expand Up @@ -174,3 +186,23 @@ export function sanitizeRequestArguments(value: unknown): RequestArguments {
const json = JSON.parse(JSON.stringify(value));
return getSafeJson(json) as RequestArguments;
}

/**
* Check if the input is a valid response.
*
* @param response - The response.
* @returns True if the response is valid, otherwise false.
*/
export function isValidResponse(response: Record<string, unknown>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a test for this function, but codecov is saying these lines are uncovered? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, utils.ts has better coverage in the browser tests and therefore that is chosen when merging the two coverage reports. Not much we can do about this right now 🤷‍♂️

if (!isObject(response)) {
return false;
}

try {
// If the JSON is invalid this will throw and we should return false.
const size = getJsonSize(response);
return size < MAX_RESPONSE_JSON_SIZE;
} catch {
return false;
}
}