From 25ffa87b6c99ea7d1fdb250208fdf6fd5dc42ce4 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 14 Dec 2023 00:49:23 +0100 Subject: [PATCH 1/7] Refactor NPM location class --- .../src/snaps/location/npm.ts | 235 ++++++++++-------- 1 file changed, 136 insertions(+), 99 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts index f21ab1c94a..1f102cc15c 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -50,7 +50,8 @@ export interface NpmOptions { allowCustomRegistries?: boolean; } -export class NpmLocation implements SnapLocation { +// Base class for NPM implementation, useful for extending with custom NPM fetching logic +export abstract class BaseNpmLocation implements SnapLocation { private readonly meta: NpmMeta; private validatedManifest?: VirtualFile; @@ -169,69 +170,140 @@ export class NpmLocation implements SnapLocation { const resolvedVersion = await this.meta.resolveVersion( this.meta.requestedRange, ); - const [tarballResponse, actualVersion] = await fetchNpmTarball( + + const [files, actualVersion] = await this.fetchNpmTarball( this.meta.packageName, resolvedVersion, this.meta.registry, this.meta.fetch, ); + + this.files = files; 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; + /** + * Fetches the tarball (`.tgz` file) of the specified package and version from + * an 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 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 files for the package tarball and the + * actual version of the package. + * @throws If fetching the tarball fails. + */ + abstract fetchNpmTarball( + packageName: string, + versionRange: SemVerRange, + registryUrl: URL, + fetchFunction: typeof fetch, + ): Promise<[Map, SemVerVersion]>; +} - // 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 = new Map(); +// Safety limit for tarballs, 250 MB in bytes +const TARBALL_SIZE_SAFETY_LIMIT = 262144000; - const tarballStream = createTarballStream( - `${canonicalBase}/${this.meta.packageName}/`, - this.files, +// Main NPM implementation, contains a browser tarball fetching implementation. +export class NpmLocation extends BaseNpmLocation { + /** + * Fetches the tarball (`.tgz` file) of the specified package and version from + * an 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 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 files for the package tarball and the + * actual version of the package. + * @throws If fetching the tarball fails. + */ + async fetchNpmTarball( + packageName: string, + versionRange: SemVerRange, + registryUrl: URL, + fetchFunction: typeof fetch, + ): Promise<[Map>, SemVerVersion]> { + const { tarballURL, targetVersion } = await resolveNpmVersion( + packageName, + versionRange, + registryUrl, + fetchFunction, + ); + + if (!isValidUrl(tarballURL) || !tarballURL.toString().endsWith('.tgz')) { + throw new Error( + `Failed to find valid tarball URL in NPM metadata for package "${packageName}".`, ); + } - // The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed - // before we can actually grab any files from it. - // To prevent recursion-based zip bombs, we should not allow recursion here. + // Override the tarball hostname/protocol with registryUrl hostname/protocol + const newTarballUrl = new URL(tarballURL); + newTarballUrl.hostname = registryUrl.hostname; + newTarballUrl.protocol = registryUrl.protocol; + + // Perform a raw fetch because we want the Response object itself. + const tarballResponse = await fetchFunction(newTarballUrl.toString()); + if (!tarballResponse.ok || !tarballResponse.body) { + throw new Error(`Failed to fetch tarball for package "${packageName}".`); + } + // We assume that NPM is a good actor and provides us with a valid `content-length` header. + const tarballSizeString = tarballResponse.headers.get('content-length'); + assert(tarballSizeString, 'Snap tarball has invalid content-length'); + const tarballSize = parseInt(tarballSizeString, 10); + assert( + tarballSize <= TARBALL_SIZE_SAFETY_LIMIT, + 'Snap tarball exceeds size limit', + ); + const tarballResponseBody = tarballResponse.body; + return [ + await new Promise((resolve, reject) => { + const files = new Map(); + + const tarballStream = createTarballStream( + getNpmCanonicalBasePath(registryUrl, packageName), + files, + ); - // If native decompression stream is available we use that, otherwise fallback to zlib - if ('DecompressionStream' in globalThis) { - const decompressionStream = new DecompressionStream('gzip'); - const decompressedStream = - tarballResponse.pipeThrough(decompressionStream); + // The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed + // before we can actually grab any files from it. + // To prevent recursion-based zip bombs, we should not allow recursion here. + + // If native decompression stream is available we use that, otherwise fallback to zlib + if ('DecompressionStream' in globalThis) { + const decompressionStream = new DecompressionStream('gzip'); + const decompressedStream = + tarballResponseBody.pipeThrough(decompressionStream); + + pipeline( + getNodeStream(decompressedStream), + tarballStream, + (error: unknown) => { + error ? reject(error) : resolve(files); + }, + ); + return; + } pipeline( - getNodeStream(decompressedStream), + getNodeStream(tarballResponseBody), + createGunzip(), tarballStream, (error: unknown) => { - error ? reject(error) : resolve(); + error ? reject(error) : resolve(files); }, ); - return; - } - - pipeline( - getNodeStream(tarballResponse), - createGunzip(), - tarballStream, - (error: unknown) => { - error ? reject(error) : resolve(); - }, - ); - }); + }), + targetVersion, + ]; } } -// Safety limit for tarballs, 250 MB in bytes -const TARBALL_SIZE_SAFETY_LIMIT = 262144000; - // Incomplete type export type PartialNpmMetadata = { versions: Record; @@ -280,6 +352,26 @@ export async function fetchNpmMetadata( return packageMetadata as PartialNpmMetadata; } +/** + * Gets the canonical base path for an NPM snap. + * + * @param registryUrl - A registry URL. + * @param packageName - A package name. + * @returns The canonical base path. + */ +export function getNpmCanonicalBasePath(registryUrl: URL, packageName: string) { + let canonicalBase = 'npm://'; + if (registryUrl.username !== '') { + canonicalBase += registryUrl.username; + if (registryUrl.password !== '') { + canonicalBase += `:${registryUrl.password}`; + } + canonicalBase += '@'; + } + canonicalBase += registryUrl.host; + return `${canonicalBase}${registryUrl.host}/${packageName}/`; +} + /** * Determine if a registry URL is NPM. * @@ -303,7 +395,7 @@ function isNPM(registryUrl: URL) { * @returns An object containing the resolved version and a URL for its tarball. * @throws If fetching the metadata fails. */ -async function resolveNpmVersion( +export async function resolveNpmVersion( packageName: string, versionRange: SemVerRange, registryUrl: URL, @@ -343,61 +435,6 @@ async function resolveNpmVersion( return { tarballURL, targetVersion }; } -/** - * Fetches the tarball (`.tgz` file) of the specified package and version 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 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. - * @throws If fetching the tarball fails. - */ -async function fetchNpmTarball( - packageName: string, - versionRange: SemVerRange, - registryUrl: URL, - fetchFunction: typeof fetch, -): Promise<[ReadableStream, SemVerVersion]> { - const { tarballURL, targetVersion } = await resolveNpmVersion( - packageName, - versionRange, - registryUrl, - fetchFunction, - ); - - if (!isValidUrl(tarballURL) || !tarballURL.toString().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(tarballURL); - 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 || !tarballResponse.body) { - throw new Error(`Failed to fetch tarball for package "${packageName}".`); - } - // We assume that NPM is a good actor and provides us with a valid `content-length` header. - const tarballSizeString = tarballResponse.headers.get('content-length'); - assert(tarballSizeString, 'Snap tarball has invalid content-length'); - const tarballSize = parseInt(tarballSizeString, 10); - assert( - tarballSize <= TARBALL_SIZE_SAFETY_LIMIT, - 'Snap tarball exceeds size limit', - ); - return [tarballResponse.body, targetVersion]; -} - /** * The paths of files within npm tarballs appear to always be prefixed with * "package/". From 722a525ed9615dffbc3f76e2f8b2a85868e6eaba Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 14 Dec 2023 11:01:05 +0100 Subject: [PATCH 2/7] Update coverage --- packages/snaps-controllers/coverage.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index d5f5c7edc3..26a3db3b9a 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { "branches": 90.09, - "functions": 96.35, - "lines": 97.31, - "statements": 96.99 + "functions": 96.36, + "lines": 97.12, + "statements": 96.81 } From 9e1c48022fcc99f4e2983ba45079dc7334a8718c Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 14 Dec 2023 13:25:27 +0100 Subject: [PATCH 3/7] More refactoring --- packages/snaps-controllers/coverage.json | 4 +- .../src/snaps/location/npm.ts | 164 ++++++++---------- 2 files changed, 70 insertions(+), 98 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 26a3db3b9a..93553eebd6 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { "branches": 90.09, "functions": 96.36, - "lines": 97.12, - "statements": 96.81 + "lines": 97.18, + "statements": 96.87 } diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts index 1f102cc15c..3213b0f9c7 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -52,11 +52,11 @@ export interface NpmOptions { // Base class for NPM implementation, useful for extending with custom NPM fetching logic export abstract class BaseNpmLocation implements SnapLocation { - private readonly meta: NpmMeta; + protected readonly meta: NpmMeta; - private validatedManifest?: VirtualFile; + #validatedManifest?: VirtualFile; - private files?: Map; + #files?: Map; constructor(url: URL, opts: DetectSnapLocationOptions = {}) { const allowCustomRegistries = opts.allowCustomRegistries ?? false; @@ -119,25 +119,25 @@ export abstract class BaseNpmLocation implements SnapLocation { } async manifest(): Promise> { - if (this.validatedManifest) { - return this.validatedManifest.clone(); + if (this.#validatedManifest) { + return this.#validatedManifest.clone(); } const vfile = await this.fetch('snap.manifest.json'); const result = parseJson(vfile.toString()); vfile.result = createSnapManifest(result); - this.validatedManifest = vfile as VirtualFile; + this.#validatedManifest = vfile as VirtualFile; return this.manifest(); } async fetch(path: string): Promise { const relativePath = normalizeRelative(path); - if (!this.files) { + if (!this.#files) { await this.#lazyInit(); - assert(this.files !== undefined); + assert(this.#files !== undefined); } - const vfile = this.files.get(relativePath); + const vfile = this.#files.get(relativePath); assert( vfile !== undefined, new TypeError(`File "${path}" not found in package.`), @@ -166,42 +166,43 @@ export abstract class BaseNpmLocation implements SnapLocation { } async #lazyInit() { - assert(this.files === undefined); + assert(this.#files === undefined); const resolvedVersion = await this.meta.resolveVersion( this.meta.requestedRange, ); - const [files, actualVersion] = await this.fetchNpmTarball( + const { tarballURL, targetVersion } = await resolveNpmVersion( this.meta.packageName, resolvedVersion, this.meta.registry, this.meta.fetch, ); - this.files = files; - this.meta.version = actualVersion; + if (!isValidUrl(tarballURL) || !tarballURL.toString().endsWith('.tgz')) { + throw new Error( + `Failed to find valid tarball URL in NPM metadata for package "${this.meta.packageName}".`, + ); + } + + // Override the tarball hostname/protocol with registryUrl hostname/protocol + const newTarballUrl = new URL(tarballURL); + newTarballUrl.hostname = this.meta.registry.hostname; + newTarballUrl.protocol = this.meta.registry.protocol; + + const files = await this.fetchNpmTarball(newTarballUrl); + + this.#files = files; + this.meta.version = targetVersion; } /** - * Fetches the tarball (`.tgz` file) of the specified package and version from - * an npm registry. + * Fetches and unpacks the tarball (`.tgz` file) from the specified URL. * - * @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 files for the package tarball and the - * actual version of the package. + * @param tarballUrl - The tarball URL to fetch and unpack. + * @returns A the files for the package tarball. * @throws If fetching the tarball fails. */ - abstract fetchNpmTarball( - packageName: string, - versionRange: SemVerRange, - registryUrl: URL, - fetchFunction: typeof fetch, - ): Promise<[Map, SemVerVersion]>; + abstract fetchNpmTarball(tarballUrl: URL): Promise>; } // Safety limit for tarballs, 250 MB in bytes @@ -210,47 +211,21 @@ const TARBALL_SIZE_SAFETY_LIMIT = 262144000; // Main NPM implementation, contains a browser tarball fetching implementation. export class NpmLocation extends BaseNpmLocation { /** - * Fetches the tarball (`.tgz` file) of the specified package and version from - * an npm registry. + * Fetches and unpacks the tarball (`.tgz` file) from the specified URL. * - * @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 files for the package tarball and the - * actual version of the package. + * @param tarballUrl - The tarball URL to fetch and unpack. + * @returns A the files for the package tarball. * @throws If fetching the tarball fails. */ async fetchNpmTarball( - packageName: string, - versionRange: SemVerRange, - registryUrl: URL, - fetchFunction: typeof fetch, - ): Promise<[Map>, SemVerVersion]> { - const { tarballURL, targetVersion } = await resolveNpmVersion( - packageName, - versionRange, - registryUrl, - fetchFunction, - ); - - if (!isValidUrl(tarballURL) || !tarballURL.toString().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 newTarballUrl = new URL(tarballURL); - newTarballUrl.hostname = registryUrl.hostname; - newTarballUrl.protocol = registryUrl.protocol; - + tarballUrl: URL, + ): Promise>> { // Perform a raw fetch because we want the Response object itself. - const tarballResponse = await fetchFunction(newTarballUrl.toString()); + const tarballResponse = await this.meta.fetch(tarballUrl.toString()); if (!tarballResponse.ok || !tarballResponse.body) { - throw new Error(`Failed to fetch tarball for package "${packageName}".`); + throw new Error( + `Failed to fetch tarball for package "${this.meta.packageName}".`, + ); } // We assume that NPM is a good actor and provides us with a valid `content-length` header. const tarballSizeString = tarballResponse.headers.get('content-length'); @@ -261,46 +236,43 @@ export class NpmLocation extends BaseNpmLocation { 'Snap tarball exceeds size limit', ); const tarballResponseBody = tarballResponse.body; - return [ - await new Promise((resolve, reject) => { - const files = new Map(); + return new Promise((resolve, reject) => { + const files = new Map(); - const tarballStream = createTarballStream( - getNpmCanonicalBasePath(registryUrl, packageName), - files, - ); + const tarballStream = createTarballStream( + getNpmCanonicalBasePath(this.meta.registry, this.meta.packageName), + files, + ); - // The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed - // before we can actually grab any files from it. - // To prevent recursion-based zip bombs, we should not allow recursion here. - - // If native decompression stream is available we use that, otherwise fallback to zlib - if ('DecompressionStream' in globalThis) { - const decompressionStream = new DecompressionStream('gzip'); - const decompressedStream = - tarballResponseBody.pipeThrough(decompressionStream); - - pipeline( - getNodeStream(decompressedStream), - tarballStream, - (error: unknown) => { - error ? reject(error) : resolve(files); - }, - ); - return; - } + // The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed + // before we can actually grab any files from it. + // To prevent recursion-based zip bombs, we should not allow recursion here. + + // If native decompression stream is available we use that, otherwise fallback to zlib + if ('DecompressionStream' in globalThis) { + const decompressionStream = new DecompressionStream('gzip'); + const decompressedStream = + tarballResponseBody.pipeThrough(decompressionStream); pipeline( - getNodeStream(tarballResponseBody), - createGunzip(), + getNodeStream(decompressedStream), tarballStream, (error: unknown) => { error ? reject(error) : resolve(files); }, ); - }), - targetVersion, - ]; + return; + } + + pipeline( + getNodeStream(tarballResponseBody), + createGunzip(), + tarballStream, + (error: unknown) => { + error ? reject(error) : resolve(files); + }, + ); + }); } } @@ -395,7 +367,7 @@ function isNPM(registryUrl: URL) { * @returns An object containing the resolved version and a URL for its tarball. * @throws If fetching the metadata fails. */ -export async function resolveNpmVersion( +async function resolveNpmVersion( packageName: string, versionRange: SemVerRange, registryUrl: URL, From 1874169e21e8eb87f441332093cb93bad0ce2c2a Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 14 Dec 2023 13:53:50 +0100 Subject: [PATCH 4/7] Improve testing --- packages/snaps-controllers/coverage.json | 6 +- .../src/snaps/location/npm.test.ts | 87 +++++++++++++++++-- .../src/snaps/location/npm.ts | 1 - 3 files changed, 85 insertions(+), 9 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 93553eebd6..34212c37e7 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { - "branches": 90.09, + "branches": 90.8, "functions": 96.36, - "lines": 97.18, - "statements": 96.87 + "lines": 97.63, + "statements": 97.37 } diff --git a/packages/snaps-controllers/src/snaps/location/npm.test.ts b/packages/snaps-controllers/src/snaps/location/npm.test.ts index 2151a5fb8c..96a63d7169 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.test.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.test.ts @@ -5,7 +5,11 @@ import fetchMock from 'jest-fetch-mock'; import path from 'path'; import { Readable } from 'stream'; -import { NpmLocation } from './npm'; +import { + DEFAULT_NPM_REGISTRY, + NpmLocation, + getNpmCanonicalBasePath, +} from './npm'; fetchMock.enableMocks(); @@ -68,9 +72,17 @@ describe('NpmLocation', () => { ); const manifest = await location.manifest(); - const sourceCode = ( - await location.fetch(manifest.result.source.location.npm.filePath) - ).toString(); + expect(manifest.path).toBe('snap.manifest.json'); + expect(manifest.data.canonicalPath).toBe( + 'npm://registry.npmjs.cf/@metamask/template-snap/snap.manifest.json', + ); + const sourceCode = await location.fetch( + manifest.result.source.location.npm.filePath, + ); + expect(sourceCode.path).toBe('dist/bundle.js'); + expect(sourceCode.data.canonicalPath).toBe( + 'npm://registry.npmjs.cf/@metamask/template-snap/dist/bundle.js', + ); assert(manifest.result.source.location.npm.iconPath); const svgIcon = ( await location.fetch(manifest.result.source.location.npm.iconPath) @@ -94,7 +106,7 @@ describe('NpmLocation', () => { ), ); - expect(sourceCode).toStrictEqual( + expect(sourceCode.toString()).toStrictEqual( ( await readFile( require.resolve('@metamask/template-snap/dist/bundle.js'), @@ -105,6 +117,8 @@ describe('NpmLocation', () => { expect(svgIcon?.startsWith('')).toBe( true, ); + + expect(location.version).toBe(templateSnapVersion); }); it('fetches a package tarball directly without fetching the metadata when possible', async () => { @@ -170,6 +184,8 @@ describe('NpmLocation', () => { expect(svgIcon?.startsWith('')).toBe( true, ); + + expect(location.version).toBe(templateSnapVersion); }); it('falls back to zlib if DecompressionStream is unavailable', async () => { @@ -251,6 +267,50 @@ describe('NpmLocation', () => { ); }); + it('throws if fetching the NPM tarball fails', async () => { + const { version: templateSnapVersion } = JSON.parse( + ( + await readFile(require.resolve('@metamask/template-snap/package.json')) + ).toString('utf8'), + ); + + const tarballRegistry = `https://registry.npmjs.org/@metamask/template-snap/-/template-snap-${templateSnapVersion}.tgz`; + + const customFetchMock = jest.fn(); + + customFetchMock + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ + // eslint-disable-next-line @typescript-eslint/naming-convention + 'dist-tags': { + latest: templateSnapVersion, + }, + versions: { + [templateSnapVersion]: { + dist: { + // return npmjs.org registry here so that we can check overriding it with npmjs.cf works + tarball: tarballRegistry, + }, + }, + }, + }), + } as any) + .mockResolvedValue({ + ok: false, + body: null, + } as any); + + const location = new NpmLocation(new URL('npm:@metamask/template-snap'), { + versionRange: templateSnapVersion, + fetch: customFetchMock as typeof fetch, + }); + + await expect(location.manifest()).rejects.toThrow( + 'Failed to fetch tarball for package "@metamask/template-snap"', + ); + }); + it("can't use custom registries by default", () => { expect( () => @@ -288,3 +348,20 @@ describe('NpmLocation', () => { // TODO(ritave): Requires writing tarball packing utility out of scope for a hot-fix blocking release. it.todo('paths are normalized to remove "./" prefix'); }); + +describe('getNpmCanonicalBasePath', () => { + it('returns the default base path', () => { + expect( + getNpmCanonicalBasePath(DEFAULT_NPM_REGISTRY, '@metamask/example-snap'), + ).toBe('npm://registry.npmjs.org/@metamask/example-snap/'); + }); + + it('returns a path for a custom registry', () => { + expect( + getNpmCanonicalBasePath( + new URL('https://foo:bar@registry.com/'), + '@metamask/example-snap', + ), + ).toBe('npm://foo:bar@registry.com/@metamask/example-snap/'); + }); +}); diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts index 3213b0f9c7..54a66bf318 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -340,7 +340,6 @@ export function getNpmCanonicalBasePath(registryUrl: URL, packageName: string) { } canonicalBase += '@'; } - canonicalBase += registryUrl.host; return `${canonicalBase}${registryUrl.host}/${packageName}/`; } From 65c7b4587b143914c71e4084e550b55f942a1cff Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Fri, 15 Dec 2023 14:07:38 +0100 Subject: [PATCH 5/7] Address PR comments --- packages/snaps-controllers/coverage.json | 2 +- packages/snaps-controllers/src/snaps/location/npm.ts | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 34212c37e7..6f00fd7508 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -2,5 +2,5 @@ "branches": 90.8, "functions": 96.36, "lines": 97.63, - "statements": 97.37 + "statements": 97.3 } diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts index 54a66bf318..648f03e805 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -206,7 +206,7 @@ export abstract class BaseNpmLocation implements SnapLocation { } // Safety limit for tarballs, 250 MB in bytes -const TARBALL_SIZE_SAFETY_LIMIT = 262144000; +export const TARBALL_SIZE_SAFETY_LIMIT = 262144000; // Main NPM implementation, contains a browser tarball fetching implementation. export class NpmLocation extends BaseNpmLocation { @@ -235,7 +235,6 @@ export class NpmLocation extends BaseNpmLocation { tarballSize <= TARBALL_SIZE_SAFETY_LIMIT, 'Snap tarball exceeds size limit', ); - const tarballResponseBody = tarballResponse.body; return new Promise((resolve, reject) => { const files = new Map(); @@ -252,7 +251,8 @@ export class NpmLocation extends BaseNpmLocation { if ('DecompressionStream' in globalThis) { const decompressionStream = new DecompressionStream('gzip'); const decompressedStream = - tarballResponseBody.pipeThrough(decompressionStream); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + tarballResponse.body!.pipeThrough(decompressionStream); pipeline( getNodeStream(decompressedStream), @@ -265,7 +265,8 @@ export class NpmLocation extends BaseNpmLocation { } pipeline( - getNodeStream(tarballResponseBody), + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + getNodeStream(tarballResponse.body!), createGunzip(), tarballStream, (error: unknown) => { From 1be9bad638c220090ac86ef7dde8a2ebbd84dafb Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Fri, 15 Dec 2023 14:09:09 +0100 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: Maarten Zuidhoorn --- packages/snaps-controllers/src/snaps/location/npm.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/snaps-controllers/src/snaps/location/npm.ts b/packages/snaps-controllers/src/snaps/location/npm.ts index 648f03e805..f637ae09df 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -227,6 +227,7 @@ export class NpmLocation extends BaseNpmLocation { `Failed to fetch tarball for package "${this.meta.packageName}".`, ); } + // We assume that NPM is a good actor and provides us with a valid `content-length` header. const tarballSizeString = tarballResponse.headers.get('content-length'); assert(tarballSizeString, 'Snap tarball has invalid content-length'); From 7af87638e069f033834ea161e48e96aac11f41db Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Mon, 18 Dec 2023 11:40:13 +0100 Subject: [PATCH 7/7] Add test --- packages/snaps-controllers/coverage.json | 6 +-- .../src/snaps/location/npm.test.ts | 37 +++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 6f00fd7508..d59ba0f1c8 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { - "branches": 90.8, + "branches": 91.03, "functions": 96.36, - "lines": 97.63, - "statements": 97.3 + "lines": 97.69, + "statements": 97.37 } diff --git a/packages/snaps-controllers/src/snaps/location/npm.test.ts b/packages/snaps-controllers/src/snaps/location/npm.test.ts index 96a63d7169..364a6056ad 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.test.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.test.ts @@ -1,3 +1,4 @@ +import type { SemVerRange } from '@metamask/utils'; import { assert } from '@metamask/utils'; import { createReadStream } from 'fs'; import { readFile } from 'fs/promises'; @@ -311,6 +312,42 @@ describe('NpmLocation', () => { ); }); + it('throws if the NPM tarball URL is invalid', async () => { + const { version: templateSnapVersion } = JSON.parse( + ( + await readFile(require.resolve('@metamask/template-snap/package.json')) + ).toString('utf8'), + ); + + const customFetchMock = jest.fn(); + + customFetchMock.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + // eslint-disable-next-line @typescript-eslint/naming-convention + 'dist-tags': { + latest: templateSnapVersion, + }, + versions: { + [templateSnapVersion]: { + dist: { + tarball: 'foo', + }, + }, + }, + }), + } as any); + + const location = new NpmLocation(new URL('npm:@metamask/template-snap'), { + versionRange: '*' as SemVerRange, + fetch: customFetchMock as typeof fetch, + }); + + await expect(location.manifest()).rejects.toThrow( + `Failed to find valid tarball URL in NPM metadata for package "@metamask/template-snap".`, + ); + }); + it("can't use custom registries by default", () => { expect( () =>