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

internal/sandbox/manager_test.go

@@ -204,12 +204,19 @@ func TestSandboxManagerBuildsCommandPlanThroughWindowsRunner(t *testing.T) {
 	windowsSandboxInitialized = func() bool { return true }
 	backend := Backend{Name: BackendWindowsRestrictedToken, Available: true, Executable: `C:\zero\zero-windows-command-runner.exe`, Platform: "windows"}
 	policy := DefaultPolicy()
+	// Build the usual restricted FS profile, then clear DenyRead. On non-Windows
+	// hosts PermissionProfileFromPolicy injects credential-store DenyRead paths,
+	// and the Windows plan path rejects any non-empty DenyRead (PR #640). This
+	// happy-path plan must exercise a valid restricted profile without DenyRead;
+	// rejection coverage lives in TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers.
+	profile := PermissionProfileFromPolicy(`C:\workspace`, policy, nil)
+	profile.FileSystem.DenyRead = nil
 	manager := NewSandboxManager(SandboxManagerOptions{GOOS: "windows", Backend: backend})
 	plan, err := manager.BuildCommandPlan(SandboxManagerRequest{
 		WorkspaceRoot:     `C:\workspace`,
 		Command:           CommandSpec{Name: "cmd.exe", Args: []string{"/d", "/s", "/c", "dir"}, Dir: `C:\workspace\src`, Env: []string{"PATH=C:\\Tools", "TERM=xterm"}},
 		Policy:            policy,
-		Profile:           PermissionProfileFromPolicy(`C:\workspace`, policy, nil),
+		Profile:           profile,
 		Preference:        SandboxPreferenceAuto,
 		ValidateExecution: true,
 	})
@@ -243,6 +250,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

@@ -224,10 +224,11 @@ type credentialDenyPaths struct {
 // the preserved caller environment and Zero's own config/token stores. Four
 // 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.
 //   - Candidates are emitted whether or not they currently exist on disk.

internal/sandbox/runner_windows_integration_test.go

@@ -73,6 +73,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)
@@ -156,12 +178,14 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) {
 	}
 
 	sandboxHome := filepath.Join(root, ".zero-sandbox")
+	// Success path: restricted FS write-jail with no DenyRead. Non-empty DenyRead
+	// is unsupported on both restricted-token tiers under the narrow SID set
+	// (PR #640); the rejection probe below covers that separately.
 	profile := PermissionProfile{
 		FileSystem: FileSystemPolicy{
 			Kind:                 FileSystemRestricted,
 			ReadRoots:            []string{root},
 			WriteRoots:           []WritableRoot{{Root: root, ProtectedMetadataNames: []string{".git", ".zero", ".agents"}}},
-			DenyRead:             []string{privateDir},
 			IncludePlatformRoots: true,
 			AllowTemp:            true,
 		},
@@ -190,10 +214,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{
@@ -204,6 +236,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
@@ -387,6 +437,34 @@ 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 ctx.Err() != nil {
+		t.Fatalf("Windows sandbox command timed out: %v\n%s", ctx.Err(), output)
+	}
+	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)
+		}
+	}
+}
+
 // The write jail must hold on a path whose DACL grants Everyone write access.
 //
 // A WRITE_RESTRICTED token runs TWO checks for a write and needs both to pass:

internal/sandbox/windows_acl.go

@@ -2,6 +2,7 @@ package sandbox
 
 import (
 	"errors"
+	"fmt"
 	"path/filepath"
 	"strings"
 )
@@ -12,13 +13,40 @@ const (
 	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 is consumed by
+	// applyWindowsACLPlan for migration cleanup and tests; no current
+	// plan-generation path emits this action, preserving legacy-process confinement.
+	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), which is why direct-only denies must set this
+	// flag rather than rely on inheritance.
+	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. This flag and its associated
+	// walker are consumed by applyWindowsACLPlan and tests; no current
+	// plan-generation path sets this flag since broad shared-root DenyWrite ACEs
+	// and SID broadening are no longer planned.
+	ScanDescendants bool `json:"-"`
+	// RevokeDescendants marks a WindowsACLRevokeCapability entry as needing
+	// stale-deny cleanup applied recursively to existing descendants, not just
+	// the root path itself. This flag is consumed by applyWindowsACLPlan for
+	// migration cleanup and tests; no current plan-generation path sets this flag.
+	RevokeDescendants bool `json:"-"`
 }
 
 type WindowsACLPlan struct {
@@ -76,6 +104,7 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er
 			})
 		}
 	}
+
 	return WindowsACLPlan{Entries: dedupeWindowsACLEntries(entries)}, nil
 }
 
@@ -184,7 +213,11 @@ func dedupeWindowsACLEntries(entries []WindowsACLEntry) []WindowsACLEntry {
 		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
 		}