From bdc6576118e4267cf4930bf7553d1e674a9f6342 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Mon, 8 Sep 2025 17:21:15 +0200 Subject: [PATCH 1/5] check signature coverage --- packages/shield-controller/package.json | 1 + .../src/ShieldController.test.ts | 76 ++++++++++++++++- .../shield-controller/src/ShieldController.ts | 84 ++++++++++++++++++- .../shield-controller/src/backend.test.ts | 42 +++++++++- packages/shield-controller/src/backend.ts | 59 +++++++++---- packages/shield-controller/src/types.ts | 4 + .../shield-controller/tests/mocks/backend.ts | 3 + .../tests/mocks/messenger.ts | 5 +- packages/shield-controller/tests/utils.ts | 26 ++++++ .../shield-controller/tsconfig.build.json | 1 + packages/shield-controller/tsconfig.json | 1 + yarn.lock | 1 + 12 files changed, 282 insertions(+), 21 deletions(-) diff --git a/packages/shield-controller/package.json b/packages/shield-controller/package.json index 20055aa7831..3b8cf6e7609 100644 --- a/packages/shield-controller/package.json +++ b/packages/shield-controller/package.json @@ -67,6 +67,7 @@ "uuid": "^8.3.2" }, "peerDependencies": { + "@metamask/signature-controller": "^33.0.0", "@metamask/transaction-controller": "^60.0.0" }, "engines": { diff --git a/packages/shield-controller/src/ShieldController.test.ts b/packages/shield-controller/src/ShieldController.test.ts index 99c48de3d2c..00fb0e53477 100644 --- a/packages/shield-controller/src/ShieldController.test.ts +++ b/packages/shield-controller/src/ShieldController.test.ts @@ -1,10 +1,14 @@ import { deriveStateFromMetadata } from '@metamask/base-controller'; +import type { SignatureControllerState } from '@metamask/signature-controller'; import type { TransactionControllerState } from '@metamask/transaction-controller'; import { ShieldController } from './ShieldController'; import { createMockBackend } from '../tests/mocks/backend'; import { createMockMessenger } from '../tests/mocks/messenger'; -import { generateMockTxMeta } from '../tests/utils'; +import { + generateMockSignatureRequest, + generateMockTxMeta, +} from '../tests/utils'; /** * Sets up a ShieldController for testing. @@ -144,6 +148,76 @@ describe('ShieldController', () => { }); }); + describe('checkSignatureCoverage', () => { + it('should check signature coverage', async () => { + const { baseMessenger, backend } = setup(); + const signatureRequest = generateMockSignatureRequest(); + const coverageResultReceived = new Promise((resolve) => { + baseMessenger.subscribe( + 'ShieldController:coverageResultReceived', + (_coverageResult) => resolve(), + ); + }); + baseMessenger.publish( + 'SignatureController:stateChange', + { + signatureRequests: { [signatureRequest.id]: signatureRequest }, + } as SignatureControllerState, + undefined as never, + ); + expect(await coverageResultReceived).toBeUndefined(); + expect(backend.checkSignatureCoverage).toHaveBeenCalledWith( + signatureRequest, + ); + }); + }); + + it('should check coverage for multiple signature request', async () => { + const { baseMessenger, backend } = setup(); + const signatureRequest1 = generateMockSignatureRequest(); + const coverageResultReceived1 = new Promise((resolve) => { + baseMessenger.subscribe( + 'ShieldController:coverageResultReceived', + (_coverageResult) => resolve(), + ); + }); + baseMessenger.publish( + 'SignatureController:stateChange', + { + signatureRequests: { + [signatureRequest1.id]: signatureRequest1, + }, + } as SignatureControllerState, + undefined as never, + ); + expect(await coverageResultReceived1).toBeUndefined(); + expect(backend.checkSignatureCoverage).toHaveBeenCalledWith( + signatureRequest1, + ); + + const signatureRequest2 = generateMockSignatureRequest(); + const coverageResultReceived2 = new Promise((resolve) => { + baseMessenger.subscribe( + 'ShieldController:coverageResultReceived', + (_coverageResult) => resolve(), + ); + }); + baseMessenger.publish( + 'SignatureController:stateChange', + { + signatureRequests: { + [signatureRequest2.id]: signatureRequest2, + }, + } as SignatureControllerState, + undefined as never, + ); + + expect(await coverageResultReceived2).toBeUndefined(); + expect(backend.checkSignatureCoverage).toHaveBeenCalledWith( + signatureRequest2, + ); + }); + describe('metadata', () => { it('includes expected state in debug snapshots', () => { const { controller } = setup(); diff --git a/packages/shield-controller/src/ShieldController.ts b/packages/shield-controller/src/ShieldController.ts index af642ab9962..4e8a31fc4d4 100644 --- a/packages/shield-controller/src/ShieldController.ts +++ b/packages/shield-controller/src/ShieldController.ts @@ -3,6 +3,11 @@ import type { ControllerStateChangeEvent, RestrictedMessenger, } from '@metamask/base-controller'; +import { + SignatureRequestType, + type SignatureRequest, + type SignatureStateChange, +} from '@metamask/signature-controller'; import type { TransactionControllerStateChangeEvent, TransactionMeta, @@ -82,7 +87,9 @@ type AllowedActions = never; /** * The external events available to the ShieldController. */ -type AllowedEvents = TransactionControllerStateChangeEvent; +type AllowedEvents = + | SignatureStateChange + | TransactionControllerStateChangeEvent; /** * The messenger of the {@link ShieldController}. @@ -138,6 +145,11 @@ export class ShieldController extends BaseController< previousTransactions: TransactionMeta[] | undefined, ) => void; + readonly #signatureControllerStateChangeHandler: ( + signatureRequests: Record, + previousSignatureRequests: Record | undefined, + ) => void; + constructor(options: ShieldControllerOptions) { const { messenger, @@ -161,6 +173,8 @@ export class ShieldController extends BaseController< this.#transactionHistoryLimit = transactionHistoryLimit; this.#transactionControllerStateChangeHandler = this.#handleTransactionControllerStateChange.bind(this); + this.#signatureControllerStateChangeHandler = + this.#handleSignatureControllerStateChange.bind(this); } start() { @@ -169,6 +183,12 @@ export class ShieldController extends BaseController< this.#transactionControllerStateChangeHandler, (state) => state.transactions, ); + + this.messagingSystem.subscribe( + 'SignatureController:stateChange', + this.#signatureControllerStateChangeHandler, + (state) => state.signatureRequests, + ); } stop() { @@ -176,6 +196,41 @@ export class ShieldController extends BaseController< 'TransactionController:stateChange', this.#transactionControllerStateChangeHandler, ); + + this.messagingSystem.unsubscribe( + 'SignatureController:stateChange', + this.#signatureControllerStateChangeHandler, + ); + } + + #handleSignatureControllerStateChange( + signatureRequests: Record, + previousSignatureRequests: Record | undefined, + ) { + const signatureRequestsArray = Object.values(signatureRequests); + const previousSignatureRequestsArray = Object.values( + previousSignatureRequests ?? {}, + ); + const previousSignatureRequestsById = new Map( + previousSignatureRequestsArray.map((request) => [request.id, request]), + ); + for (const signatureRequest of signatureRequestsArray) { + const previousSignatureRequest = previousSignatureRequestsById.get( + signatureRequest.id, + ); + + // Check coverage if the signature request is new and has type + // `personal_sign`. + if ( + !previousSignatureRequest && + signatureRequest.type === SignatureRequestType.PersonalSign + ) { + this.checkSignatureCoverage(signatureRequest).catch( + // istanbul ignore next + (error) => log('Error checking coverage:', error), + ); + } + } } #handleTransactionControllerStateChange( @@ -212,7 +267,7 @@ export class ShieldController extends BaseController< */ async checkCoverage(txMeta: TransactionMeta): Promise { // Check coverage - const coverageResult = await this.#fetchCoverageResult(txMeta); + const coverageResult = await this.#backend.checkCoverage(txMeta); // Publish coverage result this.messagingSystem.publish( @@ -226,8 +281,29 @@ export class ShieldController extends BaseController< return coverageResult; } - async #fetchCoverageResult(txMeta: TransactionMeta): Promise { - return this.#backend.checkCoverage(txMeta); + /** + * Checks the coverage of a signature request. + * + * @param signatureRequest - The signature request to check coverage for. + * @returns The coverage result. + */ + async checkSignatureCoverage( + signatureRequest: SignatureRequest, + ): Promise { + // Check coverage + const coverageResult = + await this.#backend.checkSignatureCoverage(signatureRequest); + + // Publish coverage result + this.messagingSystem.publish( + `${controllerName}:coverageResultReceived`, + coverageResult, + ); + + // Update state + this.#addCoverageResult(signatureRequest.id, coverageResult); + + return coverageResult; } #addCoverageResult(txId: string, coverageResult: CoverageResult) { diff --git a/packages/shield-controller/src/backend.test.ts b/packages/shield-controller/src/backend.test.ts index b506c77da1a..6ef470bbe74 100644 --- a/packages/shield-controller/src/backend.test.ts +++ b/packages/shield-controller/src/backend.test.ts @@ -1,5 +1,9 @@ import { ShieldRemoteBackend } from './backend'; -import { generateMockTxMeta, getRandomCoverageStatus } from '../tests/utils'; +import { + generateMockSignatureRequest, + generateMockTxMeta, + getRandomCoverageStatus, +} from '../tests/utils'; /** * Setup the test environment. @@ -141,4 +145,40 @@ describe('ShieldRemoteBackend', () => { // that the polling loop is exited as expected. await new Promise((resolve) => setTimeout(resolve, 10)); }); + + describe('checkSignatureCoverage', () => { + it('should check signature coverage', async () => { + const { backend, fetchMock, getAccessToken } = setup(); + + // Mock init coverage check. + fetchMock.mockResolvedValueOnce({ + status: 200, + json: jest.fn().mockResolvedValue({ coverageId: 'coverageId' }), + } as unknown as Response); + + // Mock get coverage result. + const status = getRandomCoverageStatus(); + fetchMock.mockResolvedValueOnce({ + status: 200, + json: jest.fn().mockResolvedValue({ status }), + } as unknown as Response); + + const signatureRequest = generateMockSignatureRequest(); + const coverageResult = + await backend.checkSignatureCoverage(signatureRequest); + expect(coverageResult).toStrictEqual({ status }); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(getAccessToken).toHaveBeenCalledTimes(2); + }); + + it('throws with invalid data', async () => { + const { backend } = setup(); + + const signatureRequest = generateMockSignatureRequest(); + signatureRequest.messageParams.data = []; + await expect( + backend.checkSignatureCoverage(signatureRequest), + ).rejects.toThrow('Signature data must be a string'); + }); + }); }); diff --git a/packages/shield-controller/src/backend.ts b/packages/shield-controller/src/backend.ts index 4573a75a2ad..184d742c6df 100644 --- a/packages/shield-controller/src/backend.ts +++ b/packages/shield-controller/src/backend.ts @@ -1,3 +1,4 @@ +import type { SignatureRequest } from '@metamask/signature-controller'; import type { TransactionMeta } from '@metamask/transaction-controller'; import type { CoverageResult, CoverageStatus, ShieldBackend } from './types'; @@ -16,6 +17,14 @@ export type InitCoverageCheckRequest = { origin?: string; }; +export type InitSignatureCoverageCheckRequest = { + chainId: string; + data: string; + from: string; + method: string; + origin?: string; +}; + export type InitCoverageCheckResponse = { coverageId: string; }; @@ -59,9 +68,7 @@ export class ShieldRemoteBackend implements ShieldBackend { this.#fetch = fetchFn; } - checkCoverage: (txMeta: TransactionMeta) => Promise = async ( - txMeta, - ) => { + async checkCoverage(txMeta: TransactionMeta): Promise { const reqBody: InitCoverageCheckRequest = { txParams: [ { @@ -76,22 +83,46 @@ export class ShieldRemoteBackend implements ShieldBackend { origin: txMeta.origin, }; - const { coverageId } = await this.#initCoverageCheck(reqBody); + const { coverageId } = await this.#initCoverageCheck( + 'v1/transaction/coverage/init', + reqBody, + ); return this.#getCoverageResult(coverageId); - }; + } + + async checkSignatureCoverage( + signatureRequest: SignatureRequest, + ): Promise { + if (typeof signatureRequest.messageParams.data !== 'string') { + throw new Error('Signature data must be a string'); + } + + const reqBody: InitSignatureCoverageCheckRequest = { + chainId: signatureRequest.chainId, + data: signatureRequest.messageParams.data, + from: signatureRequest.messageParams.from, + method: signatureRequest.type, + origin: signatureRequest.messageParams.origin, + }; + + const { coverageId } = await this.#initCoverageCheck( + 'v1/signature/coverage/init', + reqBody, + ); + + return this.#getCoverageResult(coverageId); + } async #initCoverageCheck( - reqBody: InitCoverageCheckRequest, + path: string, + reqBody: unknown, ): Promise { - const res = await this.#fetch( - `${this.#baseUrl}/v1/transaction/coverage/init`, - { - method: 'POST', - headers: await this.#createHeaders(), - body: JSON.stringify(reqBody), - }, - ); + const res = await this.#fetch(`${this.#baseUrl}/${path}`, { + method: 'POST', + headers: await this.#createHeaders(), + body: JSON.stringify(reqBody), + }); if (res.status !== 200) { throw new Error(`Failed to init coverage check: ${res.status}`); } diff --git a/packages/shield-controller/src/types.ts b/packages/shield-controller/src/types.ts index 03488ce24ef..eb602e0fb6e 100644 --- a/packages/shield-controller/src/types.ts +++ b/packages/shield-controller/src/types.ts @@ -1,3 +1,4 @@ +import type { SignatureRequest } from '@metamask/signature-controller'; import type { TransactionMeta } from '@metamask/transaction-controller'; export type CoverageResult = { @@ -9,4 +10,7 @@ export type CoverageStatus = (typeof coverageStatuses)[number]; export type ShieldBackend = { checkCoverage: (txMeta: TransactionMeta) => Promise; + checkSignatureCoverage: ( + signatureRequest: SignatureRequest, + ) => Promise; }; diff --git a/packages/shield-controller/tests/mocks/backend.ts b/packages/shield-controller/tests/mocks/backend.ts index 8f2e2e5f071..3963f3aecb9 100644 --- a/packages/shield-controller/tests/mocks/backend.ts +++ b/packages/shield-controller/tests/mocks/backend.ts @@ -8,5 +8,8 @@ export function createMockBackend() { checkCoverage: jest.fn().mockResolvedValue({ status: 'covered', }), + checkSignatureCoverage: jest.fn().mockResolvedValue({ + status: 'covered', + }), }; } diff --git a/packages/shield-controller/tests/mocks/messenger.ts b/packages/shield-controller/tests/mocks/messenger.ts index 9224f9a9e38..f35b43da9b3 100644 --- a/packages/shield-controller/tests/mocks/messenger.ts +++ b/packages/shield-controller/tests/mocks/messenger.ts @@ -24,7 +24,10 @@ export function createMockMessenger() { const messenger = baseMessenger.getRestricted({ name: controllerName, allowedActions: [], - allowedEvents: ['TransactionController:stateChange'], + allowedEvents: [ + 'SignatureController:stateChange', + 'TransactionController:stateChange', + ], }); return { diff --git a/packages/shield-controller/tests/utils.ts b/packages/shield-controller/tests/utils.ts index b6ec496cd1b..cdb650699e4 100644 --- a/packages/shield-controller/tests/utils.ts +++ b/packages/shield-controller/tests/utils.ts @@ -1,3 +1,8 @@ +import { + SignatureRequestStatus, + SignatureRequestType, + type SignatureRequest, +} from '@metamask/signature-controller'; import { TransactionStatus, TransactionType, @@ -30,6 +35,27 @@ export function generateMockTxMeta(): TransactionMeta { }; } +/** + * Generate a mock signature request. + * + * @returns A mock signature request. + */ +export function generateMockSignatureRequest(): SignatureRequest { + return { + chainId: '0x1', + id: random(), + type: SignatureRequestType.PersonalSign, + messageParams: { + data: '0x00', + from: '0x0000000000000000000000000000000000000000', + origin: 'https://metamask.io', + }, + networkClientId: '1', + status: SignatureRequestStatus.Unapproved, + time: Date.now(), + }; +} + /** * Get a random coverage status. * diff --git a/packages/shield-controller/tsconfig.build.json b/packages/shield-controller/tsconfig.build.json index 0650bc3d190..1cc45a83d78 100644 --- a/packages/shield-controller/tsconfig.build.json +++ b/packages/shield-controller/tsconfig.build.json @@ -7,6 +7,7 @@ }, "references": [ { "path": "../base-controller/tsconfig.build.json" }, + { "path": "../signature-controller/tsconfig.build.json" }, { "path": "../transaction-controller/tsconfig.build.json" } ], "include": ["../../types", "./src"] diff --git a/packages/shield-controller/tsconfig.json b/packages/shield-controller/tsconfig.json index 97fd71ee0b8..0ec16827b92 100644 --- a/packages/shield-controller/tsconfig.json +++ b/packages/shield-controller/tsconfig.json @@ -5,6 +5,7 @@ }, "references": [ { "path": "../base-controller" }, + { "path": "../signature-controller" }, { "path": "../transaction-controller" } ], "include": ["../../types", "./src", "./tests"] diff --git a/yarn.lock b/yarn.lock index 25a8362f2b8..f432ee14f57 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4490,6 +4490,7 @@ __metadata: typescript: "npm:~5.2.2" uuid: "npm:^8.3.2" peerDependencies: + "@metamask/signature-controller": ^33.0.0 "@metamask/transaction-controller": ^60.0.0 languageName: unknown linkType: soft From 93c68e6d66181018ffe3ad86f3544ffd6ce6ec93 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Thu, 11 Sep 2025 12:00:46 +0200 Subject: [PATCH 2/5] Update CHANGELOG --- packages/shield-controller/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/shield-controller/CHANGELOG.md b/packages/shield-controller/CHANGELOG.md index f0ec19a0273..290efe82fad 100644 --- a/packages/shield-controller/CHANGELOG.md +++ b/packages/shield-controller/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add two new controller state metadata properties: `includeInStateLogs` and `usedInUi` ([#6504](https://github.com/MetaMask/core/pull/6504)) +- Add signature coverage checking ([#6501](https://github.com/MetaMask/core/pull/6501)) ### Changed From 8da1a1581d7479767d020a9d19d846b11d2968c9 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Thu, 11 Sep 2025 14:57:43 +0200 Subject: [PATCH 3/5] fixup import --- packages/shield-controller/src/ShieldController.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/shield-controller/src/ShieldController.ts b/packages/shield-controller/src/ShieldController.ts index 4e8a31fc4d4..20e88084e99 100644 --- a/packages/shield-controller/src/ShieldController.ts +++ b/packages/shield-controller/src/ShieldController.ts @@ -3,10 +3,9 @@ import type { ControllerStateChangeEvent, RestrictedMessenger, } from '@metamask/base-controller'; -import { - SignatureRequestType, - type SignatureRequest, - type SignatureStateChange, +import type { + SignatureRequest, + SignatureStateChange, } from '@metamask/signature-controller'; import type { TransactionControllerStateChangeEvent, @@ -223,7 +222,7 @@ export class ShieldController extends BaseController< // `personal_sign`. if ( !previousSignatureRequest && - signatureRequest.type === SignatureRequestType.PersonalSign + signatureRequest.type === 'personal_sign' ) { this.checkSignatureCoverage(signatureRequest).catch( // istanbul ignore next From 5cef2e68dc7745daf73d98d8e78f88d96319b46a Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Thu, 11 Sep 2025 16:43:24 +0200 Subject: [PATCH 4/5] Revert "fixup import" This reverts commit 8da1a1581d7479767d020a9d19d846b11d2968c9. --- packages/shield-controller/src/ShieldController.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/shield-controller/src/ShieldController.ts b/packages/shield-controller/src/ShieldController.ts index 20e88084e99..4e8a31fc4d4 100644 --- a/packages/shield-controller/src/ShieldController.ts +++ b/packages/shield-controller/src/ShieldController.ts @@ -3,9 +3,10 @@ import type { ControllerStateChangeEvent, RestrictedMessenger, } from '@metamask/base-controller'; -import type { - SignatureRequest, - SignatureStateChange, +import { + SignatureRequestType, + type SignatureRequest, + type SignatureStateChange, } from '@metamask/signature-controller'; import type { TransactionControllerStateChangeEvent, @@ -222,7 +223,7 @@ export class ShieldController extends BaseController< // `personal_sign`. if ( !previousSignatureRequest && - signatureRequest.type === 'personal_sign' + signatureRequest.type === SignatureRequestType.PersonalSign ) { this.checkSignatureCoverage(signatureRequest).catch( // istanbul ignore next From a3a219700678b71f8113c90b01fc8f76ee4f18f8 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Thu, 11 Sep 2025 16:49:51 +0200 Subject: [PATCH 5/5] fixup dependencies --- packages/shield-controller/package.json | 1 + yarn.lock | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/shield-controller/package.json b/packages/shield-controller/package.json index 3b8cf6e7609..f446e2840ee 100644 --- a/packages/shield-controller/package.json +++ b/packages/shield-controller/package.json @@ -55,6 +55,7 @@ "@lavamoat/allow-scripts": "^3.0.4", "@lavamoat/preinstall-always-fail": "^2.1.0", "@metamask/auto-changelog": "^3.4.4", + "@metamask/signature-controller": "^33.0.0", "@metamask/transaction-controller": "^60.2.0", "@ts-bridge/cli": "^0.6.1", "@types/jest": "^27.4.1", diff --git a/yarn.lock b/yarn.lock index f432ee14f57..23472b5254f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4478,6 +4478,7 @@ __metadata: "@lavamoat/preinstall-always-fail": "npm:^2.1.0" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^8.3.0" + "@metamask/signature-controller": "npm:^33.0.0" "@metamask/transaction-controller": "npm:^60.2.0" "@metamask/utils": "npm:^11.4.2" "@ts-bridge/cli": "npm:^0.6.1" @@ -4495,7 +4496,7 @@ __metadata: languageName: unknown linkType: soft -"@metamask/signature-controller@workspace:packages/signature-controller": +"@metamask/signature-controller@npm:^33.0.0, @metamask/signature-controller@workspace:packages/signature-controller": version: 0.0.0-use.local resolution: "@metamask/signature-controller@workspace:packages/signature-controller" dependencies: