Use shell: true and explicit cwd for all package manager commands (#963)

ecraig12345 · web-flow · commit 349712ee2f15 · 2024-04-23T18:39:04.000-07:00
diff --git a/change/beachball-14e77b2f-24c2-47d5-a6c0-aa1b536f322d.json b/change/beachball-14e77b2f-24c2-47d5-a6c0-aa1b536f322d.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Use shell: true and explicit cwd for all package manager commands",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}
diff --git a/src/__e2e__/syncE2E.test.ts b/src/__e2e__/syncE2E.test.ts
@@ -22,7 +22,7 @@ describe('sync command (e2e)', () => {
   const mockNpm = initNpmMock();
 
   let repositoryFactory: RepositoryFactory | undefined;
-  const publishOptions: Parameters<typeof packagePublish>[1] = { registry: 'fake', retries: 3 };
+  const publishOptions: Parameters<typeof packagePublish>[1] = { registry: 'fake', retries: 3, path: undefined };
   const tempDirs: string[] = [];
 
   initMockLogs();
diff --git a/src/__fixtures__/mockNpm.test.ts b/src/__fixtures__/mockNpm.test.ts
@@ -122,48 +122,48 @@ describe('_mockNpmShow', () => {
 
   it("errors if package doesn't exist", () => {
     const emptyData = _makeRegistryData({});
-    const result = _mockNpmShow(emptyData, ['foo'], {});
+    const result = _mockNpmShow(emptyData, ['foo'], { cwd: undefined });
     expect(result).toEqual(getShowResult({ error: '[fake] code E404 - foo - not found' }));
   });
 
   it('returns requested version plus dist-tags and version list', () => {
-    const result = _mockNpmShow(data, ['foo@1.0.0'], {});
+    const result = _mockNpmShow(data, ['foo@1.0.0'], { cwd: undefined });
     expect(result).toEqual(getShowResult({ data: data, name: 'foo', version: '1.0.0' }));
   });
 
   it('returns requested version of scoped package', () => {
-    const result = _mockNpmShow(data, ['@foo/bar@2.0.0'], {});
+    const result = _mockNpmShow(data, ['@foo/bar@2.0.0'], { cwd: undefined });
     expect(result).toEqual(getShowResult({ data, name: '@foo/bar', version: '2.0.0' }));
   });
 
   it('returns requested tag', () => {
-    const result = _mockNpmShow(data, ['foo@beta'], {});
+    const result = _mockNpmShow(data, ['foo@beta'], { cwd: undefined });
     expect(result).toEqual(getShowResult({ data, name: 'foo', version: '1.0.0-beta' }));
   });
 
   it('returns requested tag of scoped package', () => {
-    const result = _mockNpmShow(data, ['@foo/bar@beta'], {});
+    const result = _mockNpmShow(data, ['@foo/bar@beta'], { cwd: undefined });
     expect(result).toEqual(getShowResult({ data, name: '@foo/bar', version: '2.0.0-beta' }));
   });
 
   it('returns latest version if no version requested', () => {
-    const result = _mockNpmShow(data, ['foo'], {});
+    const result = _mockNpmShow(data, ['foo'], { cwd: undefined });
     expect(result).toEqual(getShowResult({ data, name: 'foo', version: '1.0.1' }));
   });
 
   it('returns latest version of scoped package if no version requested', () => {
-    const result = _mockNpmShow(data, ['@foo/bar'], {});
+    const result = _mockNpmShow(data, ['@foo/bar'], { cwd: undefined });
     expect(result).toEqual(getShowResult({ data, name: '@foo/bar', version: '2.0.1' }));
   });
 
   it("errors if requested version doesn't exist", () => {
-    const result = _mockNpmShow(data, ['foo@2.0.0'], {});
+    const result = _mockNpmShow(data, ['foo@2.0.0'], { cwd: undefined });
     expect(result).toEqual(getShowResult({ error: '[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', () => {
-    expect(() => _mockNpmShow(data, ['foo@^1.0.0'], {})).toThrow(/not currently supported/);
+    expect(() => _mockNpmShow(data, ['foo@^1.0.0'], { cwd: undefined })).toThrow(/not currently supported/);
   });
 });
 
@@ -199,7 +199,7 @@ describe('_mockNpmPublish', () => {
   });
 
   it('throws if cwd is not specified', () => {
-    expect(() => _mockNpmPublish({}, [], {})).toThrow('cwd is required for mock npm publish');
+    expect(() => _mockNpmPublish({}, [], { cwd: undefined })).toThrow('cwd is required for mock npm publish');
   });
 
   it('errors if reading package.json fails', () => {
@@ -294,7 +294,7 @@ describe('mockNpm', () => {
 
   it('mocks npm show', async () => {
     npmMock.setRegistryData({ foo: { versions: ['1.0.0'] } });
-    const result = await npm(['show', 'foo']);
+    const result = await npm(['show', 'foo'], { cwd: undefined });
     expect(result).toMatchObject({
       success: true,
       stdout: expect.stringContaining('"name":"foo"'),
@@ -304,7 +304,7 @@ describe('mockNpm', () => {
   it('resets calls and registry after each test', async () => {
     expect(npmMock.mock).not.toHaveBeenCalled();
     // registry data for foo was set in the previous test but should have been cleared
-    const result = await npm(['show', 'foo']);
+    const result = await npm(['show', 'foo'], { cwd: undefined });
     expect(result).toMatchObject({
       success: false,
       stderr: expect.stringContaining('not found'),
@@ -313,7 +313,7 @@ describe('mockNpm', () => {
 
   it('can "publish" a package to registry with helper', async () => {
     npmMock.publishPackage({ name: 'foo', version: '1.0.0' });
-    const result = await npm(['show', 'foo']);
+    const result = await npm(['show', 'foo'], { cwd: undefined });
     expect(result).toMatchObject({
       success: true,
       stdout: expect.stringContaining('"name":"foo"'),
@@ -330,22 +330,22 @@ describe('mockNpm', () => {
   });
 
   it('throws on unsupported command', async () => {
-    await expect(() => npm(['pack'])).rejects.toThrow('Command not supported by mock npm: pack');
+    await expect(() => npm(['pack'], { cwd: undefined })).rejects.toThrow('Command not supported by mock npm: pack');
   });
 
   it('respects mocked command', async () => {
     const mockShow = jest.fn(() => 'hi');
     npmMock.setCommandOverride('show', mockShow as any);
-    const result = await npm(['show', 'foo']);
+    const result = await npm(['show', 'foo'], { cwd: undefined });
     expect(result).toEqual('hi');
-    expect(mockShow).toHaveBeenCalledWith(expect.any(Object), ['foo'], undefined);
+    expect(mockShow).toHaveBeenCalledWith(expect.any(Object), ['foo'], { cwd: undefined });
   });
 
   it("respects extra mocked command that's not normally supported", async () => {
     const mockPack = jest.fn(() => 'hi');
     npmMock.setCommandOverride('pack', mockPack as any);
-    const result = await npm(['pack']);
+    const result = await npm(['pack'], { cwd: undefined });
     expect(result).toEqual('hi');
-    expect(mockPack).toHaveBeenCalledWith(expect.any(Object), [], undefined);
+    expect(mockPack).toHaveBeenCalledWith(expect.any(Object), [], { cwd: undefined });
   });
 });
diff --git a/src/__fixtures__/npmShow.ts b/src/__fixtures__/npmShow.ts
@@ -19,7 +19,7 @@ export async function npmShow(
   const timeout = env.isCI && os.platform() === 'win32' ? 4500 : 1500;
   const registryArg = registry ? ['--registry', registry.getUrl()] : [];
 
-  const showResult = await npm([...registryArg, 'show', '--json', packageName], { timeout });
+  const showResult = await npm([...registryArg, 'show', '--json', packageName], { timeout, cwd: undefined });
 
   expect(!!showResult.timedOut).toBe(false);
   expect(showResult.failed).toBe(!!shouldFail);
diff --git a/src/__functional__/packageManager/packagePublish.test.ts b/src/__functional__/packageManager/packagePublish.test.ts
@@ -82,7 +82,11 @@ describe('packagePublish', () => {
     // Do a basic publishing test against the real registry
     await registry.reset();
     const testPackageInfo = getTestPackageInfo();
-    const publishResult = await packagePublish(testPackageInfo, { registry: registry.getUrl(), retries: 2 });
+    const publishResult = await packagePublish(testPackageInfo, {
+      registry: registry.getUrl(),
+      retries: 2,
+      path: tempRoot,
+    });
     expect(publishResult).toEqual(successResult);
     expect(npmSpy).toHaveBeenCalledTimes(1);
 
@@ -103,12 +107,16 @@ describe('packagePublish', () => {
     // Use real npm for this because the republish detection relies on the real error message
     await registry.reset();
     const testPackageInfo = getTestPackageInfo();
-    let publishResult = await packagePublish(testPackageInfo, { registry: registry.getUrl(), retries: 2 });
+    let publishResult = await packagePublish(testPackageInfo, {
+      registry: registry.getUrl(),
+      retries: 2,
+      path: tempRoot,
+    });
     expect(publishResult).toEqual(successResult);
     expect(npmSpy).toHaveBeenCalledTimes(1);
     logs.clear();
 
-    publishResult = await packagePublish(testPackageInfo, { registry: registry.getUrl(), retries: 2 });
+    publishResult = await packagePublish(testPackageInfo, { registry: registry.getUrl(), retries: 2, path: tempRoot });
     expect(publishResult).toEqual(failedResult);
     // `retries` should be ignored if the version already exists
     expect(npmSpy).toHaveBeenCalledTimes(2);
@@ -130,7 +138,7 @@ describe('packagePublish', () => {
       Promise.resolve({ success: false, all: 'sloooow', timedOut: true } as NpmResult)
     );
 
-    const publishResult = await packagePublish(testPackageInfo, { registry: 'fake', retries: 3 });
+    const publishResult = await packagePublish(testPackageInfo, { registry: 'fake', retries: 3, path: tempRoot });
     expect(publishResult).toEqual(successResult);
     expect(npmSpy).toHaveBeenCalledTimes(3);
 
@@ -146,7 +154,7 @@ describe('packagePublish', () => {
     // Again, mock all npm calls for this test instead of simulating an actual error condition.
     npmSpy.mockImplementation(() => Promise.resolve({ success: false, all: 'some errors' } as NpmResult));
 
-    const publishResult = await packagePublish(getTestPackageInfo(), { registry: 'fake', retries: 3 });
+    const publishResult = await packagePublish(getTestPackageInfo(), { registry: 'fake', retries: 3, path: tempRoot });
     expect(publishResult).toEqual(failedResult);
     expect(npmSpy).toHaveBeenCalledTimes(4);
 
@@ -162,7 +170,7 @@ describe('packagePublish', () => {
     const testPackageInfo = getTestPackageInfo();
     npmSpy.mockImplementation(() => Promise.resolve({ success: false, all: 'ERR! code ENEEDAUTH' } as NpmResult));
 
-    const publishResult = await packagePublish(testPackageInfo, { registry: 'fake', retries: 3 });
+    const publishResult = await packagePublish(testPackageInfo, { registry: 'fake', retries: 3, path: tempRoot });
     expect(publishResult).toEqual(failedResult);
     expect(npmSpy).toHaveBeenCalledTimes(1);
 
@@ -177,7 +185,7 @@ describe('packagePublish', () => {
     const testPackageInfo = getTestPackageInfo();
     npmSpy.mockImplementation(() => Promise.resolve({ success: false, all: 'ERR! code E404' } as NpmResult));
 
-    const publishResult = await packagePublish(testPackageInfo, { registry: 'fake', retries: 3 });
+    const publishResult = await packagePublish(testPackageInfo, { registry: 'fake', retries: 3, path: tempRoot });
     expect(publishResult).toEqual(failedResult);
     expect(npmSpy).toHaveBeenCalledTimes(1);
 
@@ -188,7 +196,7 @@ describe('packagePublish', () => {
     const testPackageInfo = getTestPackageInfo();
     npmSpy.mockImplementation(() => Promise.resolve({ success: false, all: 'ERR! code E403' } as NpmResult));
 
-    const publishResult = await packagePublish(testPackageInfo, { registry: 'fake', retries: 3 });
+    const publishResult = await packagePublish(testPackageInfo, { registry: 'fake', retries: 3, path: tempRoot });
     expect(publishResult).toEqual(failedResult);
     expect(npmSpy).toHaveBeenCalledTimes(1);
 
diff --git a/src/__tests__/packageManager/listPackageVersions.test.ts b/src/__tests__/packageManager/listPackageVersions.test.ts
@@ -14,7 +14,7 @@ jest.mock('../../packageManager/npm');
 describe('list npm versions', () => {
   /** Mock the `npm show` command for `npmAsync` calls. This also handles cleanup after each test. */
   const npmMock = initNpmMock();
-  const npmOptions: NpmOptions = { registry: 'https://fake' };
+  const npmOptions: NpmOptions = { registry: 'https://fake', path: undefined };
 
   afterEach(() => {
     _clearPackageVersionsCache();
diff --git a/src/__tests__/packageManager/npmArgs.test.ts b/src/__tests__/packageManager/npmArgs.test.ts
@@ -25,7 +25,7 @@ describe('getNpmAuthArgs', () => {
 });
 
 describe('getNpmPublishArgs', () => {
-  const options: NpmOptions = { registry: 'https://testRegistry' };
+  const options: Omit<NpmOptions, 'path'> = { registry: 'https://testRegistry' };
 
   const packageInfos = makePackageInfos({
     basic: {},
diff --git a/src/bump/performBump.ts b/src/bump/performBump.ts
@@ -7,8 +7,7 @@ import { BeachballOptions } from '../types/BeachballOptions';
 import { PackageInfos, PackageJson } from '../types/PackageInfo';
 import { findProjectRoot } from 'workspace-tools';
 import { npm } from '../packageManager/npm';
-import { pnpm } from '../packageManager/pnpm';
-import { yarn } from '../packageManager/yarn';
+import { packageManager } from '../packageManager/packageManager';
 import { callHook } from './callHook';
 
 export function writePackageJson(modifiedPackages: Set<string>, packageInfos: PackageInfos): void {
@@ -50,19 +49,22 @@ export async function updatePackageLock(cwd: string): Promise<void> {
   const root = findProjectRoot(cwd);
   if (root && fs.existsSync(path.join(root, 'package-lock.json'))) {
     console.log('Updating package-lock.json after bumping packages');
-    const res = await npm(['install', '--package-lock-only', '--ignore-scripts'], { stdio: 'inherit' });
+    const res = await npm(['install', '--package-lock-only', '--ignore-scripts'], { stdio: 'inherit', cwd: root });
     if (!res.success) {
       console.warn('Updating package-lock.json failed. Continuing...');
     }
   } else if (root && fs.existsSync(path.join(root, 'pnpm-lock.yaml'))) {
     console.log('Updating pnpm-lock.yaml after bumping packages');
-    const res = await pnpm(['install', '--lockfile-only', '--ignore-scripts'], { stdio: 'inherit' });
+    const res = await packageManager('pnpm', ['install', '--lockfile-only', '--ignore-scripts'], {
+      stdio: 'inherit',
+      cwd: root,
+    });
     if (!res.success) {
       console.warn('Updating pnpm-lock.yaml failed. Continuing...');
     }
   } else if (root && fs.existsSync(path.join(root, 'yarn.lock'))) {
     console.log('Updating yarn.lock after bumping packages');
-    const version = await yarn(['--version']);
+    const version = await packageManager('yarn', ['--version'], { cwd: root });
     if (!version.success) {
       console.warn('Failed to get yarn version. Continuing...');
       return;
@@ -74,7 +76,7 @@ export async function updatePackageLock(cwd: string): Promise<void> {
     }
 
     // for yarn v2+
-    const res = await yarn(['install', '--mode', 'update-lockfile'], { stdio: 'inherit' });
+    const res = await packageManager('yarn', ['install', '--mode', 'update-lockfile'], { stdio: 'inherit', cwd: root });
     if (!res.success) {
       console.warn('Updating yarn.lock failed. Continuing...');
     }
diff --git a/src/commands/init.ts b/src/commands/init.ts
@@ -28,7 +28,7 @@ export async function init(options: BeachballOptions): Promise<void> {
     errorExit(`Cannot find package.json at ${packageJsonFilePath}`);
   }
 
-  const npmResult = await npm(['info', 'beachball', '--json']);
+  const npmResult = await npm(['info', 'beachball', '--json'], { cwd: undefined });
   if (!npmResult.success) {
     errorExit('Failed to retrieve beachball version from npm');
   }
diff --git a/src/packageManager/listPackageVersions.ts b/src/packageManager/listPackageVersions.ts
@@ -28,9 +28,10 @@ async function getNpmPackageInfo(packageName: string, options: NpmOptions): Prom
   const { registry, token, authType, timeout } = options;
 
   if (env.beachballDisableCache || !packageVersionsCache[packageName]) {
-    const args = ['show', '--registry', registry, '--json', packageName, ...getNpmAuthArgs(registry, token, authType)];
-
-    const showResult = await npm(args, { timeout });
+    const showResult = await npm(
+      ['show', '--registry', registry, '--json', packageName, ...getNpmAuthArgs(registry, token, authType)],
+      { timeout, cwd: options.path }
+    );
 
     if (showResult.success && showResult.stdout !== '') {
       const packageInfo = JSON.parse(showResult.stdout);
diff --git a/src/packageManager/npm.ts b/src/packageManager/npm.ts
@@ -1,24 +1,10 @@
-import execa from 'execa';
+import { PackageManagerResult, packageManager } from './packageManager';
 
-export type NpmResult = Awaited<ReturnType<typeof npm>>;
+// The npm wrapper for packageManager is preserved for convenience.
+
+export type NpmResult = PackageManagerResult;
 
 /**
  * Run an npm command. Returns the error result instead of throwing on failure.
  */
-export async function npm(
-  args: string[],
-  options: execa.Options = {}
-): Promise<execa.ExecaReturnValue & { success: boolean }> {
-  try {
-    const result = await execa('npm', args, { ...options, shell: true });
-    return {
-      ...result,
-      success: !result.failed,
-    };
-  } catch (e) {
-    return {
-      ...(e as execa.ExecaError),
-      success: false,
-    };
-  }
-}
+export const npm = packageManager.bind(null, 'npm');
diff --git a/src/packageManager/npmArgs.ts b/src/packageManager/npmArgs.ts
@@ -2,7 +2,7 @@ import { AuthType } from '../types/Auth';
 import { NpmOptions } from '../types/NpmOptions';
 import { PackageInfo } from '../types/PackageInfo';
 
-export function getNpmPublishArgs(packageInfo: PackageInfo, options: NpmOptions): string[] {
+export function getNpmPublishArgs(packageInfo: PackageInfo, options: Omit<NpmOptions, 'path'>): string[] {
   const { registry, token, authType, access } = options;
   const pkgCombinedOptions = packageInfo.combinedOptions;
   const args = [
diff --git a/src/packageManager/packageManager.ts b/src/packageManager/packageManager.ts
@@ -0,0 +1,33 @@
+import execa from 'execa';
+
+export type PackageManagerResult = execa.ExecaReturnValue & { success: boolean };
+
+/**
+ * Run a package manager command. Returns the error result instead of throwing on failure.
+ * @param manager The package manager to use
+ * @param args Package manager command and arguments
+ * @param options cwd must be specified in options to reduce the chance of accidentally running
+ * commands in the wrong place. If it's definitely irrelevant in this case, use undefined.
+ */
+export async function packageManager(
+  manager: 'npm' | 'yarn' | 'pnpm',
+  args: string[],
+  options: execa.Options & { cwd: string | undefined }
+): Promise<execa.ExecaReturnValue & { success: boolean }> {
+  try {
+    const result = await execa(manager, args, {
+      ...options,
+      // This is required for Windows due to https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2
+      shell: true,
+    });
+    return {
+      ...result,
+      success: !result.failed,
+    };
+  } catch (e) {
+    return {
+      ...(e as execa.ExecaError),
+      success: false,
+    };
+  }
+}
diff --git a/src/packageManager/pnpm.ts b/src/packageManager/pnpm.ts
diff --git a/src/packageManager/yarn.ts b/src/packageManager/yarn.ts
diff --git a/src/types/NpmOptions.ts b/src/types/NpmOptions.ts

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ export async function init(options: BeachballOptions): Promise<void> {`
`28`	`28`	errorExit(`Cannot find package.json at ${packageJsonFilePath}`);
`29`	`29`	`}`
`30`	`30`
`31`		`- const npmResult = await npm(['info', 'beachball', '--json']);`
	`31`	`+ const npmResult = await npm(['info', 'beachball', '--json'], { cwd: undefined });`
`32`	`32`	`if (!npmResult.success) {`
`33`	`33`	`errorExit('Failed to retrieve beachball version from npm');`
`34`	`34`	`}`