Improve validation and sanitizing of snap IDs (MetaMask#1237)

* Replace validateSnapId with createSnapId that does improved validation and sanitation * Disallow whitespace instead of stripping it * Use assertStruct instead of create * Replace usage of validateSnapId with assertIsValidSnapId * Remove unnecesary type * Address PR comments * Small updates following PR review * Update packages/snaps-controllers/src/snaps/SnapController.test.ts Co-authored-by: Maarten Zuidhoorn <[email protected]> --------- Co-authored-by: Maarten Zuidhoorn <[email protected]>
Shole74 · Feb 27, 2023 · 7e00df5 · 7e00df5
1 parent 57691cc
commit 7e00df5
Show file tree

Hide file tree

Showing 7 changed files with 118 additions and 65 deletions.
diff --git 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<string, any>) {
   }
   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
@@ -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
@@ -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<AddSnapArgs, 'id' | 'location'> & {
-  id: ValidatedSnapId;
+type SetSnapArgs = Omit<AddSnapArgs, 'location'> & {
   manifest: VirtualFile<SnapManifest>;
   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<ProcessSnapResult> {
-    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<PersistedSnap> {
     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
@@ -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
@@ -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
@@ -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,14 +287,19 @@ export const NpmSnapIdStruct = intersection([
 ]) as unknown as Struct<string, null>;
 
 export const HttpSnapIdStruct = intersection([
-  string(),
+  BaseSnapIdStruct,
   uri({
     protocol: enums(['http:', 'https:']),
     search: empty(string()),
     hash: empty(string()),
   }),
 ]) as unknown as Struct<string, null>;
 
+export const SnapIdStruct = union([NpmSnapIdStruct, LocalSnapIdStruct]);
+
+export type ValidatedSnapId = Opaque<string, typeof snapIdSymbol>;
+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
@@ -1,15 +1,22 @@
 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,
   DEFAULT_SNAP_SHASUM,
   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<MakeSemVer<PersistedSnap>>;