Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some issues with fetching snaps #1050

Merged
merged 3 commits into from Dec 7, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 4 additions & 5 deletions packages/snaps-controllers/src/snaps/SnapController.ts
Expand Up @@ -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.
Expand All @@ -679,7 +677,6 @@ export class SnapController extends BaseController<
state,
getAppKey,
environmentEndowmentPermissions = [],
npmRegistryUrl,
idleTimeCheckInterval = inMilliseconds(5, Duration.Second),
checkBlockList,
maxIdleTime = inMilliseconds(30, Duration.Second),
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -1663,7 +1659,10 @@ export class SnapController extends BaseController<
): Promise<ProcessSnapResult> {
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.
Expand Down
2 changes: 1 addition & 1 deletion packages/snaps-controllers/src/snaps/location/http.ts
Expand Up @@ -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;
}
Expand Down
Expand Up @@ -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/".',
);
});
Expand All @@ -27,6 +29,7 @@ describe('detectSnapLocation', () => {
])('detects %s', (url, classObj) => {
expect(
detectSnapLocation(url, {
fetch: jest.fn(),
allowHttp: true,
allowCustomRegistries: true,
}),
Expand Down
10 changes: 8 additions & 2 deletions packages/snaps-controllers/src/snaps/location/location.ts
Expand Up @@ -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
*/
Expand All @@ -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.`,
Expand Down
12 changes: 3 additions & 9 deletions packages/snaps-controllers/src/snaps/location/npm.ts
Expand Up @@ -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';

Expand All @@ -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}.
*
Expand All @@ -56,9 +50,9 @@ export class NpmLocation implements SnapLocation {

private files?: Map<string, VirtualFile>;

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: ');
Expand Down