diff --git a/init-repo/tests/dev-shell-template.test.ts b/init-repo/tests/dev-shell-template.test.ts index 12fb1a17..4901e8f8 100644 --- a/init-repo/tests/dev-shell-template.test.ts +++ b/init-repo/tests/dev-shell-template.test.ts @@ -9,7 +9,10 @@ const TRANSPARENT = "rgba(0, 0, 0, 0)"; function readTemplateFile(relativePath: string) { const parts = relativePath.split(/[\\/]/); const resolved = parts[0] === "runtime" ? path.join(runtimeRoot, ...parts.slice(1)) : relativePath; - return fs.readFileSync(path.join(root, resolved), "utf-8"); + // Several assertions compare multi-line source text. A Windows checkout, and + // the template sync that writes these files on Windows, carry CRLF, so + // normalize before matching or those assertions can never pass there. + return fs.readFileSync(path.join(root, resolved), "utf-8").replace(/\r\n/g, "\n"); } function cssForRuntimeTokens() { diff --git a/packages/localapp/native/windows/src/main.rs b/packages/localapp/native/windows/src/main.rs index db586246..97b4213a 100644 --- a/packages/localapp/native/windows/src/main.rs +++ b/packages/localapp/native/windows/src/main.rs @@ -49,7 +49,7 @@ mod platform { use windows_sys::Win32::Foundation::{CloseHandle, HANDLE}; use windows_sys::Win32::System::JobObjects::{AssignProcessToJobObject, CreateJobObjectW, SetInformationJobObject, JOBOBJECT_EXTENDED_LIMIT_INFORMATION, JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, JobObjectExtendedLimitInformation}; use windows_sys::Win32::System::Registry::{RegCloseKey, RegCreateKeyExW, RegSetValueExW, HKEY, HKEY_CURRENT_USER, KEY_WRITE, REG_OPTION_NON_VOLATILE, REG_SZ}; - use windows_sys::Win32::System::Threading::{CreateProcessW, ResumeThread, TerminateProcess, WaitForSingleObject, PROCESS_INFORMATION, STARTUPINFOW, CREATE_SUSPENDED}; + use windows_sys::Win32::System::Threading::{CreateProcessW, GetExitCodeProcess, ResumeThread, TerminateProcess, WaitForSingleObject, PROCESS_INFORMATION, STARTUPINFOW, CREATE_SUSPENDED}; use windows_sys::Win32::UI::Shell::ShellExecuteW; use windows::core::{Error, HSTRING, Interface, PCWSTR, PWSTR}; use windows::Data::Xml::Dom::XmlDocument; @@ -123,12 +123,40 @@ mod platform { CloseHandle(process.hProcess); } + /// A `.cmd`/`.bat` target is not an executable image, so CreateProcessW + /// cannot load it directly; the interpreter has to run it instead. + fn is_command_script(value: &str) -> bool { + let lower = value.to_ascii_lowercase(); + lower.ends_with(".cmd") || lower.ends_with(".bat") + } + + fn command_interpreter() -> Result { + let from_environment = std::env::var("ComSpec").ok().filter(|value| safe_absolute_path(value)); + if let Some(value) = from_environment { return Ok(value); } + let system_root = std::env::var("SystemRoot").unwrap_or_else(|_| "C:\\Windows".to_string()); + let candidate = format!("{system_root}\\System32\\cmd.exe"); + if safe_absolute_path(&candidate) { Ok(candidate) } else { Err("command interpreter is unavailable".into()) } + } + /// Suspended create -> kill-on-close Job assignment -> resume. Every /// partial failure terminates and closes the root before it can escape. - pub unsafe fn job_owner(executable: &str, arguments: &[String]) -> Result<(), String> { + /// The owned root's exit code is returned so callers see a failing child + /// as a failure instead of a successful wrapper. + pub unsafe fn job_owner(executable: &str, arguments: &[String]) -> Result { if !safe_absolute_path(executable) { return Err("invalid executable".into()); } - let application_name = wide(executable); - let mut command_line = wide(&create_process_command_line(executable, arguments)); + // The interpreter supplies its own command line, so a script target is + // wrapped here rather than by the caller: this is the only layer that + // controls quoting for the single string cmd.exe parses. + let (application, command_line_text) = if is_command_script(executable) { + let interpreter = command_interpreter()?; + if !safe_absolute_path(&interpreter) { return Err("invalid command interpreter".into()); } + let inner = create_process_command_line(executable, arguments); + (interpreter, format!("/d /s /c \"{inner}\"")) + } else { + (executable.to_string(), create_process_command_line(executable, arguments)) + }; + let application_name = wide(&application); + let mut command_line = wide(&command_line_text); let mut startup: STARTUPINFOW = std::mem::zeroed(); startup.cb = std::mem::size_of::() as u32; let mut process: PROCESS_INFORMATION = std::mem::zeroed(); @@ -154,9 +182,12 @@ mod platform { CloseHandle(job); return Err("owned process wait failed".into()); } + let mut exit_code: u32 = 1; + let observed = GetExitCodeProcess(process.hProcess, &mut exit_code) != 0; CloseHandle(process.hProcess); CloseHandle(job); - Ok(()) + if !observed { return Err("owned process exit code is unavailable".into()); } + Ok(exit_code) } pub fn forward_scheme(config_path: &str, url: &str) -> Result<(), String> { @@ -295,19 +326,24 @@ mod platform { #[cfg(windows)] fn main() { let arguments = std::env::args().skip(1).collect::>(); - let result = match arguments.first().map(String::as_str) { + // Every arm reports an exit code so a failed owned process is not reported + // as a successful wrapper; commands without an owned process use 0. + let result: Result = match arguments.first().map(String::as_str) { Some("--job-owner") if arguments.len() >= 3 && arguments[1] == "--" => unsafe { platform::job_owner(&arguments[2], &arguments[3..]) }, - Some("--register") if arguments.len() == 3 && arguments[1] == "--config" => unsafe { platform::register_scheme(&arguments[2]) }, - Some("--scheme") if arguments.len() == 4 && arguments[1] == "--config" => platform::forward_scheme(&arguments[2], &arguments[3]), - Some("--open-url") if arguments.len() == 2 => unsafe { platform::open_external_url(&arguments[1]) }, - Some("--permission-state") if arguments.len() == 1 => { println!("{}", platform::notification_permission_state()); Ok(()) }, - Some("--request-permission") if arguments.len() == 1 => { println!("{}", platform::notification_permission_state()); Ok(()) }, - Some("--show-notification") if arguments.len() == 2 => platform::show_notification(&arguments[1]), + Some("--register") if arguments.len() == 3 && arguments[1] == "--config" => unsafe { platform::register_scheme(&arguments[2]).map(|()| 0) }, + Some("--scheme") if arguments.len() == 4 && arguments[1] == "--config" => platform::forward_scheme(&arguments[2], &arguments[3]).map(|()| 0), + Some("--open-url") if arguments.len() == 2 => unsafe { platform::open_external_url(&arguments[1]).map(|()| 0) }, + Some("--permission-state") if arguments.len() == 1 => { println!("{}", platform::notification_permission_state()); Ok(0) }, + Some("--request-permission") if arguments.len() == 1 => { println!("{}", platform::notification_permission_state()); Ok(0) }, + Some("--show-notification") if arguments.len() == 2 => platform::show_notification(&arguments[1]).map(|()| 0), Some("--validate-notification") if arguments.len() == 2 => localapp_native_contract::NotificationEnvelope::parse(&arguments[1], localapp_native_contract::Platform::Windows) - .and_then(|envelope| envelope.verify_icon()).map_err(String::from), + .and_then(|envelope| envelope.verify_icon()).map(|()| 0).map_err(String::from), _ => Err("unsupported native command".into()), }; - if result.is_err() { std::process::exit(1); } + match result { + Ok(code) => std::process::exit(code as i32), + Err(_) => std::process::exit(1), + } } #[cfg(not(windows))] diff --git a/packages/localapp/scripts/native-adapter.node-test.mjs b/packages/localapp/scripts/native-adapter.node-test.mjs index 82c12c34..26b41ffd 100644 --- a/packages/localapp/scripts/native-adapter.node-test.mjs +++ b/packages/localapp/scripts/native-adapter.node-test.mjs @@ -183,6 +183,27 @@ test("Windows helper preserves argv, has an explicit application path, and keeps assert.match(source, /ShellExecuteW/); }); +test("Windows helper reports the owned process exit code instead of always succeeding", async () => { + // Break caught: --job-owner waited for the root and returned Ok(()) without + // reading its exit code, so every owned command looked successful — a failing + // project script under `localapp dev` was reported as a passing one. + const source = await fs.readFile(path.join(repositoryRoot, "packages/localapp/native/windows/src/main.rs"), "utf8"); + assert.match(source, /GetExitCodeProcess\(process\.hProcess, &mut exit_code\)/); + assert.match(source, /Ok\(exit_code\)/); + assert.match(source, /Some\("--job-owner"\)[^;]*platform::job_owner/); + // The exit code must reach the process exit status, not be dropped in main. + assert.match(source, /Ok\(code\) => std::process::exit\(code as i32\)/); + // A .cmd/.bat target is not an executable image, so the wrapper has to run it + // through the interpreter itself; a caller cannot express cmd's quoting + // through the per-argument command line the wrapper rebuilds. + assert.match(source, /fn is_command_script\(value: &str\) -> bool/); + assert.match(source, /\/d \/s \/c /); + assert.match(source, /let \(application, command_line_text\) = if is_command_script\(executable\)/); + // A relative executable stays rejected: PATH resolution belongs to the + // caller, never to the owned-process wrapper. + assert.match(source, /if !safe_absolute_path\(executable\)/); +}); + test("Windows helper hands the shortcut property store a task-allocator buffer, never a Rust allocation", async () => { // Break caught: `--register` wrote the Scheme registry key and then died with // STATUS_HEAP_CORRUPTION before the CLI could install the daemon, because the diff --git a/packages/localapp/src/commands/dev.ts b/packages/localapp/src/commands/dev.ts index d7833550..5c51d0df 100644 --- a/packages/localapp/src/commands/dev.ts +++ b/packages/localapp/src/commands/dev.ts @@ -8,6 +8,7 @@ import type { CliIo } from "../cli/output.js"; import { readOrCreateDevCredentials } from "../dev/credentials.js"; import { lifecycleError } from "../errors.js"; import { LocalAppClient } from "../http/localapp-client.js"; +import { resolveOwnedCommand } from "../process/command-invocation.js"; import { spawnOwnedProcess, type OwnedProcess, type WindowsProcessTreeAdapter } from "../process/process-tree.js"; import { waitForServerReady } from "../process/readiness.js"; import type { ProjectCommandRunner } from "../project/check.js"; @@ -136,7 +137,7 @@ export async function runDev(options: RunDevOptions, dependencies: RunDevDepende "--strictPort", ]; lifecycle.assertActive(); - const vite = lifecycle.spawn(() => spawnProcess(configuredVite.command, viteArgs, { + const vite = lifecycle.spawn(() => spawnProcess(resolveOwnedCommand(configuredVite.command), viteArgs, { cwd: projectDir, env: { ...process.env, LOCALAPP_DEV_API_KEY: credentials.apiKey }, stdio: "ignore", @@ -332,8 +333,9 @@ function createOwnedProjectCommandRunner( ): ProjectCommandRunner { return async (invocation) => { lifecycle.assertActive(); - const command = process.platform === "win32" ? `${invocation.command}.cmd` : invocation.command; - const child = lifecycle.spawn(() => spawnProcess(command, invocation.args, { + // The wrapper validates an absolute executable path and handles a .cmd/.bat + // target itself, so resolve the shim instead of handing it a bare name. + const child = lifecycle.spawn(() => spawnProcess(resolveOwnedCommand(invocation.command), invocation.args, { cwd: invocation.cwd, stdio: "ignore", windowsAdapter, diff --git a/packages/localapp/src/commands/init.ts b/packages/localapp/src/commands/init.ts index 52cca3e7..55a98f90 100644 --- a/packages/localapp/src/commands/init.ts +++ b/packages/localapp/src/commands/init.ts @@ -5,6 +5,7 @@ import { fileURLToPath } from "node:url"; import type { CliIo } from "../cli/output.js"; import { LocalAppLifecycleError, lifecycleError } from "../errors.js"; import { isValidProjectName, writeProjectManifest } from "../project/manifest.js"; +import { resolveCommandInvocation } from "../process/command-invocation.js"; import { isManagedSkillName, verifyInitParent } from "../project/safety.js"; import { copyDirectory, isDirectory, type CopyDestinationMutations } from "../template/copy.js"; @@ -101,7 +102,8 @@ export function isManagedSkill(name: string): boolean { async function installDependencies(projectDirectory: string, io: CliIo): Promise { await new Promise((resolve, reject) => { - const child = spawn("npm", ["install"], { cwd: projectDirectory, stdio: ["ignore", "pipe", "pipe"] }); + const invocation = resolveCommandInvocation("npm", ["install"]); + const child = spawn(invocation.command, invocation.args, { cwd: projectDirectory, stdio: ["ignore", "pipe", "pipe"] }); child.stdout.setEncoding("utf8"); child.stderr.setEncoding("utf8"); child.stdout.on("data", (chunk) => io.stdout(chunk)); diff --git a/packages/localapp/src/process/command-invocation.ts b/packages/localapp/src/process/command-invocation.ts new file mode 100644 index 00000000..026cdbd8 --- /dev/null +++ b/packages/localapp/src/process/command-invocation.ts @@ -0,0 +1,102 @@ +import fs from "node:fs"; +import path from "node:path"; + +export interface ResolvedCommandInvocation { + command: string; + args: string[]; +} + +export interface ResolveCommandInvocationOptions { + platform?: NodeJS.Platform; + env?: NodeJS.ProcessEnv; + commandInterpreter?: string; + /** Path-existence seam so the resolution rules stay testable off-Windows. */ + isFile?: (candidate: string) => boolean; +} + +/** + * Turns a logical command into something this platform can actually spawn. + * + * Node refuses to spawn a `.cmd`/`.bat` shim without a shell since the + * CVE-2024-27980 fix, so `spawn("npm", …)` fails with ENOENT on Windows even + * though `npm.cmd` is on PATH. Routing a bare name through the command + * interpreter fixes that: the interpreter itself is absolute, and it resolves + * the shim through PATHEXT. + * + * Only bare names are rewritten; a caller that already holds a path keeps it. + * The arguments here are fixed internal values, never user input. + */ +export function resolveCommandInvocation( + command: string, + args: readonly string[], + options: ResolveCommandInvocationOptions = {}, +): ResolvedCommandInvocation { + const platform = options.platform ?? process.platform; + const env = options.env ?? process.env; + if (platform !== "win32" || !isBareCommandName(command)) return { command, args: [...args] }; + const interpreter = options.commandInterpreter ?? env.ComSpec ?? env.COMSPEC ?? defaultCommandInterpreter(env); + return { command: interpreter, args: ["/d", "/s", "/c", command, ...args] }; +} + +function isBareCommandName(command: string): boolean { + if (command.includes("/") || command.includes("\\") || command.includes("\0")) return false; + // A name with an extension (npm.cmd, node.exe) is still bare: cmd resolves it + // through PATHEXT exactly like the extensionless form. + return command.length > 0; +} + +function defaultCommandInterpreter(env: NodeJS.ProcessEnv): string { + const systemRoot = env.SystemRoot ?? env.SYSTEMROOT ?? "C:\\Windows"; + return path.win32.join(systemRoot, "System32", "cmd.exe"); +} + +/** + * Resolves a bare command name to the absolute file PATH would run, following + * PATHEXT on Windows. Returns undefined for a path (already resolved) or when + * nothing matches. + */ +export function resolveExecutablePath(command: string, options: ResolveCommandInvocationOptions = {}): string | undefined { + const platform = options.platform ?? process.platform; + const env = options.env ?? process.env; + if (!isBareCommandName(command)) return undefined; + const pathApi = platform === "win32" ? path.win32 : path.posix; + const searchPath = env.PATH ?? env.Path ?? env.path ?? ""; + // Windows resolves a bare name through PATHEXT, so an extensionless file of + // the same name (npm ships one: the POSIX shell script) must not win. + const extensions = platform === "win32" + ? (env.PATHEXT ?? ".COM;.EXE;.BAT;.CMD").split(";").filter(Boolean).map((entry) => entry.toLowerCase()) + : [""]; + const names = platform === "win32" + ? (pathApi.extname(command) === "" ? extensions.map((extension) => `${command}${extension}`) : [command]) + : [command]; + const isFile = options.isFile ?? ((candidate: string) => { + try { + return fs.statSync(candidate).isFile(); + } catch { + // A missing or unreadable candidate is not a match. + return false; + } + }); + for (const directory of searchPath.split(pathApi.delimiter)) { + const trimmed = directory.trim(); + if (trimmed === "") continue; + const unquoted = trimmed.length >= 2 && trimmed.startsWith("\"") && trimmed.endsWith("\"") ? trimmed.slice(1, -1) : trimmed; + for (const name of names) { + const candidate = pathApi.join(unquoted, name); + if (isFile(candidate)) return candidate; + } + } + return undefined; +} + +/** + * The command to hand the owned-process wrapper, which validates an absolute + * executable path and delegates a `.cmd`/`.bat` target to the command + * interpreter itself. Falls back to the original name so the wrapper reports a + * missing executable rather than this helper silently doing nothing. + */ +export function resolveOwnedCommand(command: string, options: ResolveCommandInvocationOptions = {}): string { + const platform = options.platform ?? process.platform; + if (platform !== "win32") return command; + return resolveExecutablePath(command, options) ?? command; +} diff --git a/packages/localapp/tests/command-invocation.test.ts b/packages/localapp/tests/command-invocation.test.ts new file mode 100644 index 00000000..ef395953 --- /dev/null +++ b/packages/localapp/tests/command-invocation.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it } from "vitest"; +import { resolveCommandInvocation, resolveExecutablePath, resolveOwnedCommand } from "../src/process/command-invocation.js"; + +const WINDOWS_ENV = { SystemRoot: "C:\\Windows" } as NodeJS.ProcessEnv; + +describe("command invocation resolution", () => { + it("routes a bare Windows package-manager name through the command interpreter", () => { + // Break caught: Node cannot spawn a .cmd shim without a shell, and the + // owned-process wrapper rejects a relative executable, so `npm`/`npm.cmd` + // reached neither CreateProcessW nor cmd — `localapp init` failed to install + // dependencies and `localapp dev` never started the project scripts. + for (const name of ["npm", "npm.cmd", "pnpm", "yarn", "bun"]) { + expect(resolveCommandInvocation(name, ["run", "test"], { platform: "win32", env: WINDOWS_ENV })).toEqual({ + command: "C:\\Windows\\System32\\cmd.exe", + args: ["/d", "/s", "/c", name, "run", "test"], + }); + } + }); + + it("honours an explicit interpreter and ComSpec", () => { + expect(resolveCommandInvocation("npm", ["install"], { platform: "win32", commandInterpreter: "D:\\cmd.exe" }).command).toBe("D:\\cmd.exe"); + expect(resolveCommandInvocation("npm", ["install"], { platform: "win32", env: { ComSpec: "D:\\Windows\\cmd.exe" } as NodeJS.ProcessEnv }).command).toBe("D:\\Windows\\cmd.exe"); + expect(resolveCommandInvocation("npm", ["install"], { platform: "win32", env: {} as NodeJS.ProcessEnv }).command).toBe("C:\\Windows\\System32\\cmd.exe"); + }); + + it("leaves an explicit path untouched so the owned-process wrapper keeps validating it", () => { + const absolute = "C:\\Program Files\\nodejs\\node.exe"; + expect(resolveCommandInvocation(absolute, ["-e", "1"], { platform: "win32", env: WINDOWS_ENV })).toEqual({ + command: absolute, + args: ["-e", "1"], + }); + expect(resolveCommandInvocation("C:\\tools\\npm.cmd", [], { platform: "win32", env: WINDOWS_ENV }).command).toBe("C:\\tools\\npm.cmd"); + }); + + it("keeps other platforms unchanged", () => { + expect(resolveCommandInvocation("npm", ["run", "test"], { platform: "linux" })).toEqual({ command: "npm", args: ["run", "test"] }); + expect(resolveCommandInvocation("npm", ["run", "test"], { platform: "darwin" })).toEqual({ command: "npm", args: ["run", "test"] }); + }); + + it("resolves a bare name to the file PATH would run, following PATHEXT", () => { + const env = { PATH: "C:\\tools\\first;C:\\tools\\second", PATHEXT: ".COM;.EXE;.BAT;.CMD" } as NodeJS.ProcessEnv; + // Windows resolves a bare name through PATHEXT, so npm's extensionless + // POSIX script must not win over the .cmd shim sitting next to it. + const present = new Set(["C:\\tools\\second\\npm", "C:\\tools\\second\\npm.cmd", "C:\\tools\\first\\other.exe"]); + const isFile = (candidate: string) => present.has(candidate); + expect(resolveExecutablePath("npm", { platform: "win32", env, isFile })).toBe("C:\\tools\\second\\npm.cmd"); + expect(resolveExecutablePath("missing", { platform: "win32", env, isFile })).toBeUndefined(); + // A name that already carries an extension is used as given. + expect(resolveExecutablePath("other.exe", { platform: "win32", env, isFile })).toBe("C:\\tools\\first\\other.exe"); + // A path is already resolved and is never rewritten. + expect(resolveExecutablePath("C:\\tools\\npm.cmd", { platform: "win32", env, isFile })).toBeUndefined(); + }); + + it("hands the owned-process wrapper an absolute shim path so it can wrap .cmd itself", () => { + // Break caught: the wrapper rejects a relative executable, so `localapp dev` + // could never start `npm.cmd`; it now receives the absolute shim and wraps + // it in the command interpreter on its own side. + const env = { PATH: "C:\\tools", PATHEXT: ".CMD" } as NodeJS.ProcessEnv; + const isFile = (candidate: string) => candidate === "C:\\tools\\npm.cmd"; + expect(resolveOwnedCommand("npm", { platform: "win32", env, isFile })).toBe("C:\\tools\\npm.cmd"); + expect(resolveOwnedCommand("npm", { platform: "linux", env })).toBe("npm"); + // Unresolvable names stay untouched so the wrapper reports the real failure. + expect(resolveOwnedCommand("absent", { platform: "win32", env, isFile })).toBe("absent"); + }); +}); diff --git a/packages/server/src/lib/app-installer.ts b/packages/server/src/lib/app-installer.ts index 341d5dca..38a77845 100644 --- a/packages/server/src/lib/app-installer.ts +++ b/packages/server/src/lib/app-installer.ts @@ -973,8 +973,10 @@ async function retainExactPackage( await fs.promises.chmod(tempPath, 0o600); const copied = await inspectAppPackage(tempPath); if (copied.digest !== inspected.digest) throw new AppInstallError("APP_PACKAGE_STORAGE_CORRUPT", "Retained package digest mismatch", 500); - const handle = await fs.promises.open(tempPath, "r"); - try { await handle.sync(); } finally { await handle.close(); } + // syncFile owns the Windows rule that FlushFileBuffers needs a + // write-capable handle: a read-only handle fails with EPERM there, which + // aborted every install that had to retain its package. + syncFile(tempPath); fs.renameSync(tempPath, finalPath); syncDirectory(directory); syncDirectory(pageDir);