Narrow string allowlists in RestrictedControllerMessenger instances

[MajorLift]

Motivation

Some controllers have their allowed actions or events types for their RestrictedControllerMessenger set to string. This compromises allowlist security by enabling these controllers to gain unrestricted access to any external action or event.

Explanation

• Fixes all instances in core of string being used to define RestrictedControllerMessenger allowlists.

• Includes follow-up fixes to fix: accounts controller events and actions #4021.

  • Fixes accounts-controller allowlists being included in controller actions and events unions.
  • Fixes accounts-controller allowlists being exported at package-level.

References

• Closes Narrow string allowlists in RestrictedControllerMessenger instances #4019

Changelog

@metamask/approval-controller

Fixed

• BREAKING: Narrow ApprovalControllerMessenger generic arguments AllowedActions, AllowedEvents from string to never.

@metamask/keyring-controller

Fixed

• BREAKING: Narrow KeyringControllerMessenger generic arguments AllowedActions, AllowedEvents from string to never.

@metamask/network-controller

Fixed

• BREAKING: Narrow NetworkControllerMessenger generic arguments AllowedActions, AllowedEvents from string to never.

@metamask/permission-controller

Fixed

• BREAKING: Fix SideEffectMessenger so that it's defined with a RestrictedControllerMessenger that has access to PermissionController allowed actions.
  • The messenger's Actions generic parameter is widened to include the PermissionController actions allowlist.
  • The messenger's AllowedActions generic parameter is narrowed from string to the PermissionController actions allowlist.

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[MajorLift]

We don't want allowlists to be included in the controller actions/events unions.

• This interferes with the type narrowing base-controller does to distinguish internal/external actions/events.
• This exposes allowlists externally since actions/events unions are exported at the package-level. We generally want allowlists to be scoped to its controller.

[MajorLift]

See https://github.com/MetaMask/core/pull/4031/files#r1516591041

[MajorLift]

• This is not a breaking change. The list includes all preexisting exports and excludes Allowed{Actions,Events}.
• Allowed{Actions,Events} should be exported from the module so they're available for tests and other internal files, but they should not be exported at the package level.
• We want to avoid wildcard exports in general: Forbid wildcard exports eslint-config#331

[Gudahtt]

One suggestion for the changelog, but the diff LGTM!

[MajorLift]

Failing transaction-controller tests introduced by #3827 are fixed by the following changes. Merging #4013 first should take care of this.

diff --git a/packages/transaction-controller/src/TransactionControllerIntegration.test.ts b/packages/transaction-controller/src/TransactionControllerIntegration.test.ts
index 72c91fa44..23bd723fa 100644
--- a/packages/transaction-controller/src/TransactionControllerIntegration.test.ts
+++ b/packages/transaction-controller/src/TransactionControllerIntegration.test.ts
@@ -114,7 +114,11 @@ const setupController = async (
   const unrestrictedMessenger: UnrestrictedControllerMessenger =
     new ControllerMessenger();
   const networkController = new NetworkController({
-    messenger: unrestrictedMessenger.getRestricted({
+    messenger: unrestrictedMessenger.getRestricted<
+      'NetworkController',
+      never,
+      never
+    >({
       name: 'NetworkController',
     }),
     trackMetaMetricsEvent: () => {
@@ -129,7 +133,11 @@ const setupController = async (
   assert(blockTracker, 'Provider must be available');
 
   const approvalController = new ApprovalController({
-    messenger: unrestrictedMessenger.getRestricted({
+    messenger: unrestrictedMessenger.getRestricted<
+      'ApprovalController',
+      never,
+      never
+    >({
       name: 'ApprovalController',
     }),
     showApprovalRequest: jest.fn(),

[mcmire]

Looks good!

[MajorLift]

The unrestrictedControllerMessenger pattern newly introduced in the TransactionControllerIntegration tests seems to have exposed a flaw in the recent getRestricted fix (#4013).

Omitting the allowed{Actions,Events} options in getRestricted doesn't result in their types being inferred as never[]. Instead, these options are typed with the "unrestricted" action/event union.

A temporary fix applied here is to pass in empty arrays into allowed{Actions,Events}: 38d5e88

But getRestricted and/or RestrictedControllerMessenger will need to be fixed for a fundamental solution that lets us omit allowed{Actions,Events}. I'll create a ticket for this.