Skip to content
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
4 changes: 4 additions & 0 deletions packages/eth-json-rpc-middleware/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- **BREAKING:** Add strict validation for `eth_sendTransaction` and `eth_signTransaction` params ([#9482](https://github.com/MetaMask/core/pull/9482))
- Reject requests whose params do not match the transaction schema (extraneous top-level keys, ill-typed fields such as non-hex `to`/`data`, malformed `accessList` / `authorizationList` entries) or exceed `MAX_TRANSACTION_PARAMS_SIZE_BYTES` when serialized
- Prevents downstream normalization / PPOM WASM from crashing on deeply-nested junk fields or padded payloads and silently bypassing security scans

- Bump `@metamask/utils` from `^11.9.0` to `^11.11.0` ([#9074](https://github.com/MetaMask/core/pull/9074))
- Bump `@metamask/json-rpc-engine` from `^10.2.4` to `^10.5.0` ([#8661](https://github.com/MetaMask/core/pull/8661), [#8746](https://github.com/MetaMask/core/pull/8746), [#8753](https://github.com/MetaMask/core/pull/8753))
- Bump `@metamask/message-manager` from `^14.1.1` to `^14.1.2` ([#8755](https://github.com/MetaMask/core/pull/8755))
Expand Down
1 change: 1 addition & 0 deletions packages/eth-json-rpc-middleware/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ describe('index module', () => {
"createWalletMiddleware": [Function],
"providerAsMiddleware": [Function],
"providerAsMiddlewareV2": [Function],
"validateTransactionParams": [Function],
}
`);
});
Expand Down
1 change: 1 addition & 0 deletions packages/eth-json-rpc-middleware/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,6 @@ export {
GetSupportedExecutionPermissionsResultStruct,
} from './methods/wallet-get-supported-execution-permissions.js';
export * from './providerAsMiddleware.js';
export { validateTransactionParams } from './utils/validation.js';
export * from './retryOnEmpty.js';
export * from './wallet.js';
183 changes: 183 additions & 0 deletions packages/eth-json-rpc-middleware/src/utils/validation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ import { any, validate } from '@metamask/superstruct';

import type { WalletMiddlewareKeyValues } from '../wallet.js';
import {
MAX_TRANSACTION_PARAMS_SIZE_BYTES,
resemblesAddress,
validateAndNormalizeKeyholder,
validateParams,
validateTransactionParams,
validateTypedMessageKeys,
} from './validation.js';

Expand Down Expand Up @@ -278,4 +280,185 @@ describe('Validation Utils', () => {
});
});
});

describe('validateTransactionParams', () => {
const VALID_FROM = '0xbe93f9bacbcffc8ee6663f2647917ed7a20a57bb';
const VALID_TO = '0xdac17f958d2ee523a2206206994597c13d831ec7';

beforeEach(() => {
const actual = jest.requireActual<{
Comment thread
mcmire marked this conversation as resolved.
validate: typeof validate;
}>('@metamask/superstruct');
validateMock.mockImplementation(actual.validate);
});

it('does not throw for minimal valid params', () => {
expect(() =>
validateTransactionParams({ from: VALID_FROM }),
).not.toThrow();
});

it('does not throw for the full valid param set', () => {
expect(() =>
validateTransactionParams({
accessList: [
{
address: VALID_TO,
storageKeys: ['0x00', '0x01'],
},
],
authorizationList: [
{
chainId: '0x1',
address: VALID_TO,
nonce: '0x0',
r: '0x0',
s: '0x0',
yParity: '0x0',
},
],
chainId: '0x1',
data: '0x095ea7b3',
from: VALID_FROM,
gas: '0x5208',
gasLimit: '0x5208',
gasPrice: '0x1',
maxFeePerGas: '0x2',
maxPriorityFeePerGas: '0x1',
nonce: '0x0',
to: VALID_TO,
type: '0x2',
value: '0x0',
}),
).not.toThrow();
});

it.each([
['null', null],
['undefined', undefined],
['a string', 'not-an-object'],
['a number', 42],
['a boolean', true],
['an array', [{ from: VALID_FROM }]],
])('throws when params is %s', (_label, value) => {
expect(() => validateTransactionParams(value)).toThrow(/Invalid params/u);
});

it('throws for an extraneous top-level key', () => {
expect(() =>
validateTransactionParams({
from: VALID_FROM,
to: VALID_TO,
extraKey: 'unexpected',
}),
).toThrow(/Invalid params/u);
});

it('throws when params contain an extraneous key with a deeply-nested value', () => {
let junk: Record<string, unknown> = {};
for (let i = 0; i < 1200; i++) {
junk = { b: junk };
}

expect(() =>
validateTransactionParams({
from: VALID_FROM,
to: VALID_TO,
value: '0x0',
data: '0x095ea7b3',
test: junk,
}),
).toThrow(/Invalid params/u);
});

it('runs the size check before schema validation', () => {
const stringifySpy = jest.spyOn(JSON, 'stringify');

try {
expect(() =>
validateTransactionParams({
from: VALID_FROM,
to: VALID_TO,
extraKey: 'unexpected',
}),
).toThrow(/Invalid params/u);

expect(stringifySpy).toHaveBeenCalled();
} finally {
stringifySpy.mockRestore();
}
});

it('throws when a typed field has the wrong type', () => {
expect(() =>
validateTransactionParams({
from: VALID_FROM,
to: { nested: 'not-an-address' },
}),
).toThrow(/Invalid params/u);
});

it('throws when `data` is not a hex string', () => {
expect(() =>
validateTransactionParams({
from: VALID_FROM,
data: 1234 as unknown as string,
}),
).toThrow(/Invalid params/u);
});

it('throws when `accessList` entries are malformed', () => {
expect(() =>
validateTransactionParams({
from: VALID_FROM,
accessList: [{ address: 'not-hex', storageKeys: 'not-an-array' }],
}),
).toThrow(/Invalid params/u);
});

it('throws for a data-padding attack that passes the schema', () => {
const padded = `0x${'00'.repeat(MAX_TRANSACTION_PARAMS_SIZE_BYTES)}`;

expect(() =>
validateTransactionParams({
from: VALID_FROM,
to: VALID_TO,
data: padded,
}),
).toThrow('Request too large');
});

it('throws for an accessList-padding attack that passes the schema', () => {
const padded = Array.from(
{ length: Math.ceil(MAX_TRANSACTION_PARAMS_SIZE_BYTES / 64) },
() => ({
address: VALID_TO,
storageKeys: [`0x${'00'.repeat(32)}`],
}),
);

expect(() =>
validateTransactionParams({
from: VALID_FROM,
to: VALID_TO,
accessList: padded,
}),
).toThrow('Request too large');
});

it('does not throw for a legitimate multi-entry accessList well under the size limit', () => {
const entries = Array.from({ length: 16 }, () => ({
address: VALID_TO,
storageKeys: [`0x${'11'.repeat(32)}`, `0x${'22'.repeat(32)}`],
}));

expect(() =>
validateTransactionParams({
from: VALID_FROM,
to: VALID_TO,
accessList: entries,
}),
).not.toThrow();
});
});
});
85 changes: 84 additions & 1 deletion packages/eth-json-rpc-middleware/src/utils/validation.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
import { TYPED_MESSAGE_SCHEMA } from '@metamask/eth-sig-util';
import { providerErrors, rpcErrors } from '@metamask/rpc-errors';
import type { Struct, StructError } from '@metamask/superstruct';
import { validate } from '@metamask/superstruct';
import {
array,
number,
object,
optional,
string,
union,
validate,
} from '@metamask/superstruct';
import type { Hex } from '@metamask/utils';

import type { WalletMiddlewareContext } from '../wallet.js';
Expand Down Expand Up @@ -234,3 +242,78 @@ export function validateTypedMessageKeys(data: string): void {
}
}
}

// Numerical fields accept both hex strings and numbers, as some dapps send
// numbers and `TransactionController` normalizes them downstream.
const QuantityStruct = union([string(), number()]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❤️


export const TransactionParamsStruct = object({
accessList: optional(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm sure we'll hit issues with some dApps as we can't predict all inputs, but we're long overdue schema validation here, so this should be a safe start 👍

array(object({ address: string(), storageKeys: array(string()) })),
),
authorizationList: optional(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor, we don't allow external authorization lists for security, but that will be caught in TransactionController for now.

array(
object({
address: string(),
chainId: optional(string()),
nonce: optional(string()),
r: optional(string()),
s: optional(string()),
yParity: optional(string()),
}),
),
Comment thread
cursor[bot] marked this conversation as resolved.
),
chainId: optional(string()),
data: optional(string()),
from: string(),
gas: optional(QuantityStruct),
gasLimit: optional(QuantityStruct),
gasPrice: optional(QuantityStruct),
maxFeePerGas: optional(QuantityStruct),
maxPriorityFeePerGas: optional(QuantityStruct),
nonce: optional(QuantityStruct),
to: optional(string()),
type: optional(string()),
value: optional(QuantityStruct),
});
Comment thread
cursor[bot] marked this conversation as resolved.

// Upper bound derived from the largest valid eth_sendTransaction payload:
// EIP-3860 caps initcode at 49,152 bytes → hex-encoded in 'data' field ≈ 98 KB of JSON.
// 200 KB is ~2× that ceiling, giving clear headroom above any protocol-legal
// transaction while blocking the padding attacks this cap defends against.
// TODO(CONF-1662): tighten once P99 production data is available.
export const MAX_TRANSACTION_PARAMS_SIZE_BYTES = 200 * 1024;

/**
* Validates `eth_sendTransaction` / `eth_signTransaction` params against the
* standard transaction schema and rejects payloads whose serialized size
* exceeds `MAX_TRANSACTION_PARAMS_SIZE_BYTES`.
*
* Guards against two attack shapes:
* - Size: valid-shaped but oversized payloads (e.g. `data` padded with
* millions of hex zeros) that exhaust memory in downstream code. Checked
* first via `JSON.stringify` so oversized input is rejected before schema
* work.
* - Structural: extraneous top-level keys or ill-typed fields (e.g.
* `{ from, to, test: { b: { b: ... × 1200 } } }`) that would crash
* downstream normalization / PPOM WASM with `RangeError: Maximum call
* stack size exceeded`, silently bypassing security checks. Superstruct's
* `object()` rejects unknown keys by name without accessing their values,
* so hostile nested subtrees are never traversed by schema validation.
*
* @param params - The transaction params object supplied by the dapp.
* @throws rpcErrors.invalidParams() if params is an array or exceeds the
* serialized size limit.
* @throws rpcErrors.invalidInput() if params fails schema validation
* (wrong type, extraneous top-level key, or malformed nested field).
*/
export function validateTransactionParams(params: unknown): void {
if (
new TextEncoder().encode(JSON.stringify(params)).byteLength >
MAX_TRANSACTION_PARAMS_SIZE_BYTES
) {
throw rpcErrors.invalidParams('Request too large');
}

validateParams(params, TransactionParamsStruct);
}
Loading