From 7710379d1f17c82d26c495df25c0d44e98415b52 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 12 Sep 2024 08:50:35 -0400 Subject: [PATCH 01/22] Add navigation to button --- .../src/jsx/components/form/Button.test.tsx | 13 +++++++++++++ .../snaps-sdk/src/jsx/components/form/Button.ts | 7 +++++++ packages/snaps-sdk/src/jsx/validation.test.tsx | 3 +++ packages/snaps-sdk/src/jsx/validation.ts | 1 + 4 files changed, 24 insertions(+) diff --git a/packages/snaps-sdk/src/jsx/components/form/Button.test.tsx b/packages/snaps-sdk/src/jsx/components/form/Button.test.tsx index 2a1ef1f0d7..1fd03c3c27 100644 --- a/packages/snaps-sdk/src/jsx/components/form/Button.test.tsx +++ b/packages/snaps-sdk/src/jsx/components/form/Button.test.tsx @@ -63,4 +63,17 @@ describe('Button', () => { key: null, }); }); + + it('returns a button element with navigation props', () => { + const result = ; + + expect(result).toStrictEqual({ + type: 'Button', + props: { + children: 'bar', + navigateTo: 'home', + }, + key: null, + }); + }); }); diff --git a/packages/snaps-sdk/src/jsx/components/form/Button.ts b/packages/snaps-sdk/src/jsx/components/form/Button.ts index e41c0e6d01..7fef9488a1 100644 --- a/packages/snaps-sdk/src/jsx/components/form/Button.ts +++ b/packages/snaps-sdk/src/jsx/components/form/Button.ts @@ -1,8 +1,14 @@ +import type { EnumToUnion } from 'src/internals'; + import type { SnapsChildren, StringElement } from '../../component'; import { createSnapComponent } from '../../component'; import type { IconElement } from '../Icon'; import type { ImageElement } from '../Image'; +export enum SnapRoutes { + Home = 'home', +} + // TODO: Add the `onClick` prop to the `ButtonProps` type. /** @@ -25,6 +31,7 @@ export type ButtonProps = { variant?: 'primary' | 'destructive' | undefined; disabled?: boolean | undefined; form?: string | undefined; + navigateTo?: EnumToUnion | undefined; }; const TYPE = 'Button'; diff --git a/packages/snaps-sdk/src/jsx/validation.test.tsx b/packages/snaps-sdk/src/jsx/validation.test.tsx index 8c6137e70b..80eab41046 100644 --- a/packages/snaps-sdk/src/jsx/validation.test.tsx +++ b/packages/snaps-sdk/src/jsx/validation.test.tsx @@ -170,6 +170,7 @@ describe('ButtonStruct', () => { , , + , ])('validates a button element', (value) => { expect(is(value, ButtonStruct)).toBe(true); }); @@ -192,6 +193,8 @@ describe('ButtonStruct', () => { alt , + // @ts-expect-error - Invalid props. + , ])('does not validate "%p"', (value) => { expect(is(value, ButtonStruct)).toBe(false); }); diff --git a/packages/snaps-sdk/src/jsx/validation.ts b/packages/snaps-sdk/src/jsx/validation.ts index e06d762a72..17daafc568 100644 --- a/packages/snaps-sdk/src/jsx/validation.ts +++ b/packages/snaps-sdk/src/jsx/validation.ts @@ -226,6 +226,7 @@ export const ButtonStruct: Describe = element('Button', { variant: optional(nullUnion([literal('primary'), literal('destructive')])), disabled: optional(boolean()), form: optional(string()), + navigateTo: optional(nullUnion([literal('home')])), }); /** From b1c97e90ee4a8b5292d7b7387b95adf25f7000da Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 12 Sep 2024 08:52:19 -0400 Subject: [PATCH 02/22] update shasums --- packages/examples/packages/browserify-plugin/snap.manifest.json | 2 +- packages/examples/packages/browserify/snap.manifest.json | 2 +- packages/examples/packages/dialogs/snap.manifest.json | 2 +- packages/examples/packages/file-upload/snap.manifest.json | 2 +- packages/examples/packages/home-page/snap.manifest.json | 2 +- packages/examples/packages/interactive-ui/snap.manifest.json | 2 +- packages/examples/packages/jsx/snap.manifest.json | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/examples/packages/browserify-plugin/snap.manifest.json b/packages/examples/packages/browserify-plugin/snap.manifest.json index beb24fd426..b2da3d0945 100644 --- a/packages/examples/packages/browserify-plugin/snap.manifest.json +++ b/packages/examples/packages/browserify-plugin/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "N8UDq+EKKrGSUr/xn+g8Fn0ebj34zd+6Urgv1S7pzuM=", + "shasum": "LXvuPFnm5q2GtEtwsOWv56vNWPPArBCdurAUq8QM9N4=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/browserify/snap.manifest.json b/packages/examples/packages/browserify/snap.manifest.json index 74cb179e35..e931e86c04 100644 --- a/packages/examples/packages/browserify/snap.manifest.json +++ b/packages/examples/packages/browserify/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "j2B/JJ/tTdTWMJo+c5S8vZpnt4pRk9Xp3MhYBmDaz1s=", + "shasum": "HXFQkJQ/B4TCbIdC/rN7Zm1F4s2ElUuc7s9bPh+fNRM=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/dialogs/snap.manifest.json b/packages/examples/packages/dialogs/snap.manifest.json index 75edcaa8a6..dbac84f6fa 100644 --- a/packages/examples/packages/dialogs/snap.manifest.json +++ b/packages/examples/packages/dialogs/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "v2oIs3UY5j/3dXW5eRh3rg7Tv5qcVzHon7qzPQeRsAE=", + "shasum": "zuBFS3XWpLP7vKsIJqiecvXakBW9rgzuVToBRS0ItSg=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/file-upload/snap.manifest.json b/packages/examples/packages/file-upload/snap.manifest.json index a34c9a115e..8f0774493b 100644 --- a/packages/examples/packages/file-upload/snap.manifest.json +++ b/packages/examples/packages/file-upload/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "z4vMdrs40TdVE+vk7sPruIvWi0q669V+p3jc6WQIib4=", + "shasum": "exoRIe26MvGPe9XZxn8uq/xBn8uYvzvn0y6L5uTPlKM=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/home-page/snap.manifest.json b/packages/examples/packages/home-page/snap.manifest.json index 3afc05d1c9..715fa52014 100644 --- a/packages/examples/packages/home-page/snap.manifest.json +++ b/packages/examples/packages/home-page/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "emnlXHvrhsGBMi/y1IPhiQPX9ogHmjmOU/iujBu5GB8=", + "shasum": "x94obetFeJZPEwAU/DsNvFj5/eotAwlylCGS7IfM+84=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/interactive-ui/snap.manifest.json b/packages/examples/packages/interactive-ui/snap.manifest.json index 6581945413..6146cc4b6d 100644 --- a/packages/examples/packages/interactive-ui/snap.manifest.json +++ b/packages/examples/packages/interactive-ui/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "uOuMArCEwmOmCl0Bl1WnRRm+DKcq0Y+O+5n8Z1KBMr8=", + "shasum": "iN1fXS+FpkQR21mGkHO0N+xo4nW3HFwsfXOsGTpiM68=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/jsx/snap.manifest.json b/packages/examples/packages/jsx/snap.manifest.json index f9a9c92dcf..4e6b372e68 100644 --- a/packages/examples/packages/jsx/snap.manifest.json +++ b/packages/examples/packages/jsx/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "3U9+Lvmmdc9JRmaHLcwGgi9lpnaj25joBF9+zXY4644=", + "shasum": "Rv/yARDSnZz9twUQ/mUWpNFSjjRkNy68DUy2gTwq+ZA=", "location": { "npm": { "filePath": "dist/bundle.js", From cf1b918069ee2670fc6c806c69b3576cd40c91db Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 12 Sep 2024 08:57:04 -0400 Subject: [PATCH 03/22] recalculate shasums --- packages/examples/packages/browserify-plugin/snap.manifest.json | 2 +- packages/examples/packages/browserify/snap.manifest.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/examples/packages/browserify-plugin/snap.manifest.json b/packages/examples/packages/browserify-plugin/snap.manifest.json index b2da3d0945..ed86327c8c 100644 --- a/packages/examples/packages/browserify-plugin/snap.manifest.json +++ b/packages/examples/packages/browserify-plugin/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "LXvuPFnm5q2GtEtwsOWv56vNWPPArBCdurAUq8QM9N4=", + "shasum": "oqO5wicjNs9i86WgNV2vx79nOYYD/uxnOtAOECtWqzU=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/browserify/snap.manifest.json b/packages/examples/packages/browserify/snap.manifest.json index e931e86c04..f3bd542095 100644 --- a/packages/examples/packages/browserify/snap.manifest.json +++ b/packages/examples/packages/browserify/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "HXFQkJQ/B4TCbIdC/rN7Zm1F4s2ElUuc7s9bPh+fNRM=", + "shasum": "LQJI6F5ptgr0wY9vQ+58Q8Tv8VyMx/8lvkDSmDgdqVM=", "location": { "npm": { "filePath": "dist/bundle.js", From a531f15a6255596cb2224e638d37ce053b984132 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 13 Sep 2024 14:50:23 -0400 Subject: [PATCH 04/22] add icon as link child, introduce metamask urls --- .../src/jsx/components/Link.test.tsx | 22 +++++ packages/snaps-sdk/src/jsx/components/Link.ts | 5 +- .../src/jsx/components/form/Button.test.tsx | 13 --- .../src/jsx/components/form/Button.ts | 7 -- .../snaps-sdk/src/jsx/validation.test.tsx | 17 ++-- packages/snaps-sdk/src/jsx/validation.ts | 3 +- packages/snaps-utils/src/url.test.ts | 55 +++++++++++++ packages/snaps-utils/src/url.ts | 82 +++++++++++++++++++ 8 files changed, 172 insertions(+), 32 deletions(-) create mode 100644 packages/snaps-utils/src/url.test.ts create mode 100644 packages/snaps-utils/src/url.ts diff --git a/packages/snaps-sdk/src/jsx/components/Link.test.tsx b/packages/snaps-sdk/src/jsx/components/Link.test.tsx index a28fedd27f..31752f1a07 100644 --- a/packages/snaps-sdk/src/jsx/components/Link.test.tsx +++ b/packages/snaps-sdk/src/jsx/components/Link.test.tsx @@ -1,3 +1,4 @@ +import { Icon } from './Icon'; import { Link } from './Link'; describe('Link', () => { @@ -27,6 +28,27 @@ describe('Link', () => { }); }); + it('renders a link with an icon', () => { + const result = ( + + + + ); + + expect(result).toStrictEqual({ + type: 'Link', + key: null, + props: { + href: 'metamask://client/', + children: { + type: 'Icon', + key: null, + props: { name: 'arrow-left', size: 'md' }, + }, + }, + }); + }); + it('renders a link with a conditional value', () => { const result = ( diff --git a/packages/snaps-sdk/src/jsx/components/Link.ts b/packages/snaps-sdk/src/jsx/components/Link.ts index 047ba7b0ed..032238cdaa 100644 --- a/packages/snaps-sdk/src/jsx/components/Link.ts +++ b/packages/snaps-sdk/src/jsx/components/Link.ts @@ -1,11 +1,14 @@ import type { SnapsChildren } from '../component'; import { createSnapComponent } from '../component'; import type { StandardFormattingElement } from './formatting'; +import { type IconElement } from './Icon'; /** * The children of the {@link Link} component. */ -export type LinkChildren = SnapsChildren; +export type LinkChildren = SnapsChildren< + string | StandardFormattingElement | IconElement +>; /** * The props of the {@link Link} component. diff --git a/packages/snaps-sdk/src/jsx/components/form/Button.test.tsx b/packages/snaps-sdk/src/jsx/components/form/Button.test.tsx index 1fd03c3c27..2a1ef1f0d7 100644 --- a/packages/snaps-sdk/src/jsx/components/form/Button.test.tsx +++ b/packages/snaps-sdk/src/jsx/components/form/Button.test.tsx @@ -63,17 +63,4 @@ describe('Button', () => { key: null, }); }); - - it('returns a button element with navigation props', () => { - const result = ; - - expect(result).toStrictEqual({ - type: 'Button', - props: { - children: 'bar', - navigateTo: 'home', - }, - key: null, - }); - }); }); diff --git a/packages/snaps-sdk/src/jsx/components/form/Button.ts b/packages/snaps-sdk/src/jsx/components/form/Button.ts index 7fef9488a1..e41c0e6d01 100644 --- a/packages/snaps-sdk/src/jsx/components/form/Button.ts +++ b/packages/snaps-sdk/src/jsx/components/form/Button.ts @@ -1,14 +1,8 @@ -import type { EnumToUnion } from 'src/internals'; - import type { SnapsChildren, StringElement } from '../../component'; import { createSnapComponent } from '../../component'; import type { IconElement } from '../Icon'; import type { ImageElement } from '../Image'; -export enum SnapRoutes { - Home = 'home', -} - // TODO: Add the `onClick` prop to the `ButtonProps` type. /** @@ -31,7 +25,6 @@ export type ButtonProps = { variant?: 'primary' | 'destructive' | undefined; disabled?: boolean | undefined; form?: string | undefined; - navigateTo?: EnumToUnion | undefined; }; const TYPE = 'Button'; diff --git a/packages/snaps-sdk/src/jsx/validation.test.tsx b/packages/snaps-sdk/src/jsx/validation.test.tsx index 80eab41046..428d2e8764 100644 --- a/packages/snaps-sdk/src/jsx/validation.test.tsx +++ b/packages/snaps-sdk/src/jsx/validation.test.tsx @@ -170,7 +170,6 @@ describe('ButtonStruct', () => { , , - , ])('validates a button element', (value) => { expect(is(value, ButtonStruct)).toBe(true); }); @@ -193,8 +192,6 @@ describe('ButtonStruct', () => { alt , - // @ts-expect-error - Invalid props. - , ])('does not validate "%p"', (value) => { expect(is(value, ButtonStruct)).toBe(false); }); @@ -994,12 +991,14 @@ describe('ImageStruct', () => { }); describe('LinkStruct', () => { - it.each([foo])( - 'validates a link element', - (value) => { - expect(is(value, LinkStruct)).toBe(true); - }, - ); + it.each([ + foo, + + + , + ])('validates a link element', (value) => { + expect(is(value, LinkStruct)).toBe(true); + }); it.each([ 'foo', diff --git a/packages/snaps-sdk/src/jsx/validation.ts b/packages/snaps-sdk/src/jsx/validation.ts index f52a614a3f..27359689cf 100644 --- a/packages/snaps-sdk/src/jsx/validation.ts +++ b/packages/snaps-sdk/src/jsx/validation.ts @@ -226,7 +226,6 @@ export const ButtonStruct: Describe = element('Button', { variant: optional(nullUnion([literal('primary'), literal('destructive')])), disabled: optional(boolean()), form: optional(string()), - navigateTo: optional(nullUnion([literal('home')])), }); /** @@ -591,7 +590,7 @@ export const HeadingStruct: Describe = element('Heading', { */ export const LinkStruct: Describe = element('Link', { href: string(), - children: children([FormattingStruct, string()]), + children: children([FormattingStruct, string(), IconStruct]), }); /** diff --git a/packages/snaps-utils/src/url.test.ts b/packages/snaps-utils/src/url.test.ts new file mode 100644 index 0000000000..8fceb3dfa8 --- /dev/null +++ b/packages/snaps-utils/src/url.test.ts @@ -0,0 +1,55 @@ +import { parseMetaMaskUrl } from './url'; + +describe('parseMetaMaskUrl', () => { + it('can parse a valid url with the client authority', () => { + expect(parseMetaMaskUrl('metamask://client/')).toStrictEqual({ + authority: 'client', + path: '/', + }); + }); + + it('can parse a valid url with a namespaced snap', () => { + expect( + parseMetaMaskUrl('metamask://snap/npm:@metamask/examplesnap/home'), + ).toStrictEqual({ + authority: 'snap', + path: '/home', + snapId: 'npm:@metamask/examplesnap', + }); + }); + + it('can parse a valid url with a non-namespaced snap', () => { + expect( + parseMetaMaskUrl('metamask://snap/npm:examplesnap/home'), + ).toStrictEqual({ + authority: 'snap', + path: '/home', + snapId: 'npm:examplesnap', + }); + }); + + it('will throw on an invalid scheme', () => { + expect(() => parseMetaMaskUrl('metmask://client/')).toThrow( + 'Invalid MetaMask url.', + ); + }); + + it('will throw on an invalid authority', () => { + expect(() => parseMetaMaskUrl('metamask://bar/')).toThrow( + 'Invalid MetaMask url.', + ); + }); + + it.each([ + 'metamask://snap/npm:examplesnap/foo', + 'metamask://snap/npm:@metamask/examplesnap/foo', + ])('will throw on an invalid snap page', (url) => { + expect(() => parseMetaMaskUrl(url)).toThrow('Invalid snap path.'); + }); + + it('will throw on an invalid client page', () => { + expect(() => parseMetaMaskUrl('metamask://client/foo')).toThrow( + 'Invalid client path.', + ); + }); +}); diff --git a/packages/snaps-utils/src/url.ts b/packages/snaps-utils/src/url.ts new file mode 100644 index 0000000000..e3b9c7d5f5 --- /dev/null +++ b/packages/snaps-utils/src/url.ts @@ -0,0 +1,82 @@ +import { type SnapId } from '@metamask/snaps-sdk'; +import { type Infer, is, pattern, string } from '@metamask/superstruct'; +import { assert } from '@metamask/utils'; + +import { assertIsValidSnapId, stripSnapPrefix } from './snaps'; + +export const METAMASK_URL_REGEX = + /^metamask:\/\/(?(client|snap))(?.+)$/u; + +export const MetaMaskUrlStruct = pattern(string(), METAMASK_URL_REGEX); + +export type MetaMaskUrl = Infer; + +export type Authority = 'client' | 'snap'; + +export const ExtensionPaths = ['/']; + +export const SnapPaths = ['/home']; + +/** + * Parse a url string to an object containing the authority and path. + * This validates the url before parsing it. + * + * @param url - The url to validate and parse. + * @returns The parsed url. + * @throws If the authority or path is invalid. + */ +export function parseMetaMaskUrl(url: MetaMaskUrl): { + authority: Authority; + snapId?: SnapId; + path: string; +} { + const match = METAMASK_URL_REGEX.exec(url); + if (!match?.groups) { + throw new Error('Invalid MetaMask url.'); + } + + const { authority, path } = match.groups; + + if (authority === 'client') { + assert(ExtensionPaths.includes(path), 'Invalid client path.'); + return { + authority, + path, + }; + } else if (authority === 'snap') { + const strippedPath = stripSnapPrefix(path.slice(1)); + const isNameSpaced = strippedPath.startsWith('@'); + const pathTokens = strippedPath.split('/'); + const lastPathToken = `/${pathTokens[pathTokens.length - 1]}`; + const partialSnapId = isNameSpaced + ? `${pathTokens[0]}/${pathTokens[1]}` + : pathTokens[0]; + const location = path.slice(1).startsWith('npm:') ? 'npm:' : 'local:'; + const snapId = `${location}${partialSnapId}`; + assertIsValidSnapId(snapId); + assert( + isNameSpaced + ? pathTokens.length === 3 && SnapPaths.includes(lastPathToken) + : pathTokens.length === 2 && SnapPaths.includes(lastPathToken), + 'Invalid snap path.', + ); + + return { + authority, + snapId, + path: lastPathToken, + }; + } + + throw new Error('Invalid authority'); +} + +/** + * Check if the given value is a MetaMask url. + * + * @param value - The value to check. + * @returns Whether the value is a MetaMask url. + */ +export function isMetaMaskUrl(value: unknown): value is MetaMaskUrl { + return is(value, MetaMaskUrlStruct); +} From 432a509e8a7e08e97c8415326e0a59d9db6df78e Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 13 Sep 2024 15:44:45 -0400 Subject: [PATCH 05/22] add getSnap function for updated validation function signature --- .../src/interface/SnapInterfaceController.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts index fb323471f5..af17d9ccb9 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts @@ -24,6 +24,7 @@ import type { Json } from '@metamask/utils'; import { assert } from '@metamask/utils'; import { castDraft } from 'immer'; import { nanoid } from 'nanoid'; +import type { GetSnap } from 'src/snaps'; import { constructState, @@ -74,7 +75,8 @@ export type SnapInterfaceControllerAllowedActions = | TestOrigin | MaybeUpdateState | HasApprovalRequest - | AcceptRequest; + | AcceptRequest + | GetSnap; export type SnapInterfaceControllerActions = | CreateInterface @@ -379,6 +381,10 @@ export class SnapInterfaceController extends BaseController< ); await this.#triggerPhishingListUpdate(); - validateJsxLinks(element, this.#checkPhishingList.bind(this)); + validateJsxLinks( + element, + this.#checkPhishingList.bind(this), + (id: string) => this.messagingSystem.call('SnapController:get', id), + ); } } From 3b3ebddbdbbfbedbc9971731b96484fedb977548 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 13 Sep 2024 15:45:33 -0400 Subject: [PATCH 06/22] update link validation functions to check for metamask scheme --- packages/snaps-utils/src/ui.tsx | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/packages/snaps-utils/src/ui.tsx b/packages/snaps-utils/src/ui.tsx index b53c8b8ef4..a19b992e91 100644 --- a/packages/snaps-utils/src/ui.tsx +++ b/packages/snaps-utils/src/ui.tsx @@ -39,8 +39,11 @@ import { import { lexer, walkTokens } from 'marked'; import type { Token, Tokens } from 'marked'; +import type { Snap } from './snaps'; +import { parseMetaMaskUrl } from './url'; + const MAX_TEXT_LENGTH = 50_000; // 50 kb -const ALLOWED_PROTOCOLS = ['https:', 'mailto:']; +const ALLOWED_PROTOCOLS = ['https:', 'mailto:', 'metamask:']; /** * Get the button variant from a legacy button component variant. @@ -330,10 +333,13 @@ function getMarkdownLinks(text: string) { * @param link - The link to validate. * @param isOnPhishingList - The function that checks the link against the * phishing list. + * @param getSnap - The function that returns a snap if installed, undefined otherwise. + * @throws If the link is invalid. */ export function validateLink( link: string, isOnPhishingList: (url: string) => boolean, + getSnap: (id: string) => Snap | undefined, ) { try { const url = new URL(link); @@ -342,10 +348,20 @@ export function validateLink( `Protocol must be one of: ${ALLOWED_PROTOCOLS.join(', ')}.`, ); - const hostname = - url.protocol === 'mailto:' ? url.pathname.split('@')[1] : url.hostname; + if (url.protocol === 'metamask:') { + const linkData = parseMetaMaskUrl(link); + if (linkData.snapId) { + assert( + getSnap(linkData.snapId), + 'The snap being navigated to is not installed.', + ); + } + } else { + const hostname = + url.protocol === 'mailto:' ? url.pathname.split('@')[1] : url.hostname; - assert(!isOnPhishingList(hostname), 'The specified URL is not allowed.'); + assert(!isOnPhishingList(hostname), 'The specified URL is not allowed.'); + } } catch (error) { throw new Error( `Invalid URL: ${ @@ -362,16 +378,18 @@ export function validateLink( * @param text - The text to verify. * @param isOnPhishingList - The function that checks the link against the * phishing list. + * @param getSnap - The function that returns a snap if installed, undefined otherwise. * @throws If the text contains a link that is not allowed. */ export function validateTextLinks( text: string, isOnPhishingList: (url: string) => boolean, + getSnap: (id: string) => Snap | undefined, ) { const links = getMarkdownLinks(text); for (const link of links) { - validateLink(link.href, isOnPhishingList); + validateLink(link.href, isOnPhishingList, getSnap); } } @@ -382,17 +400,19 @@ export function validateTextLinks( * @param node - The JSX node to walk. * @param isOnPhishingList - The function that checks the link against the * phishing list. + * @param getSnap - The function that returns a snap if installed, undefined otherwise. */ export function validateJsxLinks( node: JSXElement, isOnPhishingList: (url: string) => boolean, + getSnap: (id: string) => Snap | undefined, ) { walkJsx(node, (childNode) => { if (childNode.type !== 'Link') { return; } - validateLink(childNode.props.href, isOnPhishingList); + validateLink(childNode.props.href, isOnPhishingList, getSnap); }); } From dabf3451080084b63c12251fa5b7af066575a3cf Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 13 Sep 2024 15:46:10 -0400 Subject: [PATCH 07/22] update validate link call --- packages/snaps-rpc-methods/src/restricted/notify.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/snaps-rpc-methods/src/restricted/notify.ts b/packages/snaps-rpc-methods/src/restricted/notify.ts index 8c4c11f882..86d551951a 100644 --- a/packages/snaps-rpc-methods/src/restricted/notify.ts +++ b/packages/snaps-rpc-methods/src/restricted/notify.ts @@ -11,7 +11,7 @@ import type { NotifyResult, EnumToUnion, } from '@metamask/snaps-sdk'; -import { validateTextLinks } from '@metamask/snaps-utils'; +import { type Snap, validateTextLinks } from '@metamask/snaps-utils'; import type { NonEmptyArray } from '@metamask/utils'; import { isObject } from '@metamask/utils'; @@ -53,6 +53,8 @@ export type NotifyMethodHooks = { isOnPhishingList: (url: string) => boolean; maybeUpdatePhishingList: () => Promise; + + getSnap: (snapId: string) => Snap | undefined; }; type SpecificationBuilderOptions = { @@ -95,6 +97,7 @@ const methodHooks: MethodHooksObject = { showInAppNotification: true, isOnPhishingList: true, maybeUpdatePhishingList: true, + getSnap: true, }; export const notifyBuilder = Object.freeze({ @@ -111,6 +114,7 @@ export const notifyBuilder = Object.freeze({ * @param hooks.showInAppNotification - A function that shows a notification in the MetaMask UI. * @param hooks.isOnPhishingList - A function that checks for links against the phishing list. * @param hooks.maybeUpdatePhishingList - A function that updates the phishing list if needed. + * @param hooks.getSnap - A function that checks if a snap is installed. * @returns The method implementation which returns `null` on success. * @throws If the params are invalid. */ @@ -119,6 +123,7 @@ export function getImplementation({ showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + getSnap, }: NotifyMethodHooks) { return async function implementation( args: RestrictedMethodOptions, @@ -132,7 +137,7 @@ export function getImplementation({ await maybeUpdatePhishingList(); - validateTextLinks(validatedParams.message, isOnPhishingList); + validateTextLinks(validatedParams.message, isOnPhishingList, getSnap); switch (validatedParams.type) { case NotificationType.Native: From b732264e7b584e18cd1d6e637b6a3fd430605ce0 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 13 Sep 2024 15:48:10 -0400 Subject: [PATCH 08/22] recalculate shasums --- packages/examples/packages/browserify-plugin/snap.manifest.json | 2 +- packages/examples/packages/browserify/snap.manifest.json | 2 +- packages/examples/packages/dialogs/snap.manifest.json | 2 +- packages/examples/packages/file-upload/snap.manifest.json | 2 +- packages/examples/packages/home-page/snap.manifest.json | 2 +- packages/examples/packages/interactive-ui/snap.manifest.json | 2 +- packages/examples/packages/jsx/snap.manifest.json | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/examples/packages/browserify-plugin/snap.manifest.json b/packages/examples/packages/browserify-plugin/snap.manifest.json index ed86327c8c..f9a6962bce 100644 --- a/packages/examples/packages/browserify-plugin/snap.manifest.json +++ b/packages/examples/packages/browserify-plugin/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "oqO5wicjNs9i86WgNV2vx79nOYYD/uxnOtAOECtWqzU=", + "shasum": "PHfXj/qdxoIA1JIdIqlLMztPr22RMiGQlKI2RKg2DPE=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/browserify/snap.manifest.json b/packages/examples/packages/browserify/snap.manifest.json index f3bd542095..806579c62e 100644 --- a/packages/examples/packages/browserify/snap.manifest.json +++ b/packages/examples/packages/browserify/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "LQJI6F5ptgr0wY9vQ+58Q8Tv8VyMx/8lvkDSmDgdqVM=", + "shasum": "tYGbR3Ru8MxnikWI/ZwUljGviLhF3P60NpCSII7vnUI=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/dialogs/snap.manifest.json b/packages/examples/packages/dialogs/snap.manifest.json index dbac84f6fa..75edcaa8a6 100644 --- a/packages/examples/packages/dialogs/snap.manifest.json +++ b/packages/examples/packages/dialogs/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "zuBFS3XWpLP7vKsIJqiecvXakBW9rgzuVToBRS0ItSg=", + "shasum": "v2oIs3UY5j/3dXW5eRh3rg7Tv5qcVzHon7qzPQeRsAE=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/file-upload/snap.manifest.json b/packages/examples/packages/file-upload/snap.manifest.json index 8f0774493b..a34c9a115e 100644 --- a/packages/examples/packages/file-upload/snap.manifest.json +++ b/packages/examples/packages/file-upload/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "exoRIe26MvGPe9XZxn8uq/xBn8uYvzvn0y6L5uTPlKM=", + "shasum": "z4vMdrs40TdVE+vk7sPruIvWi0q669V+p3jc6WQIib4=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/home-page/snap.manifest.json b/packages/examples/packages/home-page/snap.manifest.json index 715fa52014..3afc05d1c9 100644 --- a/packages/examples/packages/home-page/snap.manifest.json +++ b/packages/examples/packages/home-page/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "x94obetFeJZPEwAU/DsNvFj5/eotAwlylCGS7IfM+84=", + "shasum": "emnlXHvrhsGBMi/y1IPhiQPX9ogHmjmOU/iujBu5GB8=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/interactive-ui/snap.manifest.json b/packages/examples/packages/interactive-ui/snap.manifest.json index 6146cc4b6d..6581945413 100644 --- a/packages/examples/packages/interactive-ui/snap.manifest.json +++ b/packages/examples/packages/interactive-ui/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "iN1fXS+FpkQR21mGkHO0N+xo4nW3HFwsfXOsGTpiM68=", + "shasum": "uOuMArCEwmOmCl0Bl1WnRRm+DKcq0Y+O+5n8Z1KBMr8=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/jsx/snap.manifest.json b/packages/examples/packages/jsx/snap.manifest.json index 4e6b372e68..f9a9c92dcf 100644 --- a/packages/examples/packages/jsx/snap.manifest.json +++ b/packages/examples/packages/jsx/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "Rv/yARDSnZz9twUQ/mUWpNFSjjRkNy68DUy2gTwq+ZA=", + "shasum": "3U9+Lvmmdc9JRmaHLcwGgi9lpnaj25joBF9+zXY4644=", "location": { "npm": { "filePath": "dist/bundle.js", From 1ef2fc645593aa15a907d782660f68857fd22386 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 13 Sep 2024 16:13:22 -0400 Subject: [PATCH 09/22] update tests --- .../src/restricted/notify.test.ts | 15 ++- packages/snaps-utils/src/ui.test.tsx | 103 ++++++++++++------ 2 files changed, 82 insertions(+), 36 deletions(-) diff --git a/packages/snaps-rpc-methods/src/restricted/notify.test.ts b/packages/snaps-rpc-methods/src/restricted/notify.test.ts index aeecba1a38..fc62b2854a 100644 --- a/packages/snaps-rpc-methods/src/restricted/notify.test.ts +++ b/packages/snaps-rpc-methods/src/restricted/notify.test.ts @@ -20,6 +20,7 @@ describe('snap_notify', () => { showInAppNotification: jest.fn(), isOnPhishingList: jest.fn(), maybeUpdatePhishingList: jest.fn(), + getSnap: jest.fn(), }; expect( @@ -41,6 +42,7 @@ describe('snap_notify', () => { const showNativeNotification = jest.fn().mockResolvedValueOnce(true); const showInAppNotification = jest.fn().mockResolvedValueOnce(true); const isOnPhishingList = jest.fn().mockResolvedValueOnce(false); + const getSnap = jest.fn(); const maybeUpdatePhishingList = jest.fn(); const notificationImplementation = getImplementation({ @@ -48,6 +50,7 @@ describe('snap_notify', () => { showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + getSnap, }); await notificationImplementation({ @@ -72,12 +75,14 @@ describe('snap_notify', () => { const showInAppNotification = jest.fn().mockResolvedValueOnce(true); const isOnPhishingList = jest.fn().mockResolvedValueOnce(false); const maybeUpdatePhishingList = jest.fn(); + const getSnap = jest.fn(); const notificationImplementation = getImplementation({ showNativeNotification, showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + getSnap, }); await notificationImplementation({ @@ -102,12 +107,14 @@ describe('snap_notify', () => { const showInAppNotification = jest.fn().mockResolvedValueOnce(true); const isOnPhishingList = jest.fn().mockResolvedValueOnce(false); const maybeUpdatePhishingList = jest.fn(); + const getSnap = jest.fn(); const notificationImplementation = getImplementation({ showNativeNotification, showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + getSnap, }); await notificationImplementation({ @@ -132,12 +139,14 @@ describe('snap_notify', () => { const showInAppNotification = jest.fn().mockResolvedValueOnce(true); const isOnPhishingList = jest.fn().mockResolvedValueOnce(false); const maybeUpdatePhishingList = jest.fn(); + const getSnap = jest.fn(); const notificationImplementation = getImplementation({ showNativeNotification, showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + getSnap, }); await expect( @@ -160,12 +169,14 @@ describe('snap_notify', () => { const showInAppNotification = jest.fn().mockResolvedValueOnce(true); const isOnPhishingList = jest.fn().mockResolvedValueOnce(true); const maybeUpdatePhishingList = jest.fn(); + const getSnap = jest.fn(); const notificationImplementation = getImplementation({ showNativeNotification, showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + getSnap, }); await expect( @@ -187,12 +198,14 @@ describe('snap_notify', () => { const showInAppNotification = jest.fn().mockResolvedValueOnce(true); const isOnPhishingList = jest.fn().mockResolvedValueOnce(true); const maybeUpdatePhishingList = jest.fn(); + const getSnap = jest.fn(); const notificationImplementation = getImplementation({ showNativeNotification, showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + getSnap, }); await expect( @@ -207,7 +220,7 @@ describe('snap_notify', () => { }, }), ).rejects.toThrow( - 'Invalid URL: Protocol must be one of: https:, mailto:.', + 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); }); }); diff --git a/packages/snaps-utils/src/ui.test.tsx b/packages/snaps-utils/src/ui.test.tsx index 569ace6a65..29203ec14c 100644 --- a/packages/snaps-utils/src/ui.test.tsx +++ b/packages/snaps-utils/src/ui.test.tsx @@ -546,8 +546,8 @@ describe('validateLink', () => { it('passes for a valid link', () => { const fn = jest.fn().mockReturnValue(false); - expect(() => validateLink('https://foo.bar', fn)).not.toThrow(); - expect(() => validateLink('mailto:foo@bar.com', fn)).not.toThrow(); + expect(() => validateLink('https://foo.bar', fn, fn)).not.toThrow(); + expect(() => validateLink('mailto:foo@bar.com', fn, fn)).not.toThrow(); expect(fn).toHaveBeenCalledTimes(2); expect(fn).toHaveBeenCalledWith('foo.bar'); @@ -557,8 +557,8 @@ describe('validateLink', () => { it('throws an error for an invalid protocol', () => { const fn = jest.fn().mockReturnValue(false); - expect(() => validateLink('http://foo.bar', fn)).toThrow( - 'Invalid URL: Protocol must be one of: https:, mailto:.', + expect(() => validateLink('http://foo.bar', fn, fn)).toThrow( + 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); expect(fn).not.toHaveBeenCalled(); @@ -567,7 +567,7 @@ describe('validateLink', () => { it('throws an error for an invalid URL', () => { const fn = jest.fn().mockReturnValue(false); - expect(() => validateLink('foo.bar', fn)).toThrow( + expect(() => validateLink('foo.bar', fn, fn)).toThrow( 'Invalid URL: Unable to parse URL.', ); @@ -577,9 +577,9 @@ describe('validateLink', () => { it('throws an error for a phishing link', () => { const fn = jest.fn().mockReturnValue(true); - expect(() => validateLink('https://test.metamask-phishing.io', fn)).toThrow( - 'Invalid URL: The specified URL is not allowed.', - ); + expect(() => + validateLink('https://test.metamask-phishing.io', fn, fn), + ).toThrow('Invalid URL: The specified URL is not allowed.'); expect(fn).toHaveBeenCalledTimes(1); expect(fn).toHaveBeenCalledWith('test.metamask-phishing.io'); @@ -589,7 +589,7 @@ describe('validateLink', () => { const fn = jest.fn().mockReturnValue(true); expect(() => - validateLink('mailto:foo@test.metamask-phishing.io', fn), + validateLink('mailto:foo@test.metamask-phishing.io', fn, fn), ).toThrow('Invalid URL: The specified URL is not allowed.'); expect(fn).toHaveBeenCalledTimes(1); @@ -600,27 +600,31 @@ describe('validateLink', () => { describe('validateTextLinks', () => { it('passes for valid links', () => { expect(() => - validateTextLinks('[test](https://foo.bar)', () => false), + validateTextLinks('[test](https://foo.bar)', () => false, jest.fn()), ).not.toThrow(); expect(() => - validateTextLinks('[test](mailto:foo@bar.baz)', () => false), + validateTextLinks('[test](mailto:foo@bar.baz)', () => false, jest.fn()), ).not.toThrow(); expect(() => - validateTextLinks('[](https://foo.bar)', () => false), + validateTextLinks('[](https://foo.bar)', () => false, jest.fn()), ).not.toThrow(); expect(() => - validateTextLinks('[[test]](https://foo.bar)', () => false), + validateTextLinks('[[test]](https://foo.bar)', () => false, jest.fn()), ).not.toThrow(); expect(() => - validateTextLinks('[test](https://foo.bar "foo bar baz")', () => false), + validateTextLinks( + '[test](https://foo.bar "foo bar baz")', + () => false, + jest.fn(), + ), ).not.toThrow(); expect(() => - validateTextLinks('', () => false), + validateTextLinks('', () => false, jest.fn()), ).not.toThrow(); expect(() => @@ -628,6 +632,7 @@ describe('validateTextLinks', () => { `[foo][1] [1]: https://foo.bar`, () => false, + jest.fn(), ), ).not.toThrow(); @@ -636,51 +641,66 @@ describe('validateTextLinks', () => { `[foo][1] [1]: https://foo.bar "foo bar baz"`, () => false, + jest.fn(), ), ).not.toThrow(); }); it('passes for non-links', () => { expect(() => - validateTextLinks('Hello **http://localhost:3000**', () => false), + validateTextLinks( + 'Hello **http://localhost:3000**', + () => false, + jest.fn(), + ), ).not.toThrow(); }); it('throws an error if an invalid link is found in text', () => { expect(() => validateTextLinks('[test](http://foo.bar)', () => false), - ).toThrow('Invalid URL: Protocol must be one of: https:, mailto:.'); + ).toThrow( + 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', + ); expect(() => validateTextLinks('[[test]](http://foo.bar)', () => false), - ).toThrow('Invalid URL: Protocol must be one of: https:, mailto:.'); + ).toThrow( + 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', + ); expect(() => validateTextLinks('', () => false)).toThrow( - 'Invalid URL: Protocol must be one of: https:, mailto:.', + 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); expect(() => validateTextLinks('[test](http://foo.bar "foo bar baz")', () => false), - ).toThrow('Invalid URL: Protocol must be one of: https:, mailto:.'); + ).toThrow( + 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', + ); expect(() => validateTextLinks('[foo][1]\n\n[1]: http://foo.bar', () => false), - ).toThrow('Invalid URL: Protocol must be one of: https:, mailto:.'); + ).toThrow( + 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', + ); expect(() => validateTextLinks( `[foo][1]\n\n[1]: http://foo.bar "foo bar baz"`, () => false, ), - ).toThrow('Invalid URL: Protocol must be one of: https:, mailto:.'); - - expect(() => validateTextLinks('[test](#code)', () => false)).toThrow( - 'Invalid URL: Unable to parse URL.', + ).toThrow( + 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); - expect(() => validateTextLinks('[test](foo.bar)', () => false)).toThrow( - 'Invalid URL: Unable to parse URL.', - ); + expect(() => + validateTextLinks('[test](#code)', () => false, jest.fn()), + ).toThrow('Invalid URL: Unable to parse URL.'); + + expect(() => + validateTextLinks('[test](foo.bar)', () => false, jest.fn()), + ).toThrow('Invalid URL: Unable to parse URL.'); }); }); @@ -699,7 +719,9 @@ describe('validateJsxLinks', () => { ])('does not throw for a safe JSX text component', async (element) => { const isOnPhishingList = () => false; - expect(() => validateJsxLinks(element, isOnPhishingList)).not.toThrow(); + expect(() => + validateJsxLinks(element, isOnPhishingList, jest.fn()), + ).not.toThrow(); }); it('does not throw for a JSX component with a link outside of a Link component', async () => { @@ -712,6 +734,7 @@ describe('validateJsxLinks', () => { https://foo.bar , isOnPhishingList, + jest.fn(), ), ).not.toThrow(); }); @@ -730,24 +753,34 @@ describe('validateJsxLinks', () => { ])('throws for an unsafe JSX text component', async (element) => { const isOnPhishingList = () => true; - expect(() => validateJsxLinks(element, isOnPhishingList)).toThrow( - 'Invalid URL: The specified URL is not allowed.', - ); + expect(() => + validateJsxLinks(element, isOnPhishingList, jest.fn()), + ).toThrow('Invalid URL: The specified URL is not allowed.'); }); it('throws if the protocol is not allowed', () => { const isOnPhishingList = () => false; expect(() => - validateJsxLinks(Foo, isOnPhishingList), - ).toThrow('Invalid URL: Protocol must be one of: https:, mailto:.'); + validateJsxLinks( + Foo, + isOnPhishingList, + jest.fn(), + ), + ).toThrow( + 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', + ); }); it('throws if the URL cannot be parsed', () => { const isOnPhishingList = () => false; expect(() => - validateJsxLinks(Foo, isOnPhishingList), + validateJsxLinks( + Foo, + isOnPhishingList, + jest.fn(), + ), ).toThrow('Invalid URL: Unable to parse URL.'); }); }); From 082292e6e658d10f5d9cbacc0cc78d873b5ebc98 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 13 Sep 2024 16:20:55 -0400 Subject: [PATCH 10/22] fix lint issue --- .../snaps-controllers/src/interface/SnapInterfaceController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts index af17d9ccb9..70cb81abdd 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts @@ -24,8 +24,8 @@ import type { Json } from '@metamask/utils'; import { assert } from '@metamask/utils'; import { castDraft } from 'immer'; import { nanoid } from 'nanoid'; -import type { GetSnap } from 'src/snaps'; +import type { GetSnap } from '../snaps'; import { constructState, getJsxInterface, From 870f5de31f259d58523ac1bb027b427c4863b6ea Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 13 Sep 2024 17:39:08 -0400 Subject: [PATCH 11/22] add more tests, fix logic --- packages/snaps-utils/src/ui.test.tsx | 35 ++++++++++++++++++++++++---- packages/snaps-utils/src/url.test.ts | 34 ++++++++++++++++++++++++++- packages/snaps-utils/src/url.ts | 33 +++++++++++++++++--------- 3 files changed, 85 insertions(+), 17 deletions(-) diff --git a/packages/snaps-utils/src/ui.test.tsx b/packages/snaps-utils/src/ui.test.tsx index 29203ec14c..92da67a5d4 100644 --- a/packages/snaps-utils/src/ui.test.tsx +++ b/packages/snaps-utils/src/ui.test.tsx @@ -564,6 +564,20 @@ describe('validateLink', () => { expect(fn).not.toHaveBeenCalled(); }); + it('throws an error if the snap being navigated to is not installed', () => { + const fn = jest.fn().mockReturnValue(false); + + expect(() => + validateLink( + 'metamask://snap/npm:@metamask/examplesnap/home', + fn, + jest.fn().mockReturnValue(false), + ), + ).toThrow('The snap being navigated to is not installed.'); + + expect(fn).not.toHaveBeenCalled(); + }); + it('throws an error for an invalid URL', () => { const fn = jest.fn().mockReturnValue(false); @@ -658,29 +672,39 @@ describe('validateTextLinks', () => { it('throws an error if an invalid link is found in text', () => { expect(() => - validateTextLinks('[test](http://foo.bar)', () => false), + validateTextLinks('[test](http://foo.bar)', () => false, jest.fn()), ).toThrow( 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); expect(() => - validateTextLinks('[[test]](http://foo.bar)', () => false), + validateTextLinks('[[test]](http://foo.bar)', () => false, jest.fn()), ).toThrow( 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); - expect(() => validateTextLinks('', () => false)).toThrow( + expect(() => + validateTextLinks('', () => false, jest.fn()), + ).toThrow( 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); expect(() => - validateTextLinks('[test](http://foo.bar "foo bar baz")', () => false), + validateTextLinks( + '[test](http://foo.bar "foo bar baz")', + () => false, + jest.fn(), + ), ).toThrow( 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); expect(() => - validateTextLinks('[foo][1]\n\n[1]: http://foo.bar', () => false), + validateTextLinks( + '[foo][1]\n\n[1]: http://foo.bar', + () => false, + jest.fn(), + ), ).toThrow( 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); @@ -689,6 +713,7 @@ describe('validateTextLinks', () => { validateTextLinks( `[foo][1]\n\n[1]: http://foo.bar "foo bar baz"`, () => false, + jest.fn(), ), ).toThrow( 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', diff --git a/packages/snaps-utils/src/url.test.ts b/packages/snaps-utils/src/url.test.ts index 8fceb3dfa8..956d481cfa 100644 --- a/packages/snaps-utils/src/url.test.ts +++ b/packages/snaps-utils/src/url.test.ts @@ -1,4 +1,10 @@ -import { parseMetaMaskUrl } from './url'; +import { isMetaMaskUrl, METAMASK_URL_REGEX, parseMetaMaskUrl } from './url'; + +describe('METAMASK_URL_REGEX', () => { + it('will throw for non-metamask urls', () => { + expect('random:foobar'.match(METAMASK_URL_REGEX)).toBeNull(); + }); +}); describe('parseMetaMaskUrl', () => { it('can parse a valid url with the client authority', () => { @@ -28,6 +34,16 @@ describe('parseMetaMaskUrl', () => { }); }); + it('can parse a valid url with a local snap', () => { + expect( + parseMetaMaskUrl('metamask://snap/local:http://localhost:8080/home'), + ).toStrictEqual({ + authority: 'snap', + path: '/home', + snapId: 'local:http://localhost:8080', + }); + }); + it('will throw on an invalid scheme', () => { expect(() => parseMetaMaskUrl('metmask://client/')).toThrow( 'Invalid MetaMask url.', @@ -53,3 +69,19 @@ describe('parseMetaMaskUrl', () => { ); }); }); + +describe('isMetaMaskUrl', () => { + it.each(['https://www.google.com', 'metamask://foo/'])( + 'will return false for non-metamask urls', + (url) => { + expect(isMetaMaskUrl(url)).toBe(false); + }, + ); + + it.each(['metamask://snap/home', 'metamask://client/'])( + 'will not throw for valid metamask urls', + (url) => { + expect(isMetaMaskUrl(url)).toBe(true); + }, + ); +}); diff --git a/packages/snaps-utils/src/url.ts b/packages/snaps-utils/src/url.ts index e3b9c7d5f5..5a3d36a548 100644 --- a/packages/snaps-utils/src/url.ts +++ b/packages/snaps-utils/src/url.ts @@ -45,21 +45,30 @@ export function parseMetaMaskUrl(url: MetaMaskUrl): { }; } else if (authority === 'snap') { const strippedPath = stripSnapPrefix(path.slice(1)); + const location = path.slice(1).startsWith('npm:') ? 'npm:' : 'local:'; const isNameSpaced = strippedPath.startsWith('@'); const pathTokens = strippedPath.split('/'); const lastPathToken = `/${pathTokens[pathTokens.length - 1]}`; - const partialSnapId = isNameSpaced - ? `${pathTokens[0]}/${pathTokens[1]}` - : pathTokens[0]; - const location = path.slice(1).startsWith('npm:') ? 'npm:' : 'local:'; + let partialSnapId; + if (location === 'local:') { + partialSnapId = `http://${pathTokens[2]}`; + assert( + pathTokens.length === 4 && SnapPaths.includes(lastPathToken), + 'Invalid snap path.', + ); + } else { + partialSnapId = isNameSpaced + ? `${pathTokens[0]}/${pathTokens[1]}` + : pathTokens[0]; + assert( + isNameSpaced + ? pathTokens.length === 3 && SnapPaths.includes(lastPathToken) + : pathTokens.length === 2 && SnapPaths.includes(lastPathToken), + 'Invalid snap path.', + ); + } const snapId = `${location}${partialSnapId}`; assertIsValidSnapId(snapId); - assert( - isNameSpaced - ? pathTokens.length === 3 && SnapPaths.includes(lastPathToken) - : pathTokens.length === 2 && SnapPaths.includes(lastPathToken), - 'Invalid snap path.', - ); return { authority, @@ -67,7 +76,9 @@ export function parseMetaMaskUrl(url: MetaMaskUrl): { path: lastPathToken, }; } - + // You don't ever actually reach this line, TS doesn't know that no other authority will make it past + // the regex statement + /* istanbul ignore next */ throw new Error('Invalid authority'); } From c672e2b3421b753165ac297f5719b1b1f38caa09 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Mon, 16 Sep 2024 19:20:03 -0400 Subject: [PATCH 12/22] made some changes per review --- packages/snaps-sdk/src/jsx/components/Link.ts | 3 +- packages/snaps-sdk/src/jsx/validation.ts | 2 +- packages/snaps-utils/src/url.test.ts | 10 +- packages/snaps-utils/src/url.ts | 98 ++++++++++--------- 4 files changed, 60 insertions(+), 53 deletions(-) diff --git a/packages/snaps-sdk/src/jsx/components/Link.ts b/packages/snaps-sdk/src/jsx/components/Link.ts index 032238cdaa..b9698536f6 100644 --- a/packages/snaps-sdk/src/jsx/components/Link.ts +++ b/packages/snaps-sdk/src/jsx/components/Link.ts @@ -2,12 +2,13 @@ import type { SnapsChildren } from '../component'; import { createSnapComponent } from '../component'; import type { StandardFormattingElement } from './formatting'; import { type IconElement } from './Icon'; +import { type ImageElement } from './Image'; /** * The children of the {@link Link} component. */ export type LinkChildren = SnapsChildren< - string | StandardFormattingElement | IconElement + string | StandardFormattingElement | IconElement | ImageElement >; /** diff --git a/packages/snaps-sdk/src/jsx/validation.ts b/packages/snaps-sdk/src/jsx/validation.ts index 27359689cf..a2832e6116 100644 --- a/packages/snaps-sdk/src/jsx/validation.ts +++ b/packages/snaps-sdk/src/jsx/validation.ts @@ -590,7 +590,7 @@ export const HeadingStruct: Describe = element('Heading', { */ export const LinkStruct: Describe = element('Link', { href: string(), - children: children([FormattingStruct, string(), IconStruct]), + children: children([FormattingStruct, string(), IconStruct, ImageStruct]), }); /** diff --git a/packages/snaps-utils/src/url.test.ts b/packages/snaps-utils/src/url.test.ts index 956d481cfa..4e155b9ace 100644 --- a/packages/snaps-utils/src/url.test.ts +++ b/packages/snaps-utils/src/url.test.ts @@ -46,13 +46,13 @@ describe('parseMetaMaskUrl', () => { it('will throw on an invalid scheme', () => { expect(() => parseMetaMaskUrl('metmask://client/')).toThrow( - 'Invalid MetaMask url.', + 'Invalid MetaMask url: invalid scheme.', ); }); it('will throw on an invalid authority', () => { expect(() => parseMetaMaskUrl('metamask://bar/')).toThrow( - 'Invalid MetaMask url.', + 'Invalid MetaMask url: invalid authority.', ); }); @@ -60,12 +60,14 @@ describe('parseMetaMaskUrl', () => { 'metamask://snap/npm:examplesnap/foo', 'metamask://snap/npm:@metamask/examplesnap/foo', ])('will throw on an invalid snap page', (url) => { - expect(() => parseMetaMaskUrl(url)).toThrow('Invalid snap path.'); + expect(() => parseMetaMaskUrl(url)).toThrow( + 'Invalid MetaMask url: invalid snap path.', + ); }); it('will throw on an invalid client page', () => { expect(() => parseMetaMaskUrl('metamask://client/foo')).toThrow( - 'Invalid client path.', + 'Invalid MetaMask url: invalid client path.', ); }); }); diff --git a/packages/snaps-utils/src/url.ts b/packages/snaps-utils/src/url.ts index 5a3d36a548..c951e59684 100644 --- a/packages/snaps-utils/src/url.ts +++ b/packages/snaps-utils/src/url.ts @@ -21,65 +21,69 @@ export const SnapPaths = ['/home']; * Parse a url string to an object containing the authority and path. * This validates the url before parsing it. * - * @param url - The url to validate and parse. + * @param str - The url string to validate and parse. * @returns The parsed url. * @throws If the authority or path is invalid. */ -export function parseMetaMaskUrl(url: MetaMaskUrl): { +export function parseMetaMaskUrl(str: MetaMaskUrl): { authority: Authority; snapId?: SnapId; path: string; } { - const match = METAMASK_URL_REGEX.exec(url); - if (!match?.groups) { - throw new Error('Invalid MetaMask url.'); - } - - const { authority, path } = match.groups; - - if (authority === 'client') { - assert(ExtensionPaths.includes(path), 'Invalid client path.'); - return { - authority, - path, - }; - } else if (authority === 'snap') { - const strippedPath = stripSnapPrefix(path.slice(1)); - const location = path.slice(1).startsWith('npm:') ? 'npm:' : 'local:'; - const isNameSpaced = strippedPath.startsWith('@'); - const pathTokens = strippedPath.split('/'); - const lastPathToken = `/${pathTokens[pathTokens.length - 1]}`; - let partialSnapId; - if (location === 'local:') { - partialSnapId = `http://${pathTokens[2]}`; - assert( - pathTokens.length === 4 && SnapPaths.includes(lastPathToken), - 'Invalid snap path.', - ); - } else { - partialSnapId = isNameSpaced - ? `${pathTokens[0]}/${pathTokens[1]}` - : pathTokens[0]; + const baseErrorMessage = 'Invalid MetaMask url:'; + try { + const url = new URL(str); + const { hostname: authority, pathname: path, protocol } = url; + if (protocol !== 'metamask:') { + throw new Error('invalid scheme.'); + } + if (authority === 'client') { assert( - isNameSpaced - ? pathTokens.length === 3 && SnapPaths.includes(lastPathToken) - : pathTokens.length === 2 && SnapPaths.includes(lastPathToken), - 'Invalid snap path.', + ExtensionPaths.includes(path), + `${baseErrorMessage} invalid client path.`, ); + return { + authority, + path, + }; + } else if (authority === 'snap') { + const strippedPath = stripSnapPrefix(path.slice(1)); + const location = path.slice(1).startsWith('npm:') ? 'npm:' : 'local:'; + const isNameSpaced = strippedPath.startsWith('@'); + const pathTokens = strippedPath.split('/'); + const lastPathToken = `/${pathTokens[pathTokens.length - 1]}`; + let partialSnapId; + if (location === 'local:') { + partialSnapId = `http://${pathTokens[2]}`; + assert( + pathTokens.length === 4 && SnapPaths.includes(lastPathToken), + `${baseErrorMessage} invalid snap path.`, + ); + } else { + partialSnapId = isNameSpaced + ? `${pathTokens[0]}/${pathTokens[1]}` + : pathTokens[0]; + assert( + isNameSpaced + ? pathTokens.length === 3 && SnapPaths.includes(lastPathToken) + : pathTokens.length === 2 && SnapPaths.includes(lastPathToken), + `${baseErrorMessage} invalid snap path.`, + ); + } + const snapId = `${location}${partialSnapId}`; + assertIsValidSnapId(snapId); + + return { + authority, + snapId, + path: lastPathToken, + }; } - const snapId = `${location}${partialSnapId}`; - assertIsValidSnapId(snapId); - return { - authority, - snapId, - path: lastPathToken, - }; + throw new Error('invalid authority.'); + } catch (error) { + throw new Error(`${baseErrorMessage} ${error.message}`); } - // You don't ever actually reach this line, TS doesn't know that no other authority will make it past - // the regex statement - /* istanbul ignore next */ - throw new Error('Invalid authority'); } /** From eaa563b0129c8c7a5f2b946ca5cd849b663a3dbf Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 17 Sep 2024 13:07:08 -0400 Subject: [PATCH 13/22] make changes per comments --- .../browserify-plugin/snap.manifest.json | 2 +- .../packages/browserify/snap.manifest.json | 2 +- packages/snaps-utils/src/ui.test.tsx | 2 +- packages/snaps-utils/src/ui.tsx | 2 +- packages/snaps-utils/src/url.test.ts | 28 +--- packages/snaps-utils/src/url.ts | 125 ++++++++---------- 6 files changed, 61 insertions(+), 100 deletions(-) diff --git a/packages/examples/packages/browserify-plugin/snap.manifest.json b/packages/examples/packages/browserify-plugin/snap.manifest.json index f9a6962bce..922777cfe7 100644 --- a/packages/examples/packages/browserify-plugin/snap.manifest.json +++ b/packages/examples/packages/browserify-plugin/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "PHfXj/qdxoIA1JIdIqlLMztPr22RMiGQlKI2RKg2DPE=", + "shasum": "9BDkjVQUyUjKu84aHZr13nFLxHS+hmksc2Frq5OzBus=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/browserify/snap.manifest.json b/packages/examples/packages/browserify/snap.manifest.json index 806579c62e..e22df59dcd 100644 --- a/packages/examples/packages/browserify/snap.manifest.json +++ b/packages/examples/packages/browserify/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "tYGbR3Ru8MxnikWI/ZwUljGviLhF3P60NpCSII7vnUI=", + "shasum": "mJ5Nz0XN1HKyNSTaOybkp0iKDKm56dbj4dwoCbNt8k0=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/snaps-utils/src/ui.test.tsx b/packages/snaps-utils/src/ui.test.tsx index 92da67a5d4..bb4fb2b583 100644 --- a/packages/snaps-utils/src/ui.test.tsx +++ b/packages/snaps-utils/src/ui.test.tsx @@ -573,7 +573,7 @@ describe('validateLink', () => { fn, jest.fn().mockReturnValue(false), ), - ).toThrow('The snap being navigated to is not installed.'); + ).toThrow('Invalid URL: The Snap being navigated to is not installed.'); expect(fn).not.toHaveBeenCalled(); }); diff --git a/packages/snaps-utils/src/ui.tsx b/packages/snaps-utils/src/ui.tsx index a19b992e91..74dc29eb5d 100644 --- a/packages/snaps-utils/src/ui.tsx +++ b/packages/snaps-utils/src/ui.tsx @@ -353,7 +353,7 @@ export function validateLink( if (linkData.snapId) { assert( getSnap(linkData.snapId), - 'The snap being navigated to is not installed.', + 'The Snap being navigated to is not installed.', ); } } else { diff --git a/packages/snaps-utils/src/url.test.ts b/packages/snaps-utils/src/url.test.ts index 4e155b9ace..b01708ffa8 100644 --- a/packages/snaps-utils/src/url.test.ts +++ b/packages/snaps-utils/src/url.test.ts @@ -1,10 +1,4 @@ -import { isMetaMaskUrl, METAMASK_URL_REGEX, parseMetaMaskUrl } from './url'; - -describe('METAMASK_URL_REGEX', () => { - it('will throw for non-metamask urls', () => { - expect('random:foobar'.match(METAMASK_URL_REGEX)).toBeNull(); - }); -}); +import { parseMetaMaskUrl } from './url'; describe('parseMetaMaskUrl', () => { it('can parse a valid url with the client authority', () => { @@ -46,7 +40,7 @@ describe('parseMetaMaskUrl', () => { it('will throw on an invalid scheme', () => { expect(() => parseMetaMaskUrl('metmask://client/')).toThrow( - 'Invalid MetaMask url: invalid scheme.', + 'Unable to parse URL. Expected the protocol to be "metamask:", but received "metmask:".', ); }); @@ -67,23 +61,7 @@ describe('parseMetaMaskUrl', () => { it('will throw on an invalid client page', () => { expect(() => parseMetaMaskUrl('metamask://client/foo')).toThrow( - 'Invalid MetaMask url: invalid client path.', + 'Unable to navigate to "/foo". The provided path is not allowed.', ); }); }); - -describe('isMetaMaskUrl', () => { - it.each(['https://www.google.com', 'metamask://foo/'])( - 'will return false for non-metamask urls', - (url) => { - expect(isMetaMaskUrl(url)).toBe(false); - }, - ); - - it.each(['metamask://snap/home', 'metamask://client/'])( - 'will not throw for valid metamask urls', - (url) => { - expect(isMetaMaskUrl(url)).toBe(true); - }, - ); -}); diff --git a/packages/snaps-utils/src/url.ts b/packages/snaps-utils/src/url.ts index c951e59684..b8b6f8dfb0 100644 --- a/packages/snaps-utils/src/url.ts +++ b/packages/snaps-utils/src/url.ts @@ -1,97 +1,80 @@ import { type SnapId } from '@metamask/snaps-sdk'; -import { type Infer, is, pattern, string } from '@metamask/superstruct'; import { assert } from '@metamask/utils'; import { assertIsValidSnapId, stripSnapPrefix } from './snaps'; -export const METAMASK_URL_REGEX = - /^metamask:\/\/(?(client|snap))(?.+)$/u; - -export const MetaMaskUrlStruct = pattern(string(), METAMASK_URL_REGEX); - -export type MetaMaskUrl = Infer; - export type Authority = 'client' | 'snap'; -export const ExtensionPaths = ['/']; +export const CLIENT_PATHS = ['/']; -export const SnapPaths = ['/home']; +export const SNAP_PATHS = ['/home']; /** - * Parse a url string to an object containing the authority and path. - * This validates the url before parsing it. + * Parse a url string to an object containing the authority, path and Snap id (if snap authority). + * This also validates the url before parsing it. * * @param str - The url string to validate and parse. - * @returns The parsed url. + * @returns A parsed url object. * @throws If the authority or path is invalid. */ -export function parseMetaMaskUrl(str: MetaMaskUrl): { +export function parseMetaMaskUrl(str: string): { authority: Authority; snapId?: SnapId; path: string; } { const baseErrorMessage = 'Invalid MetaMask url:'; - try { - const url = new URL(str); - const { hostname: authority, pathname: path, protocol } = url; - if (protocol !== 'metamask:') { - throw new Error('invalid scheme.'); - } - if (authority === 'client') { + const url = new URL(str); + const { hostname: authority, pathname: path, protocol } = url; + if (protocol !== 'metamask:') { + throw new Error( + `Unable to parse URL. Expected the protocol to be "metamask:", but received "${protocol}".`, + ); + } + if (authority === 'client') { + assert( + CLIENT_PATHS.includes(path), + `Unable to navigate to "${path}". The provided path is not allowed.`, + ); + return { + authority, + path, + }; + } else if (authority === 'snap') { + const strippedPath = stripSnapPrefix(path.slice(1)); + const location = path.slice(1).startsWith('npm:') ? 'npm:' : 'local:'; + const isNameSpaced = strippedPath.startsWith('@'); + const pathTokens = strippedPath.split('/'); + const lastPathToken = `/${pathTokens[pathTokens.length - 1]}`; + let partialSnapId; + if (location === 'local:') { + const [localProtocol, , ...rest] = pathTokens.slice(0, -1); + partialSnapId = `${localProtocol}//${rest.join('/')}`; + // we can't make assumptions of the structure of the local snap url since it can have a nested path + // so we only check that the last path token is one of the allowed paths assert( - ExtensionPaths.includes(path), - `${baseErrorMessage} invalid client path.`, + SNAP_PATHS.includes(lastPathToken), + `${baseErrorMessage} invalid snap path.`, + ); + } else { + partialSnapId = isNameSpaced + ? `${pathTokens[0]}/${pathTokens[1]}` + : pathTokens[0]; + assert( + isNameSpaced + ? pathTokens.length === 3 && SNAP_PATHS.includes(lastPathToken) + : pathTokens.length === 2 && SNAP_PATHS.includes(lastPathToken), + `${baseErrorMessage} invalid snap path.`, ); - return { - authority, - path, - }; - } else if (authority === 'snap') { - const strippedPath = stripSnapPrefix(path.slice(1)); - const location = path.slice(1).startsWith('npm:') ? 'npm:' : 'local:'; - const isNameSpaced = strippedPath.startsWith('@'); - const pathTokens = strippedPath.split('/'); - const lastPathToken = `/${pathTokens[pathTokens.length - 1]}`; - let partialSnapId; - if (location === 'local:') { - partialSnapId = `http://${pathTokens[2]}`; - assert( - pathTokens.length === 4 && SnapPaths.includes(lastPathToken), - `${baseErrorMessage} invalid snap path.`, - ); - } else { - partialSnapId = isNameSpaced - ? `${pathTokens[0]}/${pathTokens[1]}` - : pathTokens[0]; - assert( - isNameSpaced - ? pathTokens.length === 3 && SnapPaths.includes(lastPathToken) - : pathTokens.length === 2 && SnapPaths.includes(lastPathToken), - `${baseErrorMessage} invalid snap path.`, - ); - } - const snapId = `${location}${partialSnapId}`; - assertIsValidSnapId(snapId); - - return { - authority, - snapId, - path: lastPathToken, - }; } + const snapId = `${location}${partialSnapId}`; + assertIsValidSnapId(snapId); - throw new Error('invalid authority.'); - } catch (error) { - throw new Error(`${baseErrorMessage} ${error.message}`); + return { + authority, + snapId, + path: lastPathToken, + }; } -} -/** - * Check if the given value is a MetaMask url. - * - * @param value - The value to check. - * @returns Whether the value is a MetaMask url. - */ -export function isMetaMaskUrl(value: unknown): value is MetaMaskUrl { - return is(value, MetaMaskUrlStruct); + throw new Error(`${baseErrorMessage} invalid authority.`); } From 396bc37d1736dc3afc52730d351d7846933ed26a Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 17 Sep 2024 13:14:45 -0400 Subject: [PATCH 14/22] add note to jsdoc --- packages/snaps-utils/src/url.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/snaps-utils/src/url.ts b/packages/snaps-utils/src/url.ts index b8b6f8dfb0..316c51b07a 100644 --- a/packages/snaps-utils/src/url.ts +++ b/packages/snaps-utils/src/url.ts @@ -13,6 +13,8 @@ export const SNAP_PATHS = ['/home']; * Parse a url string to an object containing the authority, path and Snap id (if snap authority). * This also validates the url before parsing it. * + * Note: The Snap id returned from this function should always be validated to be an installed snap. + * * @param str - The url string to validate and parse. * @returns A parsed url object. * @throws If the authority or path is invalid. From 9b5f00bac520b9116206a9947b7d1cf6204737b4 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 19 Sep 2024 10:30:52 -0400 Subject: [PATCH 15/22] make changes per review --- packages/snaps-utils/src/url.ts | 103 +++++++++++++++++++------------- 1 file changed, 61 insertions(+), 42 deletions(-) diff --git a/packages/snaps-utils/src/url.ts b/packages/snaps-utils/src/url.ts index 316c51b07a..e6fd525afc 100644 --- a/packages/snaps-utils/src/url.ts +++ b/packages/snaps-utils/src/url.ts @@ -32,51 +32,70 @@ export function parseMetaMaskUrl(str: string): { `Unable to parse URL. Expected the protocol to be "metamask:", but received "${protocol}".`, ); } - if (authority === 'client') { - assert( - CLIENT_PATHS.includes(path), - `Unable to navigate to "${path}". The provided path is not allowed.`, - ); - return { - authority, - path, - }; - } else if (authority === 'snap') { - const strippedPath = stripSnapPrefix(path.slice(1)); - const location = path.slice(1).startsWith('npm:') ? 'npm:' : 'local:'; - const isNameSpaced = strippedPath.startsWith('@'); - const pathTokens = strippedPath.split('/'); - const lastPathToken = `/${pathTokens[pathTokens.length - 1]}`; - let partialSnapId; - if (location === 'local:') { - const [localProtocol, , ...rest] = pathTokens.slice(0, -1); - partialSnapId = `${localProtocol}//${rest.join('/')}`; - // we can't make assumptions of the structure of the local snap url since it can have a nested path - // so we only check that the last path token is one of the allowed paths + switch (authority) { + case 'client': assert( - SNAP_PATHS.includes(lastPathToken), - `${baseErrorMessage} invalid snap path.`, + CLIENT_PATHS.includes(path), + `Unable to navigate to "${path}". The provided path is not allowed.`, ); - } else { - partialSnapId = isNameSpaced - ? `${pathTokens[0]}/${pathTokens[1]}` - : pathTokens[0]; - assert( - isNameSpaced - ? pathTokens.length === 3 && SNAP_PATHS.includes(lastPathToken) - : pathTokens.length === 2 && SNAP_PATHS.includes(lastPathToken), - `${baseErrorMessage} invalid snap path.`, - ); - } - const snapId = `${location}${partialSnapId}`; - assertIsValidSnapId(snapId); + return { + authority, + path, + }; + case 'snap': + return parseSnapPath(path, baseErrorMessage); + default: + throw new Error(`${baseErrorMessage} invalid authority.`); + } +} - return { - authority, - snapId, - path: lastPathToken, - }; +/** + * Parses a snap path and throws if it is invalid, returns an object with link data otherwise. + * + * @param path - The snap path to be parsed. + * @param baseErrorMessage - The base error message to throw. + * @returns A parsed url object. + */ +function parseSnapPath( + path: string, + baseErrorMessage: string, +): { + authority: Authority; + snapId: SnapId; + path: string; +} { + const strippedPath = stripSnapPrefix(path.slice(1)); + const location = path.slice(1).startsWith('npm:') ? 'npm:' : 'local:'; + const isNameSpaced = strippedPath.startsWith('@'); + const pathTokens = strippedPath.split('/'); + const lastPathToken = `/${pathTokens[pathTokens.length - 1]}`; + let partialSnapId; + if (location === 'local:') { + const [localProtocol, , ...rest] = pathTokens.slice(0, -1); + partialSnapId = `${localProtocol}//${rest.join('/')}`; + // we can't make assumptions of the structure of the local snap url since it can have a nested path + // so we only check that the last path token is one of the allowed paths + assert( + SNAP_PATHS.includes(lastPathToken), + `${baseErrorMessage} invalid snap path.`, + ); + } else { + partialSnapId = isNameSpaced + ? `${pathTokens[0]}/${pathTokens[1]}` + : pathTokens[0]; + assert( + isNameSpaced + ? pathTokens.length === 3 && SNAP_PATHS.includes(lastPathToken) + : pathTokens.length === 2 && SNAP_PATHS.includes(lastPathToken), + `${baseErrorMessage} invalid snap path.`, + ); } + const snapId = `${location}${partialSnapId}`; + assertIsValidSnapId(snapId); - throw new Error(`${baseErrorMessage} invalid authority.`); + return { + authority: 'snap' as Authority, + snapId, + path: lastPathToken, + }; } From 34cb66a626ac3e2bf41afa272cda12970bd62c39 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 19 Sep 2024 11:52:40 -0400 Subject: [PATCH 16/22] update jsdoc --- packages/snaps-utils/src/url.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/snaps-utils/src/url.ts b/packages/snaps-utils/src/url.ts index e6fd525afc..0e2f65243e 100644 --- a/packages/snaps-utils/src/url.ts +++ b/packages/snaps-utils/src/url.ts @@ -55,6 +55,7 @@ export function parseMetaMaskUrl(str: string): { * @param path - The snap path to be parsed. * @param baseErrorMessage - The base error message to throw. * @returns A parsed url object. + * @throws If the path or Snap id is invalid. */ function parseSnapPath( path: string, From e6009c3decdedb51813b09042deb7f8695127484 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Mon, 23 Sep 2024 15:02:23 -0400 Subject: [PATCH 17/22] rebuild --- packages/examples/packages/browserify-plugin/snap.manifest.json | 2 +- packages/examples/packages/browserify/snap.manifest.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/examples/packages/browserify-plugin/snap.manifest.json b/packages/examples/packages/browserify-plugin/snap.manifest.json index d0e563b2c0..e6bfac87e3 100644 --- a/packages/examples/packages/browserify-plugin/snap.manifest.json +++ b/packages/examples/packages/browserify-plugin/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "jwgw2+/MUcLjedTMSpFqL/dhwR8/j3gHWmNJVevvfWE=", + "shasum": "oeprRlfJxxGH+tvOyO7i4hy3l0SjzR3rA73tZS7vlZk=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/browserify/snap.manifest.json b/packages/examples/packages/browserify/snap.manifest.json index 10e6d77211..bf36e9f6a9 100644 --- a/packages/examples/packages/browserify/snap.manifest.json +++ b/packages/examples/packages/browserify/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "u+bnPdpw6us0nDDr3k28cfDoIA+5CT1Y7A88nYJLNGk=", + "shasum": "PL+Bg6eUiAl4uBSuGbX4gdcrgxFZIedy/N7FjVMzKIY=", "location": { "npm": { "filePath": "dist/bundle.js", From 972329b0ff56688dcf9dbff1db9f7b5e55cd735b Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 24 Sep 2024 08:30:01 -0400 Subject: [PATCH 18/22] apply code review --- package.json | 3 +-- packages/snaps-utils/src/url.ts | 16 +++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/package.json b/package.json index d36335534a..f96a9c88d8 100644 --- a/package.json +++ b/package.json @@ -133,6 +133,5 @@ "vite>esbuild": true, "tsx>esbuild": true } - }, - "packageManager": "yarn@4.4.1" + } } diff --git a/packages/snaps-utils/src/url.ts b/packages/snaps-utils/src/url.ts index 0e2f65243e..4e9ac30340 100644 --- a/packages/snaps-utils/src/url.ts +++ b/packages/snaps-utils/src/url.ts @@ -24,7 +24,6 @@ export function parseMetaMaskUrl(str: string): { snapId?: SnapId; path: string; } { - const baseErrorMessage = 'Invalid MetaMask url:'; const url = new URL(str); const { hostname: authority, pathname: path, protocol } = url; if (protocol !== 'metamask:') { @@ -43,28 +42,27 @@ export function parseMetaMaskUrl(str: string): { path, }; case 'snap': - return parseSnapPath(path, baseErrorMessage); + return parseSnapPath(path); default: - throw new Error(`${baseErrorMessage} invalid authority.`); + throw new Error( + `Expected "metamask:" URL to start with "client" or "snap", but received "${authority}".`, + ); } } /** - * Parses a snap path and throws if it is invalid, returns an object with link data otherwise. + * Parse a snap path and throws if it is invalid, returns an object with link data otherwise. * * @param path - The snap path to be parsed. - * @param baseErrorMessage - The base error message to throw. * @returns A parsed url object. * @throws If the path or Snap id is invalid. */ -function parseSnapPath( - path: string, - baseErrorMessage: string, -): { +function parseSnapPath(path: string): { authority: Authority; snapId: SnapId; path: string; } { + const baseErrorMessage = 'Invalid MetaMask url:'; const strippedPath = stripSnapPrefix(path.slice(1)); const location = path.slice(1).startsWith('npm:') ? 'npm:' : 'local:'; const isNameSpaced = strippedPath.startsWith('@'); From cc5b1e9458bfb0940335b63cbcd4ab255fbd093f Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 24 Sep 2024 08:33:51 -0400 Subject: [PATCH 19/22] fix order --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index f96a9c88d8..10aa81d912 100644 --- a/package.json +++ b/package.json @@ -116,6 +116,7 @@ "typescript": "~5.3.3", "vite": "^4.3.9" }, + "packageManager": "yarn@4.4.1", "engines": { "node": "^18.16 || >=20" }, From f361fd65298a2fc5b42a624df446722572ff57e1 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 24 Sep 2024 08:40:30 -0400 Subject: [PATCH 20/22] fix test --- packages/snaps-utils/src/url.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-utils/src/url.test.ts b/packages/snaps-utils/src/url.test.ts index b01708ffa8..aec81cf7b4 100644 --- a/packages/snaps-utils/src/url.test.ts +++ b/packages/snaps-utils/src/url.test.ts @@ -46,7 +46,7 @@ describe('parseMetaMaskUrl', () => { it('will throw on an invalid authority', () => { expect(() => parseMetaMaskUrl('metamask://bar/')).toThrow( - 'Invalid MetaMask url: invalid authority.', + 'Expected "metamask:" URL to start with "client" or "snap", but received "bar".', ); }); From f92e7689dd0b1cc7184e06dfb5bba8c4612e38b6 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 24 Sep 2024 10:41:50 -0400 Subject: [PATCH 21/22] fix coverage --- packages/snaps-controllers/coverage.json | 6 +++--- packages/snaps-utils/coverage.json | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 34b7a7350f..194585d11c 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { "branches": 92.6, - "functions": 96.95, - "lines": 98.02, - "statements": 97.72 + "functions": 96.65, + "lines": 97.97, + "statements": 97.67 } diff --git a/packages/snaps-utils/coverage.json b/packages/snaps-utils/coverage.json index bb96afe355..c2ea4d354f 100644 --- a/packages/snaps-utils/coverage.json +++ b/packages/snaps-utils/coverage.json @@ -2,5 +2,5 @@ "branches": 99.73, "functions": 98.9, "lines": 99.44, - "statements": 96.34 + "statements": 96.29 } From 11bfb5b3946f3fa1a2eb7d88cd8ad4c18c10509b Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 24 Sep 2024 10:51:08 -0400 Subject: [PATCH 22/22] fix coverage again --- packages/snaps-utils/coverage.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/snaps-utils/coverage.json b/packages/snaps-utils/coverage.json index c2ea4d354f..deb8bde080 100644 --- a/packages/snaps-utils/coverage.json +++ b/packages/snaps-utils/coverage.json @@ -1,6 +1,6 @@ { - "branches": 99.73, + "branches": 99.74, "functions": 98.9, - "lines": 99.44, + "lines": 99.45, "statements": 96.29 }