From 4daf27a3cd9685bbca6148ce57be88a96bc515da Mon Sep 17 00:00:00 2001 From: Benjamin Diedrichsen Date: Tue, 1 Sep 2026 12:47:50 +0200 Subject: [PATCH] [fix] nopy: spawn the pyinfra argv, never a shell string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes DOCS-AUDIT §4.2 (point 3, the last one open) and §1.3. `executeCall` joined `DeployCall.command` and ran it through `execa({shell: true})`, which made every value on that command line shell syntax. The finding framed it as a quoting problem "in the password"; it was wider than that. `--data` values were interpolated inside double quotes, so a backtick or a `$(…)` in *any* variable value was command substitution and a `;` ended the command and began another. `buildDeployCall` now emits a true argv — one element per argument, nothing pre-quoted — and the executor spawns `execa(command[0], command.slice(1))` with no `shell` option at all. pyinfra is still found on PATH and stdio stays inherited, so live output is unchanged. `maskCommand()` walks the argv by position instead of pattern-matching a joined string, which closes a leak of its own: it used to bound a secret's value on the closing `"` the builder had written two modules away, so a value containing a `"` leaked its own tail. It is now the only thing that turns the command back into a string, for display, and it shell-quotes as it goes so `--print-only` output stays pasteable. Also in `buildDeployCall`: `logConfigToFlags()` finally has a caller (§1.3). It was exported and unit-tested with nothing consuming it, so `log.verbosity` and `log.debug` in `.nopyrc.json` did nothing at all. The flags are prefixed onto the argv right after `-y`. Consequence worth knowing rather than discovering: `packages/nopy/.nopyrc.json` has always asked for `"verbosity": "trace", "debug": true`, so a run from that directory now really does get `-vvv --debug`. The tests move with it — the mock is `execa(file, args, opts)` with no factory to unwrap, and the new cases are the ones that would have caught this: an argv element holding `$(id); rm -rf /` stays one element, a secret whose value contains a quote is masked whole, and `execa` is asserted never to be asked for a shell. What remains is not fixable here: the value still reaches pyinfra on its command line, so it is visible in `ps`. That is pyinfra's `--data` interface. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DCzYTAm9QUhvLNr2EpdagJ --- packages/nopy/src/cubes/dependencies.ts | 17 +++-- packages/nopy/src/nopy.executor.ts | 63 +++++++++++++++---- .../tests/cubes.dependencies.edge.test.ts | 63 +++++++++++++++++-- packages/nopy/tests/executor.execute.test.ts | 25 +++++--- packages/nopy/tests/executor.test.ts | 59 +++++++++++++---- 5 files changed, 189 insertions(+), 38 deletions(-) diff --git a/packages/nopy/src/cubes/dependencies.ts b/packages/nopy/src/cubes/dependencies.ts index c1d8162..57852f8 100644 --- a/packages/nopy/src/cubes/dependencies.ts +++ b/packages/nopy/src/cubes/dependencies.ts @@ -7,6 +7,7 @@ import type { Cube, CubeVariables, HookContext } from '@bitsquare/nopy-cubes'; import { getLogger } from '@logtape/logtape'; import type { Variables } from '../nopy.common.js'; import type { NopyConfig } from '../nopy.config.js'; +import { logConfigToFlags } from '../nopy.config.js'; import { NopyUsageError } from '../nopy.errors.js'; import type { DeployCall } from '../nopy.executor.js'; import { VariableAssignment } from '../nopy.prompts.js'; @@ -205,6 +206,14 @@ export class BuildContext { /** * Builds and stores a deployment call for a resolved cube + * + * `command` is a true argv — one array element per argument, nothing + * pre-quoted. The executor spawns it without a shell, so a value containing a + * space, a quote or a `$(…)` is passed through verbatim instead of being + * re-parsed. It used to be a list of fragments joined into one shell string, + * which meant any variable value was shell syntax: an SSH password with a `;` + * in it ran whatever followed. {@link maskCommand} is the only thing that + * turns this back into a string, for display, and quotes as it goes. */ private buildDeployCall(cube: Cube, host: string): void { const cubeId = cube.id; @@ -212,17 +221,17 @@ export class BuildContext { if (this.resolvedCubes.has(callKey)) return; - const parts: string[] = []; + const parts: string[] = [...logConfigToFlags(this.config.log)]; if (this.auth.method === 'password' && this.auth.username && this.auth.password) { - parts.push(`--user ${this.auth.username} --password ${this.auth.password}`); + parts.push('--user', this.auth.username, '--password', this.auth.password); } const cubeVars = this.variables.get(cubeId); Object.entries(cubeVars).forEach(([key, value]) => { - parts.push(`--data "${key}=${value}"`); + parts.push('--data', `${key}=${value}`); }); - parts.push(`--chdir ${cube.dir}`); + parts.push('--chdir', cube.dir); parts.push(`${cube.dir}/${cube.deployScript}`); const command = ['pyinfra', host, '-y', ...parts]; diff --git a/packages/nopy/src/nopy.executor.ts b/packages/nopy/src/nopy.executor.ts index 8ac4efa..de4e002 100644 --- a/packages/nopy/src/nopy.executor.ts +++ b/packages/nopy/src/nopy.executor.ts @@ -20,7 +20,11 @@ export interface DeployCall { host: string; /** Working directory for execution */ cwd: string; - /** Full command array */ + /** + * The command as argv — `command[0]` is the executable, the rest are its + * arguments, one element each and none of them quoted. Nothing joins this to + * run it; {@link maskCommand} joins it to *show* it. + */ command: string[]; /** Environment variables for the cube */ env: Record; @@ -30,6 +34,18 @@ export interface DeployCall { dependencies: DependencySpec[]; } +/** + * One argv element, quoted for a POSIX shell. + * + * Display only — nothing is executed through a shell any more. The point is + * that what `--print-only` writes can be pasted into a terminal and mean the + * same thing it meant here. + */ +function shellQuote(arg: string): string { + if (arg.length > 0 && /^[\w@%+=:,./-]+$/.test(arg)) return arg; + return `'${arg.replace(/'/g, `'\\''`)}'`; +} + /** * The command as it is safe to show: the SSH password, and every `--data KEY=…` * whose key the manifest declared a secret, have their values replaced. @@ -37,18 +53,37 @@ export interface DeployCall { * pyinfra takes its data on the command line, so the real values have to be in * `call.command` — this is the last point before they would reach a log, a * `--print-only` dump or a dry-run plan. + * + * Walks the argv rather than pattern-matching a joined string. The old version + * bounded a `--data` value on the closing quote the builder had written, which + * tied masking to a quoting convention two modules apart; a value containing a + * `"` broke it, and it was the same brittleness that made the command a shell + * injection in the first place. Position is not guessable. */ export function maskCommand(call: DeployCall): string { - const command = call.command.join(' '); - const quoteMeta = (key: string) => key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const secrets = new Set(call.secrets ?? []); + const argv = call.command; + const out: string[] = []; - // The builder always quotes a `--data` value, so the closing quote bounds it. - const masked = (call.secrets ?? []).reduce( - (acc, key) => acc.replace(new RegExp(`(--data "${quoteMeta(key)}=)[^"]*"`, 'g'), `$1${MASK}"`), - command - ); + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + const next = argv[i + 1]; - return masked.replace(/(--password )\S+/g, `$1${MASK}`); + if (arg === '--password' && next !== undefined) { + out.push(arg, MASK); + i++; + } else if (arg === '--data' && next !== undefined) { + // Split on the first `=` only: the key cannot contain one, the value can. + const eq = next.indexOf('='); + const key = eq === -1 ? next : next.slice(0, eq); + out.push(arg, secrets.has(key) ? `${key}=${MASK}` : shellQuote(next)); + i++; + } else { + out.push(shellQuote(arg)); + } + } + + return out.join(' '); } /** @@ -107,14 +142,18 @@ export interface ExecutionOptions { */ async function executeCall(call: DeployCall): Promise { const startTime = Date.now(); - const commandStr = call.command.join(' '); + const [file, ...args] = call.command; try { log.info(`Executing: ${call.cube} -> ${call.host}`); log.debug(`Command: ${maskCommand(call)}`); - // Inherit stdio for live output - await execa({ shell: true })(commandStr, { + // No shell. `execa({shell: true})` used to run the whole command as one + // string, which made every variable value shell syntax — a password or a + // `--data` value containing `;`, a backtick or `$(…)` was executed rather + // than passed along. Spawning the argv directly removes the parse step; + // pyinfra is still found on PATH, and stdio stays inherited for live output. + await execa(file, args, { cwd: call.cwd, stdio: 'inherit', }); diff --git a/packages/nopy/tests/cubes.dependencies.edge.test.ts b/packages/nopy/tests/cubes.dependencies.edge.test.ts index a2781dd..dff1bad 100644 --- a/packages/nopy/tests/cubes.dependencies.edge.test.ts +++ b/packages/nopy/tests/cubes.dependencies.edge.test.ts @@ -61,6 +61,45 @@ describe('BuildContext error handling', () => { }); }); +describe('BuildContext log configuration', () => { + const build = (log: NopyConfig['log']) => + new BuildContext( + { 'cube-a': testCube('cube-a') }, + new Variables(), + session(), + { env: {}, log } as NopyConfig, + { method: 'ssh' }, + { useDefaults: true } + ); + + it('passes the configured verbosity and debug flags to pyinfra', async () => { + const context = build({ verbosity: 'verbose', debug: true }); + + await context.resolveCube('cube-a', 'host1'); + + expect(context.deployCalls[0].command.slice(0, 5)).toEqual([ + 'pyinfra', + 'host1', + '-y', + '-vv', + '--debug', + ]); + }); + + it('adds nothing when no log config is set', async () => { + const context = build(undefined); + + await context.resolveCube('cube-a', 'host1'); + + expect(context.deployCalls[0].command.slice(0, 4)).toEqual([ + 'pyinfra', + 'host1', + '-y', + '--chdir', + ]); + }); +}); + describe('BuildContext session replay', () => { it('takes variables from the session instead of prompting', async () => { const cube = testCube('cube-a', z.object({ PORT: z.string().default('3000') })); @@ -344,7 +383,7 @@ describe('BuildContext --use-defaults', () => { await context.resolveCube('cube-a', 'host1'); - expect(context.deployCalls[0].command.join(' ')).toContain('--data "PORT=8080"'); + expect(context.deployCalls[0].command).toContain('PORT=8080'); }); it('refuses to run a cube whose variable nothing can supply', async () => { @@ -485,14 +524,30 @@ describe('BuildContext command construction', () => { }); await context.resolveCube('cube-a', 'host1'); - const command = context.deployCalls[0].command.join(' '); + const command = context.deployCalls[0].command; - expect(command).toContain('--data "PORT=3000"'); - expect(command).toContain('--chdir /test/cube-a'); + // argv, not a shell string: each flag and its value are separate elements, + // and nothing is pre-quoted. + expect(command).toContain('PORT=3000'); + expect(command.join(' ')).toContain('--data PORT=3000'); + expect(command.join(' ')).toContain('--chdir /test/cube-a'); expect(command).toContain('/test/cube-a/deploy.py'); expect(context.deployCalls[0].cwd).toBe('/test/cube-a'); }); + it('keeps a value with shell metacharacters in one argv element', async () => { + // The whole point of dropping `shell: true`. Joined and handed to a shell, + // this value would have run `id` and swallowed the rest of the command. + const cube = testCube('cube-a', z.object({ MOTD: z.string().default('$(id); rm -rf /') })); + const context = new BuildContext({ 'cube-a': cube }, new Variables(), session(), config, { + method: 'ssh', + }); + + await context.resolveCube('cube-a', 'host1'); + + expect(context.deployCalls[0].command).toContain('MOTD=$(id); rm -rf /'); + }); + it('builds a separate call per host but records the cube session once', async () => { const context = new BuildContext( { 'cube-a': testCube('cube-a') }, diff --git a/packages/nopy/tests/executor.execute.test.ts b/packages/nopy/tests/executor.execute.test.ts index 4059c79..0ff113a 100644 --- a/packages/nopy/tests/executor.execute.test.ts +++ b/packages/nopy/tests/executor.execute.test.ts @@ -1,9 +1,9 @@ /** * Tests for the executeDeployCalls path of nopy.executor. * - * execa is mocked so no pyinfra process is ever spawned. Note the shape: - * the module calls execa({ shell: true })(command, opts), so the mock is a - * factory returning the runner. + * execa is mocked so no pyinfra process is ever spawned. The module calls + * `execa(file, args, opts)` directly — no shell, so no factory call to unwrap + * as there was while it went through `execa({ shell: true })`. */ import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -11,7 +11,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; const runner = vi.fn(); vi.mock('execa', () => ({ - execa: vi.fn(() => runner), + execa: vi.fn((...args: unknown[]) => runner(...args)), })); import { execa } from 'execa'; @@ -50,16 +50,27 @@ describe('executeDeployCalls', () => { logSpy.mockRestore(); }); - it('runs the joined command in the call cwd with inherited stdio', async () => { + it('spawns the argv directly in the call cwd with inherited stdio', async () => { await executeDeployCalls([call('cube-a')]); - expect(execa).toHaveBeenCalledWith({ shell: true }); - expect(runner).toHaveBeenCalledWith('pyinfra web-1 -y cube-a.deploy.py', { + expect(execa).toHaveBeenCalledWith('pyinfra', ['web-1', '-y', 'cube-a.deploy.py'], { cwd: '/cubes/cube-a', stdio: 'inherit', }); }); + it('never asks execa for a shell', async () => { + // The regression that matters: with `shell: true` every `--data` value was + // shell syntax, so a password or a variable containing `;` or `$(…)` ran. + await executeDeployCalls([ + { ...call('cube-a'), command: ['pyinfra', 'web-1', '--data', 'MOTD=$(id); rm -rf /'] }, + ]); + + const [, , options] = vi.mocked(execa).mock.calls[0] as unknown[]; + expect(options).not.toHaveProperty('shell'); + expect(vi.mocked(execa).mock.calls[0][1]).toContain('MOTD=$(id); rm -rf /'); + }); + it('reports success with a non-negative duration', async () => { const [result] = await executeDeployCalls([call('cube-a')]); diff --git a/packages/nopy/tests/executor.test.ts b/packages/nopy/tests/executor.test.ts index 2941741..05fb990 100644 --- a/packages/nopy/tests/executor.test.ts +++ b/packages/nopy/tests/executor.test.ts @@ -126,7 +126,7 @@ describe('outputExecutionPlan', () => { it('masks variables the manifest declared secret', () => { const call: DeployCall = { ...createTestCall('cube-a', 'host1'), - command: ['pyinfra', 'host1', '-y', '--data "PASSWORD=hunter2"', '--data "OTHER=visible"'], + command: ['pyinfra', 'host1', '-y', '--data', 'PASSWORD=hunter2', '--data', 'OTHER=visible'], env: { PASSWORD: 'hunter2', OTHER: 'visible' }, secrets: ['PASSWORD'], }; @@ -186,34 +186,47 @@ describe('maskCommand', () => { it('replaces the value of a declared secret', () => { const masked = maskCommand( - call(['pyinfra', 'host1', '--data "PASSWORD=hunter2"'], ['PASSWORD']) + call(['pyinfra', 'host1', '--data', 'PASSWORD=hunter2'], ['PASSWORD']) ); - expect(masked).toBe('pyinfra host1 --data "PASSWORD=********"'); + expect(masked).toBe('pyinfra host1 --data PASSWORD=********'); }); it('leaves other data alone', () => { const masked = maskCommand( - call(['--data "SSID=home"', '--data "PASSWORD=hunter2"'], ['PASSWORD']) + call(['--data', 'SSID=home', '--data', 'PASSWORD=hunter2'], ['PASSWORD']) ); - expect(masked).toBe('--data "SSID=home" --data "PASSWORD=********"'); + expect(masked).toBe('--data SSID=home --data PASSWORD=********'); }); - it('masks a value containing spaces up to the closing quote', () => { - const masked = maskCommand(call(['--data "PASSWORD=two words"', '--chdir /x'], ['PASSWORD'])); + it('masks a value containing spaces', () => { + const masked = maskCommand( + call(['--data', 'PASSWORD=two words', '--chdir', '/x'], ['PASSWORD']) + ); - expect(masked).toBe('--data "PASSWORD=********" --chdir /x'); + expect(masked).toBe('--data PASSWORD=******** --chdir /x'); }); it('masks an empty secret value', () => { - expect(maskCommand(call(['--data "PASSWORD="'], ['PASSWORD']))).toBe( - '--data "PASSWORD=********"' + expect(maskCommand(call(['--data', 'PASSWORD='], ['PASSWORD']))).toBe( + '--data PASSWORD=********' ); }); + it('masks a secret whose value contains a quote', () => { + // The old implementation bounded the value on the closing `"` the builder + // had written, so a value containing one leaked the rest of itself. + const masked = maskCommand(call(['--data', 'PASSWORD=he said "hi"'], ['PASSWORD'])); + + expect(masked).toBe('--data PASSWORD=********'); + expect(masked).not.toContain('hi'); + }); + it('masks the ssh password whether or not the cube declares secrets', () => { - const masked = maskCommand(call(['pyinfra', 'host1', '--user bob --password s3cr3t', '-y'])); + const masked = maskCommand( + call(['pyinfra', 'host1', '--user', 'bob', '--password', 's3cr3t', '-y']) + ); expect(masked).toBe('pyinfra host1 --user bob --password ******** -y'); }); @@ -221,4 +234,28 @@ describe('maskCommand', () => { it('returns the command untouched when there is nothing to hide', () => { expect(maskCommand(call(['pyinfra', 'host1', '-y']))).toBe('pyinfra host1 -y'); }); + + it('quotes an argument a shell would otherwise re-parse', () => { + // Display only — nothing runs through a shell — but `--print-only` output is + // meant to be pasteable, so it has to survive the round trip. + const masked = maskCommand(call(['--data', 'MOTD=$(id); rm -rf /'])); + + expect(masked).toBe(`--data 'MOTD=$(id); rm -rf /'`); + }); + + it('escapes an embedded single quote', () => { + expect(maskCommand(call(['--data', "NAME=o'brien"]))).toBe(`--data 'NAME=o'\\''brien'`); + }); + + it('quotes an empty argument rather than dropping it', () => { + expect(maskCommand(call(['pyinfra', '']))).toBe("pyinfra ''"); + }); + + it('leaves a trailing --password with no value alone', () => { + expect(maskCommand(call(['pyinfra', '--password']))).toBe('pyinfra --password'); + }); + + it('handles a --data argument with no equals sign', () => { + expect(maskCommand(call(['--data', 'BARE'], ['BARE']))).toBe('--data BARE=********'); + }); });