Skip to content

fix: add logic to properly rate limit snap install requests #5824

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
1 change: 0 additions & 1 deletion eslint-warning-thresholds.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
"jsdoc/check-tag-names": 13
},
"packages/approval-controller/src/ApprovalController.test.ts": {
"import-x/order": 1,
"jest/no-conditional-in-test": 16
},
"packages/approval-controller/src/ApprovalController.ts": {
Expand Down
4 changes: 4 additions & 0 deletions packages/approval-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Bump `@metamask/base-controller` from ^8.0.0 to ^8.0.1 ([#5722](https://github.com/MetaMask/core/pull/5722))

### Fixed

- Add logic to properly rate limit snap install flow requests ([#5824](https://github.com/MetaMask/core/pull/5824))

## [7.1.3]

### Changed
Expand Down
96 changes: 95 additions & 1 deletion packages/approval-controller/src/ApprovalController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { Messenger } from '@metamask/base-controller';
import { errorCodes, JsonRpcError } from '@metamask/rpc-errors';
import { nanoid } from 'nanoid';

import { flushPromises } from '../../../tests/helpers';
import type {
AddApprovalOptions,
ApprovalControllerActions,
Expand All @@ -25,6 +24,7 @@ import {
MissingApprovalFlowError,
NoApprovalFlowsError,
} from './errors';
import { flushPromises } from '../../../tests/helpers';

jest.mock('nanoid');

Expand Down Expand Up @@ -310,6 +310,100 @@ describe('approval controller', () => {
).toThrow(getInvalidRequestStateError());
});

it.each([
[
{
id: 'foo',
origin: 'bar.baz',
type: 'wallet_requestPermissions',
requestData: {
permissions: {
wallet_snap: { 'npm:@metamask/example-snap': {} },
},
},
},
{
id: 'bar',
origin: 'bar.baz',
type: 'wallet_requestPermissions',
requestData: {
permissions: {
wallet_snap: { 'npm:@metamask/foo-snap': {} },
},
},
},
],
[
{
id: 'foo',
origin: 'bar.baz',
type: 'wallet_installSnap',
requestData: {},
},
{
id: 'bar',
origin: 'bar.baz',
type: 'wallet_requestPermissions',
requestData: {
permissions: {
wallet_snap: { 'npm:@metamask/foo-snap': {} },
},
},
},
],
[
{
id: 'foo',
origin: 'bar.baz',
type: 'wallet_requestPermissions',
requestData: {
permissions: {
wallet_snap: { 'npm:@metamask/example-snap': {} },
},
},
},
{
id: 'bar',
origin: 'bar.baz',
type: 'wallet_updateSnap',
requestData: {},
},
],
[
{
id: 'foo',
origin: 'bar.baz',
type: 'wallet_installSnapResult',
requestData: {},
},
{
id: 'bar',
origin: 'bar.baz',
type: 'wallet_requestPermissions',
requestData: {
permissions: {
wallet_snap: { 'npm:@metamask/example-snap': {} },
},
},
},
],
])(
'throws on snap install flow collisions',
(firstRequest, secondRequest) => {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
approvalController.add(firstRequest);

// Second request should throw
const expectedError =
'Snap installation flow already pending for origin bar.baz. Please wait.';

expect(() => {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
approvalController.add(secondRequest);
}).toThrow(expectedError);
},
);

it('adds correctly specified entry', () => {
expect(() =>
approvalController.add({
Expand Down
47 changes: 47 additions & 0 deletions packages/approval-controller/src/ApprovalController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,15 @@ const stateMetadata = {
approvalFlows: { persist: false, anonymous: false },
};

const SnapApprovalTypes = new Set([
'wallet_installSnap',
'wallet_updateSnap',
'wallet_installSnapResult',
]);

const getAlreadyPendingSnapInstallFlowMessage = (origin: string) =>
`Snap installation flow already pending for origin ${origin}. Please wait.`;

const getAlreadyPendingMessage = (origin: string, type: string) =>
`Request of type '${type}' already pending for origin ${origin}. Please wait.`;

Expand Down Expand Up @@ -916,6 +925,8 @@ export class ApprovalController extends BaseController<
): Promise<unknown | AddResult> {
this.#validateAddParams(id, origin, type, requestData, requestState);

this.#checkForSnapInstallFlow(type, origin, requestData || {});

if (
!this.#typesExcludedFromRateLimiting.includes(type) &&
this.has({ origin, type })
Expand Down Expand Up @@ -1108,6 +1119,42 @@ export class ApprovalController extends BaseController<
}
}
}

#checkForSnapInstallFlow(
type: string,
origin: string,
requestData: Record<string, Json>,
) {
const pendingApprovals = Object.values(this.state.pendingApprovals);
const isSnapApproval = SnapApprovalTypes.has(type);
const isSnapConnectApproval =
type === 'wallet_requestPermissions' &&
Boolean(
(requestData?.permissions as Record<string, unknown>)?.wallet_snap,
);

if (!(isSnapApproval || isSnapConnectApproval)) {
return;
}

const hasSomeSnapInstallFlows = pendingApprovals.some((approval) => {
const isSnapApprovalType = SnapApprovalTypes.has(approval.type);
const isSnapConnectApprovalType = Boolean(
(approval.requestData?.permissions as Record<string, unknown>)
?.wallet_snap,
);
return (
approval.origin === origin &&
(isSnapApprovalType || isSnapConnectApprovalType)
);
});

if (hasSomeSnapInstallFlows) {
throw rpcErrors.resourceUnavailable(
getAlreadyPendingSnapInstallFlowMessage(origin),
);
}
}
}

export default ApprovalController;
4 changes: 4 additions & 0 deletions packages/controller-utils/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Add snap approval types for use in the client ([#5824](https://github.com/MetaMask/core/pull/5824))

## [11.9.0]

### Added
Expand Down
3 changes: 3 additions & 0 deletions packages/controller-utils/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@ export enum ApprovalType {
WalletConnect = 'wallet_connect',
WalletRequestPermissions = 'wallet_requestPermissions',
WatchAsset = 'wallet_watchAsset',
WalletInstallSnap = 'wallet_installSnap',
WalletUpdateSnap = 'wallet_updateSnap',
WalletInstallSnapResult = 'wallet_installSnapResult',
}

/**
Expand Down
Loading