Skip to content

Commit 1762832

Browse files
sirtimidclaude
andcommitted
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) <noreply@anthropic.com>
1 parent d6c1b20 commit 1762832

5 files changed

Lines changed: 175 additions & 18 deletions

File tree

packages/wallet/src/Wallet.test.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -417,4 +417,70 @@ describe('Wallet', () => {
417417
).toStrictEqual({ testFlag: true });
418418
});
419419
});
420+
421+
describe('PermissionController', () => {
422+
it('is wired and exposes its state on the wallet messenger', async () => {
423+
const wallet = await setupWallet();
424+
425+
expect(
426+
wallet.messenger.call('PermissionController:getState'),
427+
).toStrictEqual({ subjects: {} });
428+
});
429+
});
430+
431+
describe('SubjectMetadataController', () => {
432+
it('is wired and exposes its state on the wallet messenger', async () => {
433+
const wallet = await setupWallet();
434+
435+
expect(
436+
wallet.messenger.call('SubjectMetadataController:getState'),
437+
).toStrictEqual({ subjectMetadata: {} });
438+
});
439+
440+
it('hydrates persisted subject metadata, consulting the wired PermissionController for retention', () => {
441+
// Constructing the controller from persisted state calls
442+
// `PermissionController:hasPermissions`, so this proves the default
443+
// wiring initializes `PermissionController` before
444+
// `SubjectMetadataController` (otherwise construction would throw).
445+
const origin = 'https://metamask.io';
446+
447+
const wallet = new Wallet({
448+
state: {
449+
PermissionController: {
450+
subjects: {
451+
[origin]: { origin, permissions: { somePermission: {} } },
452+
},
453+
},
454+
SubjectMetadataController: {
455+
subjectMetadata: {
456+
[origin]: {
457+
origin,
458+
name: 'MetaMask',
459+
subjectType: null,
460+
extensionId: null,
461+
iconUrl: null,
462+
},
463+
},
464+
},
465+
},
466+
instanceOptions: {
467+
connectivityController: {
468+
connectivityAdapter: new AlwaysOnlineAdapter(),
469+
},
470+
networkController: {
471+
infuraProjectId: 'fake-infura-project-id',
472+
},
473+
storageService: {
474+
storage: new InMemoryStorageAdapter(),
475+
},
476+
remoteFeatureFlagController: REMOTE_FEATURE_FLAG_OPTIONS,
477+
},
478+
});
479+
480+
// The subject holds permissions, so its metadata is retained on hydration.
481+
expect(
482+
Object.keys(wallet.state.SubjectMetadataController.subjectMetadata),
483+
).toContain(origin);
484+
});
485+
});
420486
});

packages/wallet/src/initialization/instances/permission-controller/permission-controller.test.ts

Lines changed: 71 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import { Messenger } from '@metamask/messenger';
2-
import { PermissionController } from '@metamask/permission-controller';
2+
import {
3+
PermissionController,
4+
PermissionType,
5+
} from '@metamask/permission-controller';
36

47
import { defaultConfigurations } from '../../defaults';
58
import type {
@@ -58,23 +61,48 @@ describe('permissionController', () => {
5861
expect(instance.state.subjects).toStrictEqual(subjects);
5962
});
6063

61-
it('forwards injected specifications and unrestricted methods', () => {
64+
it('forwards injected unrestrictedMethods to the controller', () => {
6265
const messenger = permissionController.getMessenger(getRootMessenger());
6366

6467
const instance = permissionController.init({
6568
state: undefined,
6669
messenger,
67-
options: {
68-
caveatSpecifications: {},
69-
permissionSpecifications: {},
70-
unrestrictedMethods: ['eth_chainId', 'eth_blockNumber'],
71-
},
70+
options: { unrestrictedMethods: ['eth_chainId', 'eth_blockNumber'] },
7271
});
7372

7473
expect(instance.hasUnrestrictedMethod('eth_chainId')).toBe(true);
7574
expect(instance.hasUnrestrictedMethod('eth_sendTransaction')).toBe(false);
7675
});
7776

77+
it('forwards injected permission specifications to the controller', () => {
78+
const messenger = permissionController.getMessenger(getRootMessenger());
79+
const origin = 'https://metamask.io';
80+
81+
const instance = permissionController.init({
82+
state: undefined,
83+
messenger,
84+
options: {
85+
permissionSpecifications: {
86+
wallet_noop: {
87+
permissionType: PermissionType.RestrictedMethod,
88+
targetName: 'wallet_noop',
89+
allowedCaveats: null,
90+
methodImplementation: () => null,
91+
},
92+
},
93+
},
94+
});
95+
96+
// Granting the injected permission only succeeds if its specification was
97+
// forwarded to the controller; an unknown target would throw.
98+
instance.grantPermissions({
99+
subject: { origin },
100+
approvedPermissions: { wallet_noop: {} },
101+
});
102+
103+
expect(instance.getPermissions(origin)).toHaveProperty('wallet_noop');
104+
});
105+
78106
it('exposes its actions through the root messenger', () => {
79107
const rootMessenger = getRootMessenger();
80108
const messenger = permissionController.getMessenger(rootMessenger);
@@ -85,4 +113,40 @@ describe('permissionController', () => {
85113
subjects: {},
86114
});
87115
});
116+
117+
it('can reach the actions delegated to its messenger', () => {
118+
const rootMessenger = getRootMessenger();
119+
120+
// Register stub handlers as the real ApprovalController and
121+
// SubjectMetadataController would, then confirm the PermissionController's
122+
// messenger can call them — proving the delegation allowlist is wired.
123+
const approvalControllerMessenger = new Messenger({
124+
namespace: 'ApprovalController',
125+
parent: rootMessenger,
126+
});
127+
approvalControllerMessenger.registerActionHandler(
128+
'ApprovalController:hasRequest',
129+
() => true,
130+
);
131+
const subjectMetadataControllerMessenger = new Messenger({
132+
namespace: 'SubjectMetadataController',
133+
parent: rootMessenger,
134+
});
135+
subjectMetadataControllerMessenger.registerActionHandler(
136+
'SubjectMetadataController:getSubjectMetadata',
137+
() => undefined,
138+
);
139+
140+
const messenger = permissionController.getMessenger(rootMessenger);
141+
142+
expect(messenger.call('ApprovalController:hasRequest', { id: 'x' })).toBe(
143+
true,
144+
);
145+
expect(
146+
messenger.call(
147+
'SubjectMetadataController:getSubjectMetadata',
148+
'https://metamask.io',
149+
),
150+
).toBeUndefined();
151+
});
88152
});

packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.test.ts

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -116,30 +116,54 @@ describe('subjectMetadataController', () => {
116116
instance.addSubjectMetadata({ origin: 'https://a.example' });
117117
instance.addSubjectMetadata({ origin: 'https://b.example' });
118118

119-
// With a cache limit of 1 and neither subject holding permissions, the
120-
// first is evicted when the second is added.
121119
expect(Object.keys(instance.state.subjectMetadata)).toStrictEqual([
122120
'https://b.example',
123121
]);
124122
});
125123

126-
it('defaults subjectCacheLimit to 100 when omitted, retaining subjects past a small count', () => {
124+
it('retains a subject with permissions even when the cache limit is exceeded', () => {
127125
const rootMessenger = getRootMessenger();
128-
registerHasPermissionsStub(rootMessenger, false);
126+
// Every subject reports as holding permissions.
127+
registerHasPermissionsStub(rootMessenger, true);
129128
const messenger = subjectMetadataController.getMessenger(rootMessenger);
130129

131130
const instance = subjectMetadataController.init({
132131
state: undefined,
133132
messenger,
134-
options: {},
133+
options: { subjectCacheLimit: 1 },
135134
});
136135

137136
instance.addSubjectMetadata({ origin: 'https://a.example' });
138137
instance.addSubjectMetadata({ origin: 'https://b.example' });
139138

139+
// Metadata for subjects with permissions is never evicted.
140140
expect(Object.keys(instance.state.subjectMetadata)).toStrictEqual([
141141
'https://a.example',
142142
'https://b.example',
143143
]);
144144
});
145+
146+
it('does not evict until the default cache limit of 100 is exceeded', () => {
147+
const rootMessenger = getRootMessenger();
148+
registerHasPermissionsStub(rootMessenger, false);
149+
const messenger = subjectMetadataController.getMessenger(rootMessenger);
150+
151+
const instance = subjectMetadataController.init({
152+
state: undefined,
153+
messenger,
154+
options: {},
155+
});
156+
157+
for (let index = 0; index < 100; index++) {
158+
instance.addSubjectMetadata({ origin: `https://${index}.example` });
159+
}
160+
expect(Object.keys(instance.state.subjectMetadata)).toHaveLength(100);
161+
162+
// The 101st permissionless subject evicts the oldest (FIFO).
163+
instance.addSubjectMetadata({ origin: 'https://overflow.example' });
164+
const origins = Object.keys(instance.state.subjectMetadata);
165+
expect(origins).toHaveLength(100);
166+
expect(origins).not.toContain('https://0.example');
167+
expect(origins).toContain('https://overflow.example');
168+
});
145169
});

packages/wallet/src/initialization/instances/subject-metadata-controller/subject-metadata-controller.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,9 @@ import type {
1212
import type { InitializationConfiguration } from '../../types';
1313

1414
/**
15-
* Maximum number of distinct permissionless subjects to cache metadata for
16-
* before the oldest is evicted. Both the extension and mobile clients use
17-
* `100`; clients can override via
15+
* Default maximum number of distinct permissionless subjects to cache metadata
16+
* for before the oldest is evicted. `100` matches the value MetaMask clients
17+
* currently use; clients can override via
1818
* `instanceOptions.subjectMetadataController.subjectCacheLimit`.
1919
*/
2020
const DEFAULT_SUBJECT_CACHE_LIMIT = 100;
@@ -38,6 +38,10 @@ export const subjectMetadataController: InitializationConfiguration<
3838
parent,
3939
});
4040

41+
// `SubjectMetadataController` calls `PermissionController:hasPermissions`
42+
// while hydrating persisted subjects, so `PermissionController` must be
43+
// initialized first (it registers that handler in its constructor). The
44+
// alphabetical export order in `instances/index.ts` guarantees this.
4145
parent.delegate({
4246
messenger: subjectMetadataControllerMessenger,
4347
actions: ['PermissionController:hasPermissions'],

packages/wallet/src/initialization/instances/subject-metadata-controller/types.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,8 @@ import type { SubjectMetadataController } from '@metamask/permission-controller'
66
export type SubjectMetadataControllerInstanceOptions = {
77
/**
88
* Maximum number of distinct permissionless subjects (origins) to retain
9-
* metadata for. Once exceeded, the oldest permissionless subject is evicted
10-
* (FIFO). Defaults to `100` when omitted, matching the extension and mobile
11-
* clients.
9+
* metadata for, evicted oldest-first once exceeded. Defaults to a
10+
* platform-agnostic value when omitted.
1211
*/
1312
subjectCacheLimit?: ConstructorParameters<
1413
typeof SubjectMetadataController

0 commit comments

Comments
 (0)