mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 17:57:54 -10:00
* fix(tools): fix cmd.exe /S/C corrupting commands with embedded quotes Any shell command whose own text contains embedded double quotes - python -c "print(15 / 3)", git commit -m "message", node -e "..." - was silently corrupted on Windows before reaching the target program. Reported live: a model ran python -c "print(15 / 3)" and got back a SyntaxError showing the code Python received as literally "print(15, truncated right before the /. Root cause: bash.go passes the full command text as ONE argv element to cmd.exe /d /s /c, and Go's os/exec escapes it correctly for a standard CommandLineToArgvW-style parser (wrapping it in quotes, backslash-escaping the embedded ones). But cmd.exe's own /C remainder parsing does not follow those rules once /S is present: per its documented behavior, it strips the FIRST and LAST literal quote character in the whole remainder - not a matched pair - before handing the (now corrupted) text along to be re-tokenized. The escaping was correct for one layer of parsing; the command line actually goes through two. Fix: on Windows, write commandText to a temp .cmd script file and point /c at the file's path instead of the raw command text. This sidesteps cmd.exe's /C-remainder heuristic entirely, since the remainder is then just a plain path with no quotes to mis-strip. @echo off is prepended since cmd.exe treats a /c target ending in .cmd as a batch file and echoes each line it runs by default, which would otherwise pollute captured stdout (caught by the existing bash tool test suite before this commit, not left in). buildBashCommand now returns a cleanup func (in addition to plan.Cleanup) for the temp file. Both callers updated: bash.go's one-shot run defers it immediately; exec_command.go's persistent session composes it with startExecProcess's own cleanup and runs it after the session's process actually exits, not before. This fix lives upstream of the sandbox wrapping, in how the Name/Args spec is constructed before either the direct or sandboxed code path consumes it, so both benefit without separate sandbox-specific changes - cmd.exe's own command-line parsing does not depend on what token launched it. New TestBashToolPreservesEmbeddedQuotesOnWindows pins the exact failure shape via a self-dispatch test helper (a quoted argument containing a space and a slash, matching the original bug report's truncation point), with no dependency on python being installed. Full internal/tools suite green, excluding 3 failures pre-existing and identical on unmodified main in my environment (unrelated path-assumption issues under a deeply nested working directory). Co-Authored-By: Claude Fable 5 <[email protected]> * fix(tools): pass the shell command line to cmd.exe raw instead of via a temp script Review on the original fix found real problems with running commandText from a temporary .cmd file: it silently changed cmd.exe command-line syntax into batch-file syntax (for example, for %i loops need %%i only inside an actual .bat/.cmd file), /s was still corrupting the script path itself if it contained spaces, and the script leaked on sandbox planning errors. The actual root cause was narrower than the temp-file approach assumed. cmd.exe's own /C remainder parsing is not CommandLineToArgvW-compatible: it strips the first and last literal quote from the remainder whenever it starts with a quote, without undoing backslash-escaping. Passing commandText as a normal exec.Cmd argument makes Go wrap it in exactly that shape (quoted because it has spaces, inner quotes escaped as \") since it contains embedded quotes, which is what the stripping then corrupts. Fixes it at the source instead: commandText now reaches cmd.exe completely unescaped (via SysProcAttr.CmdLine for the unwrapped path, and the same treatment inside the sandboxed runner's own process creation for the wrapped path), exactly as if it had been typed at an interactive prompt. No outer quotes means the stripping heuristic never triggers for a command like `python -c "print(15 / 3)"`, and a command that legitimately starts with a quoted executable path is handled by cmd.exe's own careful quote-preserving special case for that shape. Also drops /s, which forced the more aggressive of cmd.exe's two stripping behaviors. No temp file, so nothing to leak on error, and no batch-file reinterpretation of the command text. internal/sandbox/windows_runner.go now owns the shared shell-argv shape (WindowsShellArgs/WindowsShellCommandLine) so internal/tools/bash.go and the sandboxed runner's windowsCommandLine build the exact same raw command line instead of duplicating the flag literals in two places. * chore: retrigger CI The Windows smoke job failed on TestLoadOrCreateSecretConcurrentConverges in internal/oauth (a lock-file access-denied flake), unrelated to this change. internal/tools and internal/sandbox, the packages this PR touches, both passed. Retriggering to confirm. --------- Co-authored-by: Claude Fable 5 <[email protected]>
143 lines
4.0 KiB
Go
143 lines
4.0 KiB
Go
//go:build windows
|
|
|
|
package sandbox
|
|
|
|
import (
|
|
"fmt"
|
|
"os"
|
|
"sort"
|
|
"strings"
|
|
"syscall"
|
|
"unsafe"
|
|
|
|
"golang.org/x/sys/windows"
|
|
)
|
|
|
|
func runWindowsCommandAsUser(token windows.Token, config WindowsSandboxCommandConfig) (int, error) {
|
|
if len(config.Command) == 0 {
|
|
return 1, errorsNewWindowsProcess("missing command")
|
|
}
|
|
commandLine, err := windowsCommandLine(config.Command)
|
|
if err != nil {
|
|
return 1, err
|
|
}
|
|
commandLinePtr, err := windows.UTF16PtrFromString(commandLine)
|
|
if err != nil {
|
|
return 1, fmt.Errorf("encode command line: %w", err)
|
|
}
|
|
cwdPtr, err := windows.UTF16PtrFromString(config.CommandCWD)
|
|
if err != nil {
|
|
return 1, fmt.Errorf("encode command cwd: %w", err)
|
|
}
|
|
envBlock, err := windowsEnvironmentBlock(config.Env)
|
|
if err != nil {
|
|
return 1, err
|
|
}
|
|
desktopPtr, err := windows.UTF16PtrFromString(`winsta0\default`)
|
|
if err != nil {
|
|
return 1, fmt.Errorf("encode desktop: %w", err)
|
|
}
|
|
|
|
stdin := windows.Handle(os.Stdin.Fd())
|
|
stdout := windows.Handle(os.Stdout.Fd())
|
|
stderr := windows.Handle(os.Stderr.Fd())
|
|
for _, handle := range []windows.Handle{stdin, stdout, stderr} {
|
|
if err := windows.SetHandleInformation(handle, windows.HANDLE_FLAG_INHERIT, windows.HANDLE_FLAG_INHERIT); err != nil {
|
|
return 1, fmt.Errorf("make stdio handle inheritable: %w", err)
|
|
}
|
|
}
|
|
|
|
var startup windows.StartupInfo
|
|
startup.Cb = uint32(unsafe.Sizeof(startup))
|
|
startup.Desktop = desktopPtr
|
|
startup.Flags = windows.STARTF_USESTDHANDLES
|
|
startup.StdInput = stdin
|
|
startup.StdOutput = stdout
|
|
startup.StdErr = stderr
|
|
var process windows.ProcessInformation
|
|
envPtr := &envBlock[0]
|
|
if err := windows.CreateProcessAsUser(
|
|
token,
|
|
nil,
|
|
commandLinePtr,
|
|
nil,
|
|
nil,
|
|
true,
|
|
windows.CREATE_UNICODE_ENVIRONMENT,
|
|
envPtr,
|
|
cwdPtr,
|
|
&startup,
|
|
&process,
|
|
); err != nil {
|
|
return 1, fmt.Errorf("CreateProcessAsUser: %w", err)
|
|
}
|
|
defer windows.CloseHandle(process.Process)
|
|
defer windows.CloseHandle(process.Thread)
|
|
if _, err := windows.WaitForSingleObject(process.Process, windows.INFINITE); err != nil {
|
|
return 1, fmt.Errorf("wait for sandboxed process: %w", err)
|
|
}
|
|
var exitCode uint32
|
|
if err := windows.GetExitCodeProcess(process.Process, &exitCode); err != nil {
|
|
return 1, fmt.Errorf("read sandboxed process exit code: %w", err)
|
|
}
|
|
return int(exitCode), nil
|
|
}
|
|
|
|
func windowsCommandLine(args []string) (string, error) {
|
|
if len(args) == 0 {
|
|
return "", errorsNewWindowsProcess("missing command")
|
|
}
|
|
for _, arg := range args {
|
|
if strings.ContainsRune(arg, 0) {
|
|
return "", errorsNewWindowsProcess("command argument contains NUL")
|
|
}
|
|
}
|
|
// Zero's cmd.exe shell invocation needs its final element (the shell
|
|
// command text) reaching cmd.exe raw, not escaped like a normal argv
|
|
// element: see WindowsShellCommandLine for why. Every other command this
|
|
// runner might launch is escaped normally below.
|
|
if commandLine, ok := windowsShellCommandLineFromArgs(args); ok {
|
|
return commandLine, nil
|
|
}
|
|
out := make([]string, 0, len(args))
|
|
for _, arg := range args {
|
|
out = append(out, syscall.EscapeArg(arg))
|
|
}
|
|
return strings.Join(out, " "), nil
|
|
}
|
|
|
|
func windowsEnvironmentBlock(env map[string]string) ([]uint16, error) {
|
|
keys := make([]string, 0, len(env))
|
|
for key := range env {
|
|
if strings.TrimSpace(key) == "" {
|
|
continue
|
|
}
|
|
if strings.ContainsRune(key, 0) || strings.ContainsRune(env[key], 0) {
|
|
return nil, errorsNewWindowsProcess("environment contains NUL")
|
|
}
|
|
keys = append(keys, key)
|
|
}
|
|
sort.Slice(keys, func(i, j int) bool {
|
|
left := strings.ToUpper(keys[i])
|
|
right := strings.ToUpper(keys[j])
|
|
if left == right {
|
|
return keys[i] < keys[j]
|
|
}
|
|
return left < right
|
|
})
|
|
block := make([]uint16, 0)
|
|
for _, key := range keys {
|
|
entry, err := syscall.UTF16FromString(key + "=" + env[key])
|
|
if err != nil {
|
|
return nil, fmt.Errorf("encode environment variable %s: %w", key, err)
|
|
}
|
|
block = append(block, entry...)
|
|
}
|
|
block = append(block, 0)
|
|
return block, nil
|
|
}
|
|
|
|
func errorsNewWindowsProcess(message string) error {
|
|
return fmt.Errorf("windows sandbox process: %s", message)
|
|
}
|