[fix] nopy: spawn the pyinfra argv, never a shell string

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DCzYTAm9QUhvLNr2EpdagJ
This commit is contained in:
Benjamin Diedrichsen
2026-09-01 12:47:50 +02:00
co-authored by Claude Opus 5
parent 2019626618
commit 4daf27a3cd
5 changed files with 189 additions and 38 deletions
+13 -4
View File
@@ -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];
+51 -12
View File
@@ -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<string, unknown>;
@@ -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<ExecutionResult> {
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',
});