From 4b933ab1efd02431468953e9a7b41385a0bc9aa4 Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Tue, 15 Nov 2022 18:53:18 +0100 Subject: [PATCH 01/15] [TMP] Initial commit of snap location refactor --- .../src/snaps/location/http.ts | 48 +++ .../src/snaps/location/index.ts | 4 + .../src/snaps/location/local.ts | 43 +++ .../src/snaps/location/location.ts | 43 +++ .../src/snaps/location/npm.ts | 277 ++++++++++++++++++ 5 files changed, 415 insertions(+) create mode 100644 packages/snaps-controllers/src/snaps/location/http.ts create mode 100644 packages/snaps-controllers/src/snaps/location/index.ts create mode 100644 packages/snaps-controllers/src/snaps/location/local.ts create mode 100644 packages/snaps-controllers/src/snaps/location/location.ts create mode 100644 packages/snaps-controllers/src/snaps/location/npm.ts diff --git a/packages/snaps-controllers/src/snaps/location/http.ts b/packages/snaps-controllers/src/snaps/location/http.ts new file mode 100644 index 0000000000..f0538558d7 --- /dev/null +++ b/packages/snaps-controllers/src/snaps/location/http.ts @@ -0,0 +1,48 @@ +import { SnapManifest, assertIsSnapManifest } from '@metamask/snaps-utils'; + +import { assert } from '@metamask/utils'; +import { SnapLocation } from './location'; + +export class HttpLocation implements SnapLocation { + private cache = new Map(); + + private validatedManifest?: SnapManifest; + + private url: URL; + + constructor(url: URL) { + this.url = url; + assert(url.protocol === 'http:' || url.protocol === 'https:'); + } + + async manifest(): Promise { + if (this.validatedManifest) { + return this.validatedManifest; + } + + const manifest = await (await fetch(this.url)).json(); + assertIsSnapManifest(manifest); + this.validatedManifest = manifest; + return manifest; + } + + async fetch(path: string): Promise { + const canonical = this.toCanonical(path); + const cached = this.cache.get(canonical.href); + if (cached) { + return cached; + } + const response = await fetch(canonical); + const blob = await response.blob(); + this.cache.set(canonical.href, blob); + return blob; + } + + get root(): URL { + return new URL(this.url); + } + + private toCanonical(path: string): URL { + return new URL(path, this.url); + } +} diff --git a/packages/snaps-controllers/src/snaps/location/index.ts b/packages/snaps-controllers/src/snaps/location/index.ts new file mode 100644 index 0000000000..0a2ad00b8b --- /dev/null +++ b/packages/snaps-controllers/src/snaps/location/index.ts @@ -0,0 +1,4 @@ +export * from './location'; +export * from './npm'; +export * from './local'; +export * from './http'; diff --git a/packages/snaps-controllers/src/snaps/location/local.ts b/packages/snaps-controllers/src/snaps/location/local.ts new file mode 100644 index 0000000000..9f6c4c34c2 --- /dev/null +++ b/packages/snaps-controllers/src/snaps/location/local.ts @@ -0,0 +1,43 @@ +import { SnapManifest } from '@metamask/snaps-utils'; +import { assert } from '@metamask/utils'; +import { HttpLocation } from './http'; +import { SnapLocation } from './location'; + +export const LOCALHOST_HOSTNAMES = new Set(['localhost', '127.0.0.1', '::1']); + +export class LocalLocation implements SnapLocation { + private http: HttpLocation; + + constructor(url: URL) { + assert(url.protocol === 'local:'); + assert( + isLocalHost(url), + new TypeError('local: protocol, but hostname is not localhost'), + ); + const httpUrl = new URL(url); + httpUrl.protocol = 'http:'; + this.http = new HttpLocation(httpUrl); + } + + manifest(): Promise { + return this.http.manifest(); + } + + fetch(path: string): Promise { + return this.http.fetch(path); + } + + get shouldAlwaysReload() { + return true; + } +} + +/** + * Returns whether the `url` param is local or not. + * + * @param url - Url to check. + * @returns Whether the param is local. + */ +function isLocalHost(url: URL): boolean { + return LOCALHOST_HOSTNAMES.has(url.hostname); +} diff --git a/packages/snaps-controllers/src/snaps/location/location.ts b/packages/snaps-controllers/src/snaps/location/location.ts new file mode 100644 index 0000000000..9a8cc547aa --- /dev/null +++ b/packages/snaps-controllers/src/snaps/location/location.ts @@ -0,0 +1,43 @@ +import assert from 'assert'; +import { SnapManifest } from '@metamask/snaps-utils'; +import { HttpLocation } from './http'; +import { LocalLocation } from './local'; +import { NpmLocation } from './npm'; + +export interface SnapLocation { + // TODO(ritave): Package.json object + manifest(): Promise; + fetch(path: string): Promise; + + readonly shouldAlwaysReload?: boolean; +} + +/** + * @param location + * @param versionRange + * @param opts + */ +export async function detectSnapLocation( + location: string | URL, + versionRange: string, + opts = { allowHttp: false }, +): Promise { + const root = new URL(location); + switch (root.protocol) { + case 'npm:': + return NpmLocation.create(root, versionRange); + case 'local:': + return new LocalLocation(root); + case 'http:': + case 'https:': + assert( + opts.allowHttp, + new TypeError('Fetching snaps from external http/https is disabled.'), + ); + return new HttpLocation(root); + default: + throw new TypeError( + `Unrecognized "${root.protocol}" snap location protocol`, + ); + } +} diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts new file mode 100644 index 0000000000..59169f4a8c --- /dev/null +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -0,0 +1,277 @@ +import { Readable, Writable } from 'stream'; +import { assert, isObject } from '@metamask/utils'; +import concat from 'concat-stream'; +import createGunzipStream from 'gunzip-maybe'; +import pump from 'pump'; +import { ReadableWebToNodeStream } from 'readable-web-to-node-stream'; +import { extract as tarExtract } from 'tar-stream'; +import { + getTargetVersion, + isValidUrl, + SnapManifest, +} from '@metamask/snaps-utils'; +import { SnapLocation } from './location'; + +const DEFAULT_NPM_REGISTRY = 'https://registry.npmjs.org'; + +interface NpmMeta { + registry: URL; + packageName: string; + requestedRange: string; + actualVersion: string; +} + +export interface NpmOptions { + /** + * The function used to fetch data. + */ + fetch?: typeof fetch; + /** + * Whether to allow custom NPM registries outside of {@link DEFAULT_NPM_REGISTRY}. + * + * @default false + */ + allowCustomRegistries?: boolean; +} + +export class NpmLocation implements SnapLocation { + static async create( + root: string | URL, + versionRange: string, + opts: NpmOptions = { + fetch, + allowCustomRegistries: false, + }, + ): Promise { + const allowCustomRegistries = opts.allowCustomRegistries ?? false; + + const base = new URL(root); + assert(base.protocol === 'npm:'); + + let registry; + if ( + base.host === '' && + base.port === '' && + base.username === '' && + base.password === '' + ) { + registry = new URL(DEFAULT_NPM_REGISTRY); + } else { + registry = 'https://'; + if (base.username) { + registry += base.username; + if (base.password) { + registry += `:${base.password}`; + } + registry += '@'; + } + registry += base.host; + registry = new URL(registry); + assert( + allowCustomRegistries === true, + new TypeError( + `Custom NPM registries are disabled, tried use "${registry}"`, + ), + ); + } + + assert( + base.pathname !== '' && base.pathname !== '/', + new TypeError('The package name in NPM location is empty'), + ); + let packageName = base.pathname; + if (packageName[0] === '/') { + packageName = packageName.slice(1); + } + + const [tarballResponse, actualVersion] = await fetchNpmTarball( + packageName, + versionRange, + registry, + opts.fetch ?? fetch, + ); + + // TODO(ritave): Lazily extract tar instead of up-front + const data = new Map(); + await new Promise((resolve, reject) => { + pump( + getNodeStream(tarballResponse), + // The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed + // before we can actually grab any files from it. + createGunzipStream(), + createTarballStream(data), + (error) => { + error ? reject(error) : resolve(); + }, + ); + }); + + return new NpmLocation( + { registry, packageName, requestedRange: versionRange, actualVersion }, + data, + ); + } + + private validatedManifest?: SnapManifest; + + private meta: NpmMeta; + + private data: Map; + + private constructor(meta: NpmMeta, data: Map) { + this.meta = meta; + this.data = data; + } + + async manifest(): Promise { + if (this.validatedManifest) { + return this.validatedManifest; + } + + const file = JSON.parse(await (await this.fetch('/package.json')).text()); + this.validatedManifest = file; + return file; + } + + async fetch(path: string): Promise { + let myPath = path; + if (path[0] === '/') { + myPath = path.slice(1); + } + const contents = this.data.get(myPath); + assert(contents !== undefined); + return contents; + } + + get packageName(): string { + return this.meta.packageName; + } + + get version(): string { + return this.meta.actualVersion; + } +} + +/** + * Fetches the tarball (`.tgz` file) of the specified package and version from + * the public npm registry. Throws an error if fetching fails. + * + * @param packageName - The name of the package whose tarball to fetch. + * @param versionRange - The SemVer range of the package to fetch. The highest + * version satisfying the range will be fetched. + * @param registryUrl - The URL of the npm registry to fetch the tarball from. + * @param fetchFunction - The fetch function to use. Defaults to the global + * {@link fetch}. Useful for Node.js compatibility. + * @returns A tuple of the {@link Response} for the package tarball and the + * actual version of the package. + */ +async function fetchNpmTarball( + packageName: string, + versionRange: string, + registryUrl: URL | string, + fetchFunction: typeof fetch, +): Promise<[ReadableStream, string]> { + const packageMetadata = await ( + await fetchFunction(new URL(packageName, registryUrl).toString()) + ).json(); + + if (!isObject(packageMetadata)) { + throw new Error( + `Failed to fetch package "${packageName}" metadata from npm.`, + ); + } + + const targetVersion = getTargetVersion( + Object.keys((packageMetadata as any)?.versions ?? {}), + versionRange, + ); + + if (targetVersion === null) { + throw new Error( + `Failed to find a matching version in npm metadata for package "${packageName}" and requested semver range "${versionRange}"`, + ); + } + + const tarballUrlString = (packageMetadata as any).versions?.[targetVersion] + ?.dist?.tarball; + + if (!isValidUrl(tarballUrlString) || !tarballUrlString.endsWith('.tgz')) { + throw new Error( + `Failed to find valid tarball URL in npm metadata for package "${packageName}".`, + ); + } + + // Override the tarball hostname/protocol with registryUrl hostname/protocol + const newRegistryUrl = new URL(registryUrl); + const newTarballUrl = new URL(tarballUrlString); + newTarballUrl.hostname = newRegistryUrl.hostname; + newTarballUrl.protocol = newRegistryUrl.protocol; + + // Perform a raw fetch because we want the Response object itself. + const tarballResponse = await fetchFunction(newTarballUrl.toString()); + if (!tarballResponse.ok) { + throw new Error(`Failed to fetch tarball for package "${packageName}".`); + } + const stream = await tarballResponse.blob().then((blob) => blob.stream()); + + return [stream, targetVersion]; +} + +/** + * The paths of files within npm tarballs appear to always be prefixed with + * "package/". + */ +const NPM_TARBALL_PATH_PREFIX = /^package\//u; + +/** + * Converts a {@link ReadableStream} to a Node.js {@link Readable} + * stream. Returns the stream directly if it is already a Node.js stream. + * We can't use the native Web {@link ReadableStream} directly because the + * other stream libraries we use expect Node.js streams. + * + * @param stream - The stream to convert. + * @returns The given stream as a Node.js Readable stream. + */ +function getNodeStream(stream: ReadableStream): Readable { + if (typeof stream.getReader !== 'function') { + return stream as unknown as Readable; + } + + return new ReadableWebToNodeStream(stream); +} + +/** + * Creates a `tar-stream` that will get the necessary files from an npm Snap + * package tarball (`.tgz` file). + * + * @param files - An object to write target file contents to. + * @returns The {@link Writable} tarball extraction stream. + */ +function createTarballStream(files: Map): Writable { + // `tar-stream` is pretty old-school, so we create it first and then + // instrument it by adding event listeners. + const extractStream = tarExtract(); + + // "entry" is fired for every discreet entity in the tarball. This includes + // files and folders. + extractStream.on('entry', (header, entryStream, next) => { + const { name: headerName, type: headerType } = header; + if (headerType === 'file') { + // The name is a path if the header type is "file". + const filePath = headerName.replace(NPM_TARBALL_PATH_PREFIX, ''); + return entryStream.pipe( + concat((data) => { + files.set(filePath, new Blob([data])); + return next(); + }), + ); + } + + // If we get here, the entry is not a file, and we want to ignore. The entry + // stream must be drained, or the extractStream will stop reading. This is + // effectively a no-op for the current entry. + entryStream.on('end', () => next()); + return entryStream.resume(); + }); + return extractStream; +} From a96957b788b77e809b357af2861be60d345e2768 Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Mon, 28 Nov 2022 19:52:54 +0100 Subject: [PATCH 02/15] Tests are now all green --- .../src/snaps/SnapController.test.ts | 473 +++++++----------- .../src/snaps/SnapController.ts | 254 ++++------ packages/snaps-controllers/src/snaps/index.ts | 1 - .../src/snaps/location/http.ts | 69 ++- .../src/snaps/location/index.ts | 6 +- .../src/snaps/location/local.ts | 53 +- .../src/snaps/location/location.test.ts | 12 + .../src/snaps/location/location.ts | 53 +- .../src/snaps/{utils => location}/npm.test.ts | 41 +- .../src/snaps/location/npm.ts | 262 ++++++---- .../src/snaps/utils/index.ts | 2 - .../snaps-controllers/src/snaps/utils/npm.ts | 133 ----- .../src/snaps/utils/stream.test.ts | 20 - .../src/snaps/utils/stream.ts | 144 ------ .../snaps-controllers/src/test-utils/index.ts | 1 + .../src/test-utils/location.ts | 127 +++++ packages/snaps-utils/src/index.browser.ts | 1 + packages/snaps-utils/src/index.ts | 2 + packages/snaps-utils/src/snaps.ts | 73 ++- packages/snaps-utils/src/test-utils/snap.ts | 2 +- packages/snaps-utils/src/vfile/to-vfile.ts | 61 +++ packages/snaps-utils/src/vfile/vfile.ts | 103 ++++ 22 files changed, 983 insertions(+), 910 deletions(-) create mode 100644 packages/snaps-controllers/src/snaps/location/location.test.ts rename packages/snaps-controllers/src/snaps/{utils => location}/npm.test.ts (65%) delete mode 100644 packages/snaps-controllers/src/snaps/utils/index.ts delete mode 100644 packages/snaps-controllers/src/snaps/utils/npm.ts delete mode 100644 packages/snaps-controllers/src/snaps/utils/stream.test.ts delete mode 100644 packages/snaps-controllers/src/snaps/utils/stream.ts create mode 100644 packages/snaps-controllers/src/test-utils/location.ts create mode 100644 packages/snaps-utils/src/vfile/to-vfile.ts create mode 100644 packages/snaps-utils/src/vfile/vfile.ts diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 0c86ce1f37..1723a18328 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -9,13 +9,15 @@ import { DEFAULT_ENDOWMENTS, getSnapSourceShasum, HandlerType, + SemVerRange, SemVerVersion, SnapCaveatType, - SnapManifest, SnapStatus, + VFile, } from '@metamask/snaps-utils'; import { DEFAULT_SNAP_BUNDLE, + DEFAULT_SNAP_ICON, getMockSnapData, getPersistedSnapObject, getSnapManifest, @@ -25,7 +27,9 @@ import { MOCK_ORIGIN, MOCK_SNAP_ID, } from '@metamask/snaps-utils/test-utils'; +import { AssertionError } from '@metamask/utils'; import { Crypto } from '@peculiar/webcrypto'; +import { assert } from 'console'; import { EthereumRpcError, ethErrors, serializeError } from 'eth-rpc-errors'; import fetchMock from 'jest-fetch-mock'; import { createAsyncMiddleware, JsonRpcEngine } from 'json-rpc-engine'; @@ -52,6 +56,8 @@ import { MOCK_SNAP_SUBJECT_METADATA, PERSISTED_MOCK_KEYRING_SNAP, sleep, + loopbackDetect, + LoopbackLocation, } from '../test-utils'; import { delay } from '../utils'; import { handlerEndowments, SnapEndowments } from './endowments'; @@ -66,17 +72,16 @@ Object.defineProperty(window, 'crypto', { }, }); -jest.mock('./utils/npm', () => ({ - ...jest.requireActual('./utils/npm'), - fetchNpmSnap: jest.fn().mockResolvedValue({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), -})); - fetchMock.enableMocks(); describe('SnapController', () => { + beforeEach(() => { + // eslint-disable-next-line @typescript-eslint/require-await + fetchMock.mockImplementation(async () => { + throw new AssertionError({ message: 'Unmocked access to internet.' }); + }); + }); + it('creates a snap controller and execution service', async () => { const [snapController, service] = getSnapControllerWithEES(); expect(service).toBeDefined(); @@ -535,22 +540,12 @@ describe('SnapController', () => { const snapController = getSnapController( getSnapControllerOptions({ messenger, + detectSnapLocation: loopbackDetect(), }), ); jest.spyOn(messenger, 'publish'); - jest - .spyOn(snapController as any, 'fetchSnap') - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest({ - shasum: getSnapSourceShasum(DEFAULT_SNAP_BUNDLE), - }), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ); - const eventSubscriptionPromise = Promise.all([ new Promise((resolve) => { messenger.subscribe('SnapController:snapAdded', (snap) => { @@ -673,7 +668,10 @@ describe('SnapController', () => { it('fails to install snap if user rejects installation', async () => { const messenger = getSnapControllerMessenger(); const controller = getSnapController( - getSnapControllerOptions({ messenger }), + getSnapControllerOptions({ + messenger, + detectSnapLocation: loopbackDetect(), + }), ); jest @@ -685,15 +683,6 @@ describe('SnapController', () => { return true; }); - jest - .spyOn(controller as any, 'fetchSnap') - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ); - const result = await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {}, }); @@ -713,6 +702,7 @@ describe('SnapController', () => { const snapController = getSnapController( getSnapControllerOptions({ messenger, + detectSnapLocation: loopbackDetect(), }), ); @@ -722,14 +712,6 @@ describe('SnapController', () => { .mockImplementation(); jest.spyOn(messenger, 'publish'); - jest - .spyOn(snapController as any, 'fetchSnap') - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ); jest .spyOn(snapController as any, 'authorize') @@ -1680,6 +1662,11 @@ describe('SnapController', () => { id: MOCK_LOCAL_SNAP_ID, }); + const location = new LoopbackLocation({ + manifest: snapObject.manifest, + shouldAlwaysReload: true, + }); + const snapController = getSnapController( getSnapControllerOptions({ messenger, @@ -1688,16 +1675,10 @@ describe('SnapController', () => { [MOCK_LOCAL_SNAP_ID]: snapObject, }, }, + detectSnapLocation: loopbackDetect(location), }), ); - const fetchSnapMock = jest - .spyOn(snapController as any, 'fetchSnap') - .mockImplementationOnce(() => { - return { - ...snapObject, - }; - }); const stopSnapSpy = jest.spyOn(snapController, 'stopSnap'); const result = await snapController.installSnaps(MOCK_ORIGIN, { @@ -1761,8 +1742,7 @@ describe('SnapController', () => { SnapEndowments.LongRunning, ); - expect(fetchSnapMock).toHaveBeenCalledTimes(1); - expect(fetchSnapMock).toHaveBeenCalledWith(MOCK_LOCAL_SNAP_ID, '*'); + expect(location.manifest).toHaveBeenCalledTimes(1); expect(stopSnapSpy).not.toHaveBeenCalled(); }); @@ -1780,22 +1760,24 @@ describe('SnapController', () => { id: MOCK_LOCAL_SNAP_ID, }); + const location = new LoopbackLocation({ shouldAlwaysReload: true }); + /* eslint-disable @typescript-eslint/require-await */ + location.manifest + .mockImplementationOnce( + async () => new VFile({ value: '', result: manifest }), + ) + .mockImplementationOnce( + async () => new VFile({ value: '', result: newManifest }), + ); + /* eslint-enable @typescript-eslint/require-await */ + const snapController = getSnapController( getSnapControllerOptions({ messenger, + detectSnapLocation: loopbackDetect(location), }), ); - const fetchSnapMock = jest - .spyOn(snapController as any, 'fetchSnap') - .mockImplementationOnce(() => ({ - manifest, - sourceCode: DEFAULT_SNAP_BUNDLE, - })) - .mockImplementationOnce(() => ({ - manifest: newManifest, - sourceCode: DEFAULT_SNAP_BUNDLE, - })); const stopSnapSpy = jest.spyOn(snapController, 'stopSnap'); await snapController.installSnaps(MOCK_ORIGIN, { @@ -1924,11 +1906,6 @@ describe('SnapController', () => { MOCK_LOCAL_SNAP_ID, SnapEndowments.LongRunning, ); - - expect(fetchSnapMock).toHaveBeenCalledTimes(2); - - expect(fetchSnapMock).toHaveBeenNthCalledWith(1, MOCK_LOCAL_SNAP_ID, '*'); - expect(fetchSnapMock).toHaveBeenNthCalledWith(2, MOCK_LOCAL_SNAP_ID, '*'); expect(stopSnapSpy).toHaveBeenCalledTimes(1); }); @@ -1937,19 +1914,16 @@ describe('SnapController', () => { const rootMessenger = getControllerMessenger(); const messenger = getSnapControllerMessenger(rootMessenger); const snapController = getSnapController( - getSnapControllerOptions({ messenger }), + getSnapControllerOptions({ + messenger, + detectSnapLocation: loopbackDetect({ manifest }), + }), ); const truncatedSnap = getTruncatedSnap({ initialPermissions: manifest.initialPermissions, }); - const fetchSnapMock = jest - .spyOn(snapController as any, 'fetchSnap') - .mockImplementationOnce(() => { - return getPersistedSnapObject({ manifest }); - }); - const result = await snapController.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {}, }); @@ -1957,7 +1931,6 @@ describe('SnapController', () => { expect(result).toStrictEqual({ [MOCK_SNAP_ID]: truncatedSnap, }); - expect(fetchSnapMock).toHaveBeenCalledTimes(1); expect(messenger.call).toHaveBeenCalledTimes(5); expect(messenger.call).toHaveBeenNthCalledWith( 1, @@ -2029,15 +2002,12 @@ describe('SnapController', () => { const messenger = getSnapControllerMessenger(); const snapController = getSnapController( - getSnapControllerOptions({ messenger }), + getSnapControllerOptions({ + messenger, + detectSnapLocation: loopbackDetect({ manifest }), + }), ); - jest - .spyOn(snapController as any, 'fetchSnap') - .mockImplementationOnce(() => { - return getPersistedSnapObject({ manifest }); - }); - await snapController.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {}, }); @@ -2098,19 +2068,29 @@ describe('SnapController', () => { }); it('maps endowment permission caveats to the proper format', async () => { - const { manifest } = PERSISTED_MOCK_KEYRING_SNAP; + const keyringSnap = PERSISTED_MOCK_KEYRING_SNAP; + const { manifest } = keyringSnap; const messenger = getSnapControllerMessenger(); const snapController = getSnapController( - getSnapControllerOptions({ messenger }), + getSnapControllerOptions({ + messenger, + detectSnapLocation: loopbackDetect({ + manifest, + files: [ + new VFile({ + value: keyringSnap.sourceCode, + path: manifest.source.location.npm.filePath, + }), + new VFile({ + value: DEFAULT_SNAP_ICON, + path: manifest.source.location.npm.iconPath, + }), + ], + }), + }), ); - jest - .spyOn(snapController as any, 'fetchSnap') - .mockImplementationOnce(() => { - return getPersistedSnapObject({ manifest }); - }); - await snapController.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {}, }); @@ -2184,6 +2164,7 @@ describe('SnapController', () => { state: { snaps: getPersistedSnapsState(), }, + detectSnapLocation: loopbackDetect({ manifest }), }), ); @@ -2192,12 +2173,6 @@ describe('SnapController', () => { () => ({}), ); - jest - .spyOn(snapController as any, 'fetchSnap') - .mockImplementationOnce(() => { - return getPersistedSnapObject({ manifest }); - }); - await snapController.updateSnap(MOCK_ORIGIN, MOCK_SNAP_ID); expect(messenger.call).toHaveBeenNthCalledWith( @@ -2270,8 +2245,23 @@ describe('SnapController', () => { const rootMessenger = getControllerMessenger(); const messenger = getSnapControllerMessenger(rootMessenger); + const detectLocationMock = jest + .fn() + .mockImplementationOnce( + () => new LoopbackLocation({ manifest: getSnapManifest() }), + ) + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: getSnapManifest({ version: newVersion }), + }), + ); + const controller = getSnapController( - getSnapControllerOptions({ messenger }), + getSnapControllerOptions({ + messenger, + detectSnapLocation: detectLocationMock, + }), ); rootMessenger.registerActionHandler( @@ -2279,27 +2269,13 @@ describe('SnapController', () => { () => ({}), ); - const fetchSnapMock = jest - .spyOn(controller as any, 'fetchSnap') - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ) - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest({ version: newVersion }), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ); - await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {} }); await controller.stopSnap(MOCK_SNAP_ID); const result = await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: { version: newVersionRange }, }); + assert(false, new Error(JSON.stringify(result))); expect(messenger.call).toHaveBeenCalledTimes(12); expect(messenger.call).toHaveBeenNthCalledWith( @@ -2373,11 +2349,11 @@ describe('SnapController', () => { SnapEndowments.LongRunning, ); - expect(fetchSnapMock).toHaveBeenCalledTimes(2); - expect(fetchSnapMock).toHaveBeenNthCalledWith( + expect(detectLocationMock).toHaveBeenCalledTimes(2); + expect(detectLocationMock).toHaveBeenNthCalledWith( 2, MOCK_SNAP_ID, - newVersionRange, + expect.objectContaining({ versionRange: newVersionRange }), ); expect(result).toStrictEqual({ @@ -2390,6 +2366,9 @@ describe('SnapController', () => { const newVersion = '0.9.0'; const newVersionRange = '^0.9.0'; + const detect = loopbackDetect({ + manifest: getSnapManifest({ version: newVersion }), + }); const messenger = getSnapControllerMessenger(); const controller = getSnapController( getSnapControllerOptions({ @@ -2397,18 +2376,10 @@ describe('SnapController', () => { state: { snaps: getPersistedSnapsState(), }, + detectSnapLocation: detect, }), ); - const fetchSnapMock = jest - .spyOn(controller as any, 'fetchSnap') - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest({ version: newVersion }), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ); - const result = await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: { version: newVersionRange }, }); @@ -2419,8 +2390,12 @@ describe('SnapController', () => { MOCK_ORIGIN, expect.anything(), ); - expect(fetchSnapMock).toHaveBeenCalledTimes(1); - expect(fetchSnapMock).toHaveBeenCalledWith(MOCK_SNAP_ID, newVersionRange); + expect(detect).toHaveBeenCalledTimes(1); + expect(detect).toHaveBeenCalledWith( + MOCK_SNAP_ID, + expect.objectContaining({ versionRange: newVersionRange }), + ); + expect(result).toStrictEqual({ [MOCK_SNAP_ID]: { error: expect.any(EthereumRpcError) }, }); @@ -2431,19 +2406,21 @@ describe('SnapController', () => { const newVersionRange = '^1.0.1'; const messenger = getSnapControllerMessenger(); + const location = new LoopbackLocation(); + location.manifest.mockImplementationOnce(async () => + Promise.reject(new Error('foo')), + ); + const detect = loopbackDetect(location); const controller = getSnapController( getSnapControllerOptions({ messenger, state: { snaps: getPersistedSnapsState(), }, + detectSnapLocation: detect, }), ); - const fetchSnapMock = jest - .spyOn(controller as any, 'fetchSnap') - .mockImplementationOnce(async () => Promise.reject(new Error('foo'))); - const result = await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: { version: newVersionRange }, }); @@ -2454,8 +2431,12 @@ describe('SnapController', () => { MOCK_ORIGIN, expect.anything(), ); - expect(fetchSnapMock).toHaveBeenCalledTimes(1); - expect(fetchSnapMock).toHaveBeenCalledWith(MOCK_SNAP_ID, newVersionRange); + expect(detect).toHaveBeenCalledTimes(1); + expect(detect).toHaveBeenCalledWith( + MOCK_SNAP_ID, + expect.objectContaining({ versionRange: newVersionRange }), + ); + expect(result).toStrictEqual({ [MOCK_SNAP_ID]: { error: expect.anything() }, }); @@ -2495,21 +2476,11 @@ describe('SnapController', () => { state: { snaps: getPersistedSnapsState(), }, + detectSnapLocation: loopbackDetect({ + manifest: getSnapManifest({ version: '1.1.0' as SemVerVersion }), + }), }), ); - const fetchSnapSpy = jest.spyOn(controller as any, 'fetchSnap'); - - fetchSnapSpy.mockImplementationOnce(async () => { - const manifest: SnapManifest = { - ...getSnapManifest(), - version: '1.1.0' as SemVerVersion, - }; - - return Promise.resolve({ - manifest, - sourceCode: DEFAULT_SNAP_BUNDLE, - }); - }); checkBlockListSpy.mockResolvedValueOnce({ [MOCK_SNAP_ID]: { blocked: true }, @@ -2528,24 +2499,14 @@ describe('SnapController', () => { state: { snaps: getPersistedSnapsState(), }, + detectSnapLocation: loopbackDetect({ + manifest: getSnapManifest({ version: '0.9.0' as SemVerVersion }), + }), }), ); - const fetchSnapSpy = jest.spyOn(controller as any, 'fetchSnap'); const onSnapUpdated = jest.fn(); const onSnapAdded = jest.fn(); - fetchSnapSpy.mockImplementationOnce(async () => { - const manifest: SnapManifest = { - ...getSnapManifest(), - version: '0.9.0' as SemVerVersion, - }; - - return Promise.resolve({ - manifest, - sourceCode: DEFAULT_SNAP_BUNDLE, - }); - }); - const snap = controller.getExpect(MOCK_SNAP_ID); messenger.subscribe('SnapController:snapUpdated', onSnapUpdated); @@ -2557,7 +2518,6 @@ describe('SnapController', () => { expect(result).toBeNull(); expect(newSnap?.version).toStrictEqual(snap.version); - expect(fetchSnapSpy).toHaveBeenCalledTimes(1); expect(onSnapUpdated).not.toHaveBeenCalled(); expect(onSnapAdded).not.toHaveBeenCalled(); }); @@ -2565,32 +2525,25 @@ describe('SnapController', () => { it('updates a snap', async () => { const messenger = getSnapControllerMessenger(); const controller = getSnapController( - getSnapControllerOptions({ messenger }), + getSnapControllerOptions({ + messenger, + detectSnapLocation: jest + .fn() + .mockImplementationOnce(() => new LoopbackLocation()) + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: getSnapManifest({ + version: '1.1.0' as SemVerVersion, + }), + }), + ), + }), ); - const fetchSnapSpy = jest.spyOn(controller as any, 'fetchSnap'); const callActionSpy = jest.spyOn(messenger, 'call'); const onSnapUpdated = jest.fn(); const onSnapAdded = jest.fn(); - fetchSnapSpy - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ) - .mockImplementationOnce(async () => { - const manifest: SnapManifest = { - ...getSnapManifest(), - version: '1.1.0' as SemVerVersion, - }; - - return Promise.resolve({ - manifest, - sourceCode: DEFAULT_SNAP_BUNDLE, - }); - }); - callActionSpy.mockImplementation((method, ...args): any => { if (method === 'PermissionController:hasPermission') { return true; @@ -2628,7 +2581,6 @@ describe('SnapController', () => { date: expect.any(Number), }, ]); - expect(fetchSnapSpy).toHaveBeenCalledTimes(2); expect(callActionSpy).toHaveBeenCalledTimes(11); expect(callActionSpy).toHaveBeenNthCalledWith( 7, @@ -2706,23 +2658,13 @@ describe('SnapController', () => { state: { snaps: getPersistedSnapsState(), }, + detectSnapLocation: loopbackDetect({ + manifest: getSnapManifest({ version: '1.1.0' as SemVerVersion }), + }), }), ); - const fetchSnapSpy = jest.spyOn(controller as any, 'fetchSnap'); const callActionSpy = jest.spyOn(messenger, 'call'); - fetchSnapSpy.mockImplementationOnce(async () => { - const manifest: SnapManifest = { - ...getSnapManifest(), - version: '1.1.0' as SemVerVersion, - }; - - return Promise.resolve({ - manifest, - sourceCode: DEFAULT_SNAP_BUNDLE, - }); - }); - callActionSpy.mockImplementation((method, ...args): any => { if (method === 'PermissionController:hasPermission') { return true; @@ -2743,7 +2685,6 @@ describe('SnapController', () => { const isRunning = controller.isRunning(MOCK_SNAP_ID); - expect(fetchSnapSpy).toHaveBeenCalledTimes(1); expect(callActionSpy).toHaveBeenCalledTimes(8); expect(callActionSpy).toHaveBeenNthCalledWith( @@ -2813,23 +2754,13 @@ describe('SnapController', () => { state: { snaps: getPersistedSnapsState(), }, + detectSnapLocation: loopbackDetect({ + manifest: getSnapManifest({ version: '1.1.0' as SemVerVersion }), + }), }), ); - const fetchSnapSpy = jest.spyOn(controller as any, 'fetchSnap'); const callActionSpy = jest.spyOn(messenger, 'call'); - fetchSnapSpy.mockImplementationOnce(async () => { - const manifest: SnapManifest = { - ...getSnapManifest(), - version: '1.1.0' as SemVerVersion, - }; - - return Promise.resolve({ - manifest, - sourceCode: DEFAULT_SNAP_BUNDLE, - }); - }); - callActionSpy.mockImplementation((method, ..._args: unknown[]) => { if (method === 'PermissionController:hasPermission') { return true; @@ -2848,7 +2779,6 @@ describe('SnapController', () => { const newSnap = controller.get(MOCK_SNAP_ID); expect(newSnap?.version).toBe('1.0.0'); - expect(fetchSnapSpy).toHaveBeenCalledTimes(1); expect(callActionSpy).toHaveBeenCalledTimes(2); expect(callActionSpy).toHaveBeenNthCalledWith( 1, @@ -2883,9 +2813,6 @@ describe('SnapController', () => { it('requests approval for new and already approved permissions and revoke unused permissions', async () => { const messenger = getSnapControllerMessenger(); - const controller = getSnapController( - getSnapControllerOptions({ messenger }), - ); /* eslint-disable @typescript-eslint/naming-convention */ const initialPermissions = { @@ -2912,30 +2839,34 @@ describe('SnapController', () => { }, }; - const fetchSnapSpy = jest.spyOn(controller as any, 'fetchSnap'); const callActionSpy = jest.spyOn(messenger, 'call'); - fetchSnapSpy - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest({ initialPermissions }), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), + /* eslint-disable @typescript-eslint/require-await */ + const detect = jest + .fn() + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: getSnapManifest({ initialPermissions }), + }), ) - .mockImplementationOnce(async () => { - const manifest: SnapManifest = getSnapManifest({ - version: '1.1.0', - initialPermissions: { - snap_confirm: {}, - 'endowment:network-access': {}, - }, - }); + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: getSnapManifest({ + version: '1.1.0' as SemVerRange, + initialPermissions: { + snap_confirm: {}, + 'endowment:network-access': {}, + }, + }), + }), + ); + /* eslint-enable @typescript-eslint/require-await */ - return Promise.resolve({ - manifest, - sourceCode: DEFAULT_SNAP_BUNDLE, - }); - }); + const controller = getSnapController( + getSnapControllerOptions({ messenger, detectSnapLocation: detect }), + ); callActionSpy.mockImplementation((method, ...args): any => { if (method === 'PermissionController:hasPermission') { @@ -2958,7 +2889,6 @@ describe('SnapController', () => { await controller.updateSnap(MOCK_ORIGIN, MOCK_SNAP_ID); - expect(fetchSnapSpy).toHaveBeenCalledTimes(2); expect(callActionSpy).toHaveBeenCalledTimes(12); expect(callActionSpy).toHaveBeenNthCalledWith( 7, @@ -3044,11 +2974,6 @@ describe('SnapController', () => { expect.assertions(2); const messenger = getSnapControllerMessenger(); - const snapController = getSnapController( - getSnapControllerOptions({ - messenger, - }), - ); /* eslint-disable @typescript-eslint/naming-convention */ const initialPermissions = { @@ -3074,32 +2999,36 @@ describe('SnapController', () => { }, }; - const fetchSnapSpy = jest.spyOn(snapController as any, 'fetchSnap'); - const callActionSpy = jest.spyOn(messenger, 'call'); - - fetchSnapSpy - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest({ initialPermissions }), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), + const detect = jest + .fn() + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: getSnapManifest({ initialPermissions }), + }), ) - .mockImplementationOnce(async () => { - const manifest: SnapManifest = getSnapManifest({ - version: '1.1.0', - initialPermissions: { - snap_confirm: {}, - 'endowment:network-access': {}, - }, - }); - - return Promise.resolve({ - manifest, - sourceCode: DEFAULT_SNAP_BUNDLE, - }); - }); + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: getSnapManifest({ + version: '1.1.0' as SemVerRange, + initialPermissions: { + snap_confirm: {}, + 'endowment:network-access': {}, + }, + }), + }), + ); + const callActionSpy = jest.spyOn(messenger, 'call'); /* eslint-enable @typescript-eslint/naming-convention */ + const snapController = getSnapController( + getSnapControllerOptions({ + messenger, + detectSnapLocation: detect, + }), + ); + callActionSpy.mockImplementation((method, ...args: unknown[]): any => { if (method === 'PermissionController:hasPermission') { return true; @@ -3125,38 +3054,6 @@ describe('SnapController', () => { }); }); - describe('fetchSnap', () => { - it('can fetch NPM snaps', async () => { - const controller = getSnapController(); - - const result = await controller.installSnaps(MOCK_ORIGIN, { - [MOCK_SNAP_ID]: {}, - }); - expect(result).toStrictEqual({ [MOCK_SNAP_ID]: getTruncatedSnap() }); - }); - - it('can fetch local snaps', async () => { - const controller = getSnapController(); - - fetchMock - .mockResponseOnce(JSON.stringify(getSnapManifest())) - .mockResponseOnce(DEFAULT_SNAP_BUNDLE); - - const id = 'local:https://localhost:8081'; - const result = await controller.installSnaps(MOCK_ORIGIN, { - [id]: {}, - }); - // Fetch is called 3 times, for fetching the manifest, the sourcecode and icon (icon just has the default response for now) - expect(fetchMock).toHaveBeenCalledTimes(3); - expect(result).toStrictEqual({ - [id]: getTruncatedSnap({ - id, - permissionName: 'wallet_snap_local:https://localhost:8081', - }), - }); - }); - }); - describe('enableSnap', () => { it('enables a disabled snap', () => { const snapController = getSnapController( diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index f522b14c26..13789ee434 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -27,12 +27,9 @@ import { DEFAULT_REQUESTED_SNAP_VERSION, fromEntries, getSnapPermissionName, - getSnapPrefix, gtVersion, InstallSnapsResult, isValidSemVerRange, - LOCALHOST_HOSTNAMES, - NpmSnapFileNames, PersistedSnap, ProcessSnapResult, RequestedSnapPermissions, @@ -41,7 +38,6 @@ import { SemVerRange, Snap, SnapId, - SnapIdPrefixes, SnapManifest, SnapPermissions, SnapRpcHook, @@ -58,6 +54,7 @@ import { ValidatedSnapId, validateSnapId, validateSnapShasum, + VFile, } from '@metamask/snaps-utils'; import { GetSubjectMetadata, @@ -65,7 +62,6 @@ import { } from '@metamask/subject-metadata-controller'; import { assert, - assertExhaustive, Duration, hasProperty, inMilliseconds, @@ -95,9 +91,9 @@ import { SnapEndowments, } from './endowments'; import { getRpcCaveatOrigins } from './endowments/rpc'; +import { detectSnapLocation, SnapLocation } from './location'; import { RequestQueue } from './RequestQueue'; import { Timer } from './Timer'; -import { fetchNpmSnap } from './utils'; export const controllerName = 'SnapController'; @@ -188,17 +184,19 @@ type FetchSnapResult = { /** * The manifest of the fetched Snap. */ - manifest: SnapManifest; + manifest: VFile; /** - * The source code of the fetched Snap. + * Auxillary files references in manifest. */ - sourceCode: string; + files: VFile[]; /** - * The raw XML content of the Snap's SVG icon, if any. + * Location that was used to fetch the snap. + * + * Helpful if you want to pass it forward since files will be still cached. */ - svgIcon?: string; + location: SnapLocation; }; // Types that probably should be defined elsewhere in prod @@ -545,30 +543,31 @@ type SnapControllerArgs = { * Persisted state that will be used for rehydration. */ state?: PersistedSnapControllerState; -}; -type AddSnapArgsBase = { + /** + * A function that takes Snap Id and converts it into a class that fetches files. + * + * Used for test overrides. + */ + detectSnapLocation?: typeof detectSnapLocation; +}; +type AddSnapArgs = { id: SnapId; origin: string; - versionRange?: SemVerRange; + location: SnapLocation; }; -// A snap can either be added directly, with manifest and source code, or it -// can be fetched and then added. -type AddSnapArgs = - | AddSnapArgsBase - | (AddSnapArgsBase & { - manifest: SnapManifest; - sourceCode: string; - }); - // When we set a snap, we need all required properties to be present and // validated. -type SetSnapArgs = Omit & { +type SetSnapArgs = Omit & { id: ValidatedSnapId; - manifest: SnapManifest; - sourceCode: string; - svgIcon?: string; + manifest: VFile; + files: VFile[]; + /** + * @default '*' + */ + // TODO(ritave): Used only for validation in #set, should be moved elsewhere. + versionRange?: SemVerRange; }; const defaultState: SnapControllerState = { @@ -634,6 +633,8 @@ export class SnapController extends BaseController< #npmRegistryUrl?: string; + #detectSnapLocation: typeof detectSnapLocation; + // This property cannot be hash private yet because of tests. private readonly snapsRuntimeData: Map; @@ -660,6 +661,7 @@ export class SnapController extends BaseController< maxRequestTime = inMilliseconds(60, Duration.Second), fetchFunction = globalThis.fetch.bind(globalThis), featureFlags = {}, + detectSnapLocation: detectSnapLocationFunction = detectSnapLocation, }: SnapControllerArgs) { super({ messenger, @@ -726,6 +728,7 @@ export class SnapController extends BaseController< this.#maxIdleTime = maxIdleTime; this.maxRequestTime = maxRequestTime; this.#npmRegistryUrl = npmRegistryUrl; + this.#detectSnapLocation = detectSnapLocationFunction; this._onUnhandledSnapError = this._onUnhandledSnapError.bind(this); this._onOutboundRequest = this._onOutboundRequest.bind(this); this._onOutboundResponse = this._onOutboundResponse.bind(this); @@ -1612,9 +1615,11 @@ export class SnapController extends BaseController< }; } + const location = this.#detectSnapLocation(snapId, { versionRange }); + const existingSnap = this.getTruncated(snapId); // For devX we always re-install local snaps. - if (existingSnap && getSnapPrefix(snapId) !== SnapIdPrefixes.local) { + if (existingSnap && !location.shouldAlwaysReload) { if (satisfiesVersionRange(existingSnap.version, versionRange)) { return existingSnap; } @@ -1625,6 +1630,7 @@ export class SnapController extends BaseController< origin, snapId, versionRange, + location, ); if (updateResult === null) { return { @@ -1655,7 +1661,7 @@ export class SnapController extends BaseController< const { sourceCode } = await this.#add({ origin, id: snapId, - versionRange, + location, }); await this.authorize(origin, snapId); @@ -1694,12 +1700,14 @@ export class SnapController extends BaseController< * @param origin - The origin requesting the snap update. * @param snapId - The id of the Snap to be updated. * @param newVersionRange - A semver version range in which the maximum version will be chosen. + * @param location - Optional location that was already used during installation flow. * @returns The snap metadata if updated, `null` otherwise. */ async updateSnap( origin: string, snapId: ValidatedSnapId, newVersionRange: string = DEFAULT_REQUESTED_SNAP_VERSION, + location?: SnapLocation, ): Promise { const snap = this.getExpect(snapId); @@ -1708,9 +1716,15 @@ export class SnapController extends BaseController< `Received invalid snap version range: "${newVersionRange}".`, ); } - - const newSnap = await this.fetchSnap(snapId, newVersionRange); - const newVersion = newSnap.manifest.version; + const newSnap = await this.fetchSnap( + snapId, + location ?? + this.#detectSnapLocation(snapId, { versionRange: newVersionRange }), + ); + if (newSnap.manifest.result === undefined) { + throw new Error(JSON.stringify(newSnap.manifest)); + } + const newVersion = newSnap.manifest.result.version; if (!gtVersion(newVersion, snap.version)) { console.warn( `Tried updating snap "${snapId}" within "${newVersionRange}" version range, but newer version "${snap.version}" is already installed`, @@ -1720,11 +1734,11 @@ export class SnapController extends BaseController< await this.#assertIsUnblocked(snapId, { version: newVersion, - shasum: newSnap.manifest.source.shasum, + shasum: newSnap.manifest.result.source.shasum, }); const processedPermissions = this.#processSnapPermissions( - newSnap.manifest.initialPermissions, + newSnap.manifest.result.initialPermissions, ); const { newPermissions, unusedPermissions, approvedPermissions } = @@ -1743,7 +1757,7 @@ export class SnapController extends BaseController< metadata: { id, origin: snapId, dappOrigin: origin }, permissions: newPermissions, snapId, - newVersion: newSnap.manifest.version, + newVersion: newSnap.manifest.result.version, newPermissions, approvedPermissions, unusedPermissions, @@ -1762,7 +1776,7 @@ export class SnapController extends BaseController< origin, id: snapId, manifest: newSnap.manifest, - sourceCode: newSnap.sourceCode, + files: newSnap.files, versionRange: newVersionRange, }); @@ -1781,7 +1795,17 @@ export class SnapController extends BaseController< }); } - await this.#startSnap({ snapId, sourceCode: newSnap.sourceCode }); + const sourceCode = newSnap.files + .find( + (file) => + file.path === newSnap.manifest.result.source.location.npm.filePath, + ) + ?.toString(); + assert(sourceCode !== undefined); + await this.#startSnap({ + snapId, + sourceCode, + }); const truncatedSnap = this.getTruncatedExpect(snapId); this.messagingSystem.publish( @@ -1803,18 +1827,9 @@ export class SnapController extends BaseController< * @returns The resulting snap object. */ async #add(args: AddSnapArgs): Promise { - const { id: snapId } = args; + const { id: snapId, location } = args; validateSnapId(snapId); - if ( - !args || - !('origin' in args) || - !('id' in args) || - (!('manifest' in args) && 'sourceCode' in args) || - ('manifest' in args && !('sourceCode' in args)) - ) { - throw new Error(`Invalid add snap args for snap "${snapId}".`); - } this.#setupRuntime(snapId, { sourceCode: null, state: null }); const runtime = this.#getRuntimeExpect(snapId); if (!runtime.installPromise) { @@ -1823,14 +1838,10 @@ export class SnapController extends BaseController< // If fetching and setting the snap succeeds, this property will be set // to null in the authorize() method. runtime.installPromise = (async () => { - if ('manifest' in args && 'sourceCode' in args) { - return this.#set({ ...args, id: snapId }); - } - - const fetchedSnap = await this.fetchSnap(snapId, args.versionRange); + const fetchedSnap = await this.fetchSnap(snapId, location); await this.#assertIsUnblocked(snapId, { - version: fetchedSnap.manifest.version, - shasum: fetchedSnap.manifest.source.shasum, + version: fetchedSnap.manifest.result.version, + shasum: fetchedSnap.manifest.result.source.shasum, }); return this.#set({ @@ -1953,13 +1964,12 @@ export class SnapController extends BaseController< id: snapId, origin, manifest, - sourceCode, - svgIcon, + files, versionRange = DEFAULT_REQUESTED_SNAP_VERSION, } = args; - assertIsSnapManifest(manifest); - const { version } = manifest; + assertIsSnapManifest(manifest.result); + const { version } = manifest.result; if (!satisfiesVersionRange(version, versionRange)) { throw new Error( @@ -1967,19 +1977,19 @@ export class SnapController extends BaseController< ); } + const sourceCode = files + .find( + (file) => file.path === manifest.result.source.location.npm.filePath, + ) + ?.toString(); + const svgIcon = files.find( + (file) => file.path === manifest.result.source.location.npm.iconPath, + ); + assert(sourceCode !== undefined); if (typeof sourceCode !== 'string' || sourceCode.length === 0) { throw new Error(`Invalid source code for snap "${snapId}".`); } - const initialPermissions = manifest?.initialPermissions; - if ( - !initialPermissions || - typeof initialPermissions !== 'object' || - Array.isArray(initialPermissions) - ) { - throw new Error(`Invalid initial permissions for snap "${snapId}".`); - } - const snapsState = this.state.snaps; const existingSnap = snapsState[snapId]; @@ -2007,8 +2017,8 @@ export class SnapController extends BaseController< permissionName: getSnapPermissionName(snapId), id: snapId, - initialPermissions, - manifest, + initialPermissions: manifest.result.initialPermissions, + manifest: manifest.result, status: this.#statusMachine.config.initial as StatusStates['value'], version, versionHistory, @@ -2024,7 +2034,11 @@ export class SnapController extends BaseController< const runtime = this.#getRuntimeExpect(snapId); runtime.sourceCode = sourceCode; - this.messagingSystem.publish(`SnapController:snapAdded`, snap, svgIcon); + this.messagingSystem.publish( + `SnapController:snapAdded`, + snap, + svgIcon?.toString(), + ); return { ...snap, sourceCode }; } @@ -2034,104 +2048,32 @@ export class SnapController extends BaseController< * This function is not hash private yet because of tests. * * @param snapId - The id of the Snap. - * @param versionRange - The SemVer version of the Snap to fetch. + * @param location - Source from which snap will be fetched. * @returns A tuple of the Snap manifest object and the Snap source code. */ async fetchSnap( snapId: ValidatedSnapId, - versionRange: string = DEFAULT_REQUESTED_SNAP_VERSION, + location: SnapLocation, ): Promise { try { - const snapPrefix = getSnapPrefix(snapId); - switch (snapPrefix) { - case SnapIdPrefixes.local: - return this.#fetchLocalSnap(snapId.replace(SnapIdPrefixes.local, '')); - case SnapIdPrefixes.npm: - return this.#fetchNpmSnap( - snapId.replace(SnapIdPrefixes.npm, ''), - versionRange, - ); - /* istanbul ignore next */ - default: - // This whill fail to compile if the above switch is not fully exhaustive - return assertExhaustive(snapPrefix); - } - } catch (error) { - throw new Error( - `Failed to fetch Snap "${snapId}": ${(error as Error).message}`, - ); - } - } - - async #fetchNpmSnap( - packageName: string, - versionRange: string, - ): Promise { - if (!isValidSemVerRange(versionRange)) { - throw new Error( - `Received invalid Snap version range: "${versionRange}".`, + const manifest = await location.manifest(); + const sourceCode = await location.fetch( + manifest.result.source.location.npm.filePath, ); - } + validateSnapShasum(manifest.result, sourceCode.toString()); + const { iconPath } = manifest.result.source.location.npm; - const { manifest, sourceCode, svgIcon } = await fetchNpmSnap( - packageName, - versionRange, - this.#npmRegistryUrl, - this.#fetchFunction, - ); - return { manifest, sourceCode, svgIcon }; - } + const files = [sourceCode]; + if (iconPath) { + files.push(await location.fetch(iconPath)); + } - /** - * Fetches the manifest and source code of a local snap. - * - * @param localhostUrl - The localhost URL to download from. - * @returns The validated manifest and the source code. - */ - async #fetchLocalSnap(localhostUrl: string): Promise { - // Local snaps are mostly used for development purposes. Fetches were cached in the browser and were not requested - // afterwards which lead to confusing development where old versions of snaps were installed. - // Thus we disable caching - const fetchOptions: RequestInit = { cache: 'no-cache' }; - const manifestUrl = new URL(NpmSnapFileNames.Manifest, localhostUrl); - if (!LOCALHOST_HOSTNAMES.has(manifestUrl.hostname)) { + return { manifest, files, location }; + } catch (error) { throw new Error( - `Invalid URL: Locally hosted Snaps must be hosted on localhost. Received URL: "${manifestUrl.toString()}"`, + `Failed to fetch Snap "${snapId}": ${(error as Error).message}`, ); } - - const manifest = await ( - await this.#fetchFunction(manifestUrl.toString(), fetchOptions) - ).json(); - assertIsSnapManifest(manifest); - - const { - source: { - location: { - npm: { filePath, iconPath }, - }, - }, - } = manifest; - - const [sourceCode, svgIcon] = await Promise.all([ - ( - await this.#fetchFunction( - new URL(filePath, localhostUrl).toString(), - fetchOptions, - ) - ).text(), - iconPath - ? ( - await this.#fetchFunction( - new URL(iconPath, localhostUrl).toString(), - fetchOptions, - ) - ).text() - : undefined, - ]); - - validateSnapShasum(manifest, sourceCode); - return { manifest, sourceCode, svgIcon }; } /** diff --git a/packages/snaps-controllers/src/snaps/index.ts b/packages/snaps-controllers/src/snaps/index.ts index 8110b8e9b4..8dd75a0e60 100644 --- a/packages/snaps-controllers/src/snaps/index.ts +++ b/packages/snaps-controllers/src/snaps/index.ts @@ -1,4 +1,3 @@ -export * from './utils'; export * from './SnapController'; export * from './endowments'; export * from './selectors'; diff --git a/packages/snaps-controllers/src/snaps/location/http.ts b/packages/snaps-controllers/src/snaps/location/http.ts index f0538558d7..50a21d7ce2 100644 --- a/packages/snaps-controllers/src/snaps/location/http.ts +++ b/packages/snaps-controllers/src/snaps/location/http.ts @@ -1,41 +1,69 @@ -import { SnapManifest, assertIsSnapManifest } from '@metamask/snaps-utils'; +import { + SnapManifest, + assertIsSnapManifest, + VFile, + deepClone, + HttpSnapIdStruct, +} from '@metamask/snaps-utils'; +import { assert, assertStruct } from '@metamask/utils'; -import { assert } from '@metamask/utils'; import { SnapLocation } from './location'; -export class HttpLocation implements SnapLocation { - private cache = new Map(); +export default class HttpLocation implements SnapLocation { + // We keep contents separate because then we can use only one Blob in cache, + // which we convert to Uint8Array when actually returning the file. + // + // That avoids deepCloning file contents. + // I imagine ArrayBuffers are copy-on-write optimized, meaning + // in most often case we'll only have one file contents in common case. + private readonly cache: [file: VFile, contents: Blob][] = []; - private validatedManifest?: SnapManifest; + private validatedManifest?: VFile; - private url: URL; + private readonly url: URL; constructor(url: URL) { + assertStruct(url.toString(), HttpSnapIdStruct, 'Invalid Snap Id: '); this.url = url; - assert(url.protocol === 'http:' || url.protocol === 'https:'); } - async manifest(): Promise { + async manifest(): Promise> { if (this.validatedManifest) { - return this.validatedManifest; + return deepClone(this.validatedManifest); } - const manifest = await (await fetch(this.url)).json(); + const contents = await (await fetch(this.url)).text(); + const manifest = JSON.parse(contents); assertIsSnapManifest(manifest); - this.validatedManifest = manifest; - return manifest; + const vfile = new VFile({ + value: contents, + result: manifest, + path: './snap.manifest.json', + data: { canonicalPath: this.url.toString() }, + }); + this.validatedManifest = vfile; + + return this.manifest(); } - async fetch(path: string): Promise { - const canonical = this.toCanonical(path); - const cached = this.cache.get(canonical.href); - if (cached) { - return cached; + async fetch(path: string): Promise { + const cached = this.cache.find(([file]) => file.path === path); + if (cached !== undefined) { + const [cachedFile, contents] = cached; + const value = new Uint8Array(await contents.arrayBuffer()); + const vfile = deepClone(cachedFile); + vfile.value = value; + return vfile; } - const response = await fetch(canonical); + + const canonicalPath = this.toCanonical(path).toString(); + const response = await fetch(canonicalPath); + const vfile = new VFile({ value: '', path, data: { canonicalPath } }); + // TODO(ritave): When can we assume the file is string vs binary? const blob = await response.blob(); - this.cache.set(canonical.href, blob); - return blob; + this.cache.push([vfile, blob]); + + return this.fetch(path); } get root(): URL { @@ -43,6 +71,7 @@ export class HttpLocation implements SnapLocation { } private toCanonical(path: string): URL { + assert(!path.startsWith('/'), 'Tried to parse absolute path'); return new URL(path, this.url); } } diff --git a/packages/snaps-controllers/src/snaps/location/index.ts b/packages/snaps-controllers/src/snaps/location/index.ts index 0a2ad00b8b..58d233e48f 100644 --- a/packages/snaps-controllers/src/snaps/location/index.ts +++ b/packages/snaps-controllers/src/snaps/location/index.ts @@ -1,4 +1,4 @@ export * from './location'; -export * from './npm'; -export * from './local'; -export * from './http'; +export { default as NpmLocation } from './npm'; +export { default as LocalLocation } from './local'; +export { default as HttpLocation } from './http'; diff --git a/packages/snaps-controllers/src/snaps/location/local.ts b/packages/snaps-controllers/src/snaps/location/local.ts index 9f6c4c34c2..e04d2f4087 100644 --- a/packages/snaps-controllers/src/snaps/location/local.ts +++ b/packages/snaps-controllers/src/snaps/location/local.ts @@ -1,30 +1,32 @@ -import { SnapManifest } from '@metamask/snaps-utils'; -import { assert } from '@metamask/utils'; -import { HttpLocation } from './http'; -import { SnapLocation } from './location'; +import { + LocalSnapIdStruct, + SnapIdPrefixes, + SnapManifest, + VFile, +} from '@metamask/snaps-utils'; +import { assert, assertStruct } from '@metamask/utils'; -export const LOCALHOST_HOSTNAMES = new Set(['localhost', '127.0.0.1', '::1']); +import HttpLocation from './http'; +import { SnapLocation } from './location'; -export class LocalLocation implements SnapLocation { - private http: HttpLocation; +export default class LocalLocation implements SnapLocation { + readonly #http: HttpLocation; constructor(url: URL) { - assert(url.protocol === 'local:'); - assert( - isLocalHost(url), - new TypeError('local: protocol, but hostname is not localhost'), + assertStruct(url.toString(), LocalSnapIdStruct, 'Invalid Snap Id: '); + this.#http = new HttpLocation( + new URL(url.toString().slice(SnapIdPrefixes.local.length)), ); - const httpUrl = new URL(url); - httpUrl.protocol = 'http:'; - this.http = new HttpLocation(httpUrl); } - manifest(): Promise { - return this.http.manifest(); + async manifest(): Promise> { + const vfile = await this.#http.manifest(); + + return convertCanonical(vfile); } - fetch(path: string): Promise { - return this.http.fetch(path); + async fetch(path: string): Promise { + return convertCanonical(await this.#http.fetch(path)); } get shouldAlwaysReload() { @@ -33,11 +35,16 @@ export class LocalLocation implements SnapLocation { } /** - * Returns whether the `url` param is local or not. + * Converts vfiles with canonical `http:` paths into `local:` paths. * - * @param url - Url to check. - * @returns Whether the param is local. + * @param vfile - The {@link VFile} to convert. + * @returns The same object with updated `.data.canonicalPath`. */ -function isLocalHost(url: URL): boolean { - return LOCALHOST_HOSTNAMES.has(url.hostname); +function convertCanonical(vfile: VFile): VFile { + assert(vfile.data.canonicalPath !== undefined); + const canonicalPath = new URL(vfile.data.canonicalPath); + assert(canonicalPath.protocol === 'http:'); + canonicalPath.protocol = 'local:'; + vfile.data.canonicalPath = canonicalPath.toString(); + return vfile; } diff --git a/packages/snaps-controllers/src/snaps/location/location.test.ts b/packages/snaps-controllers/src/snaps/location/location.test.ts new file mode 100644 index 0000000000..dc697fe7bc --- /dev/null +++ b/packages/snaps-controllers/src/snaps/location/location.test.ts @@ -0,0 +1,12 @@ +import { detectSnapLocation } from './location'; + +describe('detectSnapLocation', () => { + it.each(['http:', 'https:'])( + 'disallows http like protocols by default', + (protocol) => { + expect(() => detectSnapLocation(`${protocol}//127.0.0.1/`)).toThrow( + 'Fetching snaps from external http/https is disabled.', + ); + }, + ); +}); diff --git a/packages/snaps-controllers/src/snaps/location/location.ts b/packages/snaps-controllers/src/snaps/location/location.ts index 9a8cc547aa..7bcce34d3e 100644 --- a/packages/snaps-controllers/src/snaps/location/location.ts +++ b/packages/snaps-controllers/src/snaps/location/location.ts @@ -1,37 +1,58 @@ +import { SnapManifest, VFile } from '@metamask/snaps-utils'; import assert from 'assert'; -import { SnapManifest } from '@metamask/snaps-utils'; -import { HttpLocation } from './http'; -import { LocalLocation } from './local'; -import { NpmLocation } from './npm'; + +import HttpLocation from './http'; +import LocalLocation from './local'; +import NpmLocation, { NpmOptions } from './npm'; + +declare module '@metamask/snaps-utils' { + interface DataMap { + /** + * Fully qualified, canonical path for the file in {@link https://github.com/MetaMask/SIPs/blob/main/SIPS/sip-8.md SIP-8 } URI format. + */ + canonicalPath: string; + } +} export interface SnapLocation { - // TODO(ritave): Package.json object - manifest(): Promise; - fetch(path: string): Promise; + /** + * All files are relative to the manifest, except the manifest itself. + */ + manifest(): Promise>; + fetch(path: string): Promise; readonly shouldAlwaysReload?: boolean; } +export type DetectSnapLocationOptions = NpmOptions & { + /** + * @default false + */ + allowHttp?: boolean; +}; + /** - * @param location - * @param versionRange - * @param opts + * Auto-magically detects which SnapLocation object to create based on the provided {@link location}. + * + * @param location - A {@link https://github.com/MetaMask/SIPs/blob/main/SIPS/sip-8.md SIP-8} uri. + * @param opts - NPM options and feature flags. + * @returns SnapLocation based on url. */ -export async function detectSnapLocation( +export function detectSnapLocation( location: string | URL, - versionRange: string, - opts = { allowHttp: false }, -): Promise { + opts?: DetectSnapLocationOptions, +): SnapLocation { + const allowHttp = opts?.allowHttp ?? false; const root = new URL(location); switch (root.protocol) { case 'npm:': - return NpmLocation.create(root, versionRange); + return new NpmLocation(root, opts); case 'local:': return new LocalLocation(root); case 'http:': case 'https:': assert( - opts.allowHttp, + allowHttp, new TypeError('Fetching snaps from external http/https is disabled.'), ); return new HttpLocation(root); diff --git a/packages/snaps-controllers/src/snaps/utils/npm.test.ts b/packages/snaps-controllers/src/snaps/location/npm.test.ts similarity index 65% rename from packages/snaps-controllers/src/snaps/utils/npm.test.ts rename to packages/snaps-controllers/src/snaps/location/npm.test.ts index 08d400ea16..67edf42e37 100644 --- a/packages/snaps-controllers/src/snaps/utils/npm.test.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.test.ts @@ -1,16 +1,17 @@ +import { assert } from '@metamask/utils'; import { createReadStream } from 'fs'; import { readFile } from 'fs/promises'; import fetchMock from 'jest-fetch-mock'; import path from 'path'; -import { fetchNpmSnap } from './npm'; +import NpmLocation from './npm'; -describe('fetchNpmSnap', () => { +describe('NpmLocation', () => { beforeEach(() => { fetchMock.resetMocks(); }); - it('fetches a package tarball, extracts the necessary files, and validates them', async () => { + it('fetches a package tarball, extracts the neecessary files, and validaes them', async () => { const { version: templateSnapVersion } = JSON.parse( ( await readFile(require.resolve('@metamask/template-snap/package.json')) @@ -49,13 +50,24 @@ describe('fetchNpmSnap', () => { }) as any, ); - const { manifest, sourceCode, svgIcon } = await fetchNpmSnap( - '@metamask/template-snap', - templateSnapVersion, - 'https://registry.npmjs.cf', - fetchMock as typeof fetch, + const location = new NpmLocation( + new URL('npm://registry.npmjs.cf/@metamask/template-snap'), + { + versionRange: templateSnapVersion, + fetch: fetchMock as typeof fetch, + allowCustomRegistries: true, + }, ); + const manifest = await location.manifest(); + const sourceCode = ( + await location.fetch(manifest.result.source.location.npm.filePath) + ).value.toString(); + assert(manifest.result.source.location.npm.iconPath); + const svgIcon = ( + await location.fetch(manifest.result.source.location.npm.iconPath) + ).value.toString(); + expect(fetchMock).toHaveBeenCalledTimes(2); expect(fetchMock).toHaveBeenNthCalledWith( 1, @@ -63,7 +75,7 @@ describe('fetchNpmSnap', () => { ); expect(fetchMock).toHaveBeenNthCalledWith(2, tarballUrl); - expect(manifest).toStrictEqual( + expect(manifest.result).toStrictEqual( JSON.parse( ( await readFile( @@ -85,4 +97,15 @@ describe('fetchNpmSnap', () => { true, ); }); + + it("can't use custom registries by default", () => { + expect( + () => + new NpmLocation( + new URL('npm://registry.npmjs.cf/@metamask/template-snap'), + ), + ).toThrow( + 'Custom NPM registries are disabled, tried to use "https://registry.npmjs.cf/"', + ); + }); }); diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts index 59169f4a8c..0ec0810b76 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -1,15 +1,24 @@ -import { Readable, Writable } from 'stream'; -import { assert, isObject } from '@metamask/utils'; -import concat from 'concat-stream'; -import createGunzipStream from 'gunzip-maybe'; -import pump from 'pump'; -import { ReadableWebToNodeStream } from 'readable-web-to-node-stream'; -import { extract as tarExtract } from 'tar-stream'; import { + assertIsSemVerVersion, + assertIsSnapManifest, + deepClone, + DEFAULT_REQUESTED_SNAP_VERSION, getTargetVersion, isValidUrl, + NpmSnapIdStruct, + SemVerRange, + SemVerVersion, SnapManifest, + VFile, } from '@metamask/snaps-utils'; +import { assert, assertStruct, isObject } from '@metamask/utils'; +import concat from 'concat-stream'; +import createGunzipStream from 'gunzip-maybe'; +import pump from 'pump'; +import { ReadableWebToNodeStream } from 'readable-web-to-node-stream'; +import { Readable, Writable } from 'stream'; +import { extract as tarExtract } from 'tar-stream'; + import { SnapLocation } from './location'; const DEFAULT_NPM_REGISTRY = 'https://registry.npmjs.org'; @@ -17,13 +26,19 @@ const DEFAULT_NPM_REGISTRY = 'https://registry.npmjs.org'; interface NpmMeta { registry: URL; packageName: string; - requestedRange: string; - actualVersion: string; + requestedRange: SemVerRange; + version?: string; + fetch: typeof fetch; } - export interface NpmOptions { + /** + * @default DEFAULT_REQUESTED_SNAP_VERSION + */ + versionRange?: SemVerRange; /** * The function used to fetch data. + * + * @default globalThis.fetch */ fetch?: typeof fetch; /** @@ -34,113 +49,99 @@ export interface NpmOptions { allowCustomRegistries?: boolean; } -export class NpmLocation implements SnapLocation { - static async create( - root: string | URL, - versionRange: string, - opts: NpmOptions = { - fetch, - allowCustomRegistries: false, - }, - ): Promise { +export default class NpmLocation implements SnapLocation { + private readonly meta: NpmMeta; + + private validatedManifest?: VFile; + + private files?: VFile[]; + + constructor(url: URL, opts: NpmOptions = {}) { const allowCustomRegistries = opts.allowCustomRegistries ?? false; + // eslint-disable-next-line @typescript-eslint/no-shadow + const fetch = opts.fetch ?? globalThis.fetch; + const requestedRange = opts.versionRange ?? DEFAULT_REQUESTED_SNAP_VERSION; - const base = new URL(root); - assert(base.protocol === 'npm:'); + assertStruct(url.toString(), NpmSnapIdStruct, 'Invalid Snap Id: '); - let registry; + let registry: string | URL; if ( - base.host === '' && - base.port === '' && - base.username === '' && - base.password === '' + url.host === '' && + url.port === '' && + url.username === '' && + url.password === '' ) { registry = new URL(DEFAULT_NPM_REGISTRY); } else { registry = 'https://'; - if (base.username) { - registry += base.username; - if (base.password) { - registry += `:${base.password}`; + if (url.username) { + registry += url.username; + if (url.password) { + registry += `:${url.password}`; } registry += '@'; } - registry += base.host; + registry += url.host; registry = new URL(registry); assert( - allowCustomRegistries === true, + allowCustomRegistries, new TypeError( - `Custom NPM registries are disabled, tried use "${registry}"`, + `Custom NPM registries are disabled, tried to use "${registry.toString()}"`, ), ); } assert( - base.pathname !== '' && base.pathname !== '/', + registry.pathname === '/' && + registry.search === '' && + registry.hash === '', + ); + + assert( + url.pathname !== '' && url.pathname !== '/', new TypeError('The package name in NPM location is empty'), ); - let packageName = base.pathname; - if (packageName[0] === '/') { + let packageName = url.pathname; + if (packageName.startsWith('/')) { packageName = packageName.slice(1); } - const [tarballResponse, actualVersion] = await fetchNpmTarball( - packageName, - versionRange, + this.meta = { + requestedRange, registry, - opts.fetch ?? fetch, - ); - - // TODO(ritave): Lazily extract tar instead of up-front - const data = new Map(); - await new Promise((resolve, reject) => { - pump( - getNodeStream(tarballResponse), - // The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed - // before we can actually grab any files from it. - createGunzipStream(), - createTarballStream(data), - (error) => { - error ? reject(error) : resolve(); - }, - ); - }); - - return new NpmLocation( - { registry, packageName, requestedRange: versionRange, actualVersion }, - data, - ); - } - - private validatedManifest?: SnapManifest; - - private meta: NpmMeta; - - private data: Map; - - private constructor(meta: NpmMeta, data: Map) { - this.meta = meta; - this.data = data; + packageName, + fetch, + }; } - async manifest(): Promise { + async manifest(): Promise> { if (this.validatedManifest) { - return this.validatedManifest; + return deepClone(this.validatedManifest); } - const file = JSON.parse(await (await this.fetch('/package.json')).text()); - this.validatedManifest = file; - return file; + const vfile = await this.fetch('./snap.manifest.json'); + const result = JSON.parse(vfile.value.toString()); + assertIsSnapManifest(result); + vfile.result = result; + this.validatedManifest = vfile as VFile; + + return this.manifest(); } - async fetch(path: string): Promise { - let myPath = path; - if (path[0] === '/') { - myPath = path.slice(1); + // eslint-disable-next-line @typescript-eslint/require-await + async fetch(path: string): Promise { + if (!this.files) { + await this.#lazyInit(); + assert(this.files !== undefined); } - const contents = this.data.get(myPath); - assert(contents !== undefined); - return contents; + assert(!path.startsWith('/'), 'Tried to fetch absolute path'); + const relativePath = ensureRelative(path); + const vfile = this.files.find((file) => file.path === relativePath); + assert( + vfile !== undefined, + new TypeError(`File "${path}" not found in package`), + ); + return deepClone(vfile); } get packageName(): string { @@ -148,7 +149,51 @@ export class NpmLocation implements SnapLocation { } get version(): string { - return this.meta.actualVersion; + assert( + this.meta.version !== undefined, + 'Tried to access version without first fetching NPM package', + ); + return this.meta.version; + } + + async #lazyInit() { + assert(this.files === undefined); + const [tarballResponse, actualVersion] = await fetchNpmTarball( + this.meta.packageName, + this.meta.requestedRange, + this.meta.registry, + this.meta.fetch, + ); + this.meta.version = actualVersion; + + let canonicalBase = 'npm://'; + if (this.meta.registry.username !== '') { + canonicalBase += this.meta.registry.username; + if (this.meta.registry.password !== '') { + canonicalBase += `:${this.meta.registry.password}`; + } + canonicalBase += '@'; + } + canonicalBase += this.meta.registry.host; + + // TODO(ritave): Lazily extract files instead of up-front extracting all of them + // We would need to replace tar-stream package because it requires immediate consumption of streams. + await new Promise((resolve, reject) => { + this.files = []; + pump( + getNodeStream(tarballResponse), + // The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed + // before we can actually grab any files from it. + createGunzipStream(), + createTarballStream( + `${canonicalBase}/${this.meta.packageName}/`, + this.files, + ), + (error) => { + error ? reject(error) : resolve(); + }, + ); + }); } } @@ -167,10 +212,10 @@ export class NpmLocation implements SnapLocation { */ async function fetchNpmTarball( packageName: string, - versionRange: string, + versionRange: SemVerRange, registryUrl: URL | string, fetchFunction: typeof fetch, -): Promise<[ReadableStream, string]> { +): Promise<[ReadableStream, SemVerVersion]> { const packageMetadata = await ( await fetchFunction(new URL(packageName, registryUrl).toString()) ).json(); @@ -181,11 +226,15 @@ async function fetchNpmTarball( ); } - const targetVersion = getTargetVersion( - Object.keys((packageMetadata as any)?.versions ?? {}), - versionRange, + const versions = Object.keys((packageMetadata as any)?.versions ?? {}).map( + (version) => { + assertIsSemVerVersion(version); + return version; + }, ); + const targetVersion = getTargetVersion(versions, versionRange); + if (targetVersion === null) { throw new Error( `Failed to find a matching version in npm metadata for package "${packageName}" and requested semver range "${versionRange}"`, @@ -244,10 +293,16 @@ function getNodeStream(stream: ReadableStream): Readable { * Creates a `tar-stream` that will get the necessary files from an npm Snap * package tarball (`.tgz` file). * + * @param canonicalBase - A base URI as specified in {@link https://github.com/MetaMask/SIPs/blob/main/SIPS/sip-8.md SIP-8}. Starting with 'npm:'. Will be used for canonicalPath vfile argument. * @param files - An object to write target file contents to. * @returns The {@link Writable} tarball extraction stream. */ -function createTarballStream(files: Map): Writable { +function createTarballStream(canonicalBase: string, files: VFile[]): Writable { + assert( + canonicalBase.endsWith('/'), + "Base needs to end with '/' for relative paths to be added as children instead of siblings.", + ); + assert(canonicalBase.startsWith('npm:'), 'Protocol mismatch, expected "npm:'); // `tar-stream` is pretty old-school, so we create it first and then // instrument it by adding event listeners. const extractStream = tarExtract(); @@ -258,10 +313,17 @@ function createTarballStream(files: Map): Writable { const { name: headerName, type: headerType } = header; if (headerType === 'file') { // The name is a path if the header type is "file". - const filePath = headerName.replace(NPM_TARBALL_PATH_PREFIX, ''); + const path = headerName.replace(NPM_TARBALL_PATH_PREFIX, './'); return entryStream.pipe( concat((data) => { - files.set(filePath, new Blob([data])); + const vfile = new VFile({ + value: data, + path, + data: { + canonicalPath: new URL(path, canonicalBase).toString(), + }, + }); + files.push(vfile); return next(); }), ); @@ -275,3 +337,17 @@ function createTarballStream(files: Map): Writable { }); return extractStream; } + +/** + * Ensures that a relative path starts with `./` prefix. + * + * @param path - Path to make relative. + * @returns The same path, with optional `./` prefix. + */ +function ensureRelative(path: string): string { + assert(!path.startsWith('/')); + if (path.startsWith('./')) { + return path; + } + return `./${path}`; +} diff --git a/packages/snaps-controllers/src/snaps/utils/index.ts b/packages/snaps-controllers/src/snaps/utils/index.ts deleted file mode 100644 index cf6fc7c86b..0000000000 --- a/packages/snaps-controllers/src/snaps/utils/index.ts +++ /dev/null @@ -1,2 +0,0 @@ -export * from './npm'; -export * from './stream'; diff --git a/packages/snaps-controllers/src/snaps/utils/npm.ts b/packages/snaps-controllers/src/snaps/utils/npm.ts deleted file mode 100644 index 510e14a959..0000000000 --- a/packages/snaps-controllers/src/snaps/utils/npm.ts +++ /dev/null @@ -1,133 +0,0 @@ -import { - SnapFiles, - UnvalidatedSnapFiles, - isValidUrl, - getTargetVersion, - validateNpmSnap, - assertIsSemVerVersion, - SemVerRange, - SemVerVersion, -} from '@metamask/snaps-utils'; -import { isObject } from '@metamask/utils'; -import createGunzipStream from 'gunzip-maybe'; -import pump from 'pump'; - -import { createTarballExtractionStream, getNodeStream } from './stream'; - -export const DEFAULT_NPM_REGISTRY = 'https://registry.npmjs.org'; - -/** - * Fetches a Snap from the public npm registry. - * - * @param packageName - The name of the package whose tarball to fetch. - * @param versionRange - The SemVer range of the package to fetch. The highest - * version satisfying the range will be fetched. - * @param registryUrl - The URL of the npm registry to fetch from. - * @param fetchFunction - The fetch function to use. Defaults to the global - * {@link fetch}. Useful for Node.js compatibility. - * @returns A tuple of the Snap manifest object and the Snap source code. - */ -export async function fetchNpmSnap( - packageName: string, - versionRange: SemVerRange, - registryUrl = DEFAULT_NPM_REGISTRY, - fetchFunction = fetch, -): Promise { - const [tarballResponse, actualVersion] = await fetchNpmTarball( - packageName, - versionRange, - registryUrl, - fetchFunction, - ); - - // Extract the tarball and get the necessary files from it. - const snapFiles: UnvalidatedSnapFiles = {}; - await new Promise((resolve, reject) => { - pump( - getNodeStream(tarballResponse), - // The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed - // before we can actually grab any files from it. - createGunzipStream(), - createTarballExtractionStream(snapFiles), - (error) => { - error ? reject(error) : resolve(); - }, - ); - }); - - // At this point, the necessary files will have been added to the snapFiles - // object if they exist. - return validateNpmSnap( - snapFiles, - `npm Snap "${packageName}@${actualVersion}" validation error: ` as `${string}: `, - ); -} - -/** - * Fetches the tarball (`.tgz` file) of the specified package and version from - * the public npm registry. Throws an error if fetching fails. - * - * @param packageName - The name of the package whose tarball to fetch. - * @param versionRange - The SemVer range of the package to fetch. The highest - * version satisfying the range will be fetched. - * @param registryUrl - The URL of the npm registry to fetch the tarball from. - * @param fetchFunction - The fetch function to use. Defaults to the global - * {@link fetch}. Useful for Node.js compatibility. - * @returns A tuple of the {@link Response} for the package tarball and the - * actual version of the package. - */ -async function fetchNpmTarball( - packageName: string, - versionRange: SemVerRange, - registryUrl = DEFAULT_NPM_REGISTRY, - fetchFunction = fetch, -): Promise<[ReadableStream, SemVerVersion]> { - const packageMetadata = await ( - await fetchFunction(new URL(packageName, registryUrl).toString()) - ).json(); - - if (!isObject(packageMetadata)) { - throw new Error( - `Failed to fetch package "${packageName}" metadata from npm.`, - ); - } - - const versions = Object.keys((packageMetadata as any)?.versions ?? {}).map( - (version) => { - assertIsSemVerVersion(version); - return version; - }, - ); - - const targetVersion = getTargetVersion(versions, versionRange); - - if (targetVersion === null) { - throw new Error( - `Failed to find a matching version in npm metadata for package "${packageName}" and requested semver range "${versionRange}"`, - ); - } - - const tarballUrlString = (packageMetadata as any).versions?.[targetVersion] - ?.dist?.tarball; - - if (!isValidUrl(tarballUrlString) || !tarballUrlString.endsWith('.tgz')) { - throw new Error( - `Failed to find valid tarball URL in npm metadata for package "${packageName}".`, - ); - } - - // Override the tarball hostname/protocol with registryUrl hostname/protocol - const newRegistryUrl = new URL(registryUrl); - const newTarballUrl = new URL(tarballUrlString); - newTarballUrl.hostname = newRegistryUrl.hostname; - newTarballUrl.protocol = newRegistryUrl.protocol; - - // Perform a raw fetch because we want the Response object itself. - const tarballResponse = await fetchFunction(newTarballUrl.toString()); - if (!tarballResponse.ok) { - throw new Error(`Failed to fetch tarball for package "${packageName}".`); - } - const stream = await tarballResponse.blob().then((blob) => blob.stream()); - - return [stream, targetVersion]; -} diff --git a/packages/snaps-controllers/src/snaps/utils/stream.test.ts b/packages/snaps-controllers/src/snaps/utils/stream.test.ts deleted file mode 100644 index 1fcc1695b8..0000000000 --- a/packages/snaps-controllers/src/snaps/utils/stream.test.ts +++ /dev/null @@ -1,20 +0,0 @@ -import { stripDotSlash } from './stream'; - -describe('stripDotSlash', () => { - it('handles inputs as expected', () => { - ( - [ - ['./foo', 'foo'], - ['./', ''], - ['', ''], - ['foo', 'foo'], - [undefined, undefined], - // Some contrived but illustrative examples - ['././', './'], - ['././foo', './foo'], - ] as const - ).forEach(([input, expected]) => { - expect(stripDotSlash(input)).toBe(expected); - }); - }); -}); diff --git a/packages/snaps-controllers/src/snaps/utils/stream.ts b/packages/snaps-controllers/src/snaps/utils/stream.ts deleted file mode 100644 index f6788e51d5..0000000000 --- a/packages/snaps-controllers/src/snaps/utils/stream.ts +++ /dev/null @@ -1,144 +0,0 @@ -import { - SnapManifest, - NpmSnapFileNames, - UnvalidatedSnapFiles, -} from '@metamask/snaps-utils'; -import { isObject } from '@metamask/utils'; -import concat from 'concat-stream'; -import { ReadableWebToNodeStream } from 'readable-web-to-node-stream'; -import { Readable, Writable } from 'stream'; -import { extract as tarExtract } from 'tar-stream'; - -// The paths of files within npm tarballs appear to always be prefixed with -// "package/". -const NPM_TARBALL_PATH_PREFIX = /^package\//u; - -/** - * Strips the leading `./` from a string, or does nothing if no string is - * provided. - * - * @param pathString - The path string to normalize. - * @returns The specified path without a `./` prefix, or `undefined` if no - * string was provided. - */ -export function stripDotSlash(pathString?: string): string | undefined { - return pathString?.replace(/^\.\//u, ''); -} - -/** - * Converts a {@link ReadableStream} to a Node.js {@link Readable} - * stream. Returns the stream directly if it is already a Node.js stream. - * We can't use the native Web {@link ReadableStream} directly because the - * other stream libraries we use expect Node.js streams. - * - * @param stream - The stream to convert. - * @returns The given stream as a Node.js Readable stream. - */ -export function getNodeStream(stream: ReadableStream): Readable { - if (typeof stream.getReader !== 'function') { - return stream as unknown as Readable; - } - - return new ReadableWebToNodeStream(stream); -} - -/** - * Creates a `tar-stream` that will get the necessary files from an npm Snap - * package tarball (`.tgz` file). - * - * @param snapFiles - An object to write target file contents to. - * @returns The {@link Writable} tarball extraction stream. - */ -export function createTarballExtractionStream( - snapFiles: UnvalidatedSnapFiles, -): Writable { - // `tar-stream` is pretty old-school, so we create it first and then - // instrument it by adding event listeners. - const extractStream = tarExtract(); - - // `tar-stream` reads every file in the tarball serially. We already know - // where to look for package.json and the Snap manifest, but we don't know - // where the source code is. Therefore, we cache the contents of each .js - // file in the tarball and pick out the correct one when the stream has ended. - const jsFileCache = new Map(); - - // "entry" is fired for every discreet entity in the tarball. This includes - // files and folders. - extractStream.on('entry', (header, entryStream, next) => { - const { name: headerName, type: headerType } = header; - if (headerType === 'file') { - // The name is a path if the header type is "file". - const filePath = headerName.replace(NPM_TARBALL_PATH_PREFIX, ''); - - // Note the use of `concat-stream` since the data for each file may be - // chunked. - if (filePath === NpmSnapFileNames.PackageJson) { - return entryStream.pipe( - concat((data) => { - try { - snapFiles.packageJson = JSON.parse(data.toString()); - } catch (_error) { - return extractStream.destroy( - new Error(`Failed to parse "${NpmSnapFileNames.PackageJson}".`), - ); - } - - return next(); - }), - ); - } else if (filePath === NpmSnapFileNames.Manifest) { - return entryStream.pipe( - concat((data) => { - try { - snapFiles.manifest = JSON.parse(data.toString()); - } catch (_error) { - return extractStream.destroy( - new Error(`Failed to parse "${NpmSnapFileNames.Manifest}".`), - ); - } - - return next(); - }), - ); - } else if (/\w+\.(?:js|svg)$/u.test(filePath)) { - return entryStream.pipe( - concat((data) => { - jsFileCache.set(filePath, data); - return next(); - }), - ); - } - } - - // If we get here, the entry is not a file, and we want to ignore. The entry - // stream must be drained, or the extractStream will stop reading. This is - // effectively a no-op for the current entry. - entryStream.on('end', () => next()); - return entryStream.resume(); - }); - - // Once we've read the entire tarball, attempt to grab the bundle file - // contents from the .js file cache. - extractStream.once('finish', () => { - if (isObject(snapFiles.manifest)) { - /* istanbul ignore next: optional chaining */ - const { filePath: _bundlePath, iconPath: _iconPath } = - (snapFiles.manifest as unknown as Partial).source - ?.location?.npm ?? {}; - - const bundlePath = stripDotSlash(_bundlePath); - const iconPath = stripDotSlash(_iconPath); - - if (bundlePath) { - snapFiles.sourceCode = jsFileCache.get(bundlePath)?.toString('utf8'); - } - - if (typeof iconPath === 'string' && iconPath.endsWith('.svg')) { - snapFiles.svgIcon = jsFileCache.get(iconPath)?.toString('utf8'); - } - } - jsFileCache.clear(); - }); - - return extractStream; -} diff --git a/packages/snaps-controllers/src/test-utils/index.ts b/packages/snaps-controllers/src/test-utils/index.ts index 725e72167d..b3f48621f8 100644 --- a/packages/snaps-controllers/src/test-utils/index.ts +++ b/packages/snaps-controllers/src/test-utils/index.ts @@ -3,3 +3,4 @@ export * from './execution-environment'; export * from './multichain'; export * from './service'; export * from './sleep'; +export * from './location'; diff --git a/packages/snaps-controllers/src/test-utils/location.ts b/packages/snaps-controllers/src/test-utils/location.ts new file mode 100644 index 0000000000..d306f52781 --- /dev/null +++ b/packages/snaps-controllers/src/test-utils/location.ts @@ -0,0 +1,127 @@ +import { VFile, SnapManifest } from '@metamask/snaps-utils'; +import { + DEFAULT_SNAP_BUNDLE, + DEFAULT_SNAP_ICON, + getSnapManifest, +} from '@metamask/snaps-utils/test-utils'; +import { assert } from '@metamask/utils'; +import { SnapLocation } from 'src/snaps/location'; + +const MANIFEST_PATH = './snap.manifest.json'; + +type LoopbackOptions = { + /** + * @default getSnapManifest() + */ + manifest?: SnapManifest | VFile; + /** + * @default [new VFile({ value: DEFAULT_SNAP_BUNDLE, path: manifest.source.location.npm.filePath }] + */ + files?: VFile[]; + /** + * @default false + */ + shouldAlwaysReload?: boolean; +}; + +const isVfile = (obj: unknown): obj is VFile => { + return ( + typeof obj === 'object' && + obj !== null && + typeof (obj as any).value === 'string' + ); +}; + +export class LoopbackLocation implements SnapLocation { + #manifest: VFile; + + #files: VFile[]; + + #shouldAlwaysReload: boolean; + + constructor(opts: LoopbackOptions = {}) { + const shouldAlwaysReload = opts.shouldAlwaysReload ?? false; + const manifest = isVfile(opts.manifest) + ? opts.manifest + : new VFile({ + value: '', + result: opts.manifest ?? getSnapManifest(), + path: './snap.manifest.json', + }); + let files; + if (opts.files === undefined) { + files = [ + new VFile({ + value: DEFAULT_SNAP_BUNDLE, + path: manifest.result.source.location.npm.filePath, + }), + ]; + + if (manifest.result.source.location.npm.iconPath !== undefined) { + files.push( + new VFile({ + value: DEFAULT_SNAP_ICON, + path: manifest.result.source.location.npm.iconPath, + }), + ); + } + } else { + files = opts.files; + assert( + files.find( + (file) => file.path === manifest.result.source.location.npm.filePath, + ) !== undefined, + 'Source bundle not found in files', + ); + + assert( + manifest.result.source.location.npm.iconPath === undefined || + files.find( + (file) => + file.path === manifest.result.source.location.npm.iconPath, + ) !== undefined, + 'Icon not found in files', + ); + } + + assert( + !files.find((file) => file.path === MANIFEST_PATH), + 'Manifest in fetch() files', + ); + assert(manifest.path === MANIFEST_PATH, 'Manifest has wrong path'); + + this.#shouldAlwaysReload = shouldAlwaysReload; + this.#manifest = manifest; + this.#files = files; + } + + /* eslint-disable @typescript-eslint/require-await */ + manifest = jest.fn(async () => this.#manifest); + + fetch = jest.fn(async (path: string) => { + const file = this.#files.find((candidate) => candidate.path === path); + assert( + file !== undefined, + `Tried to access file "${path}" not found in loopback location mock. ${this.#files + .map((candidate) => candidate.path) + .join(' ,')}`, + ); + return file; + }); + /* eslint-enable @typescript-eslint/require-await */ + + get shouldAlwaysReload() { + return this._shouldAlwaysReload(); + } + + _shouldAlwaysReload = jest.fn(() => this.#shouldAlwaysReload); +} + +export const loopbackDetect = ( + opts: LoopbackOptions | LoopbackLocation = {}, +) => { + if (opts instanceof LoopbackLocation) { + return jest.fn(() => opts); + } + return jest.fn(() => new LoopbackLocation(opts)); +}; diff --git a/packages/snaps-utils/src/index.browser.ts b/packages/snaps-utils/src/index.browser.ts index 778fbe1e00..9ccd519c41 100644 --- a/packages/snaps-utils/src/index.browser.ts +++ b/packages/snaps-utils/src/index.browser.ts @@ -11,3 +11,4 @@ export * from './snaps'; export * from './types'; export * from './url'; export * from './versions'; +export * from './vfile/vfile'; diff --git a/packages/snaps-utils/src/index.ts b/packages/snaps-utils/src/index.ts index 706aa037b0..05babce7ad 100644 --- a/packages/snaps-utils/src/index.ts +++ b/packages/snaps-utils/src/index.ts @@ -17,3 +17,5 @@ export * from './snaps'; export * from './types'; export * from './url'; export * from './versions'; +export * from './vfile/vfile'; +export * from './vfile/to-vfile'; diff --git a/packages/snaps-utils/src/snaps.ts b/packages/snaps-utils/src/snaps.ts index 4e925c7bf7..b74a4f6fa2 100644 --- a/packages/snaps-utils/src/snaps.ts +++ b/packages/snaps-utils/src/snaps.ts @@ -2,12 +2,24 @@ import { Json } from '@metamask/utils'; import { sha256 } from '@noble/hashes/sha256'; import { base64 } from '@scure/base'; import { SerializedEthereumRpcError } from 'eth-rpc-errors/dist/classes'; +import { + define, + empty, + enums, + intersection, + literal, + refine, + size, + string, + Struct, + type, + assert as assertSuperstruct, +} from 'superstruct'; import { SnapManifest, SnapPermissions } from './manifest/validation'; import { SnapId, SnapIdPrefixes, SnapValidationFailureReason } from './types'; import { SemVerVersion } from './versions'; -export const LOCALHOST_HOSTNAMES = new Set(['localhost', '127.0.0.1', '::1']); export const SNAP_PREFIX = 'wallet_snap_'; export const SNAP_PREFIX_REGEX = new RegExp(`^${SNAP_PREFIX}`, 'u'); @@ -195,6 +207,65 @@ export function validateSnapShasum( } } +export const LOCALHOST_HOSTNAMES = ['localhost', '127.0.0.1', '::1'] as const; + +type UriOptions = { + protocol?: Struct; + hash?: Struct; + port?: Struct; + hostname?: Struct; + pathname?: Struct; + search?: Struct; +}; +export const uri = (opts: UriOptions = {}) => + define('uri', (value) => { + try { + const url = new URL(value as any); + + const UrlStruct = type(opts); + assertSuperstruct(url, UrlStruct); + return true; + } catch { + return `Expected URL, got "${(value as any).toString()}"`; + } + }); + +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}"`; + } + + assertSuperstruct( + value.slice(SnapIdPrefixes.local.length), + LocalSnapIdSubUrlStruct, + ); + return true; +}); +export const NpmSnapIdStruct = intersection([ + string(), + uri({ + protocol: literal(SnapIdPrefixes.npm), + pathname: size(string(), 2, Infinity), // potentially starting with / if registry is set. + search: empty(string()), + hash: empty(string()), + }), +]) as unknown as Struct; + +export const HttpSnapIdStruct = intersection([ + string(), + uri({ + protocol: enums(['http:', 'https:']), + search: empty(string()), + hash: empty(string()), + }), +]) as unknown as Struct; + /** * Extracts the snap prefix from a snap ID. * diff --git a/packages/snaps-utils/src/test-utils/snap.ts b/packages/snaps-utils/src/test-utils/snap.ts index dcae80cca0..2203b2b90e 100644 --- a/packages/snaps-utils/src/test-utils/snap.ts +++ b/packages/snaps-utils/src/test-utils/snap.ts @@ -26,7 +26,7 @@ export const DEFAULT_SNAP_SHASUM = getSnapSourceShasum(DEFAULT_SNAP_BUNDLE); export const DEFAULT_SNAP_ICON = ''; export const MOCK_SNAP_ID = 'npm:@metamask/example-snap'; -export const MOCK_LOCAL_SNAP_ID = 'local:@metamask/example-snap'; +export const MOCK_LOCAL_SNAP_ID = 'local:http://localhost:8080'; export const MOCK_ORIGIN = 'example.com'; type GetPersistedSnapObjectOptions = Partial>; diff --git a/packages/snaps-utils/src/vfile/to-vfile.ts b/packages/snaps-utils/src/vfile/to-vfile.ts new file mode 100644 index 0000000000..a1cd11dd05 --- /dev/null +++ b/packages/snaps-utils/src/vfile/to-vfile.ts @@ -0,0 +1,61 @@ +import fs from 'fs'; +import fsPromises from 'fs/promises'; + +import { VFile } from './vfile'; + +/** + * Reads a file from filesystem and creates a vfile. + * + * @param path - Filesystem path to load the contents from. + * @param encoding - Optional encoding to pass down to fs.readFile. + * @returns Promise returning VFile with loaded file contents. + */ +export async function readVfile(path: string, encoding?: BufferEncoding) { + return new VFile({ + path, + value: await fsPromises.readFile(path, { encoding: encoding ?? null }), + }); +} + +/** + * Reads a file from filesystem and creates a vfile synchronously. + * + * @param path - Filesystem path to load the contents from. + * @param encoding - Optional encoding to pass down to fs.readFile. + * @returns VFile with loaded file contents. + */ +export function readVfileSync(path: string, encoding?: BufferEncoding) { + return new VFile({ + path, + value: fs.readFileSync(path, { encoding: encoding ?? null }), + }); +} + +type WriteVFileOptions = Exclude< + Parameters[2], + undefined +>; +type WriteVFileOptionsSync = Exclude< + Parameters[2], + undefined +>; + +/** + * Writes vfile to filesystem. + * + * @param vfile - The vfile to write. + * @param options - Options to pass down to fs.writeFile. + */ +export async function writeVFile(vfile: VFile, options?: WriteVFileOptions) { + return fsPromises.writeFile(vfile.path, vfile.value, options); +} + +/** + * Writes vfile to filesystem, synchronously. + * + * @param vfile - The vfile to write. + * @param options - Options to pass down to fs.writeFile. + */ +export function writeVFileSync(vfile: VFile, options?: WriteVFileOptionsSync) { + fs.writeFileSync(vfile.path, vfile.value, options); +} diff --git a/packages/snaps-utils/src/vfile/vfile.ts b/packages/snaps-utils/src/vfile/vfile.ts new file mode 100644 index 0000000000..a2d65e6cf6 --- /dev/null +++ b/packages/snaps-utils/src/vfile/vfile.ts @@ -0,0 +1,103 @@ +// TODO(ritave): Move into separate package @metamask/vfile / @metamask/utils + @metamask/to-vfile when passes code review +// TODO(ritave): Streaming vfile contents similar to vinyl maybe? +// TODO(ritave): Move fixing manifest to write messages to vfile similar to unified instead of throwing "ProgrammaticallyFixableErrors". +// Better yet, move manifest fixing into eslint fixing altogether +import { assert, hasProperty } from '@metamask/utils'; +import { instance, integer, is, size, type } from 'superstruct'; + +// Using https://github.com/vfile/vfile would be helpful, but they only support ESM. +// https://github.com/gulpjs/vinyl is also good, but they normalize paths, which we can't do, because +// we're calculating checksums based on original path. + +/** + * This map registers the type of the `data` key of a `VFile`. + * + * This type can be augmented to register custom `data` types. + * + * @example + * declare module '@metamask/vfile' { + * interface DataMap { + * // `file.data.name` is typed as `string` + * name: string + * } + * } + */ +// eslint-disable-next-line @typescript-eslint/consistent-type-definitions, @typescript-eslint/no-empty-interface +export interface DataMap {} + +export type Value = string | Uint8Array; +export type Compatible = + | string + | Uint8Array + | Options; +export type Data = Record & Partial; +export type Options = { + value: Value; + path?: string; + data?: Data; + result?: Result; +}; +export type TypedArray = + | Int8Array + | Uint8Array + | Uint8ClampedArray + | Int16Array + | Uint16Array + | Int32Array + | Uint32Array + | Float32Array + | Float64Array + | BigInt64Array + | BigUint64Array; + +const TypedArrayStruct = type({ + buffer: instance(ArrayBuffer), + BYTES_PER_ELEMENT: size(integer(), 1, Infinity), +}); + +/** + * Returns whether the given parameter is one of TypedArray subtypes. + * + * @param obj - Object to check. + * @returns Whether the parameter is TypeArray subtype. + */ +export function isTypedArray(obj: unknown): obj is TypedArray { + return is(obj, TypedArrayStruct); +} + +export class VFile { + constructor(value?: Compatible) { + let options: Options; + if (typeof value === 'string' || isTypedArray(value)) { + options = { value }; + } else { + options = value as Options; + } + + for (const prop in options) { + if ( + hasProperty(options, prop) && + options[prop as keyof Options] !== undefined + ) { + this[prop as keyof Options] = options[prop as keyof Options] as any; + } + } + } + + value!: Value; + + result!: Result; + + data: Data = {}; + + path = '/'; + + toString(encoding?: string) { + if (typeof this.value === 'string') { + assert(encoding === undefined, 'Tried to encode string.'); + return this.value; + } + const decoder = new TextDecoder(encoding); + return decoder.decode(this.value); + } +} From 854d8ce3b938bedc2ae3eb0ed00f5dea7aefda5c Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Mon, 28 Nov 2022 20:17:35 +0100 Subject: [PATCH 03/15] fetchSnaps is now private --- packages/snaps-controllers/src/snaps/SnapController.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 13789ee434..2b5e405bbf 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -178,7 +178,7 @@ export type SnapError = { }; /** - * The return type of {@link SnapController.fetchSnap} and its sibling methods. + * The return type of {@link SnapController.#fetchSnap} and its sibling methods. */ type FetchSnapResult = { /** @@ -1716,7 +1716,7 @@ export class SnapController extends BaseController< `Received invalid snap version range: "${newVersionRange}".`, ); } - const newSnap = await this.fetchSnap( + const newSnap = await this.#fetchSnap( snapId, location ?? this.#detectSnapLocation(snapId, { versionRange: newVersionRange }), @@ -1838,7 +1838,7 @@ export class SnapController extends BaseController< // If fetching and setting the snap succeeds, this property will be set // to null in the authorize() method. runtime.installPromise = (async () => { - const fetchedSnap = await this.fetchSnap(snapId, location); + const fetchedSnap = await this.#fetchSnap(snapId, location); await this.#assertIsUnblocked(snapId, { version: fetchedSnap.manifest.result.version, shasum: fetchedSnap.manifest.result.source.shasum, @@ -2051,7 +2051,7 @@ export class SnapController extends BaseController< * @param location - Source from which snap will be fetched. * @returns A tuple of the Snap manifest object and the Snap source code. */ - async fetchSnap( + async #fetchSnap( snapId: ValidatedSnapId, location: SnapLocation, ): Promise { From e9d7f2dd638962403969ce44cc15d4db22c5abee Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Tue, 29 Nov 2022 19:52:52 +0100 Subject: [PATCH 04/15] PR review + more test coverage --- .../src/snaps/SnapController.test.ts | 14 +--- .../src/snaps/SnapController.ts | 8 +- .../src/snaps/location/http.test.ts | 29 +++++++ .../src/snaps/location/http.ts | 35 +++++--- .../src/snaps/location/local.ts | 4 +- .../src/snaps/location/location.test.ts | 29 +++++++ .../src/snaps/location/location.ts | 4 +- .../src/snaps/location/npm.test.ts | 22 ++++- .../src/snaps/location/npm.ts | 81 ++++++++++--------- .../src/test-utils/location.ts | 12 +-- packages/snaps-controllers/src/utils.ts | 16 ++++ packages/snaps-utils/src/index.browser.ts | 2 +- packages/snaps-utils/src/index.ts | 3 +- packages/snaps-utils/src/snaps.ts | 13 +-- .../snaps-utils/src/vfile/index.browser.ts | 1 + packages/snaps-utils/src/vfile/index.ts | 2 + packages/snaps-utils/src/vfile/to-vfile.ts | 14 +++- packages/snaps-utils/src/vfile/vfile.ts | 55 ++++++++----- 18 files changed, 231 insertions(+), 113 deletions(-) create mode 100644 packages/snaps-controllers/src/snaps/location/http.test.ts create mode 100644 packages/snaps-utils/src/vfile/index.browser.ts create mode 100644 packages/snaps-utils/src/vfile/index.ts diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 1723a18328..c3315a7f9d 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -29,7 +29,6 @@ import { } from '@metamask/snaps-utils/test-utils'; import { AssertionError } from '@metamask/utils'; import { Crypto } from '@peculiar/webcrypto'; -import { assert } from 'console'; import { EthereumRpcError, ethErrors, serializeError } from 'eth-rpc-errors'; import fetchMock from 'jest-fetch-mock'; import { createAsyncMiddleware, JsonRpcEngine } from 'json-rpc-engine'; @@ -1761,15 +1760,13 @@ describe('SnapController', () => { }); const location = new LoopbackLocation({ shouldAlwaysReload: true }); - /* eslint-disable @typescript-eslint/require-await */ location.manifest - .mockImplementationOnce( - async () => new VFile({ value: '', result: manifest }), + .mockImplementationOnce(async () => + Promise.resolve(new VFile({ value: '', result: manifest })), ) - .mockImplementationOnce( - async () => new VFile({ value: '', result: newManifest }), + .mockImplementationOnce(async () => + Promise.resolve(new VFile({ value: '', result: newManifest })), ); - /* eslint-enable @typescript-eslint/require-await */ const snapController = getSnapController( getSnapControllerOptions({ @@ -2275,7 +2272,6 @@ describe('SnapController', () => { const result = await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: { version: newVersionRange }, }); - assert(false, new Error(JSON.stringify(result))); expect(messenger.call).toHaveBeenCalledTimes(12); expect(messenger.call).toHaveBeenNthCalledWith( @@ -2841,7 +2837,6 @@ describe('SnapController', () => { const callActionSpy = jest.spyOn(messenger, 'call'); - /* eslint-disable @typescript-eslint/require-await */ const detect = jest .fn() .mockImplementationOnce( @@ -2862,7 +2857,6 @@ describe('SnapController', () => { }), }), ); - /* eslint-enable @typescript-eslint/require-await */ const controller = getSnapController( getSnapControllerOptions({ messenger, detectSnapLocation: detect }), diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 2b5e405bbf..d661e23541 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1721,9 +1721,6 @@ export class SnapController extends BaseController< location ?? this.#detectSnapLocation(snapId, { versionRange: newVersionRange }), ); - if (newSnap.manifest.result === undefined) { - throw new Error(JSON.stringify(newSnap.manifest)); - } const newVersion = newSnap.manifest.result.version; if (!gtVersion(newVersion, snap.version)) { console.warn( @@ -2070,9 +2067,8 @@ export class SnapController extends BaseController< return { manifest, files, location }; } catch (error) { - throw new Error( - `Failed to fetch Snap "${snapId}": ${(error as Error).message}`, - ); + const message = error instanceof Error ? error.message : error.toString(); + throw new Error(`Failed to fetch Snap "${snapId}": ${message}.`); } } diff --git a/packages/snaps-controllers/src/snaps/location/http.test.ts b/packages/snaps-controllers/src/snaps/location/http.test.ts new file mode 100644 index 0000000000..0cfaaaab6d --- /dev/null +++ b/packages/snaps-controllers/src/snaps/location/http.test.ts @@ -0,0 +1,29 @@ +import { + DEFAULT_SNAP_BUNDLE, + getSnapManifest, +} from '@metamask/snaps-utils/test-utils'; +import fetchMock from 'jest-fetch-mock'; + +import HttpLocation from './http'; + +fetchMock.enableMocks(); + +describe('HttpLocation', () => { + it('loads the files', async () => { + const base = 'http://foo.bar/foo/'; + const manifest = getSnapManifest(); + const manifestStr = JSON.stringify(manifest); + + fetchMock.mockResponses(manifestStr, DEFAULT_SNAP_BUNDLE); + const location = new HttpLocation(new URL(base)); + + expect(await location.manifest()).toStrictEqual( + expect.objectContaining({ value: manifestStr, result: manifest }), + ); + + const bundle = await location.fetch( + new URL(manifest.source.location.npm.filePath, base).toString(), + ); + expect(bundle.toString()).toStrictEqual(DEFAULT_SNAP_BUNDLE); + }); +}); diff --git a/packages/snaps-controllers/src/snaps/location/http.ts b/packages/snaps-controllers/src/snaps/location/http.ts index 50a21d7ce2..fffcd2636f 100644 --- a/packages/snaps-controllers/src/snaps/location/http.ts +++ b/packages/snaps-controllers/src/snaps/location/http.ts @@ -2,11 +2,11 @@ import { SnapManifest, assertIsSnapManifest, VFile, - deepClone, HttpSnapIdStruct, } from '@metamask/snaps-utils'; import { assert, assertStruct } from '@metamask/utils'; +import { ensureRelative } from '../../utils'; import { SnapLocation } from './location'; export default class HttpLocation implements SnapLocation { @@ -16,7 +16,7 @@ export default class HttpLocation implements SnapLocation { // That avoids deepCloning file contents. // I imagine ArrayBuffers are copy-on-write optimized, meaning // in most often case we'll only have one file contents in common case. - private readonly cache: [file: VFile, contents: Blob][] = []; + private readonly cache = new Map(); private validatedManifest?: VFile; @@ -29,10 +29,11 @@ export default class HttpLocation implements SnapLocation { async manifest(): Promise> { if (this.validatedManifest) { - return deepClone(this.validatedManifest); + return this.validatedManifest.clone(); } - const contents = await (await fetch(this.url)).text(); + // jest-fetch-mock doesn't handle new URL(), we need to convert this.url.toString() + const contents = await (await fetch(this.url.toString())).text(); const manifest = JSON.parse(contents); assertIsSnapManifest(manifest); const vfile = new VFile({ @@ -47,23 +48,31 @@ export default class HttpLocation implements SnapLocation { } async fetch(path: string): Promise { - const cached = this.cache.find(([file]) => file.path === path); + const relativePath = ensureRelative(path); + const cached = this.cache.get(relativePath); if (cached !== undefined) { - const [cachedFile, contents] = cached; + const { file, contents } = cached; const value = new Uint8Array(await contents.arrayBuffer()); - const vfile = deepClone(cachedFile); + const vfile = file.clone(); vfile.value = value; return vfile; } - const canonicalPath = this.toCanonical(path).toString(); + const canonicalPath = this.toCanonical(relativePath).toString(); const response = await fetch(canonicalPath); - const vfile = new VFile({ value: '', path, data: { canonicalPath } }); - // TODO(ritave): When can we assume the file is string vs binary? + const vfile = new VFile({ + value: '', + path: relativePath, + data: { canonicalPath }, + }); const blob = await response.blob(); - this.cache.push([vfile, blob]); + assert( + !this.cache.has(relativePath), + 'Corrupted cache, multiple files with same path.', + ); + this.cache.set(relativePath, { file: vfile, contents: blob }); - return this.fetch(path); + return this.fetch(relativePath); } get root(): URL { @@ -71,7 +80,7 @@ export default class HttpLocation implements SnapLocation { } private toCanonical(path: string): URL { - assert(!path.startsWith('/'), 'Tried to parse absolute path'); + assert(!path.startsWith('/'), 'Tried to parse absolute path.'); return new URL(path, this.url); } } diff --git a/packages/snaps-controllers/src/snaps/location/local.ts b/packages/snaps-controllers/src/snaps/location/local.ts index e04d2f4087..0ec07f116c 100644 --- a/packages/snaps-controllers/src/snaps/location/local.ts +++ b/packages/snaps-controllers/src/snaps/location/local.ts @@ -13,9 +13,9 @@ export default class LocalLocation implements SnapLocation { readonly #http: HttpLocation; constructor(url: URL) { - assertStruct(url.toString(), LocalSnapIdStruct, 'Invalid Snap Id: '); + assertStruct(url.toString(), LocalSnapIdStruct, 'Invalid Snap Id'); this.#http = new HttpLocation( - new URL(url.toString().slice(SnapIdPrefixes.local.length)), + new URL(`https://${url.toString().slice(SnapIdPrefixes.local.length)}`), ); } diff --git a/packages/snaps-controllers/src/snaps/location/location.test.ts b/packages/snaps-controllers/src/snaps/location/location.test.ts index dc697fe7bc..5da4a7ad21 100644 --- a/packages/snaps-controllers/src/snaps/location/location.test.ts +++ b/packages/snaps-controllers/src/snaps/location/location.test.ts @@ -1,4 +1,7 @@ +import HttpLocation from './http'; +import LocalLocation from './local'; import { detectSnapLocation } from './location'; +import NpmLocation from './npm'; describe('detectSnapLocation', () => { it.each(['http:', 'https:'])( @@ -9,4 +12,30 @@ describe('detectSnapLocation', () => { ); }, ); + + it('disallows custom registries by default', () => { + expect(() => detectSnapLocation('npm://foo.com/bar')).toThrow( + 'Custom NPM registries are disabled, tried to use "https://foo.com/".', + ); + }); + + it.each([ + ['npm:', NpmLocation], + ['local:', LocalLocation], + ['http:', HttpLocation], + ['https:', HttpLocation], + ])('detects %s protocol', (protocol, classObj) => { + expect( + detectSnapLocation(`${protocol}localhost/foo`, { + allowHttp: true, + allowCustomRegistries: true, + }), + ).toBeInstanceOf(classObj); + }); + + it('throws on unrecognized protocol', () => { + expect(() => detectSnapLocation('foo://bar.com/asd')).toThrow( + `Unrecognized "foo:" snap location protocol.`, + ); + }); }); diff --git a/packages/snaps-controllers/src/snaps/location/location.ts b/packages/snaps-controllers/src/snaps/location/location.ts index 7bcce34d3e..249c5cf851 100644 --- a/packages/snaps-controllers/src/snaps/location/location.ts +++ b/packages/snaps-controllers/src/snaps/location/location.ts @@ -1,5 +1,5 @@ import { SnapManifest, VFile } from '@metamask/snaps-utils'; -import assert from 'assert'; +import { assert } from '@metamask/utils'; import HttpLocation from './http'; import LocalLocation from './local'; @@ -58,7 +58,7 @@ export function detectSnapLocation( return new HttpLocation(root); default: throw new TypeError( - `Unrecognized "${root.protocol}" snap location protocol`, + `Unrecognized "${root.protocol}" snap location protocol.`, ); } } diff --git a/packages/snaps-controllers/src/snaps/location/npm.test.ts b/packages/snaps-controllers/src/snaps/location/npm.test.ts index 67edf42e37..5d4f27dcf3 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.test.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.test.ts @@ -11,7 +11,7 @@ describe('NpmLocation', () => { fetchMock.resetMocks(); }); - it('fetches a package tarball, extracts the neecessary files, and validaes them', async () => { + it('fetches a package tarball, extracts the necessary files, and validates them', async () => { const { version: templateSnapVersion } = JSON.parse( ( await readFile(require.resolve('@metamask/template-snap/package.json')) @@ -108,4 +108,24 @@ describe('NpmLocation', () => { 'Custom NPM registries are disabled, tried to use "https://registry.npmjs.cf/"', ); }); + + it.each(['foo:bar@registry.com', 'foo@registry.com'])( + 'supports registries with usernames and passwords', + (host) => { + const location = new NpmLocation(new URL(`npm://${host}/snap`), { + allowCustomRegistries: true, + }); + expect(location.registry.toString()).toBe(`https://${host}/`); + }, + ); + + it('has meta properties', () => { + const location = new NpmLocation(new URL('npm:foo')); + expect(location.packageName).toBe('foo'); + expect(location.registry.toString()).toBe('https://registry.npmjs.org/'); + expect(location.versionRange).toBe('*'); + expect(() => location.version).toThrow( + 'Tried to access version without first fetching NPM package.', + ); + }); }); diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts index 0ec0810b76..c71f875043 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -1,7 +1,6 @@ import { assertIsSemVerVersion, assertIsSnapManifest, - deepClone, DEFAULT_REQUESTED_SNAP_VERSION, getTargetVersion, isValidUrl, @@ -19,6 +18,7 @@ import { ReadableWebToNodeStream } from 'readable-web-to-node-stream'; import { Readable, Writable } from 'stream'; import { extract as tarExtract } from 'tar-stream'; +import { ensureRelative } from '../../utils'; import { SnapLocation } from './location'; const DEFAULT_NPM_REGISTRY = 'https://registry.npmjs.org'; @@ -54,12 +54,11 @@ export default class NpmLocation implements SnapLocation { private validatedManifest?: VFile; - private files?: VFile[]; + private files?: Map; constructor(url: URL, opts: NpmOptions = {}) { const allowCustomRegistries = opts.allowCustomRegistries ?? false; - // eslint-disable-next-line @typescript-eslint/no-shadow - const fetch = opts.fetch ?? globalThis.fetch; + const fetchFunction = opts.fetch ?? globalThis.fetch; const requestedRange = opts.versionRange ?? DEFAULT_REQUESTED_SNAP_VERSION; assertStruct(url.toString(), NpmSnapIdStruct, 'Invalid Snap Id: '); @@ -86,7 +85,7 @@ export default class NpmLocation implements SnapLocation { assert( allowCustomRegistries, new TypeError( - `Custom NPM registries are disabled, tried to use "${registry.toString()}"`, + `Custom NPM registries are disabled, tried to use "${registry.toString()}".`, ), ); } @@ -99,7 +98,7 @@ export default class NpmLocation implements SnapLocation { assert( url.pathname !== '' && url.pathname !== '/', - new TypeError('The package name in NPM location is empty'), + new TypeError('The package name in NPM location is empty.'), ); let packageName = url.pathname; if (packageName.startsWith('/')) { @@ -110,17 +109,17 @@ export default class NpmLocation implements SnapLocation { requestedRange, registry, packageName, - fetch, + fetch: fetchFunction, }; } async manifest(): Promise> { if (this.validatedManifest) { - return deepClone(this.validatedManifest); + return this.validatedManifest.clone(); } const vfile = await this.fetch('./snap.manifest.json'); - const result = JSON.parse(vfile.value.toString()); + const result = JSON.parse(vfile.toString()); assertIsSnapManifest(result); vfile.result = result; this.validatedManifest = vfile as VFile; @@ -128,20 +127,19 @@ export default class NpmLocation implements SnapLocation { return this.manifest(); } - // eslint-disable-next-line @typescript-eslint/require-await async fetch(path: string): Promise { if (!this.files) { await this.#lazyInit(); assert(this.files !== undefined); } - assert(!path.startsWith('/'), 'Tried to fetch absolute path'); + assert(!path.startsWith('/'), 'Tried to fetch absolute path.'); const relativePath = ensureRelative(path); - const vfile = this.files.find((file) => file.path === relativePath); + const vfile = this.files.get(relativePath); assert( vfile !== undefined, - new TypeError(`File "${path}" not found in package`), + new TypeError(`File "${path}" not found in package.`), ); - return deepClone(vfile); + return vfile.clone(); } get packageName(): string { @@ -151,11 +149,19 @@ export default class NpmLocation implements SnapLocation { get version(): string { assert( this.meta.version !== undefined, - 'Tried to access version without first fetching NPM package', + 'Tried to access version without first fetching NPM package.', ); return this.meta.version; } + get registry(): URL { + return this.meta.registry; + } + + get versionRange(): SemVerRange { + return this.meta.requestedRange; + } + async #lazyInit() { assert(this.files === undefined); const [tarballResponse, actualVersion] = await fetchNpmTarball( @@ -179,7 +185,7 @@ export default class NpmLocation implements SnapLocation { // TODO(ritave): Lazily extract files instead of up-front extracting all of them // We would need to replace tar-stream package because it requires immediate consumption of streams. await new Promise((resolve, reject) => { - this.files = []; + this.files = new Map(); pump( getNodeStream(tarballResponse), // The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed @@ -237,16 +243,16 @@ async function fetchNpmTarball( if (targetVersion === null) { throw new Error( - `Failed to find a matching version in npm metadata for package "${packageName}" and requested semver range "${versionRange}"`, + `Failed to find a matching version in npm metadata for package "${packageName}" and requested semver range "${versionRange}".`, ); } - const tarballUrlString = (packageMetadata as any).versions?.[targetVersion] + const tarballUrlString = (packageMetadata as any)?.versions?.[targetVersion] ?.dist?.tarball; if (!isValidUrl(tarballUrlString) || !tarballUrlString.endsWith('.tgz')) { throw new Error( - `Failed to find valid tarball URL in npm metadata for package "${packageName}".`, + `Failed to find valid tarball URL in NPM metadata for package "${packageName}".`, ); } @@ -258,12 +264,10 @@ async function fetchNpmTarball( // Perform a raw fetch because we want the Response object itself. const tarballResponse = await fetchFunction(newTarballUrl.toString()); - if (!tarballResponse.ok) { + if (!tarballResponse.ok || !tarballResponse.body) { throw new Error(`Failed to fetch tarball for package "${packageName}".`); } - const stream = await tarballResponse.blob().then((blob) => blob.stream()); - - return [stream, targetVersion]; + return [tarballResponse.body, targetVersion]; } /** @@ -297,12 +301,19 @@ function getNodeStream(stream: ReadableStream): Readable { * @param files - An object to write target file contents to. * @returns The {@link Writable} tarball extraction stream. */ -function createTarballStream(canonicalBase: string, files: VFile[]): Writable { +function createTarballStream( + canonicalBase: string, + files: Map, +): Writable { assert( canonicalBase.endsWith('/'), "Base needs to end with '/' for relative paths to be added as children instead of siblings.", ); - assert(canonicalBase.startsWith('npm:'), 'Protocol mismatch, expected "npm:'); + + assert( + canonicalBase.startsWith('npm:'), + 'Protocol mismatch, expected "npm:".', + ); // `tar-stream` is pretty old-school, so we create it first and then // instrument it by adding event listeners. const extractStream = tarExtract(); @@ -323,7 +334,11 @@ function createTarballStream(canonicalBase: string, files: VFile[]): Writable { canonicalPath: new URL(path, canonicalBase).toString(), }, }); - files.push(vfile); + assert( + !files.has(path), + 'Malformed tarball, multiple files with the same path.', + ); + files.set(path, vfile); return next(); }), ); @@ -337,17 +352,3 @@ function createTarballStream(canonicalBase: string, files: VFile[]): Writable { }); return extractStream; } - -/** - * Ensures that a relative path starts with `./` prefix. - * - * @param path - Path to make relative. - * @returns The same path, with optional `./` prefix. - */ -function ensureRelative(path: string): string { - assert(!path.startsWith('/')); - if (path.startsWith('./')) { - return path; - } - return `./${path}`; -} diff --git a/packages/snaps-controllers/src/test-utils/location.ts b/packages/snaps-controllers/src/test-utils/location.ts index d306f52781..1e6abadd3a 100644 --- a/packages/snaps-controllers/src/test-utils/location.ts +++ b/packages/snaps-controllers/src/test-utils/location.ts @@ -71,7 +71,7 @@ export class LoopbackLocation implements SnapLocation { files.find( (file) => file.path === manifest.result.source.location.npm.filePath, ) !== undefined, - 'Source bundle not found in files', + 'Source bundle not found in files.', ); assert( @@ -80,7 +80,7 @@ export class LoopbackLocation implements SnapLocation { (file) => file.path === manifest.result.source.location.npm.iconPath, ) !== undefined, - 'Icon not found in files', + 'Icon not found in files.', ); } @@ -88,7 +88,7 @@ export class LoopbackLocation implements SnapLocation { !files.find((file) => file.path === MANIFEST_PATH), 'Manifest in fetch() files', ); - assert(manifest.path === MANIFEST_PATH, 'Manifest has wrong path'); + assert(manifest.path === MANIFEST_PATH, 'Manifest has wrong path.'); this.#shouldAlwaysReload = shouldAlwaysReload; this.#manifest = manifest; @@ -102,9 +102,9 @@ export class LoopbackLocation implements SnapLocation { const file = this.#files.find((candidate) => candidate.path === path); assert( file !== undefined, - `Tried to access file "${path}" not found in loopback location mock. ${this.#files - .map((candidate) => candidate.path) - .join(' ,')}`, + `Tried to access file "${path}" not found in loopback location mock.\nFile list:\n${this.#files + .map((candidate) => `\t-> ${candidate.path}`) + .join('\n')}`, ); return file; }); diff --git a/packages/snaps-controllers/src/utils.ts b/packages/snaps-controllers/src/utils.ts index 35ee193d04..af1fac2b56 100644 --- a/packages/snaps-controllers/src/utils.ts +++ b/packages/snaps-controllers/src/utils.ts @@ -1,3 +1,5 @@ +import { assert } from '@metamask/utils'; + import { Timer } from './snaps/Timer'; /** @@ -198,3 +200,17 @@ export type Mutable< } & { [Key in keyof Omit]: T[Key]; }; + +/** + * Ensures that a relative path starts with `./` prefix. + * + * @param path - Path to make relative. + * @returns The same path, with optional `./` prefix. + */ +export function ensureRelative(path: string): string { + assert(!path.startsWith('/')); + if (path.startsWith('./')) { + return path; + } + return `./${path}`; +} diff --git a/packages/snaps-utils/src/index.browser.ts b/packages/snaps-utils/src/index.browser.ts index 9ccd519c41..69ff2fde51 100644 --- a/packages/snaps-utils/src/index.browser.ts +++ b/packages/snaps-utils/src/index.browser.ts @@ -11,4 +11,4 @@ export * from './snaps'; export * from './types'; export * from './url'; export * from './versions'; -export * from './vfile/vfile'; +export * from './vfile/index.browser'; diff --git a/packages/snaps-utils/src/index.ts b/packages/snaps-utils/src/index.ts index 05babce7ad..523d76cf6d 100644 --- a/packages/snaps-utils/src/index.ts +++ b/packages/snaps-utils/src/index.ts @@ -17,5 +17,4 @@ export * from './snaps'; export * from './types'; export * from './url'; export * from './versions'; -export * from './vfile/vfile'; -export * from './vfile/to-vfile'; +export * from './vfile'; diff --git a/packages/snaps-utils/src/snaps.ts b/packages/snaps-utils/src/snaps.ts index b74a4f6fa2..9709191676 100644 --- a/packages/snaps-utils/src/snaps.ts +++ b/packages/snaps-utils/src/snaps.ts @@ -3,7 +3,6 @@ import { sha256 } from '@noble/hashes/sha256'; import { base64 } from '@scure/base'; import { SerializedEthereumRpcError } from 'eth-rpc-errors/dist/classes'; import { - define, empty, enums, intersection, @@ -14,6 +13,8 @@ import { Struct, type, assert as assertSuperstruct, + union, + instance, } from 'superstruct'; import { SnapManifest, SnapPermissions } from './manifest/validation'; @@ -218,15 +219,15 @@ type UriOptions = { search?: Struct; }; export const uri = (opts: UriOptions = {}) => - define('uri', (value) => { + refine(union([string(), instance(URL)]), 'uri', (value) => { try { - const url = new URL(value as any); + const url = new URL(value); const UrlStruct = type(opts); assertSuperstruct(url, UrlStruct); return true; } catch { - return `Expected URL, got "${(value as any).toString()}"`; + return `Expected URL, got "${value.toString()}".`; } }); @@ -238,11 +239,11 @@ const LocalSnapIdSubUrlStruct = uri({ }); export const LocalSnapIdStruct = refine(string(), 'local Snap Id', (value) => { if (!value.startsWith(SnapIdPrefixes.local)) { - return `Expected local Snap Id, got "${value}"`; + return `Expected local Snap ID, got "${value}".`; } assertSuperstruct( - value.slice(SnapIdPrefixes.local.length), + `https://${value.slice(SnapIdPrefixes.local.length)}`, LocalSnapIdSubUrlStruct, ); return true; diff --git a/packages/snaps-utils/src/vfile/index.browser.ts b/packages/snaps-utils/src/vfile/index.browser.ts new file mode 100644 index 0000000000..0550ddf949 --- /dev/null +++ b/packages/snaps-utils/src/vfile/index.browser.ts @@ -0,0 +1 @@ +export * from './vfile'; diff --git a/packages/snaps-utils/src/vfile/index.ts b/packages/snaps-utils/src/vfile/index.ts new file mode 100644 index 0000000000..ed9878be00 --- /dev/null +++ b/packages/snaps-utils/src/vfile/index.ts @@ -0,0 +1,2 @@ +export * from './to-vfile'; +export * from './vfile'; diff --git a/packages/snaps-utils/src/vfile/to-vfile.ts b/packages/snaps-utils/src/vfile/to-vfile.ts index a1cd11dd05..9996e714c3 100644 --- a/packages/snaps-utils/src/vfile/to-vfile.ts +++ b/packages/snaps-utils/src/vfile/to-vfile.ts @@ -10,10 +10,13 @@ import { VFile } from './vfile'; * @param encoding - Optional encoding to pass down to fs.readFile. * @returns Promise returning VFile with loaded file contents. */ -export async function readVfile(path: string, encoding?: BufferEncoding) { +export async function readVfile( + path: string, + encoding: BufferEncoding | null = null, +) { return new VFile({ path, - value: await fsPromises.readFile(path, { encoding: encoding ?? null }), + value: await fsPromises.readFile(path, { encoding }), }); } @@ -24,10 +27,13 @@ export async function readVfile(path: string, encoding?: BufferEncoding) { * @param encoding - Optional encoding to pass down to fs.readFile. * @returns VFile with loaded file contents. */ -export function readVfileSync(path: string, encoding?: BufferEncoding) { +export function readVfileSync( + path: string, + encoding: BufferEncoding | null = null, +) { return new VFile({ path, - value: fs.readFileSync(path, { encoding: encoding ?? null }), + value: fs.readFileSync(path, { encoding }), }); } diff --git a/packages/snaps-utils/src/vfile/vfile.ts b/packages/snaps-utils/src/vfile/vfile.ts index a2d65e6cf6..af1d5a7a75 100644 --- a/packages/snaps-utils/src/vfile/vfile.ts +++ b/packages/snaps-utils/src/vfile/vfile.ts @@ -3,7 +3,9 @@ // TODO(ritave): Move fixing manifest to write messages to vfile similar to unified instead of throwing "ProgrammaticallyFixableErrors". // Better yet, move manifest fixing into eslint fixing altogether import { assert, hasProperty } from '@metamask/utils'; -import { instance, integer, is, size, type } from 'superstruct'; +import { Infer, instance, is, union } from 'superstruct'; + +import { deepClone } from '../deep-clone'; // Using https://github.com/vfile/vfile would be helpful, but they only support ESM. // https://github.com/gulpjs/vinyl is also good, but they normalize paths, which we can't do, because @@ -37,32 +39,31 @@ export type Options = { data?: Data; result?: Result; }; -export type TypedArray = - | Int8Array - | Uint8Array - | Uint8ClampedArray - | Int16Array - | Uint16Array - | Int32Array - | Uint32Array - | Float32Array - | Float64Array - | BigInt64Array - | BigUint64Array; -const TypedArrayStruct = type({ - buffer: instance(ArrayBuffer), - BYTES_PER_ELEMENT: size(integer(), 1, Infinity), -}); +const TypedArrayStruct = union([ + instance(Int8Array), + instance(Uint8Array), + instance(Uint8ClampedArray), + instance(Int16Array), + instance(Uint16Array), + instance(Int32Array), + instance(Uint32Array), + instance(Float32Array), + instance(Float64Array), + instance(BigInt64Array), + instance(BigUint64Array), +]); + +export type TypedArray = Infer; /** * Returns whether the given parameter is one of TypedArray subtypes. * - * @param obj - Object to check. + * @param value - Object to check. * @returns Whether the parameter is TypeArray subtype. */ -export function isTypedArray(obj: unknown): obj is TypedArray { - return is(obj, TypedArrayStruct); +export function isTypedArray(value: unknown): value is TypedArray { + return is(value, TypedArrayStruct); } export class VFile { @@ -100,4 +101,18 @@ export class VFile { const decoder = new TextDecoder(encoding); return decoder.decode(this.value); } + + clone() { + const vfile = new VFile(); + if (typeof this.value === 'string') { + vfile.value = this.value; + } else { + // deep-clone doesn't clone Buffer properly, even if it's a sub-class of Uint8Array + vfile.value = this.value.slice(0); + } + vfile.result = deepClone(this.result); + vfile.data = deepClone(this.data); + vfile.path = this.path; + return vfile; + } } From a2dcbaa944167f258766b75c2e2fdccca65cf763 Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Wed, 30 Nov 2022 02:12:02 +0100 Subject: [PATCH 05/15] snaps-utils tests coverage --- .../src/snaps/location/npm.ts | 5 +- packages/snaps-utils/jest.config.js | 12 ++-- packages/snaps-utils/package.json | 2 +- packages/snaps-utils/src/__mocks__/fs.ts | 6 +- .../snaps-utils/src/__mocks__/fs/promises.ts | 3 + packages/snaps-utils/src/index.browser.ts | 1 - packages/snaps-utils/src/index.ts | 1 - packages/snaps-utils/src/snaps.ts | 31 ++------ packages/snaps-utils/src/types.test.ts | 45 +++++++++++- packages/snaps-utils/src/types.ts | 40 +++++++++++ packages/snaps-utils/src/url.ts | 13 ---- .../snaps-utils/src/vfile/to-vfile.test.ts | 53 ++++++++++++++ packages/snaps-utils/src/vfile/to-vfile.ts | 4 +- packages/snaps-utils/src/vfile/vfile.test.ts | 72 +++++++++++++++++++ yarn.lock | 10 +-- 15 files changed, 241 insertions(+), 57 deletions(-) create mode 100644 packages/snaps-utils/src/__mocks__/fs/promises.ts delete mode 100644 packages/snaps-utils/src/url.ts create mode 100644 packages/snaps-utils/src/vfile/to-vfile.test.ts create mode 100644 packages/snaps-utils/src/vfile/vfile.test.ts diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts index c71f875043..c916ce3969 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -250,7 +250,10 @@ async function fetchNpmTarball( const tarballUrlString = (packageMetadata as any)?.versions?.[targetVersion] ?.dist?.tarball; - if (!isValidUrl(tarballUrlString) || !tarballUrlString.endsWith('.tgz')) { + if ( + !isValidUrl(tarballUrlString) || + !tarballUrlString.toString().endsWith('.tgz') + ) { throw new Error( `Failed to find valid tarball URL in NPM metadata for package "${packageName}".`, ); diff --git a/packages/snaps-utils/jest.config.js b/packages/snaps-utils/jest.config.js index 505f7368f6..e0957e96ff 100644 --- a/packages/snaps-utils/jest.config.js +++ b/packages/snaps-utils/jest.config.js @@ -6,6 +6,10 @@ module.exports = deepmerge(baseConfig, { coveragePathIgnorePatterns: [ './src/index.ts', './src/index.browser.ts', + './src/vfile/index.ts', + './src/vfile/index.browser.ts', + './src/manifest/index.ts', + './src/manifest/index.browser.ts', './src/test-utils', './src/json-schemas', // Jest currently doesn't collect coverage for child processes. @@ -14,10 +18,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 93.49, - functions: 95.45, - lines: 98.34, - statements: 98.34, + branches: 94.76, + functions: 98.95, + lines: 98.74, + statements: 98.74, }, }, testTimeout: 2500, diff --git a/packages/snaps-utils/package.json b/packages/snaps-utils/package.json index 9975f638ef..8ca0cbf1ea 100644 --- a/packages/snaps-utils/package.json +++ b/packages/snaps-utils/package.json @@ -85,7 +85,7 @@ "eslint-plugin-prettier": "^4.2.1", "jest": "^29.0.2", "jest-it-up": "^2.0.0", - "memfs": "^3.4.7", + "memfs": "^3.4.12", "prettier": "^2.7.1", "prettier-plugin-packagejson": "^2.2.11", "rimraf": "^3.0.2", diff --git a/packages/snaps-utils/src/__mocks__/fs.ts b/packages/snaps-utils/src/__mocks__/fs.ts index 6d845ba096..2882a4e4de 100644 --- a/packages/snaps-utils/src/__mocks__/fs.ts +++ b/packages/snaps-utils/src/__mocks__/fs.ts @@ -1,4 +1,4 @@ -import { Volume } from 'memfs'; +import { fs } from 'memfs'; -// Note: `Volume` implements most of the `fs` API, but not all. -export = new Volume(); +// Note: 'memfs' implements most of the `fs` API, but not all. +export = fs; diff --git a/packages/snaps-utils/src/__mocks__/fs/promises.ts b/packages/snaps-utils/src/__mocks__/fs/promises.ts new file mode 100644 index 0000000000..e2fea38edc --- /dev/null +++ b/packages/snaps-utils/src/__mocks__/fs/promises.ts @@ -0,0 +1,3 @@ +import { fs } from 'memfs'; + +export = fs.promises; diff --git a/packages/snaps-utils/src/index.browser.ts b/packages/snaps-utils/src/index.browser.ts index 69ff2fde51..b080b50c59 100644 --- a/packages/snaps-utils/src/index.browser.ts +++ b/packages/snaps-utils/src/index.browser.ts @@ -9,6 +9,5 @@ export * from './notification'; export * from './object'; export * from './snaps'; export * from './types'; -export * from './url'; export * from './versions'; export * from './vfile/index.browser'; diff --git a/packages/snaps-utils/src/index.ts b/packages/snaps-utils/src/index.ts index 523d76cf6d..bd449c50a2 100644 --- a/packages/snaps-utils/src/index.ts +++ b/packages/snaps-utils/src/index.ts @@ -15,6 +15,5 @@ export * from './object'; export * from './post-process'; export * from './snaps'; export * from './types'; -export * from './url'; export * from './versions'; export * from './vfile'; diff --git a/packages/snaps-utils/src/snaps.ts b/packages/snaps-utils/src/snaps.ts index 9709191676..ad8f760fc3 100644 --- a/packages/snaps-utils/src/snaps.ts +++ b/packages/snaps-utils/src/snaps.ts @@ -11,14 +11,16 @@ import { size, string, Struct, - type, assert as assertSuperstruct, - union, - instance, } from 'superstruct'; import { SnapManifest, SnapPermissions } from './manifest/validation'; -import { SnapId, SnapIdPrefixes, SnapValidationFailureReason } from './types'; +import { + SnapId, + SnapIdPrefixes, + SnapValidationFailureReason, + uri, +} from './types'; import { SemVerVersion } from './versions'; export const SNAP_PREFIX = 'wallet_snap_'; @@ -210,27 +212,6 @@ export function validateSnapShasum( export const LOCALHOST_HOSTNAMES = ['localhost', '127.0.0.1', '::1'] as const; -type UriOptions = { - protocol?: Struct; - hash?: Struct; - port?: Struct; - hostname?: Struct; - pathname?: Struct; - search?: Struct; -}; -export const uri = (opts: UriOptions = {}) => - refine(union([string(), instance(URL)]), 'uri', (value) => { - try { - const url = new URL(value); - - const UrlStruct = type(opts); - assertSuperstruct(url, UrlStruct); - return true; - } catch { - return `Expected URL, got "${value.toString()}".`; - } - }); - const LocalSnapIdSubUrlStruct = uri({ protocol: enums(['http:', 'https:']), hostname: enums(LOCALHOST_HOSTNAMES), diff --git a/packages/snaps-utils/src/types.test.ts b/packages/snaps-utils/src/types.test.ts index fbeb99b093..879d42d995 100644 --- a/packages/snaps-utils/src/types.test.ts +++ b/packages/snaps-utils/src/types.test.ts @@ -1,5 +1,12 @@ +import { enums, is, literal } from 'superstruct'; + import { getPackageJson } from './test-utils'; -import { assertIsNpmSnapPackageJson, isNpmSnapPackageJson } from './types'; +import { + assertIsNpmSnapPackageJson, + isNpmSnapPackageJson, + isValidUrl, + uri, +} from './types'; describe('isNpmSnapPackageJson', () => { it('returns true for a valid package.json', () => { @@ -50,3 +57,39 @@ describe('assertIsNpmSnapPackageJson', () => { ); }); }); + +describe.each([ + isValidUrl, + (value: unknown, opts?: any) => is(value, uri(opts)), +])('uri', (testedFn) => { + it.each([ + 'npm:foo-bar', + 'http://asd.com', + 'https://dsa.com/foo', + 'http://dsa.com/foo?asd=5&dsa=6#bar', + 'npm:foo/bar?asd', + 'local:asd.com', + 'http://asd@asd.com', + 'http://asd:foo@asd.com', + new URL('http://asd.com'), + ])('validates correct uri', (value) => { + expect(testedFn(value)).toBe(true); + }); + + it.each([5, 'asd', undefined, null, {}, uri, URL])( + 'invalidates invalid uri', + (value) => { + expect(testedFn(value)).toBe(false); + }, + ); + + it('takes additional constraints', () => { + const constraints = { + protocol: enums(['foo:', 'bar:']), + hash: literal('#hello'), + }; + expect(testedFn('foo://asd.com/#hello', constraints)).toBe(true); + expect(testedFn('foo://asd.com/', constraints)).toBe(false); + expect(testedFn('http://asd.com/#hello', constraints)).toBe(false); + }); +}); diff --git a/packages/snaps-utils/src/types.ts b/packages/snaps-utils/src/types.ts index 29020359b7..3d0f08a76e 100644 --- a/packages/snaps-utils/src/types.ts +++ b/packages/snaps-utils/src/types.ts @@ -5,13 +5,18 @@ import { import { assertStruct, Json } from '@metamask/utils'; import { Infer, + instance, is, object, optional, pattern, + refine, size, string, + Struct, type, + union, + assert as assertSuperstruct, } from 'superstruct'; import { SnapManifest } from './manifest/validation'; @@ -163,3 +168,38 @@ export type SnapExportsParameters = // while internal symbols do not. declare const brand: unique symbol; export type Opaque = Base & { [brand]: Brand }; + +type UriOptions = { + protocol?: Struct; + hash?: Struct; + port?: Struct; + hostname?: Struct; + pathname?: Struct; + search?: Struct; +}; +export const uri = (opts: UriOptions = {}) => + refine(union([string(), instance(URL)]), 'uri', (value) => { + try { + const url = new URL(value); + + const UrlStruct = type(opts); + assertSuperstruct(url, UrlStruct); + return true; + } catch { + return `Expected URL, got "${value.toString()}".`; + } + }); + +/** + * Returns whether a given value is a valid URL. + * + * @param url - The value to check. + * @param opts - Optional constraints for url checking. + * @returns Whether `url` is valid URL or not. + */ +export function isValidUrl( + url: unknown, + opts: UriOptions = {}, +): url is string | URL { + return is(url, uri(opts)); +} diff --git a/packages/snaps-utils/src/url.ts b/packages/snaps-utils/src/url.ts deleted file mode 100644 index 56c481d64d..0000000000 --- a/packages/snaps-utils/src/url.ts +++ /dev/null @@ -1,13 +0,0 @@ -/** - * Checks whether a URL is valid. - * - * @param maybeUrl - The string to check. - * @returns Whether the specified string is a valid URL. - */ -export function isValidUrl(maybeUrl: string): maybeUrl is string { - try { - return Boolean(new URL(maybeUrl)); - } catch (_error) { - return false; - } -} diff --git a/packages/snaps-utils/src/vfile/to-vfile.test.ts b/packages/snaps-utils/src/vfile/to-vfile.test.ts new file mode 100644 index 0000000000..e426879561 --- /dev/null +++ b/packages/snaps-utils/src/vfile/to-vfile.test.ts @@ -0,0 +1,53 @@ +import { vol } from 'memfs'; + +import { + readVFile, + readVFileSync, + writeVFile, + writeVFileSync, +} from './to-vfile'; +import { VFile } from './vfile'; + +const CONTENTS_UTF8 = 'foo\nbar'; + +jest.mock('fs'); +jest.mock('fs/promises'); + +describe('to-vfile', () => { + beforeEach(() => { + vol.reset(); + /* eslint-disable @typescript-eslint/naming-convention */ + vol.fromJSON({ + '/foo/utf-8.txt': CONTENTS_UTF8, + }); + /* eslint-enable @typescript-eslint/naming-convention */ + }); + + describe.each([readVFile, readVFileSync])('readVFile', (testedFn) => { + it('reads file', async () => { + const file = await testedFn('/foo/utf-8.txt'); + expect(file).toBeInstanceOf(VFile); + expect(typeof file.value).not.toBe('string'); + expect(file.toString()).toBe(CONTENTS_UTF8); + }); + + it('decodes file', async () => { + const file = await testedFn('/foo/utf-8.txt', 'utf8'); + expect(file).toBeInstanceOf(VFile); + expect(typeof file.value).toBe('string'); + expect(file.value).toBe(CONTENTS_UTF8); + }); + }); + + describe.each([writeVFile, writeVFileSync])('writeVFile', (testedFn) => { + it('writes files', async () => { + const PATH = '/foo/out.txt'; + await testedFn(new VFile({ value: CONTENTS_UTF8, path: PATH })); + + expect(vol.toJSON(PATH)).toStrictEqual( + // eslint-disable-next-line @typescript-eslint/naming-convention + { [PATH]: CONTENTS_UTF8 }, + ); + }); + }); +}); diff --git a/packages/snaps-utils/src/vfile/to-vfile.ts b/packages/snaps-utils/src/vfile/to-vfile.ts index 9996e714c3..bf58725ddb 100644 --- a/packages/snaps-utils/src/vfile/to-vfile.ts +++ b/packages/snaps-utils/src/vfile/to-vfile.ts @@ -10,7 +10,7 @@ import { VFile } from './vfile'; * @param encoding - Optional encoding to pass down to fs.readFile. * @returns Promise returning VFile with loaded file contents. */ -export async function readVfile( +export async function readVFile( path: string, encoding: BufferEncoding | null = null, ) { @@ -27,7 +27,7 @@ export async function readVfile( * @param encoding - Optional encoding to pass down to fs.readFile. * @returns VFile with loaded file contents. */ -export function readVfileSync( +export function readVFileSync( path: string, encoding: BufferEncoding | null = null, ) { diff --git a/packages/snaps-utils/src/vfile/vfile.test.ts b/packages/snaps-utils/src/vfile/vfile.test.ts new file mode 100644 index 0000000000..11341eae6e --- /dev/null +++ b/packages/snaps-utils/src/vfile/vfile.test.ts @@ -0,0 +1,72 @@ +import { VFile } from './vfile'; + +const VALUE = 'foo\nbar'; + +describe('VFile', () => { + it('stores value string', () => { + const file = new VFile({ value: VALUE }); + expect(file.value).toBe(VALUE); + expect(file.toString()).toStrictEqual(VALUE); + }); + + it('stores value array', () => { + const array = new TextEncoder().encode(VALUE); + const file = new VFile({ value: array }); + expect(file.value).toStrictEqual(array); + expect(file.toString()).toStrictEqual(VALUE); + }); + + it('cant take options or string', () => { + const file1 = new VFile(VALUE); + const file2 = new VFile({ value: VALUE }); + expect(file1.value).toStrictEqual(file2.value); + expect(file1.value).toStrictEqual(VALUE); + }); + + describe('toString()', () => { + it('supports encodings', () => { + // TextEncoder doesn't support utf-16 anymore + // "foo\nbar" in utf-16be + const array = new Uint8Array([ + 0x00, 0x66, 0x00, 0x6f, 0x00, 0x6f, 0x00, 0x0a, 0x00, 0x62, 0x00, 0x61, + 0x00, 0x72, + ]); + const file = new VFile({ value: array }); + expect(file.toString('utf-16be')).toStrictEqual(VALUE); + }); + }); + + describe('clone()', () => { + it('deep clones properties', () => { + const value = VALUE; + const result = { foo: 'bar' }; + const data = { bar: 'foo' }; + const path = '/foo/bar'; + const file = new VFile({ value, result, data, path }); + const file2 = file.clone(); + file2.value = 'asd'; + file2.result.foo = 'asd'; + file2.data.bar = 'asd'; + file2.path = 'asd'; + expect(file.value).toStrictEqual(value); + expect(file2.value).not.toStrictEqual(value); + expect(file.result).toStrictEqual(result); + expect(file2.result).not.toStrictEqual(result); + expect(file.data).toStrictEqual(data); + expect(file2.data).not.toStrictEqual(data); + expect(file.path).toStrictEqual(path); + expect(file2.path).not.toStrictEqual(path); + }); + + it('clones Uint8Array properly', () => { + const array = new TextEncoder().encode(VALUE); + const file1 = new VFile({ value: array }); + const file2 = file1.clone(); + + array[0] = 42; + + expect(file1.value[0]).toBe(42); + expect(file2.value[0]).not.toBe(42); + }); + }); +}); diff --git a/yarn.lock b/yarn.lock index 0a073fcd59..15865fc14b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3064,7 +3064,7 @@ __metadata: fast-deep-equal: ^3.1.3 jest: ^29.0.2 jest-it-up: ^2.0.0 - memfs: ^3.4.7 + memfs: ^3.4.12 prettier: ^2.7.1 prettier-plugin-packagejson: ^2.2.11 rfdc: ^1.3.0 @@ -11172,12 +11172,12 @@ __metadata: languageName: node linkType: hard -"memfs@npm:^3.4.1, memfs@npm:^3.4.10, memfs@npm:^3.4.7": - version: 3.4.10 - resolution: "memfs@npm:3.4.10" +"memfs@npm:^3.4.1, memfs@npm:^3.4.10, memfs@npm:^3.4.12, memfs@npm:^3.4.7": + version: 3.4.12 + resolution: "memfs@npm:3.4.12" dependencies: fs-monkey: ^1.0.3 - checksum: bee25e00682a440adedafb81277f1a192c31e27995236d899c5402511648645dec8a0a6c2753c36632d4522bce08ebbaa38dece15753589cbe98223a47aeab4c + checksum: dab8dec1ae0b2a92e4d563ac86846047cd7aeb17cde4ad51da85cff6e580c32d12b886354527788e36eb75f733dd8edbaf174476b7cea73fed9c5a0e45a6b428 languageName: node linkType: hard From 10b46bbfb77c7452b544d93a8273fd5bef33f562 Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Wed, 30 Nov 2022 14:40:55 +0100 Subject: [PATCH 06/15] Added coverage for Snap IDs --- packages/snaps-utils/jest.config.js | 6 +- packages/snaps-utils/package.json | 4 +- packages/snaps-utils/src/snaps.test.ts | 171 +++++++++++++++++++++++-- packages/snaps-utils/src/snaps.ts | 29 +++-- yarn.lock | 29 ++++- 5 files changed, 217 insertions(+), 22 deletions(-) diff --git a/packages/snaps-utils/jest.config.js b/packages/snaps-utils/jest.config.js index e0957e96ff..2b59d8f470 100644 --- a/packages/snaps-utils/jest.config.js +++ b/packages/snaps-utils/jest.config.js @@ -18,10 +18,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 94.76, + branches: 94.41, functions: 98.95, - lines: 98.74, - statements: 98.74, + lines: 99.01, + statements: 99.01, }, }, testTimeout: 2500, diff --git a/packages/snaps-utils/package.json b/packages/snaps-utils/package.json index 8ca0cbf1ea..78187c3521 100644 --- a/packages/snaps-utils/package.json +++ b/packages/snaps-utils/package.json @@ -62,7 +62,8 @@ "rfdc": "^1.3.0", "semver": "^7.3.7", "ses": "^0.17.0", - "superstruct": "^0.16.7" + "superstruct": "^0.16.7", + "validate-npm-package-name": "^5.0.0" }, "devDependencies": { "@lavamoat/allow-scripts": "^2.0.3", @@ -73,6 +74,7 @@ "@metamask/eslint-config-typescript": "^11.0.0", "@types/jest": "^27.5.1", "@types/semver": "^7.3.10", + "@types/validate-npm-package-name": "^4.0.0", "@typescript-eslint/eslint-plugin": "^5.42.1", "@typescript-eslint/parser": "^5.42.1", "deepmerge": "^4.2.2", diff --git a/packages/snaps-utils/src/snaps.test.ts b/packages/snaps-utils/src/snaps.test.ts index 88b3f29377..9a2525b2b4 100644 --- a/packages/snaps-utils/src/snaps.test.ts +++ b/packages/snaps-utils/src/snaps.test.ts @@ -1,12 +1,23 @@ -import { isCaipChainId, validateSnapId } from './snaps'; -import { SnapIdPrefixes } from './types'; +import { is } from 'superstruct'; + +import { + HttpSnapIdStruct, + isCaipChainId, + LocalSnapIdStruct, + NpmSnapIdStruct, + validateSnapId, +} from './snaps'; +import { SnapIdPrefixes, uri } from './types'; describe('validateSnapId', () => { - it.each([undefined, {}, null, true, 2])('throws for non-strings', (value) => { - expect(() => validateSnapId(value)).toThrow( - 'Invalid snap id. Not a string.', - ); - }); + it.each([undefined, {}, null, true, 2])( + 'throws for non-strings (#%#)', + (value) => { + expect(() => validateSnapId(value)).toThrow( + 'Invalid snap id. Not a string.', + ); + }, + ); it('throws for invalid snap id', () => { expect(() => validateSnapId('foo:bar')).toThrow( @@ -24,7 +35,7 @@ describe('validateSnapId', () => { describe('isCaipChainId', () => { it.each([undefined, {}, null, true, 2])( - 'returns false for non-strings', + 'returns false for non-strings (#%#)', (value) => { expect(isCaipChainId(value)).toBe(false); }, @@ -35,7 +46,149 @@ describe('isCaipChainId', () => { 'cosmos:iov-mainnet', 'bip122:000000000019d6689c085ae165831e93', 'cosmos:cosmoshub-2', - ])('returns true for valid IDs', (value) => { + ])('returns true for valid IDs (#%#)', (value) => { expect(isCaipChainId(value)).toBe(true); }); }); + +describe('LocalSnapIdStruct', () => { + it.each([ + 'local:http://localhost', + 'local:http://localhost/', + 'local:http://localhost/', + 'local:https://localhost', + 'local:https://localhost/', + 'local:http://127.0.0.1/foo/bar', + 'local:http://127.0.0.1:8080/', + 'local:http://localhost:8080/', + 'local:http://[::1]:8080/', + 'local:http://[::1]', + 'local:https://foo@localhost/', + 'local:http://foo:bar@127.0.01/', + ])('validates "%s" as proper local ID', (value) => { + expect(is(value, LocalSnapIdStruct)).toBe(true); + }); + + it.each([ + 0, + 1, + '', + 'foo', + false, + true, + {}, + [], + uri, + URL, + new URL('http://127.0.01'), + new URL('local:127.0.0.1'), + 'http://localhost', + '127.0.0.1', + 'local:127.0.0.1', + 'local:127.0.0.1/', + 'local:foo://127.0.0.1/', + 'local:http://github.com', + 'local:http://localhost/foo?bar=true', + 'local:http://localhost/foo#bar', + 'local:http://localhost/42?foo=true#bar', + 'local', + 'local:', + 'local:http://', + ])('invalidates an improper local ID (#%#)', (value) => { + expect(is(value, LocalSnapIdStruct)).toBe(false); + }); +}); + +describe('NpmSnapIdStruct', () => { + it.each([ + 'npm:foo', + 'npm:foo-bar', + 'npm://registry.com/foo', + 'npm:@foo/bar', + 'npm://registry.com/@foo/bar', + 'npm://user@registry.com/bar', + 'npm://user:pass@registry.com/bar', + 'npm://[::1]/bar', + 'npm://[::1]:8080/bar', + ])('validates "%s" as proper NPM ID', (value) => { + expect(is(value, NpmSnapIdStruct)).toBe(true); + }); + + it.each([ + 0, + 1, + false, + true, + {}, + [], + uri, + URL, + '', + 'npm:', + 'npm', + 'npm:http://registry.com/foo', + 'npm://registry.com', + 'npm://registry.com/', + 'npm:foo#bar', + 'npm:foo?bar=true', + 'npm:snap?foo=true#bar', + 'npm://registry.com/snap?foo=true', + 'npm://registry.com/snap#foo', + 'npm://registry.com/snap?foo=true#bar', + 'local:foo', + 'local://registry.com/foo', + 'foo:bar', + 'npm:ASDASDasd', + 'npm:.', + 'npm:excited!', + // 220 characters, limit is 214 + 'npm:abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij', + new URL('npm:foo'), + new URL('npm://registry.com/foo'), + ])('invalidates an improper NPM ID (#%#)', (value) => { + expect(is(value, NpmSnapIdStruct)).toBe(false); + }); +}); + +describe('HttpSnapIdStruct', () => { + it.each([ + 'http://[::1]', + 'http://[::1]:8080', + 'http://localhost', + 'http://localhost/', + 'https://localhost', + 'https://localhost/', + 'http://github.com', + 'http://github.com/foo', + 'http://gihtub.com/@foo/bar', + 'https://github.com/@foo/bar', + ])('validates "%s" as proper http ID', (value) => { + expect(is(value, HttpSnapIdStruct)).toBe(true); + }); + + it.each([ + 0, + 1, + false, + true, + {}, + [], + uri, + URL, + new URL('http://github.com'), + '', + 'http', + 'http:', + 'http://', + 'foo://localhost', + 'npm://localhost', + 'npm:localhost', + 'local:http://localhost', + 'http://github.com/?foo=true', + 'http://github.com/#foo', + 'http://github.com/?foo=true#bar', + 'http://github.com/snap?foo=true#bar', + ])('invalidates an improper http ID (#%#)', (value) => { + expect(is(value, HttpSnapIdStruct)).toBe(false); + }); +}); diff --git a/packages/snaps-utils/src/snaps.ts b/packages/snaps-utils/src/snaps.ts index ad8f760fc3..bcd80b1691 100644 --- a/packages/snaps-utils/src/snaps.ts +++ b/packages/snaps-utils/src/snaps.ts @@ -1,4 +1,4 @@ -import { Json } from '@metamask/utils'; +import { assert, Json } from '@metamask/utils'; import { sha256 } from '@noble/hashes/sha256'; import { base64 } from '@scure/base'; import { SerializedEthereumRpcError } from 'eth-rpc-errors/dist/classes'; @@ -8,11 +8,11 @@ import { intersection, literal, refine, - size, string, Struct, - assert as assertSuperstruct, + validate, } from 'superstruct'; +import validateNPMPackage from 'validate-npm-package-name'; import { SnapManifest, SnapPermissions } from './manifest/validation'; import { @@ -210,7 +210,7 @@ export function validateSnapShasum( } } -export const LOCALHOST_HOSTNAMES = ['localhost', '127.0.0.1', '::1'] as const; +export const LOCALHOST_HOSTNAMES = ['localhost', '127.0.0.1', '[::1]'] as const; const LocalSnapIdSubUrlStruct = uri({ protocol: enums(['http:', 'https:']), @@ -223,17 +223,30 @@ export const LocalSnapIdStruct = refine(string(), 'local Snap Id', (value) => { return `Expected local Snap ID, got "${value}".`; } - assertSuperstruct( - `https://${value.slice(SnapIdPrefixes.local.length)}`, + const [error] = validate( + value.slice(SnapIdPrefixes.local.length), LocalSnapIdSubUrlStruct, ); - return true; + return error ?? true; }); export const NpmSnapIdStruct = intersection([ string(), uri({ protocol: literal(SnapIdPrefixes.npm), - pathname: size(string(), 2, Infinity), // potentially starting with / if registry is set. + pathname: refine(string(), 'package name', function* (value) { + const normalized = value.startsWith('/') ? value.slice(1) : value; + const { errors, validForNewPackages, warnings } = + validateNPMPackage(normalized); + if (!validForNewPackages) { + if (errors === undefined) { + assert(warnings !== undefined); + yield* warnings; + } else { + yield* errors; + } + } + return true; + }), search: empty(string()), hash: empty(string()), }), diff --git a/yarn.lock b/yarn.lock index 15865fc14b..4058bb6422 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3049,6 +3049,7 @@ __metadata: "@scure/base": ^1.1.1 "@types/jest": ^27.5.1 "@types/semver": ^7.3.10 + "@types/validate-npm-package-name": ^4.0.0 "@typescript-eslint/eslint-plugin": ^5.42.1 "@typescript-eslint/parser": ^5.42.1 cron-parser: ^4.5.0 @@ -3074,6 +3075,7 @@ __metadata: superstruct: ^0.16.7 ts-jest: ^29.0.0 typescript: ~4.8.4 + validate-npm-package-name: ^5.0.0 languageName: unknown linkType: soft @@ -3850,6 +3852,13 @@ __metadata: languageName: node linkType: hard +"@types/validate-npm-package-name@npm:^4.0.0": + version: 4.0.0 + resolution: "@types/validate-npm-package-name@npm:4.0.0" + checksum: 09efd659f4cce362931a57c7ee6d645855c7ecb346c6496ef31bab638e578472f779c77233adad8502e90a7d7a146a40a5029ff9c4739a38a4187ef1fe7187c3 + languageName: node + linkType: hard + "@types/vinyl-fs@npm:*": version: 2.4.12 resolution: "@types/vinyl-fs@npm:2.4.12" @@ -5665,6 +5674,15 @@ __metadata: languageName: node linkType: hard +"builtins@npm:^5.0.0": + version: 5.0.1 + resolution: "builtins@npm:5.0.1" + dependencies: + semver: ^7.0.0 + checksum: 66d204657fe36522822a95b288943ad11b58f5eaede235b11d8c4edaa28ce4800087d44a2681524c340494aadb120a0068011acabe99d30e8f11a7d826d83515 + languageName: node + linkType: hard + "bytes@npm:3.0.0": version: 3.0.0 resolution: "bytes@npm:3.0.0" @@ -13581,7 +13599,7 @@ __metadata: languageName: node linkType: hard -"semver@npm:7.x, semver@npm:^7.3.2, semver@npm:^7.3.4, semver@npm:^7.3.5, semver@npm:^7.3.7, semver@npm:^7.3.8": +"semver@npm:7.x, semver@npm:^7.0.0, semver@npm:^7.3.2, semver@npm:^7.3.4, semver@npm:^7.3.5, semver@npm:^7.3.7, semver@npm:^7.3.8": version: 7.3.8 resolution: "semver@npm:7.3.8" dependencies: @@ -15369,6 +15387,15 @@ __metadata: languageName: node linkType: hard +"validate-npm-package-name@npm:^5.0.0": + version: 5.0.0 + resolution: "validate-npm-package-name@npm:5.0.0" + dependencies: + builtins: ^5.0.0 + checksum: 5342a994986199b3c28e53a8452a14b2bb5085727691ea7aa0d284a6606b127c371e0925ae99b3f1ef7cc7d2c9de75f52eb61a3d1cc45e39bca1e3a9444cbb4e + languageName: node + linkType: hard + "value-or-function@npm:^3.0.0": version: 3.0.0 resolution: "value-or-function@npm:3.0.0" From f4e877deb68f8e40a82338b9ad4bd13e143416e7 Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Thu, 1 Dec 2022 12:00:44 +0100 Subject: [PATCH 07/15] Fix test after stringer SnapID requirements --- .../src/snaps/location/location.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/location/location.test.ts b/packages/snaps-controllers/src/snaps/location/location.test.ts index 5da4a7ad21..2f0af7775f 100644 --- a/packages/snaps-controllers/src/snaps/location/location.test.ts +++ b/packages/snaps-controllers/src/snaps/location/location.test.ts @@ -20,13 +20,13 @@ describe('detectSnapLocation', () => { }); it.each([ - ['npm:', NpmLocation], - ['local:', LocalLocation], - ['http:', HttpLocation], - ['https:', HttpLocation], - ])('detects %s protocol', (protocol, classObj) => { + ['npm:package', NpmLocation], + ['local:http://localhost', LocalLocation], + ['https://localhost', HttpLocation], + ['http://localhost', HttpLocation], + ])('detects %s', (url, classObj) => { expect( - detectSnapLocation(`${protocol}localhost/foo`, { + detectSnapLocation(url, { allowHttp: true, allowCustomRegistries: true, }), From c1ee147ddb1edbe9cafb774cc639a24bfeca8c6a Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Thu, 1 Dec 2022 13:07:22 +0100 Subject: [PATCH 08/15] SnapLocation coverage --- packages/snaps-controllers/jest.config.js | 8 +-- .../src/snaps/location/http.test.ts | 23 +++++++-- .../src/snaps/location/local.test.ts | 49 +++++++++++++++++++ .../src/snaps/location/local.ts | 7 +-- .../src/snaps/location/npm.test.ts | 18 +++++++ .../src/snaps/location/npm.ts | 3 +- packages/snaps-controllers/src/utils.ts | 5 ++ 7 files changed, 99 insertions(+), 14 deletions(-) create mode 100644 packages/snaps-controllers/src/snaps/location/local.test.ts diff --git a/packages/snaps-controllers/jest.config.js b/packages/snaps-controllers/jest.config.js index 14340c3030..7f239b1d25 100644 --- a/packages/snaps-controllers/jest.config.js +++ b/packages/snaps-controllers/jest.config.js @@ -5,10 +5,10 @@ const baseConfig = require('../../jest.config.base'); module.exports = deepmerge(baseConfig, { coverageThreshold: { global: { - branches: 88.74, - functions: 97.23, - lines: 97.09, - statements: 97.09, + branches: 90.98, + functions: 96.25, + lines: 97.45, + statements: 97.45, }, }, projects: [ diff --git a/packages/snaps-controllers/src/snaps/location/http.test.ts b/packages/snaps-controllers/src/snaps/location/http.test.ts index 0cfaaaab6d..cb4be3e08a 100644 --- a/packages/snaps-controllers/src/snaps/location/http.test.ts +++ b/packages/snaps-controllers/src/snaps/location/http.test.ts @@ -9,6 +9,10 @@ import HttpLocation from './http'; fetchMock.enableMocks(); describe('HttpLocation', () => { + beforeEach(() => { + fetchMock.resetMocks(); + }); + it('loads the files', async () => { const base = 'http://foo.bar/foo/'; const manifest = getSnapManifest(); @@ -21,9 +25,22 @@ describe('HttpLocation', () => { expect.objectContaining({ value: manifestStr, result: manifest }), ); - const bundle = await location.fetch( - new URL(manifest.source.location.npm.filePath, base).toString(), - ); + const bundle = await location.fetch(manifest.source.location.npm.filePath); expect(bundle.toString()).toStrictEqual(DEFAULT_SNAP_BUNDLE); }); + + it('returns proper root', () => { + const base = 'http://foo.bar/foo/'; + expect(new HttpLocation(new URL(base)).root.toString()).toStrictEqual(base); + }); + + it.each([ + ['http://foo.bar/foo', 'http://foo.bar/foo.js'], + ['http://foo.bar/foo/', 'http://foo.bar/foo/foo.js'], + ])('sets canonical path properly', async (base, canonical) => { + fetchMock.mockResponses(DEFAULT_SNAP_BUNDLE); + + const file = await new HttpLocation(new URL(base)).fetch('./foo.js'); + expect(file.data.canonicalPath).toBe(canonical); + }); }); diff --git a/packages/snaps-controllers/src/snaps/location/local.test.ts b/packages/snaps-controllers/src/snaps/location/local.test.ts new file mode 100644 index 0000000000..70c7f4cead --- /dev/null +++ b/packages/snaps-controllers/src/snaps/location/local.test.ts @@ -0,0 +1,49 @@ +import { + DEFAULT_SNAP_BUNDLE, + getSnapManifest, +} from '@metamask/snaps-utils/test-utils'; +import fetchMock from 'jest-fetch-mock'; + +import LocalLocation from './local'; + +fetchMock.enableMocks(); + +describe('LocalLocation', () => { + beforeEach(() => { + fetchMock.resetMocks(); + }); + + it('fetches files', async () => { + const manifest = getSnapManifest(); + const manifestStr = JSON.stringify(manifest); + fetchMock.mockResponses(manifestStr, DEFAULT_SNAP_BUNDLE); + const location = new LocalLocation(new URL('local:http://localhost')); + + expect(await location.manifest()).toStrictEqual( + expect.objectContaining({ value: manifestStr, result: manifest }), + ); + const bundle = ( + await location.fetch(manifest.source.location.npm.filePath) + ).toString(); + expect(bundle).toBe(DEFAULT_SNAP_BUNDLE); + }); + + it('signals that it should be reloaded', () => { + expect( + new LocalLocation(new URL('local:http://localhost')).shouldAlwaysReload, + ).toBe(true); + }); + + it.each([ + ['local:http://localhost/foo', 'local:http://localhost/foo.js'], + ['local:http://127.0.0.1/foo/', 'local:http://127.0.0.1/foo/foo.js'], + [ + 'local:https://user:pass@[::1]:8080/foo/bar', + 'local:https://user:pass@[::1]:8080/foo/foo.js', + ], + ])('sets canonical path properly', async (base, canonical) => { + fetchMock.mockResponses(DEFAULT_SNAP_BUNDLE); + const file = await new LocalLocation(new URL(base)).fetch('./foo.js'); + expect(file.data.canonicalPath).toBe(canonical); + }); +}); diff --git a/packages/snaps-controllers/src/snaps/location/local.ts b/packages/snaps-controllers/src/snaps/location/local.ts index 0ec07f116c..aac28c85e1 100644 --- a/packages/snaps-controllers/src/snaps/location/local.ts +++ b/packages/snaps-controllers/src/snaps/location/local.ts @@ -15,7 +15,7 @@ export default class LocalLocation implements SnapLocation { constructor(url: URL) { assertStruct(url.toString(), LocalSnapIdStruct, 'Invalid Snap Id'); this.#http = new HttpLocation( - new URL(`https://${url.toString().slice(SnapIdPrefixes.local.length)}`), + new URL(url.toString().slice(SnapIdPrefixes.local.length)), ); } @@ -42,9 +42,6 @@ export default class LocalLocation implements SnapLocation { */ function convertCanonical(vfile: VFile): VFile { assert(vfile.data.canonicalPath !== undefined); - const canonicalPath = new URL(vfile.data.canonicalPath); - assert(canonicalPath.protocol === 'http:'); - canonicalPath.protocol = 'local:'; - vfile.data.canonicalPath = canonicalPath.toString(); + vfile.data.canonicalPath = `local:${vfile.data.canonicalPath}`; return vfile; } diff --git a/packages/snaps-controllers/src/snaps/location/npm.test.ts b/packages/snaps-controllers/src/snaps/location/npm.test.ts index 5d4f27dcf3..200d89dae6 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.test.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.test.ts @@ -128,4 +128,22 @@ describe('NpmLocation', () => { 'Tried to access version without first fetching NPM package.', ); }); + + // TODO(ritave): Doing this tests requires writing tarball packing utility function + // With the the current changeset going way out of scope, I'm leaving this for future. + it.todo('sets canonical path properly'); + // eslint-disable-next-line jest/no-commented-out-tests + /* + it.each([ + ['npm:foo', 'npm://registry.npmjs.org/foo/foo.js'], + [ + 'npm:@foo/bar', + ['npm://registry.npmjs.org/@foo/bar/foo.js'], + [ + 'npm://user:pass@asd.com:8080/@foo/bar', + 'npm://user:pass@asd.com:8080/@foo/bar/foo.js', + ], + ], + ]); + */ }); diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts index c916ce3969..44d7b182d2 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -128,12 +128,11 @@ export default class NpmLocation implements SnapLocation { } async fetch(path: string): Promise { + const relativePath = ensureRelative(path); if (!this.files) { await this.#lazyInit(); assert(this.files !== undefined); } - assert(!path.startsWith('/'), 'Tried to fetch absolute path.'); - const relativePath = ensureRelative(path); const vfile = this.files.get(relativePath); assert( vfile !== undefined, diff --git a/packages/snaps-controllers/src/utils.ts b/packages/snaps-controllers/src/utils.ts index af1fac2b56..78cf5b2f91 100644 --- a/packages/snaps-controllers/src/utils.ts +++ b/packages/snaps-controllers/src/utils.ts @@ -209,6 +209,11 @@ export type Mutable< */ export function ensureRelative(path: string): string { assert(!path.startsWith('/')); + assert( + path.search(/:|\/\//u) === -1, + `Path "${path}" potentially an URI instead of local relative`, + ); + if (path.startsWith('./')) { return path; } From d76f14123f756beaf5b70427492c4324ef6ab515 Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Thu, 1 Dec 2022 13:31:36 +0100 Subject: [PATCH 09/15] PR review * vfile -> VirtualFile * Added tests for ensure relative * Made locations not default exports * Minor typos * Remove Sync to-vfile functions * Add fetch parameter to Locations --- packages/snaps-controllers/jest.config.js | 8 +-- .../src/snaps/SnapController.test.ts | 10 +-- .../src/snaps/SnapController.ts | 10 +-- .../src/snaps/location/http.test.ts | 2 +- .../src/snaps/location/http.ts | 35 +++++++--- .../src/snaps/location/index.ts | 6 +- .../src/snaps/location/local.test.ts | 2 +- .../src/snaps/location/local.ts | 17 ++--- .../src/snaps/location/location.test.ts | 6 +- .../src/snaps/location/location.ts | 12 ++-- .../src/snaps/location/npm.test.ts | 2 +- .../src/snaps/location/npm.ts | 18 ++--- .../src/test-utils/location.ts | 18 ++--- packages/snaps-controllers/src/utils.test.ts | 24 ++++++- packages/snaps-utils/src/index.browser.ts | 2 +- packages/snaps-utils/src/index.ts | 2 +- .../snaps-utils/src/vfile/index.browser.ts | 1 - packages/snaps-utils/src/vfile/index.ts | 2 - packages/snaps-utils/src/vfile/to-vfile.ts | 67 ------------------- .../VirtualFile.test.ts} | 16 ++--- .../vfile.ts => virtual-file/VirtualFile.ts} | 6 +- .../src/virtual-file/index.browser.ts | 1 + .../snaps-utils/src/virtual-file/index.ts | 2 + .../toVirtualFile.test.ts} | 26 +++---- .../src/virtual-file/toVirtualFile.ts | 38 +++++++++++ 25 files changed, 168 insertions(+), 165 deletions(-) delete mode 100644 packages/snaps-utils/src/vfile/index.browser.ts delete mode 100644 packages/snaps-utils/src/vfile/index.ts delete mode 100644 packages/snaps-utils/src/vfile/to-vfile.ts rename packages/snaps-utils/src/{vfile/vfile.test.ts => virtual-file/VirtualFile.test.ts} (81%) rename packages/snaps-utils/src/{vfile/vfile.ts => virtual-file/VirtualFile.ts} (96%) create mode 100644 packages/snaps-utils/src/virtual-file/index.browser.ts create mode 100644 packages/snaps-utils/src/virtual-file/index.ts rename packages/snaps-utils/src/{vfile/to-vfile.test.ts => virtual-file/toVirtualFile.test.ts} (61%) create mode 100644 packages/snaps-utils/src/virtual-file/toVirtualFile.ts diff --git a/packages/snaps-controllers/jest.config.js b/packages/snaps-controllers/jest.config.js index 7f239b1d25..c76ff3f295 100644 --- a/packages/snaps-controllers/jest.config.js +++ b/packages/snaps-controllers/jest.config.js @@ -5,10 +5,10 @@ const baseConfig = require('../../jest.config.base'); module.exports = deepmerge(baseConfig, { coverageThreshold: { global: { - branches: 90.98, - functions: 96.25, - lines: 97.45, - statements: 97.45, + branches: 90.88, + functions: 97.46, + lines: 97.46, + statements: 97.46, }, }, projects: [ diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index c3315a7f9d..76edaeccfc 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -13,7 +13,7 @@ import { SemVerVersion, SnapCaveatType, SnapStatus, - VFile, + VirtualFile, } from '@metamask/snaps-utils'; import { DEFAULT_SNAP_BUNDLE, @@ -1762,10 +1762,10 @@ describe('SnapController', () => { const location = new LoopbackLocation({ shouldAlwaysReload: true }); location.manifest .mockImplementationOnce(async () => - Promise.resolve(new VFile({ value: '', result: manifest })), + Promise.resolve(new VirtualFile({ value: '', result: manifest })), ) .mockImplementationOnce(async () => - Promise.resolve(new VFile({ value: '', result: newManifest })), + Promise.resolve(new VirtualFile({ value: '', result: newManifest })), ); const snapController = getSnapController( @@ -2075,11 +2075,11 @@ describe('SnapController', () => { detectSnapLocation: loopbackDetect({ manifest, files: [ - new VFile({ + new VirtualFile({ value: keyringSnap.sourceCode, path: manifest.source.location.npm.filePath, }), - new VFile({ + new VirtualFile({ value: DEFAULT_SNAP_ICON, path: manifest.source.location.npm.iconPath, }), diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index d661e23541..7993ca0b32 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -54,7 +54,7 @@ import { ValidatedSnapId, validateSnapId, validateSnapShasum, - VFile, + VirtualFile, } from '@metamask/snaps-utils'; import { GetSubjectMetadata, @@ -184,12 +184,12 @@ type FetchSnapResult = { /** * The manifest of the fetched Snap. */ - manifest: VFile; + manifest: VirtualFile; /** * Auxillary files references in manifest. */ - files: VFile[]; + files: VirtualFile[]; /** * Location that was used to fetch the snap. @@ -561,8 +561,8 @@ type AddSnapArgs = { // validated. type SetSnapArgs = Omit & { id: ValidatedSnapId; - manifest: VFile; - files: VFile[]; + manifest: VirtualFile; + files: VirtualFile[]; /** * @default '*' */ diff --git a/packages/snaps-controllers/src/snaps/location/http.test.ts b/packages/snaps-controllers/src/snaps/location/http.test.ts index cb4be3e08a..5b66cb2613 100644 --- a/packages/snaps-controllers/src/snaps/location/http.test.ts +++ b/packages/snaps-controllers/src/snaps/location/http.test.ts @@ -4,7 +4,7 @@ import { } from '@metamask/snaps-utils/test-utils'; import fetchMock from 'jest-fetch-mock'; -import HttpLocation from './http'; +import { HttpLocation } from './http'; fetchMock.enableMocks(); diff --git a/packages/snaps-controllers/src/snaps/location/http.ts b/packages/snaps-controllers/src/snaps/location/http.ts index fffcd2636f..7b30031e7a 100644 --- a/packages/snaps-controllers/src/snaps/location/http.ts +++ b/packages/snaps-controllers/src/snaps/location/http.ts @@ -1,7 +1,7 @@ import { SnapManifest, assertIsSnapManifest, - VFile, + VirtualFile, HttpSnapIdStruct, } from '@metamask/snaps-utils'; import { assert, assertStruct } from '@metamask/utils'; @@ -9,34 +9,47 @@ import { assert, assertStruct } from '@metamask/utils'; import { ensureRelative } from '../../utils'; import { SnapLocation } from './location'; -export default class HttpLocation implements SnapLocation { +export interface HttpOptions { + /** + * @default fetch + */ + fetch?: typeof fetch; +} + +export class HttpLocation implements SnapLocation { // We keep contents separate because then we can use only one Blob in cache, // which we convert to Uint8Array when actually returning the file. // // That avoids deepCloning file contents. // I imagine ArrayBuffers are copy-on-write optimized, meaning // in most often case we'll only have one file contents in common case. - private readonly cache = new Map(); + private readonly cache = new Map< + string, + { file: VirtualFile; contents: Blob } + >(); - private validatedManifest?: VFile; + private validatedManifest?: VirtualFile; private readonly url: URL; - constructor(url: URL) { + private readonly fetchFn: typeof fetch; + + constructor(url: URL, opts: HttpOptions = {}) { assertStruct(url.toString(), HttpSnapIdStruct, 'Invalid Snap Id: '); + this.fetchFn = opts.fetch ?? globalThis.fetch; this.url = url; } - async manifest(): Promise> { + async manifest(): Promise> { if (this.validatedManifest) { return this.validatedManifest.clone(); } // jest-fetch-mock doesn't handle new URL(), we need to convert this.url.toString() - const contents = await (await fetch(this.url.toString())).text(); + const contents = await (await this.fetchFn(this.url.toString())).text(); const manifest = JSON.parse(contents); assertIsSnapManifest(manifest); - const vfile = new VFile({ + const vfile = new VirtualFile({ value: contents, result: manifest, path: './snap.manifest.json', @@ -47,7 +60,7 @@ export default class HttpLocation implements SnapLocation { return this.manifest(); } - async fetch(path: string): Promise { + async fetch(path: string): Promise { const relativePath = ensureRelative(path); const cached = this.cache.get(relativePath); if (cached !== undefined) { @@ -59,8 +72,8 @@ export default class HttpLocation implements SnapLocation { } const canonicalPath = this.toCanonical(relativePath).toString(); - const response = await fetch(canonicalPath); - const vfile = new VFile({ + const response = await this.fetchFn(canonicalPath); + const vfile = new VirtualFile({ value: '', path: relativePath, data: { canonicalPath }, diff --git a/packages/snaps-controllers/src/snaps/location/index.ts b/packages/snaps-controllers/src/snaps/location/index.ts index 58d233e48f..0a2ad00b8b 100644 --- a/packages/snaps-controllers/src/snaps/location/index.ts +++ b/packages/snaps-controllers/src/snaps/location/index.ts @@ -1,4 +1,4 @@ export * from './location'; -export { default as NpmLocation } from './npm'; -export { default as LocalLocation } from './local'; -export { default as HttpLocation } from './http'; +export * from './npm'; +export * from './local'; +export * from './http'; diff --git a/packages/snaps-controllers/src/snaps/location/local.test.ts b/packages/snaps-controllers/src/snaps/location/local.test.ts index 70c7f4cead..8bc2208d45 100644 --- a/packages/snaps-controllers/src/snaps/location/local.test.ts +++ b/packages/snaps-controllers/src/snaps/location/local.test.ts @@ -4,7 +4,7 @@ import { } from '@metamask/snaps-utils/test-utils'; import fetchMock from 'jest-fetch-mock'; -import LocalLocation from './local'; +import { LocalLocation } from './local'; fetchMock.enableMocks(); diff --git a/packages/snaps-controllers/src/snaps/location/local.ts b/packages/snaps-controllers/src/snaps/location/local.ts index aac28c85e1..2921cd6b2b 100644 --- a/packages/snaps-controllers/src/snaps/location/local.ts +++ b/packages/snaps-controllers/src/snaps/location/local.ts @@ -2,30 +2,31 @@ import { LocalSnapIdStruct, SnapIdPrefixes, SnapManifest, - VFile, + VirtualFile, } from '@metamask/snaps-utils'; import { assert, assertStruct } from '@metamask/utils'; -import HttpLocation from './http'; +import { HttpLocation, HttpOptions } from './http'; import { SnapLocation } from './location'; -export default class LocalLocation implements SnapLocation { +export class LocalLocation implements SnapLocation { readonly #http: HttpLocation; - constructor(url: URL) { + constructor(url: URL, opts: HttpOptions = {}) { assertStruct(url.toString(), LocalSnapIdStruct, 'Invalid Snap Id'); this.#http = new HttpLocation( new URL(url.toString().slice(SnapIdPrefixes.local.length)), + opts, ); } - async manifest(): Promise> { + async manifest(): Promise> { const vfile = await this.#http.manifest(); return convertCanonical(vfile); } - async fetch(path: string): Promise { + async fetch(path: string): Promise { return convertCanonical(await this.#http.fetch(path)); } @@ -37,10 +38,10 @@ export default class LocalLocation implements SnapLocation { /** * Converts vfiles with canonical `http:` paths into `local:` paths. * - * @param vfile - The {@link VFile} to convert. + * @param vfile - The {@link VirtualFile} to convert. * @returns The same object with updated `.data.canonicalPath`. */ -function convertCanonical(vfile: VFile): VFile { +function convertCanonical(vfile: VirtualFile): VirtualFile { assert(vfile.data.canonicalPath !== undefined); vfile.data.canonicalPath = `local:${vfile.data.canonicalPath}`; return vfile; diff --git a/packages/snaps-controllers/src/snaps/location/location.test.ts b/packages/snaps-controllers/src/snaps/location/location.test.ts index 2f0af7775f..cfdb353c57 100644 --- a/packages/snaps-controllers/src/snaps/location/location.test.ts +++ b/packages/snaps-controllers/src/snaps/location/location.test.ts @@ -1,7 +1,7 @@ -import HttpLocation from './http'; -import LocalLocation from './local'; +import { HttpLocation } from './http'; +import { LocalLocation } from './local'; import { detectSnapLocation } from './location'; -import NpmLocation from './npm'; +import { NpmLocation } from './npm'; describe('detectSnapLocation', () => { it.each(['http:', 'https:'])( diff --git a/packages/snaps-controllers/src/snaps/location/location.ts b/packages/snaps-controllers/src/snaps/location/location.ts index 249c5cf851..ad0288117b 100644 --- a/packages/snaps-controllers/src/snaps/location/location.ts +++ b/packages/snaps-controllers/src/snaps/location/location.ts @@ -1,9 +1,9 @@ -import { SnapManifest, VFile } from '@metamask/snaps-utils'; +import { SnapManifest, VirtualFile } from '@metamask/snaps-utils'; import { assert } from '@metamask/utils'; -import HttpLocation from './http'; -import LocalLocation from './local'; -import NpmLocation, { NpmOptions } from './npm'; +import { HttpLocation } from './http'; +import { LocalLocation } from './local'; +import { NpmLocation, NpmOptions } from './npm'; declare module '@metamask/snaps-utils' { interface DataMap { @@ -18,8 +18,8 @@ export interface SnapLocation { /** * All files are relative to the manifest, except the manifest itself. */ - manifest(): Promise>; - fetch(path: string): Promise; + manifest(): Promise>; + fetch(path: string): Promise; readonly shouldAlwaysReload?: boolean; } diff --git a/packages/snaps-controllers/src/snaps/location/npm.test.ts b/packages/snaps-controllers/src/snaps/location/npm.test.ts index 200d89dae6..b50084dd4d 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.test.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.test.ts @@ -4,7 +4,7 @@ import { readFile } from 'fs/promises'; import fetchMock from 'jest-fetch-mock'; import path from 'path'; -import NpmLocation from './npm'; +import { NpmLocation } from './npm'; describe('NpmLocation', () => { beforeEach(() => { diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts index 44d7b182d2..e41efc8072 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -8,7 +8,7 @@ import { SemVerRange, SemVerVersion, SnapManifest, - VFile, + VirtualFile, } from '@metamask/snaps-utils'; import { assert, assertStruct, isObject } from '@metamask/utils'; import concat from 'concat-stream'; @@ -49,12 +49,12 @@ export interface NpmOptions { allowCustomRegistries?: boolean; } -export default class NpmLocation implements SnapLocation { +export class NpmLocation implements SnapLocation { private readonly meta: NpmMeta; - private validatedManifest?: VFile; + private validatedManifest?: VirtualFile; - private files?: Map; + private files?: Map; constructor(url: URL, opts: NpmOptions = {}) { const allowCustomRegistries = opts.allowCustomRegistries ?? false; @@ -113,7 +113,7 @@ export default class NpmLocation implements SnapLocation { }; } - async manifest(): Promise> { + async manifest(): Promise> { if (this.validatedManifest) { return this.validatedManifest.clone(); } @@ -122,12 +122,12 @@ export default class NpmLocation implements SnapLocation { const result = JSON.parse(vfile.toString()); assertIsSnapManifest(result); vfile.result = result; - this.validatedManifest = vfile as VFile; + this.validatedManifest = vfile as VirtualFile; return this.manifest(); } - async fetch(path: string): Promise { + async fetch(path: string): Promise { const relativePath = ensureRelative(path); if (!this.files) { await this.#lazyInit(); @@ -305,7 +305,7 @@ function getNodeStream(stream: ReadableStream): Readable { */ function createTarballStream( canonicalBase: string, - files: Map, + files: Map, ): Writable { assert( canonicalBase.endsWith('/'), @@ -329,7 +329,7 @@ function createTarballStream( const path = headerName.replace(NPM_TARBALL_PATH_PREFIX, './'); return entryStream.pipe( concat((data) => { - const vfile = new VFile({ + const vfile = new VirtualFile({ value: data, path, data: { diff --git a/packages/snaps-controllers/src/test-utils/location.ts b/packages/snaps-controllers/src/test-utils/location.ts index 1e6abadd3a..4a77427235 100644 --- a/packages/snaps-controllers/src/test-utils/location.ts +++ b/packages/snaps-controllers/src/test-utils/location.ts @@ -1,4 +1,4 @@ -import { VFile, SnapManifest } from '@metamask/snaps-utils'; +import { VirtualFile, SnapManifest } from '@metamask/snaps-utils'; import { DEFAULT_SNAP_BUNDLE, DEFAULT_SNAP_ICON, @@ -13,18 +13,18 @@ type LoopbackOptions = { /** * @default getSnapManifest() */ - manifest?: SnapManifest | VFile; + manifest?: SnapManifest | VirtualFile; /** * @default [new VFile({ value: DEFAULT_SNAP_BUNDLE, path: manifest.source.location.npm.filePath }] */ - files?: VFile[]; + files?: VirtualFile[]; /** * @default false */ shouldAlwaysReload?: boolean; }; -const isVfile = (obj: unknown): obj is VFile => { +const isVfile = (obj: unknown): obj is VirtualFile => { return ( typeof obj === 'object' && obj !== null && @@ -33,9 +33,9 @@ const isVfile = (obj: unknown): obj is VFile => { }; export class LoopbackLocation implements SnapLocation { - #manifest: VFile; + #manifest: VirtualFile; - #files: VFile[]; + #files: VirtualFile[]; #shouldAlwaysReload: boolean; @@ -43,7 +43,7 @@ export class LoopbackLocation implements SnapLocation { const shouldAlwaysReload = opts.shouldAlwaysReload ?? false; const manifest = isVfile(opts.manifest) ? opts.manifest - : new VFile({ + : new VirtualFile({ value: '', result: opts.manifest ?? getSnapManifest(), path: './snap.manifest.json', @@ -51,7 +51,7 @@ export class LoopbackLocation implements SnapLocation { let files; if (opts.files === undefined) { files = [ - new VFile({ + new VirtualFile({ value: DEFAULT_SNAP_BUNDLE, path: manifest.result.source.location.npm.filePath, }), @@ -59,7 +59,7 @@ export class LoopbackLocation implements SnapLocation { if (manifest.result.source.location.npm.iconPath !== undefined) { files.push( - new VFile({ + new VirtualFile({ value: DEFAULT_SNAP_ICON, path: manifest.result.source.location.npm.iconPath, }), diff --git a/packages/snaps-controllers/src/utils.test.ts b/packages/snaps-controllers/src/utils.test.ts index fe0e2a1b75..3b352a4f3c 100644 --- a/packages/snaps-controllers/src/utils.test.ts +++ b/packages/snaps-controllers/src/utils.test.ts @@ -1,4 +1,6 @@ -import { setDiff } from './utils'; +import { AssertionError } from '@metamask/utils'; + +import { ensureRelative, setDiff } from './utils'; describe('setDiff', () => { it('does nothing on empty type {}-B', () => { @@ -29,3 +31,23 @@ describe('setDiff', () => { expect(setDiff(object, object)).toStrictEqual({}); }); }); + +describe('ensureRelative', () => { + it('throws on absolute paths', () => { + expect(() => ensureRelative('/foo/bar.js')).toThrow(AssertionError); + }); + + it('throws on URIs', () => { + expect(() => ensureRelative('http://foo.bar')).toThrow( + 'Path "http://foo.bar" potentially an URI instead of local relative', + ); + }); + + it('does nothing on "./" paths', () => { + expect(ensureRelative('./foo/bar.js')).toBe('./foo/bar.js'); + }); + + it('adds "./" if it\'s missing', () => { + expect(ensureRelative('foo/bar.js')).toBe('./foo/bar.js'); + }); +}); diff --git a/packages/snaps-utils/src/index.browser.ts b/packages/snaps-utils/src/index.browser.ts index b080b50c59..74312c31d7 100644 --- a/packages/snaps-utils/src/index.browser.ts +++ b/packages/snaps-utils/src/index.browser.ts @@ -10,4 +10,4 @@ export * from './object'; export * from './snaps'; export * from './types'; export * from './versions'; -export * from './vfile/index.browser'; +export * from './virtual-file/index.browser'; diff --git a/packages/snaps-utils/src/index.ts b/packages/snaps-utils/src/index.ts index bd449c50a2..58d0f9830a 100644 --- a/packages/snaps-utils/src/index.ts +++ b/packages/snaps-utils/src/index.ts @@ -16,4 +16,4 @@ export * from './post-process'; export * from './snaps'; export * from './types'; export * from './versions'; -export * from './vfile'; +export * from './virtual-file'; diff --git a/packages/snaps-utils/src/vfile/index.browser.ts b/packages/snaps-utils/src/vfile/index.browser.ts deleted file mode 100644 index 0550ddf949..0000000000 --- a/packages/snaps-utils/src/vfile/index.browser.ts +++ /dev/null @@ -1 +0,0 @@ -export * from './vfile'; diff --git a/packages/snaps-utils/src/vfile/index.ts b/packages/snaps-utils/src/vfile/index.ts deleted file mode 100644 index ed9878be00..0000000000 --- a/packages/snaps-utils/src/vfile/index.ts +++ /dev/null @@ -1,2 +0,0 @@ -export * from './to-vfile'; -export * from './vfile'; diff --git a/packages/snaps-utils/src/vfile/to-vfile.ts b/packages/snaps-utils/src/vfile/to-vfile.ts deleted file mode 100644 index bf58725ddb..0000000000 --- a/packages/snaps-utils/src/vfile/to-vfile.ts +++ /dev/null @@ -1,67 +0,0 @@ -import fs from 'fs'; -import fsPromises from 'fs/promises'; - -import { VFile } from './vfile'; - -/** - * Reads a file from filesystem and creates a vfile. - * - * @param path - Filesystem path to load the contents from. - * @param encoding - Optional encoding to pass down to fs.readFile. - * @returns Promise returning VFile with loaded file contents. - */ -export async function readVFile( - path: string, - encoding: BufferEncoding | null = null, -) { - return new VFile({ - path, - value: await fsPromises.readFile(path, { encoding }), - }); -} - -/** - * Reads a file from filesystem and creates a vfile synchronously. - * - * @param path - Filesystem path to load the contents from. - * @param encoding - Optional encoding to pass down to fs.readFile. - * @returns VFile with loaded file contents. - */ -export function readVFileSync( - path: string, - encoding: BufferEncoding | null = null, -) { - return new VFile({ - path, - value: fs.readFileSync(path, { encoding }), - }); -} - -type WriteVFileOptions = Exclude< - Parameters[2], - undefined ->; -type WriteVFileOptionsSync = Exclude< - Parameters[2], - undefined ->; - -/** - * Writes vfile to filesystem. - * - * @param vfile - The vfile to write. - * @param options - Options to pass down to fs.writeFile. - */ -export async function writeVFile(vfile: VFile, options?: WriteVFileOptions) { - return fsPromises.writeFile(vfile.path, vfile.value, options); -} - -/** - * Writes vfile to filesystem, synchronously. - * - * @param vfile - The vfile to write. - * @param options - Options to pass down to fs.writeFile. - */ -export function writeVFileSync(vfile: VFile, options?: WriteVFileOptionsSync) { - fs.writeFileSync(vfile.path, vfile.value, options); -} diff --git a/packages/snaps-utils/src/vfile/vfile.test.ts b/packages/snaps-utils/src/virtual-file/VirtualFile.test.ts similarity index 81% rename from packages/snaps-utils/src/vfile/vfile.test.ts rename to packages/snaps-utils/src/virtual-file/VirtualFile.test.ts index 11341eae6e..1930789440 100644 --- a/packages/snaps-utils/src/vfile/vfile.test.ts +++ b/packages/snaps-utils/src/virtual-file/VirtualFile.test.ts @@ -1,24 +1,24 @@ -import { VFile } from './vfile'; +import { VirtualFile } from './VirtualFile'; const VALUE = 'foo\nbar'; describe('VFile', () => { it('stores value string', () => { - const file = new VFile({ value: VALUE }); + const file = new VirtualFile({ value: VALUE }); expect(file.value).toBe(VALUE); expect(file.toString()).toStrictEqual(VALUE); }); it('stores value array', () => { const array = new TextEncoder().encode(VALUE); - const file = new VFile({ value: array }); + const file = new VirtualFile({ value: array }); expect(file.value).toStrictEqual(array); expect(file.toString()).toStrictEqual(VALUE); }); it('cant take options or string', () => { - const file1 = new VFile(VALUE); - const file2 = new VFile({ value: VALUE }); + const file1 = new VirtualFile(VALUE); + const file2 = new VirtualFile({ value: VALUE }); expect(file1.value).toStrictEqual(file2.value); expect(file1.value).toStrictEqual(VALUE); }); @@ -31,7 +31,7 @@ describe('VFile', () => { 0x00, 0x66, 0x00, 0x6f, 0x00, 0x6f, 0x00, 0x0a, 0x00, 0x62, 0x00, 0x61, 0x00, 0x72, ]); - const file = new VFile({ value: array }); + const file = new VirtualFile({ value: array }); expect(file.toString('utf-16be')).toStrictEqual(VALUE); }); }); @@ -42,7 +42,7 @@ describe('VFile', () => { const result = { foo: 'bar' }; const data = { bar: 'foo' }; const path = '/foo/bar'; - const file = new VFile({ value, result, data, path }); + const file = new VirtualFile({ value, result, data, path }); const file2 = file.clone(); file2.value = 'asd'; file2.result.foo = 'asd'; @@ -60,7 +60,7 @@ describe('VFile', () => { it('clones Uint8Array properly', () => { const array = new TextEncoder().encode(VALUE); - const file1 = new VFile({ value: array }); + const file1 = new VirtualFile({ value: array }); const file2 = file1.clone(); array[0] = 42; diff --git a/packages/snaps-utils/src/vfile/vfile.ts b/packages/snaps-utils/src/virtual-file/VirtualFile.ts similarity index 96% rename from packages/snaps-utils/src/vfile/vfile.ts rename to packages/snaps-utils/src/virtual-file/VirtualFile.ts index af1d5a7a75..57bcc68290 100644 --- a/packages/snaps-utils/src/vfile/vfile.ts +++ b/packages/snaps-utils/src/virtual-file/VirtualFile.ts @@ -17,7 +17,7 @@ import { deepClone } from '../deep-clone'; * This type can be augmented to register custom `data` types. * * @example - * declare module '@metamask/vfile' { + * declare module '@metamask/snaps-utils' { * interface DataMap { * // `file.data.name` is typed as `string` * name: string @@ -66,7 +66,7 @@ export function isTypedArray(value: unknown): value is TypedArray { return is(value, TypedArrayStruct); } -export class VFile { +export class VirtualFile { constructor(value?: Compatible) { let options: Options; if (typeof value === 'string' || isTypedArray(value)) { @@ -103,7 +103,7 @@ export class VFile { } clone() { - const vfile = new VFile(); + const vfile = new VirtualFile(); if (typeof this.value === 'string') { vfile.value = this.value; } else { diff --git a/packages/snaps-utils/src/virtual-file/index.browser.ts b/packages/snaps-utils/src/virtual-file/index.browser.ts new file mode 100644 index 0000000000..06eb287d5f --- /dev/null +++ b/packages/snaps-utils/src/virtual-file/index.browser.ts @@ -0,0 +1 @@ +export * from './VirtualFile'; diff --git a/packages/snaps-utils/src/virtual-file/index.ts b/packages/snaps-utils/src/virtual-file/index.ts new file mode 100644 index 0000000000..339ddf76ee --- /dev/null +++ b/packages/snaps-utils/src/virtual-file/index.ts @@ -0,0 +1,2 @@ +export * from './toVirtualFile'; +export * from './VirtualFile'; diff --git a/packages/snaps-utils/src/vfile/to-vfile.test.ts b/packages/snaps-utils/src/virtual-file/toVirtualFile.test.ts similarity index 61% rename from packages/snaps-utils/src/vfile/to-vfile.test.ts rename to packages/snaps-utils/src/virtual-file/toVirtualFile.test.ts index e426879561..68b42042b5 100644 --- a/packages/snaps-utils/src/vfile/to-vfile.test.ts +++ b/packages/snaps-utils/src/virtual-file/toVirtualFile.test.ts @@ -1,16 +1,10 @@ import { vol } from 'memfs'; -import { - readVFile, - readVFileSync, - writeVFile, - writeVFileSync, -} from './to-vfile'; -import { VFile } from './vfile'; +import { readVirtualFile, writeVirtualFile } from './toVirtualFile'; +import { VirtualFile } from './VirtualFile'; const CONTENTS_UTF8 = 'foo\nbar'; -jest.mock('fs'); jest.mock('fs/promises'); describe('to-vfile', () => { @@ -23,26 +17,28 @@ describe('to-vfile', () => { /* eslint-enable @typescript-eslint/naming-convention */ }); - describe.each([readVFile, readVFileSync])('readVFile', (testedFn) => { + describe('readVirtualFile', () => { it('reads file', async () => { - const file = await testedFn('/foo/utf-8.txt'); - expect(file).toBeInstanceOf(VFile); + const file = await readVirtualFile('/foo/utf-8.txt'); + expect(file).toBeInstanceOf(VirtualFile); expect(typeof file.value).not.toBe('string'); expect(file.toString()).toBe(CONTENTS_UTF8); }); it('decodes file', async () => { - const file = await testedFn('/foo/utf-8.txt', 'utf8'); - expect(file).toBeInstanceOf(VFile); + const file = await readVirtualFile('/foo/utf-8.txt', 'utf8'); + expect(file).toBeInstanceOf(VirtualFile); expect(typeof file.value).toBe('string'); expect(file.value).toBe(CONTENTS_UTF8); }); }); - describe.each([writeVFile, writeVFileSync])('writeVFile', (testedFn) => { + describe('writeVirtualFile', () => { it('writes files', async () => { const PATH = '/foo/out.txt'; - await testedFn(new VFile({ value: CONTENTS_UTF8, path: PATH })); + await writeVirtualFile( + new VirtualFile({ value: CONTENTS_UTF8, path: PATH }), + ); expect(vol.toJSON(PATH)).toStrictEqual( // eslint-disable-next-line @typescript-eslint/naming-convention diff --git a/packages/snaps-utils/src/virtual-file/toVirtualFile.ts b/packages/snaps-utils/src/virtual-file/toVirtualFile.ts new file mode 100644 index 0000000000..894cddeb40 --- /dev/null +++ b/packages/snaps-utils/src/virtual-file/toVirtualFile.ts @@ -0,0 +1,38 @@ +import fsPromises from 'fs/promises'; + +import { VirtualFile } from './VirtualFile'; + +/** + * Reads a file from filesystem and creates a vfile. + * + * @param path - Filesystem path to load the contents from. + * @param encoding - Optional encoding to pass down to fs.readFile. + * @returns Promise returning VFile with loaded file contents. + */ +export async function readVirtualFile( + path: string, + encoding: BufferEncoding | null = null, +) { + return new VirtualFile({ + path, + value: await fsPromises.readFile(path, { encoding }), + }); +} + +type WriteVFileOptions = Exclude< + Parameters[2], + undefined +>; + +/** + * Writes vfile to filesystem. + * + * @param vfile - The vfile to write. + * @param options - Options to pass down to fs.writeFile. + */ +export async function writeVirtualFile( + vfile: VirtualFile, + options?: WriteVFileOptions, +) { + return fsPromises.writeFile(vfile.path, vfile.value, options); +} From e1c1bd83950845d4ca60038312bc13b2619f3b5c Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Thu, 1 Dec 2022 13:41:40 +0100 Subject: [PATCH 10/15] Jest it up --- packages/snaps-utils/jest.config.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/snaps-utils/jest.config.js b/packages/snaps-utils/jest.config.js index 2b59d8f470..28aac828f2 100644 --- a/packages/snaps-utils/jest.config.js +++ b/packages/snaps-utils/jest.config.js @@ -6,8 +6,8 @@ module.exports = deepmerge(baseConfig, { coveragePathIgnorePatterns: [ './src/index.ts', './src/index.browser.ts', - './src/vfile/index.ts', - './src/vfile/index.browser.ts', + './src/virtual-file/index.ts', + './src/virtual-file/index.browser.ts', './src/manifest/index.ts', './src/manifest/index.browser.ts', './src/test-utils', @@ -18,10 +18,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 94.41, - functions: 98.95, - lines: 99.01, - statements: 99.01, + branches: 94.38, + functions: 98.93, + lines: 98.9, + statements: 98.9, }, }, testTimeout: 2500, From 251facc66e6cbd5c0f145bd54c02440ffbd2e778 Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Fri, 2 Dec 2022 13:35:02 +0100 Subject: [PATCH 11/15] TypedArray check now tracks types in VFile --- packages/snaps-utils/jest.config.js | 8 ++-- .../src/virtual-file/VirtualFile.ts | 41 ++++--------------- 2 files changed, 11 insertions(+), 38 deletions(-) diff --git a/packages/snaps-utils/jest.config.js b/packages/snaps-utils/jest.config.js index 28aac828f2..e1cb61249e 100644 --- a/packages/snaps-utils/jest.config.js +++ b/packages/snaps-utils/jest.config.js @@ -18,10 +18,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 94.38, - functions: 98.93, - lines: 98.9, - statements: 98.9, + branches: 94.36, + functions: 98.92, + lines: 98.99, + statements: 98.99, }, }, testTimeout: 2500, diff --git a/packages/snaps-utils/src/virtual-file/VirtualFile.ts b/packages/snaps-utils/src/virtual-file/VirtualFile.ts index 57bcc68290..2c65464549 100644 --- a/packages/snaps-utils/src/virtual-file/VirtualFile.ts +++ b/packages/snaps-utils/src/virtual-file/VirtualFile.ts @@ -1,16 +1,15 @@ // TODO(ritave): Move into separate package @metamask/vfile / @metamask/utils + @metamask/to-vfile when passes code review // TODO(ritave): Streaming vfile contents similar to vinyl maybe? -// TODO(ritave): Move fixing manifest to write messages to vfile similar to unified instead of throwing "ProgrammaticallyFixableErrors". -// Better yet, move manifest fixing into eslint fixing altogether +// TODO(ritave): Move fixing manifest in cli and bundler plugins to write messages to vfile +// similar to unified instead of throwing "ProgrammaticallyFixableErrors". +// +// Using https://github.com/vfile/vfile would be helpful, but they only support ESM and we need to support CommonJS. +// https://github.com/gulpjs/vinyl is also good, but they normalize paths, which we can't do, because +// we're calculating checksums based on original path. import { assert, hasProperty } from '@metamask/utils'; -import { Infer, instance, is, union } from 'superstruct'; import { deepClone } from '../deep-clone'; -// Using https://github.com/vfile/vfile would be helpful, but they only support ESM. -// https://github.com/gulpjs/vinyl is also good, but they normalize paths, which we can't do, because -// we're calculating checksums based on original path. - /** * This map registers the type of the `data` key of a `VFile`. * @@ -40,36 +39,10 @@ export type Options = { result?: Result; }; -const TypedArrayStruct = union([ - instance(Int8Array), - instance(Uint8Array), - instance(Uint8ClampedArray), - instance(Int16Array), - instance(Uint16Array), - instance(Int32Array), - instance(Uint32Array), - instance(Float32Array), - instance(Float64Array), - instance(BigInt64Array), - instance(BigUint64Array), -]); - -export type TypedArray = Infer; - -/** - * Returns whether the given parameter is one of TypedArray subtypes. - * - * @param value - Object to check. - * @returns Whether the parameter is TypeArray subtype. - */ -export function isTypedArray(value: unknown): value is TypedArray { - return is(value, TypedArrayStruct); -} - export class VirtualFile { constructor(value?: Compatible) { let options: Options; - if (typeof value === 'string' || isTypedArray(value)) { + if (typeof value === 'string' || value instanceof Uint8Array) { options = { value }; } else { options = value as Options; From 630fa5ce201d0608f4921f6c09efed1ec66a5253 Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Fri, 2 Dec 2022 14:52:25 +0100 Subject: [PATCH 12/15] PR review --- packages/snaps-controllers/jest.config.js | 8 ++-- .../src/snaps/location/local.ts | 4 +- .../src/snaps/location/location.test.ts | 2 +- .../src/snaps/location/location.ts | 2 +- .../src/test-utils/location.ts | 25 +++++-------- .../src/virtual-file/VirtualFile.test.ts | 2 +- .../src/virtual-file/VirtualFile.ts | 37 +++++++++++-------- 7 files changed, 40 insertions(+), 40 deletions(-) diff --git a/packages/snaps-controllers/jest.config.js b/packages/snaps-controllers/jest.config.js index c76ff3f295..3200d016cc 100644 --- a/packages/snaps-controllers/jest.config.js +++ b/packages/snaps-controllers/jest.config.js @@ -5,10 +5,10 @@ const baseConfig = require('../../jest.config.base'); module.exports = deepmerge(baseConfig, { coverageThreshold: { global: { - branches: 90.88, - functions: 97.46, - lines: 97.46, - statements: 97.46, + branches: 90.85, + functions: 97.45, + lines: 97.45, + statements: 97.45, }, }, projects: [ diff --git a/packages/snaps-controllers/src/snaps/location/local.ts b/packages/snaps-controllers/src/snaps/location/local.ts index 2921cd6b2b..a783c1d44c 100644 --- a/packages/snaps-controllers/src/snaps/location/local.ts +++ b/packages/snaps-controllers/src/snaps/location/local.ts @@ -41,7 +41,9 @@ export class LocalLocation implements SnapLocation { * @param vfile - The {@link VirtualFile} to convert. * @returns The same object with updated `.data.canonicalPath`. */ -function convertCanonical(vfile: VirtualFile): VirtualFile { +function convertCanonical( + vfile: VirtualFile, +): VirtualFile { assert(vfile.data.canonicalPath !== undefined); vfile.data.canonicalPath = `local:${vfile.data.canonicalPath}`; return vfile; diff --git a/packages/snaps-controllers/src/snaps/location/location.test.ts b/packages/snaps-controllers/src/snaps/location/location.test.ts index cfdb353c57..0bbb2cf1ed 100644 --- a/packages/snaps-controllers/src/snaps/location/location.test.ts +++ b/packages/snaps-controllers/src/snaps/location/location.test.ts @@ -8,7 +8,7 @@ describe('detectSnapLocation', () => { 'disallows http like protocols by default', (protocol) => { expect(() => detectSnapLocation(`${protocol}//127.0.0.1/`)).toThrow( - 'Fetching snaps from external http/https is disabled.', + 'Fetching snaps through http/https is disabled.', ); }, ); diff --git a/packages/snaps-controllers/src/snaps/location/location.ts b/packages/snaps-controllers/src/snaps/location/location.ts index ad0288117b..63c0f49a98 100644 --- a/packages/snaps-controllers/src/snaps/location/location.ts +++ b/packages/snaps-controllers/src/snaps/location/location.ts @@ -53,7 +53,7 @@ export function detectSnapLocation( case 'https:': assert( allowHttp, - new TypeError('Fetching snaps from external http/https is disabled.'), + new TypeError('Fetching snaps through http/https is disabled.'), ); return new HttpLocation(root); default: diff --git a/packages/snaps-controllers/src/test-utils/location.ts b/packages/snaps-controllers/src/test-utils/location.ts index 4a77427235..1bb06bc11d 100644 --- a/packages/snaps-controllers/src/test-utils/location.ts +++ b/packages/snaps-controllers/src/test-utils/location.ts @@ -15,7 +15,7 @@ type LoopbackOptions = { */ manifest?: SnapManifest | VirtualFile; /** - * @default [new VFile({ value: DEFAULT_SNAP_BUNDLE, path: manifest.source.location.npm.filePath }] + * @default [new VirtualFile({ value: DEFAULT_SNAP_BUNDLE, path: manifest.source.location.npm.filePath }] */ files?: VirtualFile[]; /** @@ -24,14 +24,6 @@ type LoopbackOptions = { shouldAlwaysReload?: boolean; }; -const isVfile = (obj: unknown): obj is VirtualFile => { - return ( - typeof obj === 'object' && - obj !== null && - typeof (obj as any).value === 'string' - ); -}; - export class LoopbackLocation implements SnapLocation { #manifest: VirtualFile; @@ -41,13 +33,14 @@ export class LoopbackLocation implements SnapLocation { constructor(opts: LoopbackOptions = {}) { const shouldAlwaysReload = opts.shouldAlwaysReload ?? false; - const manifest = isVfile(opts.manifest) - ? opts.manifest - : new VirtualFile({ - value: '', - result: opts.manifest ?? getSnapManifest(), - path: './snap.manifest.json', - }); + const manifest = + opts.manifest instanceof VirtualFile + ? opts.manifest + : new VirtualFile({ + value: '', + result: opts.manifest ?? getSnapManifest(), + path: './snap.manifest.json', + }); let files; if (opts.files === undefined) { files = [ diff --git a/packages/snaps-utils/src/virtual-file/VirtualFile.test.ts b/packages/snaps-utils/src/virtual-file/VirtualFile.test.ts index 1930789440..7c1b2565a3 100644 --- a/packages/snaps-utils/src/virtual-file/VirtualFile.test.ts +++ b/packages/snaps-utils/src/virtual-file/VirtualFile.test.ts @@ -2,7 +2,7 @@ import { VirtualFile } from './VirtualFile'; const VALUE = 'foo\nbar'; -describe('VFile', () => { +describe('VirtualFile', () => { it('stores value string', () => { const file = new VirtualFile({ value: VALUE }); expect(file.value).toBe(VALUE); diff --git a/packages/snaps-utils/src/virtual-file/VirtualFile.ts b/packages/snaps-utils/src/virtual-file/VirtualFile.ts index 2c65464549..3fb3dbce80 100644 --- a/packages/snaps-utils/src/virtual-file/VirtualFile.ts +++ b/packages/snaps-utils/src/virtual-file/VirtualFile.ts @@ -6,12 +6,12 @@ // Using https://github.com/vfile/vfile would be helpful, but they only support ESM and we need to support CommonJS. // https://github.com/gulpjs/vinyl is also good, but they normalize paths, which we can't do, because // we're calculating checksums based on original path. -import { assert, hasProperty } from '@metamask/utils'; +import { assert } from '@metamask/utils'; import { deepClone } from '../deep-clone'; /** - * This map registers the type of the `data` key of a `VFile`. + * This map registers the type of the {@link VirtualFile.data} key of a {@link VirtualFile}. * * This type can be augmented to register custom `data` types. * @@ -41,30 +41,35 @@ export type Options = { export class VirtualFile { constructor(value?: Compatible) { - let options: Options; + let options: Options | undefined; if (typeof value === 'string' || value instanceof Uint8Array) { options = { value }; } else { - options = value as Options; + options = value; } - for (const prop in options) { - if ( - hasProperty(options, prop) && - options[prop as keyof Options] !== undefined - ) { - this[prop as keyof Options] = options[prop as keyof Options] as any; - } - } + this.value = options?.value ?? ''; + // This situations happens when there's no .result used, + // we expect the file to have default generic in that situation: + // VirtualFile which will handle undefined properly + // + // While not 100% type safe, it'll be way less frustrating to work with. + // The alternative would be to have VirtualFile.result be Result | undefined + // and that would result in needing to branch out and check in all situations. + // + // In short, optimizing for most common use case. + this.result = options?.result ?? (undefined as any); + this.data = options?.data ?? {}; + this.path = options?.path ?? '/'; } - value!: Value; + value: Value; - result!: Result; + result: Result; - data: Data = {}; + data: Data; - path = '/'; + path: string; toString(encoding?: string) { if (typeof this.value === 'string') { From 0c88727c64174c1f88a9ab6f81edf2fb298d287d Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Mon, 5 Dec 2022 09:31:44 +0100 Subject: [PATCH 13/15] Fix snap.manifest.json fetchin from local: ids Also added no-cache --- .../src/snaps/SnapController.ts | 2 ++ .../src/snaps/location/http.test.ts | 13 ++++++++ .../src/snaps/location/http.ts | 16 +++++++-- .../src/snaps/location/local.test.ts | 33 +++++++++++++++++++ .../src/snaps/location/local.ts | 8 ++++- 5 files changed, 68 insertions(+), 4 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 7993ca0b32..1f44aec48f 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -2067,6 +2067,8 @@ export class SnapController extends BaseController< return { manifest, files, location }; } catch (error) { + // TODO(ritave): Export `getErrorMessage()` from @metamask/utils and use it here + // https://github.com/MetaMask/utils/blob/62d022ef83c91fa4d150e51913be4441508a0ab1/src/assert.ts const message = error instanceof Error ? error.message : error.toString(); throw new Error(`Failed to fetch Snap "${snapId}": ${message}.`); } diff --git a/packages/snaps-controllers/src/snaps/location/http.test.ts b/packages/snaps-controllers/src/snaps/location/http.test.ts index 5b66cb2613..6c4262eb4d 100644 --- a/packages/snaps-controllers/src/snaps/location/http.test.ts +++ b/packages/snaps-controllers/src/snaps/location/http.test.ts @@ -43,4 +43,17 @@ describe('HttpLocation', () => { const file = await new HttpLocation(new URL(base)).fetch('./foo.js'); expect(file.data.canonicalPath).toBe(canonical); }); + + it.each([ + ['http://foo.bar/foo', 'http://foo.bar/snap.manifest.json'], + ['https://foo.bar/foo', 'https://foo.bar/snap.manifest.json'], + ['http://foo.bar/foo/', 'http://foo.bar/foo/snap.manifest.json'], + ])('fetches manifest from proper location', async (base, actuallyFetched) => { + fetchMock.mockResponses(JSON.stringify(getSnapManifest())); + + await new HttpLocation(new URL(base)).manifest(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock).toHaveBeenNthCalledWith(1, actuallyFetched, undefined); + }); }); diff --git a/packages/snaps-controllers/src/snaps/location/http.ts b/packages/snaps-controllers/src/snaps/location/http.ts index 7b30031e7a..2d3f2c4cfe 100644 --- a/packages/snaps-controllers/src/snaps/location/http.ts +++ b/packages/snaps-controllers/src/snaps/location/http.ts @@ -3,6 +3,7 @@ import { assertIsSnapManifest, VirtualFile, HttpSnapIdStruct, + NpmSnapFileNames, } from '@metamask/snaps-utils'; import { assert, assertStruct } from '@metamask/utils'; @@ -14,6 +15,7 @@ export interface HttpOptions { * @default fetch */ fetch?: typeof fetch; + fetchOptions?: RequestInit; } export class HttpLocation implements SnapLocation { @@ -34,9 +36,12 @@ export class HttpLocation implements SnapLocation { private readonly fetchFn: typeof fetch; + private readonly fetchOptions?: RequestInit; + constructor(url: URL, opts: HttpOptions = {}) { assertStruct(url.toString(), HttpSnapIdStruct, 'Invalid Snap Id: '); this.fetchFn = opts.fetch ?? globalThis.fetch; + this.fetchOptions = opts.fetchOptions; this.url = url; } @@ -45,8 +50,13 @@ export class HttpLocation implements SnapLocation { return this.validatedManifest.clone(); } - // jest-fetch-mock doesn't handle new URL(), we need to convert this.url.toString() - const contents = await (await this.fetchFn(this.url.toString())).text(); + // jest-fetch-mock doesn't handle new URL(), we need to convert .toString() + const contents = await ( + await this.fetchFn( + new URL(NpmSnapFileNames.Manifest, this.url).toString(), + this.fetchOptions, + ) + ).text(); const manifest = JSON.parse(contents); assertIsSnapManifest(manifest); const vfile = new VirtualFile({ @@ -72,7 +82,7 @@ export class HttpLocation implements SnapLocation { } const canonicalPath = this.toCanonical(relativePath).toString(); - const response = await this.fetchFn(canonicalPath); + const response = await this.fetchFn(canonicalPath, this.fetchOptions); const vfile = new VirtualFile({ value: '', path: relativePath, diff --git a/packages/snaps-controllers/src/snaps/location/local.test.ts b/packages/snaps-controllers/src/snaps/location/local.test.ts index 8bc2208d45..4cc835873b 100644 --- a/packages/snaps-controllers/src/snaps/location/local.test.ts +++ b/packages/snaps-controllers/src/snaps/location/local.test.ts @@ -46,4 +46,37 @@ describe('LocalLocation', () => { const file = await new LocalLocation(new URL(base)).fetch('./foo.js'); expect(file.data.canonicalPath).toBe(canonical); }); + + it.each([ + ['local:http://localhost/foo', 'http://localhost/snap.manifest.json'], + ['local:https://localhost/foo', 'https://localhost/snap.manifest.json'], + ['local:http://localhost/foo/', 'http://localhost/foo/snap.manifest.json'], + ])('fetches manifest from proper location', async (base, actuallyFetched) => { + fetchMock.mockResponses(JSON.stringify(getSnapManifest())); + + await new LocalLocation(new URL(base)).manifest(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock).toHaveBeenNthCalledWith( + 1, + actuallyFetched, + expect.anything(), + ); + }); + + it.each(['local:http://localhost', 'local:https://localhost'])( + 'fetches with caching disabled', + async (url) => { + fetchMock.mockResponses(DEFAULT_SNAP_BUNDLE); + + await new LocalLocation(new URL(url)).fetch('./foo.js'); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock).toHaveBeenNthCalledWith( + 1, + expect.anything(), + expect.objectContaining({ cache: 'no-cache' }), + ); + }, + ); }); diff --git a/packages/snaps-controllers/src/snaps/location/local.ts b/packages/snaps-controllers/src/snaps/location/local.ts index a783c1d44c..797e68d077 100644 --- a/packages/snaps-controllers/src/snaps/location/local.ts +++ b/packages/snaps-controllers/src/snaps/location/local.ts @@ -14,9 +14,15 @@ export class LocalLocation implements SnapLocation { constructor(url: URL, opts: HttpOptions = {}) { assertStruct(url.toString(), LocalSnapIdStruct, 'Invalid Snap Id'); + // TODO(ritave): Write deepMerge() which merges fetchOptions. + assert( + opts.fetchOptions === undefined, + 'Currently adding fetch options to local: is unsupported.', + ); + this.#http = new HttpLocation( new URL(url.toString().slice(SnapIdPrefixes.local.length)), - opts, + { ...opts, fetchOptions: { cache: 'no-cache' } }, ); } From 5ef0e974b1ed25cc9b48f5d2276c650d102eccf2 Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Mon, 5 Dec 2022 13:11:35 +0100 Subject: [PATCH 14/15] Fix SnapLocation canonical paths being set wrong --- .../src/snaps/location/http.test.ts | 33 +++++++++++++--- .../src/snaps/location/http.ts | 14 ++++--- .../src/snaps/location/local.test.ts | 38 +++++++++++++++---- 3 files changed, 66 insertions(+), 19 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/location/http.test.ts b/packages/snaps-controllers/src/snaps/location/http.test.ts index 6c4262eb4d..fe094a6c7a 100644 --- a/packages/snaps-controllers/src/snaps/location/http.test.ts +++ b/packages/snaps-controllers/src/snaps/location/http.test.ts @@ -35,13 +35,34 @@ describe('HttpLocation', () => { }); it.each([ - ['http://foo.bar/foo', 'http://foo.bar/foo.js'], - ['http://foo.bar/foo/', 'http://foo.bar/foo/foo.js'], - ])('sets canonical path properly', async (base, canonical) => { - fetchMock.mockResponses(DEFAULT_SNAP_BUNDLE); + [ + 'http://foo.bar/foo', + { + manifest: 'http://foo.bar/snap.manifest.json', + bundle: 'http://foo.bar/foo.js', + }, + ], + [ + 'http://foo.bar/foo/', + { + manifest: 'http://foo.bar/foo/snap.manifest.json', + bundle: 'http://foo.bar/foo/foo.js', + }, + ], + ])('sets paths properly', async (base, canonical) => { + fetchMock.mockResponses( + JSON.stringify(getSnapManifest()), + DEFAULT_SNAP_BUNDLE, + ); + + const location = new HttpLocation(new URL(base)); + const manifest = await location.manifest(); + const bundle = await location.fetch('./foo.js'); - const file = await new HttpLocation(new URL(base)).fetch('./foo.js'); - expect(file.data.canonicalPath).toBe(canonical); + expect(manifest.path).toBe('./snap.manifest.json'); + expect(bundle.path).toBe('./foo.js'); + expect(manifest.data.canonicalPath).toBe(canonical.manifest); + expect(bundle.data.canonicalPath).toBe(canonical.bundle); }); it.each([ diff --git a/packages/snaps-controllers/src/snaps/location/http.ts b/packages/snaps-controllers/src/snaps/location/http.ts index 2d3f2c4cfe..7a1aafd34f 100644 --- a/packages/snaps-controllers/src/snaps/location/http.ts +++ b/packages/snaps-controllers/src/snaps/location/http.ts @@ -51,19 +51,21 @@ export class HttpLocation implements SnapLocation { } // jest-fetch-mock doesn't handle new URL(), we need to convert .toString() + const canonicalPath = new URL( + NpmSnapFileNames.Manifest, + this.url, + ).toString(); + const contents = await ( - await this.fetchFn( - new URL(NpmSnapFileNames.Manifest, this.url).toString(), - this.fetchOptions, - ) + await this.fetchFn(canonicalPath, this.fetchOptions) ).text(); const manifest = JSON.parse(contents); assertIsSnapManifest(manifest); const vfile = new VirtualFile({ value: contents, result: manifest, - path: './snap.manifest.json', - data: { canonicalPath: this.url.toString() }, + path: `./${NpmSnapFileNames.Manifest}`, + data: { canonicalPath }, }); this.validatedManifest = vfile; diff --git a/packages/snaps-controllers/src/snaps/location/local.test.ts b/packages/snaps-controllers/src/snaps/location/local.test.ts index 4cc835873b..65afe338f5 100644 --- a/packages/snaps-controllers/src/snaps/location/local.test.ts +++ b/packages/snaps-controllers/src/snaps/location/local.test.ts @@ -35,16 +35,40 @@ describe('LocalLocation', () => { }); it.each([ - ['local:http://localhost/foo', 'local:http://localhost/foo.js'], - ['local:http://127.0.0.1/foo/', 'local:http://127.0.0.1/foo/foo.js'], + [ + 'local:http://localhost/foo', + { + manifest: 'local:http://localhost/snap.manifest.json', + bundle: 'local:http://localhost/foo.js', + }, + ], + [ + 'local:http://127.0.0.1/foo/', + { + manifest: 'local:http://127.0.0.1/foo/snap.manifest.json', + bundle: 'local:http://127.0.0.1/foo/foo.js', + }, + ], [ 'local:https://user:pass@[::1]:8080/foo/bar', - 'local:https://user:pass@[::1]:8080/foo/foo.js', + { + manifest: 'local:https://user:pass@[::1]:8080/foo/snap.manifest.json', + bundle: 'local:https://user:pass@[::1]:8080/foo/foo.js', + }, ], - ])('sets canonical path properly', async (base, canonical) => { - fetchMock.mockResponses(DEFAULT_SNAP_BUNDLE); - const file = await new LocalLocation(new URL(base)).fetch('./foo.js'); - expect(file.data.canonicalPath).toBe(canonical); + ])('sets paths properly for %s', async (base, canonical) => { + fetchMock.mockResponses( + JSON.stringify(getSnapManifest()), + DEFAULT_SNAP_BUNDLE, + ); + const location = new LocalLocation(new URL(base)); + const manifest = await location.manifest(); + const bundle = await location.fetch('./foo.js'); + + expect(manifest.path).toBe('./snap.manifest.json'); + expect(bundle.path).toBe('./foo.js'); + expect(manifest.data.canonicalPath).toBe(canonical.manifest); + expect(bundle.data.canonicalPath).toBe(canonical.bundle); }); it.each([ From c51b44352dc9e185215d974a1da2d0fba4fdfd23 Mon Sep 17 00:00:00 2001 From: Olaf Tomalka Date: Mon, 5 Dec 2022 13:32:49 +0100 Subject: [PATCH 15/15] Naming --- packages/snaps-utils/src/virtual-file/toVirtualFile.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-utils/src/virtual-file/toVirtualFile.test.ts b/packages/snaps-utils/src/virtual-file/toVirtualFile.test.ts index 68b42042b5..56b967c626 100644 --- a/packages/snaps-utils/src/virtual-file/toVirtualFile.test.ts +++ b/packages/snaps-utils/src/virtual-file/toVirtualFile.test.ts @@ -7,7 +7,7 @@ const CONTENTS_UTF8 = 'foo\nbar'; jest.mock('fs/promises'); -describe('to-vfile', () => { +describe('toVirtualFile', () => { beforeEach(() => { vol.reset(); /* eslint-disable @typescript-eslint/naming-convention */