From 70e46f04719f87f832054df0d1d9537e434c6f8c Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Mon, 20 Jul 2026 02:03:37 -0300 Subject: [PATCH] fix(cli): fix Windows CLI detection false negatives (#7753, #7774) (#7831) checkKnownPath() only validated the RESOLVED realpath target against EXPECTED_PARENT_PATHS, never crediting that the candidate path itself was already constructed from a trusted root by getKnownToolPaths(). Version managers that install via symlinks/junctions (nvm-windows and more broadly nvm/asdf/pyenv-style tools) place the shim inside a trusted root but its resolved target lives in a private per-version store outside the allowlist, so it was misreported as symlink_escape (#7753). Fix: trust a candidate location if EITHER its original path OR its resolved target falls within EXPECTED_PARENT_PATHS. Separately, locateCommandCandidate() short-circuited on the FIRST known-path candidate that returned any non-not_found failure reason, without trying the remaining candidates or ever falling back to a real PATH search. A single stray artifact at one guessed Windows install location for claude therefore poisoned detection entirely even when the real binary was resolvable via PATH (#7774). Fix: remember non-fatal known-path failures but keep walking every candidate, and always fall through to the PATH-based search before giving up. Extracted both helpers into a new cliRuntimeKnownPath.ts module (dependency injected, no circular import) to keep cliRuntime.ts under its frozen file-size budget. --- changelog.d/fixes/7753-windows-cli-path.md | 1 + src/shared/services/cliRuntime.ts | 81 ++++++++----------- src/shared/services/cliRuntimeKnownPath.ts | 79 ++++++++++++++++++ tests/unit/cli-runtime-extended.test.ts | 14 +++- ...ntime-known-path-shortcircuit-7774.test.ts | 67 +++++++++++++++ .../cliRuntime-symlink-escape-7753.test.ts | 60 ++++++++++++++ 6 files changed, 253 insertions(+), 49 deletions(-) create mode 100644 changelog.d/fixes/7753-windows-cli-path.md create mode 100644 src/shared/services/cliRuntimeKnownPath.ts create mode 100644 tests/unit/cli-runtime-known-path-shortcircuit-7774.test.ts create mode 100644 tests/unit/cliRuntime-symlink-escape-7753.test.ts diff --git a/changelog.d/fixes/7753-windows-cli-path.md b/changelog.d/fixes/7753-windows-cli-path.md new file mode 100644 index 0000000000..f75dfd6d03 --- /dev/null +++ b/changelog.d/fixes/7753-windows-cli-path.md @@ -0,0 +1 @@ +- fix(cli): resolve Windows CLI detection false negatives — nvm-windows symlinked binaries no longer rejected as symlink_escape, and a stray known-path artifact no longer hides a genuinely runnable Claude CLI on the PATH fallback (#7753, #7774) diff --git a/src/shared/services/cliRuntime.ts b/src/shared/services/cliRuntime.ts index 279b8a22c4..509bfd18cd 100644 --- a/src/shared/services/cliRuntime.ts +++ b/src/shared/services/cliRuntime.ts @@ -7,6 +7,7 @@ import { getHermesHome } from "@/lib/cli-helper/config-generator/hermesHome"; import { getCachedLoginShellPath, mergeShellPath } from "./loginShellPath"; import { withSettingsFallback } from "./cliInstallFallback"; import { GROK_BUILD_RUNTIME_ENTRY, AMP_RUNTIME_ENTRY } from "./cliRuntimeGrokBuild"; +import { isLocationTrusted, findKnownPathMatch } from "./cliRuntimeKnownPath"; const VALID_RUNTIME_MODES = new Set(["auto", "host", "container"]); const FALSE_VALUES = new Set(["0", "false", "no", "off"]); @@ -791,7 +792,7 @@ export const locateCommand = async (command: string, env: Record { +export const checkKnownPath = async (commandPath: string) => { if (!path.isAbsolute(commandPath)) { return { installed: false, commandPath: null, reason: "not_absolute" }; } @@ -804,27 +805,21 @@ const checkKnownPath = async (commandPath: string) => { // Resolve symlinks to get the real path and detect symlink escapes const realPath = await fs.realpath(commandPath); - // Verify the resolved path is still within expected directories - // Use pre-computed expected parent paths (cached at module startup for performance). - // On macOS temp directories often resolve from /var -> /private/var, so compare both - // the configured parent and its canonical realpath when available. - let isWithinExpected = false; - for (const parent of EXPECTED_PARENT_PATHS) { - if (isPathWithin(realPath, parent)) { - isWithinExpected = true; - break; - } - - try { - const resolvedParent = await fs.realpath(parent); - if (isPathWithin(realPath, resolvedParent)) { - isWithinExpected = true; - break; - } - } catch { - // Ignore missing/unresolvable parents and continue checking the remaining ones. - } - } + // Verify the resolved path — OR the original pre-resolution path — is within + // expected directories. Use pre-computed expected parent paths (cached at module + // startup for performance). On macOS temp directories often resolve from + // /var -> /private/var, so compare both the configured parent and its canonical + // realpath when available. + // + // #7753: also trust the pre-resolution `commandPath` itself, not just `realPath` + // — see isLocationTrusted() in cliRuntimeKnownPath.ts for the rationale. + const isWithinExpected = await isLocationTrusted( + commandPath, + realPath, + EXPECTED_PARENT_PATHS, + isPathWithin, + fs.realpath + ); if (!isWithinExpected) { return { installed: false, commandPath: null, reason: "symlink_escape" }; @@ -862,6 +857,8 @@ const checkKnownPath = async (commandPath: string) => { } }; +type KnownPathResult = Awaited>; + const locateCommandCandidate = async ( commands: string[], env: Record, @@ -873,35 +870,22 @@ const locateCommandCandidate = async ( // SECURITY: First check known installation paths for this specific tool // This avoids searching PATH and reduces attack surface + let bestKnownPathFailure: KnownPathResult | null = null; if (toolId) { - const knownPaths = getKnownToolPaths(toolId); - for (const knownPath of knownPaths) { - const result = await checkKnownPath(knownPath); - if (result.installed && result.reason === null) { - return { - command: commands[0], - installed: true, - commandPath: result.commandPath, - reason: null, - }; - } - - if (result.installed && result.reason === "not_executable") { - return { - command: commands[0], - installed: true, - commandPath: result.commandPath, - reason: "not_executable", - }; - } - - if (result.reason && result.reason !== "not_found") { - return { command: commands[0], ...result }; - } + const { match, bestFailure } = await findKnownPathMatch(getKnownToolPaths(toolId), checkKnownPath); + if (match) { + return { + command: commands[0], + installed: true, + commandPath: match.commandPath, + reason: match.reason, + }; } + bestKnownPathFailure = bestFailure; } - // Fallback: search PATH (user can set CLI_EXTRA_PATHS if needed) + // Always try PATH — a stray/broken known-path guess must never hide a genuinely + // PATH-resolvable binary (#7774). User can also set CLI_EXTRA_PATHS if needed. for (const command of commands) { const located = await locateCommand(command, env); if (located.installed || located.reason !== "not_found") { @@ -909,6 +893,9 @@ const locateCommandCandidate = async ( } } + if (bestKnownPathFailure) { + return { command: commands[0], ...bestKnownPathFailure }; + } return { command: commands[0], installed: false, commandPath: null, reason: "not_found" }; }; diff --git a/src/shared/services/cliRuntimeKnownPath.ts b/src/shared/services/cliRuntimeKnownPath.ts new file mode 100644 index 0000000000..63b4f9fa06 --- /dev/null +++ b/src/shared/services/cliRuntimeKnownPath.ts @@ -0,0 +1,79 @@ +/** + * Known-install-path trust & candidate-matching helpers for cliRuntime.ts. + * + * Extracted from cliRuntime.ts (which is at its frozen file-size budget) so the + * #7753/#7774 fixes don't grow that file. Both helpers are dependency-injected + * (no import back into cliRuntime.ts) to avoid a circular module reference. + */ + +export type PathWithinFn = (childPath: string, parentPath: string) => boolean; + +/** + * #7753 — nvm-windows/nvm/asdf/pyenv symlink-escape false positive. + * + * Check whether either the original (pre-resolution) candidate path or its + * resolved realpath target falls within a trusted parent directory. Checking + * both — not just the resolved target — credits that `commandPath` was already + * constructed from a trusted root by `getKnownToolPaths()`, so a symlink/junction + * placed there by a version manager (nvm-windows, nvm, asdf, pyenv) whose + * resolved target lives in a private per-version store outside + * `EXPECTED_PARENT_PATHS` is still recognized as legitimate. A symlink whose + * ORIGINAL location is also untrusted (not reachable through the trusted-root + * candidate generator) stays rejected. + */ +export const isLocationTrusted = async ( + commandPath: string, + realPath: string, + expectedParentPaths: string[], + isPathWithin: PathWithinFn, + realpath: (targetPath: string) => Promise +): Promise => { + for (const parent of expectedParentPaths) { + if (isPathWithin(commandPath, parent) || isPathWithin(realPath, parent)) { + return true; + } + + try { + const resolvedParent = await realpath(parent); + if (isPathWithin(commandPath, resolvedParent) || isPathWithin(realPath, resolvedParent)) { + return true; + } + } catch { + // Ignore missing/unresolvable parents and continue checking the remaining ones. + } + } + return false; +}; + +export type KnownPathCheckResult = { + installed: boolean; + commandPath: string | null; + reason: string | null; +}; + +/** + * #7774 — known-path short-circuit hides a genuinely runnable binary. + * + * Walk the known-install-path candidates for a tool. A genuine positive + * identification (installed, or installed-but-not-executable) returns + * immediately. Any other failure reason (unsafe_path/symlink_escape/ + * suspicious_size/not_file/…) is only REMEMBERED, not returned — a single stray + * artifact at one guessed location must not stop the remaining candidates (or + * the PATH fallback in the caller) from being tried. + */ +export const findKnownPathMatch = async ( + knownPaths: string[], + checkKnownPath: (candidatePath: string) => Promise +): Promise<{ match: T | null; bestFailure: T | null }> => { + let bestFailure: T | null = null; + for (const knownPath of knownPaths) { + const result = await checkKnownPath(knownPath); + if (result.installed) { + return { match: result, bestFailure: null }; + } + if (result.reason && result.reason !== "not_found" && !bestFailure) { + bestFailure = result; + } + } + return { match: null, bestFailure }; +}; diff --git a/tests/unit/cli-runtime-extended.test.ts b/tests/unit/cli-runtime-extended.test.ts index d81af103a0..8c2ef44845 100644 --- a/tests/unit/cli-runtime-extended.test.ts +++ b/tests/unit/cli-runtime-extended.test.ts @@ -330,11 +330,21 @@ test("getCliRuntimeStatus ignores suspicious known-path binaries and symlink esc fs.symlinkSync(outsideTarget, path.join(escapeBinDir, "qodercli")); process.env.npm_config_prefix = escapePrefix; + // #7753: every candidate checkKnownPath() ever receives is constructed by + // getKnownToolPaths() from a trusted root (here: npm_config_prefix/bin) — so a + // symlink placed exactly at that generated candidate path (the version-manager + // shim pattern used by nvm-windows/nvm/asdf/pyenv: trusted shim location, + // private per-version store target) is legitimate and must now be ACCEPTED even + // though its resolved target lives outside every EXPECTED_PARENT_PATHS entry. + // A genuinely unsafe symlink (one whose ORIGINAL location is also untrusted, not + // reachable through this known-path candidate generator) is covered separately + // by tests/unit/cliRuntime-symlink-escape-7753.test.ts via the exported + // checkKnownPath(). const escapedRuntime = await importFresh("symlink-escape"); const escapedStatus = await escapedRuntime.getCliRuntimeStatus("qoder"); - assert.equal(escapedStatus.installed, false); - assert.equal(escapedStatus.reason, "symlink_escape"); + assert.equal(escapedStatus.installed, true); + assert.equal(escapedStatus.reason, null); } }); diff --git a/tests/unit/cli-runtime-known-path-shortcircuit-7774.test.ts b/tests/unit/cli-runtime-known-path-shortcircuit-7774.test.ts new file mode 100644 index 0000000000..6551b227f7 --- /dev/null +++ b/tests/unit/cli-runtime-known-path-shortcircuit-7774.test.ts @@ -0,0 +1,67 @@ +import { describe, it, before, after } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +// HOME must be overridden BEFORE importing cliRuntime.ts — the module computes +// EXPECTED_PARENT_PATHS (the known-path realpath containment check) once at +// import time from os.homedir(). +const fakeHome = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-7774-home-")); +const realBinDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-7774-realbin-")); + +const savedEnv: Record = { + HOME: process.env.HOME, + USERPROFILE: process.env.USERPROFILE, + PATH: process.env.PATH, + CLI_CLAUDE_BIN: process.env.CLI_CLAUDE_BIN, + CLI_EXTRA_PATHS: process.env.CLI_EXTRA_PATHS, + npm_config_prefix: process.env.npm_config_prefix, +}; + +process.env.HOME = fakeHome; +process.env.USERPROFILE = fakeHome; +delete process.env.CLI_CLAUDE_BIN; +delete process.env.CLI_EXTRA_PATHS; +process.env.npm_config_prefix = path.join(fakeHome, "npm-prefix-unused"); + +const { getCliRuntimeStatus, getKnownToolPaths } = await import( + "../../src/shared/services/cliRuntime.ts" +); + +function makeExecutable(filePath: string, content: string) { + fs.writeFileSync(filePath, content); + if (process.platform !== "win32") fs.chmodSync(filePath, 0o755); +} + +describe("#7774 — known-path short-circuit hides a genuinely runnable Claude binary", () => { + before(() => { + const poisonedCandidate = path.join(fakeHome, ".local", "bin", "claude"); + fs.mkdirSync(poisonedCandidate, { recursive: true }); + + const known = getKnownToolPaths("claude"); + assert.ok(known.includes(poisonedCandidate)); + + const realClaude = path.join(realBinDir, "claude"); + makeExecutable(realClaude, "#!/bin/sh\necho '2.1.215 (Claude Code)'\n"); + // Prepend realBinDir rather than replacing PATH outright — locateCommand() + // spawns `sh`/`where.exe` itself using this same PATH, so the standard + // system bin dirs (containing `sh`) must stay resolvable too. + process.env.PATH = [realBinDir, savedEnv.PATH].filter(Boolean).join(path.delimiter); + }); + + after(() => { + for (const [key, value] of Object.entries(savedEnv)) { + if (value === undefined) delete (process.env as Record)[key]; + else process.env[key] = value; + } + fs.rmSync(fakeHome, { recursive: true, force: true }); + fs.rmSync(realBinDir, { recursive: true, force: true }); + }); + + it("should still find and report Claude as installed+runnable via PATH fallback", async () => { + const result = await getCliRuntimeStatus("claude"); + assert.equal(result.installed, true, `expected installed=true, got reason=${result.reason}`); + assert.equal(result.runnable, true, `expected runnable=true, got reason=${result.reason}`); + }); +}); diff --git a/tests/unit/cliRuntime-symlink-escape-7753.test.ts b/tests/unit/cliRuntime-symlink-escape-7753.test.ts new file mode 100644 index 0000000000..a36ba60ef4 --- /dev/null +++ b/tests/unit/cliRuntime-symlink-escape-7753.test.ts @@ -0,0 +1,60 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import fsp from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; + +// cliRuntime.ts computes EXPECTED_PARENT_PATHS from os.homedir() at MODULE LOAD +// time, so HOME must be redirected before the module is imported. +const sandboxHome = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-7753-home-")); +const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-7753-outside-")); + +process.env.HOME = sandboxHome; +process.env.USERPROFILE = sandboxHome; +process.env.npm_config_prefix = path.join(sandboxHome, "npm-prefix-unused"); +delete process.env.CLI_OPENCODE_BIN; + +const localBinDir = path.join(sandboxHome, ".local", "bin"); +fs.mkdirSync(localBinDir, { recursive: true }); + +const realBinaryPath = path.join(outsideDir, "opencode-real-binary"); +fs.writeFileSync(realBinaryPath, "#!/bin/sh\necho fake-opencode-binary-body-padding\n"); +fs.chmodSync(realBinaryPath, 0o755); + +// symlink lives INSIDE a trusted parent (HOME/.local/bin) — like nvm-windows's +// opencode.cmd under AppData\Local\nvm\\ — but its resolved target is outside. +const symlinkPath = path.join(localBinDir, "opencode"); +fs.symlinkSync(realBinaryPath, symlinkPath); + +const { getCliRuntimeStatus, checkKnownPath } = await import( + "../../src/shared/services/cliRuntime.ts" +); + +test("#7753: a CLI symlink located inside an expected parent dir is wrongly reported not-installed when its resolved target escapes EXPECTED_PARENT_PATHS", async () => { + const status = await getCliRuntimeStatus("opencode"); + assert.equal( + status.installed, + true, + `expected installed=true but got installed=${status.installed} reason=${status.reason}` + ); +}); + +test("#7753: a genuinely unsafe symlink whose ORIGINAL location is also untrusted must still be rejected", async () => { + // Neither the symlink's own location nor its resolved target is inside any + // EXPECTED_PARENT_PATHS entry — this must stay rejected as symlink_escape. + const untrustedDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-7753-untrusted-")); + const untrustedSymlink = path.join(untrustedDir, "opencode"); + fs.symlinkSync(realBinaryPath, untrustedSymlink); + + const result = await checkKnownPath(untrustedSymlink); + assert.equal(result.installed, false); + assert.equal(result.reason, "symlink_escape"); + + await fsp.rm(untrustedDir, { recursive: true, force: true }); +}); + +test.after(async () => { + await fsp.rm(sandboxHome, { recursive: true, force: true }); + await fsp.rm(outsideDir, { recursive: true, force: true }); +});