From cf62d8085b64a3994512d9f7de76edfad21333fd Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 29 Jun 2026 12:42:10 +0200 Subject: [PATCH 1/6] feat(wallet)!: wire `PermissionController` and `SubjectMetadataController` into default initialization Wire `PermissionController` and `SubjectMetadataController` (both from `@metamask/permission-controller`) into the default wallet initialization. The two reference each other's messenger actions and are wired as a pair; `PermissionController` also consumes the already-wired `ApprovalController` request actions. Client-varying values are injectable `instanceOptions` with platform-agnostic defaults: `permissionController.{caveatSpecifications,permissionSpecifications, unrestrictedMethods}` (each defaulting to an empty set) and `subjectMetadataController.subjectCacheLimit` (defaulting to 100). Co-Authored-By: Claude Opus 4.8 (1M context) --- codeowners.ts | 29 ++-- packages/wallet/CHANGELOG.md | 5 + packages/wallet/package.json | 1 + .../src/initialization/instances/index.ts | 2 + .../permission-controller.test.ts | 88 +++++++++++ .../permission-controller.ts | 53 +++++++ .../instances/permission-controller/types.ts | 35 +++++ .../subject-metadata-controller.test.ts | 145 ++++++++++++++++++ .../subject-metadata-controller.ts | 48 ++++++ .../subject-metadata-controller/types.ts | 16 ++ packages/wallet/src/types.ts | 4 + 11 files changed, 412 insertions(+), 14 deletions(-) create mode 100644 packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts create mode 100644 packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts create mode 100644 packages/wallet/src/initialization/instances/permission-controller/types.ts create mode 100644 packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.test.ts create mode 100644 packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts create mode 100644 packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts diff --git a/codeowners.ts b/codeowners.ts index db72782dc89..073918b0302 100644 --- a/codeowners.ts +++ b/codeowners.ts @@ -14,12 +14,13 @@ type PackageInfo = { teams: string[]; /** - * The package's directory name under - * `/packages/wallet/src/initialization/instances`, used to generate its rule - * in the "Initialization" section. Omit this if the package has not been - * added to the Wallet Library yet. + * The package's directory name(s) under + * `/packages/wallet/src/initialization/instances`, used to generate its rule(s) + * in the "Initialization" section. Pass an array for packages that export more + * than one wired controller. Omit this if the package has not been added to the + * Wallet Library yet. */ - initializationPath?: string; + initializationPath?: string | string[]; }; /** @@ -256,6 +257,7 @@ const PACKAGES: Record = { }, 'permission-controller': { teams: ['@MetaMask/core-platform'], + initializationPath: ['permission-controller', 'subject-metadata-controller'], }, 'permission-log-controller': { teams: ['@MetaMask/core-platform'], @@ -607,16 +609,15 @@ function buildJointTeamOwnershipSection(): CodeownersSection { function buildInitializationSection(): CodeownersSection { return { title: 'Initialization', - rules: Object.keys(PACKAGES) - .filter((name) => PACKAGES[name].initializationPath !== undefined) - .sort() - .map((name) => { - const { teams, initializationPath } = PACKAGES[name]; - return { - pattern: `/packages/wallet/src/initialization/instances/${initializationPath}/`, + rules: Object.values(PACKAGES) + .filter((info) => info.initializationPath !== undefined) + .flatMap(({ teams, initializationPath }) => + [initializationPath].flat().map((instancePath) => ({ + pattern: `/packages/wallet/src/initialization/instances/${instancePath}/`, owners: teams, - }; - }), + })), + ) + .sort((ruleA, ruleB) => ruleA.pattern.localeCompare(ruleB.pattern)), }; } diff --git a/packages/wallet/CHANGELOG.md b/packages/wallet/CHANGELOG.md index de719b4991a..946f26d1a4a 100644 --- a/packages/wallet/CHANGELOG.md +++ b/packages/wallet/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **BREAKING:** Wire `PermissionController` and `SubjectMetadataController` into the default wallet initialization ([#XXXX](https://github.com/MetaMask/core/pull/XXXX)) + - Adds optional `instanceOptions.permissionController` (`caveatSpecifications`, `permissionSpecifications`, `unrestrictedMethods`) and `instanceOptions.subjectMetadataController.subjectCacheLimit`. + ### Changed - Bump `@metamask/network-controller` from `^35.0.0` to `^35.0.1` ([#9758](https://github.com/MetaMask/core/pull/9758)) diff --git a/packages/wallet/package.json b/packages/wallet/package.json index b5e3f192d1f..aa9183e70d4 100644 --- a/packages/wallet/package.json +++ b/packages/wallet/package.json @@ -67,6 +67,7 @@ "@metamask/messenger": "^2.0.0", "@metamask/network-controller": "^35.0.1", "@metamask/passkey-controller": "^3.0.0", + "@metamask/permission-controller": "^13.1.1", "@metamask/remote-feature-flag-controller": "^5.0.0", "@metamask/scure-bip39": "^2.1.1", "@metamask/seedless-onboarding-controller": "^10.1.0", diff --git a/packages/wallet/src/initialization/instances/index.ts b/packages/wallet/src/initialization/instances/index.ts index d45bb0917b2..6e3e88ff278 100644 --- a/packages/wallet/src/initialization/instances/index.ts +++ b/packages/wallet/src/initialization/instances/index.ts @@ -6,7 +6,9 @@ export { gasFeeController } from './gas-fee-controller/gas-fee-controller.js'; export { keyringController } from './keyring-controller/keyring-controller.js'; export { networkController } from './network-controller/network-controller.js'; export { passkeyController } from './passkey-controller/passkey-controller.js'; +export { permissionController } from './permission-controller/permission-controller.js'; export { remoteFeatureFlagController } from './remote-feature-flag-controller/remote-feature-flag-controller.js'; export { seedlessOnboardingController } from './seedless-onboarding-controller/seedless-onboarding-controller.js'; export { storageService } from './storage-service/storage-service.js'; +export { subjectMetadataController } from './subject-metadata-controller/subject-metadata-controller.js'; export { transactionController } from './transaction-controller/transaction-controller.js'; diff --git a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts new file mode 100644 index 00000000000..09b8e3bfab5 --- /dev/null +++ b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts @@ -0,0 +1,88 @@ +import { Messenger } from '@metamask/messenger'; +import { PermissionController } from '@metamask/permission-controller'; + +import { defaultConfigurations } from '../../defaults.js'; +import type { + DefaultActions, + DefaultEvents, + RootMessenger, +} from '../../defaults.js'; +import { permissionController } from './permission-controller.js'; + +/** + * Creates a root messenger for use in tests. + * + * @returns A root messenger. + */ +function getRootMessenger(): RootMessenger { + return new Messenger({ namespace: 'Root' }); +} + +describe('permissionController', () => { + it('is registered as a default initialization configuration', () => { + expect(Object.values(defaultConfigurations)).toContain( + permissionController, + ); + }); + + it('initializes a PermissionController with default state and no permissions registered', () => { + const messenger = permissionController.getMessenger(getRootMessenger()); + + const instance = permissionController.init({ + state: undefined, + messenger, + options: {}, + }); + + expect(instance).toBeInstanceOf(PermissionController); + expect(instance.state).toStrictEqual({ subjects: {} }); + expect(instance.unrestrictedMethods).toStrictEqual(new Set()); + }); + + it('forwards the provided state to the controller', () => { + const messenger = permissionController.getMessenger(getRootMessenger()); + + const subjects = { + 'https://metamask.io': { + origin: 'https://metamask.io', + permissions: {}, + }, + }; + + const instance = permissionController.init({ + state: { subjects }, + messenger, + options: {}, + }); + + expect(instance.state.subjects).toStrictEqual(subjects); + }); + + it('forwards injected specifications and unrestricted methods', () => { + const messenger = permissionController.getMessenger(getRootMessenger()); + + const instance = permissionController.init({ + state: undefined, + messenger, + options: { + caveatSpecifications: {}, + permissionSpecifications: {}, + unrestrictedMethods: ['eth_chainId', 'eth_blockNumber'], + }, + }); + + expect(instance.hasUnrestrictedMethod('eth_chainId')).toBe(true); + expect(instance.hasUnrestrictedMethod('eth_sendTransaction')).toBe(false); + }); + + it('exposes its actions through the root messenger', () => { + const rootMessenger = getRootMessenger(); + const messenger = permissionController.getMessenger(rootMessenger); + + permissionController.init({ state: undefined, messenger, options: {} }); + + expect(rootMessenger.call('PermissionController:getState')).toStrictEqual({ + subjects: {}, + }); + }); +}); diff --git a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts new file mode 100644 index 00000000000..2ec98fcb452 --- /dev/null +++ b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts @@ -0,0 +1,53 @@ +import { Messenger } from '@metamask/messenger'; +import { + CaveatSpecificationConstraint, + GenericPermissionController, + PermissionController, + PermissionControllerMessenger, + PermissionSpecificationConstraint, +} from '@metamask/permission-controller'; + +import type { + DefaultActions, + DefaultEvents, + RootMessenger, +} from '../../defaults.js'; +import type { InitializationConfiguration } from '../../types.js'; + +export const permissionController: InitializationConfiguration< + GenericPermissionController, + PermissionControllerMessenger +> = { + name: 'PermissionController', + init: ({ state, messenger, options }) => + new PermissionController< + PermissionSpecificationConstraint, + CaveatSpecificationConstraint + >({ + state, + messenger, + caveatSpecifications: options.caveatSpecifications ?? {}, + permissionSpecifications: options.permissionSpecifications ?? {}, + unrestrictedMethods: options.unrestrictedMethods ?? [], + }), + getMessenger: (parent: RootMessenger) => { + const permissionControllerMessenger: PermissionControllerMessenger = + new Messenger({ + namespace: 'PermissionController', + parent, + }); + + parent.delegate({ + messenger: permissionControllerMessenger, + actions: [ + 'ApprovalController:addRequest', + 'ApprovalController:hasRequest', + 'ApprovalController:acceptRequest', + 'ApprovalController:rejectRequest', + 'SubjectMetadataController:getSubjectMetadata', + ], + }); + + return permissionControllerMessenger; + }, +}; diff --git a/packages/wallet/src/initialization/instances/permission-controller/types.ts b/packages/wallet/src/initialization/instances/permission-controller/types.ts new file mode 100644 index 00000000000..f615cc6343e --- /dev/null +++ b/packages/wallet/src/initialization/instances/permission-controller/types.ts @@ -0,0 +1,35 @@ +import type { + CaveatSpecificationConstraint, + PermissionControllerOptions, + PermissionSpecificationConstraint, +} from '@metamask/permission-controller'; + +type GenericPermissionControllerOptions = PermissionControllerOptions< + PermissionSpecificationConstraint, + CaveatSpecificationConstraint +>; + +/** + * Per-instance options for the wallet's `PermissionController`. + * + * The permission and caveat specifications define which permissions exist and + * how their caveats behave; they vary substantially between clients (CAIP-25 + * account permissions, Snaps endowments and restricted methods, etc.), as does + * the set of unrestricted JSON-RPC methods. They are therefore injected rather + * than hardcoded. Each field is optional and defaults to an empty set, so the + * controller initializes with no permissions registered. + */ +export type PermissionControllerInstanceOptions = { + /** + * Specifications of all caveats available to the controller. + */ + caveatSpecifications?: GenericPermissionControllerOptions['caveatSpecifications']; + /** + * Specifications of all permissions available to the controller. + */ + permissionSpecifications?: GenericPermissionControllerOptions['permissionSpecifications']; + /** + * Names of all JSON-RPC methods that bypass permission gating. + */ + unrestrictedMethods?: GenericPermissionControllerOptions['unrestrictedMethods']; +}; diff --git a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.test.ts b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.test.ts new file mode 100644 index 00000000000..021ba503855 --- /dev/null +++ b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.test.ts @@ -0,0 +1,145 @@ +import { Messenger } from '@metamask/messenger'; +import { SubjectMetadataController } from '@metamask/permission-controller'; + +import { defaultConfigurations } from '../../defaults.js'; +import type { + DefaultActions, + DefaultEvents, + RootMessenger, +} from '../../defaults.js'; +import { subjectMetadataController } from './subject-metadata-controller.js'; + +/** + * Creates a root messenger for use in tests. + * + * @returns A root messenger. + */ +function getRootMessenger(): RootMessenger { + return new Messenger({ namespace: 'Root' }); +} + +/** + * Registers a stub `PermissionController:hasPermissions` handler on the bus, as + * the real `PermissionController` would. `SubjectMetadataController` calls this + * action when hydrating from state and when trimming its cache. + * + * @param rootMessenger - The root messenger to register the handler on. + * @param hasPermissions - The value the stub returns for any origin. + */ +function registerHasPermissionsStub( + rootMessenger: RootMessenger, + hasPermissions: boolean, +): void { + const permissionControllerMessenger = new Messenger({ + namespace: 'PermissionController', + parent: rootMessenger, + }); + + permissionControllerMessenger.registerActionHandler( + 'PermissionController:hasPermissions', + () => hasPermissions, + ); +} + +describe('subjectMetadataController', () => { + it('is registered as a default initialization configuration', () => { + expect(Object.values(defaultConfigurations)).toContain( + subjectMetadataController, + ); + }); + + it('initializes a SubjectMetadataController with default state', () => { + const messenger = + subjectMetadataController.getMessenger(getRootMessenger()); + + const instance = subjectMetadataController.init({ + state: undefined, + messenger, + options: {}, + }); + + expect(instance).toBeInstanceOf(SubjectMetadataController); + expect(instance.state).toStrictEqual({ subjectMetadata: {} }); + }); + + it('forwards the provided state to the controller', () => { + const rootMessenger = getRootMessenger(); + // Subjects retained on hydration only when they still hold permissions. + registerHasPermissionsStub(rootMessenger, true); + const messenger = subjectMetadataController.getMessenger(rootMessenger); + + const subjectMetadata = { + 'https://metamask.io': { + origin: 'https://metamask.io', + name: 'MetaMask', + subjectType: null, + extensionId: null, + iconUrl: null, + }, + }; + + const instance = subjectMetadataController.init({ + state: { subjectMetadata }, + messenger, + options: {}, + }); + + expect(instance.state.subjectMetadata).toStrictEqual(subjectMetadata); + }); + + it('exposes its actions through the root messenger', () => { + const rootMessenger = getRootMessenger(); + const messenger = subjectMetadataController.getMessenger(rootMessenger); + + subjectMetadataController.init({ + state: undefined, + messenger, + options: {}, + }); + + expect( + rootMessenger.call('SubjectMetadataController:getState'), + ).toStrictEqual({ subjectMetadata: {} }); + }); + + it('forwards a custom subjectCacheLimit, evicting the oldest permissionless subject', () => { + const rootMessenger = getRootMessenger(); + registerHasPermissionsStub(rootMessenger, false); + const messenger = subjectMetadataController.getMessenger(rootMessenger); + + const instance = subjectMetadataController.init({ + state: undefined, + messenger, + options: { subjectCacheLimit: 1 }, + }); + + instance.addSubjectMetadata({ origin: 'https://a.example' }); + instance.addSubjectMetadata({ origin: 'https://b.example' }); + + // With a cache limit of 1 and neither subject holding permissions, the + // first is evicted when the second is added. + expect(Object.keys(instance.state.subjectMetadata)).toStrictEqual([ + 'https://b.example', + ]); + }); + + it('defaults subjectCacheLimit to 100 when omitted, retaining subjects past a small count', () => { + const rootMessenger = getRootMessenger(); + registerHasPermissionsStub(rootMessenger, false); + const messenger = subjectMetadataController.getMessenger(rootMessenger); + + const instance = subjectMetadataController.init({ + state: undefined, + messenger, + options: {}, + }); + + instance.addSubjectMetadata({ origin: 'https://a.example' }); + instance.addSubjectMetadata({ origin: 'https://b.example' }); + + expect(Object.keys(instance.state.subjectMetadata)).toStrictEqual([ + 'https://a.example', + 'https://b.example', + ]); + }); +}); diff --git a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts new file mode 100644 index 00000000000..70f91363dc0 --- /dev/null +++ b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts @@ -0,0 +1,48 @@ +import { Messenger } from '@metamask/messenger'; +import { + SubjectMetadataController, + SubjectMetadataControllerMessenger, +} from '@metamask/permission-controller'; + +import type { + DefaultActions, + DefaultEvents, + RootMessenger, +} from '../../defaults.js'; +import type { InitializationConfiguration } from '../../types.js'; + +/** + * Maximum number of distinct permissionless subjects to cache metadata for + * before the oldest is evicted. Both the extension and mobile clients use + * `100`; clients can override via + * `instanceOptions.subjectMetadataController.subjectCacheLimit`. + */ +const DEFAULT_SUBJECT_CACHE_LIMIT = 100; + +export const subjectMetadataController: InitializationConfiguration< + SubjectMetadataController, + SubjectMetadataControllerMessenger +> = { + name: 'SubjectMetadataController', + init: ({ state, messenger, options }) => + new SubjectMetadataController({ + state, + messenger, + subjectCacheLimit: + options.subjectCacheLimit ?? DEFAULT_SUBJECT_CACHE_LIMIT, + }), + getMessenger: (parent: RootMessenger) => { + const subjectMetadataControllerMessenger: SubjectMetadataControllerMessenger = + new Messenger({ + namespace: 'SubjectMetadataController', + parent, + }); + + parent.delegate({ + messenger: subjectMetadataControllerMessenger, + actions: ['PermissionController:hasPermissions'], + }); + + return subjectMetadataControllerMessenger; + }, +}; diff --git a/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts b/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts new file mode 100644 index 00000000000..160c71e55c7 --- /dev/null +++ b/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts @@ -0,0 +1,16 @@ +import type { SubjectMetadataController } from '@metamask/permission-controller'; + +/** + * Per-instance options for the wallet's `SubjectMetadataController`. + */ +export type SubjectMetadataControllerInstanceOptions = { + /** + * Maximum number of distinct permissionless subjects (origins) to retain + * metadata for. Once exceeded, the oldest permissionless subject is evicted + * (FIFO). Defaults to `100` when omitted, matching the extension and mobile + * clients. + */ + subjectCacheLimit?: ConstructorParameters< + typeof SubjectMetadataController + >[0]['subjectCacheLimit']; +}; diff --git a/packages/wallet/src/types.ts b/packages/wallet/src/types.ts index 978bf1b1afd..25321bf4dfd 100644 --- a/packages/wallet/src/types.ts +++ b/packages/wallet/src/types.ts @@ -11,9 +11,11 @@ import type { GasFeeControllerInstanceOptions } from './initialization/instances import type { KeyringControllerInstanceOptions } from './initialization/instances/keyring-controller/types.js'; import type { NetworkControllerInstanceOptions } from './initialization/instances/network-controller/types.js'; import type { PasskeyControllerInstanceOptions } from './initialization/instances/passkey-controller/types.js'; +import type { PermissionControllerInstanceOptions } from './initialization/instances/permission-controller/types.js'; import type { RemoteFeatureFlagControllerInstanceOptions } from './initialization/instances/remote-feature-flag-controller/types.js'; import type { SeedlessOnboardingControllerInstanceOptions } from './initialization/instances/seedless-onboarding-controller/types.js'; import type { StorageServiceInstanceOptions } from './initialization/instances/storage-service/types.js'; +import type { SubjectMetadataControllerInstanceOptions } from './initialization/instances/subject-metadata-controller/types.js'; import type { TransactionControllerInstanceOptions } from './initialization/instances/transaction-controller/types.js'; import type { InitializationConfiguration } from './initialization/types.js'; @@ -33,8 +35,10 @@ export type InstanceSpecificOptions = { gasFeeController: GasFeeControllerInstanceOptions; keyringController?: KeyringControllerInstanceOptions; networkController: NetworkControllerInstanceOptions; + permissionController?: PermissionControllerInstanceOptions; remoteFeatureFlagController: RemoteFeatureFlagControllerInstanceOptions; storageService: StorageServiceInstanceOptions; + subjectMetadataController?: SubjectMetadataControllerInstanceOptions; transactionController?: TransactionControllerInstanceOptions; passkeyController?: PasskeyControllerInstanceOptions; seedlessOnboardingController?: SeedlessOnboardingControllerInstanceOptions; From a849c97e0caf1a6333e1c6fc9381eb5997644dce Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 29 Jun 2026 12:43:21 +0200 Subject: [PATCH 2/6] docs(wallet): set changelog PR link to #9300 Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/wallet/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/wallet/CHANGELOG.md b/packages/wallet/CHANGELOG.md index 946f26d1a4a..95c0c0a9168 100644 --- a/packages/wallet/CHANGELOG.md +++ b/packages/wallet/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- **BREAKING:** Wire `PermissionController` and `SubjectMetadataController` into the default wallet initialization ([#XXXX](https://github.com/MetaMask/core/pull/XXXX)) +- **BREAKING:** Wire `PermissionController` and `SubjectMetadataController` into the default wallet initialization ([#9300](https://github.com/MetaMask/core/pull/9300)) - Adds optional `instanceOptions.permissionController` (`caveatSpecifications`, `permissionSpecifications`, `unrestrictedMethods`) and `instanceOptions.subjectMetadataController.subjectCacheLimit`. ### Changed From 41418944f0b2b245f310f7bc2320a9a8e7eae1cb Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 29 Jun 2026 13:03:54 +0200 Subject: [PATCH 3/6] test(wallet): strengthen permission/subject-metadata coverage from review - Add `Wallet.test.ts` integration coverage: both controllers are reachable on the assembled wallet, and persisted subject metadata hydrates without throwing (proving the default wiring initializes `PermissionController` before `SubjectMetadataController`, which calls `PermissionController:hasPermissions` during hydration). - Verify `PermissionController` can reach its delegated `ApprovalController` and `SubjectMetadataController` actions, so a dropped allowlist entry now fails. - Prove the injected permission specifications are forwarded (via `grantPermissions`) rather than only exercising the empty default. - Replace the weak "defaults to 100" test with a boundary test (100 retained, 101st evicts the oldest) and add coverage for retaining subjects that hold permissions past the cache limit. - Document the construction-order dependency and soften cross-repo comments. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/wallet/src/Wallet.test.ts | 66 ++++++++++++++++ .../permission-controller.test.ts | 78 +++++++++++++++++-- .../subject-metadata-controller.test.ts | 34 ++++++-- .../subject-metadata-controller.ts | 10 ++- .../subject-metadata-controller/types.ts | 5 +- 5 files changed, 175 insertions(+), 18 deletions(-) diff --git a/packages/wallet/src/Wallet.test.ts b/packages/wallet/src/Wallet.test.ts index e370c699aa0..31a64640c5c 100644 --- a/packages/wallet/src/Wallet.test.ts +++ b/packages/wallet/src/Wallet.test.ts @@ -508,4 +508,70 @@ describe('Wallet', () => { ).toStrictEqual({ testFlag: true }); }); }); + + describe('PermissionController', () => { + it('is wired and exposes its state on the wallet messenger', async () => { + const wallet = await setupWallet(); + + expect( + wallet.messenger.call('PermissionController:getState'), + ).toStrictEqual({ subjects: {} }); + }); + }); + + describe('SubjectMetadataController', () => { + it('is wired and exposes its state on the wallet messenger', async () => { + const wallet = await setupWallet(); + + expect( + wallet.messenger.call('SubjectMetadataController:getState'), + ).toStrictEqual({ subjectMetadata: {} }); + }); + + it('hydrates persisted subject metadata, consulting the wired PermissionController for retention', () => { + // Constructing the controller from persisted state calls + // `PermissionController:hasPermissions`, so this proves the default + // wiring initializes `PermissionController` before + // `SubjectMetadataController` (otherwise construction would throw). + const origin = 'https://metamask.io'; + + const wallet = new Wallet({ + state: { + PermissionController: { + subjects: { + [origin]: { origin, permissions: { somePermission: {} } }, + }, + }, + SubjectMetadataController: { + subjectMetadata: { + [origin]: { + origin, + name: 'MetaMask', + subjectType: null, + extensionId: null, + iconUrl: null, + }, + }, + }, + }, + instanceOptions: { + connectivityController: { + connectivityAdapter: new AlwaysOnlineAdapter(), + }, + networkController: { + infuraProjectId: 'fake-infura-project-id', + }, + storageService: { + storage: new InMemoryStorageAdapter(), + }, + remoteFeatureFlagController: REMOTE_FEATURE_FLAG_OPTIONS, + }, + }); + + // The subject holds permissions, so its metadata is retained on hydration. + expect( + Object.keys(wallet.state.SubjectMetadataController.subjectMetadata), + ).toContain(origin); + }); + }); }); diff --git a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts index 09b8e3bfab5..aaeb819531f 100644 --- a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts +++ b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts @@ -1,5 +1,8 @@ import { Messenger } from '@metamask/messenger'; -import { PermissionController } from '@metamask/permission-controller'; +import { + PermissionController, + PermissionType, +} from '@metamask/permission-controller'; import { defaultConfigurations } from '../../defaults.js'; import type { @@ -58,23 +61,48 @@ describe('permissionController', () => { expect(instance.state.subjects).toStrictEqual(subjects); }); - it('forwards injected specifications and unrestricted methods', () => { + it('forwards injected unrestrictedMethods to the controller', () => { const messenger = permissionController.getMessenger(getRootMessenger()); const instance = permissionController.init({ state: undefined, messenger, - options: { - caveatSpecifications: {}, - permissionSpecifications: {}, - unrestrictedMethods: ['eth_chainId', 'eth_blockNumber'], - }, + options: { unrestrictedMethods: ['eth_chainId', 'eth_blockNumber'] }, }); expect(instance.hasUnrestrictedMethod('eth_chainId')).toBe(true); expect(instance.hasUnrestrictedMethod('eth_sendTransaction')).toBe(false); }); + it('forwards injected permission specifications to the controller', () => { + const messenger = permissionController.getMessenger(getRootMessenger()); + const origin = 'https://metamask.io'; + + const instance = permissionController.init({ + state: undefined, + messenger, + options: { + permissionSpecifications: { + wallet_noop: { + permissionType: PermissionType.RestrictedMethod, + targetName: 'wallet_noop', + allowedCaveats: null, + methodImplementation: () => null, + }, + }, + }, + }); + + // Granting the injected permission only succeeds if its specification was + // forwarded to the controller; an unknown target would throw. + instance.grantPermissions({ + subject: { origin }, + approvedPermissions: { wallet_noop: {} }, + }); + + expect(instance.getPermissions(origin)).toHaveProperty('wallet_noop'); + }); + it('exposes its actions through the root messenger', () => { const rootMessenger = getRootMessenger(); const messenger = permissionController.getMessenger(rootMessenger); @@ -85,4 +113,40 @@ describe('permissionController', () => { subjects: {}, }); }); + + it('can reach the actions delegated to its messenger', () => { + const rootMessenger = getRootMessenger(); + + // Register stub handlers as the real ApprovalController and + // SubjectMetadataController would, then confirm the PermissionController's + // messenger can call them — proving the delegation allowlist is wired. + const approvalControllerMessenger = new Messenger({ + namespace: 'ApprovalController', + parent: rootMessenger, + }); + approvalControllerMessenger.registerActionHandler( + 'ApprovalController:hasRequest', + () => true, + ); + const subjectMetadataControllerMessenger = new Messenger({ + namespace: 'SubjectMetadataController', + parent: rootMessenger, + }); + subjectMetadataControllerMessenger.registerActionHandler( + 'SubjectMetadataController:getSubjectMetadata', + () => undefined, + ); + + const messenger = permissionController.getMessenger(rootMessenger); + + expect(messenger.call('ApprovalController:hasRequest', { id: 'x' })).toBe( + true, + ); + expect( + messenger.call( + 'SubjectMetadataController:getSubjectMetadata', + 'https://metamask.io', + ), + ).toBeUndefined(); + }); }); diff --git a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.test.ts b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.test.ts index 021ba503855..11ad83032d0 100644 --- a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.test.ts +++ b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.test.ts @@ -116,30 +116,54 @@ describe('subjectMetadataController', () => { instance.addSubjectMetadata({ origin: 'https://a.example' }); instance.addSubjectMetadata({ origin: 'https://b.example' }); - // With a cache limit of 1 and neither subject holding permissions, the - // first is evicted when the second is added. expect(Object.keys(instance.state.subjectMetadata)).toStrictEqual([ 'https://b.example', ]); }); - it('defaults subjectCacheLimit to 100 when omitted, retaining subjects past a small count', () => { + it('retains a subject with permissions even when the cache limit is exceeded', () => { const rootMessenger = getRootMessenger(); - registerHasPermissionsStub(rootMessenger, false); + // Every subject reports as holding permissions. + registerHasPermissionsStub(rootMessenger, true); const messenger = subjectMetadataController.getMessenger(rootMessenger); const instance = subjectMetadataController.init({ state: undefined, messenger, - options: {}, + options: { subjectCacheLimit: 1 }, }); instance.addSubjectMetadata({ origin: 'https://a.example' }); instance.addSubjectMetadata({ origin: 'https://b.example' }); + // Metadata for subjects with permissions is never evicted. expect(Object.keys(instance.state.subjectMetadata)).toStrictEqual([ 'https://a.example', 'https://b.example', ]); }); + + it('does not evict until the default cache limit of 100 is exceeded', () => { + const rootMessenger = getRootMessenger(); + registerHasPermissionsStub(rootMessenger, false); + const messenger = subjectMetadataController.getMessenger(rootMessenger); + + const instance = subjectMetadataController.init({ + state: undefined, + messenger, + options: {}, + }); + + for (let index = 0; index < 100; index++) { + instance.addSubjectMetadata({ origin: `https://${index}.example` }); + } + expect(Object.keys(instance.state.subjectMetadata)).toHaveLength(100); + + // The 101st permissionless subject evicts the oldest (FIFO). + instance.addSubjectMetadata({ origin: 'https://overflow.example' }); + const origins = Object.keys(instance.state.subjectMetadata); + expect(origins).toHaveLength(100); + expect(origins).not.toContain('https://0.example'); + expect(origins).toContain('https://overflow.example'); + }); }); diff --git a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts index 70f91363dc0..2f4be41ed58 100644 --- a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts +++ b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts @@ -12,9 +12,9 @@ import type { import type { InitializationConfiguration } from '../../types.js'; /** - * Maximum number of distinct permissionless subjects to cache metadata for - * before the oldest is evicted. Both the extension and mobile clients use - * `100`; clients can override via + * Default maximum number of distinct permissionless subjects to cache metadata + * for before the oldest is evicted. `100` matches the value MetaMask clients + * currently use; clients can override via * `instanceOptions.subjectMetadataController.subjectCacheLimit`. */ const DEFAULT_SUBJECT_CACHE_LIMIT = 100; @@ -38,6 +38,10 @@ export const subjectMetadataController: InitializationConfiguration< parent, }); + // `SubjectMetadataController` calls `PermissionController:hasPermissions` + // while hydrating persisted subjects, so `PermissionController` must be + // initialized first (it registers that handler in its constructor). The + // alphabetical export order in `instances/index.ts` guarantees this. parent.delegate({ messenger: subjectMetadataControllerMessenger, actions: ['PermissionController:hasPermissions'], diff --git a/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts b/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts index 160c71e55c7..bcfa1d134ce 100644 --- a/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts +++ b/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts @@ -6,9 +6,8 @@ import type { SubjectMetadataController } from '@metamask/permission-controller' export type SubjectMetadataControllerInstanceOptions = { /** * Maximum number of distinct permissionless subjects (origins) to retain - * metadata for. Once exceeded, the oldest permissionless subject is evicted - * (FIFO). Defaults to `100` when omitted, matching the extension and mobile - * clients. + * metadata for, evicted oldest-first once exceeded. Defaults to a + * platform-agnostic value when omitted. */ subjectCacheLimit?: ConstructorParameters< typeof SubjectMetadataController From ff97222549090c77971d94522cde0117c7fcddef Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 29 Jun 2026 18:06:36 +0200 Subject: [PATCH 4/6] fix(wallet): initialize overriding configurations in their default's position `initialize` concatenated consumer-supplied `initializationConfigurations` ahead of the defaults, so a configuration overriding a default ran before the other defaults. A custom `SubjectMetadataController` with persisted state would then call `PermissionController:hasPermissions` during hydration before the default `PermissionController` registered that handler, throwing on construction. Overrides now occupy their default's position, preserving construction-order dependencies between defaults. Configurations that don't match a default remain additive and are initialized first. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/wallet/CHANGELOG.md | 4 ++ packages/wallet/src/Wallet.test.ts | 57 +++++++++++++++++++ .../src/initialization/initialization.ts | 27 +++++++-- .../subject-metadata-controller.ts | 7 +-- 4 files changed, 85 insertions(+), 10 deletions(-) diff --git a/packages/wallet/CHANGELOG.md b/packages/wallet/CHANGELOG.md index 95c0c0a9168..5b65ee3f734 100644 --- a/packages/wallet/CHANGELOG.md +++ b/packages/wallet/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- A configuration that overrides a default is now initialized in that default's position rather than ahead of all defaults, preserving construction-order dependencies between default controllers (e.g. `PermissionController` before `SubjectMetadataController`) ([#9300](https://github.com/MetaMask/core/pull/9300)) - Bump `@metamask/network-controller` from `^35.0.0` to `^35.0.1` ([#9758](https://github.com/MetaMask/core/pull/9758)) ## [9.0.0] @@ -79,6 +80,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/accounts-controller` from `^39.0.3` to `^39.0.4` ([#9349](https://github.com/MetaMask/core/pull/9349)) - Bump `@metamask/network-controller` from `^33.0.0` to `^34.0.0` ([#9349](https://github.com/MetaMask/core/pull/9349)) +======= +- A configuration that overrides a default is now initialized in that default's position rather than ahead of all defaults, preserving construction-order dependencies between default controllers (e.g. `PermissionController` before `SubjectMetadataController`) ([#9300](https://github.com/MetaMask/core/pull/9300)) +>>>>>>> 0b8cac1b5b (fix(wallet): initialize overriding configurations in their default's position) ## [5.0.0] diff --git a/packages/wallet/src/Wallet.test.ts b/packages/wallet/src/Wallet.test.ts index 31a64640c5c..dde2225d6b8 100644 --- a/packages/wallet/src/Wallet.test.ts +++ b/packages/wallet/src/Wallet.test.ts @@ -8,6 +8,8 @@ import { webcrypto } from 'crypto'; import MockEncryptor from '../../keyring-controller/tests/mocks/mockEncryptor.js'; import * as initializationModule from './initialization/initialization.js'; import { AlwaysOnlineAdapter } from './initialization/instances/connectivity-controller/always-online-adapter.js'; +import { subjectMetadataController } from './initialization/instances/subject-metadata-controller/subject-metadata-controller.js'; +import type { InitializationConfiguration } from './initialization/types.js'; import { importSecretRecoveryPhrase } from './utilities.js'; import { Wallet } from './Wallet.js'; @@ -573,5 +575,60 @@ describe('Wallet', () => { Object.keys(wallet.state.SubjectMetadataController.subjectMetadata), ).toContain(origin); }); + + it('is constructed after the default PermissionController even when overridden', () => { + // An override hydrates via `PermissionController:hasPermissions`, so it + // must run after the default `PermissionController`, not ahead of it. + const origin = 'https://metamask.io'; + + let wallet: Wallet | undefined; + + expect(() => { + wallet = new Wallet({ + initializationConfigurations: [ + subjectMetadataController as InitializationConfiguration< + unknown, + unknown + >, + ], + state: { + PermissionController: { + subjects: { + [origin]: { origin, permissions: { somePermission: {} } }, + }, + }, + SubjectMetadataController: { + subjectMetadata: { + [origin]: { + origin, + name: 'MetaMask', + subjectType: null, + extensionId: null, + iconUrl: null, + }, + }, + }, + }, + instanceOptions: { + connectivityController: { + connectivityAdapter: new AlwaysOnlineAdapter(), + }, + networkController: { + infuraProjectId: 'fake-infura-project-id', + }, + storageService: { + storage: new InMemoryStorageAdapter(), + }, + remoteFeatureFlagController: REMOTE_FEATURE_FLAG_OPTIONS, + }, + }); + }).not.toThrow(); + + expect( + Object.keys( + (wallet as Wallet).state.SubjectMetadataController.subjectMetadata, + ), + ).toContain(origin); + }); }); }); diff --git a/packages/wallet/src/initialization/initialization.ts b/packages/wallet/src/initialization/initialization.ts index 9ed426722de..8564a1a5373 100644 --- a/packages/wallet/src/initialization/initialization.ts +++ b/packages/wallet/src/initialization/initialization.ts @@ -25,14 +25,29 @@ export function initialize(options: InitializeOptions): DefaultInstances { instanceOptions, } = options; - const overriddenConfiguration = initializationConfigurations.map( - (config) => config.name, + const defaultConfigurationEntries = Object.values( + defaultConfigurations, + ) as InitializationConfiguration[]; + + const overrideConfigurationsByName = new Map( + initializationConfigurations.map((config) => [config.name, config]), + ); + const defaultConfigurationNames = new Set( + defaultConfigurationEntries.map((config) => config.name), + ); + + // Overrides keep their default's position so construction order between + // defaults is preserved (e.g. `PermissionController` before + // `SubjectMetadataController`). Non-default configs are additive and run first. + const additionalConfigurations = initializationConfigurations.filter( + (config) => !defaultConfigurationNames.has(config.name), + ); + const mergedDefaultConfigurations = defaultConfigurationEntries.map( + (config) => overrideConfigurationsByName.get(config.name) ?? config, ); - const configurationEntries = initializationConfigurations.concat( - Object.values(defaultConfigurations).filter( - (config) => !overriddenConfiguration.includes(config.name), - ) as InitializationConfiguration[], + const configurationEntries = additionalConfigurations.concat( + mergedDefaultConfigurations, ); const instances: Record = {}; diff --git a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts index 2f4be41ed58..4fb5b43349b 100644 --- a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts +++ b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts @@ -38,10 +38,9 @@ export const subjectMetadataController: InitializationConfiguration< parent, }); - // `SubjectMetadataController` calls `PermissionController:hasPermissions` - // while hydrating persisted subjects, so `PermissionController` must be - // initialized first (it registers that handler in its constructor). The - // alphabetical export order in `instances/index.ts` guarantees this. + // Hydration calls `PermissionController:hasPermissions`, so + // `PermissionController` must be constructed first. It sorts earlier in + // `instances/index.ts`, and `initialize` keeps that order under overrides. parent.delegate({ messenger: subjectMetadataControllerMessenger, actions: ['PermissionController:hasPermissions'], From 3e7952b6c1e3ab6bf30353a4886875bee9aef8a3 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 3 Aug 2026 17:34:36 +0200 Subject: [PATCH 5/6] fix(wallet): adapt permission wiring to current `main` Rebasing #9300 onto `main` after five weeks of drift required these adjustments, none of which change the feature being wired: - `codeowners.ts` is now the source of truth for `.github/CODEOWNERS`, which is generated. `initializationPath` accepts an array so `permission-controller` can claim both of the instance directories it supplies, and rules are sorted by pattern so the generated section stays alphabetical. - `Wallet` tests pass the `gasFeeController` instance option, required since #9527. - Regenerate `README.md` and both `tsconfig`s via their fixers. - Repair the `[6.0.0]` changelog section, which a conflict resolution had left with duplicated content. - Spell out the client-facing consequences of the wiring in the changelog: the duplicate-registration collision for consumers passing their own messenger, and the fact that permission specifications with side effects (the Snaps specifications reach `SnapController:*`) need a wider delegation allowlist than the default configuration provides. - Cover `caveatSpecifications` injection, the one instance option without a behavioural assertion. Co-Authored-By: Claude Opus 5 (1M context) --- .github/CODEOWNERS | 2 + README.md | 1 + codeowners.ts | 8 ++- packages/wallet/CHANGELOG.md | 7 +-- packages/wallet/src/Wallet.test.ts | 6 ++ .../permission-controller.test.ts | 55 +++++++++++++++++++ .../permission-controller.ts | 5 ++ .../subject-metadata-controller.ts | 7 ++- packages/wallet/tsconfig.build.json | 1 + packages/wallet/tsconfig.json | 3 + yarn.lock | 1 + 11 files changed, 87 insertions(+), 9 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index c795476942f..94b3ae59b64 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -154,9 +154,11 @@ /packages/wallet/src/initialization/instances/gas-fee-controller/ @MetaMask/confirmations /packages/wallet/src/initialization/instances/keyring-controller/ @MetaMask/accounts-engineers @MetaMask/core-platform /packages/wallet/src/initialization/instances/passkey-controller/ @MetaMask/web3auth +/packages/wallet/src/initialization/instances/permission-controller/ @MetaMask/core-platform /packages/wallet/src/initialization/instances/remote-feature-flag-controller/ @MetaMask/extension-platform @MetaMask/mobile-platform @MetaMask/core-platform /packages/wallet/src/initialization/instances/seedless-onboarding-controller/ @MetaMask/web3auth /packages/wallet/src/initialization/instances/storage-service/ @MetaMask/extension-platform @MetaMask/mobile-platform @MetaMask/core-platform +/packages/wallet/src/initialization/instances/subject-metadata-controller/ @MetaMask/core-platform /packages/wallet/src/initialization/instances/transaction-controller/ @MetaMask/confirmations ## Package Release related diff --git a/README.md b/README.md index 733f6c8b295..ee1db6e74ce 100644 --- a/README.md +++ b/README.md @@ -662,6 +662,7 @@ linkStyle default opacity:0.5 wallet --> messenger; wallet --> network_controller; wallet --> passkey_controller; + wallet --> permission_controller; wallet --> remote_feature_flag_controller; wallet --> seedless_onboarding_controller; wallet --> storage_service; diff --git a/codeowners.ts b/codeowners.ts index 073918b0302..0beca537d6e 100644 --- a/codeowners.ts +++ b/codeowners.ts @@ -257,7 +257,10 @@ const PACKAGES: Record = { }, 'permission-controller': { teams: ['@MetaMask/core-platform'], - initializationPath: ['permission-controller', 'subject-metadata-controller'], + initializationPath: [ + 'permission-controller', + 'subject-metadata-controller', + ], }, 'permission-log-controller': { teams: ['@MetaMask/core-platform'], @@ -610,8 +613,7 @@ function buildInitializationSection(): CodeownersSection { return { title: 'Initialization', rules: Object.values(PACKAGES) - .filter((info) => info.initializationPath !== undefined) - .flatMap(({ teams, initializationPath }) => + .flatMap(({ teams, initializationPath = [] }) => [initializationPath].flat().map((instancePath) => ({ pattern: `/packages/wallet/src/initialization/instances/${instancePath}/`, owners: teams, diff --git a/packages/wallet/CHANGELOG.md b/packages/wallet/CHANGELOG.md index 5b65ee3f734..426fd469f45 100644 --- a/packages/wallet/CHANGELOG.md +++ b/packages/wallet/CHANGELOG.md @@ -10,7 +10,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **BREAKING:** Wire `PermissionController` and `SubjectMetadataController` into the default wallet initialization ([#9300](https://github.com/MetaMask/core/pull/9300)) - - Adds optional `instanceOptions.permissionController` (`caveatSpecifications`, `permissionSpecifications`, `unrestrictedMethods`) and `instanceOptions.subjectMetadataController.subjectCacheLimit`. + - The default `Wallet` now constructs both controllers and registers their `PermissionController:*` and `SubjectMetadataController:*` messenger actions. Consumers that pass their own `messenger` and already wire either controller must remove their own before upgrading, or the duplicate registration will collide. + - Adds optional `instanceOptions.permissionController` (`caveatSpecifications`, `permissionSpecifications`, `unrestrictedMethods`) and `instanceOptions.subjectMetadataController.subjectCacheLimit`. Each defaults to an empty set, and `subjectCacheLimit` defaults to `100`, so the default `PermissionController` gates nothing until a consumer injects specifications. + - The delegated action allowlist covers only what `PermissionControllerMessenger` declares. A consumer whose `permissionSpecifications` invoke additional actions through the side-effect messenger (for example, the Snaps `wallet_snap` specifications, which call `SnapController:getPermittedSnaps` and `SnapController:installSnaps`) must override the `PermissionController` configuration via `initializationConfigurations` to widen the allowlist. ### Changed @@ -80,9 +82,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/accounts-controller` from `^39.0.3` to `^39.0.4` ([#9349](https://github.com/MetaMask/core/pull/9349)) - Bump `@metamask/network-controller` from `^33.0.0` to `^34.0.0` ([#9349](https://github.com/MetaMask/core/pull/9349)) -======= -- A configuration that overrides a default is now initialized in that default's position rather than ahead of all defaults, preserving construction-order dependencies between default controllers (e.g. `PermissionController` before `SubjectMetadataController`) ([#9300](https://github.com/MetaMask/core/pull/9300)) ->>>>>>> 0b8cac1b5b (fix(wallet): initialize overriding configurations in their default's position) ## [5.0.0] diff --git a/packages/wallet/src/Wallet.test.ts b/packages/wallet/src/Wallet.test.ts index dde2225d6b8..c9d398ceec9 100644 --- a/packages/wallet/src/Wallet.test.ts +++ b/packages/wallet/src/Wallet.test.ts @@ -560,6 +560,9 @@ describe('Wallet', () => { connectivityController: { connectivityAdapter: new AlwaysOnlineAdapter(), }, + gasFeeController: { + clientId: 'test', + }, networkController: { infuraProjectId: 'fake-infura-project-id', }, @@ -613,6 +616,9 @@ describe('Wallet', () => { connectivityController: { connectivityAdapter: new AlwaysOnlineAdapter(), }, + gasFeeController: { + clientId: 'test', + }, networkController: { infuraProjectId: 'fake-infura-project-id', }, diff --git a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts index aaeb819531f..5459555067a 100644 --- a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts +++ b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts @@ -103,6 +103,61 @@ describe('permissionController', () => { expect(instance.getPermissions(origin)).toHaveProperty('wallet_noop'); }); + it('forwards injected caveat specifications to the controller', () => { + const messenger = permissionController.getMessenger(getRootMessenger()); + const origin = 'https://metamask.io'; + + const instance = permissionController.init({ + state: undefined, + messenger, + options: { + caveatSpecifications: { + noopCaveat: { + type: 'noopCaveat', + decorator: (methodImplementation) => methodImplementation, + validator: ({ value }) => { + if (value !== 'allowed') { + throw new Error('Invalid noopCaveat value'); + } + }, + }, + }, + permissionSpecifications: { + wallet_noop: { + permissionType: PermissionType.RestrictedMethod, + targetName: 'wallet_noop', + allowedCaveats: ['noopCaveat'], + methodImplementation: () => null, + }, + }, + }, + }); + + // The injected caveat's validator only runs if its specification was + // forwarded to the controller. + expect(() => + instance.grantPermissions({ + subject: { origin }, + approvedPermissions: { + wallet_noop: { + caveats: [{ type: 'noopCaveat', value: 'rejected' }], + }, + }, + }), + ).toThrow('Invalid noopCaveat value'); + + instance.grantPermissions({ + subject: { origin }, + approvedPermissions: { + wallet_noop: { caveats: [{ type: 'noopCaveat', value: 'allowed' }] }, + }, + }); + + expect( + instance.getCaveat(origin, 'wallet_noop', 'noopCaveat'), + ).toStrictEqual({ type: 'noopCaveat', value: 'allowed' }); + }); + it('exposes its actions through the root messenger', () => { const rootMessenger = getRootMessenger(); const messenger = permissionController.getMessenger(rootMessenger); diff --git a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts index 2ec98fcb452..67cc9a6890c 100644 --- a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts +++ b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts @@ -37,6 +37,11 @@ export const permissionController: InitializationConfiguration< parent, }); + // Only what `PermissionControllerMessenger` declares. Permission + // specifications whose side effects call further actions (e.g. the Snaps + // specifications, which reach `SnapController:*`) need a wider allowlist + // than this, so such a consumer must override this configuration rather + // than rely on the default. parent.delegate({ messenger: permissionControllerMessenger, actions: [ diff --git a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts index 4fb5b43349b..728cb711d4e 100644 --- a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts +++ b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts @@ -39,8 +39,11 @@ export const subjectMetadataController: InitializationConfiguration< }); // Hydration calls `PermissionController:hasPermissions`, so - // `PermissionController` must be constructed first. It sorts earlier in - // `instances/index.ts`, and `initialize` keeps that order under overrides. + // `PermissionController` must be constructed first. `initialize` builds + // defaults from `Object.values(defaultConfigurations)`, whose keys a module + // namespace object always sorts alphabetically — so `permissionController` + // precedes `subjectMetadataController` regardless of declaration order — + // and `initialize` keeps that position under overrides. parent.delegate({ messenger: subjectMetadataControllerMessenger, actions: ['PermissionController:hasPermissions'], diff --git a/packages/wallet/tsconfig.build.json b/packages/wallet/tsconfig.build.json index dae6d88285a..dadece731c6 100644 --- a/packages/wallet/tsconfig.build.json +++ b/packages/wallet/tsconfig.build.json @@ -17,6 +17,7 @@ { "path": "../messenger/tsconfig.build.json" }, { "path": "../network-controller/tsconfig.build.json" }, { "path": "../passkey-controller/tsconfig.build.json" }, + { "path": "../permission-controller/tsconfig.build.json" }, { "path": "../remote-feature-flag-controller/tsconfig.build.json" }, { "path": "../seedless-onboarding-controller/tsconfig.build.json" }, { "path": "../storage-service/tsconfig.build.json" }, diff --git a/packages/wallet/tsconfig.json b/packages/wallet/tsconfig.json index 10221177468..7e3cb6d8ba6 100644 --- a/packages/wallet/tsconfig.json +++ b/packages/wallet/tsconfig.json @@ -37,6 +37,9 @@ { "path": "../passkey-controller" }, + { + "path": "../permission-controller" + }, { "path": "../remote-feature-flag-controller" }, diff --git a/yarn.lock b/yarn.lock index 011f96c52aa..44efb4eeae7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9543,6 +9543,7 @@ __metadata: "@metamask/messenger": "npm:^2.0.0" "@metamask/network-controller": "npm:^35.0.1" "@metamask/passkey-controller": "npm:^3.0.0" + "@metamask/permission-controller": "npm:^13.1.1" "@metamask/remote-feature-flag-controller": "npm:^5.0.0" "@metamask/scure-bip39": "npm:^2.1.1" "@metamask/seedless-onboarding-controller": "npm:^10.1.0" From 8eb2c7f6ee0fa22dd628a0bb4df7750ac138538a Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 3 Aug 2026 18:33:57 +0200 Subject: [PATCH 6/6] fix(wallet): address review of permission/subject-metadata wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correct the construction-order rationale, harden `initialize` against ambiguous configuration lists, and close the documentation and test gaps found while reviewing #9300. - The ordering comment claimed a module namespace object sorts its keys regardless of declaration order. That holds only for the ESM build; the CommonJS build — what Jest runs and what `require` consumers load — preserves declaration order. Record the real invariant on `instances/index.ts` and assert the two controllers' relative order. - `initialize` now throws on duplicate names in `initializationConfigurations` instead of silently keeping the last, which could drop an override carrying a wider allowlist. - Mark the override-ordering change `**BREAKING:**`: it applies to any overridden default, not just the two controllers wired here. - Correct the changelog claim that the default `PermissionController` "gates nothing" — with no specifications and no unrestricted methods it is maximally restrictive — and document that subject metadata is retained only for origins present in `PermissionController` state. - Export `InitializationConfiguration`, which a consumer needs to write the override the Snaps guidance calls for, and state what that override actually costs. - Document that `subjectCacheLimit` must be a positive integer, drop the `ConstructorParameters` indirection that resolved to `number`, and cover all five delegated actions plus the reversed-override ordering. Co-Authored-By: Claude Opus 5 (1M context) --- packages/wallet/CHANGELOG.md | 12 +- packages/wallet/src/Wallet.test.ts | 168 +++++--------- packages/wallet/src/index.ts | 1 + .../src/initialization/initialization.test.ts | 213 ++++++++++++++++++ .../src/initialization/initialization.ts | 40 ++-- .../src/initialization/instances/index.ts | 4 + .../permission-controller.test.ts | 34 +++ .../permission-controller.ts | 8 +- .../subject-metadata-controller.ts | 15 +- .../subject-metadata-controller/types.ts | 11 +- 10 files changed, 356 insertions(+), 150 deletions(-) create mode 100644 packages/wallet/src/initialization/initialization.test.ts diff --git a/packages/wallet/CHANGELOG.md b/packages/wallet/CHANGELOG.md index 426fd469f45..67bcb72e0dd 100644 --- a/packages/wallet/CHANGELOG.md +++ b/packages/wallet/CHANGELOG.md @@ -10,13 +10,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **BREAKING:** Wire `PermissionController` and `SubjectMetadataController` into the default wallet initialization ([#9300](https://github.com/MetaMask/core/pull/9300)) - - The default `Wallet` now constructs both controllers and registers their `PermissionController:*` and `SubjectMetadataController:*` messenger actions. Consumers that pass their own `messenger` and already wire either controller must remove their own before upgrading, or the duplicate registration will collide. - - Adds optional `instanceOptions.permissionController` (`caveatSpecifications`, `permissionSpecifications`, `unrestrictedMethods`) and `instanceOptions.subjectMetadataController.subjectCacheLimit`. Each defaults to an empty set, and `subjectCacheLimit` defaults to `100`, so the default `PermissionController` gates nothing until a consumer injects specifications. - - The delegated action allowlist covers only what `PermissionControllerMessenger` declares. A consumer whose `permissionSpecifications` invoke additional actions through the side-effect messenger (for example, the Snaps `wallet_snap` specifications, which call `SnapController:getPermittedSnaps` and `SnapController:installSnaps`) must override the `PermissionController` configuration via `initializationConfigurations` to widen the allowlist. + - Consumers that pass their own `messenger` and already wire either controller must remove their own before upgrading, or the duplicate action registration will collide. + - Adds optional `instanceOptions.permissionController` (`caveatSpecifications`, `permissionSpecifications`, `unrestrictedMethods`) and `instanceOptions.subjectMetadataController.subjectCacheLimit`. Specifications default to empty, so no permission can be granted until a consumer injects them; `subjectCacheLimit` defaults to `100`. + - Consumers restoring persisted state must restore the `PermissionController` key alongside the `SubjectMetadataController` key, since subject metadata is retained only for origins holding permissions. + - Consumers whose `permissionSpecifications` invoke actions that `PermissionControllerMessenger` does not declare (for example the Snaps `wallet_snap` specifications) must replace the `PermissionController` configuration via `initializationConfigurations`. +- Export the `InitializationConfiguration` type, which a consumer needs in order to supply `initializationConfigurations` ([#9300](https://github.com/MetaMask/core/pull/9300)) ### Changed -- A configuration that overrides a default is now initialized in that default's position rather than ahead of all defaults, preserving construction-order dependencies between default controllers (e.g. `PermissionController` before `SubjectMetadataController`) ([#9300](https://github.com/MetaMask/core/pull/9300)) +- **BREAKING:** A configuration passed to `initializationConfigurations` that overrides a default is now initialized in that default's position rather than ahead of all defaults, preserving construction-order dependencies between default controllers (e.g. `PermissionController` before `SubjectMetadataController`) ([#9300](https://github.com/MetaMask/core/pull/9300)) + - An override that relied on being constructed before every other default must now account for its default's position; configurations that do not override a default are still initialized first. +- `initialize` now throws when two entries in `initializationConfigurations` share a `name` ([#9300](https://github.com/MetaMask/core/pull/9300)) - Bump `@metamask/network-controller` from `^35.0.0` to `^35.0.1` ([#9758](https://github.com/MetaMask/core/pull/9758)) ## [9.0.0] diff --git a/packages/wallet/src/Wallet.test.ts b/packages/wallet/src/Wallet.test.ts index c9d398ceec9..1f854225df1 100644 --- a/packages/wallet/src/Wallet.test.ts +++ b/packages/wallet/src/Wallet.test.ts @@ -10,6 +10,7 @@ import * as initializationModule from './initialization/initialization.js'; import { AlwaysOnlineAdapter } from './initialization/instances/connectivity-controller/always-online-adapter.js'; import { subjectMetadataController } from './initialization/instances/subject-metadata-controller/subject-metadata-controller.js'; import type { InitializationConfiguration } from './initialization/types.js'; +import type { WalletOptions } from './types.js'; import { importSecretRecoveryPhrase } from './utilities.js'; import { Wallet } from './Wallet.js'; @@ -25,8 +26,35 @@ const REMOTE_FEATURE_FLAG_OPTIONS = { }, }; -async function setupWallet(): Promise { - const wallet = new Wallet({ +const PERSISTED_SUBJECT_ORIGIN = 'https://metamask.io'; + +const PERSISTED_PERMISSIONED_SUBJECT_STATE = { + PermissionController: { + subjects: { + [PERSISTED_SUBJECT_ORIGIN]: { + origin: PERSISTED_SUBJECT_ORIGIN, + permissions: { somePermission: {} }, + }, + }, + }, + SubjectMetadataController: { + subjectMetadata: { + [PERSISTED_SUBJECT_ORIGIN]: { + origin: PERSISTED_SUBJECT_ORIGIN, + name: 'MetaMask', + subjectType: null, + extensionId: null, + iconUrl: null, + }, + }, + }, +}; + +function createWallet( + options: Omit = {}, +): Wallet { + return new Wallet({ + ...options, instanceOptions: { connectivityController: { connectivityAdapter: new AlwaysOnlineAdapter(), @@ -43,6 +71,10 @@ async function setupWallet(): Promise { remoteFeatureFlagController: REMOTE_FEATURE_FLAG_OPTIONS, }, }); +} + +async function setupWallet(): Promise { + const wallet = createWallet(); await importSecretRecoveryPhrase(wallet, TEST_PASSWORD, TEST_SRP); @@ -530,111 +562,35 @@ describe('Wallet', () => { ).toStrictEqual({ subjectMetadata: {} }); }); - it('hydrates persisted subject metadata, consulting the wired PermissionController for retention', () => { - // Constructing the controller from persisted state calls - // `PermissionController:hasPermissions`, so this proves the default - // wiring initializes `PermissionController` before - // `SubjectMetadataController` (otherwise construction would throw). - const origin = 'https://metamask.io'; - - const wallet = new Wallet({ - state: { - PermissionController: { - subjects: { - [origin]: { origin, permissions: { somePermission: {} } }, - }, - }, - SubjectMetadataController: { - subjectMetadata: { - [origin]: { - origin, - name: 'MetaMask', - subjectType: null, - extensionId: null, - iconUrl: null, - }, - }, - }, - }, - instanceOptions: { - connectivityController: { - connectivityAdapter: new AlwaysOnlineAdapter(), - }, - gasFeeController: { - clientId: 'test', - }, - networkController: { - infuraProjectId: 'fake-infura-project-id', - }, - storageService: { - storage: new InMemoryStorageAdapter(), - }, - remoteFeatureFlagController: REMOTE_FEATURE_FLAG_OPTIONS, - }, - }); - - // The subject holds permissions, so its metadata is retained on hydration. - expect( - Object.keys(wallet.state.SubjectMetadataController.subjectMetadata), - ).toContain(origin); - }); - - it('is constructed after the default PermissionController even when overridden', () => { - // An override hydrates via `PermissionController:hasPermissions`, so it - // must run after the default `PermissionController`, not ahead of it. - const origin = 'https://metamask.io'; - - let wallet: Wallet | undefined; - - expect(() => { - wallet = new Wallet({ - initializationConfigurations: [ - subjectMetadataController as InitializationConfiguration< - unknown, - unknown - >, - ], - state: { - PermissionController: { - subjects: { - [origin]: { origin, permissions: { somePermission: {} } }, - }, - }, - SubjectMetadataController: { - subjectMetadata: { - [origin]: { - origin, - name: 'MetaMask', - subjectType: null, - extensionId: null, - iconUrl: null, - }, - }, - }, - }, - instanceOptions: { - connectivityController: { - connectivityAdapter: new AlwaysOnlineAdapter(), - }, - gasFeeController: { - clientId: 'test', - }, - networkController: { - infuraProjectId: 'fake-infura-project-id', - }, - storageService: { - storage: new InMemoryStorageAdapter(), - }, - remoteFeatureFlagController: REMOTE_FEATURE_FLAG_OPTIONS, - }, + // Hydrating persisted metadata calls `PermissionController:hasPermissions`, + // so construction throws unless `PermissionController` came first. + it.each([ + { + description: 'from the defaults', + initializationConfigurations: undefined, + }, + { + description: 'when overridden', + initializationConfigurations: [ + subjectMetadataController as InitializationConfiguration< + unknown, + unknown + >, + ], + }, + ])( + 'hydrates persisted subject metadata $description, consulting the wired PermissionController for retention', + ({ initializationConfigurations }) => { + const wallet = createWallet({ + initializationConfigurations, + state: PERSISTED_PERMISSIONED_SUBJECT_STATE, }); - }).not.toThrow(); - expect( - Object.keys( - (wallet as Wallet).state.SubjectMetadataController.subjectMetadata, - ), - ).toContain(origin); - }); + // The subject holds permissions, so its metadata survives hydration. + expect( + Object.keys(wallet.state.SubjectMetadataController.subjectMetadata), + ).toContain(PERSISTED_SUBJECT_ORIGIN); + }, + ); }); }); diff --git a/packages/wallet/src/index.ts b/packages/wallet/src/index.ts index 50e9b004160..1c85a0fcfd5 100644 --- a/packages/wallet/src/index.ts +++ b/packages/wallet/src/index.ts @@ -2,6 +2,7 @@ export { Wallet } from './Wallet.js'; export { AlwaysOnlineAdapter } from './initialization/instances/connectivity-controller/always-online-adapter.js'; export { importSecretRecoveryPhrase } from './utilities.js'; export type { WalletOptions } from './types.js'; +export type { InitializationConfiguration } from './initialization/types.js'; export type { DefaultActions, DefaultEvents, diff --git a/packages/wallet/src/initialization/initialization.test.ts b/packages/wallet/src/initialization/initialization.test.ts new file mode 100644 index 00000000000..a6f07b1499f --- /dev/null +++ b/packages/wallet/src/initialization/initialization.test.ts @@ -0,0 +1,213 @@ +import { Messenger } from '@metamask/messenger'; +import { InMemoryStorageAdapter } from '@metamask/storage-service'; + +import type { InstanceSpecificOptions } from '../types.js'; +import type { + DefaultActions, + DefaultEvents, + RootMessenger, +} from './defaults.js'; +import { defaultConfigurations } from './defaults.js'; +import { initialize } from './initialization.js'; +import { AlwaysOnlineAdapter } from './instances/connectivity-controller/always-online-adapter.js'; +import type { InitializationConfiguration } from './types.js'; + +type StubInstance = { state: Record }; + +/** + * The names of every default configuration, in the order `initialize` builds + * them. + * + * @returns The default configuration names. + */ +function getDefaultNames(): string[] { + return Object.values(defaultConfigurations).map((config) => config.name); +} + +/** + * Creates a stand-in configuration that records when it is constructed, so a + * test can assert construction order without building real controllers. + * + * @param name - The instance name. + * @param constructionOrder - An array each `init` call appends its name to. + * @returns A configuration usable as an override or an addition. + */ +function createStubConfiguration( + name: string, + constructionOrder: string[], +): InitializationConfiguration { + return { + name, + getMessenger: (): Messenger => new Messenger({ namespace: name }), + init: (): StubInstance => { + constructionOrder.push(name); + return { state: {} }; + }, + }; +} + +/** + * Builds the required instance options. Every test here overrides all defaults + * with stubs, so these values are never read; they exist to satisfy the type. + * + * @returns The instance options. + */ +function getInstanceOptions(): InstanceSpecificOptions { + return { + connectivityController: { + connectivityAdapter: new AlwaysOnlineAdapter(), + }, + gasFeeController: { clientId: 'test' }, + networkController: { infuraProjectId: 'fake-infura-project-id' }, + storageService: { storage: new InMemoryStorageAdapter() }, + remoteFeatureFlagController: { + clientConfigApiService: { + fetchRemoteFeatureFlags: async () => ({ + remoteFeatureFlags: {}, + cacheTimestamp: 0, + }), + }, + }, + }; +} + +/** + * Creates a root messenger for use in tests. + * + * @returns A root messenger. + */ +function getRootMessenger(): RootMessenger { + return new Messenger({ namespace: 'Root' }); +} + +describe('initialize', () => { + describe('default construction order', () => { + it('exports `PermissionController` before `SubjectMetadataController`', () => { + // Pins the export order of `instances/index.ts`, which the hydration + // dependency relies on and which no lint rule enforces. + const names = getDefaultNames(); + + expect(names.indexOf('PermissionController')).toBeLessThan( + names.indexOf('SubjectMetadataController'), + ); + }); + }); + + describe('overriding configurations', () => { + it('initializes an override in its default position, not in the caller-supplied order', () => { + const constructionOrder: string[] = []; + const defaultNames = getDefaultNames(); + // Reversed, so caller order and default order cannot coincide. + const overrides = [...defaultNames] + .reverse() + .map((name) => createStubConfiguration(name, constructionOrder)); + + initialize({ + messenger: getRootMessenger(), + initializationConfigurations: overrides, + instanceOptions: getInstanceOptions(), + }); + + expect(constructionOrder).toStrictEqual(defaultNames); + }); + + it('keeps `PermissionController` before `SubjectMetadataController` when both are overridden in reverse', () => { + const constructionOrder: string[] = []; + const overrides = [ + createStubConfiguration('SubjectMetadataController', constructionOrder), + createStubConfiguration('PermissionController', constructionOrder), + ]; + + initialize({ + messenger: getRootMessenger(), + initializationConfigurations: [ + ...getDefaultNames() + .filter( + (name) => + name !== 'PermissionController' && + name !== 'SubjectMetadataController', + ) + .map((name) => createStubConfiguration(name, constructionOrder)), + ...overrides, + ], + instanceOptions: getInstanceOptions(), + }); + + expect(constructionOrder.indexOf('PermissionController')).toBeLessThan( + constructionOrder.indexOf('SubjectMetadataController'), + ); + }); + + it('replaces the default instance with the override', () => { + const overridden = { state: { overridden: true } }; + + const instances = initialize({ + messenger: getRootMessenger(), + initializationConfigurations: getDefaultNames().map((name) => ({ + name, + getMessenger: (): Messenger => + new Messenger({ namespace: name }), + init: (): StubInstance => overridden, + })), + instanceOptions: getInstanceOptions(), + }); + + expect(instances.PermissionController).toBe(overridden); + }); + }); + + describe('additional configurations', () => { + it('initializes configurations that do not override a default before the defaults', () => { + const constructionOrder: string[] = []; + + initialize({ + messenger: getRootMessenger(), + initializationConfigurations: [ + ...getDefaultNames().map((name) => + createStubConfiguration(name, constructionOrder), + ), + createStubConfiguration('TestService', constructionOrder), + ], + instanceOptions: getInstanceOptions(), + }); + + expect(constructionOrder[0]).toBe('TestService'); + }); + }); + + describe('duplicate names', () => { + it('throws when two configurations share a name', () => { + const constructionOrder: string[] = []; + + expect(() => + initialize({ + messenger: getRootMessenger(), + initializationConfigurations: [ + createStubConfiguration('PermissionController', constructionOrder), + createStubConfiguration('PermissionController', constructionOrder), + ], + instanceOptions: getInstanceOptions(), + }), + ).toThrow( + 'Duplicate initialization configuration name: PermissionController', + ); + }); + + it('throws before constructing anything', () => { + const constructionOrder: string[] = []; + + expect(() => + initialize({ + messenger: getRootMessenger(), + initializationConfigurations: [ + createStubConfiguration('TestService', constructionOrder), + createStubConfiguration('TestService', constructionOrder), + ], + instanceOptions: getInstanceOptions(), + }), + ).toThrow('Duplicate initialization configuration name: TestService'); + + expect(constructionOrder).toStrictEqual([]); + }); + }); +}); diff --git a/packages/wallet/src/initialization/initialization.ts b/packages/wallet/src/initialization/initialization.ts index 8564a1a5373..4d969b2d0b4 100644 --- a/packages/wallet/src/initialization/initialization.ts +++ b/packages/wallet/src/initialization/initialization.ts @@ -29,26 +29,34 @@ export function initialize(options: InitializeOptions): DefaultInstances { defaultConfigurations, ) as InitializationConfiguration[]; - const overrideConfigurationsByName = new Map( - initializationConfigurations.map((config) => [config.name, config]), - ); - const defaultConfigurationNames = new Set( - defaultConfigurationEntries.map((config) => config.name), - ); + // Resolving two configurations with the same name would mean silently + // discarding one, so reject it instead of picking a winner. + const seenNames = new Set(); + for (const { name } of initializationConfigurations) { + if (seenNames.has(name)) { + throw new Error(`Duplicate initialization configuration name: ${name}`); + } + seenNames.add(name); + } - // Overrides keep their default's position so construction order between - // defaults is preserved (e.g. `PermissionController` before - // `SubjectMetadataController`). Non-default configs are additive and run first. - const additionalConfigurations = initializationConfigurations.filter( - (config) => !defaultConfigurationNames.has(config.name), + const defaultNames = new Set( + defaultConfigurationEntries.map((config) => config.name), ); - const mergedDefaultConfigurations = defaultConfigurationEntries.map( - (config) => overrideConfigurationsByName.get(config.name) ?? config, + const overridesByName = new Map( + initializationConfigurations.map((config) => [config.name, config]), ); - const configurationEntries = additionalConfigurations.concat( - mergedDefaultConfigurations, - ); + const configurationEntries = [ + // A configuration that does not override a default is additive, and runs + // before the defaults — a default may depend on an action it registers. + ...initializationConfigurations.filter( + (config) => !defaultNames.has(config.name), + ), + // An override takes its default's slot; see `instances/index.ts`. + ...defaultConfigurationEntries.map( + (config) => overridesByName.get(config.name) ?? config, + ), + ]; const instances: Record = {}; diff --git a/packages/wallet/src/initialization/instances/index.ts b/packages/wallet/src/initialization/instances/index.ts index 6e3e88ff278..bf31342e052 100644 --- a/packages/wallet/src/initialization/instances/index.ts +++ b/packages/wallet/src/initialization/instances/index.ts @@ -1,3 +1,7 @@ +// `initialize` constructs defaults in this order, and some depend on an earlier +// one (`PermissionController` before `SubjectMetadataController`). Keep it +// alphabetical: the ESM build sorts namespace keys per spec, but the CommonJS +// build preserves the order below, so only declaration order holds for both. export { accountsController } from './accounts-controller/accounts-controller.js'; export { addressBookController } from './address-book-controller/address-book-controller.js'; export { approvalController } from './approval-controller/approval-controller.js'; diff --git a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts index 5459555067a..d73f57a074e 100644 --- a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts +++ b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts @@ -169,6 +169,40 @@ describe('permissionController', () => { }); }); + it('has every external action it declares delegated to its messenger', () => { + const messenger = permissionController.getMessenger(getRootMessenger()); + + // A delegated action with no handler reports "has not been registered"; + // one missing from the allowlist reports "has not been delegated to". A + // dropped entry still type-checks, so only this distinction catches it. + expect(() => + messenger.call( + 'ApprovalController:addRequest', + { origin: 'https://metamask.io', type: 'test' }, + false, + ), + ).toThrow('has not been registered'); + expect(() => + messenger.call('ApprovalController:hasRequest', { id: 'test-id' }), + ).toThrow('has not been registered'); + expect(() => + messenger.call('ApprovalController:acceptRequest', 'test-id'), + ).toThrow('has not been registered'); + expect(() => + messenger.call( + 'ApprovalController:rejectRequest', + 'test-id', + new Error('rejected'), + ), + ).toThrow('has not been registered'); + expect(() => + messenger.call( + 'SubjectMetadataController:getSubjectMetadata', + 'https://metamask.io', + ), + ).toThrow('has not been registered'); + }); + it('can reach the actions delegated to its messenger', () => { const rootMessenger = getRootMessenger(); diff --git a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts index 67cc9a6890c..07c2eba0138 100644 --- a/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts +++ b/packages/wallet/src/initialization/instances/permission-controller/permission-controller.ts @@ -37,11 +37,9 @@ export const permissionController: InitializationConfiguration< parent, }); - // Only what `PermissionControllerMessenger` declares. Permission - // specifications whose side effects call further actions (e.g. the Snaps - // specifications, which reach `SnapController:*`) need a wider allowlist - // than this, so such a consumer must override this configuration rather - // than rely on the default. + // Snaps specifications reach `SnapController:*` through side effects, which + // `DefaultActions` does not declare — so widening this allowlist means + // replacing the whole configuration, not appending to it. parent.delegate({ messenger: permissionControllerMessenger, actions: [ diff --git a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts index 728cb711d4e..49c26eab602 100644 --- a/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts +++ b/packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts @@ -11,12 +11,7 @@ import type { } from '../../defaults.js'; import type { InitializationConfiguration } from '../../types.js'; -/** - * Default maximum number of distinct permissionless subjects to cache metadata - * for before the oldest is evicted. `100` matches the value MetaMask clients - * currently use; clients can override via - * `instanceOptions.subjectMetadataController.subjectCacheLimit`. - */ +// `100` matches the value both the extension and mobile use. const DEFAULT_SUBJECT_CACHE_LIMIT = 100; export const subjectMetadataController: InitializationConfiguration< @@ -24,6 +19,8 @@ export const subjectMetadataController: InitializationConfiguration< SubjectMetadataControllerMessenger > = { name: 'SubjectMetadataController', + // Hydrating persisted metadata calls `PermissionController:hasPermissions`; + // see the ordering note in `instances/index.ts`. init: ({ state, messenger, options }) => new SubjectMetadataController({ state, @@ -38,12 +35,6 @@ export const subjectMetadataController: InitializationConfiguration< parent, }); - // Hydration calls `PermissionController:hasPermissions`, so - // `PermissionController` must be constructed first. `initialize` builds - // defaults from `Object.values(defaultConfigurations)`, whose keys a module - // namespace object always sorts alphabetically — so `permissionController` - // precedes `subjectMetadataController` regardless of declaration order — - // and `initialize` keeps that position under overrides. parent.delegate({ messenger: subjectMetadataControllerMessenger, actions: ['PermissionController:hasPermissions'], diff --git a/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts b/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts index bcfa1d134ce..6032f4aba9d 100644 --- a/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts +++ b/packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts @@ -1,15 +1,12 @@ -import type { SubjectMetadataController } from '@metamask/permission-controller'; - /** * Per-instance options for the wallet's `SubjectMetadataController`. */ export type SubjectMetadataControllerInstanceOptions = { /** * Maximum number of distinct permissionless subjects (origins) to retain - * metadata for, evicted oldest-first once exceeded. Defaults to a - * platform-agnostic value when omitted. + * metadata for, evicted oldest-first once exceeded. Subjects that hold + * permissions are never evicted. Defaults to `100`. Must be a positive + * integer or the controller throws, failing wallet construction. */ - subjectCacheLimit?: ConstructorParameters< - typeof SubjectMetadataController - >[0]['subjectCacheLimit']; + subjectCacheLimit?: number; };