mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-02 21:32:10 +03:00
detectBinary() in tool-detector.ts never checked process.platform and never
passed shell:true, so on native Windows an installed CLI (npm installs
claude/codex/opencode as .cmd shims) was reported as NOT installed:
1. execFileImpl(binary, ["--version"]) fails without shell:true for .cmd
shims (Node's CVE-2024-27980 hardening).
2. the `which` fallback doesn't exist on native Windows (no WSL/git-bash).
Both threw, both were swallowed by empty catches, detectBinary returned
{installed: false}. cliRuntime.ts::locateCommand already solved this for the
runtime-spawn path (#968) but never propagated here — re-drift, per the
issue title.
Exports locateCommand from cliRuntime.ts and reuses it (+ shouldUseShellForCommand,
+ getLookupEnv) for the win32 existence/path probe in tool-detector.ts, keeping
the --version probe local but shell-gated. Also routes the which fallback through
the injectable execFileImpl hook (it previously called the raw execFileAsync,
making it unmockable and prone to false-positives from a real system which).
This commit is contained in:
committed by
GitHub
parent
f5d0f9548d
commit
6459dde35c
1
changelog.d/fixes/7279-cli-detector-windows-drift.md
Normal file
1
changelog.d/fixes/7279-cli-detector-windows-drift.md
Normal file
@@ -0,0 +1 @@
|
||||
- fix(cli): reuse cliRuntime's win32-aware `locateCommand`/`shell:true` probe in tool-detector so installed CLIs (npm `.cmd` shims) are no longer reported as absent on native Windows (#7279)
|
||||
@@ -3,24 +3,24 @@ import path from "node:path";
|
||||
import { execFile } from "node:child_process";
|
||||
import { promisify } from "node:util";
|
||||
import { getCurrentHermesAgentRoles } from "./config-generator/hermes-agent";
|
||||
import { getCachedLoginShellPath, mergeShellPath } from "../../shared/services/loginShellPath";
|
||||
import {
|
||||
getLookupEnv,
|
||||
locateCommand,
|
||||
shouldUseShellForCommand,
|
||||
} from "../../shared/services/cliRuntime";
|
||||
|
||||
const execFileAsync = promisify(execFile);
|
||||
let execFileImpl = execFileAsync;
|
||||
|
||||
// #3321: macOS GUI/Electron truncates PATH, so `which`/`--version` probes miss Homebrew/
|
||||
// nvm/volta CLIs and the doctor reports them "not installed". Build a lookup env enriched
|
||||
// with the login-shell PATH (darwin-only, cached, fail-safe → returns process.env elsewhere).
|
||||
function detectorEnv(): NodeJS.ProcessEnv {
|
||||
const loginShellPath = getCachedLoginShellPath();
|
||||
if (!loginShellPath) return process.env;
|
||||
return { ...process.env, PATH: mergeShellPath(process.env.PATH || "", loginShellPath) };
|
||||
}
|
||||
let locateCommandImpl = locateCommand;
|
||||
|
||||
export function __setExecFileImpl(fn: typeof execFileAsync): void {
|
||||
execFileImpl = fn;
|
||||
}
|
||||
|
||||
export function __setLocateCommandImpl(fn: typeof locateCommand): void {
|
||||
locateCommandImpl = fn;
|
||||
}
|
||||
|
||||
export interface DetectedTool {
|
||||
id: string;
|
||||
name: string;
|
||||
@@ -79,17 +79,52 @@ function isConfigured(content: string, baseUrl: string): boolean {
|
||||
);
|
||||
}
|
||||
|
||||
// #968/#7279: on native Windows, npm installs CLI wrappers (claude/codex/opencode/…)
|
||||
// as .cmd/.bat shims. Node's CVE-2024-27980 hardening makes execFile()/spawn() reject
|
||||
// those without `shell: true`, and the `which` fallback below doesn't exist natively
|
||||
// on Windows (no WSL/git-bash) — so both probes threw, both were swallowed, and an
|
||||
// installed CLI was reported as absent. Reuse cliRuntime.ts's `locateCommand`
|
||||
// (already win32-aware since #968: `where.exe` + `.cmd`/`.exe`/`.bat`/`.com`
|
||||
// preference) for existence/path, then probe `--version` with `shell: true` when the
|
||||
// resolved binary needs it. If this drifts again, check cliRuntime.ts first.
|
||||
async function detectBinaryWindows(
|
||||
binary: string,
|
||||
env: NodeJS.ProcessEnv
|
||||
): Promise<{ installed: boolean; version?: string }> {
|
||||
const located = await locateCommandImpl(binary, env);
|
||||
if (!located.installed || !located.commandPath) return { installed: false };
|
||||
|
||||
try {
|
||||
const useShell = shouldUseShellForCommand(located.commandPath);
|
||||
const { stdout } = await execFileImpl(located.commandPath, ["--version"], {
|
||||
timeout: 5000,
|
||||
env,
|
||||
...(useShell ? { shell: true } : {}),
|
||||
});
|
||||
return { installed: true, version: stdout.trim().replace(/^v/, "") };
|
||||
} catch {
|
||||
// Binary exists on PATH but the --version probe failed (unusual flag, slow
|
||||
// startup, etc.) — still report it as installed since locateCommand confirmed it.
|
||||
return { installed: true };
|
||||
}
|
||||
}
|
||||
|
||||
async function detectBinary(name: string): Promise<{ installed: boolean; version?: string }> {
|
||||
const binary = BINARY_NAMES[name] || name;
|
||||
const env = detectorEnv();
|
||||
const env = getLookupEnv();
|
||||
|
||||
if (process.platform === "win32") {
|
||||
return detectBinaryWindows(binary, env);
|
||||
}
|
||||
|
||||
try {
|
||||
const { stdout } = await execFileImpl(binary, ["--version"], { timeout: 5000, env });
|
||||
const version = stdout.trim().replace(/^v/, "");
|
||||
return { installed: true, version };
|
||||
} catch {
|
||||
try {
|
||||
// Try `which` as fallback
|
||||
const { stdout } = await execFileAsync("which", [binary], { timeout: 5000, env });
|
||||
// Try `which` as fallback (routed through execFileImpl so it stays mockable)
|
||||
const { stdout } = await execFileImpl("which", [binary], { timeout: 5000, env });
|
||||
if (stdout.trim()) {
|
||||
return { installed: true };
|
||||
}
|
||||
|
||||
@@ -742,7 +742,7 @@ const checkExplicitPath = async (commandPath: string) => {
|
||||
}
|
||||
};
|
||||
|
||||
const locateCommand = async (command: string, env: Record<string, string | undefined>) => {
|
||||
export const locateCommand = async (command: string, env: Record<string, string | undefined>) => {
|
||||
if (!command) {
|
||||
return { installed: false, commandPath: null, reason: "missing_command" };
|
||||
}
|
||||
|
||||
80
tests/unit/cli-helper/tool-detector-win32-7279.test.ts
Normal file
80
tests/unit/cli-helper/tool-detector-win32-7279.test.ts
Normal file
@@ -0,0 +1,80 @@
|
||||
import { describe, it, before, after } from "node:test";
|
||||
import assert from "node:assert";
|
||||
import * as toolDetector from "../../../src/lib/cli-helper/tool-detector.ts";
|
||||
|
||||
// #7279 (re-drift of #968) — detectBinary() in tool-detector.ts never checked
|
||||
// process.platform and never passed shell:true, so on native Windows an
|
||||
// installed CLI (npm installs claude/codex/opencode as .cmd shims) was reported
|
||||
// as NOT installed:
|
||||
// 1. execFileImpl(binary, ["--version"]) fails without shell:true for .cmd shims
|
||||
// (Node's CVE-2024-27980 hardening).
|
||||
// 2. the `which` fallback doesn't exist on native Windows (no WSL/git-bash).
|
||||
// Both throw, both are swallowed by empty catches, detectBinary returns
|
||||
// { installed: false }. cliRuntime.ts::locateCommand already solved this for
|
||||
// the runtime-spawn path (#968); this fix reuses it here.
|
||||
//
|
||||
// Methodological note (see plan-file): the `which` fallback previously called
|
||||
// the RAW execFileAsync, not the injected __setExecFileImpl hook, so it wasn't
|
||||
// mockable and could silently "pass" using the real system `which`. Uses
|
||||
// `hermes` (confirmed absent from PATH) to avoid that trap; also uses a
|
||||
// dedicated __setLocateCommandImpl hook (mirrors __setExecFileImpl) so the
|
||||
// win32 existence probe is deterministic here instead of depending on a real
|
||||
// `where.exe`.
|
||||
|
||||
describe("tool-detector — win32 (#7279)", () => {
|
||||
const originalPlatformDescriptor = Object.getOwnPropertyDescriptor(process, "platform");
|
||||
|
||||
function setPlatform(value: string) {
|
||||
Object.defineProperty(process, "platform", { configurable: true, value });
|
||||
}
|
||||
|
||||
before(() => {
|
||||
setPlatform("win32");
|
||||
|
||||
toolDetector.__setLocateCommandImpl(async (command: string) => {
|
||||
if (command === "hermes") {
|
||||
return {
|
||||
installed: true,
|
||||
commandPath: "C:\\Users\\dev\\AppData\\Roaming\\npm\\hermes.cmd",
|
||||
reason: null,
|
||||
};
|
||||
}
|
||||
return { installed: false, commandPath: null, reason: "not_found" };
|
||||
});
|
||||
|
||||
// @ts-expect-error - internal test hook
|
||||
toolDetector.__setExecFileImpl(async (_cmd: string, _args: string[], opts?: { shell?: boolean }) => {
|
||||
// Reproduces the real-world failure: without shell:true, spawning the
|
||||
// .cmd shim throws (Node's CVE-2024-27980 hardening on Windows).
|
||||
if (opts?.shell === true) {
|
||||
return { stdout: "v0.75.3\n" };
|
||||
}
|
||||
throw new Error("spawn hermes.cmd ENOENT (shell:true required on win32 for .cmd shims)");
|
||||
});
|
||||
});
|
||||
|
||||
after(() => {
|
||||
// This is the only test file exercising these hooks — node:test isolates
|
||||
// each file's module cache, so no further reset is needed for other suites.
|
||||
if (originalPlatformDescriptor) {
|
||||
Object.defineProperty(process, "platform", originalPlatformDescriptor);
|
||||
}
|
||||
});
|
||||
|
||||
it("reports an installed CLI as installed on native Windows (.cmd shim probed with shell:true)", async () => {
|
||||
const result = await toolDetector.detectTool("hermes");
|
||||
assert.ok(result !== null);
|
||||
assert.strictEqual(
|
||||
result!.installed,
|
||||
true,
|
||||
"expected hermes to be detected as installed via locateCommand + shell:true probe on win32"
|
||||
);
|
||||
assert.strictEqual(result!.version, "0.75.3");
|
||||
});
|
||||
|
||||
it("reports a genuinely absent CLI as not installed on native Windows", async () => {
|
||||
const result = await toolDetector.detectTool("openclaw");
|
||||
assert.ok(result !== null);
|
||||
assert.strictEqual(result!.installed, false);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user