fix(cli): upgrade and update notice use the detected install method

* fix(cli): upgrade + update-notice use the detected install method

hyperframes upgrade hardcoded 'npm install -g', so bun/pnpm/brew users either
saw it fail or silently got a shadowed npm copy while their real (older) binary
kept running. Route the install through detectInstaller() via a new
installInvocation() argv helper; for skip kinds (ephemeral npx/bunx,
project-local, unknown) print 'npx hyperframes@latest' instead of guessing.

The passive update notice now shows the detected manager's command too. Semver
safety guard consolidated into a shared isSafeVersion(). Suppression gates and
the background auto-update flow are unchanged.

* test(cli): pin the shell:false contract of the --yes install path

Export runDetectedInstall and add a mocked-execFileSync test asserting the
detected manager binary is spawned with the exact installInvocation argv,
{stdio:inherit, shell:false}, and that an install failure sets a non-zero exit
code without throwing. Addresses review nit on the untested --yes path.

* fix(cli): guard the registry version at the boundary; execFile the auto-installer

Security (addresses review): a poisoned registry data.version (e.g.
'1.2.3; rm -rf /') was cached unvalidated and flowed into the background
auto-updater, which ran it via exec() -- a shell -- so a registry compromise
meant RCE on the next CLI run. isSafeVersion only covered the two touched
consumers (upgrade, notice), not this third sibling (scheduleBackgroundInstall).

- Guard at the registry boundary in checkForUpdate: only a strict-semver STRING
  is trusted; a non-string or metachar-bearing data.version is never cached and
  falls back to the last known-good version. The cache-read and fallback paths
  re-validate too, so a pre-existing poisoned cache can't leak through. One gate
  closes all three consumers and any future one; per-consumer checks stay as
  defense in depth.
- The detached auto-installer now runs via execFile(bin, args, shell:false),
  reusing installInvocation, matching the interactive runDetectedInstall path --
  the shell is gone from that path entirely.

Tests: reject poisoned / non-string registry version (never cached); accept a
valid semver.
This commit is contained in:
Miguel Ángel
2026-07-07 18:40:43 -04:00
committed by GitHub
parent 4a36655b2b
commit 4b3c73d941
8 changed files with 469 additions and 62 deletions
+57
View File
@@ -0,0 +1,57 @@
import { afterEach, describe, expect, it, vi } from "vitest";
/**
* Pins the security-relevant contract of the `--yes` install path: the detected
* manager binary is spawned via execFileSync with `shell: false` and the exact
* argv from installInvocation — no shell, so a version can never be re-parsed
* as shell syntax. installInvocation's argv correctness is covered separately
* in installerDetection.test.ts; this locks how it's executed.
*/
describe("runDetectedInstall", () => {
afterEach(() => {
vi.doUnmock("node:child_process");
vi.resetModules();
});
it("spawns the manager binary with shell:false and inherited stdio", async () => {
const execSpy = vi.fn();
vi.resetModules();
vi.doMock("node:child_process", () => ({ execFileSync: execSpy }));
const { runDetectedInstall } = await import("./upgrade.js");
runDetectedInstall(
{ bin: "bun", args: ["add", "-g", "hyperframes@1.2.3"] },
"bun add -g hyperframes@1.2.3",
"1.2.3",
);
expect(execSpy).toHaveBeenCalledTimes(1);
expect(execSpy).toHaveBeenCalledWith("bun", ["add", "-g", "hyperframes@1.2.3"], {
stdio: "inherit",
shell: false,
});
});
it("sets a non-zero exit code when the install fails, without throwing", async () => {
const execSpy = vi.fn(() => {
throw new Error("install boom");
});
vi.resetModules();
vi.doMock("node:child_process", () => ({ execFileSync: execSpy }));
const { runDetectedInstall } = await import("./upgrade.js");
const original = process.exitCode;
try {
expect(() =>
runDetectedInstall(
{ bin: "npm", args: ["install", "-g", "hyperframes@1.2.3"] },
"npm install -g hyperframes@1.2.3",
"1.2.3",
),
).not.toThrow();
expect(process.exitCode).toBe(1);
} finally {
process.exitCode = original;
}
});
});
+96 -46
View File
@@ -10,7 +10,13 @@ export const examples: Example[] = [
["Upgrade non-interactively", "hyperframes upgrade --yes"],
];
import { VERSION } from "../version.js";
import { checkForUpdate, withMeta } from "../utils/updateCheck.js";
import {
checkForUpdate,
withMeta,
isSafeVersion,
type UpdateCheckResult,
} from "../utils/updateCheck.js";
import { detectInstaller, installInvocation } from "../utils/installerDetection.js";
export default defineCommand({
meta: { name: "upgrade", description: "Check for updates and show upgrade instructions" },
@@ -19,6 +25,7 @@ export default defineCommand({
check: { type: "boolean", description: "Check for updates and exit (no prompt)" },
json: { type: "boolean", description: "Output as JSON", default: false },
},
// fallow-ignore-next-line complexity
async run({ args }) {
const useJson = args.json === true;
const checkOnly = args.check === true;
@@ -56,53 +63,96 @@ export default defineCommand({
return;
}
if (!autoYes) {
const shouldUpgrade = await clack.confirm({
message: "Upgrade now?",
});
if (clack.isCancel(shouldUpgrade) || !shouldUpgrade) {
clack.outro(c.dim("Skipped."));
return;
}
}
// Reject anything that isn't a strict semver-shaped string before it reaches
// the install command. A poisoned npm registry response could otherwise put
// shell metacharacters into `result.latest`; rejecting up front means the
// version flows through execFile (and the displayed command) as an opaque
// token, not something the shell might re-parse.
const SAFE_VERSION = /^[0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/;
if (!SAFE_VERSION.test(result.latest)) {
clack.outro(c.dim("Refusing to install: unexpected version string from npm registry."));
process.exitCode = 1;
if (!autoYes && !(await confirmUpgrade())) {
clack.outro(c.dim("Skipped."));
return;
}
const installArgs = ["install", "-g", `hyperframes@${result.latest}`];
const installCmd = `npm ${installArgs.join(" ")}`;
if (autoYes) {
console.log();
console.log(` ${c.dim("Running:")} ${c.accent(installCmd)}`);
console.log();
try {
// execFileSync with shell:false — the version is now provably safe per
// SAFE_VERSION above, but keep the no-shell call so future edits can't
// regress the shell-injection surface area.
execFileSync("npm", installArgs, { stdio: "inherit", shell: false });
clack.outro(c.success(`Upgraded to v${result.latest}`));
} catch {
clack.outro(c.dim("Install failed. Try running manually:"));
console.log(` ${c.accent(installCmd)}`);
process.exitCode = 1;
}
} else {
console.log();
console.log(` ${c.accent(installCmd)}`);
console.log(` ${c.dim("or")}`);
console.log(` ${c.accent("npx hyperframes@" + result.latest + " --version")}`);
console.log();
clack.outro(c.success("Run one of the commands above to upgrade."));
}
applyUpgrade(result, autoYes);
},
});
/** Interactive "Upgrade now?" prompt; false on decline or cancel. */
async function confirmUpgrade(): Promise<boolean> {
const shouldUpgrade = await clack.confirm({ message: "Upgrade now?" });
return !clack.isCancel(shouldUpgrade) && shouldUpgrade === true;
}
/**
* Show (or, with `autoYes`, run) the upgrade for the user's ACTUAL install
* method — not a hardcoded `npm install -g`, which fails or silently shadows a
* bun/pnpm/brew install. Extracted from `run` to keep that handler simple.
*/
// fallow-ignore-next-line complexity
function applyUpgrade(result: UpdateCheckResult, autoYes: boolean): void {
// Reject anything that isn't a strict semver before it reaches a command. A
// poisoned npm registry response could otherwise put shell metacharacters
// into `result.latest`; the guard means the version flows through execFile
// (and the displayed command) as an opaque token. Shared with the update
// notice via isSafeVersion.
if (!isSafeVersion(result.latest)) {
clack.outro(c.dim("Refusing to install: unexpected version string from npm registry."));
process.exitCode = 1;
return;
}
const installer = detectInstaller();
const invocation = installInvocation(installer.kind, result.latest);
const displayCmd = installer.installCommand(result.latest);
const npxFallback = `npx hyperframes@${result.latest}`;
// Undetectable / ephemeral (npx, bunx) / project-local / workspace: don't
// guess a manager command; point at the universal npx fallback instead.
if (!invocation || !displayCmd) {
printNpxFallback(installer.reason, npxFallback, autoYes);
return;
}
if (!autoYes) {
printManualCommands(displayCmd, npxFallback);
return;
}
runDetectedInstall(invocation, displayCmd, result.latest);
}
function printNpxFallback(reason: string, npxFallback: string, autoYes: boolean): void {
console.log();
if (autoYes) {
console.log(
` ${c.dim("Couldn't detect a global install to upgrade")} ${c.dim("(" + reason + ")")}`,
);
}
console.log(` ${c.accent(npxFallback)}`);
console.log();
clack.outro(c.success("Run the command above to use the latest version."));
}
function printManualCommands(displayCmd: string, npxFallback: string): void {
console.log();
console.log(` ${c.accent(displayCmd)}`);
console.log(` ${c.dim("or")}`);
console.log(` ${c.accent(npxFallback)}`);
console.log();
clack.outro(c.success("Run one of the commands above to upgrade."));
}
export function runDetectedInstall(
invocation: { bin: string; args: string[] },
displayCmd: string,
version: string,
): void {
console.log();
console.log(` ${c.dim("Running:")} ${c.accent(displayCmd)}`);
console.log();
try {
// shell:false — version is provably safe per isSafeVersion above; keep the
// no-shell call so future edits can't regress the injection surface.
execFileSync(invocation.bin, invocation.args, { stdio: "inherit", shell: false });
clack.outro(c.success(`Upgraded to v${version}`));
} catch {
clack.outro(c.dim("Install failed. Try running manually:"));
console.log(` ${c.accent(displayCmd)}`);
process.exitCode = 1;
}
}