diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index c2bba324074..2501c697c07 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -22,6 +22,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@ethersproject/address` from `^5.7.0` to `^5.8.0` ([#10478](https://github.com/MetaMask/core/pull/10478)) - Bump `@tanstack/query-core` from `^5.89.0` to `^5.103.2` ([#10511](https://github.com/MetaMask/core/pull/10511)) +- `NftController` now attributes its `PhishingController:bulkScanUrls` calls to the `nft-detection` request source, so NFT metadata URL scans are distinguishable from other callers in phishing-detection service metrics ([#10357](https://github.com/MetaMask/core/pull/10357)) + ## [112.0.4] ### Changed diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 9ebbc5e6404..4481d0e2603 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -33,7 +33,10 @@ import type { NetworkClientId, } from '@metamask/network-controller'; import type { BulkPhishingDetectionScanResponse } from '@metamask/phishing-controller'; -import { RecommendedAction } from '@metamask/phishing-controller'; +import { + RecommendedAction, + RequestSourceFlow, +} from '@metamask/phishing-controller'; import { getDefaultPreferencesState } from '@metamask/preferences-controller'; import type { PreferencesState } from '@metamask/preferences-controller'; import type { Hex } from '@metamask/utils'; @@ -5287,6 +5290,29 @@ describe('NftController', () => { expect(safeNft?.externalLink).toBe('http://legitimate-domain.com'); }); + it('should attribute URL scans to the NFT detection flow', async () => { + const mockBulkScanUrls = jest.fn().mockResolvedValue({ results: {} }); + + const { nftController } = setupController({ + bulkScanUrlsMock: mockBulkScanUrls, + }); + + await nftController.addNft('0xsafe', '1', 'mainnet', { + nftMetadata: { + name: 'Safe NFT', + description: 'NFT with safe links', + image: 'http://safe-site.com/image.png', + standard: ERC721, + }, + userAddress: OWNER_ADDRESS, + }); + + expect(mockBulkScanUrls).toHaveBeenCalledWith( + expect.any(Array), + RequestSourceFlow.NftDetection, + ); + }); + it('should handle errors during phishing detection when adding NFTs', async () => { const mockBulkScanUrls = jest .fn() @@ -5472,9 +5498,10 @@ describe('NftController', () => { }); // Verify only HTTP(S) URLs were sent for scanning - expect(mockBulkScanUrls).toHaveBeenCalledWith([ - 'https://secure-site.com', - ]); + expect(mockBulkScanUrls).toHaveBeenCalledWith( + ['https://secure-site.com'], + RequestSourceFlow.NftDetection, + ); const storedNft = nftController.state.allNfts[OWNER_ADDRESS][ChainId.mainnet][0]; @@ -5605,9 +5632,10 @@ describe('NftController', () => { }); // Should not throw error - expect(mockBulkScanUrls).toHaveBeenCalledWith([ - 'http://image.com/image.png', - ]); + expect(mockBulkScanUrls).toHaveBeenCalledWith( + ['http://image.com/image.png'], + RequestSourceFlow.NftDetection, + ); }); }); diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index e2797b83e9f..4614f3bd3c9 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -35,7 +35,10 @@ import type { NetworkControllerGetNetworkClientByIdAction, } from '@metamask/network-controller'; import type { PhishingControllerBulkScanUrlsAction } from '@metamask/phishing-controller'; -import { RecommendedAction } from '@metamask/phishing-controller'; +import { + RecommendedAction, + RequestSourceFlow, +} from '@metamask/phishing-controller'; import type { PreferencesControllerStateChangeEvent } from '@metamask/preferences-controller'; import { rpcErrors } from '@metamask/rpc-errors'; import type { Hex } from '@metamask/utils'; @@ -2299,6 +2302,7 @@ export class NftController extends BaseController< const bulkScanResponse = await this.messenger.call( 'PhishingController:bulkScanUrls', batch, + RequestSourceFlow.NftDetection, ); // Collect blocked URLs from this batch diff --git a/packages/phishing-controller/CHANGELOG.md b/packages/phishing-controller/CHANGELOG.md index 63c14148c85..3b99ebc60fd 100644 --- a/packages/phishing-controller/CHANGELOG.md +++ b/packages/phishing-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add optional request-source attribution parameters to `PhishingController.scanUrl` and `bulkScanUrls`, emitting an `x-request-source` header. ([#10357](https://github.com/MetaMask/core/pull/10357)) + ### Changed - Bump `@types/punycode` from `^2.1.0` to `^2.1.4` ([#10441](https://github.com/MetaMask/core/pull/10441)) diff --git a/packages/phishing-controller/src/PhishingController-method-action-types.ts b/packages/phishing-controller/src/PhishingController-method-action-types.ts index cfd36b561ed..7a666345031 100644 --- a/packages/phishing-controller/src/PhishingController-method-action-types.ts +++ b/packages/phishing-controller/src/PhishingController-method-action-types.ts @@ -75,6 +75,9 @@ export type PhishingControllerBypassAction = { * Only supports web URLs (`http:` / `https:`). * * @param url - The URL to scan. + * @param flow - Product flow used to attribute a network scan in the + * `x-request-source` header. If omitted, the header uses an unknown flow. + * Cached results do not make a network scan or emit a header. * @returns The phishing detection scan result. */ export type PhishingControllerScanUrlAction = { @@ -87,6 +90,9 @@ export type PhishingControllerScanUrlAction = { * It also only supports web URLs. * * @param urls - The URLs to scan. + * @param flow - Product flow used to attribute a network scan in the + * `x-request-source` header. If omitted, the header uses an unknown flow. + * Cached results do not make a network scan or emit a header. * @returns A mapping of URLs to their phishing detection scan results and errors. */ export type PhishingControllerBulkScanUrlsAction = { diff --git a/packages/phishing-controller/src/PhishingController.test.ts b/packages/phishing-controller/src/PhishingController.test.ts index cbbc6d343b0..21d049dc353 100644 --- a/packages/phishing-controller/src/PhishingController.test.ts +++ b/packages/phishing-controller/src/PhishingController.test.ts @@ -34,6 +34,7 @@ import type { BulkPhishingDetectionScanResponse, PhishingControllerMessenger, } from './PhishingController.js'; +import { RequestSourceFlow, RequestSourcePlatform } from './request-source.js'; import { createMockStateChangePayload, createMockTransaction, @@ -3057,6 +3058,108 @@ describe('PhishingController', () => { }); }); + describe('request source attribution', () => { + // NOTE: nock does not model CORS preflight, so nothing here can catch a + // server-side `Access-Control-Allow-Headers` allowlist that omits + // `x-request-source`. That is verified against the service, not in tests. + const scanResponse: PhishingDetectionScanResult = { + hostname: 'example.com', + recommendedAction: RecommendedAction.None, + }; + + it('reports the composed source on a single URL scan', async () => { + const { rootMessenger } = getPhishingController({ + platform: RequestSourcePlatform.Mobile, + }); + const scope = nock(PHISHING_DETECTION_BASE_URL) + .matchHeader('x-request-source', 'mobile-browser') + .get(`/${PHISHING_DETECTION_SCAN_ENDPOINT}`) + .query({ url: 'example.com' }) + .reply(200, scanResponse); + + await rootMessenger.call( + 'PhishingController:scanUrl', + 'https://example.com', + RequestSourceFlow.Browser, + ); + + expect(scope.isDone()).toBe(true); + }); + + it('reports the composed source on a bulk URL scan', async () => { + const { rootMessenger } = getPhishingController({ + platform: RequestSourcePlatform.Extension, + }); + const scope = nock(PHISHING_DETECTION_BASE_URL) + .matchHeader('x-request-source', 'extension-nft-detection') + .post(`/${PHISHING_DETECTION_BULK_SCAN_ENDPOINT}`) + .reply(200, { results: {}, errors: {} }); + + await rootMessenger.call( + 'PhishingController:bulkScanUrls', + ['https://example.com'], + RequestSourceFlow.NftDetection, + ); + + expect(scope.isDone()).toBe(true); + }); + + it('reports the platform sentinel when a caller omits the flow', async () => { + const { rootMessenger } = getPhishingController({ + platform: RequestSourcePlatform.Extension, + }); + const scope = nock(PHISHING_DETECTION_BASE_URL) + .matchHeader('x-request-source', 'extension-unknown') + .get(`/${PHISHING_DETECTION_SCAN_ENDPOINT}`) + .query({ url: 'example.com' }) + .reply(200, scanResponse); + + await rootMessenger.call( + 'PhishingController:scanUrl', + 'https://example.com', + ); + + expect(scope.isDone()).toBe(true); + }); + + it('reports the bare sentinel when no platform is configured', async () => { + const { rootMessenger } = getPhishingController(); + const scope = nock(PHISHING_DETECTION_BASE_URL) + .matchHeader('x-request-source', 'unknown') + .get(`/${PHISHING_DETECTION_SCAN_ENDPOINT}`) + .query({ url: 'example.com' }) + .reply(200, scanResponse); + + await rootMessenger.call( + 'PhishingController:scanUrl', + 'https://example.com', + RequestSourceFlow.Browser, + ); + + expect(scope.isDone()).toBe(true); + }); + + it('degrades an unrecognised flow to the platform sentinel', async () => { + // Guards the plain-JavaScript call sites, which get no type checking. + const { rootMessenger } = getPhishingController({ + platform: RequestSourcePlatform.Mobile, + }); + const scope = nock(PHISHING_DETECTION_BASE_URL) + .matchHeader('x-request-source', 'mobile-unknown') + .get(`/${PHISHING_DETECTION_SCAN_ENDPOINT}`) + .query({ url: 'example.com' }) + .reply(200, scanResponse); + + await rootMessenger.call( + 'PhishingController:scanUrl', + 'https://example.com', + 'browsr' as RequestSourceFlow, + ); + + expect(scope.isDone()).toBe(true); + }); + }); + describe('bulkScanUrls', () => { let rootMessenger: RootMessenger; diff --git a/packages/phishing-controller/src/PhishingController.ts b/packages/phishing-controller/src/PhishingController.ts index 62f9e132227..c38981ab032 100644 --- a/packages/phishing-controller/src/PhishingController.ts +++ b/packages/phishing-controller/src/PhishingController.ts @@ -43,6 +43,11 @@ import type { PhishingControllerTestOriginAction, } from './PhishingController-method-action-types.js'; import { PhishingDetector } from './PhishingDetector.js'; +import { REQUEST_SOURCE_HEADER, buildRequestSource } from './request-source.js'; +import type { + RequestSourceFlow, + RequestSourcePlatform, +} from './request-source.js'; import { PhishingDetectorResultType, RecommendedAction, @@ -388,6 +393,7 @@ export type PhishingControllerState = { * tokenScanCacheMaxSize - Maximum number of entries in the token scan cache. * addressScanCacheTTL - Time to live in seconds for cached address scan results. * addressScanCacheMaxSize - Maximum number of entries in the address scan cache. + * platform - Client used to attribute URL scans in the `x-request-source` header. */ export type PhishingControllerOptions = { stalelistRefreshInterval?: number; @@ -401,6 +407,7 @@ export type PhishingControllerOptions = { addressScanCacheMaxSize?: number; messenger: PhishingControllerMessenger; state?: Partial; + platform?: RequestSourcePlatform; }; const MESSENGER_EXPOSED_METHODS = [ @@ -509,6 +516,8 @@ export class PhishingController extends BaseController< readonly #addressBookRecipients: Set; + readonly #platform?: RequestSourcePlatform; + #inProgressHotlistUpdate?: Promise; #inProgressStalelistUpdate?: Promise; @@ -539,6 +548,8 @@ export class PhishingController extends BaseController< * @param config.addressScanCacheMaxSize - Maximum number of entries in the address scan cache. * @param config.messenger - The controller restricted messenger. * @param config.state - Initial state to set on this controller. + * @param config.platform - Client used to attribute URL scans in the + * `x-request-source` header. When omitted, URL scans use the `unknown` value. */ constructor({ stalelistRefreshInterval = STALELIST_REFRESH_INTERVAL, @@ -552,6 +563,7 @@ export class PhishingController extends BaseController< addressScanCacheMaxSize = DEFAULT_ADDRESS_SCAN_CACHE_MAX_SIZE, messenger, state = {}, + platform, }: PhishingControllerOptions) { super({ name: controllerName, @@ -563,6 +575,7 @@ export class PhishingController extends BaseController< }, }); + this.#platform = platform; this.#stalelistRefreshInterval = stalelistRefreshInterval; this.#hotlistRefreshInterval = hotlistRefreshInterval; this.#c2DomainBlocklistRefreshInterval = c2DomainBlocklistRefreshInterval; @@ -1208,9 +1221,15 @@ export class PhishingController extends BaseController< * Only supports web URLs (`http:` / `https:`). * * @param url - The URL to scan. + * @param flow - Product flow used to attribute a network scan in the + * `x-request-source` header. If omitted, the header uses an unknown flow. + * Cached results do not make a network scan or emit a header. * @returns The phishing detection scan result. */ - async scanUrl(url: string): Promise { + async scanUrl( + url: string, + flow?: RequestSourceFlow, + ): Promise { const [scanUrlParam, scanParamOk] = getPhishingDetectionScanUrlParam(url); if (!scanParamOk) { return { @@ -1235,6 +1254,7 @@ export class PhishingController extends BaseController< method: 'GET', headers: { Accept: 'application/json', + [REQUEST_SOURCE_HEADER]: buildRequestSource(this.#platform, flow), }, }, ); @@ -1281,10 +1301,14 @@ export class PhishingController extends BaseController< * It also only supports web URLs. * * @param urls - The URLs to scan. + * @param flow - Product flow used to attribute a network scan in the + * `x-request-source` header. If omitted, the header uses an unknown flow. + * Cached results do not make a network scan or emit a header. * @returns A mapping of URLs to their phishing detection scan results and errors. */ async bulkScanUrls( urls: string[], + flow?: RequestSourceFlow, ): Promise { if (!urls || urls.length === 0) { return { @@ -1353,7 +1377,7 @@ export class PhishingController extends BaseController< // Process each batch in parallel const batchResults = await Promise.all( - batches.map((batchUrls) => this.#processBatch(batchUrls)), + batches.map((batchUrls) => this.#processBatch(batchUrls, flow)), ); // Merge results and errors from all batches @@ -1692,10 +1716,13 @@ export class PhishingController extends BaseController< * Process a batch of URLs (up to 50) for phishing detection. * * @param urls - A batch of URLs to scan. + * @param flow - The flow that caused this scan, reported via the + * `x-request-source` header. * @returns The scan results and errors for this batch. */ readonly #processBatch = async ( urls: string[], + flow?: RequestSourceFlow, ): Promise => { const apiResponse = await safelyExecuteWithTimeout( async () => { @@ -1706,6 +1733,7 @@ export class PhishingController extends BaseController< headers: { Accept: 'application/json', 'Content-Type': 'application/json', + [REQUEST_SOURCE_HEADER]: buildRequestSource(this.#platform, flow), }, body: JSON.stringify({ urls }), }, diff --git a/packages/phishing-controller/src/index.ts b/packages/phishing-controller/src/index.ts index 5a656acf38c..d019a08a6bb 100644 --- a/packages/phishing-controller/src/index.ts +++ b/packages/phishing-controller/src/index.ts @@ -47,6 +47,14 @@ export type { ExtractedSignatureAddresses, ExtractSignatureAddressesOptions, } from './signature-address-extraction.js'; +export { + REQUEST_SOURCE_HEADER, + UNKNOWN_REQUEST_SOURCE, + RequestSourceFlow, + RequestSourcePlatform, + buildRequestSource, +} from './request-source.js'; +export type { RequestSource } from './request-source.js'; export type { PhishingControllerMaybeUpdateStateAction, diff --git a/packages/phishing-controller/src/request-source.test.ts b/packages/phishing-controller/src/request-source.test.ts new file mode 100644 index 00000000000..58645661b2a --- /dev/null +++ b/packages/phishing-controller/src/request-source.test.ts @@ -0,0 +1,115 @@ +import { + REQUEST_SOURCE_HEADER, + RequestSourceFlow, + RequestSourcePlatform, + UNKNOWN_REQUEST_SOURCE, + buildRequestSource, +} from './request-source.js'; + +describe('REQUEST_SOURCE_HEADER', () => { + it('is the header name agreed with the phishing detection service', () => { + expect(REQUEST_SOURCE_HEADER).toBe('x-request-source'); + }); +}); + +describe('buildRequestSource', () => { + const validCombinations: [ + RequestSourcePlatform, + RequestSourceFlow, + string, + ][] = [ + [ + RequestSourcePlatform.Extension, + RequestSourceFlow.DappConnection, + 'extension-dapp-connection', + ], + [ + RequestSourcePlatform.Extension, + RequestSourceFlow.RpcTrustSignals, + 'extension-rpc-trust-signals', + ], + [ + RequestSourcePlatform.Extension, + RequestSourceFlow.Confirmations, + 'extension-confirmations', + ], + [ + RequestSourcePlatform.Extension, + RequestSourceFlow.RevealSrp, + 'extension-reveal-srp', + ], + [ + RequestSourcePlatform.Extension, + RequestSourceFlow.NftDetection, + 'extension-nft-detection', + ], + [ + RequestSourcePlatform.Mobile, + RequestSourceFlow.DappConnection, + 'mobile-dapp-connection', + ], + [RequestSourcePlatform.Mobile, RequestSourceFlow.Browser, 'mobile-browser'], + [ + RequestSourcePlatform.Mobile, + RequestSourceFlow.RpcTrustSignals, + 'mobile-rpc-trust-signals', + ], + [ + RequestSourcePlatform.Mobile, + RequestSourceFlow.NftDetection, + 'mobile-nft-detection', + ], + ]; + + it.each(validCombinations)( + 'composes %s + %s into %s', + (platform, flow, expected) => { + expect(buildRequestSource(platform, flow)).toBe(expected); + }, + ); + + it('falls back to the platform sentinel when no flow is given', () => { + expect(buildRequestSource(RequestSourcePlatform.Extension)).toBe( + 'extension-unknown', + ); + expect(buildRequestSource(RequestSourcePlatform.Mobile)).toBe( + 'mobile-unknown', + ); + }); + + it('falls back to the platform sentinel when the flow is not recognised', () => { + // Guards against a typo from an untyped (JavaScript) call site, which would + // otherwise be reported as a real attribution value. + expect( + buildRequestSource( + RequestSourcePlatform.Extension, + 'dapp-conection' as RequestSourceFlow, + ), + ).toBe('extension-unknown'); + }); + + it('returns the bare sentinel when no platform is configured', () => { + expect(buildRequestSource()).toBe(UNKNOWN_REQUEST_SOURCE); + expect( + buildRequestSource(undefined, RequestSourceFlow.DappConnection), + ).toBe(UNKNOWN_REQUEST_SOURCE); + }); + + it('returns the bare sentinel when the platform is not recognised', () => { + expect( + buildRequestSource( + 'Desktop' as RequestSourcePlatform, + RequestSourceFlow.DappConnection, + ), + ).toBe(UNKNOWN_REQUEST_SOURCE); + }); + + it('never throws, so attribution cannot fail a scan', () => { + expect(() => + buildRequestSource( + null as unknown as RequestSourcePlatform, + null as unknown as RequestSourceFlow, + ), + ).not.toThrow(); + }); +}); diff --git a/packages/phishing-controller/src/request-source.ts b/packages/phishing-controller/src/request-source.ts new file mode 100644 index 00000000000..669201f317f --- /dev/null +++ b/packages/phishing-controller/src/request-source.ts @@ -0,0 +1,104 @@ +/** + * HTTP header used to attribute a phishing-detection URL scan to a MetaMask + * client and product flow. + */ +export const REQUEST_SOURCE_HEADER = 'x-request-source'; + +/** + * Request-source value used when the client platform is unknown. + */ +export const UNKNOWN_REQUEST_SOURCE = 'unknown'; + +/** + * MetaMask client that initiated a phishing-detection URL scan. + */ +export enum RequestSourcePlatform { + Extension = 'extension', + Mobile = 'mobile', +} + +/** + * Product flow that initiated a phishing-detection URL scan. + */ +export enum RequestSourceFlow { + /** + * A dapp requested account-access permission, such as via + * `eth_requestAccounts` or `wallet_requestPermissions`. + */ + DappConnection = 'dapp-connection', + /** + * A main-frame navigation in the mobile in-app browser. + */ + Browser = 'browser', + /** + * Dapp RPC traffic used as a trust signal. + */ + RpcTrustSignals = 'rpc-trust-signals', + /** + * A dapp request that opens a transaction, signature, or permission approval. + */ + Confirmations = 'confirmations', + /** + * A scan of the active dapp origin when the user opens the Secret Recovery + * Phrase reveal screen. + */ + RevealSrp = 'reveal-srp', + /** + * NFT metadata, image, and external URLs scanned while NFTs are added or + * auto-detected. + */ + NftDetection = 'nft-detection', +} + +/** + * Valid {@link REQUEST_SOURCE_HEADER} value: a platform and flow, a platform + * with an unknown flow, or an unknown platform. + */ +export type RequestSource = + | `${RequestSourcePlatform}-${RequestSourceFlow}` + | `${RequestSourcePlatform}-${typeof UNKNOWN_REQUEST_SOURCE}` + | typeof UNKNOWN_REQUEST_SOURCE; + +/** + * Checks whether a value is a recognised platform. + * + * @param value - The value to check. + * @returns Whether the value is a {@link RequestSourcePlatform}. + */ +const isKnownPlatform = (value?: string): value is RequestSourcePlatform => + Object.values(RequestSourcePlatform).includes(value as RequestSourcePlatform); + +/** + * Checks whether a value is a recognised flow. + * + * @param value - The value to check. + * @returns Whether the value is a {@link RequestSourceFlow}. + */ +const isKnownFlow = (value?: string): value is RequestSourceFlow => + Object.values(RequestSourceFlow).includes(value as RequestSourceFlow); + +/** + * Builds a value for {@link REQUEST_SOURCE_HEADER}. + * + * Returns {@link UNKNOWN_REQUEST_SOURCE} when `platform` is unknown, or a + * platform-specific unknown value when `flow` is unknown. This function never + * throws for an unrecognised input. + * + * @param platform - The client emitting the scan. + * @param flow - The flow that caused the scan. + * @returns The composed source, or a sentinel when either part is unknown. + */ +export function buildRequestSource( + platform?: RequestSourcePlatform, + flow?: RequestSourceFlow, +): RequestSource { + if (!isKnownPlatform(platform)) { + return UNKNOWN_REQUEST_SOURCE; + } + + if (!isKnownFlow(flow)) { + return `${platform}-${UNKNOWN_REQUEST_SOURCE}`; + } + + return `${platform}-${flow}`; +}