fix(sandbox): keep Windows restricted-token SIDs narrow (no Users broaden)

internal/sandbox/manager_test.go

@@ -230,6 +230,87 @@ func TestSandboxManagerBuildsCommandPlanThroughWindowsRunner(t *testing.T) {
 	}
 }
 
+// TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers is the
+// regression for PR #640: DenyRead cannot be launched or provisioned through
+// either the elevated restricted-token path or the unelevated auto fallback.
+// Both build the same fully restricted narrow-SID token.
+func TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers(t *testing.T) {
+	backend := Backend{Name: BackendWindowsRestrictedToken, Available: true, Executable: `C:\zero\zero-windows-command-runner.exe`, Platform: "windows"}
+	manager := NewSandboxManager(SandboxManagerOptions{GOOS: "windows", Backend: backend})
+	policy := DefaultPolicy()
+	profile := PermissionProfile{
+		FileSystem: FileSystemPolicy{
+			Kind:       FileSystemRestricted,
+			WriteRoots: []WritableRoot{{Root: `C:\workspace`}},
+			DenyRead:   []string{`C:\workspace\secret`},
+		},
+		Network: NetworkPolicy{Mode: NetworkDeny},
+	}
+	cmd := CommandSpec{Name: "cmd.exe", Args: []string{"/c", "dir"}, Dir: `C:\workspace`}
+
+	t.Run("elevated_restricted_token", func(t *testing.T) {
+		restore := windowsSandboxInitialized
+		t.Cleanup(func() { windowsSandboxInitialized = restore })
+		windowsSandboxInitialized = func() bool { return true }
+
+		_, err := manager.BuildCommandPlan(SandboxManagerRequest{
+			WorkspaceRoot:     `C:\workspace`,
+			Command:           cmd,
+			Policy:            policy,
+			Profile:           profile,
+			Preference:        SandboxPreferenceAuto,
+			ValidateExecution: true,
+		})
+		if err == nil {
+			t.Fatal("expected BuildCommandPlan error for elevated restricted-token DenyRead")
+		}
+		msg := err.Error()
+		for _, want := range []string{"DenyRead", "not supported", "restricted-token"} {
+			if !strings.Contains(msg, want) {
+				t.Fatalf("error %q missing %q", msg, want)
+			}
+		}
+	})
+
+	t.Run("unelevated_auto_fallback", func(t *testing.T) {
+		restore := windowsSandboxInitialized
+		t.Cleanup(func() { windowsSandboxInitialized = restore })
+		windowsSandboxInitialized = func() bool { return false }
+
+		req, err := manager.BuildExecutionRequest(SandboxManagerRequest{
+			WorkspaceRoot:     `C:\workspace`,
+			Command:           cmd,
+			Policy:            policy,
+			Profile:           profile,
+			Preference:        SandboxPreferenceAuto,
+			ValidateExecution: true,
+		})
+		if err != nil {
+			t.Fatalf("BuildExecutionRequest: %v", err)
+		}
+		if req.EnforcementLevel != EnforcementUnelevated {
+			t.Fatalf("EnforcementLevel = %v, want unelevated auto fallback before DenyRead rejection", req.EnforcementLevel)
+		}
+		_, err = manager.BuildCommandPlan(SandboxManagerRequest{
+			WorkspaceRoot:     `C:\workspace`,
+			Command:           cmd,
+			Policy:            policy,
+			Profile:           profile,
+			Preference:        SandboxPreferenceAuto,
+			ValidateExecution: true,
+		})
+		if err == nil {
+			t.Fatal("expected BuildCommandPlan error for unelevated DenyRead")
+		}
+		if !strings.Contains(err.Error(), "DenyRead") || !strings.Contains(err.Error(), "not supported") {
+			t.Fatalf("unelevated DenyRead error = %v", err)
+		}
+		if strings.Contains(err.Error(), "Use `--sandbox forbid`, the unelevated") {
+			t.Fatalf("error still recommends unelevated as a workaround: %v", err)
+		}
+	})
+}
+
 func TestSandboxManagerDegradesUnavailableCommandPlan(t *testing.T) {
 	policy := DefaultPolicy()
 	backend := Backend{Name: BackendUnavailable, Platform: "windows", Fallback: true, Message: "native sandbox unavailable"}

internal/sandbox/profile.go

@@ -150,10 +150,11 @@
 // carry credentials and are now discoverable through the preserved caller
 // environment. Two deliberate limits:
 //
-//   - Windows is skipped: a non-empty profile DenyRead switches the Windows
-//     runner onto the capability-SID/ACL deny path and away from the
-//     WRITE_RESTRICTED token, which the unelevated tier depends on. Revisit
-//     once the Windows deny-read model is settled.
+//   - Windows is skipped: a non-empty profile DenyRead is unsupported on both
+//     restricted-token runner levels under the narrow SID set (PR #640). The
+//     fully restricted token cannot load ordinary system binaries without
+//     Users/AuthUsers, and adding those groups reopens write grants outside
+//     WriteRoots. Revisit once access-time confinement exists.
 //   - A candidate nested under a user-configured AllowRead entry is dropped,
 //     so `allowRead: ["~/.aws"]` remains an explicit opt-out.
 //
@@ -182,7 +183,7 @@
 
 // credentialDenyReadPathsIn is the pure core of credentialDenyReadPaths,
 // separated so tests can exercise it against a synthetic home directory.
 func credentialDenyReadPathsIn(home string, googleCredentials string, allowRead []string) []string {
 	return credentialDenyReadPathsForEnvironment(credentialPathEnvironment{
 		Home:              home,
 		GoogleCredentials: googleCredentials,

internal/sandbox/runner_windows_integration_test.go

@@ -72,6 +72,28 @@ func TestWindowsRestrictedTokenRealSandboxSmoke(t *testing.T) {
 		t.Fatalf("sandboxed write marker = %q, %v; want ok", bytes, err)
 	}
 
+	// SID broadening is disabled, so the restricted-SID list never includes
+	// Users/Authenticated Users. The write grant those groups hold on
+	// C:\Users\Public must not be reachable through the restricted-SID check.
+	// Pin that a write there fails: an independent shared-writable directory
+	// outside every workspace write root.
+	publicDir := os.Getenv("PUBLIC")
+	if publicDir == "" {
+		t.Log("PUBLIC is not set; skipping C:\\Users\\Public write-jail probe")
+	} else {
+		publicMarker := filepath.Join(publicDir, "zero-elevated-write-denied.txt")
+		_ = os.Remove(publicMarker)
+		runWindowsRealSmokeCommand(t, runnerExe, config, []string{
+			"cmd.exe", "/d", "/s", "/c", "echo leaked>" + publicMarker,
+		}, 1)
+		if _, err := os.Stat(publicMarker); err == nil {
+			_ = os.Remove(publicMarker)
+			t.Fatalf("Windows sandbox allowed a write to the shared C:\\Users\\Public directory")
+		} else if !os.IsNotExist(err) {
+			t.Fatalf("stat public marker: %v", err)
+		}
+	}
+
 	listener, err := net.Listen("tcp4", "127.0.0.1:0")
 	if err != nil {
 		t.Fatalf("listen loopback for Windows network smoke: %v", err)
@@ -189,10 +211,18 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) {
 		t.Fatalf("expected the unelevated setup marker to be recorded: %v", err)
 	}
 
-	// DenyRead check: reading from the privateDir must be blocked (exit code 1)
-	runWindowsRealSmokeCommand(t, runnerExe, config, []string{
+	// DenyRead is unsupported on both restricted-token tiers under the narrow
+	// SID set (PR #640): the runner must reject before launch rather than
+	// attempting a fully restricted token that cannot load system tools.
+	denyReadConfig := config
+	denyReadConfig.PermissionProfile.FileSystem.DenyRead = []string{privateDir}
+	runWindowsRealSmokeCommandExpectError(t, runnerExe, denyReadConfig, []string{
 		"cmd.exe", "/d", "/s", "/c", "type " + secretFile,
-	}, 1)
+	}, "DenyRead", "not supported")
+	// The secret must remain readable from the host; the sandbox never ran.
+	if data, err := os.ReadFile(secretFile); err != nil || string(data) != "super-secret" {
+		t.Fatalf("host secret file after rejected DenyRead launch: %q, %v", data, err)
+	}
 
 	outsideMarker := filepath.Join(outside, "unelevated-write-denied.txt")
 	runWindowsRealSmokeCommand(t, runnerExe, config, []string{
@@ -203,6 +233,24 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) {
 	} else if !os.IsNotExist(err) {
 		t.Fatalf("stat outside marker: %v", err)
 	}
+
+	// Verify write to C:\ProgramData is blocked
+	programData := os.Getenv("ProgramData")
+	if programData != "" {
+		programDataMarker := filepath.Join(programData, "zero-unelevated-write-denied.txt")
+		_ = os.Remove(programDataMarker)
+
+		runWindowsRealSmokeCommand(t, runnerExe, config, []string{
+			"cmd.exe", "/d", "/s", "/c", "echo leaked>" + programDataMarker,
+		}, 1)
+
+		if _, err := os.Stat(programDataMarker); err == nil {
+			_ = os.Remove(programDataMarker)
+			t.Fatalf("unelevated sandbox allowed a write to ProgramData shared directory")
+		} else if !os.IsNotExist(err) {
+			t.Fatalf("stat ProgramData marker: %v", err)
+		}
+	}
 }
 
 // TestWindowsRestrictedTokenNestedPipeCapture pins the fix in
@@ -386,6 +434,31 @@ func runWindowsRealSmokeCommand(t *testing.T, runnerExe string, base WindowsSand
 	}
 }
 
+// runWindowsRealSmokeCommandExpectError runs the command runner and requires a
+// non-zero exit whose combined output contains each want substring (used for
+// explicit unsupported-mode rejections rather than sandboxed command failures).
+func runWindowsRealSmokeCommandExpectError(t *testing.T, runnerExe string, base WindowsSandboxCommandArgsOptions, command []string, wantSubstr ...string) {
+	t.Helper()
+	base.Command = command
+	args, err := BuildWindowsSandboxCommandArgs(base)
+	if err != nil {
+		t.Fatalf("BuildWindowsSandboxCommandArgs: %v", err)
+	}
+	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
+	defer cancel()
+	cmd := exec.CommandContext(ctx, runnerExe, args...)
+	output, err := cmd.CombinedOutput()
+	if err == nil {
+		t.Fatalf("Windows sandbox command exit code = 0, want error containing %v\n%s", wantSubstr, output)
+	}
+	text := string(output)
+	for _, want := range wantSubstr {
+		if !strings.Contains(text, want) {
+			t.Fatalf("Windows sandbox command error missing %q: %v\n%s", want, err, output)
+		}
+	}
+}
+
 func powershellSingleQuote(value string) string {
 	out := "'"
 	for _, r := range value {

internal/sandbox/windows_acl.go

@@ -2,6 +2,7 @@
 
 import (
 	"errors"
+	"fmt"
 	"path/filepath"
 	"strings"
 )
@@ -12,13 +13,65 @@
 	WindowsACLAllowWrite WindowsACLAction = "allow-write"
 	WindowsACLDenyRead   WindowsACLAction = "deny-read"
 	WindowsACLDenyWrite  WindowsACLAction = "deny-write"
+	// WindowsACLRevokeCapability removes any existing ACE (allow or deny) for
+	// Capability at Path, without itself granting or denying anything (applied
+	// via SetEntriesInAclW's SET_ACCESS mode with a zero mask, not
+	// REVOKE_ACCESS — see windowsACLAccess for why). It reconciles stale
+	// shared/descendant DenyWrite ACEs an earlier setup run applied for the
+	// stable read-only capability SID (see BuildWindowsACLPlan) that a later
+	// run no longer intends: if a path previously covered by the
+	// shared-root/descendant DenyWrite mitigation is later configured as an
+	// allowed write root, that old deny is otherwise left on disk and wins
+	// over the new Allow under Windows' deny-before-allow evaluation — see
+	// jatmn's review. Clearing a SID with no matching ACE is a safe no-op, so
+	// this can always be emitted unconditionally alongside every write-root
+	// Allow entry.
+	WindowsACLRevokeCapability WindowsACLAction = "revoke-capability"
 )
 
 type WindowsACLEntry struct {
-	Action      WindowsACLAction `json:"action"`
-	Path        string           `json:"path"`
-	Capability  string           `json:"capability"`
-	Materialize bool             `json:"materialize,omitempty"`
+	Action     WindowsACLAction `json:"action"`
+	Path       string           `json:"path"`
+	Capability string           `json:"capability"`
+	// NoInherit forces the applied ACE to carry no inheritance flags, even
+	// when the target is a directory. Without it, applyWindowsACLPlan makes
+	// every directory ACE inheritable (SUB_CONTAINERS_AND_OBJECTS_INHERIT),
+	// and SetNamedSecurityInfo automatically propagates any inheritable ACE
+	// down onto the target's EXISTING descendants (not just new ones it
+	// creates going forward) — see the shared-deny-path entries below for
+	// why that is unsafe on broad system roots.
+	NoInherit   bool `json:"noInherit,omitempty"`
+	Materialize bool `json:"materialize,omitempty"`
+	// ScanDescendants marks a shared-root DenyWrite entry whose EXISTING
+	// writable descendants must ALSO be denied, one direct (non-inheriting)
+	// deny per writable descendant, at apply time. A non-inherited deny on the
+	// root object alone does not cover a pre-existing child that independently
+	// grants Users/Authenticated Users write, because a Windows access check
+	// for that child never consults a non-inherited ACE on its parent. This is
+	// deliberately NOT serialized (json:"-"): the concrete descendant set is
+	// live-filesystem state that differs between the setup process and a later
+	// command run, so folding it into the hashed plan would make
+	// ValidateWindowsSandboxSetupMarker non-deterministic. The flag itself is
+	// derived deterministically from the same inputs on both sides, and the
+	// descendant enumeration/denies happen as an apply-time side effect in
+	// applyWindowsACLPlan (windows-only), never in the cross-platform plan hash.
+	ScanDescendants bool `json:"-"`
+	// RevokeDescendants marks a write-root's WindowsACLRevokeCapability entry
+	// (see the constant below) as needing the same stale-deny cleanup applied
+	// recursively to the root's existing descendants, not just the root path
+	// itself. A tree scanned and denied by an earlier setup run (either
+	// because it WAS one of the four shared roots, or because it was a
+	// writable descendant applyWindowsSharedDescendantDenies found and denied
+	// elsewhere in the tree) can later be promoted to an allowed write root by
+	// the caller configuring some ANCESTOR of it as a WriteRoot. Revoking only
+	// at the exact configured root leaves any stale direct, non-inheriting
+	// deny on that ancestor's descendants in place, and a stale deny wins over
+	// the newly-added inheritable Allow under Windows' deny-before-allow
+	// evaluation — see jatmn's review. Like ScanDescendants, this is
+	// deliberately NOT serialized (json:"-"): the concrete stale-deny set is
+	// live-filesystem state, and the actual descendant walk/revoke happens as
+	// an apply-time side effect in applyWindowsACLPlan (windows-only).
+	RevokeDescendants bool `json:"-"`
 }
 
 type WindowsACLPlan struct {
@@ -76,9 +129,62 @@
 			})
 		}
 	}
+
+	// Shared-path DenyWrite mitigations (C:\, ProgramData, Windows\Temp,
+	// Users\Public) existed only to compensate for Users/Authenticated Users
+	// SID broadening on fully restricted DenyRead tokens. That broadening is
+	// permanently disabled (see runWindowsSandboxCommand): preflight DACL
+	// snapshots cannot enforce a write boundary for the command's lifetime.
+	// Do not stamp new machine-wide DenyWrite ACEs for a feature that never
+	// enables. Still revoke the stable read-only capability SID on configured
+	// write roots when DenyRead is set on the elevated tier, so hosts that
+	// ran earlier PR builds (which did apply shared/descendant denies) do not
+	// keep a stale deny that wins over the write root's Allow.
+	if config.SandboxLevel == WindowsSandboxLevelRestrictedToken && len(config.PermissionProfile.FileSystem.DenyRead) > 0 && len(writeCapabilities) > 0 {
+		caps, err := LoadOrCreateWindowsCapabilitySIDs(config.SandboxHome)
+		if err != nil {
+			return WindowsACLPlan{}, err
+		}
+		denySID := caps.ReadOnly
+		for _, capability := range writeCapabilities {
+			entries = append(entries, WindowsACLEntry{
+				Action:            WindowsACLRevokeCapability,
+				Path:              capability.Root,
+				Capability:        denySID,
+				NoInherit:         true,
+				RevokeDescendants: true,
+			})
+		}
+	}
+
 	return WindowsACLPlan{Entries: dedupeWindowsACLEntries(entries)}, nil
 }
 
+// windowsPathUnderAnyRoot reports whether path is exactly, or nested under, one
+// of the configured write-root capabilities. A shared-path deny must skip both
+// cases: denying a root that IS a write root would block the workspace outright,
+// and denying a root that merely CONTAINS one (e.g. a shared path of
+// C:\Users\Public when C:\Users itself is a configured write root) would place
+// an explicit Deny ahead of that root's Allow for every broadened token,
+// winning under Windows' deny-before-allow evaluation and jailing a directory
+// the user explicitly configured as writable.
+func windowsPathUnderAnyRoot(path string, capabilities []windowsWriteRootCapability) bool {
+	key := windowsCapabilityPathKey(path)
+	if key == "" {
+		return false
+	}
+	for _, cap := range capabilities {
+		rootKey := windowsCapabilityPathKey(cap.Root)
+		if rootKey == "" {
+			continue
+		}
+		if key == rootKey || strings.HasPrefix(key, rootKey+`\`) {
+			return true
+		}
+	}
+	return false
+}
+
 type windowsWriteRootCapability struct {
 	Root                    string
 	SID                     string
@@ -184,7 +290,11 @@
 		if entry.Action == "" || strings.TrimSpace(entry.Path) == "" || strings.TrimSpace(entry.Capability) == "" {
 			continue
 		}
-		key := string(entry.Action) + "\x00" + windowsCapabilityPathKey(entry.Path) + "\x00" + strings.ToLower(entry.Capability)
+		// NoInherit is part of the identity: a direct-only deny and an
+		// inheritable one on the same path/SID are different ACL shapes, and
+		// collapsing them could silently promote a deliberately non-inherited
+		// shared-path deny into an inheritable one (or vice versa).
+		key := string(entry.Action) + "\x00" + windowsCapabilityPathKey(entry.Path) + "\x00" + strings.ToLower(entry.Capability) + "\x00" + fmt.Sprintf("%t", entry.NoInherit)
 		if _, ok := seen[key]; ok {
 			continue
 		}