From 1168cbf39f76f4d4b34ac8affe715fe48bdd8131 Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Tue, 23 Apr 2024 18:16:29 -0700 Subject: [PATCH] Use shell: true and explicit cwd for all package manager commands --- ...-14e77b2f-24c2-47d5-a6c0-aa1b536f322d.json | 7 ++++ src/__e2e__/syncE2E.test.ts | 2 +- src/__fixtures__/mockNpm.test.ts | 36 +++++++++---------- src/__fixtures__/npmShow.ts | 2 +- .../packageManager/packagePublish.test.ts | 24 ++++++++----- .../listPackageVersions.test.ts | 2 +- src/__tests__/packageManager/npmArgs.test.ts | 2 +- src/bump/performBump.ts | 14 ++++---- src/commands/init.ts | 2 +- src/packageManager/listPackageVersions.ts | 7 ++-- src/packageManager/npm.ts | 24 +++---------- src/packageManager/npmArgs.ts | 2 +- src/packageManager/packageManager.ts | 33 +++++++++++++++++ src/packageManager/pnpm.ts | 24 ------------- src/packageManager/yarn.ts | 24 ------------- src/types/NpmOptions.ts | 5 +-- 16 files changed, 100 insertions(+), 110 deletions(-) create mode 100644 change/beachball-14e77b2f-24c2-47d5-a6c0-aa1b536f322d.json create mode 100644 src/packageManager/packageManager.ts delete mode 100644 src/packageManager/pnpm.ts delete mode 100644 src/packageManager/yarn.ts diff --git a/change/beachball-14e77b2f-24c2-47d5-a6c0-aa1b536f322d.json b/change/beachball-14e77b2f-24c2-47d5-a6c0-aa1b536f322d.json new file mode 100644 index 000000000..85fca06b4 --- /dev/null +++ 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 index b29e400d0..572b18901 100644 --- 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[1] = { registry: 'fake', retries: 3 }; + const publishOptions: Parameters[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 index 52a2e1f1e..5dee120f0 100644 --- 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 index 14e03a1c8..33b2c75f3 100644 --- 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 index 152df1427..6269f5a1a 100644 --- 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 index aef93697e..ac09e48fe 100644 --- 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 index e98859d75..992f7ede7 100644 --- 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 = { registry: 'https://testRegistry' }; const packageInfos = makePackageInfos({ basic: {}, diff --git a/src/bump/performBump.ts b/src/bump/performBump.ts index 40e9f8c88..16a6b80b1 100644 --- 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, packageInfos: PackageInfos): void { @@ -50,19 +49,22 @@ export async function updatePackageLock(cwd: string): Promise { 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 { } // 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 index 78ab271d8..2cc69d00b 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -28,7 +28,7 @@ export async function init(options: BeachballOptions): Promise { 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 index c66dd6f99..2ebcd7323 100644 --- 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 index 7137aacbe..1621d40ff 100644 --- 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>; +// 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 { - 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 index f1ed3bc1c..886eaf096 100644 --- 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): 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 new file mode 100644 index 000000000..42147d642 --- /dev/null +++ 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 { + 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 deleted file mode 100644 index ae5e5d623..000000000 --- a/src/packageManager/pnpm.ts +++ /dev/null @@ -1,24 +0,0 @@ -import execa from 'execa'; - -export type PnpmResult = Awaited>; - -/** - * Run an pnpm command. Returns the error result instead of throwing on failure. - */ -export async function pnpm( - args: string[], - options: execa.Options = {} -): Promise { - try { - const result = await execa('pnpm', args, { ...options }); - return { - ...result, - success: !result.failed, - }; - } catch (e) { - return { - ...(e as execa.ExecaError), - success: false, - }; - } -} diff --git a/src/packageManager/yarn.ts b/src/packageManager/yarn.ts deleted file mode 100644 index 837f5c300..000000000 --- a/src/packageManager/yarn.ts +++ /dev/null @@ -1,24 +0,0 @@ -import execa from 'execa'; - -export type YarnResult = Awaited>; - -/** - * Run an yarn command. Returns the error result instead of throwing on failure. - */ -export async function yarn( - args: string[], - options: execa.Options = {} -): Promise { - try { - const result = await execa('yarn', args, { ...options }); - return { - ...result, - success: !result.failed, - }; - } catch (e) { - return { - ...(e as execa.ExecaError), - success: false, - }; - } -} diff --git a/src/types/NpmOptions.ts b/src/types/NpmOptions.ts index 7fd709077..f161c9a98 100644 --- a/src/types/NpmOptions.ts +++ b/src/types/NpmOptions.ts @@ -1,4 +1,5 @@ import { BeachballOptions } from './BeachballOptions'; -export type NpmOptions = Required> & - Partial>; +export type NpmOptions = Required> & { path: string | undefined } & Partial< + Pick + >;