[base-controller] Enforce that RestrictedControllerMessenger is n…

…ot 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>
MetaMask · Nov 22, 2023 · fda4269 · fda4269
1 parent be55ceb
commit fda4269
Show file tree

Hide file tree

Showing 17 changed files with 50 additions and 101 deletions.
diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts
@@ -194,12 +194,6 @@ function buildAccountsControllerMessenger(messenger = buildMessenger()) {
       'KeyringController:getAccounts',
       'KeyringController:getKeyringForAccount',
       'KeyringController:getKeyringsByType',
-      'AccountsController:listAccounts',
-      'AccountsController:setAccountName',
-      'AccountsController:setSelectedAccount',
-      'AccountsController:updateAccounts',
-      'AccountsController:getSelectedAccount',
-      'AccountsController:getAccountByAddress',
     ],
   });
 }

diff --git a/packages/assets-controllers/src/AssetsContractController.test.ts b/packages/assets-controllers/src/AssetsContractController.test.ts
@@ -52,11 +52,6 @@ async function setupAssetContractControllers() {
   const messenger: NetworkControllerMessenger =
     new ControllerMessenger().getRestricted({
       name: 'NetworkController',
-      allowedEvents: [
-        'NetworkController:stateChange',
-        'NetworkController:networkDidChange',
-      ],
-      allowedActions: [],
     });
   const network = new NetworkController({
     infuraProjectId: networkClientConfiguration.infuraProjectId,

diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts
@@ -165,7 +165,6 @@ function setupController({
 
   const approvalControllerMessenger = messenger.getRestricted({
     name: 'ApprovalController',
-    allowedActions: ['ApprovalController:addRequest'],
   });
 
   const approvalController = new ApprovalController({

diff --git a/packages/assets-controllers/src/TokenBalancesController.test.ts b/packages/assets-controllers/src/TokenBalancesController.test.ts
@@ -123,8 +123,6 @@ describe('TokenBalancesController', () => {
     const messenger: NetworkControllerMessenger =
       new ControllerMessenger().getRestricted({
         name: 'NetworkController',
-        allowedEvents: ['NetworkController:stateChange'],
-        allowedActions: [],
       });
 
     new NetworkController({

diff --git a/packages/assets-controllers/src/TokenDetectionController.test.ts b/packages/assets-controllers/src/TokenDetectionController.test.ts
@@ -109,10 +109,7 @@ const setupTokenListController = (
   const tokenListMessenger = controllerMessenger.getRestricted({
     name: 'TokenListController',
     allowedActions: [],
-    allowedEvents: [
-      'TokenListController:stateChange',
-      'NetworkController:stateChange',
-    ],
+    allowedEvents: ['NetworkController:stateChange'],
   });
 
   const tokenList = new TokenListController({

diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts
@@ -591,10 +591,7 @@ const getRestrictedMessenger = (
   const messenger = controllerMessenger.getRestricted({
     name,
     allowedActions: ['NetworkController:getNetworkClientById'],
-    allowedEvents: [
-      'TokenListController:stateChange',
-      'NetworkController:stateChange',
-    ],
+    allowedEvents: ['NetworkController:stateChange'],
   });
 
   return messenger;

diff --git a/packages/assets-controllers/src/TokensController.test.ts b/packages/assets-controllers/src/TokensController.test.ts
@@ -69,7 +69,6 @@ describe('TokensController', () => {
 
   const approvalControllerMessenger = messenger.getRestricted({
     name: 'ApprovalController',
-    allowedActions: ['ApprovalController:addRequest'],
   });
 
   const approvalController = new ApprovalController({

diff --git a/packages/base-controller/src/ControllerMessenger.ts b/packages/base-controller/src/ControllerMessenger.ts
@@ -86,11 +86,16 @@ type EventSubscriptionMap<
  * @template Namespace - The namespace we're checking for.
  * @template Name - The full string, including the namespace.
  */
-export type Namespaced<
+export type NamespacedBy<
   Namespace extends string,
   Name,
 > = Name extends `${Namespace}:${string}` ? Name : never;
 
+export type NotNamespacedBy<
+  Namespace extends string,
+  Name,
+> = Name extends `${Namespace}:${string}` ? never : Name;
+
 export type NamespacedName<Namespace extends string = string> =
   `${Namespace}:${string}`;
 
@@ -379,21 +384,29 @@ export class ControllerMessenger<
    * module that this messenger has been created for. The authority to publish events and register
    * actions under this namespace is granted to this restricted messenger instance.
    * @template AllowedAction - A type union of the 'type' string for any allowed actions.
+   * This must not include internal actions that are in the messenger's namespace.
    * @template AllowedEvent - A type union of the 'type' string for any allowed events.
+   * This must not include internal events that are in the messenger's namespace.
    * @returns The restricted controller messenger.
    */
   getRestricted<
     Namespace extends string,
-    AllowedAction extends string,
-    AllowedEvent extends string,
+    AllowedAction extends NotNamespacedBy<Namespace, Action['type']>,
+    AllowedEvent extends NotNamespacedBy<Namespace, Event['type']>,
   >({
     name,
     allowedActions,
     allowedEvents,
   }: {
     name: Namespace;
-    allowedActions?: Extract<Action['type'], AllowedAction>[];
-    allowedEvents?: Extract<Event['type'], AllowedEvent>[];
+    allowedActions?: NotNamespacedBy<
+      Namespace,
+      Extract<Action['type'], AllowedAction>
+    >[];
+    allowedEvents?: NotNamespacedBy<
+      Namespace,
+      Extract<Event['type'], AllowedEvent>
+    >[];
   }): RestrictedControllerMessenger<
     Namespace,
     | NarrowToNamespace<Action, Namespace>

diff --git a/packages/base-controller/src/RestrictedControllerMessenger.ts b/packages/base-controller/src/RestrictedControllerMessenger.ts
@@ -8,6 +8,7 @@ import type {
   ExtractEventHandler,
   ExtractEventPayload,
   NamespacedName,
+  NotNamespacedBy,
   SelectorEventHandler,
   SelectorFunction,
 } from './ControllerMessenger';
@@ -24,7 +25,9 @@ import type {
  * @template Action - A type union of all Action types.
  * @template Event - A type union of all Event types.
  * @template AllowedAction - A type union of the 'type' string for any allowed actions.
+ * This must not include internal actions that are in the messenger's namespace.
  * @template AllowedEvent - A type union of the 'type' string for any allowed events.
+ * This must not include internal events that are in the messenger's namespace.
  */
 export class RestrictedControllerMessenger<
   Namespace extends string,
@@ -40,9 +43,9 @@ export class RestrictedControllerMessenger<
 
   readonly #controllerName: Namespace;
 
-  readonly #allowedActions: AllowedAction[] | null;
+  readonly #allowedActions: NotNamespacedBy<Namespace, AllowedAction>[] | null;
 
-  readonly #allowedEvents: AllowedEvent[] | null;
+  readonly #allowedEvents: NotNamespacedBy<Namespace, AllowedEvent>[] | null;
 
   /**
    * Constructs a restricted controller messenger
@@ -70,13 +73,13 @@ export class RestrictedControllerMessenger<
   }: {
     controllerMessenger: ControllerMessenger<ActionConstraint, EventConstraint>;
     name: Namespace;
-    allowedActions?: AllowedAction[];
-    allowedEvents?: AllowedEvent[];
+    allowedActions?: NotNamespacedBy<Namespace, AllowedAction>[];
+    allowedEvents?: NotNamespacedBy<Namespace, AllowedEvent>[];
   }) {
     this.#controllerMessenger = controllerMessenger;
     this.#controllerName = name;
-    this.#allowedActions = allowedActions || null;
-    this.#allowedEvents = allowedEvents || null;
+    this.#allowedActions = allowedActions ?? null;
+    this.#allowedEvents = allowedEvents ?? null;
   }
 
   /**
@@ -226,6 +229,7 @@ export class RestrictedControllerMessenger<
    * @param selector - The selector function used to select relevant data from
    * the event payload. The type of the parameters for this selector must match
    * the type of the payload for this event type.
+   * @throws Will throw if the given event is not an allowed event for this controller messenger.
    * @template EventType - A type union of Event type strings.
    * @template SelectorReturnValue - The selector return value.
    */
@@ -275,7 +279,7 @@ export class RestrictedControllerMessenger<
    *
    * @param event - The event type. This is a unique identifier for this event.
    * @param handler - The event handler to unregister.
-   * @throws Will throw when the given event is not an allowed event for this controller messenger.
+   * @throws Will throw if the given event is not an allowed event for this controller messenger.
    * @template EventType - A type union of allowed Event type strings.
    */
   unsubscribe<
@@ -319,7 +323,11 @@ export class RestrictedControllerMessenger<
    * @param eventType - The event type to check.
    * @returns Whether the event type is allowed.
    */
-  #isAllowedEvent(eventType: Event['type']): eventType is AllowedEvent {
+  #isAllowedEvent(
+    eventType: Event['type'],
+  ): eventType is
+    | NamespacedName<Namespace>
+    | NotNamespacedBy<Namespace, AllowedEvent> {
     // Safely upcast to allow runtime check
     const allowedEvents: string[] | null = this.#allowedEvents;
     return (
@@ -336,7 +344,11 @@ export class RestrictedControllerMessenger<
    * @param actionType - The action type to check.
    * @returns Whether the action type is allowed.
    */
-  #isAllowedAction(actionType: Action['type']): actionType is AllowedAction {
+  #isAllowedAction(
+    actionType: Action['type'],
+  ): actionType is
+    | NamespacedName<Namespace>
+    | NotNamespacedBy<Namespace, AllowedAction> {
     // Safely upcast to allow runtime check
     const allowedActions: string[] | null = this.#allowedActions;
     return (

diff --git a/packages/gas-fee-controller/src/GasFeeController.test.ts b/packages/gas-fee-controller/src/GasFeeController.test.ts
@@ -71,15 +71,8 @@ const setupNetworkController = async ({
 }) => {
   const restrictedMessenger = unrestrictedMessenger.getRestricted({
     name: 'NetworkController',
-    allowedActions: [
-      'NetworkController:getState',
-      'NetworkController:getNetworkClientById',
-      'NetworkController:getEIP1559Compatibility',
-    ],
-    allowedEvents: [
-      'NetworkController:stateChange',
-      'NetworkController:networkDidChange',
-    ],
+    allowedActions: [],
+    allowedEvents: [],
   });
 
   const networkController = new NetworkController({

diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts
@@ -2306,25 +2306,6 @@ function buildMessenger() {
 function buildKeyringControllerMessenger(messenger = buildMessenger()) {
   return messenger.getRestricted({
     name: 'KeyringController',
-    allowedActions: [
-      'KeyringController:getState',
-      'KeyringController:signMessage',
-      'KeyringController:signPersonalMessage',
-      'KeyringController:signTypedMessage',
-      'KeyringController:decryptMessage',
-      'KeyringController:getEncryptionPublicKey',
-      'KeyringController:getKeyringsByType',
-      'KeyringController:getKeyringForAccount',
-      'KeyringController:getAccounts',
-      'KeyringController:persistAllKeyrings',
-    ],
-    allowedEvents: [
-      'KeyringController:stateChange',
-      'KeyringController:lock',
-      'KeyringController:unlock',
-      'KeyringController:accountRemoved',
-      'KeyringController:qrKeyringStateChange',
-    ],
   });
 }
 

diff --git a/packages/network-controller/tests/NetworkController.test.ts b/packages/network-controller/tests/NetworkController.test.ts
@@ -7069,17 +7069,6 @@ function buildMessenger() {
 function buildNetworkControllerMessenger(messenger = buildMessenger()) {
   return messenger.getRestricted({
     name: 'NetworkController',
-    allowedActions: [
-      'NetworkController:getProviderConfig',
-      'NetworkController:getEthQuery',
-    ],
-    allowedEvents: [
-      'NetworkController:stateChange',
-      'NetworkController:infuraIsBlocked',
-      'NetworkController:infuraIsUnblocked',
-      'NetworkController:networkDidChange',
-      'NetworkController:networkWillChange',
-    ],
   });
 }
 

diff --git a/packages/permission-controller/src/PermissionController.test.ts b/packages/permission-controller/src/PermissionController.test.ts
@@ -476,8 +476,8 @@ function getPermissionControllerMessenger(
 ) {
   return messenger.getRestricted<
     typeof controllerName,
-    PermissionControllerActions['type'] | AllowedActions['type'],
-    PermissionControllerEvents['type']
+    AllowedActions['type'],
+    never
   >({
     name: controllerName,
     allowedActions: [

diff --git a/packages/permission-controller/src/SubjectMetadataController.test.ts b/packages/permission-controller/src/SubjectMetadataController.test.ts
@@ -34,14 +34,11 @@ function getSubjectMetadataControllerMessenger() {
   return [
     controllerMessenger.getRestricted<
       typeof controllerName,
-      SubjectMetadataControllerActions['type'] | HasPermissions['type'],
-      SubjectMetadataControllerEvents['type']
+      HasPermissions['type'],
+      never
     >({
       name: controllerName,
-      allowedActions: [
-        'PermissionController:hasPermissions',
-        'SubjectMetadataController:getState',
-      ],
+      allowedActions: ['PermissionController:hasPermissions'],
     }) as SubjectMetadataControllerMessenger,
     hasPermissionsSpy,
   ] as const;

diff --git a/packages/queued-request-controller/tests/utils.ts b/packages/queued-request-controller/tests/utils.ts
@@ -4,10 +4,6 @@ import type {
   QueuedRequestControllerActions,
   QueuedRequestControllerEvents,
 } from '../src/QueuedRequestController';
-import {
-  QueuedRequestControllerActionTypes,
-  QueuedRequestControllerEventTypes,
-} from '../src/QueuedRequestController';
 
 /**
  * Build a controller messenger that includes all events used by the selected network
@@ -32,7 +28,5 @@ export function buildQueuedRequestControllerMessenger(
 ) {
   return messenger.getRestricted({
     name: 'QueuedRequestController',
-    allowedActions: [...Object.values(QueuedRequestControllerActionTypes)],
-    allowedEvents: [...Object.values(QueuedRequestControllerEventTypes)],
   });
 }
diff --git a/packages/rate-limit-controller/src/RateLimitController.test.ts b/packages/rate-limit-controller/src/RateLimitController.test.ts
@@ -43,13 +43,8 @@ function getUnrestrictedMessenger() {
 function getRestrictedMessenger(
   controllerMessenger = getUnrestrictedMessenger(),
 ) {
-  return controllerMessenger.getRestricted<
-    typeof name,
-    RateLimitControllerActions<RateLimitedApis>['type'],
-    never
-  >({
+  return controllerMessenger.getRestricted<typeof name, never, never>({
     name,
-    allowedActions: ['RateLimitController:call'],
   }) as RateLimitMessenger<RateLimitedApis>;
 }
 

diff --git a/packages/selected-network-controller/tests/utils.ts b/packages/selected-network-controller/tests/utils.ts
@@ -5,7 +5,6 @@ import type {
   SelectedNetworkControllerAction,
   SelectedNetworkControllerEvent,
 } from '../src/SelectedNetworkController';
-import { SelectedNetworkControllerActionTypes } from '../src/SelectedNetworkController';
 
 /**
  * Build a controller messenger that includes all events used by the selected network
@@ -37,10 +36,7 @@ export function buildSelectedNetworkControllerMessenger(
   );
   return messenger.getRestricted({
     name: 'SelectedNetworkController',
-    allowedActions: [
-      ...Object.values(SelectedNetworkControllerActionTypes),
-      'NetworkController:getNetworkClientById',
-    ],
+    allowedActions: ['NetworkController:getNetworkClientById'],
     allowedEvents: ['NetworkController:stateChange'],
   });
 }