Skip to content

Commit fda4269

Browse files
MajorLiftmcmire
MajorLift
and
mcmire
authored
[base-controller] Enforce that RestrictedControllerMessenger is not initialized with internal actions/events in allow lists (#2051)
## Explanation - `RestrictedControllerMessenger` constructor and `ControllerMessenger.getRestricted()` method now raise a type error if an internal action is passed into `allowedActions`, or an internal event into `allowedEvents`. - ` Type 'string' is not assignable to type 'never'.` - `Type '"SomeController:internalAction"' is not assignable to type '"OtherController:externalAction1" | "OtherController:externalAction2"'.` - Fixes breaking tests in downstream controllers in core repo. ## Impact - The `allowed{Actions,Events}` arrays in `ControllerMessenger.getRestricted()` now align with the `Allowed{Actions,Events}` generic params of `RestrictedControllerMessenger`: Both only enumerate external actions/events. - This commit disallows `allowed{Actions,Events}` arrays that contain an incomplete list of internal actions/events. - A partial list doesn't provide any useful information as all internal actions/events will be available to the messenger regardless of whether any of them are allowlisted. - A partial list also gives the confusing impression that some internal actions/events can be *disallowed* through omission, which is not true as of #2050. --------- Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
1 parent be55ceb commit fda4269

17 files changed

Lines changed: 50 additions & 101 deletions

File tree

‎packages/accounts-controller/src/AccountsController.test.ts‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -194,12 +194,6 @@ function buildAccountsControllerMessenger(messenger = buildMessenger()) {
194194
'KeyringController:getAccounts',
195195
'KeyringController:getKeyringForAccount',
196196
'KeyringController:getKeyringsByType',
197-
'AccountsController:listAccounts',
198-
'AccountsController:setAccountName',
199-
'AccountsController:setSelectedAccount',
200-
'AccountsController:updateAccounts',
201-
'AccountsController:getSelectedAccount',
202-
'AccountsController:getAccountByAddress',
203197
],
204198
});
205199
}

‎packages/assets-controllers/src/AssetsContractController.test.ts‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,11 +52,6 @@ async function setupAssetContractControllers() {
5252
const messenger: NetworkControllerMessenger =
5353
new ControllerMessenger().getRestricted({
5454
name: 'NetworkController',
55-
allowedEvents: [
56-
'NetworkController:stateChange',
57-
'NetworkController:networkDidChange',
58-
],
59-
allowedActions: [],
6055
});
6156
const network = new NetworkController({
6257
infuraProjectId: networkClientConfiguration.infuraProjectId,

‎packages/assets-controllers/src/NftController.test.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,6 @@ function setupController({
165165

166166
const approvalControllerMessenger = messenger.getRestricted({
167167
name: 'ApprovalController',
168-
allowedActions: ['ApprovalController:addRequest'],
169168
});
170169

171170
const approvalController = new ApprovalController({

‎packages/assets-controllers/src/TokenBalancesController.test.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,6 @@ describe('TokenBalancesController', () => {
123123
const messenger: NetworkControllerMessenger =
124124
new ControllerMessenger().getRestricted({
125125
name: 'NetworkController',
126-
allowedEvents: ['NetworkController:stateChange'],
127-
allowedActions: [],
128126
});
129127

130128
new NetworkController({

‎packages/assets-controllers/src/TokenDetectionController.test.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,7 @@ const setupTokenListController = (
109109
const tokenListMessenger = controllerMessenger.getRestricted({
110110
name: 'TokenListController',
111111
allowedActions: [],
112-
allowedEvents: [
113-
'TokenListController:stateChange',
114-
'NetworkController:stateChange',
115-
],
112+
allowedEvents: ['NetworkController:stateChange'],
116113
});
117114

118115
const tokenList = new TokenListController({

‎packages/assets-controllers/src/TokenListController.test.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -591,10 +591,7 @@ const getRestrictedMessenger = (
591591
const messenger = controllerMessenger.getRestricted({
592592
name,
593593
allowedActions: ['NetworkController:getNetworkClientById'],
594-
allowedEvents: [
595-
'TokenListController:stateChange',
596-
'NetworkController:stateChange',
597-
],
594+
allowedEvents: ['NetworkController:stateChange'],
598595
});
599596

600597
return messenger;

‎packages/assets-controllers/src/TokensController.test.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@ describe('TokensController', () => {
6969

7070
const approvalControllerMessenger = messenger.getRestricted({
7171
name: 'ApprovalController',
72-
allowedActions: ['ApprovalController:addRequest'],
7372
});
7473

7574
const approvalController = new ApprovalController({

‎packages/base-controller/src/ControllerMessenger.ts‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -86,11 +86,16 @@ type EventSubscriptionMap<
8686
* @template Namespace - The namespace we're checking for.
8787
* @template Name - The full string, including the namespace.
8888
*/
89-
export type Namespaced<
89+
export type NamespacedBy<
9090
Namespace extends string,
9191
Name,
9292
> = Name extends `${Namespace}:${string}` ? Name : never;
9393

94+
export type NotNamespacedBy<
95+
Namespace extends string,
96+
Name,
97+
> = Name extends `${Namespace}:${string}` ? never : Name;
98+
9499
export type NamespacedName<Namespace extends string = string> =
95100
`${Namespace}:${string}`;
96101

@@ -379,21 +384,29 @@ export class ControllerMessenger<
379384
* module that this messenger has been created for. The authority to publish events and register
380385
* actions under this namespace is granted to this restricted messenger instance.
381386
* @template AllowedAction - A type union of the 'type' string for any allowed actions.
387+
* This must not include internal actions that are in the messenger's namespace.
382388
* @template AllowedEvent - A type union of the 'type' string for any allowed events.
389+
* This must not include internal events that are in the messenger's namespace.
383390
* @returns The restricted controller messenger.
384391
*/
385392
getRestricted<
386393
Namespace extends string,
387-
AllowedAction extends string,
388-
AllowedEvent extends string,
394+
AllowedAction extends NotNamespacedBy<Namespace, Action['type']>,
395+
AllowedEvent extends NotNamespacedBy<Namespace, Event['type']>,
389396
>({
390397
name,
391398
allowedActions,
392399
allowedEvents,
393400
}: {
394401
name: Namespace;
395-
allowedActions?: Extract<Action['type'], AllowedAction>[];
396-
allowedEvents?: Extract<Event['type'], AllowedEvent>[];
402+
allowedActions?: NotNamespacedBy<
403+
Namespace,
404+
Extract<Action['type'], AllowedAction>
405+
>[];
406+
allowedEvents?: NotNamespacedBy<
407+
Namespace,
408+
Extract<Event['type'], AllowedEvent>
409+
>[];
397410
}): RestrictedControllerMessenger<
398411
Namespace,
399412
| NarrowToNamespace<Action, Namespace>

‎packages/base-controller/src/RestrictedControllerMessenger.ts‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import type {
88
ExtractEventHandler,
99
ExtractEventPayload,
1010
NamespacedName,
11+
NotNamespacedBy,
1112
SelectorEventHandler,
1213
SelectorFunction,
1314
} from './ControllerMessenger';
@@ -24,7 +25,9 @@ import type {
2425
* @template Action - A type union of all Action types.
2526
* @template Event - A type union of all Event types.
2627
* @template AllowedAction - A type union of the 'type' string for any allowed actions.
28+
* This must not include internal actions that are in the messenger's namespace.
2729
* @template AllowedEvent - A type union of the 'type' string for any allowed events.
30+
* This must not include internal events that are in the messenger's namespace.
2831
*/
2932
export class RestrictedControllerMessenger<
3033
Namespace extends string,
@@ -40,9 +43,9 @@ export class RestrictedControllerMessenger<
4043

4144
readonly #controllerName: Namespace;
4245

43-
readonly #allowedActions: AllowedAction[] | null;
46+
readonly #allowedActions: NotNamespacedBy<Namespace, AllowedAction>[] | null;
4447

45-
readonly #allowedEvents: AllowedEvent[] | null;
48+
readonly #allowedEvents: NotNamespacedBy<Namespace, AllowedEvent>[] | null;
4649

4750
/**
4851
* Constructs a restricted controller messenger
@@ -70,13 +73,13 @@ export class RestrictedControllerMessenger<
7073
}: {
7174
controllerMessenger: ControllerMessenger<ActionConstraint, EventConstraint>;
7275
name: Namespace;
73-
allowedActions?: AllowedAction[];
74-
allowedEvents?: AllowedEvent[];
76+
allowedActions?: NotNamespacedBy<Namespace, AllowedAction>[];
77+
allowedEvents?: NotNamespacedBy<Namespace, AllowedEvent>[];
7578
}) {
7679
this.#controllerMessenger = controllerMessenger;
7780
this.#controllerName = name;
78-
this.#allowedActions = allowedActions || null;
79-
this.#allowedEvents = allowedEvents || null;
81+
this.#allowedActions = allowedActions ?? null;
82+
this.#allowedEvents = allowedEvents ?? null;
8083
}
8184

8285
/**
@@ -226,6 +229,7 @@ export class RestrictedControllerMessenger<
226229
* @param selector - The selector function used to select relevant data from
227230
* the event payload. The type of the parameters for this selector must match
228231
* the type of the payload for this event type.
232+
* @throws Will throw if the given event is not an allowed event for this controller messenger.
229233
* @template EventType - A type union of Event type strings.
230234
* @template SelectorReturnValue - The selector return value.
231235
*/
@@ -275,7 +279,7 @@ export class RestrictedControllerMessenger<
275279
*
276280
* @param event - The event type. This is a unique identifier for this event.
277281
* @param handler - The event handler to unregister.
278-
* @throws Will throw when the given event is not an allowed event for this controller messenger.
282+
* @throws Will throw if the given event is not an allowed event for this controller messenger.
279283
* @template EventType - A type union of allowed Event type strings.
280284
*/
281285
unsubscribe<
@@ -319,7 +323,11 @@ export class RestrictedControllerMessenger<
319323
* @param eventType - The event type to check.
320324
* @returns Whether the event type is allowed.
321325
*/
322-
#isAllowedEvent(eventType: Event['type']): eventType is AllowedEvent {
326+
#isAllowedEvent(
327+
eventType: Event['type'],
328+
): eventType is
329+
| NamespacedName<Namespace>
330+
| NotNamespacedBy<Namespace, AllowedEvent> {
323331
// Safely upcast to allow runtime check
324332
const allowedEvents: string[] | null = this.#allowedEvents;
325333
return (
@@ -336,7 +344,11 @@ export class RestrictedControllerMessenger<
336344
* @param actionType - The action type to check.
337345
* @returns Whether the action type is allowed.
338346
*/
339-
#isAllowedAction(actionType: Action['type']): actionType is AllowedAction {
347+
#isAllowedAction(
348+
actionType: Action['type'],
349+
): actionType is
350+
| NamespacedName<Namespace>
351+
| NotNamespacedBy<Namespace, AllowedAction> {
340352
// Safely upcast to allow runtime check
341353
const allowedActions: string[] | null = this.#allowedActions;
342354
return (

‎packages/gas-fee-controller/src/GasFeeController.test.ts‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -71,15 +71,8 @@ const setupNetworkController = async ({
7171
}) => {
7272
const restrictedMessenger = unrestrictedMessenger.getRestricted({
7373
name: 'NetworkController',
74-
allowedActions: [
75-
'NetworkController:getState',
76-
'NetworkController:getNetworkClientById',
77-
'NetworkController:getEIP1559Compatibility',
78-
],
79-
allowedEvents: [
80-
'NetworkController:stateChange',
81-
'NetworkController:networkDidChange',
82-
],
74+
allowedActions: [],
75+
allowedEvents: [],
8376
});
8477

8578
const networkController = new NetworkController({

0 commit comments

Comments
 (0)