diff --git a/packages/snap-account-service/CHANGELOG.md b/packages/snap-account-service/CHANGELOG.md index 46b2aef97e2..625d185d75c 100644 --- a/packages/snap-account-service/CHANGELOG.md +++ b/packages/snap-account-service/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Report migration error in case of failure ([#9696](https://github.com/MetaMask/core/pull/9696)) + - The migration is a mandatory and it is not supposed to fail. + - Reporting should help identifying which step could have failed and help addressing the actual issue. + ### Changed - Bump `@metamask/account-tree-controller` from `^7.5.4` to `^7.5.5` ([#9470](https://github.com/MetaMask/core/pull/9470)) diff --git a/packages/snap-account-service/src/SnapAccountService.test.ts b/packages/snap-account-service/src/SnapAccountService.test.ts index 00a09e3fa43..42063c1e2b4 100644 --- a/packages/snap-account-service/src/SnapAccountService.test.ts +++ b/packages/snap-account-service/src/SnapAccountService.test.ts @@ -39,6 +39,7 @@ import type { SnapControllerState } from '@metamask/snaps-controllers'; import type { SnapId } from '@metamask/snaps-sdk'; import type { TruncatedSnap } from '@metamask/snaps-utils'; +import { SafeError } from './errors.js'; import type { SnapAccountServiceMessenger, SnapAccountServiceOptions, @@ -89,10 +90,13 @@ type Mocks = { /** * Constructs the root messenger for the service under test. * + * @param captureException - Optional method to capture exceptions in Sentry. * @returns The root messenger. */ -function getRootMessenger(): RootMessenger { - return new Messenger({ namespace: MOCK_ANY_NAMESPACE }); +function getRootMessenger( + captureException?: (error: Error) => void, +): RootMessenger { + return new Messenger({ namespace: MOCK_ANY_NAMESPACE, captureException }); } /** @@ -400,23 +404,26 @@ function mockWithKeyringV2Unsafe( * @param args.snapIsReady - Initial value of `SnapController.isReady`. * @param args.runnableSnaps - Snaps returned by `SnapController:getRunnableSnaps`. * @param args.config - Optional service config. + * @param args.captureException - Optional method to capture exceptions in Sentry. * @returns The new service, root messenger, service messenger, and mocks. */ async function setup({ snapIsReady = true, runnableSnaps = [], config, + captureException, }: { snapIsReady?: boolean; runnableSnaps?: TruncatedSnap[]; config?: SnapAccountServiceOptions['config']; + captureException?: (error: Error) => void; } = {}): Promise<{ service: SnapAccountService; rootMessenger: RootMessenger; messenger: SnapAccountServiceMessenger; mocks: Mocks; }> { - const rootMessenger = getRootMessenger(); + const rootMessenger = getRootMessenger(captureException); const messenger = getMessenger(rootMessenger); const mocks: Mocks = { @@ -614,6 +621,116 @@ describe('SnapAccountService', () => { expect(removeKeyring).toHaveBeenCalledWith(legacyKeyringId); }); + it('wraps unknown addNewKeyring failures as SafeError', async () => { + const legacyKeyringId = 'legacy-id'; + const legacyKeyring = { + type: SNAP_KEYRING_TYPE, + listAccounts: jest.fn().mockReturnValue([ + { + id: 'a1', + address: '0x1', + metadata: { snap: { id: MOCK_SNAP_ID } }, + }, + ]), + }; + const { service, mocks } = await setup(); + const addNewKeyring = jest + .fn() + .mockRejectedValue(new Error('superstruct: address 0x1 invalid')); + mocks.KeyringController.withController.mockImplementation( + async (operation) => + operation({ + keyrings: [ + { keyring: legacyKeyring, metadata: { id: legacyKeyringId } }, + ], + addNewKeyring, + removeKeyring: jest.fn(), + }), + ); + + await expect(service.ensureMigrated()).rejects.toThrow(SafeError); + }); + + it('re-throws KeyringControllerError from addNewKeyring as-is', async () => { + const legacyKeyringId = 'legacy-id'; + const legacyKeyring = { + type: SNAP_KEYRING_TYPE, + listAccounts: jest.fn().mockReturnValue([ + { + id: 'a1', + address: '0x1', + metadata: { snap: { id: MOCK_SNAP_ID } }, + }, + ]), + }; + const { service, mocks } = await setup(); + const error = new KeyringControllerError( + KeyringControllerErrorMessage.DuplicatedAccount, + ); + const addNewKeyring = jest.fn().mockRejectedValue(error); + mocks.KeyringController.withController.mockImplementation( + async (operation) => + operation({ + keyrings: [ + { keyring: legacyKeyring, metadata: { id: legacyKeyringId } }, + ], + addNewKeyring, + removeKeyring: jest.fn(), + }), + ); + + await expect(service.ensureMigrated()).rejects.toThrow(error); + }); + + it('wraps unknown removeKeyring failures as SafeError', async () => { + const legacyKeyringId = 'legacy-id'; + const legacyKeyring = { + type: SNAP_KEYRING_TYPE, + listAccounts: jest.fn().mockReturnValue([]), + }; + const { service, mocks } = await setup(); + const removeKeyring = jest + .fn() + .mockRejectedValue(new Error('unexpected internal error')); + mocks.KeyringController.withController.mockImplementation( + async (operation) => + operation({ + keyrings: [ + { keyring: legacyKeyring, metadata: { id: legacyKeyringId } }, + ], + addNewKeyring: jest.fn(), + removeKeyring, + }), + ); + + await expect(service.ensureMigrated()).rejects.toThrow(SafeError); + }); + + it('re-throws KeyringControllerError from removeKeyring as-is', async () => { + const legacyKeyringId = 'legacy-id'; + const legacyKeyring = { + type: SNAP_KEYRING_TYPE, + listAccounts: jest.fn().mockReturnValue([]), + }; + const { service, mocks } = await setup(); + const error = new KeyringControllerError( + KeyringControllerErrorMessage.KeyringNotFound, + ); + const removeKeyring = jest.fn().mockRejectedValue(error); + mocks.KeyringController.withController.mockImplementation( + async (operation) => + operation({ + keyrings: [ + { keyring: legacyKeyring, metadata: { id: legacyKeyringId } }, + ], + addNewKeyring: jest.fn(), + removeKeyring, + }), + ); + + await expect(service.ensureMigrated()).rejects.toThrow(error); + }); + it('does not re-run after a successful migration', async () => { const { service, mocks } = await setup(); mocks.KeyringController.withController.mockResolvedValue(undefined); @@ -1390,9 +1507,24 @@ describe('SnapAccountService', () => { expect(service).toBeDefined(); }); - it('logs an error when migration fails', async () => { - const { service, rootMessenger, mocks } = await setup(); - const error = new Error('migration boom'); + it.each([ + { + name: 'SafeError', + error: new SafeError( + 'Adding v2 Snap keyring for "npm:test-snap": Validation failed at "accounts.id.type" (expected: enums)', + ), + }, + { + name: 'KeyringControllerError', + error: new KeyringControllerError( + KeyringControllerErrorMessage.DuplicatedAccount, + ), + }, + ])('passes a $name through to Sentry as-is', async ({ error }) => { + const captureException = jest.fn(); + const { service, rootMessenger, mocks } = await setup({ + captureException, + }); mocks.KeyringController.withController.mockRejectedValueOnce(error); const consoleErrorSpy = jest .spyOn(console, 'error') @@ -1402,9 +1534,49 @@ describe('SnapAccountService', () => { await flushMicrotasks(); expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Migration failed after unlock:', + 'Migration failed after unlock', error, ); + expect(captureException).toHaveBeenCalledWith( + expect.objectContaining({ + message: 'Migration failed after unlock', + cause: error, + }), + ); + expect(service).toBeDefined(); + consoleErrorSpy.mockRestore(); + }); + + it('replaces an unknown error with a generic SafeError before reporting to Sentry', async () => { + const captureException = jest.fn(); + const { service, rootMessenger, mocks } = await setup({ + captureException, + }); + // An error from snap keyring internals or superstruct could contain + // account addresses in its message — withSafeError replaces it with a safe one. + mocks.KeyringController.withController.mockRejectedValueOnce( + new Error('internal error with address 0x1234'), + ); + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + + publishUnlock(rootMessenger); + await flushMicrotasks(); + + const safeError = expect.objectContaining({ + message: 'Snap keyring v2 migration: An unexpected error occurred', + }); + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Migration failed after unlock', + safeError, + ); + expect(captureException).toHaveBeenCalledWith( + expect.objectContaining({ + message: 'Migration failed after unlock', + cause: safeError, + }), + ); expect(service).toBeDefined(); consoleErrorSpy.mockRestore(); }); @@ -1423,8 +1595,9 @@ describe('SnapAccountService', () => { mocks.AccountTreeController.getAccountGroupObject.mockReturnValue( buildGroup(MOCK_GROUP_ID, MOCK_ACCOUNTS), ); - const error = new Error('migration boom'); - mocks.KeyringController.withController.mockRejectedValueOnce(error); + mocks.KeyringController.withController.mockRejectedValueOnce( + new Error('migration boom'), + ); const consoleErrorSpy = jest .spyOn(console, 'error') .mockImplementation(() => undefined); @@ -1437,8 +1610,8 @@ describe('SnapAccountService', () => { mocks.KeyringController.withKeyringV2Unsafe, ).not.toHaveBeenCalled(); expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Migration failed after unlock:', - error, + 'Migration failed after unlock', + expect.any(Error), ); expect(service).toBeDefined(); consoleErrorSpy.mockRestore(); diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index c357197499f..441f3d2dda5 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -65,6 +65,7 @@ import { SnapId } from '@metamask/snaps-sdk'; import type { Json } from '@metamask/utils'; import { assertStruct } from '@metamask/utils'; +import { reportError, withSafeError } from './errors.js'; import { projectLogger as log } from './logger.js'; import type { SnapAccountServiceEnsureReadyAction, @@ -343,18 +344,23 @@ export class SnapAccountService { */ #handleUnlock(): void { // eslint-disable-next-line no-void - void this.ensureMigrated() - .then(async () => { + void withSafeError('Snap keyring v2 migration', () => + this.ensureMigrated(), + ).then( + async () => { // If the migration is successful, we re-forward the current groups to each new keyrings! const groupId = this.#getSelectedAccountGroupId(); + // NOTE: This cannot throw, all errors are swallowed under the hood, so we don't need to + // catch anything here. return await this.#forwardSelectedAccounts( groupId, this.#getAccountGroup(groupId)?.accounts, ); - }) - .catch((error) => { - console.error('Migration failed after unlock:', error); - }); + }, + (error) => { + reportError(this.#messenger, 'Migration failed after unlock', error); + }, + ); } /** @@ -508,18 +514,24 @@ export class SnapAccountService { // accounts over. for (const state of states.values()) { log(`Migrating accounts for Snap "${state.snapId}"...`); - await controller.addNewKeyring( - // IMPORTANT: The Snap keyring (v2) can also be used as a v1 - // keyring. So the builder associated with the v2 keyring type is - // able to build both v1 and v2 keyrings. - KeyringType.Snap, - state, + await withSafeError( + `Adding v2 Snap keyring for "${state.snapId}"`, + async () => + controller.addNewKeyring( + // IMPORTANT: The Snap keyring (v2) can also be used as a v1 + // keyring. So the builder associated with the v2 keyring type + // is able to build both v1 and v2 keyrings. + KeyringType.Snap, + state, + ), ); } // Remove the legacy Snap keyring after migration. log('Removing legacy Snap keyring...'); - await controller.removeKeyring(legacySnapKeyringEntry.metadata.id); + await withSafeError('Removing legacy Snap keyring', async () => + controller.removeKeyring(legacySnapKeyringEntry.metadata.id), + ); log('Migration completed!'); }, diff --git a/packages/snap-account-service/src/SnapTracker.test.ts b/packages/snap-account-service/src/SnapTracker.test.ts index ec030fc0994..1a5d86b77e7 100644 --- a/packages/snap-account-service/src/SnapTracker.test.ts +++ b/packages/snap-account-service/src/SnapTracker.test.ts @@ -347,5 +347,13 @@ describe('SnapTracker', () => { expect(tracker.getSnaps()).toStrictEqual([]); }); + + it('does nothing when a removal event fires for an untracked Snap', () => { + const { tracker, rootMessenger } = setup(); + + publishSnapDisabled(rootMessenger, buildSnap(MOCK_SNAP_ID, true)); + + expect(tracker.getSnaps()).toStrictEqual([]); + }); }); }); diff --git a/packages/snap-account-service/src/errors.test.ts b/packages/snap-account-service/src/errors.test.ts new file mode 100644 index 00000000000..a5d18c121e6 --- /dev/null +++ b/packages/snap-account-service/src/errors.test.ts @@ -0,0 +1,197 @@ +import { + KeyringControllerError, + KeyringControllerErrorMessage, +} from '@metamask/keyring-controller'; + +import { + createSentryError, + isSafeError, + reportError, + SafeError, + withSafeError, +} from './errors.js'; + +/** + * Builds a minimal duck-typed StructError for testing the superstruct branch. + * + * @param path - The path to the failing field. + * @param type - The expected superstruct type string. + * @param refinement - Optional refinement name. + * @returns A TypeError whose shape matches StructError. + */ +function buildStructError( + path: string[], + type: string, + refinement?: string, +): Error { + return Object.assign( + new TypeError( + `At path: ${path.join('.')} -- Expected a value of type \`${type}\``, + ), + { name: 'StructError' as const, path, type, refinement }, + ); +} + +describe('SafeError', () => { + it('is an Error with name SafeError', () => { + const error = new SafeError('step failed'); + expect(error).toBeInstanceOf(Error); + expect(error.name).toBe('SafeError'); + expect(error.message).toBe('step failed'); + }); +}); + +describe('isSafeError', () => { + it('returns true for a SafeError', () => { + expect(isSafeError(new SafeError('step failed'))).toBe(true); + }); + + it('returns true for a duck-typed object with name SafeError', () => { + expect(isSafeError({ name: 'SafeError', message: 'step failed' })).toBe( + true, + ); + }); + + it('returns false for a plain Error', () => { + expect(isSafeError(new Error('oops'))).toBe(false); + }); + + it('returns false for null and non-objects', () => { + expect(isSafeError(null)).toBe(false); + expect(isSafeError('string')).toBe(false); + expect(isSafeError(undefined)).toBe(false); + }); +}); + +describe('withSafeError', () => { + it('returns the callback result on success', async () => { + expect(await withSafeError('step', async () => 42)).toBe(42); + }); + + it('passes a SafeError through unchanged', async () => { + const inner = new SafeError('inner step: detail'); + await expect( + withSafeError('outer step', async () => { + throw inner; + }), + ).rejects.toThrow(inner); + }); + + it('passes a KeyringControllerError through unchanged', async () => { + const error = new KeyringControllerError( + KeyringControllerErrorMessage.DuplicatedAccount, + ); + await expect( + withSafeError('step', async () => { + throw error; + }), + ).rejects.toThrow(error); + }); + + it('wraps a StructError as a SafeError with path and type', async () => { + const structError = buildStructError( + ['accounts', 'uuid-123', 'type'], + 'enums', + ); + await expect( + withSafeError('Adding keyring', async () => { + throw structError; + }), + ).rejects.toThrow( + new SafeError( + 'Adding keyring: Validation failed at "accounts.uuid-123.type" (expected: enums)', + ), + ); + }); + + it('uses refinement name over type when present', async () => { + const structError = buildStructError(['address'], 'string', 'nonempty'); + await expect( + withSafeError('step', async () => { + throw structError; + }), + ).rejects.toThrow( + new SafeError( + 'step: Validation failed at "address" (expected: nonempty)', + ), + ); + }); + + it('uses "root" when StructError path is empty', async () => { + const structError = buildStructError([], 'object'); + await expect( + withSafeError('step', async () => { + throw structError; + }), + ).rejects.toThrow( + new SafeError('step: Validation failed at (expected: object)'), + ); + }); + + it('replaces an unknown error with a generic SafeError', async () => { + await expect( + withSafeError('Adding keyring', async () => { + throw new Error('internal error with address 0x1234'); + }), + ).rejects.toThrow( + new SafeError('Adding keyring: An unexpected error occurred'), + ); + }); + + it('re-forwards a SafeError from a nested withSafeError without re-wrapping', async () => { + const inner = new SafeError('inner: detail'); + const result = withSafeError('outer', async () => + withSafeError('inner', async () => { + throw inner; + }), + ); + await expect(result).rejects.toThrow(inner); + }); +}); + +describe('createSentryError', () => { + it('creates an error with a cause', () => { + const inner = new Error('inner'); + const result = createSentryError('outer', inner); + + expect(result.message).toBe('outer'); + expect(result.cause).toBe(inner); + expect(result.context).toBeUndefined(); + }); + + it('attaches context when provided', () => { + const inner = new Error('inner'); + const context = { snapId: 'npm:test-snap' }; + const result = createSentryError('outer', inner, context); + + expect(result.context).toStrictEqual(context); + }); +}); + +describe('reportError', () => { + beforeEach(() => { + jest.spyOn(console, 'error').mockImplementation(); + }); + + it('logs to console.error', () => { + const error = new Error('boom'); + reportError({}, 'Something failed', error); + + expect(console.error).toHaveBeenCalledWith('Something failed', error); + }); + + it('calls captureException when provided', () => { + const error = new Error('boom'); + const captureException = jest.fn(); + reportError({ captureException }, 'Something failed', error); + + expect(captureException).toHaveBeenCalledWith( + expect.objectContaining({ message: 'Something failed', cause: error }), + ); + }); + + it('does not throw when captureException is absent', () => { + const error = new Error('boom'); + expect(() => reportError({}, 'Something failed', error)).not.toThrow(); + }); +}); diff --git a/packages/snap-account-service/src/errors.ts b/packages/snap-account-service/src/errors.ts new file mode 100644 index 00000000000..6d5932cdd50 --- /dev/null +++ b/packages/snap-account-service/src/errors.ts @@ -0,0 +1,189 @@ +import { isKeyringControllerError } from '@metamask/keyring-controller'; + +/** + * Thrown by {@link withSafeError} when the wrapped callback fails with an error + * whose message is not safe to forward to Sentry (e.g., a superstruct + * `StructError` that may embed account addresses, or an unknown third-party + * error). The message on a `SafeError` is always authored by us and contains + * no user data. + * + * `SafeError` instances pass through nested {@link withSafeError} calls unchanged; + * the innermost step description is preserved rather than re-wrapped. + */ +export class SafeError extends Error { + constructor(message: string) { + super(message); + this.name = 'SafeError'; + } +} + +/** + * Returns `true` if the error is a `SafeError`. + * + * Uses duck-typing on `error.name` rather than `instanceof` so that the check + * remains correct when multiple versions of this package coexist in the + * dependency tree (different versions produce different classes, so + * `instanceof` would return `false` for errors from another version). + * + * @param error - The value to check. + * @returns Whether the error is a `SafeError`. + */ +export function isSafeError(error: unknown): error is SafeError { + return ( + typeof error === 'object' && + error !== null && + (error as { name?: unknown }).name === 'SafeError' + ); +} + +/** + * Duck-typed shape of `@metamask/superstruct`'s `StructError`. + * + * We avoid a direct `instanceof` check because `@metamask/superstruct` is not + * a declared dependency of this package; it reaches us transitively. Checking + * `name` plus the presence of `path` and `type` is sufficient to identify it + * without coupling to the transitive dep. + */ +type StructErrorLike = Error & { + name: 'StructError'; + path: unknown[]; + type: string; + refinement?: string; +}; + +/** + * Returns `true` if the error is a `StructError`-like object. + * + * @param error - The value to check. + * @returns Whether the error is a `StructError`-like object. + */ +function isStructError(error: unknown): error is StructErrorLike { + const isError = error instanceof Error && error.name === 'StructError'; + if (!isError) { + return false; + } + + const structError = error as { + path?: string[]; + type?: string; + }; + return ( + Array.isArray(structError.path) && typeof structError.type === 'string' + ); +} + +/** + * Produces a safe, human-readable summary of a superstruct validation failure. + * Reports only the field path and expected type, never the actual failing + * value, which may contain account addresses or other user data. + * + * @param error - The StructError-like object. + * @returns A safe description string. + */ +function describeStructFailure(error: StructErrorLike): string { + const path = + error.path.length > 0 ? `"${error.path.map(String).join('.')}"` : ''; + return `Validation failed at ${path} (expected: ${error.refinement ?? error.type})`; +} + +/** + * Executes `fn` and, if it throws, sanitizes the error before re-throwing to + * guarantee the caller always gets an error whose message is safe to forward + * to Sentry. + * + * Sanitization rules applied to whatever `fn` throws: + * - {@link SafeError}: re-thrown as-is (innermost step description preserved, + * no re-wrapping on nested {@link withSafeError} calls). + * - {@link KeyringControllerError}: re-thrown as-is (uses static, enum-based + * messages; no user data). + * - `StructError` (from `@metamask/superstruct`): wrapped in a + * {@link SafeError} containing only the failing field path and expected type, + * never the actual value. + * - Anything else: wrapped in a {@link SafeError} with a generic fallback + * message; the original error is discarded to prevent accidental leakage. + * + * @param step - Human-readable label for this execution step, prepended to the + * `SafeError` message when the error is not already safe. + * @param fn - The async callback to execute. + * @returns The result of `fn`. + */ +export async function withSafeError( + step: string, + fn: () => Promise, +): Promise { + try { + return await fn(); + } catch (error) { + if (isSafeError(error) || isKeyringControllerError(error)) { + throw error; + } + if (isStructError(error)) { + throw new SafeError(`${step}: ${describeStructFailure(error)}`); + } + throw new SafeError(`${step}: An unexpected error occurred`); + } +} + +/** + * Augmented `Error` shape produced by {@link createSentryError}. The runtime + * value carries a `cause` and (optionally) a structured `context` payload + * that downstream Sentry tooling can read. + * + * The `TContext` type parameter narrows the shape of `context` for callers + * that know what they put in — most useful in tests when asserting on a + * captured error. + */ +export type SentryError< + TContext extends Record = Record, +> = Error & { + cause: Error; + context?: TContext; +}; + +/** + * Creates a Sentry error from an error message, an inner error and a context. + * + * NOTE: Sentry defaults to a depth of 3 when extracting non-native attributes. + * As such, the context depth shouldn't be too deep. + * + * @param message - The error message to create a Sentry error from. + * @param innerError - The inner error to create a Sentry error from. + * @param context - The context to add to the Sentry error. + * @returns A Sentry error. + */ +export const createSentryError = < + TContext extends Record = Record, +>( + message: string, + innerError: Error, + context?: TContext, +): SentryError => { + const error = new Error(message) as SentryError; + error.cause = innerError; + if (context) { + error.context = context; + } + return error; +}; + +/** + * Reports an error by logging it to the console and optionally capturing it + * in Sentry via the messenger's `captureException` method. + * + * @param messenger - Object with an optional `captureException` method. + * @param messenger.captureException - Optional method to capture exceptions in Sentry. + * @param message - The static message describing what failed. + * @param error - The caught error. + * @param context - Optional context to attach to the Sentry error. + */ +export function reportError( + messenger: { captureException?: (error: Error) => void }, + message: string, + error: unknown, + context?: Record, +): void { + console.error(message, error); + + const sentryError = createSentryError(message, error as Error, context); + messenger.captureException?.(sentryError); +}