[base-controller] Enforce that RestrictedControllerMessenger is not initialized with internal actions/events in allow lists

[MajorLift]

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 feat(base-controller): Allow using internal events/actions #2050.

References

• Builds on feat(base-controller): Allow using internal events/actions #2050
• Closes [base-controller] Enforce that RestrictedControllerMessenger is not initialized with internal actions/events in allow lists #2057

Changelog

@metamask/base-controller

• BREAKING: The RestrictedControllerMessenger constructor and ControllerMessenger.getRestricted() method raise a type error if internal actions or events are passed into allowedActions or allowedEvents.

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

The base branch was changed.

[mcmire]

Is there any harm in allowlisting internal events/actions? What do you think about showing a warning instead?

[MajorLift]

@mcmire I agree that this should probably be done at the type level rather than throwing an error at runtime. Will update with new approach.

[Mrtenz]

What is considered "internal action/event"? In the Snap Controller, we subscribe to events fired from the Snap Controller. Would this no longer be allowed?

[MajorLift]

@Mrtenz No worries, this PR won't affect the snap controller event subscriptions you linked to. It doesn't restrict any event subscriptions or action handler registrations in general.

Internal actions/events for a controller are those with type names that are in the controller's namespace (e.g. SnapController:snapUpdated is internal to SnapController).

These internal actions/events should always be allowed for usage by its controller, but previously they needed to be explicitly specified in RestrictedControllerMessenger allowlists (allowedActions, allowedEvents). This was fixed recently in #2050.

This PR is a follow-up to that enforcing that internal actions/events are not redundantly included in the allowlists at all. So it only affects how RestrictedControllerMessenger is initialized (by its constructor, or with ControllerMessenger.getRestricted()), not which actions/events it can use.

[Mrtenz]

@Mrtenz No worries, this PR won't affect the snap controller event subscriptions you linked to. It doesn't restrict any event subscriptions or action handler registrations in general.

Internal actions/events for a controller are those with type names that are in the controller's namespace (e.g. SnapController:snapUpdated is internal to SnapController).

These internal actions/events should always be allowed for usage by its controller, but previously they needed to be explicitly specified in RestrictedControllerMessenger allowlists (allowedActions, allowedEvents). This was fixed recently in #2050.

This PR is a follow-up to that enforcing that internal actions/events are not redundantly included in the allowlists at all. So it only affects how RestrictedControllerMessenger is initialized (by its constructor, or with ControllerMessenger.getRestricted()), not which actions/events it can use.

Thanks for the clarification!

[mcmire]

Sorry this took so long to review. I realized that all I really needed to do was look at the changes here and see if they made sense on their own, which they do. I still think that the types in RestrictedControllerMessenger are super confusing, but this PR doesn't make them more confusing, it just narrows what's already there.