diff --git a/packages/rpc-methods/src/restricted/invokeSnap.ts b/packages/rpc-methods/src/restricted/invokeSnap.ts index 9d99d62941..79a6e22fb7 100644 --- a/packages/rpc-methods/src/restricted/invokeSnap.ts +++ b/packages/rpc-methods/src/restricted/invokeSnap.ts @@ -14,7 +14,7 @@ import { HandlerType, SnapRpcHookArgs, SnapCaveatType, - validateSnapId, + assertIsValidSnapId, } from '@metamask/snaps-utils'; import { isJsonRpcRequest, @@ -71,7 +71,7 @@ export function validateCaveat(caveat: Caveat) { } const snapIds = Object.keys(caveat.value); for (const snapId of snapIds) { - validateSnapId(snapId); + assertIsValidSnapId(snapId); } } diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index e93171848b..396f7132b8 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -2377,18 +2377,31 @@ describe('SnapController', () => { controller.installSnaps(MOCK_ORIGIN, { [snapId]: {}, }), - ).rejects.toThrow('Invalid snap id. Unknown prefix. Received: "foo".'); + ).rejects.toThrow( + `Invalid snap ID: Expected the value to satisfy a union of \`intersection | string\`, but received: "foo".`, + ); }); it('returns an error if an origin does not have the permission to install a snap', async () => { - const controller = getSnapController(); + const rootMessenger = getControllerMessenger(); + + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + () => ({}), + ); + + const controller = getSnapController( + getSnapControllerOptions({ + messenger: getSnapControllerMessenger(rootMessenger), + }), + ); await expect( controller.installSnaps(MOCK_ORIGIN, { - bar: {}, + [MOCK_SNAP_ID]: {}, }), ).rejects.toThrow( - `Not authorized to install snap "bar". Request the permission for the snap before attempting to install it.`, + `Not authorized to install snap "${MOCK_SNAP_ID}". Request the permission for the snap before attempting to install it.`, ); }); @@ -2750,15 +2763,15 @@ describe('SnapController', () => { }); describe('updateSnap', () => { - it('throws an error on invalid snap id', async () => { + it('throws an error for non installed snap', async () => { const detectSnapLocation = loopbackDetect(); await expect(async () => getSnapController().updateSnap( MOCK_ORIGIN, - 'local:foo', + MOCK_LOCAL_SNAP_ID, detectSnapLocation(), ), - ).rejects.toThrow('Snap "local:foo" not found'); + ).rejects.toThrow(`Snap "${MOCK_LOCAL_SNAP_ID}" not found.`); }); it('throws an error if the specified SemVer range is invalid', async () => { diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index b4aafdcdfe..fa82528c8b 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -53,7 +53,7 @@ import { TruncatedSnap, TruncatedSnapFields, ValidatedSnapId, - validateSnapId, + assertIsValidSnapId, validateSnapShasum, VirtualFile, logError, @@ -570,15 +570,14 @@ type SnapControllerArgs = { detectSnapLocation?: typeof detectSnapLocation; }; type AddSnapArgs = { - id: SnapId; + id: ValidatedSnapId; origin: string; location: SnapLocation; }; // When we set a snap, we need all required properties to be present and // validated. -type SetSnapArgs = Omit & { - id: ValidatedSnapId; +type SetSnapArgs = Omit & { manifest: VirtualFile; files: VirtualFile[]; /** @@ -1599,6 +1598,8 @@ export class SnapController extends BaseController< for (const [snapId, { version: rawVersion }] of Object.entries( requestedSnaps, )) { + assertIsValidSnapId(snapId); + const [error, version] = resolveVersionRange(rawVersion); if (error) { @@ -1664,11 +1665,9 @@ export class SnapController extends BaseController< */ private async processRequestedSnap( origin: string, - snapId: SnapId, + snapId: ValidatedSnapId, versionRange: SemVerRange, ): Promise { - validateSnapId(snapId); - const location = this.#detectSnapLocation(snapId, { versionRange, fetch: this.#fetchFunction, @@ -1897,7 +1896,6 @@ export class SnapController extends BaseController< */ async #add(args: AddSnapArgs): Promise { const { id: snapId, location } = args; - validateSnapId(snapId); this.#setupRuntime(snapId, { sourceCode: null, state: null }); const runtime = this.#getRuntimeExpect(snapId); diff --git a/packages/snaps-utils/jest.config.js b/packages/snaps-utils/jest.config.js index 3415e2e1d0..af6ee57b22 100644 --- a/packages/snaps-utils/jest.config.js +++ b/packages/snaps-utils/jest.config.js @@ -19,9 +19,9 @@ const coveragePathIgnorePatterns = [ module.exports = deepmerge(baseConfig, { coverageThreshold: { global: { - branches: 91.43, + branches: 91.41, functions: 100, - lines: 98.93, + lines: 98.94, statements: 98.97, }, }, diff --git a/packages/snaps-utils/src/snaps.test.ts b/packages/snaps-utils/src/snaps.test.ts index aea236b9ba..e20bc8363a 100644 --- a/packages/snaps-utils/src/snaps.test.ts +++ b/packages/snaps-utils/src/snaps.test.ts @@ -11,31 +11,57 @@ import { isCaipChainId, LocalSnapIdStruct, NpmSnapIdStruct, - validateSnapId, + assertIsValidSnapId, verifyRequestedSnapPermissions, } from './snaps'; -import { SnapIdPrefixes, uri, WALLET_SNAP_PERMISSION_KEY } from './types'; +import { uri, WALLET_SNAP_PERMISSION_KEY } from './types'; -describe('validateSnapId', () => { +describe('assertIsValidSnapId', () => { it.each([undefined, {}, null, true, 2])( 'throws for non-strings (#%#)', (value) => { - expect(() => validateSnapId(value)).toThrow( - 'Invalid snap id. Not a string.', + expect(() => assertIsValidSnapId(value)).toThrow( + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + `Invalid snap ID: Expected the value to satisfy a union of \`intersection | string\`, but received: ${value}.`, ); }, ); it('throws for invalid snap id', () => { - expect(() => validateSnapId('foo:bar')).toThrow( - 'Invalid snap id. Unknown prefix.', + expect(() => assertIsValidSnapId('foo:bar')).toThrow( + `Invalid snap ID: Expected the value to satisfy a union of \`intersection | string\`, but received: "foo:bar".`, ); }); - it.each(Object.values(SnapIdPrefixes))( - 'returns with "%s" prefix', - (prefix) => { - expect(() => validateSnapId(`${prefix}bar`)).not.toThrow(); + it('supports valid NPM IDs', () => { + expect(() => + assertIsValidSnapId('npm:@metamask/test-snap-bip44'), + ).not.toThrow(); + }); + + it('supports valid local IDs', () => { + expect(() => + assertIsValidSnapId('local:http://localhost:8000'), + ).not.toThrow(); + }); + + it.each([ + ' local:http://localhost:8000', + 'local:http://localhost:8000 ', + 'local:http://localhost:8000\n', + 'local:http://localhost:8000\r', + ])('disallows whitespace #%#', (value) => { + expect(() => assertIsValidSnapId(value)).toThrow( + /Invalid snap ID: Expected the value to satisfy a union of `intersection \| string`, but received: .+\./u, + ); + }); + + it.each(['local:😎', 'local:␡'])( + 'disallows non-ASCII symbols #%#', + (value) => { + expect(() => assertIsValidSnapId(value)).toThrow( + `Invalid snap ID: Expected the value to satisfy a union of \`intersection | string\`, but received: "${value}".`, + ); }, ); }); diff --git a/packages/snaps-utils/src/snaps.ts b/packages/snaps-utils/src/snaps.ts index 3da016f9cb..b30dcfcfa6 100644 --- a/packages/snaps-utils/src/snaps.ts +++ b/packages/snaps-utils/src/snaps.ts @@ -4,7 +4,14 @@ import { PermissionConstraint, } from '@metamask/permission-controller'; import { BlockReason } from '@metamask/snaps-registry'; -import { assert, Json, SemVerVersion, isObject } from '@metamask/utils'; +import { + assert, + Json, + SemVerVersion, + isObject, + Opaque, + assertStruct, +} from '@metamask/utils'; import { base64 } from '@scure/base'; import { SerializedEthereumRpcError } from 'eth-rpc-errors/dist/classes'; import stableStringify from 'fast-json-stable-stringify'; @@ -13,9 +20,11 @@ import { enums, intersection, literal, + pattern, refine, string, Struct, + union, validate, } from 'superstruct'; import validateNPMPackage from 'validate-npm-package-name'; @@ -209,8 +218,6 @@ export function getSnapChecksum( return base64.encode(checksumFiles(all as VirtualFile[])); } -export type ValidatedSnapId = `local:${string}` | `npm:${string}`; - /** * Checks whether the `source.shasum` property of a Snap manifest matches the * shasum of the snap. @@ -232,25 +239,32 @@ export function validateSnapShasum( export const LOCALHOST_HOSTNAMES = ['localhost', '127.0.0.1', '[::1]'] as const; +// Require snap ids to only consist of printable ASCII characters +export const BaseSnapIdStruct = pattern(string(), /^[\x21-\x7E]*$/u); + const LocalSnapIdSubUrlStruct = uri({ protocol: enums(['http:', 'https:']), hostname: enums(LOCALHOST_HOSTNAMES), hash: empty(string()), search: empty(string()), }); -export const LocalSnapIdStruct = refine(string(), 'local Snap Id', (value) => { - if (!value.startsWith(SnapIdPrefixes.local)) { - return `Expected local Snap ID, got "${value}".`; - } +export const LocalSnapIdStruct = refine( + BaseSnapIdStruct, + 'local Snap Id', + (value) => { + if (!value.startsWith(SnapIdPrefixes.local)) { + return `Expected local snap ID, got "${value}".`; + } - const [error] = validate( - value.slice(SnapIdPrefixes.local.length), - LocalSnapIdSubUrlStruct, - ); - return error ?? true; -}); + const [error] = validate( + value.slice(SnapIdPrefixes.local.length), + LocalSnapIdSubUrlStruct, + ); + return error ?? true; + }, +); export const NpmSnapIdStruct = intersection([ - string(), + BaseSnapIdStruct, uri({ protocol: literal(SnapIdPrefixes.npm), pathname: refine(string(), 'package name', function* (value) { @@ -273,7 +287,7 @@ export const NpmSnapIdStruct = intersection([ ]) as unknown as Struct; export const HttpSnapIdStruct = intersection([ - string(), + BaseSnapIdStruct, uri({ protocol: enums(['http:', 'https:']), search: empty(string()), @@ -281,6 +295,11 @@ export const HttpSnapIdStruct = intersection([ }), ]) as unknown as Struct; +export const SnapIdStruct = union([NpmSnapIdStruct, LocalSnapIdStruct]); + +export type ValidatedSnapId = Opaque; +declare const snapIdSymbol: unique symbol; + /** * Extracts the snap prefix from a snap ID. * @@ -298,25 +317,15 @@ export function getSnapPrefix(snapId: string): SnapIdPrefixes { } /** - * Asserts the provided object is a snapId with a supported prefix. + * Assert that the given value is a valid snap ID. * - * @param snapId - The object to validate. - * @throws {@link Error}. If the validation fails. + * @param value - The value to check. + * @throws If the value is not a valid snap ID. */ -export function validateSnapId( - snapId: unknown, -): asserts snapId is ValidatedSnapId { - if (!snapId || typeof snapId !== 'string') { - throw new Error(`Invalid snap id. Not a string.`); - } - - for (const prefix of Object.values(SnapIdPrefixes)) { - if (snapId.startsWith(prefix) && snapId.replace(prefix, '').length > 0) { - return; - } - } - - throw new Error(`Invalid snap id. Unknown prefix. Received: "${snapId}".`); +export function assertIsValidSnapId( + value: unknown, +): asserts value is ValidatedSnapId { + assertStruct(value, SnapIdStruct, 'Invalid snap ID'); } /** diff --git a/packages/snaps-utils/src/test-utils/snap.ts b/packages/snaps-utils/src/test-utils/snap.ts index a3b9b789c2..178e8c5ca2 100644 --- a/packages/snaps-utils/src/test-utils/snap.ts +++ b/packages/snaps-utils/src/test-utils/snap.ts @@ -1,6 +1,12 @@ import { SemVerVersion } from '@metamask/utils'; -import { PersistedSnap, Snap, SnapStatus, TruncatedSnap } from '../snaps'; +import { + PersistedSnap, + Snap, + SnapStatus, + TruncatedSnap, + ValidatedSnapId, +} from '../snaps'; import { MakeSemVer } from './common'; import { DEFAULT_SNAP_BUNDLE, @@ -8,8 +14,9 @@ import { getSnapManifest, } from './manifest'; -export const MOCK_SNAP_ID = 'npm:@metamask/example-snap'; -export const MOCK_LOCAL_SNAP_ID = 'local:http://localhost:8080'; +export const MOCK_SNAP_ID = 'npm:@metamask/example-snap' as ValidatedSnapId; +export const MOCK_LOCAL_SNAP_ID = + 'local:http://localhost:8080' as ValidatedSnapId; export const MOCK_ORIGIN = 'example.com'; type GetPersistedSnapObjectOptions = Partial>;