Temporarily revert npm-registry-fetch usage

Author: ecraig12345

README.md

@@ -93,7 +93,7 @@ $ beachball publish -r http://localhost:4873 -t beta
 
 ### Overriding concurrency
 
-In large monorepos, the process of fetching versions for sync or before publishing can be time-consuming due to the high number of packages. To optimize performance, you can override the concurrency for fetching from the registry by setting `options.npmReadConcurrency` (default: 10). You can also increase concurrency for hook calls and publish operations via `options.concurrency` (default: 1; respects topological order).
+In large monorepos, the process of fetching versions for sync or before publishing can be time-consuming due to the high number of packages. To optimize performance, you can override the concurrency for fetching from the registry by setting `options.npmReadConcurrency` (default: 5). You can also increase concurrency for hook calls and publish operations via `options.concurrency` (default: 1; respects topological order).
 
 ### API surface
 

change/beachball-0b315c39-e39b-4b90-a3c9-504124425c0d.json

@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "Revert to using npm CLI for fetching package info due to vulnerability in older `npm-registry-fetch`'s old `tar` dependency (updating would require a major change to bump beachball's minimum Node version)",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}

docs/overview/configuration.md

@@ -91,7 +91,7 @@ For the latest full list of supported options, see `RepoOptions` [in this file](
 | `groupChanges` | `boolean` | `false` | repo | Write multiple changes to a single change file |
 | `hooks` | [`HooksOptions`][4] | | repo | Hooks for custom pre/post publish actions |
 | `ignorePatterns` | `string[]` | | repo | Ignore changes in files matching these glob patterns ([see notes][6]) |
-| `npmReadConcurrency` | number | 10 | repo | Maximum concurrency for fetching package versions from the registry (see `concurrency` for write operations) |
+| `npmReadConcurrency` | number | 5 | repo | Maximum concurrency for fetching package versions from the registry (see `concurrency` for write operations) |
 | `package` | `string` | | repo | Specifies which package the command relates to (overrides change detection based on `git diff`) |
 | `prereleasePrefix` | `string` | | repo | Prerelease prefix, e.g. `"beta"`. Note that if this is specified, packages with change type major/minor/patch will be bumped as prerelease instead. |
 | `publish` | `boolean` | `true` | repo | Whether to publish to npm registry |

package.json

@@ -54,7 +54,6 @@
     "cosmiconfig": "^9.0.0",
     "execa": "^5.0.0",
     "minimatch": "^3.0.4",
-    "npm-registry-fetch": "^14.0.0",
     "p-graph": "^1.1.2",
     "p-limit": "^3.0.2",
     "prompts": "^2.4.2",
@@ -67,7 +66,6 @@
     "@jest/globals": "^29.0.0",
     "@types/minimatch": "^5.0.0",
     "@types/node": "^14.0.0",
-    "@types/npm-registry-fetch": "^8.0.9",
     "@types/prompts": "^2.4.2",
     "@types/semver": "^7.3.13",
     "@types/tmp": "^0.2.3",

src/__e2e__/publishE2E.test.ts

@@ -24,7 +24,7 @@ import { deepFreezeProperties } from '../__fixtures__/object';
 // If an issue is found in the future that could only be caught by this test using real npm,
 // a new test file with a real registry should be created to cover that specific scenario.
 jest.mock('../packageManager/npm');
-jest.mock('npm-registry-fetch');
+// jest.mock('npm-registry-fetch');
 
 describe('publish command (e2e)', () => {
   const npmMock = initNpmMock();

src/__e2e__/publishRegistry.test.ts

@@ -20,7 +20,7 @@ import { deepFreezeProperties } from '../__fixtures__/object';
 // If an issue is found in the future that could only be caught by this test using real npm,
 // a new test file with a real registry should be created to cover that specific scenario.
 jest.mock('../packageManager/npm');
-jest.mock('npm-registry-fetch');
+// jest.mock('npm-registry-fetch');
 
 describe('publish command (registry)', () => {
   const npmMock = initNpmMock();
@@ -188,7 +188,8 @@ describe('publish command (registry)', () => {
 
     expect(npmMock.getPublishedPackage('foo')!.version).toEqual('1.1.0');
     expect(npmMock.getPublishedPackage('bar')!.version).toEqual('1.4.0');
-    expect(npmMock.mock).toHaveBeenCalledTimes(2);
+    expect(npmMock.mock).toHaveBeenCalledTimes(4);
+    // expect(npmMock.mock).toHaveBeenCalledTimes(2);
     expect(logs.mocks.error).not.toHaveBeenCalled();
   });
 

src/__e2e__/syncE2E.test.ts

@@ -18,7 +18,7 @@ import { getScopedPackages } from '../monorepo/getScopedPackages';
 // If an issue is found in the future that could only be caught by this test using real npm,
 // a new test file with a real registry should be created to cover that specific scenario.
 jest.mock('../packageManager/npm');
-jest.mock('npm-registry-fetch');
+// jest.mock('npm-registry-fetch');
 
 describe('sync command (e2e)', () => {
   const mockNpm = initNpmMock();

src/__fixtures__/mockNpm.test.ts

@@ -4,14 +4,21 @@
 
 import { afterAll, afterEach, beforeAll, describe, expect, it, jest } from '@jest/globals';
 import fs from 'fs';
-import fetch from 'npm-registry-fetch';
+// import fetch from 'npm-registry-fetch';
 import { type NpmResult, npm } from '../packageManager/npm';
 import type { PackageJson } from '../types/PackageInfo';
-import { initNpmMock, _makeRegistryData, _mockNpmPack, _mockNpmPublish, type MockNpmResult } from './mockNpm';
+import {
+  initNpmMock,
+  _makeRegistryData,
+  _mockNpmPack,
+  _mockNpmPublish,
+  _mockNpmShow,
+  type MockNpmResult,
+} from './mockNpm';
 import * as readJsonModule from '../object/readJson';
 
 jest.mock('fs');
-jest.mock('npm-registry-fetch');
+// jest.mock('npm-registry-fetch');
 jest.mock('../object/readJson');
 jest.mock('../packageManager/npm');
 
@@ -86,6 +93,94 @@ describe('_makeRegistryData', () => {
   });
 });
 
+describe('_mockNpmShow', () => {
+  function getErrorResult(errorMessage: string) {
+    return {
+      stdout: '',
+      stderr: errorMessage,
+      all: errorMessage,
+      success: false,
+      failed: true,
+    } as NpmResult;
+  }
+
+  function getShowResult(params: { name: string; version: string }) {
+    const { name, version } = params;
+    const output = JSON.stringify({
+      ...data[name].versions[version],
+      'dist-tags': data[name]['dist-tags'],
+      versions: Object.keys(data[name].versions),
+    });
+
+    return {
+      stdout: output,
+      stderr: '',
+      all: output,
+      success: true,
+      failed: false,
+    } as NpmResult;
+  }
+
+  const data = _makeRegistryData({
+    foo: {
+      versions: ['1.0.0-beta', '1.0.0', '1.0.1'],
+      'dist-tags': { latest: '1.0.1', beta: '1.0.0-beta' },
+    },
+    '@foo/bar': {
+      versions: ['2.0.0-beta', '2.0.0', '2.0.1'],
+      'dist-tags': { latest: '2.0.1', beta: '2.0.0-beta' },
+    },
+  });
+
+  it("errors if package doesn't exist", async () => {
+    const emptyData = _makeRegistryData({});
+    const result = await _mockNpmShow(emptyData, ['foo'], { cwd: undefined });
+    expect(result).toEqual(getErrorResult('[fake] code E404 - foo - not found'));
+  });
+
+  it('returns requested version plus dist-tags and version list', async () => {
+    const result = await _mockNpmShow(data, ['foo@1.0.0'], { cwd: undefined });
+    expect(result).toEqual(getShowResult({ name: 'foo', version: '1.0.0' }));
+  });
+
+  it('returns requested version of scoped package', async () => {
+    const result = await _mockNpmShow(data, ['@foo/bar@2.0.0'], { cwd: undefined });
+    expect(result).toEqual(getShowResult({ name: '@foo/bar', version: '2.0.0' }));
+  });
+
+  it('returns requested tag', async () => {
+    const result = await _mockNpmShow(data, ['foo@beta'], { cwd: undefined });
+    expect(result).toEqual(getShowResult({ name: 'foo', version: '1.0.0-beta' }));
+  });
+
+  it('returns requested tag of scoped package', async () => {
+    const result = await _mockNpmShow(data, ['@foo/bar@beta'], { cwd: undefined });
+    expect(result).toEqual(getShowResult({ name: '@foo/bar', version: '2.0.0-beta' }));
+  });
+
+  it('returns latest version if no version requested', async () => {
+    const result = await _mockNpmShow(data, ['foo'], { cwd: undefined });
+    expect(result).toEqual(getShowResult({ name: 'foo', version: '1.0.1' }));
+  });
+
+  it('returns latest version of scoped package if no version requested', async () => {
+    const result = await _mockNpmShow(data, ['@foo/bar'], { cwd: undefined });
+    expect(result).toEqual(getShowResult({ name: '@foo/bar', version: '2.0.1' }));
+  });
+
+  it("errors if requested version doesn't exist", async () => {
+    const result = await _mockNpmShow(data, ['foo@2.0.0'], { cwd: undefined });
+    expect(result).toEqual(getErrorResult('[fake] code E404 - foo@2.0.0 - not found'));
+  });
+
+  // support for this could be added later
+  it('currently throws if requested version is a range', async () => {
+    await expect(() => _mockNpmShow(data, ['foo@^1.0.0'], { cwd: undefined })).rejects.toThrow(
+      /not currently supported/
+    );
+  });
+});
+
 describe('_mockNpmPublish', () => {
   function getPublishResult(params: { error?: string; tag?: string }) {
     const { error, tag } = params;
@@ -279,25 +374,25 @@ describe('mockNpm', () => {
     jest.restoreAllMocks();
   });
 
-  describe('mockFetchJson', () => {
-    it('mocks registry fetch', async () => {
-      npmMock.setRegistryData({ foo: { versions: ['1.0.0'] } });
-      expect(fetch.json).toHaveProperty('mock');
-      const result = await fetch.json('/foo');
-      expect(result).toEqual({
-        name: 'foo',
-        modified: expect.any(String),
-        versions: { '1.0.0': { name: 'foo', version: '1.0.0' } },
-        'dist-tags': { latest: '1.0.0' },
-      });
-    });
-
-    it('resets calls and registry after each test', () => {
-      expect(npmMock.mockFetchJson).not.toHaveBeenCalled();
-      // registry data for foo was set in the previous test but should have been cleared
-      expect(() => fetch.json('/foo')).toThrow('404 Not Found');
-    });
-  });
+  // describe('mockFetchJson', () => {
+  //   it('mocks registry fetch', async () => {
+  //     npmMock.setRegistryData({ foo: { versions: ['1.0.0'] } });
+  //     expect(fetch.json).toHaveProperty('mock');
+  //     const result = await fetch.json('/foo');
+  //     expect(result).toEqual({
+  //       name: 'foo',
+  //       modified: expect.any(String),
+  //       versions: { '1.0.0': { name: 'foo', version: '1.0.0' } },
+  //       'dist-tags': { latest: '1.0.0' },
+  //     });
+  //   });
+
+  //   it('resets calls and registry after each test', () => {
+  //     expect(npmMock.mockFetchJson).not.toHaveBeenCalled();
+  //     // registry data for foo was set in the previous test but should have been cleared
+  //     expect(() => fetch.json('/foo')).toThrow('404 Not Found');
+  //   });
+  // });
 
   describe('getPublishedVersions', () => {
     it('gets data for a package', () => {
@@ -350,14 +445,23 @@ describe('mockNpm', () => {
         versions: ['1.0.0'],
         'dist-tags': { latest: '1.0.0' },
       });
-      expect(await fetch.json('/foo')).toEqual({
-        name: 'foo',
-        modified: expect.any(String),
-        versions: {
-          '1.0.0': { name: 'foo', version: '1.0.0' },
-        },
-        'dist-tags': { latest: '1.0.0' },
+      expect(await npm(['show', 'foo'], { cwd: undefined })).toMatchObject({
+        success: true,
+        stdout: JSON.stringify({
+          name: 'foo',
+          version: '1.0.0',
+          'dist-tags': { latest: '1.0.0' },
+          versions: ['1.0.0'],
+        }),
       });
+      // expect(await fetch.json('/foo')).toEqual({
+      //   name: 'foo',
+      //   modified: expect.any(String),
+      //   versions: {
+      //     '1.0.0': { name: 'foo', version: '1.0.0' },
+      //   },
+      //   'dist-tags': { latest: '1.0.0' },
+      // });
     });
   });
 
@@ -373,6 +477,11 @@ describe('mockNpm', () => {
       expect(npmMock.mock).toHaveBeenCalledWith(['publish'], expect.objectContaining({ cwd: 'fake' }));
     });
 
+    it('resets calls after each test', () => {
+      expect(npmMock.mock).not.toHaveBeenCalled();
+      expect(npmMock.getPublishedVersions('foo')).toBeUndefined();
+    });
+
     it('mocks npm pack command', async () => {
       packageJson = { name: 'foo', version: '2.0.0' };
       const result = await npm(['pack'], { cwd: 'fake' });
@@ -398,8 +507,13 @@ describe('mockNpm', () => {
       expect(npmMock.mock).toHaveBeenCalledWith(['foo'], expect.objectContaining({ cwd: undefined }));
     });
 
-    it('resets calls after each test', () => {
-      expect(npmMock.mock).not.toHaveBeenCalled();
+    it('TEMP mocks npm show command', async () => {
+      npmMock.setRegistryData({ foo: { versions: ['1.0.0'] } });
+      const result = await npm(['show', 'foo'], { cwd: undefined });
+      expect(result).toMatchObject({
+        success: true,
+        stdout: expect.stringContaining('"name":"foo"'),
+      });
     });
   });