diff --git a/src/remediation/fix-commands.ts b/src/remediation/fix-commands.ts index a25c62280..0fe8ca5a4 100644 --- a/src/remediation/fix-commands.ts +++ b/src/remediation/fix-commands.ts @@ -284,6 +284,7 @@ export function buildSuggestedFixCommandPlan( // reason to re-resolve its subtree. command: buildNpmUpdateCommand(finding.pkg.name, finding.recommendedNpmTransitiveRemediation.workspaces), isDev: devLookup.get(`${finding.recommendedNpmTransitiveRemediation.package}@${finding.recommendedNpmTransitiveRemediation.currentVersion}`) ?? false, + workspaces: finding.recommendedNpmTransitiveRemediation.workspaces, })); continue; } @@ -392,14 +393,15 @@ export function buildSuggestedFixCommandPlan( const lockfileRefreshCommand = packageManager === "pnpm" ? buildPnpmLockfileRefreshCommand(finding.pkg.name, finding.recommendedNpmTransitiveRemediation.workspaces) : packageManager === "yarn" - ? `yarn upgrade ${finding.pkg.name}` - : `bun update ${finding.pkg.name}`; + ? buildYarnUpgradeCommand(finding.pkg.name, finding.recommendedNpmTransitiveRemediation.workspaces) + : buildBunUpdateCommand(finding.pkg.name); upsertTarget(targetsByKey, finding, parentUpdateTarget(finding, urgent, { package: finding.pkg.name, currentVersion: finding.pkg.version, childTargetVersion: finding.recommendedNpmTransitiveRemediation.targetChildVersion, reason: `${finding.pkg.name}@${finding.pkg.version} can be refreshed to ${finding.recommendedNpmTransitiveRemediation.targetChildVersion}+; no parent upgrade needed.`, command: lockfileRefreshCommand, + workspaces: finding.recommendedNpmTransitiveRemediation?.workspaces, })); continue; } @@ -468,10 +470,24 @@ export function buildSuggestedFixCommandPlan( // Some install targets carry an explicit `command` built without workspace // context (e.g. a chain-resolution command). This raw field is exposed in JSON - // and embedded in the HTML report, so scope it to the declaring member too - - // parent-update commands are already workspace-aware and left untouched. + // and embedded in the HTML report, so scope it to the declaring member too. + // Parent-update commands are rebuilt from the (possibly merged) workspace list + // so the printed plan matches what --fix will spawn. for (const target of targets) { - if (target.kind !== "parent-update" && target.command && target.workspaces?.length) { + if (target.kind === "parent-update") { + const updatePkg = target.childPackage ?? target.package; + if (packageManager === "npm") { + target.command = buildNpmUpdateCommand(updatePkg, target.workspaces); + } else if (packageManager === "pnpm") { + target.command = buildPnpmLockfileRefreshCommand(updatePkg, target.workspaces); + } else if (packageManager === "yarn") { + target.command = buildYarnUpgradeCommand(updatePkg, target.workspaces); + } else if (packageManager === "bun") { + target.command = buildBunUpdateCommand(updatePkg); + } + continue; + } + if (target.command && target.workspaces?.length) { const scoped = buildWorkspaceInstallCommands([target], packageManager); if (scoped.length > 0) target.command = scoped.join(" && "); } @@ -539,24 +555,43 @@ function inferPackageManager(scanInput: ScanInput): SuggestedFixPackageManager | function buildNpmUpdateCommand(pkg: string, workspaces?: string[]): string { const nonRoot = (workspaces ?? []).filter(w => w !== "."); + const pkgQ = shellQuote(pkg); if (nonRoot.length === 1) { - return `npm update --workspace=${nonRoot[0]} ${pkg}`; + return `npm update --workspace=${shellQuote(nonRoot[0])} ${pkgQ}`; } if (nonRoot.length > 1) { - return `npm update --workspaces ${pkg}`; + return `npm update --workspaces ${pkgQ}`; } - return `npm update ${pkg}`; + return `npm update ${pkgQ}`; } function buildPnpmLockfileRefreshCommand(pkg: string, workspaces?: string[]): string { + const pkgQ = shellQuote(pkg); if (!workspaces || workspaces.length === 0 || workspaces.length > 1) { - return `pnpm update --recursive --no-save ${pkg}`; + return `pnpm update --recursive --no-save ${pkgQ}`; } const workspace = workspaces[0]; if (workspace === ".") { - return `pnpm update --no-save ${pkg}`; + return `pnpm update --no-save ${pkgQ}`; } - return `pnpm --filter ./${workspace} update --no-save ${pkg}`; + return `pnpm --filter ${shellQuote("./" + workspace)} update --no-save ${pkgQ}`; +} + +function buildYarnUpgradeCommand(pkg: string, workspaces?: string[]): string { + const pkgQ = shellQuote(pkg); + const nonRoot = (workspaces ?? []).filter(w => w !== "."); + if (nonRoot.length === 0) return `yarn upgrade ${pkgQ}`; + return nonRoot.map(ws => `yarn workspace ${shellQuote(ws)} upgrade ${pkgQ}`).join(" && "); +} + +function buildBunUpdateCommand(pkg: string): string { + return `bun update ${shellQuote(pkg)}`; +} + +/** Quote a token for human-facing plan strings that users may paste into a shell. */ +function shellQuote(token: string): string { + if (/^[A-Za-z0-9_@%+./:=,-]+$/.test(token)) return token; + return "'" + token.replace(/'/g, "'\\''") + "'"; } export function commandPrefix(packageManager: SuggestedFixPackageManager): string { @@ -660,7 +695,7 @@ export function buildTargetCommand( } if (target.command) return target.command; const flag = target.isDev ? ` ${devFlag(packageManager)}` : ""; - return `${commandPrefix(packageManager)}${flag} ${target.package}@${target.targetVersion}`; + return `${commandPrefix(packageManager)}${flag} ${shellQuote(`${target.package}@${target.targetVersion}`)}`; } export function findSuggestedCommandForFinding( @@ -722,9 +757,14 @@ function parentUpdateTarget( reason: string; command: string; isDev?: boolean; + workspaces?: string[]; }, ): SuggestedFixTarget { const namesParent = args.package !== finding.pkg.name; + // Keep "." in the list: pnpm treats a sole root workspace as a plain refresh + // (no --filter / no --recursive). Stripping it here collapsed that case into + // the empty-workspaces branch and emitted --recursive. + const workspaces = args.workspaces ?? []; return { package: args.package, currentVersion: args.currentVersion, @@ -742,6 +782,7 @@ function parentUpdateTarget( command: args.command, usage: finding.usage ?? null, isDev: args.isDev, + workspaces: workspaces.length > 0 ? workspaces : undefined, }; } @@ -1033,16 +1074,16 @@ function buildWorkspaceInstallCommands( for (const [targets, isDev] of [[prodTargets, false], [devTargets, true]] as const) { if (targets.length === 0) continue; - const pkgArgs = targets.map(t => `${t.package}@${t.targetVersion}`).join(" "); + const pkgArgs = targets.map(t => shellQuote(`${t.package}@${t.targetVersion}`)).join(" "); const flag = isDev ? ` ${devFlag(packageManager)}` : ""; if (packageManager === "npm") { - const wsFlags = workspaces.map(ws => `-w ${ws}`).join(" "); + const wsFlags = workspaces.map(ws => `-w ${shellQuote(ws)}`).join(" "); commands.push(`npm install${flag}${wsFlags ? " " + wsFlags : ""} ${pkgArgs}`); } else if (packageManager === "pnpm") { // pnpm's --filter selector is a CLI-level flag that precedes the command // (`pnpm --filter ./client add pkg`), not a trailing argument. - const wsFlags = workspaces.map(ws => `--filter ./${ws}`).join(" "); + const wsFlags = workspaces.map(ws => `--filter ${shellQuote("./" + ws)}`).join(" "); const prefix = wsFlags ? `pnpm ${wsFlags} add` : "pnpm add"; commands.push(`${prefix}${flag} ${pkgArgs}`); } else if (packageManager === "yarn") { @@ -1050,11 +1091,11 @@ function buildWorkspaceInstallCommands( commands.push(`yarn add${flag} ${pkgArgs}`); } else { for (const ws of workspaces) { - commands.push(`yarn workspace ${ws} add${flag} ${pkgArgs}`); + commands.push(`yarn workspace ${shellQuote(ws)} add${flag} ${pkgArgs}`); } } } else if (packageManager === "bun") { - const wsFlags = workspaces.map(ws => `--filter ${ws}`).join(" "); + const wsFlags = workspaces.map(ws => `--filter ${shellQuote(ws)}`).join(" "); commands.push(`bun add${flag}${wsFlags ? " " + wsFlags : ""} ${pkgArgs}`); } else { commands.push(`${commandPrefix(packageManager)}${flag} ${pkgArgs}`); @@ -1081,7 +1122,7 @@ function buildCommandForTargets( const updateCommands = explicitCommands.length > 0 ? [...new Set(explicitCommands)] : packageManager === "npm" - ? [`npm update ${parentUpdateTargets.map(target => target.childPackage ?? target.package).join(" ")}`] + ? [`npm update ${parentUpdateTargets.map(target => shellQuote(target.childPackage ?? target.package)).join(" ")}`] : []; commandParts.push(...updateCommands); } @@ -1094,10 +1135,10 @@ function buildCommandForTargets( const prodInstall = installTargets.filter(t => !t.isDev); const devInstall = installTargets.filter(t => t.isDev); if (prodInstall.length > 0) { - commandParts.push(`${commandPrefix(packageManager)} ${prodInstall.map(t => `${t.package}@${t.targetVersion}`).join(" ")}`); + commandParts.push(`${commandPrefix(packageManager)} ${prodInstall.map(t => shellQuote(`${t.package}@${t.targetVersion}`)).join(" ")}`); } if (devInstall.length > 0) { - commandParts.push(`${commandPrefix(packageManager)} ${devFlag(packageManager)} ${devInstall.map(t => `${t.package}@${t.targetVersion}`).join(" ")}`); + commandParts.push(`${commandPrefix(packageManager)} ${devFlag(packageManager)} ${devInstall.map(t => shellQuote(`${t.package}@${t.targetVersion}`)).join(" ")}`); } } } diff --git a/src/utils/fix-runner.ts b/src/utils/fix-runner.ts index d7c31a917..6863ed4ed 100644 --- a/src/utils/fix-runner.ts +++ b/src/utils/fix-runner.ts @@ -7,7 +7,6 @@ import type { ParsedOptions } from "../types.js"; import type { SeverityLabel } from "../types.js"; import { pluralize } from "./string.js"; import { isBreakingUpgrade } from "./version.js"; -import { buildTargetCommand } from "../remediation/fix-commands.js"; import { formatParentUpdateChildNote } from "../remediation/parent-update-child-note.js"; import type { SuggestedFixCommandPlan, SuggestedFixTarget } from "../remediation/fix-commands.js"; import type { DebugLogger } from "../output/debug.js"; @@ -101,22 +100,114 @@ const EMPTY_UNAPPLIED: UnappliedBreakdown = { noFixCount: 0, }; -// Build the command(s) to apply a target. Within-range refresh (parent-update) -// targets carry an explicit, already-workspace-scoped command; direct targets -// are installed by version. A multi-member workspace command is `&&`-joined and -// is split into one spawn per member. +// Build argv lists for --fix without round-tripping through a shell string. +// Workspace paths and package tokens are passed as whole argv elements to spawn +// (no shell), so a directory name containing `&&` cannot become a second executable. +// Human-facing plan strings still come from buildTargetCommand. function buildApplyCommands( packageManager: SuggestedFixCommandPlan["packageManager"], target: SuggestedFixTarget, ): string[][] { - // Use the same workspace-scoped command the report/plan shows, so --fix applies - // fixes to the declaring member (npm install -w, pnpm --filter ...) rather than - // flat at the root. A multi-member command is `&&`-joined; split per spawn. - return buildTargetCommand(target, packageManager) - .split("&&") - .map(part => part.trim()) - .filter(Boolean) - .map(part => part.split(/\s+/)); + if (target.kind === "parent-update") { + return buildParentUpdateApplyArgv(packageManager, target); + } + return buildInstallApplyArgv(packageManager, target); +} + +function buildInstallApplyArgv( + packageManager: SuggestedFixCommandPlan["packageManager"], + target: SuggestedFixTarget, +): string[][] { + const workspaces = (target.workspaces ?? []).filter(w => w !== "."); + const pkgArg = `${target.package}@${target.targetVersion}`; + const isDev = Boolean(target.isDev); + + if (packageManager === "npm") { + const args = ["install"]; + if (isDev) args.push("-D"); + for (const ws of workspaces) { + args.push("-w", ws); + } + args.push(pkgArg); + return [["npm", ...args]]; + } + + if (packageManager === "pnpm") { + const args: string[] = []; + for (const ws of workspaces) { + args.push("--filter", `./${ws}`); + } + args.push("add"); + if (isDev) args.push("-D"); + args.push(pkgArg); + return [["pnpm", ...args]]; + } + + if (packageManager === "yarn") { + if (workspaces.length === 0) { + const args = ["add"]; + if (isDev) args.push("-D"); + args.push(pkgArg); + return [["yarn", ...args]]; + } + return workspaces.map(ws => { + const args = ["workspace", ws, "add"]; + if (isDev) args.push("-D"); + args.push(pkgArg); + return ["yarn", ...args]; + }); + } + + // bun + const args = ["add"]; + if (isDev) args.push("--dev"); + for (const ws of workspaces) { + args.push("--filter", ws); + } + args.push(pkgArg); + return [["bun", ...args]]; +} + +function buildParentUpdateApplyArgv( + packageManager: SuggestedFixCommandPlan["packageManager"], + target: SuggestedFixTarget, +): string[][] { + // Always build from structured fields. Never tokenize target.command: plan + // strings may be shell-quoted for display, and must not be re-parsed into argv. + const updatePkg = target.childPackage ?? target.package; + const workspaces = target.workspaces ?? []; + const nonRoot = workspaces.filter(w => w !== "."); + + if (packageManager === "npm") { + const args = ["update"]; + if (nonRoot.length === 1) { + args.push(`--workspace=${nonRoot[0]}`); + } else if (nonRoot.length > 1) { + args.push("--workspaces"); + } + args.push(updatePkg); + return [["npm", ...args]]; + } + + if (packageManager === "pnpm") { + if (workspaces.length === 0 || workspaces.length > 1) { + return [["pnpm", "update", "--recursive", "--no-save", updatePkg]]; + } + const workspace = workspaces[0]; + if (workspace === ".") { + return [["pnpm", "update", "--no-save", updatePkg]]; + } + return [["pnpm", "--filter", `./${workspace}`, "update", "--no-save", updatePkg]]; + } + + if (packageManager === "yarn") { + if (nonRoot.length === 0) { + return [["yarn", "upgrade", updatePkg]]; + } + return nonRoot.map(ws => ["yarn", "workspace", ws, "upgrade", updatePkg]); + } + + return [["bun", "update", updatePkg]]; } export type FixResultJson = { diff --git a/tests/fix-commands-shell-quote.test.ts b/tests/fix-commands-shell-quote.test.ts new file mode 100644 index 000000000..71d8cc0a8 --- /dev/null +++ b/tests/fix-commands-shell-quote.test.ts @@ -0,0 +1,36 @@ +import { buildTargetCommand } from "../src/remediation/fix-commands.js"; +import type { SuggestedFixTarget } from "../src/remediation/fix-commands.js"; + +function target(overrides: Partial): SuggestedFixTarget { + return { + package: "lodash", + currentVersion: "4.17.20", + targetVersion: "4.18.0", + kind: "direct", + urgent: true, + severity: "high", + adjusted: false, + adjustmentNote: null, + reason: "", + usage: null, + ...overrides, + }; +} + +describe("buildTargetCommand - shell quoting for plan display", () => { + it("quotes a workspace path that contains && so paste into a shell stays one token", () => { + const cmd = buildTargetCommand( + target({ workspaces: ["pkgs/evil&&touch PWNED"] }), + "npm", + ); + expect(cmd).toBe("npm install -w 'pkgs/evil&&touch PWNED' lodash@4.18.0"); + }); + + it("quotes a workspace path that contains spaced &&", () => { + const cmd = buildTargetCommand( + target({ workspaces: ["pkgs/evil && touch PWNED"] }), + "npm", + ); + expect(cmd).toBe("npm install -w 'pkgs/evil && touch PWNED' lodash@4.18.0"); + }); +}); diff --git a/tests/fix-runner-apply.test.ts b/tests/fix-runner-apply.test.ts index e0fe0f33b..b6c376a58 100644 --- a/tests/fix-runner-apply.test.ts +++ b/tests/fix-runner-apply.test.ts @@ -35,11 +35,13 @@ const options = { quiet: true } as unknown as ParsedOptions; function recordingRunner() { const calls: string[] = []; + const argvs: string[][] = []; const run = async (command: string, args: string[]) => { + argvs.push([command, ...args]); calls.push([command, ...args].join(" ")); return { status: 0 as number | null, error: null as Error | null }; }; - return { calls, run }; + return { calls, argvs, run }; } describe("applyFixesIfRequested - within-range refresh auto-apply", () => { @@ -52,7 +54,8 @@ describe("applyFixesIfRequested - within-range refresh auto-apply", () => { package: "follow-redirects", currentVersion: "1.14.0", targetVersion: "1.16.0", - command: "npm update -w packages/api follow-redirects", + workspaces: ["packages/api"], + command: "npm update --workspace=packages/api follow-redirects", }), ]), projectPath: "/tmp/proj", @@ -61,7 +64,7 @@ describe("applyFixesIfRequested - within-range refresh auto-apply", () => { runCommand: run, }); - expect(calls).toEqual(["npm update -w packages/api follow-redirects"]); + expect(calls).toEqual(["npm update --workspace=packages/api follow-redirects"]); expect(result.appliedFixCount).toBe(1); expect(result.appliedWithinRangeRefreshCount).toBe(1); expect(result.applied.map(a => a.package)).toContain("follow-redirects"); @@ -208,4 +211,95 @@ describe("applyFixesIfRequested - within-range refresh auto-apply", () => { expect(result.note).toContain("No fixes were auto-applied"); expect(result.parentUpgradeCount).toBe(1); }); + + it("keeps a workspace path containing && as one -w argument under --fix", async () => { + const { argvs, run } = recordingRunner(); + await applyFixesIfRequested({ + plan: plan([ + target({ + kind: "direct", + package: "lodash", + currentVersion: "4.17.20", + targetVersion: "4.18.0", + workspaces: ["pkgs/evil&&touch PWNED"], + }), + ]), + projectPath: "/tmp/proj", + totalFindings: 1, + options, + runCommand: run, + }); + + expect(argvs).toEqual([["npm", "install", "-w", "pkgs/evil&&touch PWNED", "lodash@4.18.0"]]); + }); + + it("keeps a workspace path containing spaced && as one -w argument under --fix", async () => { + const { argvs, run } = recordingRunner(); + await applyFixesIfRequested({ + plan: plan([ + target({ + kind: "direct", + package: "lodash", + currentVersion: "4.17.20", + targetVersion: "4.18.0", + workspaces: ["pkgs/evil && touch PWNED"], + }), + ]), + projectPath: "/tmp/proj", + totalFindings: 1, + options, + runCommand: run, + }); + + expect(argvs).toEqual([["npm", "install", "-w", "pkgs/evil && touch PWNED", "lodash@4.18.0"]]); + expect(argvs.map(a => a[0])).toEqual(["npm"]); + }); + + it("keeps a parent-update --workspace path containing && as one argv element", async () => { + const { argvs, run } = recordingRunner(); + await applyFixesIfRequested({ + plan: plan([ + target({ + kind: "parent-update", + package: "follow-redirects", + currentVersion: "1.14.0", + targetVersion: "1.16.0", + workspaces: ["pkgs/evil&&touch PWNED"], + command: "npm update --workspace=pkgs/evil&&touch PWNED follow-redirects", + }), + ]), + projectPath: "/tmp/proj", + totalFindings: 1, + options, + runCommand: run, + }); + + expect(argvs).toEqual([["npm", "update", "--workspace=pkgs/evil&&touch PWNED", "follow-redirects"]]); + }); + + it("does not spawn a second executable when a parent-update workspace path contains spaced &&", async () => { + const { argvs, run } = recordingRunner(); + await applyFixesIfRequested({ + plan: plan([ + target({ + kind: "parent-update", + package: "follow-redirects", + currentVersion: "1.14.0", + targetVersion: "1.16.0", + workspaces: ["pkgs/evil && touch PWNED"], + command: "npm update --workspace='pkgs/evil && touch PWNED' follow-redirects", + }), + ]), + projectPath: "/tmp/proj", + totalFindings: 1, + options, + runCommand: run, + }); + + expect(argvs).toEqual([ + ["npm", "update", "--workspace=pkgs/evil && touch PWNED", "follow-redirects"], + ]); + expect(argvs.map(a => a[0])).toEqual(["npm"]); + }); + });