Skip to content

Commit 3642f3d

Browse files
committed
fix: use execFile for pnpm remove to avoid shell interpolation
installPackages interpolated package names from featureDefinitions into a shell command string via exec(). While the data is developer- controlled, this contradicts the stated security principle. Switch to execFile with an args array for consistency with cloneRepo.
1 parent 69c84de commit 3642f3d

2 files changed

Lines changed: 40 additions & 14 deletions

File tree

source/__tests__/operations/installPackages.test.ts

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ vi.mock('../../operations/exec.js', () => ({
66
execFile: vi.fn().mockResolvedValue(''),
77
}))
88

9-
const { exec } = await import('../../operations/exec.js')
9+
const { exec, execFile } = await import('../../operations/exec.js')
1010
const { installPackages } = await import('../../operations/installPackages.js')
1111

1212
describe('installPackages', () => {
@@ -50,25 +50,28 @@ describe('installPackages', () => {
5050
await installPackages('/project/my_app', 'custom', ['demo'])
5151

5252
const removeCall = vi
53-
.mocked(exec)
54-
.mock.calls.find((call) => typeof call[0] === 'string' && call[0].startsWith('pnpm remove'))
53+
.mocked(execFile)
54+
.mock.calls.find((call) => call[0] === 'pnpm' && call[1][0] === 'remove')
5555
expect(removeCall).toBeDefined()
5656

57-
const removeCmd = removeCall?.[0] as string
57+
const removeArgs = removeCall?.[1] as string[]
5858
for (const pkg of featureDefinitions.subgraph.packages) {
59-
expect(removeCmd).toContain(pkg)
59+
expect(removeArgs).toContain(pkg)
6060
}
6161
for (const pkg of featureDefinitions.typedoc.packages) {
62-
expect(removeCmd).toContain(pkg)
62+
expect(removeArgs).toContain(pkg)
6363
}
6464
})
6565

6666
it('runs postinstall after pnpm remove', async () => {
6767
const callOrder: string[] = []
68-
vi.mocked(exec).mockImplementation(async (cmd) => {
69-
if (typeof cmd === 'string' && cmd.startsWith('pnpm remove')) {
68+
vi.mocked(execFile).mockImplementation(async (_file, args) => {
69+
if (args[0] === 'remove') {
7070
callOrder.push('remove')
7171
}
72+
return ''
73+
})
74+
vi.mocked(exec).mockImplementation(async (cmd) => {
7275
if (typeof cmd === 'string' && cmd.includes('postinstall')) {
7376
callOrder.push('postinstall')
7477
}
@@ -84,13 +87,36 @@ describe('installPackages', () => {
8487
await installPackages('/project/my_app', 'custom', ['demo', 'subgraph'])
8588

8689
const removeCall = vi
87-
.mocked(exec)
88-
.mock.calls.find((call) => typeof call[0] === 'string' && call[0].startsWith('pnpm remove'))
90+
.mocked(execFile)
91+
.mock.calls.find((call) => call[0] === 'pnpm' && call[1][0] === 'remove')
92+
expect(removeCall).toBeDefined()
93+
94+
const removeArgs = removeCall?.[1] as string[]
95+
for (const pkg of featureDefinitions.subgraph.packages) {
96+
expect(removeArgs).not.toContain(pkg)
97+
}
98+
})
99+
100+
it('uses execFile for pnpm remove to avoid shell interpolation', async () => {
101+
await installPackages('/project/my_app', 'custom', ['demo'])
102+
103+
expect(execFile).toHaveBeenCalledWith('pnpm', expect.arrayContaining(['remove']), {
104+
cwd: '/project/my_app',
105+
})
106+
})
107+
108+
it('passes each package as a separate arg to execFile', async () => {
109+
await installPackages('/project/my_app', 'custom', ['demo'])
110+
111+
const removeCall = vi
112+
.mocked(execFile)
113+
.mock.calls.find((call) => call[0] === 'pnpm' && call[1][0] === 'remove')
89114
expect(removeCall).toBeDefined()
90115

91-
const removeCmd = removeCall?.[0] as string
116+
const removeArgs = removeCall?.[1] as string[]
117+
expect(removeArgs[0]).toBe('remove')
92118
for (const pkg of featureDefinitions.subgraph.packages) {
93-
expect(removeCmd).not.toContain(pkg)
119+
expect(removeArgs).toContain(pkg)
94120
}
95121
})
96122
})

source/operations/installPackages.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type { FeatureName } from '../constants/config.js'
22
import type { InstallationType } from '../types/types.js'
33
import { getPackagesToRemove } from '../utils/utils.js'
4-
import { exec } from './exec.js'
4+
import { exec, execFile } from './exec.js'
55

66
export async function installPackages(
77
projectFolder: string,
@@ -20,6 +20,6 @@ export async function installPackages(
2020
return
2121
}
2222

23-
await exec(`pnpm remove ${packagesToRemove.join(' ')}`, { cwd: projectFolder })
23+
await execFile('pnpm', ['remove', ...packagesToRemove], { cwd: projectFolder })
2424
await exec('pnpm run postinstall', { cwd: projectFolder })
2525
}

0 commit comments

Comments
 (0)