Skip to content

Commit 0bbe326

Browse files
FrederikBoldingFrederikBolding
authored
Use DecompressionStream when available (#1971)
This PR changes the NPM unpacking implementation slightly to use native browser `DecompressionStream` for gzip decompression, where possible. If not possible, we use `browserify-zlib` instead of `gunzip-maybe` as it is slightly faster. Also adds a header to the NPM manifest fetching code that strips a bunch of unnecessary information.
1 parent a6ea785 commit 0bbe326

8 files changed

Lines changed: 185 additions & 150 deletions

File tree

‎packages/snaps-controllers/coverage.json‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
2-
"branches": 90.47,
3-
"functions": 96.32,
4-
"lines": 97.28,
5-
"statements": 96.96
2+
"branches": 90.11,
3+
"functions": 96.34,
4+
"lines": 97.29,
5+
"statements": 96.98
66
}

‎packages/snaps-controllers/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,9 @@
5656
"@metamask/snaps-utils": "workspace:^",
5757
"@metamask/utils": "^8.2.1",
5858
"@xstate/fsm": "^2.0.0",
59+
"browserify-zlib": "^0.2.0",
5960
"concat-stream": "^2.0.0",
6061
"get-npm-tarball-url": "^2.0.3",
61-
"gunzip-maybe": "^1.4.2",
6262
"immer": "^9.0.6",
6363
"json-rpc-middleware-stream": "^5.0.0",
6464
"nanoid": "^3.1.31",

‎packages/snaps-controllers/src/snaps/location/npm.test.ts‎

Lines changed: 112 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { createReadStream } from 'fs';
33
import { readFile } from 'fs/promises';
44
import fetchMock from 'jest-fetch-mock';
55
import path from 'path';
6+
import { Readable } from 'stream';
67

78
import { NpmLocation } from './npm';
89

@@ -22,9 +23,13 @@ describe('NpmLocation', () => {
2223

2324
const tarballUrl = `https://registry.npmjs.cf/@metamask/template-snap/-/template-snap-${templateSnapVersion}.tgz`;
2425
const tarballRegistry = `https://registry.npmjs.org/@metamask/template-snap/-/template-snap-${templateSnapVersion}.tgz`;
25-
fetchMock
26-
.mockResponseOnce(
27-
JSON.stringify({
26+
27+
const customFetchMock = jest.fn();
28+
29+
customFetchMock
30+
.mockResolvedValueOnce({
31+
ok: true,
32+
json: async () => ({
2833
// eslint-disable-next-line @typescript-eslint/naming-convention
2934
'dist-tags': {
3035
latest: templateSnapVersion,
@@ -38,27 +43,26 @@ describe('NpmLocation', () => {
3843
},
3944
},
4045
}),
41-
)
42-
.mockResponseOnce(
43-
(_req) =>
44-
Promise.resolve({
45-
ok: true,
46-
// eslint-disable-next-line @typescript-eslint/naming-convention
47-
headers: new Headers({ 'content-length': '5477' }),
48-
body: createReadStream(
49-
path.resolve(
50-
__dirname,
51-
`../../../test/fixtures/metamask-template-snap-${templateSnapVersion}.tgz`,
52-
),
46+
} as any)
47+
.mockResolvedValue({
48+
ok: true,
49+
// eslint-disable-next-line @typescript-eslint/naming-convention
50+
headers: new Headers({ 'content-length': '5477' }),
51+
body: Readable.toWeb(
52+
createReadStream(
53+
path.resolve(
54+
__dirname,
55+
`../../../test/fixtures/metamask-template-snap-${templateSnapVersion}.tgz`,
5356
),
54-
}) as any,
55-
);
57+
),
58+
),
59+
} as any);
5660

5761
const location = new NpmLocation(
5862
new URL('npm://registry.npmjs.cf/@metamask/template-snap'),
5963
{
6064
versionRange: templateSnapVersion,
61-
fetch: fetchMock as typeof fetch,
65+
fetch: customFetchMock as typeof fetch,
6266
allowCustomRegistries: true,
6367
},
6468
);
@@ -72,12 +76,13 @@ describe('NpmLocation', () => {
7276
await location.fetch(manifest.result.source.location.npm.iconPath)
7377
).toString();
7478

75-
expect(fetchMock).toHaveBeenCalledTimes(2);
76-
expect(fetchMock).toHaveBeenNthCalledWith(
79+
expect(customFetchMock).toHaveBeenCalledTimes(2);
80+
expect(customFetchMock).toHaveBeenNthCalledWith(
7781
1,
7882
'https://registry.npmjs.cf/@metamask/template-snap',
83+
{ headers: { accept: 'application/json' } },
7984
);
80-
expect(fetchMock).toHaveBeenNthCalledWith(2, tarballUrl);
85+
expect(customFetchMock).toHaveBeenNthCalledWith(2, tarballUrl);
8186

8287
expect(manifest.result).toStrictEqual(
8388
JSON.parse(
@@ -109,25 +114,95 @@ describe('NpmLocation', () => {
109114
).toString('utf8'),
110115
);
111116

112-
const tarballUrl = `https://registry.npmjs.org/@metamask/template-snap/-/template-snap-${templateSnapVersion}.tgz`;
113-
fetchMock.mockResponseOnce(
114-
(_req) =>
115-
Promise.resolve({
116-
ok: true,
117-
// eslint-disable-next-line @typescript-eslint/naming-convention
118-
headers: new Headers({ 'content-length': '5477' }),
119-
body: createReadStream(
120-
path.resolve(
121-
__dirname,
122-
`../../../test/fixtures/metamask-template-snap-${templateSnapVersion}.tgz`,
123-
),
117+
const customFetchMock = jest.fn();
118+
119+
customFetchMock.mockResolvedValue({
120+
ok: true,
121+
// eslint-disable-next-line @typescript-eslint/naming-convention
122+
headers: new Headers({ 'content-length': '5477' }),
123+
body: Readable.toWeb(
124+
createReadStream(
125+
path.resolve(
126+
__dirname,
127+
`../../../test/fixtures/metamask-template-snap-${templateSnapVersion}.tgz`,
124128
),
125-
}) as any,
129+
),
130+
),
131+
} as any);
132+
133+
const tarballUrl = `https://registry.npmjs.org/@metamask/template-snap/-/template-snap-${templateSnapVersion}.tgz`;
134+
135+
const location = new NpmLocation(new URL('npm:@metamask/template-snap'), {
136+
versionRange: templateSnapVersion,
137+
fetch: customFetchMock as typeof fetch,
138+
});
139+
140+
const manifest = await location.manifest();
141+
const sourceCode = (
142+
await location.fetch(manifest.result.source.location.npm.filePath)
143+
).toString();
144+
assert(manifest.result.source.location.npm.iconPath);
145+
const svgIcon = (
146+
await location.fetch(manifest.result.source.location.npm.iconPath)
147+
).toString();
148+
149+
expect(customFetchMock).toHaveBeenCalledTimes(1);
150+
expect(customFetchMock).toHaveBeenNthCalledWith(1, tarballUrl);
151+
152+
expect(manifest.result).toStrictEqual(
153+
JSON.parse(
154+
(
155+
await readFile(
156+
require.resolve('@metamask/template-snap/snap.manifest.json'),
157+
)
158+
).toString('utf8'),
159+
),
160+
);
161+
162+
expect(sourceCode).toStrictEqual(
163+
(
164+
await readFile(
165+
require.resolve('@metamask/template-snap/dist/bundle.js'),
166+
)
167+
).toString('utf8'),
168+
);
169+
170+
expect(svgIcon?.startsWith('<svg') && svgIcon.endsWith('</svg>')).toBe(
171+
true,
126172
);
173+
});
174+
175+
it('falls back to zlib if DecompressionStream is unavailable', async () => {
176+
// @ts-expect-error Deleting DecompressionStream for testing reasons
177+
delete globalThis.DecompressionStream;
178+
179+
const { version: templateSnapVersion } = JSON.parse(
180+
(
181+
await readFile(require.resolve('@metamask/template-snap/package.json'))
182+
).toString('utf8'),
183+
);
184+
185+
const customFetchMock = jest.fn();
186+
187+
customFetchMock.mockResolvedValue({
188+
ok: true,
189+
// eslint-disable-next-line @typescript-eslint/naming-convention
190+
headers: new Headers({ 'content-length': '5477' }),
191+
body: Readable.toWeb(
192+
createReadStream(
193+
path.resolve(
194+
__dirname,
195+
`../../../test/fixtures/metamask-template-snap-${templateSnapVersion}.tgz`,
196+
),
197+
),
198+
),
199+
} as any);
200+
201+
const tarballUrl = `https://registry.npmjs.org/@metamask/template-snap/-/template-snap-${templateSnapVersion}.tgz`;
127202

128203
const location = new NpmLocation(new URL('npm:@metamask/template-snap'), {
129204
versionRange: templateSnapVersion,
130-
fetch: fetchMock as typeof fetch,
205+
fetch: customFetchMock as typeof fetch,
131206
});
132207

133208
const manifest = await location.manifest();
@@ -139,8 +214,8 @@ describe('NpmLocation', () => {
139214
await location.fetch(manifest.result.source.location.npm.iconPath)
140215
).toString();
141216

142-
expect(fetchMock).toHaveBeenCalledTimes(1);
143-
expect(fetchMock).toHaveBeenNthCalledWith(1, tarballUrl);
217+
expect(customFetchMock).toHaveBeenCalledTimes(1);
218+
expect(customFetchMock).toHaveBeenNthCalledWith(1, tarballUrl);
144219

145220
expect(manifest.result).toStrictEqual(
146221
JSON.parse(

‎packages/snaps-controllers/src/snaps/location/npm.ts‎

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,9 @@ import {
1717
isObject,
1818
isValidSemVerVersion,
1919
} from '@metamask/utils';
20+
import { createGunzip } from 'browserify-zlib';
2021
import concat from 'concat-stream';
2122
import getNpmTarballUrl from 'get-npm-tarball-url';
22-
import createGunzipStream from 'gunzip-maybe';
2323
import { pipeline } from 'readable-stream';
2424
import type { Readable, Writable } from 'readable-stream';
2525
import { ReadableWebToNodeStream } from 'readable-web-to-node-stream';
@@ -191,16 +191,36 @@ export class NpmLocation implements SnapLocation {
191191
// We would need to replace tar-stream package because it requires immediate consumption of streams.
192192
await new Promise<void>((resolve, reject) => {
193193
this.files = new Map();
194+
195+
const tarballStream = createTarballStream(
196+
`${canonicalBase}/${this.meta.packageName}/`,
197+
this.files,
198+
);
199+
200+
// The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed
201+
// before we can actually grab any files from it.
202+
// To prevent recursion-based zip bombs, we should not allow recursion here.
203+
204+
// If native decompression stream is available we use that, otherwise fallback to zlib
205+
if ('DecompressionStream' in globalThis) {
206+
const decompressionStream = new DecompressionStream('gzip');
207+
const decompressedStream =
208+
tarballResponse.pipeThrough(decompressionStream);
209+
210+
pipeline(
211+
getNodeStream(decompressedStream),
212+
tarballStream,
213+
(error: unknown) => {
214+
error ? reject(error) : resolve();
215+
},
216+
);
217+
return;
218+
}
219+
194220
pipeline(
195221
getNodeStream(tarballResponse),
196-
// The "gz" in "tgz" stands for "gzip". The tarball needs to be decompressed
197-
// before we can actually grab any files from it.
198-
// To prevent recursion-based zip bombs, we set a maximum recursion depth of 1.
199-
createGunzipStream(1),
200-
createTarballStream(
201-
`${canonicalBase}/${this.meta.packageName}/`,
202-
this.files,
203-
),
222+
createGunzip(),
223+
tarballStream,
204224
(error: unknown) => {
205225
error ? reject(error) : resolve();
206226
},
@@ -230,11 +250,19 @@ export type PartialNpmMetadata = {
230250
*/
231251
export async function fetchNpmMetadata(
232252
packageName: string,
233-
registryUrl: URL | string,
253+
registryUrl: URL,
234254
fetchFunction: typeof fetch,
235255
): Promise<PartialNpmMetadata> {
236256
const packageResponse = await fetchFunction(
237257
new URL(packageName, registryUrl).toString(),
258+
{
259+
headers: {
260+
// Corgi format is slightly smaller: https://github.com/npm/pacote/blob/main/lib/registry.js#L71
261+
accept: isNPM(registryUrl)
262+
? 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*'
263+
: 'application/json',
264+
},
265+
},
238266
);
239267
if (!packageResponse.ok) {
240268
throw new Error(
@@ -252,6 +280,16 @@ export async function fetchNpmMetadata(
252280
return packageMetadata as PartialNpmMetadata;
253281
}
254282

283+
/**
284+
* Determine if a registry URL is NPM.
285+
*
286+
* @param registryUrl - A registry url.
287+
* @returns True if the registry is the NPM registry, otherwise false.
288+
*/
289+
function isNPM(registryUrl: URL) {
290+
return registryUrl.toString() === DEFAULT_NPM_REGISTRY.toString();
291+
}
292+
255293
/**
256294
* Resolves a version range to a version using the NPM registry.
257295
*
@@ -272,10 +310,7 @@ async function resolveNpmVersion(
272310
fetchFunction: typeof fetch,
273311
): Promise<{ tarballURL: string; targetVersion: SemVerVersion }> {
274312
// If the version range is already a static version we don't need to look for the metadata.
275-
if (
276-
registryUrl.toString() === DEFAULT_NPM_REGISTRY.toString() &&
277-
isValidSemVerVersion(versionRange)
278-
) {
313+
if (isNPM(registryUrl) && isValidSemVerVersion(versionRange)) {
279314
return {
280315
tarballURL: getNpmTarballUrl(packageName, versionRange),
281316
targetVersion: versionRange,

‎packages/snaps-controllers/src/types/vendor/global.d.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
export {};
2+
3+
// eslint-disable-next-line import/unambiguous
4+
declare global {
5+
class DecompressionStream extends TransformStream<Uint8Array, Uint8Array> {
6+
constructor(format?: string);
7+
}
8+
}

‎packages/snaps-controllers/src/types/vendor/readable-stream.d.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ declare module 'readable-stream' {
55
Readable,
66
Writable,
77
TransformCallback,
8+
Transform,
89
} from 'stream';
910
export { Duplex, pipeline } from 'stream';
1011
}

‎packages/snaps-controllers/src/types/vendor/zlib.d.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
// eslint-disable-next-line import/unambiguous
2+
declare module 'browserify-zlib' {
3+
import type { Transform } from 'readable-stream';
4+
5+
export function createGunzip(): Transform;
6+
}

0 commit comments

Comments
 (0)