feat(sandbox): Windows sandbox principals (foundation for #662, does not close it)

internal/cli/sandbox.go

@@ -22,7 +22,7 @@ const permissionProfileScopeNote = "permissionProfile is derived from this proce
 
 func runSandbox(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int {
 	if len(args) == 0 {
-		return writeExecUsageError(stderr, "sandbox subcommand required. Use `zero sandbox policy` or `zero sandbox grants list`.")
+		return writeExecUsageError(stderr, "sandbox subcommand required. Use `zero sandbox policy`, `zero sandbox exec`, or `zero sandbox grants list`.")
 	}
 	switch args[0] {
 	case "-h", "--help", "help":
@@ -36,6 +36,8 @@ func runSandbox(args []string, stdout io.Writer, stderr io.Writer, deps appDeps)
 		return runSandboxSetup(args[1:], stdout, stderr, deps)
 	case "check":
 		return runSandboxCheck(args[1:], stdout, stderr, deps)
+	case "exec":
+		return runSandboxExec(args[1:], stdout, stderr, deps)
 	case "grants":
 		return runSandboxGrants(args[1:], stdout, stderr, deps)
 	default:
@@ -174,10 +176,17 @@ func runSandboxSetup(args []string, stdout io.Writer, stderr io.Writer, deps app
 	if !setupHelper.Available() {
 		return writeAppError(stderr, "Windows sandbox setup helper is not available", exitProvider)
 	}
+	// Resolved here, in the shell the user typed `zero sandbox setup` into, and
+	// carried in the args. The helper may be launched elevated, and an elevated
+	// process does not inherit this shell's environment. Stated explicitly rather
+	// than left nil (which resolves the same way) because this is the call site
+	// the opt-in is about.
+	principalOptIn := zeroSandbox.WindowsSandboxPrincipalOptIn(nil)
 	setupArgs, err := zeroSandbox.BuildWindowsSandboxSetupArgs(zeroSandbox.WindowsSandboxSetupArgsOptions{
 		CommandCWD:        workspaceRoot,
 		WorkspaceRoots:    []string{workspaceRoot},
 		PermissionProfile: profile,
+		PrincipalOptIn:    &principalOptIn,
 	})
 	if err != nil {
 		return writeAppError(stderr, err.Error(), exitCrash)
@@ -646,6 +655,7 @@ Commands:
   policy      Inspect active sandbox policy and platform backend
   setup       Run native platform sandbox setup
   check       Evaluate the sandbox decision for a hypothetical tool action
+  exec        Run one command through the real sandbox
   grants      Manage persistent sandbox grants
 
 `)

internal/cli/sandbox_exec.go

@@ -0,0 +1,272 @@
+package cli
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"io"
+	"os"
+	"os/exec"
+	"strings"
+	"time"
+
+	"github.com/Gitlawb/zero/internal/config"
+	"github.com/Gitlawb/zero/internal/execution"
+	zeroSandbox "github.com/Gitlawb/zero/internal/sandbox"
+)
+
+// runSandboxExec runs ONE command through the real sandbox and exits with its
+// status.
+//
+// This exists because until now the sandbox could only be exercised through a
+// full agent turn with a model in the loop. `zero sandbox policy` reports what
+// the posture would be and `zero sandbox check` evaluates a hypothetical
+// decision, but nothing actually ran a command and let you look at what
+// happened on disk afterwards. The practical result is that enforcement is
+// covered almost entirely by tests asserting the shape of an ACL plan, and
+// almost not at all by tests asserting a write was refused.
+//
+// A plan can be perfectly correct and never reach the filesystem. That is not
+// hypothetical here: the .git rename guard was emitted correctly by the planner
+// and silently skipped by the applier, and four tests covering the plan all
+// passed while the ACE was absent from disk. Something that runs the real
+// binary and then stats the file is the only thing that catches that class.
+//
+// Deliberately NOT a debug curiosity: it takes the same path a shell tool
+// takes, through SandboxManager.BuildCommandPlan, so what it proves is what
+// users get. It prints the resolved backend and enforcement level to stderr
+// before running, so a harness can assert the sandbox was actually engaged
+// rather than quietly downgraded.
+func runSandboxExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int {
+	command, err := parseSandboxExecArgs(args)
+	if err != nil {
+		if errors.Is(err, errSandboxExecHelp) {
+			if writeErr := writeSandboxExecHelp(stdout); writeErr != nil {
+				return exitCrash
+			}
+			return exitSuccess
+		}
+		return writeExecUsageError(stderr, err.Error())
+	}
+
+	workspaceRoot, err := resolveWorkspaceRoot("", deps)
+	if err != nil {
+		return writeExecUsageError(stderr, err.Error())
+	}
+	resolved, err := deps.resolveConfig(workspaceRoot, config.Overrides{})
+	if err != nil {
+		return writeAppError(stderr, err.Error(), exitProvider)
+	}
+	policy := applyConfiguredSandboxPolicy(zeroSandbox.DefaultPolicy(), resolved.Sandbox)
+
+	scope, err := zeroSandbox.NewScope(workspaceRoot, resolved.Sandbox.AdditionalWriteRoots)
+	if err != nil {
+		return writeAppError(stderr, fmt.Sprintf("resolve sandbox write roots: %v", err), exitCrash)
+	}
+
+	// Through the ENGINE, not a SandboxManager built here.
+	//
+	// The engine is what a real tool call goes through, and it does more than
+	// hand the manager a request: it resolves the permission profile, calls
+	// prepareSandboxRuntime, and folds the runtime state into that profile
+	// before planning. Building a manager directly skipped all of it, so the
+	// command ran without the runtime write root, and cache or temp writes could
+	// pass or fail differently from the sandboxed command this exists to imitate.
+	// A harness that exercises the wrong path is worse than no harness, because
+	// its result still reads as evidence.
+	engine := zeroSandbox.NewEngine(zeroSandbox.EngineOptions{
+		WorkspaceRoot: workspaceRoot,
+		Policy:        policy,
+		Scope:         scope,
+		Backend:       deps.selectSandboxBackend(zeroSandbox.BackendOptions{}),
+		// The same scrub list a real tool call gets. Without it the engine keeps an
+		// empty set, and only the hardcoded names plus the provider catalog's own
+		// AuthEnvVars are removed from the child's environment: a key named by
+		// `apiKeyEnv` in the user's config would be scrubbed for every sandboxed
+		// tool call and handed to this one. Reproducing the production environment
+		// is the entire point of the command, and a credential is the last part of
+		// it that may differ.
+		SensitiveEnvKeys: providerSensitiveEnvKeys(resolved),
+	})
+	plan, err := engine.BuildCommandPlan(zeroSandbox.CommandSpec{
+		Name: command[0],
+		Args: command[1:],
+		Dir:  workspaceRoot,
+		Env:  os.Environ(),
+	})
+	if err != nil {
+		return writeAppError(stderr, fmt.Sprintf("build sandbox command plan: %v", err), exitCrash)
+	}
+	// The plan owns resources beyond its construction: on Linux it allocates a
+	// policy-report file and registers the removal here, so without this every
+	// invocation leaves a /tmp/zero-sandbox-report-* behind.
+	defer plan.Cleanup()
+
+	// Printed before the command runs and on stderr, so it survives a command
+	// that writes to stdout and stays greppable by a test harness. A downgrade
+	// is reported loudly for the same reason: a smoke test that passes because
+	// the sandbox quietly stood down is worse than no smoke test.
+	fmt.Fprintf(stderr, "sandbox: backend=%s enforcement=%s wrapped=%t workspace=%s\n",
+		plan.Backend.Name, plan.EnforcementLevel, plan.Wrapped, plan.WorkspaceRoot)
+	if strings.TrimSpace(plan.DowngradeReason) != "" {
+		fmt.Fprintf(stderr, "sandbox: DOWNGRADED: %s\n", plan.DowngradeReason)
+	}
+
+	// The CLI's shared shutdown context, so Ctrl+C and a directed SIGTERM both
+	// arrive here rather than only at whatever the terminal happens to signal.
+	runCtx, stopSignals := signalContext()
+	defer stopSignals()
+	return runSandboxPlannedCommand(runCtx, plan, stdout, stderr)
+}
+
+const (
+	// sandboxExecShutdownGrace is how long a cancelled child has to exit on its
+	// own after the graceful request, before it is killed outright.
+	sandboxExecShutdownGrace = 5 * time.Second
+	// sandboxExecShutdownPoll is how often that wait re-checks, matching the
+	// background package's termination policy.
+	sandboxExecShutdownPoll = 50 * time.Millisecond
+)
+
+func runSandboxPlannedCommand(ctx context.Context, plan zeroSandbox.CommandPlan, stdout io.Writer, stderr io.Writer) int {
+	// CANCELLING THE WRAPPER HAS TO REACH THE COMMAND.
+	//
+	// This started the backend wrapper with a bare exec.Command().Run(): no
+	// context, no forwarding, no shutdown path. A terminal masks it, because
+	// terminals signal the whole foreground process group, but a supervisor or
+	// task runner that sends SIGTERM to the zero sandbox exec PID killed only
+	// Zero. The wrapper and everything under it kept running, doing filesystem
+	// and network work after the caller considered the task cancelled, and the
+	// deferred plan cleanup never ran because the process was gone.
+	//
+	// DELIBERATELY NOT execution.ConfigureProcessGroup. Putting the child in its
+	// own process group closes this hole and opens a worse one: it severs every
+	// kernel-delivered group signal, so a supervisor escalating to a group kill,
+	// and a terminal delivering Ctrl+C to its foreground group, would stop
+	// reaching the child. That trades a path that works today for one that does
+	// not. Keeping the child in Zero's group leaves group delivery unchanged,
+	// while the directed-signal case, which reached nothing at all before, now
+	// reaches the child.
+	process := exec.CommandContext(ctx, plan.Name, plan.Args...)
+	// TWO PHASES, AND THE FIRST ONE HAS TO EXIST.
+	//
+	// The comments here used to describe a graceful signal followed by escalation
+	// while the code did neither: Cancel called KillProcessTree, which is SIGKILL
+	// on Unix and taskkill /T /F on Windows, and WaitDelay only starts counting
+	// AFTER Cancel returns, so it could never postpone a kill that had already
+	// happened. A directed SIGTERM reached the child as 137 rather than 143, with
+	// no chance to flush output, drop a lock file, or run a shutdown handler.
+	//
+	// TerminateProcessTree is the two-phase policy the background package already
+	// uses: SIGTERM, poll for the grace, then SIGKILL what is still alive. On
+	// Windows it is the single-phase kill, which is correct rather than lazy,
+	// because there is no signal to send a child that owns no console.
+	process.Cancel = func() error {
+		return execution.TerminateProcessTree(process.Process.Pid, sandboxExecShutdownGrace, sandboxExecShutdownPoll)
+	}
+	// A backstop only. Cancel above already waits out the grace and escalates, so
+	// this bounds the case where the tree is gone but Wait is still blocked on an
+	// inherited pipe a grandchild holds open.
+	process.WaitDelay = sandboxExecShutdownGrace + sandboxExecShutdownPoll
+	process.Dir = plan.Dir
+	if process.Dir == "" {
+		process.Dir = plan.WorkspaceRoot
+	}
+	// SPECIFIED-EMPTY IS NOT UNSPECIFIED.
+	//
+	// exec.Cmd treats a nil Env as "inherit this process's entire environment",
+	// which is a different statement from "run with no variables". The plan owns
+	// its environment: directCommandEnv and scrubSensitiveEnv return a slice they
+	// built, and that slice is non-nil with length zero when every entry was
+	// sensitive. Testing length collapsed those two states and turned the strictest
+	// possible answer into the loosest one.
+	//
+	// Not reachable through `zero sandbox exec` today, because the child
+	// environment is os.Environ() and an environment holding only sensitive keys
+	// has no %AppData%, so config resolution fails before the planner runs. Fixed
+	// anyway: the guard is one assignment, and the next caller that hands the plan
+	// a deliberately narrow environment would inherit everything instead, silently.
+	if plan.Env != nil {
+		process.Env = plan.Env
+	}
+	process.Stdin = os.Stdin
+	process.Stdout = stdout
+	process.Stderr = stderr
+
+	if err := process.Run(); err != nil {
+		var exitErr *exec.ExitError
+		if errors.As(err, &exitErr) {
+			// The command's own status, not ours. A harness asserting "the write
+			// was refused" needs the refusal's exit code, not a wrapper's.
+			// A SIGNALED CHILD HAS NO EXIT CODE TO REPORT. ExitCode() answers -1
+			// there, and os.Exit truncates that to 255, so a child that took SIGTERM
+			// became indistinguishable from one that chose to exit 255. Fold the
+			// signal into the conventional 128+n a shell would report, which needs
+			// the ProcessState rather than the integer.
+			if status, signaled := signaledExitStatus(exitErr.ProcessState); signaled {
+				return status
+			}
+			return exitErr.ExitCode()
+		}
+		fmt.Fprintf(stderr, "sandbox exec: %v\n", err)
+		return exitCrash
+	}
+	return exitSuccess
+}
+
+var errSandboxExecHelp = errors.New("help requested")
+
+// parseSandboxExecArgs takes everything after `--` as the command, so the
+// command's own flags are never mistaken for ours.
+func parseSandboxExecArgs(args []string) ([]string, error) {
+	if len(args) == 0 {
+		return nil, errors.New("usage: zero sandbox exec -- <command> [args...]")
+	}
+	// Only the FIRST token is ours to interpret. Everything from the second
+	// onwards belongs to the child, help flags included.
+	//
+	// Written as a straight-line decision on args[0] rather than a loop, because
+	// a loop here is a lie. An earlier version scanned for `--` "up front" and
+	// said so in its comment, but every branch of its body returned, so it only
+	// ever examined index 0 and the promised scan did not exist. The rule below
+	// is what that code actually implemented, and it is the rule we want: it is
+	// the wrapper's own prefix that can ask for the wrapper's help, and the
+	// prefix is at most one token long.
+	switch args[0] {
+	case "--":
+		// Everything after the separator is the command, verbatim, including
+		// `--help`. That is the documented contract: `zero sandbox exec -- cmd
+		// --help` must run cmd's help, not ours.
+		command := args[1:]
+		if len(command) == 0 {
+			return nil, errors.New("usage: zero sandbox exec -- <command> [args...]")
+		}
+		return command, nil
+	case "-h", "--help", "help":
+		return nil, errSandboxExecHelp
+	}
+	// The separator-less form, tolerated for interactive use: the first token is
+	// the command, so nothing after it is ours to read. Without this,
+	// `zero sandbox exec mycmd --help` printed OUR help and never ran mycmd,
+	// which is the same contract break as reading past `--`. The help text still
+	// shows the separator, because anything with a leading dash needs it.
+	return args, nil
+}
+
+func writeSandboxExecHelp(w io.Writer) error {
+	_, err := fmt.Fprint(w, `Usage:
+  zero sandbox exec -- <command> [args...]
+
+Runs one command through the real sandbox and exits with its status.
+
+Everything after the -- separator is the command, so its own flags are not
+parsed as Zero's. The resolved backend and enforcement level are written to
+stderr before the command runs, and a downgrade is reported there explicitly.
+
+Examples:
+  zero sandbox exec -- cmd /c echo hello
+  zero sandbox exec -- powershell -Command "Set-Content out.txt x"
+
+`)
+	return err
+}

internal/cli/sandbox_exec_cancel_test.go

@@ -0,0 +1,78 @@
+package cli
+
+import (
+	"context"
+	"io"
+	"os"
+	"runtime"
+	"testing"
+	"time"
+
+	zeroSandbox "github.com/Gitlawb/zero/internal/sandbox"
+)
+
+// longLivedPlan returns a plan whose child outlives the test unless something
+// terminates it.
+func longLivedPlan(t *testing.T) zeroSandbox.CommandPlan {
+	t.Helper()
+	plan := zeroSandbox.CommandPlan{Dir: t.TempDir()}
+	if runtime.GOOS == "windows" {
+		plan.Name = "cmd.exe"
+		plan.Args = []string{"/c", "ping -n 120 127.0.0.1 >NUL"}
+		return plan
+	}
+	plan.Name = "/bin/sh"
+	plan.Args = []string{"-c", "sleep 120"}
+	return plan
+}
+
+// CANCELLING THE WRAPPER HAS TO REACH THE COMMAND.
+//
+// The sandboxed command was started with a bare exec.Command().Run(): no
+// context, no forwarding, no shutdown path. A terminal masks that, because it
+// signals the whole foreground process group, but a supervisor or task runner
+// that sends SIGTERM to the wrapper's PID killed only Zero. The command kept
+// running, doing filesystem and network work after the caller considered the
+// task cancelled, and the deferred plan cleanup never ran.
+//
+// Driven with a real long-lived child and a real cancellation, asserting that
+// the call actually returns rather than that a field is set.
+func TestCancellingTheWrapperTerminatesTheSandboxedCommand(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+
+	done := make(chan int, 1)
+	go func() {
+		done <- runSandboxPlannedCommand(ctx, longLivedPlan(t), io.Discard, io.Discard)
+	}()
+
+	// Let the child actually start, or cancelling proves nothing.
+	time.Sleep(300 * time.Millisecond)
+	select {
+	case <-done:
+		t.Fatal("SETUP INVALID: the child exited on its own, so cancellation was not exercised")
+	default:
+	}
+
+	cancel()
+	select {
+	case <-done:
+	case <-time.After(sandboxExecShutdownGrace + 10*time.Second):
+		t.Fatal("cancelling the wrapper did not terminate the sandboxed command; it would keep running after the caller gave up")
+	}
+}
+
+// And an uncancelled command still runs to completion and reports its own
+// status, or the fix above would be "kill everything immediately".
+func TestAnUncancelledSandboxedCommandStillReportsItsStatus(t *testing.T) {
+	plan := zeroSandbox.CommandPlan{Dir: t.TempDir()}
+	if runtime.GOOS == "windows" {
+		plan.Name = "cmd.exe"
+		plan.Args = []string{"/c", "exit 3"}
+	} else {
+		plan.Name = "/bin/sh"
+		plan.Args = []string{"-c", "exit 3"}
+	}
+	if code := runSandboxPlannedCommand(context.Background(), plan, io.Discard, os.Stderr); code != 3 {
+		t.Fatalf("exit code = %d, want the child's own 3", code)
+	}
+}