Files
zero/internal/sandbox/windows_process_windows.go
euxaristiaandClaude Fable 5 190241bd59 fix(tools): fix cmd.exe /S/C corrupting commands with embedded quotes (#465)
* 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]>
2026-07-05 11:36:00 +08:00

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)
}