fix(sandbox): address CodeRabbit review feedback on PR 640

Author: euxaristia

internal/sandbox/runner_windows_integration_test.go

@@ -451,6 +451,9 @@ func runWindowsRealSmokeCommandExpectError(t *testing.T, runnerExe string, base
 	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)
 	}

internal/sandbox/windows_acl.go

@@ -16,16 +16,9 @@ const (
 	// 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.
+	// 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"
 )
 
@@ -44,33 +37,15 @@ type WindowsACLEntry struct {
 	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.
+	// 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 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 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:"-"`
 }
 

internal/sandbox/windows_acl_descendants.go

@@ -42,32 +42,3 @@ func windowsPathIsDriveRootPath(path string) bool {
 	c := trimmed[0]
 	return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')
 }
-
-// windowsMountPathIsOnlySystemDrive reports whether a volume mount path is the
-// system drive root (e.g. `C:\` or `C:`) rather than another letter or a
-// folder mount such as `C:\mnt\data`. Used by the volume gate so a second
-// fixed volume mounted only as a folder is rejected the same as one mounted
-// on a drive letter.
-func windowsMountPathIsOnlySystemDrive(mountPath, systemDrive string) bool {
-	trimmed := strings.TrimSuffix(mountPath, `\`)
-	return strings.EqualFold(strings.ToUpper(trimmed), strings.ToUpper(systemDrive))
-}
-
-// windowsMountPathsAreOnlySystemDrive reports whether mountPaths (a fixed
-// volume's DOS/folder mount points from GetVolumePathNamesForVolumeName) name
-// the system drive root and nothing else. A fixed volume with NO mount paths
-// at all is still directly reachable through its raw "\\?\Volume{GUID}\"
-// path even though it has no conventional mount point, so an empty list
-// fails closed (false) instead of being read as "unreachable." Pure string
-// logic so non-Windows tests can pin the fail-closed cases without Win32.
-func windowsMountPathsAreOnlySystemDrive(mountPaths []string, systemDrive string) bool {
-	if len(mountPaths) == 0 {
-		return false
-	}
-	for _, mountPath := range mountPaths {
-		if !windowsMountPathIsOnlySystemDrive(mountPath, systemDrive) {
-			return false
-		}
-	}
-	return true
-}

internal/sandbox/windows_acl_descendants_test.go

@@ -43,53 +43,3 @@ func TestWindowsPathIsDriveRootPath(t *testing.T) {
 		}
 	}
 }
-
-func TestWindowsMountPathIsOnlySystemDrive(t *testing.T) {
-	cases := []struct {
-		mount, system string
-		want          bool
-	}{
-		{`C:\`, `C:`, true},
-		{`C:`, `C:`, true},
-		{`c:\`, `C:`, true},
-		{`D:\`, `C:`, false},
-		{`C:\mnt\data`, `C:`, false},
-		{`C:\mnt\data\`, `C:`, false},
-		{`\\?\Volume{guid}\`, `C:`, false},
-	}
-	for _, tc := range cases {
-		got := windowsMountPathIsOnlySystemDrive(tc.mount, tc.system)
-		if got != tc.want {
-			t.Fatalf("windowsMountPathIsOnlySystemDrive(%q, %q) = %v, want %v", tc.mount, tc.system, got, tc.want)
-		}
-	}
-}
-
-// TestWindowsMountPathsAreOnlySystemDrive pins the volume-gate fail-closed fix
-// (jatmn's review): a fixed volume with no mount points at all must not be
-// read as "unreachable" (it is still reachable via its raw
-// "\\?\Volume{GUID}\" path), and any mount path other than the system drive
-// root disqualifies the volume, matching the existing per-path behavior.
-func TestWindowsMountPathsAreOnlySystemDrive(t *testing.T) {
-	cases := []struct {
-		name       string
-		mountPaths []string
-		system     string
-		want       bool
-	}{
-		{"only system drive", []string{`C:\`}, `C:`, true},
-		{"no mount points at all", nil, `C:`, false},
-		{"empty mount list", []string{}, `C:`, false},
-		{"extra drive letter", []string{`C:\`, `D:\`}, `C:`, false},
-		{"folder mount point", []string{`C:\mnt\data`}, `C:`, false},
-		{"other drive only", []string{`D:\`}, `C:`, false},
-	}
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			got := windowsMountPathsAreOnlySystemDrive(tc.mountPaths, tc.system)
-			if got != tc.want {
-				t.Fatalf("windowsMountPathsAreOnlySystemDrive(%#v, %q) = %v, want %v", tc.mountPaths, tc.system, got, tc.want)
-			}
-		})
-	}
-}

internal/sandbox/windows_acl_descendants_windows_test.go

@@ -568,8 +568,12 @@ func TestWindowsAceSIDSkipsUnhandledAceTypes(t *testing.T) {
 }
 
 func TestWindowsPathDeniesCapabilitySIDRequiresEssentialWriteMask(t *testing.T) {
+	caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir())
+	if err != nil {
+		t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err)
+	}
+	sid := caps.ReadOnly
 	dir := t.TempDir()
-	sid := "S-1-1-0"
 
 	group := windowsACLPathGroup{
 		Path: dir,
@@ -634,7 +638,14 @@ func denyCapabilityMask(t *testing.T, path, capabilitySID string, mask windows.A
 // pass through a Users/AuthUsers allow under a partial-deny check that only
 // tested non-zero overlap with the probe mask.
 func TestWindowsPathDeniesCapabilitySIDRejectsPartialWriteDeny(t *testing.T) {
-	const sidStr = "S-1-1-0"
+	// Synthetic capability SID: a full DenyWrite for a group the test process
+	// belongs to (e.g. Everyone) denies DELETE/WRITE_DAC and blocks t.TempDir
+	// cleanup.
+	caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir())
+	if err != nil {
+		t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err)
+	}
+	sidStr := caps.ReadOnly
 	dir := t.TempDir()
 	denyCapabilityMask(t, dir, sidStr, windows.FILE_WRITE_ATTRIBUTES)