From d992d4ee8c172cad81d0bfad5a96c24de7907b3d Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Wed, 7 Dec 2022 14:18:26 +0100 Subject: [PATCH 1/2] Fix some issues with fetching snaps --- .../snaps-controllers/src/snaps/SnapController.ts | 9 ++++----- .../snaps-controllers/src/snaps/location/http.ts | 2 +- .../snaps-controllers/src/snaps/location/location.ts | 10 ++++++++-- packages/snaps-controllers/src/snaps/location/npm.ts | 12 +++--------- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 784e718ffa..5cecef0155 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -654,8 +654,6 @@ export class SnapController extends BaseController< // This property cannot be hash private yet because of tests. private readonly maxRequestTime: number; - #npmRegistryUrl?: string; - #detectSnapLocation: typeof detectSnapLocation; // This property cannot be hash private yet because of tests. @@ -679,7 +677,6 @@ export class SnapController extends BaseController< state, getAppKey, environmentEndowmentPermissions = [], - npmRegistryUrl, idleTimeCheckInterval = inMilliseconds(5, Duration.Second), checkBlockList, maxIdleTime = inMilliseconds(30, Duration.Second), @@ -752,7 +749,6 @@ export class SnapController extends BaseController< this.#checkSnapBlockList = checkBlockList; this.#maxIdleTime = maxIdleTime; this.maxRequestTime = maxRequestTime; - this.#npmRegistryUrl = npmRegistryUrl; this.#detectSnapLocation = detectSnapLocationFunction; this._onUnhandledSnapError = this._onUnhandledSnapError.bind(this); this._onOutboundRequest = this._onOutboundRequest.bind(this); @@ -1663,7 +1659,10 @@ export class SnapController extends BaseController< ): Promise { validateSnapId(snapId); - const location = this.#detectSnapLocation(snapId, { versionRange }); + const location = this.#detectSnapLocation(snapId, { + versionRange, + fetch: this.#fetchFunction, + }); const existingSnap = this.getTruncated(snapId); // For devX we always re-install local snaps. diff --git a/packages/snaps-controllers/src/snaps/location/http.ts b/packages/snaps-controllers/src/snaps/location/http.ts index 7a1aafd34f..27db5ea049 100644 --- a/packages/snaps-controllers/src/snaps/location/http.ts +++ b/packages/snaps-controllers/src/snaps/location/http.ts @@ -40,7 +40,7 @@ export class HttpLocation implements SnapLocation { constructor(url: URL, opts: HttpOptions = {}) { assertStruct(url.toString(), HttpSnapIdStruct, 'Invalid Snap Id: '); - this.fetchFn = opts.fetch ?? globalThis.fetch; + this.fetchFn = opts.fetch ?? globalThis.fetch.bind(globalThis); this.fetchOptions = opts.fetchOptions; this.url = url; } diff --git a/packages/snaps-controllers/src/snaps/location/location.ts b/packages/snaps-controllers/src/snaps/location/location.ts index 63c0f49a98..bcb60d9c1f 100644 --- a/packages/snaps-controllers/src/snaps/location/location.ts +++ b/packages/snaps-controllers/src/snaps/location/location.ts @@ -25,6 +25,12 @@ export interface SnapLocation { } export type DetectSnapLocationOptions = NpmOptions & { + /** + * The function used to fetch data. + * + * @default globalThis.fetch + */ + fetch?: typeof fetch; /** * @default false */ @@ -48,14 +54,14 @@ export function detectSnapLocation( case 'npm:': return new NpmLocation(root, opts); case 'local:': - return new LocalLocation(root); + return new LocalLocation(root, opts); case 'http:': case 'https:': assert( allowHttp, new TypeError('Fetching snaps through http/https is disabled.'), ); - return new HttpLocation(root); + return new HttpLocation(root, opts); 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 index e41efc8072..6919a92db9 100644 --- a/packages/snaps-controllers/src/snaps/location/npm.ts +++ b/packages/snaps-controllers/src/snaps/location/npm.ts @@ -19,7 +19,7 @@ import { Readable, Writable } from 'stream'; import { extract as tarExtract } from 'tar-stream'; import { ensureRelative } from '../../utils'; -import { SnapLocation } from './location'; +import { DetectSnapLocationOptions, SnapLocation } from './location'; const DEFAULT_NPM_REGISTRY = 'https://registry.npmjs.org'; @@ -35,12 +35,6 @@ export interface NpmOptions { * @default DEFAULT_REQUESTED_SNAP_VERSION */ versionRange?: SemVerRange; - /** - * The function used to fetch data. - * - * @default globalThis.fetch - */ - fetch?: typeof fetch; /** * Whether to allow custom NPM registries outside of {@link DEFAULT_NPM_REGISTRY}. * @@ -56,9 +50,9 @@ export class NpmLocation implements SnapLocation { private files?: Map; - constructor(url: URL, opts: NpmOptions = {}) { + constructor(url: URL, opts: DetectSnapLocationOptions = {}) { const allowCustomRegistries = opts.allowCustomRegistries ?? false; - const fetchFunction = opts.fetch ?? globalThis.fetch; + const fetchFunction = opts.fetch ?? globalThis.fetch.bind(globalThis); const requestedRange = opts.versionRange ?? DEFAULT_REQUESTED_SNAP_VERSION; assertStruct(url.toString(), NpmSnapIdStruct, 'Invalid Snap Id: '); From 7e2d211048be7540003eadf6e8599cd6142e7f4f Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Wed, 7 Dec 2022 14:29:41 +0100 Subject: [PATCH 2/2] Fix test --- .../snaps-controllers/src/snaps/location/location.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/snaps-controllers/src/snaps/location/location.test.ts b/packages/snaps-controllers/src/snaps/location/location.test.ts index 0bbb2cf1ed..24741602ec 100644 --- a/packages/snaps-controllers/src/snaps/location/location.test.ts +++ b/packages/snaps-controllers/src/snaps/location/location.test.ts @@ -14,7 +14,9 @@ describe('detectSnapLocation', () => { ); it('disallows custom registries by default', () => { - expect(() => detectSnapLocation('npm://foo.com/bar')).toThrow( + expect(() => + detectSnapLocation('npm://foo.com/bar', { fetch: jest.fn() }), + ).toThrow( 'Custom NPM registries are disabled, tried to use "https://foo.com/".', ); }); @@ -27,6 +29,7 @@ describe('detectSnapLocation', () => { ])('detects %s', (url, classObj) => { expect( detectSnapLocation(url, { + fetch: jest.fn(), allowHttp: true, allowCustomRegistries: true, }),