Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions packages/phishing-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- **BREAKING:** Use new `Messenger` from `@metamask/messenger` ([#6535](https://github.com/MetaMask/core/pull/6535))
- Previously, `PhishingController` accepted a `RestrictedMessenger` instance from `@metamask/base-controller`.
- Bump `@metamask/transaction-controller` from `^60.7.0` to `^60.8.0` ([#6883](https://github.com/MetaMask/core/pull/6883))

## [14.1.2]
Expand Down
1 change: 1 addition & 0 deletions packages/phishing-controller/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
"dependencies": {
"@metamask/base-controller": "^8.4.1",
"@metamask/controller-utils": "^11.14.1",
"@metamask/messenger": "^0.3.0",
"@noble/hashes": "^1.8.0",
"@types/punycode": "^2.1.0",
"ethereum-cryptography": "^2.1.2",
Expand Down
67 changes: 51 additions & 16 deletions packages/phishing-controller/src/BulkTokenScan.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
import { Messenger } from '@metamask/base-controller';
import { safelyExecuteWithTimeout } from '@metamask/controller-utils';
import type { TransactionControllerStateChangeEvent } from '@metamask/transaction-controller';
import {
Messenger,
MOCK_ANY_NAMESPACE,
type MessengerActions,
type MessengerEvents,
type MockAnyNamespace,
} from '@metamask/messenger';
import nock, { cleanAll } from 'nock';
import sinon from 'sinon';

import type { PhishingControllerEvents } from './PhishingController';
import type { PhishingControllerMessenger } from './PhishingController';
import {
PhishingController,
type PhishingControllerActions,
type PhishingControllerOptions,
SECURITY_ALERTS_BASE_URL,
TOKEN_BULK_SCANNING_ENDPOINT,
Expand All @@ -30,24 +34,55 @@ const mockSafelyExecuteWithTimeout =

const controllerName = 'PhishingController';

type AllPhishingControllerActions =
MessengerActions<PhishingControllerMessenger>;

type AllPhishingControllerEvents = MessengerEvents<PhishingControllerMessenger>;

type RootMessenger = Messenger<
MockAnyNamespace,
AllPhishingControllerActions,
AllPhishingControllerEvents,
RootMessenger
>;

/**
* Creates and returns a root messenger for testing
*
* @returns A messenger instance
*/
function getRootMessenger(): RootMessenger {
return new Messenger({
namespace: MOCK_ANY_NAMESPACE,
});
}

/**
* Constructs a restricted messenger with transaction events enabled.
* Constructs a messenger with transaction events enabled.
*
* @returns A restricted messenger that can listen to TransactionController events.
*/
function getRestrictedMessengerWithTransactionEvents() {
function getMessengerWithTransactionEvents() {
const rootMessenger = getRootMessenger();

const messenger = new Messenger<
PhishingControllerActions,
PhishingControllerEvents | TransactionControllerStateChangeEvent
>();
typeof controllerName,
AllPhishingControllerActions,
AllPhishingControllerEvents,
RootMessenger
>({
namespace: controllerName,
parent: rootMessenger,
});

rootMessenger.delegate({
actions: [],
events: ['TransactionController:stateChange'],
messenger,
});

return {
messenger: messenger.getRestricted({
name: controllerName,
allowedActions: [],
allowedEvents: ['TransactionController:stateChange'],
}),
globalMessenger: messenger,
messenger,
};
}

Expand All @@ -59,7 +94,7 @@ function getRestrictedMessengerWithTransactionEvents() {
*/
function getPhishingController(options?: Partial<PhishingControllerOptions>) {
return new PhishingController({
messenger: getRestrictedMessengerWithTransactionEvents().messenger,
messenger: getMessengerWithTransactionEvents().messenger,
...options,
});
}
Expand Down
92 changes: 66 additions & 26 deletions packages/phishing-controller/src/PhishingController.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import { deriveStateFromMetadata, Messenger } from '@metamask/base-controller';
import type { TransactionControllerStateChangeEvent } from '@metamask/transaction-controller';
import { deriveStateFromMetadata } from '@metamask/base-controller/next';
import {
Messenger,
MOCK_ANY_NAMESPACE,
type MessengerActions,
type MessengerEvents,
type MockAnyNamespace,
} from '@metamask/messenger';
import { strict as assert } from 'assert';
import nock, { cleanAll, isDone, pendingMocks } from 'nock';
import sinon from 'sinon';
Expand All @@ -10,15 +16,14 @@ import {
METAMASK_STALELIST_FILE,
PhishingController,
PHISHING_CONFIG_BASE_URL,
type PhishingControllerActions,
type PhishingControllerEvents,
type PhishingControllerOptions,
CLIENT_SIDE_DETECION_BASE_URL,
C2_DOMAIN_BLOCKLIST_ENDPOINT,
PHISHING_DETECTION_BASE_URL,
PHISHING_DETECTION_SCAN_ENDPOINT,
PHISHING_DETECTION_BULK_SCAN_ENDPOINT,
type BulkPhishingDetectionScanResponse,
type PhishingControllerMessenger,
} from './PhishingController';
import {
createMockStateChangePayload,
Expand All @@ -32,24 +37,59 @@ import { getHostnameFromUrl } from './utils';

const controllerName = 'PhishingController';

type AllPhishingControllerActions =
MessengerActions<PhishingControllerMessenger>;

type AllPhishingControllerEvents = MessengerEvents<PhishingControllerMessenger>;

type RootMessenger = Messenger<
MockAnyNamespace,
AllPhishingControllerActions,
AllPhishingControllerEvents,
RootMessenger
>;

/**
* Constructs a restricted messenger with transaction events enabled.
* Creates and returns a root messenger for testing
*
* @returns A restricted messenger that can listen to TransactionController events.
* @returns A messenger instance
*/
function getRestrictedMessengerWithTransactionEvents() {
function getRootMessenger(): RootMessenger {
return new Messenger({
namespace: MOCK_ANY_NAMESPACE,
});
}

/**
* Constructs a messenger for use in PhishingController tests.
*
* @returns A messenger and the root messenger.
*/
function setupMessenger(): {
messenger: PhishingControllerMessenger;
rootMessenger: RootMessenger;
} {
const rootMessenger = getRootMessenger();

const messenger = new Messenger<
PhishingControllerActions,
PhishingControllerEvents | TransactionControllerStateChangeEvent
>();
typeof controllerName,
AllPhishingControllerActions,
AllPhishingControllerEvents,
RootMessenger
>({
namespace: controllerName,
parent: rootMessenger,
});

rootMessenger.delegate({
actions: [],
events: ['TransactionController:stateChange'],
messenger,
});
Copy link

Choose a reason for hiding this comment

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

Bug: PhishingController Actions Not Delegated

The setupMessenger function's delegate call has an empty actions array. This means PhishingController actions like maybeUpdateState, testOrigin, bulkScanUrls, and bulkScanTokens are not delegated to the root messenger, potentially preventing external components from calling them.

Fix in Cursor Fix in Web


return {
messenger: messenger.getRestricted({
name: controllerName,
allowedActions: [],
allowedEvents: ['TransactionController:stateChange'],
}),
globalMessenger: messenger,
messenger,
rootMessenger,
};
}

Expand All @@ -60,8 +100,9 @@ function getRestrictedMessengerWithTransactionEvents() {
* @returns The constructed Phishing Controller.
*/
function getPhishingController(options?: Partial<PhishingControllerOptions>) {
const { messenger } = setupMessenger();
return new PhishingController({
messenger: getRestrictedMessengerWithTransactionEvents().messenger,
messenger,
...options,
});
}
Expand Down Expand Up @@ -407,8 +448,9 @@ describe('PhishingController', () => {
});

it('replaces existing phishing lists with completely new list from phishing detection API', async () => {
const { messenger } = setupMessenger();
const controller = new PhishingController({
messenger: getRestrictedMessengerWithTransactionEvents().messenger,
messenger,
stalelistRefreshInterval: 10,
state: {
phishingLists: [
Expand Down Expand Up @@ -3495,7 +3537,7 @@ describe('URL Scan Cache', () => {
deriveStateFromMetadata(
controller.state,
controller.metadata,
'anonymous',
'includeInDebugSnapshot',
),
).toMatchInlineSnapshot(`Object {}`);
});
Expand Down Expand Up @@ -3561,18 +3603,16 @@ describe('URL Scan Cache', () => {

describe('Transaction Controller State Change Integration', () => {
let controller: PhishingController;
let globalMessenger: Messenger<
PhishingControllerActions,
PhishingControllerEvents | TransactionControllerStateChangeEvent
>;
let globalMessenger: RootMessenger;
let bulkScanTokensSpy: jest.SpyInstance;

beforeEach(() => {
const messengerSetup = getRestrictedMessengerWithTransactionEvents();
globalMessenger = messengerSetup.globalMessenger;
const { messenger, rootMessenger } = setupMessenger();

globalMessenger = rootMessenger;

controller = new PhishingController({
messenger: messengerSetup.messenger,
messenger,
});

bulkScanTokensSpy = jest
Expand Down
Loading
Loading