diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 12aaf3a82..1a004ad39 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -546,6 +546,7 @@ FACESIZE FAILIFTHERE fastlink fcharset +ffi fgbg FGCOLOR FGHIJ @@ -670,6 +671,7 @@ GMEM Goldmine gonce goutput +gpt GPUs GREENSCROLL Grehan @@ -1340,6 +1342,7 @@ PUCHAR pvar pwch PWDDMCONSOLECONTEXT +pwned pws pwstr pwsz diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 742fec642..e84484bd9 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -31,6 +31,7 @@ #include "TerminalSettingsCache.h" #include "TerminalProtocolPipeServer.h" #include "WtaProcessLauncher.h" +#include "../inc/QuoteArgForCommandLine.h" #include "LaunchPositionRequest.g.cpp" #include "RenameWindowRequestedArgs.g.cpp" @@ -1163,37 +1164,37 @@ namespace winrt::TerminalApp::implementation const auto& globals = _settings.GlobalSettings(); const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); - // Helper: escape and quote an argument for the command line. - auto quoteArg = [](std::wstring_view arg) -> std::wstring { - std::wstring escaped{ arg }; - for (size_t pos = 0; (pos = escaped.find(L'"', pos)) != std::wstring::npos; pos += 2) - { - escaped.replace(pos, 1, L"\\\""); - } - return L"\"" + escaped + L"\""; - }; - - // Build: wta delegate --agent --delegate-agent "" - std::wstring cmdline = quoteArg(wtaPath) + L" delegate"; + using Microsoft::Terminal::CommandLine::QuoteArgForCommandLine; + using Microsoft::Terminal::CommandLine::QuoteProgramPath; - if (!agentCliPath.empty()) + // Build: wta delegate --agent-config --cwd "" + auto quotedPath = QuoteProgramPath(wtaPath); + if (!quotedPath) { - cmdline += L" --agent " + quoteArg(std::wstring_view{ agentCliPath }); + _agentPaneLog("ABORT: wta path contains invalid characters, cannot delegate"); + return; // Invalid WTA path (contains NUL or quote) — cannot launch. } + std::wstring cmdline = *quotedPath + L" delegate"; const auto delegateAgent = _ResolveEffectiveDelegateAgent(globals); - if (!delegateAgent.empty()) + const auto delegateModel = globals.DelegateModel(); + + // Pass agent config fields as JSON (same mechanism as the agent pane). + if (auto agentArg = Microsoft::Terminal::CommandLine::BuildAgentConfigArg( + std::wstring_view{ agentCliPath }, + std::wstring_view{} /* agentId — not needed for delegate */, + std::wstring_view{ delegateAgent }, + std::wstring_view{ delegateModel }, + std::wstring_view{} /* acpModel */)) { - cmdline += L" --delegate-agent " + quoteArg(std::wstring_view{ delegateAgent }); + cmdline += *agentArg; } - const auto delegateModel = globals.DelegateModel(); - if (!delegateModel.empty()) + else { - cmdline += L" --delegate-model " + quoteArg(std::wstring_view{ delegateModel }); + _agentPaneLog("ABORT: failed to build agent config for delegate"); + return; } - - // Pass CWD from the active pane. winrt::hstring activeCwd; if (const auto& activeControl = _GetActiveControl()) @@ -1210,16 +1211,24 @@ namespace winrt::TerminalApp::implementation } if (!activeCwd.empty()) { - cmdline += L" --cwd " + quoteArg(std::wstring_view{ activeCwd }); + if (auto q = QuoteArgForCommandLine(std::wstring_view{ activeCwd })) + { + cmdline += L" --cwd " + *q; + } + else + { + _agentPaneLog("WARNING: delegate CWD contains invalid characters, omitting --cwd"); + } } - // Append the prompt as a positional argument. - std::wstring escapedPrompt{ prompt }; - for (size_t pos = 0; (pos = escapedPrompt.find(L'"', pos)) != std::wstring::npos; pos += 2) + // Append the prompt as a positional argument (required by wta delegate). + auto quotedPrompt = QuoteArgForCommandLine(std::wstring_view{ prompt }); + if (!quotedPrompt) { - escapedPrompt.replace(pos, 1, L"\"\""); + _agentPaneLog("ABORT: delegate prompt contains invalid characters"); + return; // Prompt contains embedded NUL — cannot safely launch. } - cmdline += fmt::format(FMT_COMPILE(L" \"{}\""), escapedPrompt); + cmdline += L" " + *quotedPrompt; _agentPaneLog("launching: " + winrt::to_string(winrt::hstring{ cmdline })); @@ -1720,7 +1729,13 @@ namespace winrt::TerminalApp::implementation } const auto& globals = _settings.GlobalSettings(); - std::wstring cmdline = std::wstring{ wtaPath }; + auto quotedWtaPath = Microsoft::Terminal::CommandLine::QuoteProgramPath(wtaPath); + if (!quotedWtaPath) + { + _agentPaneLog("ABORT: wta path contains invalid characters"); + return; + } + std::wstring cmdline = *quotedWtaPath; // Tell wta which tab owns its pane up-front (passed as a hidden CLI // arg). wta seeds app_state.tab_id from this before any ACP work @@ -1730,59 +1745,40 @@ namespace winrt::TerminalApp::implementation // _NotifyAgentTabChanged → tab_changed events. if (const auto stableId = tab->StableId(); !stableId.empty()) { - cmdline += fmt::format(FMT_COMPILE(L" --owner-tab-id \"{}\""), std::wstring_view{ stableId }); + if (auto q = Microsoft::Terminal::CommandLine::QuoteArgForCommandLine(std::wstring_view{ stableId })) + { + cmdline += L" --owner-tab-id " + *q; + } + else + { + _agentPaneLog("ABORT: owner-tab-id contains invalid characters"); + return; + } } const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); - if (!agentCliPath.empty()) - { - std::wstring s{ agentCliPath }; - for (size_t pos = 0; (pos = s.find(L'"', pos)) != std::wstring::npos; pos += 2) - s.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --agent \"{}\""), s); - } - - // See `_OpenOrReuseAgentPane` for the rationale: wta needs the - // canonical `acpAgent` setting value (not the expanded command - // line) to drive the session-management view's CLI filter. - if (const auto acpAgent = globals.AcpAgent(); !acpAgent.empty()) - { - std::wstring s{ acpAgent }; - for (size_t pos = 0; (pos = s.find(L'"', pos)) != std::wstring::npos; pos += 2) - s.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --agent-id \"{}\""), s); - } - + const auto acpAgent = globals.AcpAgent(); const auto delegateAgent = _ResolveEffectiveDelegateAgent(globals); - if (!delegateAgent.empty()) - { - std::wstring s{ delegateAgent }; - for (size_t pos = 0; (pos = s.find(L'"', pos)) != std::wstring::npos; pos += 2) - s.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --delegate-agent \"{}\""), s); - } - const auto delegateModel = globals.DelegateModel(); - if (!delegateModel.empty()) + const auto acpModel = globals.AcpModel(); + + // Pass all agent-related config as a single JSON-encoded argument. + // This eliminates per-field hand-rolled escaping — BuildAgentConfigArg + // performs RFC 8259 JSON encoding internally, and only one correctly- + // quoted argument boundary is needed (via QuoteArgForCommandLine). + if (auto agentArg = Microsoft::Terminal::CommandLine::BuildAgentConfigArg( + std::wstring_view{ agentCliPath }, + std::wstring_view{ acpAgent }, + std::wstring_view{ delegateAgent }, + std::wstring_view{ delegateModel }, + std::wstring_view{ acpModel })) { - std::wstring s{ delegateModel }; - for (size_t pos = 0; (pos = s.find(L'"', pos)) != std::wstring::npos; pos += 2) - s.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --delegate-model \"{}\""), s); + cmdline += *agentArg; } - - // Pass the ACP model selection out-of-band so it works for adapter - // launches (claude/codex via npx) where --model can't be put on the - // adapter cmdline. wta sends this via ACP setSessionModel after - // handshake; for copilot/gemini it's redundant (their --model is - // already on the agent cmdline) but harmless. - const auto acpModel = globals.AcpModel(); - if (!acpModel.empty()) + else { - std::wstring s{ acpModel }; - for (size_t pos = 0; (pos = s.find(L'"', pos)) != std::wstring::npos; pos += 2) - s.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --acp-model \"{}\""), s); + _agentPaneLog("ABORT: failed to build agent config"); + return; } if (!globals.AutoFixEnabled()) @@ -2118,48 +2114,33 @@ namespace winrt::TerminalApp::implementation // Build the wta command line (single-process TUI mode, no subcommand). if (const auto wtaPath = _DetectWtaPath(); !wtaPath.empty()) { - cmdline = std::wstring{ wtaPath }; - - const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); - if (!agentCliPath.empty()) + auto quotedWtaPath = Microsoft::Terminal::CommandLine::QuoteProgramPath(wtaPath); + if (!quotedWtaPath) { - std::wstring agentStr{ agentCliPath }; - for (size_t pos = 0; (pos = agentStr.find(L'"', pos)) != std::wstring::npos; pos += 2) - agentStr.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --agent \"{}\""), agentStr); - } - - // Tell wta which `acpAgent` setting value produced this launch - // — wta needs the canonical id ("copilot" / "claude" / "codex" - // / "gemini" / "custom:…") to drive the session-management - // view's CLI filter, and parsing it back out of the expanded - // `--agent` command line is fragile (adapter launches expand - // to "npx -y …" and lose the agent's name). Passing it through - // here keeps a single source of truth. - if (const auto acpAgent = globals.AcpAgent(); !acpAgent.empty()) - { - std::wstring idStr{ acpAgent }; - for (size_t pos = 0; (pos = idStr.find(L'"', pos)) != std::wstring::npos; pos += 2) - idStr.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --agent-id \"{}\""), idStr); + _agentPaneLog("ABORT: wta path contains invalid characters"); + return; } + cmdline = *quotedWtaPath; + const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); + const auto acpAgent = globals.AcpAgent(); const auto delegateAgent = _ResolveEffectiveDelegateAgent(globals); - if (!delegateAgent.empty()) + const auto delegateModel = globals.DelegateModel(); + + // Pass all agent-related config as a single JSON-encoded argument. + if (auto agentArg = Microsoft::Terminal::CommandLine::BuildAgentConfigArg( + std::wstring_view{ agentCliPath }, + std::wstring_view{ acpAgent }, + std::wstring_view{ delegateAgent }, + std::wstring_view{ delegateModel }, + std::wstring_view{} /* acpModel — not used in this path */)) { - std::wstring delegateStr{ delegateAgent }; - for (size_t pos = 0; (pos = delegateStr.find(L'"', pos)) != std::wstring::npos; pos += 2) - delegateStr.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --delegate-agent \"{}\""), delegateStr); + cmdline += *agentArg; } - - const auto delegateModel = globals.DelegateModel(); - if (!delegateModel.empty()) + else { - std::wstring modelStr{ delegateModel }; - for (size_t pos = 0; (pos = modelStr.find(L'"', pos)) != std::wstring::npos; pos += 2) - modelStr.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --delegate-model \"{}\""), modelStr); + _agentPaneLog("ABORT: failed to build agent config"); + return; } if (!globals.AutoFixEnabled()) diff --git a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp index 476d50a54..f4fcdefd1 100644 --- a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp @@ -10,6 +10,7 @@ #include "../inc/AgentRegistry.h" #include "../inc/AgentHooksStatus.h" #include "../inc/WtaProcess.h" +#include "../inc/QuoteArgForCommandLine.h" #include @@ -966,17 +967,16 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation std::string stdoutText; if (!wtaPath.empty()) { - // Quote-escape internal `"` per Windows CRT rules. - std::wstring escaped = agentCmdline; - for (size_t pos = 0; (pos = escaped.find(L'"', pos)) != std::wstring::npos; pos += 2) + // Use correct CommandLineToArgvW quoting for the agent argument. + auto quoted = ::Microsoft::Terminal::CommandLine::QuoteArgForCommandLine(std::wstring_view{ agentCmdline }); + if (quoted) { - escaped.replace(pos, 1, L"\"\""); + const std::wstring args = L"probe-models --agent " + *quoted; + // 40s ceiling matches probe.rs's internal limits (npx + // initialize 25s + new_session 10s + slack). Cached + // adapters return in <2s. + stdoutText = ::Microsoft::Terminal::WtaProcess::RunWtaCaptureStdout(wtaPath, args, 40'000); } - const std::wstring args = L"probe-models --agent \"" + escaped + L"\""; - // 40s ceiling matches probe.rs's internal limits (npx - // initialize 25s + new_session 10s + slack). Cached - // adapters return in <2s. - stdoutText = ::Microsoft::Terminal::WtaProcess::RunWtaCaptureStdout(wtaPath, args, 40'000); } std::vector parsed; diff --git a/src/cascadia/inc/QuoteArgForCommandLine.h b/src/cascadia/inc/QuoteArgForCommandLine.h new file mode 100644 index 000000000..265f17969 --- /dev/null +++ b/src/cascadia/inc/QuoteArgForCommandLine.h @@ -0,0 +1,211 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// QuoteArgForCommandLine.h +// +// Correct CommandLineToArgvW-compatible quoting for a single argument. +// Eliminates hand-rolled escaping throughout the codebase. Use this +// whenever building a commandline string for CreateProcess/ShellExecute. +// +// Pure Win32 + STL, no WinRT dependency. Returns empty optional on +// invalid input (embedded NUL, invalid program path) or allocation failure. + +#pragma once + +#include +#include +#include +#include + +namespace Microsoft::Terminal::CommandLine +{ + // Quote a single argument for use in a Windows commandline string. + // The result is always wrapped in double quotes for unambiguous parsing + // by CommandLineToArgvW. Handles: + // - Backslashes before `"` are doubled (2n+1 backslashes + `"`) + // - Trailing backslashes before the closing `"` are doubled + // - All other characters are passed through literally + // + // Returns std::nullopt if the argument contains embedded NUL (which + // would silently truncate the commandline at the OS level). + // + // NOTE: This is for argv[1..n] only. argv[0] (the program path) has + // different rules — use QuoteProgramPath() for that. + inline std::optional QuoteArgForCommandLine(std::wstring_view arg) + { + try + { + // Reject embedded NUL — it would truncate the commandline. + for (const auto ch : arg) + { + if (ch == L'\0') + { + return std::nullopt; + } + } + + std::wstring result; + result.reserve(arg.size() + 8); + result.push_back(L'"'); + + size_t backslashes = 0; + for (const auto ch : arg) + { + if (ch == L'\\') + { + ++backslashes; + } + else if (ch == L'"') + { + // Double the accumulated backslashes, then emit \" + result.append(backslashes * 2 + 1, L'\\'); + result.push_back(L'"'); + backslashes = 0; + } + else + { + // Flush any accumulated backslashes as-is + result.append(backslashes, L'\\'); + backslashes = 0; + result.push_back(ch); + } + } + // Trailing backslashes must be doubled (they precede the closing `"`) + result.append(backslashes * 2, L'\\'); + result.push_back(L'"'); + + return result; + } + catch (...) + { + return std::nullopt; + } + } + + // Quote a program path (argv[0]) for use in a Windows commandline string. + // argv[0] has simpler rules than argv[1..n]: backslashes are always literal + // and `"` cannot be escaped inside it. We wrap in quotes (for paths with + // spaces) and reject paths containing `"` or NUL (which are invalid on + // Windows file systems anyway). + // + // Returns std::nullopt if the path contains `"` or embedded NUL. + inline std::optional QuoteProgramPath(std::wstring_view path) + { + try + { + for (const auto ch : path) + { + if (ch == L'"' || ch == L'\0') + { + return std::nullopt; + } + } + std::wstring result; + result.reserve(path.size() + 2); + result.push_back(L'"'); + result.append(path); + result.push_back(L'"'); + return result; + } + catch (...) + { + return std::nullopt; + } + } + + // Build a JSON-encoded `--agent-config` argument from the given fields. + // Returns the full fragment: ` --agent-config ""` + // Uses manual RFC 8259-compliant JSON construction (no external JSON + // library dependency) and QuoteArgForCommandLine for the single + // argument boundary. + // + // Usage: + // cmdline += BuildAgentConfigArg(agentCli, agentId, delegateAgent, + // delegateModel, acpModel); + // + // Any empty field is omitted from the JSON (the Rust side uses + // Option and falls back to defaults for missing fields). + // + // All control characters in field values (including NUL) are escaped + // per RFC 8259, so the resulting JSON blob is always valid and safe + // for commandline transport. Returns std::nullopt on (unlikely) + // quoting failure; callers should log and abort. + inline std::optional BuildAgentConfigArg( + std::wstring_view agent, + std::wstring_view agentId, + std::wstring_view delegateAgent, + std::wstring_view delegateModel, + std::wstring_view acpModel) + { + // Build a compact JSON object with only non-empty fields. + // We use manual JSON construction to avoid pulling in JsonCpp here + // (this header is used in both TerminalApp and TerminalSettingsEditor). + // The JSON spec is simple enough for known-safe field names: only the + // VALUES need escaping, and we do it correctly per RFC 8259. + // NUL characters in values are escaped as \u0000 (valid JSON), so + // they don't pose a truncation risk in the final commandline. + auto jsonEscapeValue = [](std::wstring_view val) -> std::wstring { + std::wstring out; + out.reserve(val.size() + 4); + for (const auto ch : val) + { + switch (ch) + { + case L'"': out += L"\\\""; break; + case L'\\': out += L"\\\\"; break; + case L'\b': out += L"\\b"; break; + case L'\f': out += L"\\f"; break; + case L'\n': out += L"\\n"; break; + case L'\r': out += L"\\r"; break; + case L'\t': out += L"\\t"; break; + default: + if (ch < 0x20) + { + wchar_t buf[8]; + swprintf_s(buf, L"\\u%04x", static_cast(ch)); + out += buf; + } + else + { + out.push_back(ch); + } + break; + } + } + return out; + }; + + std::wstring json = L"{"; + bool first = true; + + auto appendField = [&](const wchar_t* key, std::wstring_view val) { + if (val.empty()) + return; + if (!first) + json += L','; + first = false; + json += L'"'; + json += key; + json += L"\":\""; + json += jsonEscapeValue(val); + json += L'"'; + }; + + appendField(L"agent", agent); + appendField(L"agentId", agentId); + appendField(L"delegateAgent", delegateAgent); + appendField(L"delegateModel", delegateModel); + appendField(L"acpModel", acpModel); + + json += L'}'; + + // The JSON blob itself won't contain NUL (all control chars are + // escaped above), so QuoteArgForCommandLine won't fail here. + auto quoted = QuoteArgForCommandLine(json); + if (!quoted) + { + return std::nullopt; + } + return L" --agent-config " + *quoted; + } +} diff --git a/tools/wta/Cargo.toml b/tools/wta/Cargo.toml index 3a9d9b20c..654c4dbdd 100644 --- a/tools/wta/Cargo.toml +++ b/tools/wta/Cargo.toml @@ -27,6 +27,7 @@ windows-sys = { version = "0.61", features = [ "Win32_System_Environment", "Win32_System_Registry", "Win32_System_Threading", + "Win32_UI_Shell", ] } tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter", "fmt"] } diff --git a/tools/wta/src/cmdline.rs b/tools/wta/src/cmdline.rs new file mode 100644 index 000000000..f9ee53365 --- /dev/null +++ b/tools/wta/src/cmdline.rs @@ -0,0 +1,408 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// Pure functions for building Windows commandline strings. +// Extracted from shell_manager for testability. + +/// Error returned by [`build_wt_commandline`] when the input cannot be +/// encoded as a valid Windows commandline. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BuildCommandlineError { + /// The program path (argv[0]) contains a literal `"`. There is no + /// `CommandLineToArgvW`-compatible way to escape it. + QuoteInProgram, + /// The program path contains a NUL byte. + NulInProgram, + /// An argument contains a NUL byte. + NulInArgument, +} + +impl std::fmt::Display for BuildCommandlineError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::QuoteInProgram => { + f.write_str("executable path cannot contain a literal double quote") + } + Self::NulInProgram => f.write_str("executable path cannot contain a NUL byte"), + Self::NulInArgument => f.write_str("argument cannot contain a NUL byte"), + } + } +} + +impl std::error::Error for BuildCommandlineError {} + +/// Quote the program path (argv[0]). `CommandLineToArgvW` treats the first +/// token specially: backslashes are literal, and the first unescaped `"` +/// ends argv[0]. There is no way to escape `"` inside it, so we reject +/// inputs containing `"`. +fn append_program(cmdline: &mut String, value: &str) -> Result<(), BuildCommandlineError> { + if value.contains('\0') { + return Err(BuildCommandlineError::NulInProgram); + } + if value.contains('"') { + return Err(BuildCommandlineError::QuoteInProgram); + } + cmdline.push('"'); + cmdline.push_str(value); + cmdline.push('"'); + Ok(()) +} + +/// Append a non-first argument, quoting per `CommandLineToArgvW` rules. +/// Always quotes unconditionally — a `needs_quotes` heuristic is fragile +/// because the OS parser splits on more than just space/tab. +fn append_arg(cmdline: &mut String, value: &str) -> Result<(), BuildCommandlineError> { + if value.contains('\0') { + return Err(BuildCommandlineError::NulInArgument); + } + cmdline.push('"'); + let mut backslashes: usize = 0; + for ch in value.chars() { + match ch { + '\\' => backslashes += 1, + '"' => { + // Double the backslashes before a `"`, then escape the `"`. + for _ in 0..(backslashes * 2 + 1) { + cmdline.push('\\'); + } + cmdline.push('"'); + backslashes = 0; + } + _ => { + for _ in 0..backslashes { + cmdline.push('\\'); + } + backslashes = 0; + cmdline.push(ch); + } + } + } + // Trailing backslashes must be doubled (they precede the closing `"`). + for _ in 0..(backslashes * 2) { + cmdline.push('\\'); + } + cmdline.push('"'); + Ok(()) +} + +/// Build a commandline string from a command and its arguments. +/// +/// The result is compatible with `CommandLineToArgvW` / `CreateProcess`. +/// There is no shell in this pipeline, so metacharacters like `&` / `|` +/// are not special. +pub fn build_wt_commandline( + command: &str, + args: &[String], +) -> Result { + let mut cmdline = String::new(); + append_program(&mut cmdline, command)?; + for arg in args { + cmdline.push(' '); + append_arg(&mut cmdline, arg)?; + } + Ok(cmdline) +} + +#[cfg(all(test, windows))] +mod tests { + use super::*; + + /// Parse a commandline string through the real Windows OS parser. + /// This is the ground truth — if our output round-trips through + /// `CommandLineToArgvW` and matches the original input, we're correct. + fn parse_via_os(cmdline: &str) -> Vec { + use std::ffi::OsStr; + use std::os::windows::ffi::OsStrExt; + use windows_sys::Win32::Foundation::LocalFree; + use windows_sys::Win32::UI::Shell::CommandLineToArgvW; + + let wide: Vec = OsStr::new(cmdline) + .encode_wide() + .chain(std::iter::once(0)) + .collect(); + + let mut argc: i32 = 0; + let argv = unsafe { CommandLineToArgvW(wide.as_ptr(), &mut argc) }; + assert!(!argv.is_null(), "CommandLineToArgvW returned null"); + + let mut parsed = Vec::with_capacity(argc as usize); + for i in 0..argc as isize { + let ptr = unsafe { *argv.offset(i) }; + let mut len = 0isize; + while unsafe { *ptr.offset(len) } != 0 { + len += 1; + } + let slice = unsafe { std::slice::from_raw_parts(ptr, len as usize) }; + parsed.push(String::from_utf16(slice).expect("invalid UTF-16 from OS")); + } + unsafe { LocalFree(argv as _) }; + parsed + } + + /// Helper: build commandline, parse via OS, assert round-trip matches. + fn assert_roundtrip(command: &str, args: &[&str]) { + let args_owned: Vec = args.iter().map(|s| s.to_string()).collect(); + let cmdline = + build_wt_commandline(command, &args_owned).expect("build_wt_commandline failed"); + let parsed = parse_via_os(&cmdline); + + let mut expected = vec![command.to_string()]; + expected.extend(args_owned); + + assert_eq!( + parsed, expected, + "\n cmdline = {:?}\n parsed = {:?}\n expected= {:?}", + cmdline, parsed, expected, + ); + } + + // ── Basic cases ──────────────────────────────────────────────── + + #[test] + fn simple_command_no_args() { + assert_roundtrip("pwsh.exe", &[]); + } + + #[test] + fn simple_command_with_args() { + assert_roundtrip("pwsh.exe", &["-c", "git status"]); + } + + #[test] + fn command_with_spaces_in_path() { + assert_roundtrip("C:\\Program Files\\my tool\\run.exe", &["--verbose"]); + } + + // ── Quoting edge cases ───────────────────────────────────────── + + #[test] + fn arg_with_embedded_double_quote() { + assert_roundtrip("cmd.exe", &["/c", "echo \"hello\""]); + } + + #[test] + fn arg_with_only_double_quotes() { + assert_roundtrip("test.exe", &["\"\"\""]); + } + + #[test] + fn arg_with_spaces_and_quotes() { + assert_roundtrip("test.exe", &["hello \"world\" foo"]); + } + + // ── Backslash edge cases ─────────────────────────────────────── + + #[test] + fn arg_with_trailing_backslash() { + assert_roundtrip("test.exe", &["C:\\path\\"]); + } + + #[test] + fn arg_with_trailing_backslashes() { + assert_roundtrip("test.exe", &["C:\\path\\\\\\"]); + } + + #[test] + fn arg_with_backslash_before_quote() { + assert_roundtrip("test.exe", &["foo\\\"bar"]); + } + + #[test] + fn arg_with_multiple_backslashes_before_quote() { + assert_roundtrip("test.exe", &["foo\\\\\"bar"]); + } + + #[test] + fn arg_backslashes_not_before_quote() { + assert_roundtrip("test.exe", &["C:\\Users\\test\\file.txt"]); + } + + // ── Whitespace edge cases ────────────────────────────────────── + + #[test] + fn arg_with_tab() { + assert_roundtrip("test.exe", &["hello\t world"]); + } + + #[test] + fn arg_with_newline() { + assert_roundtrip("test.exe", &["line1\n line2"]); + } + + #[test] + fn arg_with_carriage_return() { + assert_roundtrip("test.exe", &["line1\r line2"]); + } + + #[test] + fn empty_arg() { + assert_roundtrip("test.exe", &[""]); + } + + #[test] + fn multiple_empty_args() { + assert_roundtrip("test.exe", &["", "", ""]); + } + + // ── Shell metacharacters (should be literal, no shell) ───────── + + #[test] + fn arg_with_pipe() { + assert_roundtrip("test.exe", &["foo|bar"]); + } + + #[test] + fn arg_with_ampersand() { + assert_roundtrip("test.exe", &["foo&bar"]); + } + + #[test] + fn arg_with_percent() { + assert_roundtrip("test.exe", &["%PATH%"]); + } + + #[test] + fn arg_with_caret() { + assert_roundtrip("test.exe", &["foo^bar"]); + } + + // ── Error cases ──────────────────────────────────────────────── + + #[test] + fn rejects_quote_in_program() { + let result = build_wt_commandline("bad\"path.exe", &[]); + assert_eq!(result, Err(BuildCommandlineError::QuoteInProgram)); + } + + #[test] + fn rejects_nul_in_program() { + let result = build_wt_commandline("bad\0path.exe", &[]); + assert_eq!(result, Err(BuildCommandlineError::NulInProgram)); + } + + #[test] + fn rejects_nul_in_argument() { + let args = vec!["hello\0world".to_string()]; + let result = build_wt_commandline("test.exe", &args); + assert_eq!(result, Err(BuildCommandlineError::NulInArgument)); + } + + // ── Stress / combo cases ─────────────────────────────────────── + + #[test] + fn many_args_mixed() { + assert_roundtrip( + "C:\\Program Files\\app.exe", + &[ + "--flag", + "simple", + "has spaces", + "has\"quote", + "trailing\\", + "back\\\"slash-quote", + "", + "\\\\server\\share\\", + "multi\n line\t tab", + ], + ); + } + + #[test] + fn realistic_agent_command() { + assert_roundtrip( + "pwsh.exe", + &["-NoProfile", "-Command", "& { git log --oneline -5 }"], + ); + } + + #[test] + fn realistic_npx_adapter() { + assert_roundtrip( + "npx", + &["-y", "@zed-industries/claude-code-acp"], + ); + } + + // ── JSON-in-commandline-arg round-trip ────────────────────────── + // These simulate the WT→WTA flow: WT serializes agent config as JSON, + // passes it as a single `--agent-config` argument. We verify the JSON + // survives the commandline quoting + CommandLineToArgvW round-trip and + // can be deserialized back to the original structured data. + + #[test] + fn json_arg_simple() { + let json = r#"{"agent":"copilot --acp --stdio","agentId":"copilot"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_with_quotes_in_value() { + // Agent command contains quotes (e.g. a path with spaces that was quoted) + let json = r#"{"agent":"\"C:\\Program Files\\agent.exe\" --acp","agentId":"custom"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_with_backslashes() { + // Windows paths with backslashes inside JSON values + let json = r#"{"agent":"C:\\Users\\test\\copilot.exe --acp --stdio","agentId":"copilot"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_all_fields() { + let json = r#"{"agent":"copilot --acp --stdio","agentId":"copilot","delegateAgent":"codex","delegateModel":"gpt-4o","acpModel":"claude-sonnet"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_unicode_values() { + let json = r#"{"agent":"copilot --acp","agentId":"自定义代理"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_with_special_chars_in_model() { + // Model name with characters that could trip up naive escaping + let json = r#"{"agent":"copilot","acpModel":"org/model-name:v1.0 (beta)"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_adversarial_injection_attempt() { + // Attacker tries to break out of JSON arg to inject --no-autofix + let json = r#"{"agent":"evil\" --no-autofix --agent \"pwned"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_deserialization_roundtrip() { + // Full end-to-end: build commandline with JSON arg, parse via OS, + // then deserialize the JSON to verify structured data survives. + use serde_json::Value; + + let config = serde_json::json!({ + "agent": "copilot --acp --stdio", + "agentId": "copilot", + "delegateAgent": "codex --model gpt-4", + "delegateModel": "gpt-4o", + "acpModel": "claude-3.5-sonnet" + }); + let json_str = serde_json::to_string(&config).unwrap(); + + let args = vec![ + "--agent-config".to_string(), + json_str.clone(), + ]; + let cmdline = build_wt_commandline("wta.exe", &args).unwrap(); + let parsed = parse_via_os(&cmdline); + + // parsed[0] = "wta.exe", parsed[1] = "--agent-config", parsed[2] = json + assert_eq!(parsed.len(), 3); + assert_eq!(parsed[1], "--agent-config"); + + // The JSON must deserialize back to the same structured data + let recovered: Value = serde_json::from_str(&parsed[2]).unwrap(); + assert_eq!(recovered, config); + } +} diff --git a/tools/wta/src/main.rs b/tools/wta/src/main.rs index 45ed54ddd..508e8046b 100644 --- a/tools/wta/src/main.rs +++ b/tools/wta/src/main.rs @@ -6,6 +6,7 @@ mod agent_registry; mod agent_sessions; mod agent_hooks_installer; mod app; +pub mod cmdline; mod commands; mod coordinator; mod event; @@ -102,6 +103,31 @@ fn normalize_locale(locale: &str) -> String { // ─── CLI Definition ───────────────────────────────────────────────────────── +/// JSON-encoded agent configuration passed via `--agent-config`. +/// All fields are optional; missing fields fall back to individual CLI args. +#[derive(Debug, serde::Deserialize)] +#[serde(rename_all = "camelCase")] +struct AgentConfig { + agent: Option, + agent_id: Option, + delegate_agent: Option, + delegate_model: Option, + acp_model: Option, +} + +/// Parse a JSON `--agent-config` value and return the deserialized config. +/// Returns `None` if `config_json` is `None`; errors on invalid JSON. +fn parse_agent_config(config_json: Option<&str>) -> anyhow::Result> { + match config_json { + None => Ok(None), + Some(json) => { + let config: AgentConfig = serde_json::from_str(json) + .context("--agent-config: invalid JSON")?; + Ok(Some(config)) + } + } +} + #[derive(Parser, Debug)] #[command( name = "wta", @@ -150,6 +176,22 @@ struct Cli { #[arg(long)] delegate_model: Option, + /// JSON-encoded agent configuration. When present, fields from this + /// JSON object override the corresponding individual CLI args above. + /// This eliminates hand-rolled commandline escaping on the host side — + /// JSON encoding handles all special characters, and only one + /// correctly-quoted argument boundary is needed. + /// + /// Expected shape: + /// ```json + /// {"agent":"copilot --acp --stdio","agentId":"copilot", + /// "delegateAgent":"codex","delegateModel":"gpt-4","acpModel":"..."} + /// ``` + /// All fields are optional; missing fields fall back to the + /// corresponding individual CLI arg or its default. + #[arg(long, hide = true)] + agent_config: Option, + /// Disable auto-fix on command failure #[arg(long)] no_autofix: bool, @@ -349,6 +391,10 @@ enum Command { /// Working directory for the delegate agent tab #[arg(long)] cwd: Option, + + /// JSON-encoded agent configuration (same as top-level --agent-config) + #[arg(long, hide = true)] + agent_config: Option, }, /// Manage the wt-agent-hooks bridge for supported CLI agents @@ -440,7 +486,29 @@ async fn main() -> Result<()> { rust_i18n::set_locale(&normalize_locale(&locale)); } - let cli = Cli::parse(); + let mut cli = Cli::parse(); + + // If --agent-config is present, overlay its fields onto the individual + // CLI args. This is the secure path: the host serializes all agent + // config as a single JSON blob, eliminating per-field commandline + // escaping. Invalid JSON is a hard error (don't silently fall back). + if let Some(AgentConfig { agent, agent_id, delegate_agent, delegate_model, acp_model }) = parse_agent_config(cli.agent_config.as_deref())? { + if let Some(v) = agent { + cli.agent = v; + } + if agent_id.is_some() { + cli.agent_id = agent_id; + } + if delegate_agent.is_some() { + cli.delegate_agent = delegate_agent; + } + if delegate_model.is_some() { + cli.delegate_model = delegate_model; + } + if acp_model.is_some() { + cli.acp_model = acp_model; + } + } // Legacy flags first (backward compat) if cli.test_pipe { @@ -649,11 +717,24 @@ async fn main() -> Result<()> { // ── Delegate prompt to new tab agent ── Some(Command::Delegate { prompt, - agent, - delegate_agent, - delegate_model, + mut agent, + mut delegate_agent, + mut delegate_model, cwd, + agent_config, }) => { + // Apply JSON config overlay (same as top-level path). + if let Some(AgentConfig { agent: cfg_agent, delegate_agent: cfg_da, delegate_model: cfg_dm, .. }) = parse_agent_config(agent_config.as_deref())? { + if let Some(a) = cfg_agent { + agent = a; + } + if cfg_da.is_some() { + delegate_agent = cfg_da; + } + if cfg_dm.is_some() { + delegate_model = cfg_dm; + } + } run_delegate(&prompt, &agent, delegate_agent.as_deref(), delegate_model.as_deref(), cwd.as_deref()).await } diff --git a/tools/wta/src/shell/shell_manager.rs b/tools/wta/src/shell/shell_manager.rs index a727d4a28..f2cb2575d 100644 --- a/tools/wta/src/shell/shell_manager.rs +++ b/tools/wta/src/shell/shell_manager.rs @@ -6,6 +6,7 @@ use tokio::process::Child; use tokio::process::Command; use super::wt_channel::WtChannel; +use crate::cmdline::build_wt_commandline; /// Configuration for creating a new terminal. pub struct TerminalConfig { @@ -115,19 +116,11 @@ impl ShellManager { let id = self.next_id(); let wt = self.wt()?; - // Build the commandline string: "command arg1 arg2 ..." - let mut cmdline = config.command.clone(); - for arg in &config.args { - cmdline.push(' '); - // Quote args containing spaces - if arg.contains(' ') { - cmdline.push('"'); - cmdline.push_str(arg); - cmdline.push('"'); - } else { - cmdline.push_str(arg); - } - } + // Build the commandline string for WT pane creation. Returns an + // error if the agent-supplied command is unrepresentable (e.g. + // contains a literal `"`); the caller catches this and falls back + // to create_terminal_local. + let cmdline = build_wt_commandline(&config.command, &config.args)?; // Create a new tab in WT with the command let mut params = serde_json::Map::new();