Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Narrow string allowlists in RestrictedControllerMessenger instances #4031

Merged
merged 8 commits into from
Mar 7, 2024
Merged

Narrow string allowlists in RestrictedControllerMessenger instances #4031

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3ae1ebe
Narrow all `string` allowlists in core `RestrictedControllerMessenger…
MajorLift Mar 7, 2024
495f377
[accounts-controller] Fix allowlists being included in controller act…
MajorLift Mar 7, 2024
53582b1
[accounts-controller] Fix wildcard exports so that allowlists aren't …
MajorLift Mar 7, 2024
29c8095
Merge branch 'main' into 240307-narrow-string-allowlists
MajorLift Mar 7, 2024
bbba64a
Merge branch 'main' into 240307-narrow-string-allowlists
MajorLift Mar 7, 2024
a4aea11
Merge branch 'main' into 240307-narrow-string-allowlists
MajorLift Mar 7, 2024
38d5e88
Fix omitted `allowed{Actions,Events}` not being inferred as `never[]`…
MajorLift Mar 7, 2024
42d0e02
Merge branch 'main' into 240307-narrow-string-allowlists
MajorLift Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/accounts-controller/src/AccountsController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import type {
AccountsControllerActions,
AccountsControllerEvents,
AccountsControllerState,
AllowedActions,
AllowedEvents,
} from './AccountsController';
import { AccountsController } from './AccountsController';
import { keyringTypeToName } from './utils';
Expand Down Expand Up @@ -180,8 +182,8 @@ function setLastSelectedAsAny(account: InternalAccount): InternalAccount {
*/
function buildMessenger() {
return new ControllerMessenger<
AccountsControllerActions,
AccountsControllerEvents
AccountsControllerActions | AllowedActions,
AccountsControllerEvents | AllowedEvents
>();
}

Expand Down Expand Up @@ -220,8 +222,8 @@ function setupAccountsController({
}: {
initialState?: Partial<AccountsControllerState>;
messenger?: ControllerMessenger<
AccountsControllerActions,
AccountsControllerEvents
AccountsControllerActions | AllowedActions,
AccountsControllerEvents | AllowedEvents
>;
}): AccountsController {
const accountsControllerMessenger =
Expand Down
20 changes: 8 additions & 12 deletions packages/accounts-controller/src/AccountsController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export type AccountsControllerGetAccountAction = {
handler: AccountsController['getAccount'];
};

export type AllowedAccountsControllerActions =
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
export type AllowedActions =
| KeyringControllerGetKeyringForAccountAction
| KeyringControllerGetKeyringsByTypeAction
| KeyringControllerGetAccountsAction;
Expand All @@ -91,8 +91,7 @@ export type AccountsControllerActions =
| AccountsControllerUpdateAccountsAction
| AccountsControllerGetAccountByAddressAction
| AccountsControllerGetSelectedAccountAction
| AccountsControllerGetAccountAction
| AllowedAccountsControllerActions;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| AccountsControllerGetAccountAction;

export type AccountsControllerChangeEvent = ControllerStateChangeEvent<
typeof controllerName,
Expand All @@ -104,21 +103,18 @@ export type AccountsControllerSelectedAccountChangeEvent = {
payload: [InternalAccount];
};

export type AllowedAccountsControllerEvents =
| SnapStateChange
| KeyringControllerStateChangeEvent;
export type AllowedEvents = SnapStateChange | KeyringControllerStateChangeEvent;

export type AccountsControllerEvents =
| AccountsControllerChangeEvent
| AccountsControllerSelectedAccountChangeEvent
| AllowedAccountsControllerEvents;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

| AccountsControllerSelectedAccountChangeEvent;

export type AccountsControllerMessenger = RestrictedControllerMessenger<
typeof controllerName,
AccountsControllerActions,
AccountsControllerEvents,
AllowedAccountsControllerActions['type'],
AllowedAccountsControllerEvents['type']
AccountsControllerActions | AllowedActions,
AccountsControllerEvents | AllowedEvents,
AllowedActions['type'],
AllowedEvents['type']
>;

type AddressAndKeyringTypeObject = {
Expand Down
20 changes: 18 additions & 2 deletions packages/accounts-controller/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,18 @@
export * from './AccountsController';
export * from './utils';
export type {
AccountsControllerState,
AccountsControllerGetStateAction,
AccountsControllerSetSelectedAccountAction,
AccountsControllerSetAccountNameAction,
AccountsControllerListAccountsAction,
AccountsControllerUpdateAccountsAction,
AccountsControllerGetSelectedAccountAction,
AccountsControllerGetAccountByAddressAction,
AccountsControllerGetAccountAction,
AccountsControllerActions,
AccountsControllerChangeEvent,
AccountsControllerSelectedAccountChangeEvent,
AccountsControllerEvents,
AccountsControllerMessenger,
} from './AccountsController';
Comment on lines +2 to +15
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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

export { AccountsController } from './AccountsController';
export { keyringTypeToName, getUUIDFromAddressOfNormalAccount } from './utils';
4 changes: 2 additions & 2 deletions packages/approval-controller/src/ApprovalController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ export type ApprovalControllerMessenger = RestrictedControllerMessenger<
typeof controllerName,
ApprovalControllerActions,
ApprovalControllerEvents,
string,
string
never,
never
>;

// Option Types
Expand Down
4 changes: 2 additions & 2 deletions packages/keyring-controller/src/KeyringController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,8 @@ export type KeyringControllerMessenger = RestrictedControllerMessenger<
typeof name,
KeyringControllerActions,
KeyringControllerEvents,
string,
string
never,
never
>;

export type KeyringControllerOptions = {
Expand Down
4 changes: 2 additions & 2 deletions packages/network-controller/src/NetworkController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,8 @@ export type NetworkControllerMessenger = RestrictedControllerMessenger<
typeof name,
NetworkControllerActions,
NetworkControllerEvents,
string,
string
never,
never
>;

export type NetworkControllerOptions = {
Expand Down
4 changes: 2 additions & 2 deletions packages/permission-controller/src/PermissionController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,9 +363,9 @@ export type SideEffectMessenger<
Events extends EventConstraint,
> = RestrictedControllerMessenger<
typeof controllerName,
Actions,
Actions | AllowedActions,
Events,
string,
AllowedActions['type'],
never
>;

Expand Down
Loading