fix: harden thothctl Windows safe I/O

tools/thothctl/cmd/thothctl/main_test.go

@@ -3,14 +3,14 @@ package main
 import (
 	"bytes"
 	"context"
-	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
-	"runtime"
 	"strconv"
 	"strings"
 	"testing"
+
+	"github.com/aritmolab/thothii/tools/thothctl/internal/testsupport"
 )
 
 func TestRunLogsRedactsAnUnlabelledDeclaredSecret(t *testing.T) {
@@ -155,7 +155,7 @@ func TestRunFailsClosedForTraversalAndParentSymlinkSecretSources(t *testing.T) {
 				t.Fatal(err)
 			}
 			linkDirectory := filepath.Join(fixture.root, "linked")
-			symlinkOrSkip(t, realDirectory, linkDirectory)
+			testsupport.SymlinkOrSkip(t, realDirectory, linkDirectory)
 			return filepath.Join(linkDirectory, "secret")
 		},
 		"final symlink": func(t *testing.T, fixture cliFixture) string {
@@ -164,7 +164,7 @@ func TestRunFailsClosedForTraversalAndParentSymlinkSecretSources(t *testing.T) {
 				t.Fatal(err)
 			}
 			linkSecret := filepath.Join(fixture.root, "linked-secret")
-			symlinkOrSkip(t, realSecret, linkSecret)
+			testsupport.SymlinkOrSkip(t, realSecret, linkSecret)
 			return linkSecret
 		},
 	} {
@@ -458,13 +458,3 @@ func assertDockerNotInvoked(t *testing.T, fixture cliFixture) {
 		t.Errorf("Docker was invoked: stat error = %v", err)
 	}
 }
-
-func symlinkOrSkip(t *testing.T, target, link string) {
-	t.Helper()
-	if err := os.Symlink(target, link); err != nil {
-		if runtime.GOOS == "windows" && errors.Is(err, os.ErrPermission) {
-			t.Skip("Windows symlink privilege is unavailable")
-		}
-		t.Fatal(err)
-	}
-}

tools/thothctl/internal/safeio/files_test.go

@@ -4,8 +4,9 @@ import (
 	"errors"
 	"os"
 	"path/filepath"
-	"runtime"
 	"testing"
+
+	"github.com/aritmolab/thothii/tools/thothctl/internal/testsupport"
 )
 
 func TestReadCanonicalRegularRejectsFinalAndParentSymlinks(t *testing.T) {
@@ -29,24 +30,14 @@ func TestReadCanonicalRegularRejectsFinalAndParentSymlinks(t *testing.T) {
 	}
 
 	parentLink := filepath.Join(root, "parent-link")
-	symlinkOrSkip(t, realDirectory, parentLink)
+	testsupport.SymlinkOrSkip(t, realDirectory, parentLink)
 	if _, err := ReadCanonicalRegular(filepath.Join(parentLink, "secret"), 1024); !errors.Is(err, ErrUnsafeFile) {
 		t.Fatalf("parent symlink error = %v, want ErrUnsafeFile", err)
 	}
 
 	finalLink := filepath.Join(root, "final-link")
-	symlinkOrSkip(t, realFile, finalLink)
+	testsupport.SymlinkOrSkip(t, realFile, finalLink)
 	if _, err := ReadCanonicalRegular(finalLink, 1024); !errors.Is(err, ErrUnsafeFile) {
 		t.Fatalf("final symlink error = %v, want ErrUnsafeFile", err)
 	}
 }
-
-func symlinkOrSkip(t *testing.T, target, link string) {
-	t.Helper()
-	if err := os.Symlink(target, link); err != nil {
-		if runtime.GOOS == "windows" && errors.Is(err, os.ErrPermission) {
-			t.Skip("Windows symlink privilege is unavailable")
-		}
-		t.Fatal(err)
-	}
-}

tools/thothctl/internal/safeio/files_windows.go

@@ -10,11 +10,15 @@ import (
 	"golang.org/x/sys/windows"
 )
 
+const windowsRetainedHandleShareMode uint32 = windows.FILE_SHARE_READ | windows.FILE_SHARE_WRITE
+
 // ReadCanonicalRegular opens each component with FILE_FLAG_OPEN_REPARSE_POINT and rejects a
-// reparse point on the opened handle before opening the next component. Windows' Win32 API does
+// reparse point on the opened handle before opening the next component. Retained handles allow
-// not expose a portable descriptor-relative equivalent of POSIX openat, so a hostile local actor
+// ordinary read/write sharing but deny delete sharing, which blocks rename or deletion after a
-// with permission to rename a normal parent between these opens remains outside this guarantee.
+// component is opened and throughout the final read. Windows' Win32 API does not expose a
-// Installation directories therefore need trusted local filesystem/ACL ownership on Windows.
+// portable descriptor-relative equivalent of POSIX openat, so a hostile local actor can still
+// replace a not-yet-opened normal component between absolute-path opens. Installation directories
+// therefore need trusted local filesystem/ACL ownership on Windows.
 func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) {
 	if err := ValidateCanonicalPath(path); err != nil {
 		return nil, err
@@ -62,7 +66,7 @@ func openWindowsComponent(path string, directory bool) (windows.Handle, error) {
 	handle, err := windows.CreateFile(
 		windows.StringToUTF16Ptr(path),
 		windows.GENERIC_READ,
-		windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE,
+		windowsRetainedHandleShareMode,
 		nil,
 		windows.OPEN_EXISTING,
 		flags,

tools/thothctl/internal/safeio/files_windows_test.go

@@ -0,0 +1,68 @@
+//go:build windows
+
+package safeio
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"golang.org/x/sys/windows"
+)
+
+const expectedWindowsRetainedHandleShareMode = windows.FILE_SHARE_READ | windows.FILE_SHARE_WRITE
+
+// Keep this contract compile-enforced so Windows cross-test compilation catches a future
+// FILE_SHARE_DELETE regression even when the tests are compiled on a non-Windows host.
+var _ [windowsRetainedHandleShareMode - expectedWindowsRetainedHandleShareMode]struct{}
+var _ [expectedWindowsRetainedHandleShareMode - windowsRetainedHandleShareMode]struct{}
+
+func TestOpenWindowsComponentBlocksMutationWhileHandleIsRetained(t *testing.T) {
+	t.Run("parent rename", func(t *testing.T) {
+		parent := filepath.Join(t.TempDir(), "parent")
+		if err := os.Mkdir(parent, 0o700); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(filepath.Join(parent, "secret"), []byte("secret"), 0o600); err != nil {
+			t.Fatal(err)
+		}
+
+		handle, err := openWindowsComponent(parent, true)
+		if err != nil {
+			t.Fatal(err)
+		}
+		renamed := parent + "-renamed"
+		if err := os.Rename(parent, renamed); err == nil {
+			windows.CloseHandle(handle)
+			t.Fatal("parent rename succeeded while its safe-I/O handle was retained")
+		}
+		if err := windows.CloseHandle(handle); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Rename(parent, renamed); err != nil {
+			t.Fatalf("parent rename after closing its safe-I/O handle: %v", err)
+		}
+	})
+
+	t.Run("final delete", func(t *testing.T) {
+		path := filepath.Join(t.TempDir(), "secret")
+		if err := os.WriteFile(path, []byte("secret"), 0o600); err != nil {
+			t.Fatal(err)
+		}
+
+		handle, err := openWindowsComponent(path, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Remove(path); err == nil {
+			windows.CloseHandle(handle)
+			t.Fatal("final-file deletion succeeded while its safe-I/O handle was retained")
+		}
+		if err := windows.CloseHandle(handle); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Remove(path); err != nil {
+			t.Fatalf("final-file deletion after closing its safe-I/O handle: %v", err)
+		}
+	})
+}

tools/thothctl/internal/testsupport/symlink.go

@@ -0,0 +1,19 @@
+// Package testsupport provides portable helpers shared by thothctl tests.
+package testsupport
+
+import (
+	"os"
+	"testing"
+)
+
+// SymlinkOrSkip creates a symlink or skips only when Windows reports that symlink privilege is
+// unavailable. All other failures remain test failures.
+func SymlinkOrSkip(t testing.TB, target, link string) {
+	t.Helper()
+	if err := os.Symlink(target, link); err != nil {
+		if isSymlinkPrivilegeUnavailable(err) {
+			t.Skip("Windows symlink privilege is unavailable")
+		}
+		t.Fatal(err)
+	}
+}

tools/thothctl/internal/testsupport/symlink_nonwindows.go

@@ -0,0 +1,7 @@
+//go:build !windows
+
+package testsupport
+
+func isSymlinkPrivilegeUnavailable(_ error) bool {
+	return false
+}

tools/thothctl/internal/testsupport/symlink_test.go

@@ -0,0 +1,12 @@
+package testsupport
+
+import (
+	"errors"
+	"testing"
+)
+
+func TestSymlinkPrivilegeUnavailableDoesNotMatchUnrelatedErrors(t *testing.T) {
+	if isSymlinkPrivilegeUnavailable(errors.New("unrelated symlink failure")) {
+		t.Fatal("unrelated symlink failure was classified as a missing Windows privilege")
+	}
+}

tools/thothctl/internal/testsupport/symlink_windows.go

@@ -0,0 +1,14 @@
+//go:build windows
+
+package testsupport
+
+import (
+	"errors"
+	"os"
+
+	"golang.org/x/sys/windows"
+)
+
+func isSymlinkPrivilegeUnavailable(err error) bool {
+	return errors.Is(err, os.ErrPermission) || errors.Is(err, windows.ERROR_PRIVILEGE_NOT_HELD)
+}

tools/thothctl/internal/testsupport/symlink_windows_test.go

@@ -0,0 +1,38 @@
+//go:build windows
+
+package testsupport
+
+import (
+	"os"
+	"testing"
+
+	"golang.org/x/sys/windows"
+)
+
+func TestSymlinkPrivilegeUnavailableRecognizesOnlyWindowsPrivilegeErrors(t *testing.T) {
+	for name, err := range map[string]error{
+		"permission": os.ErrPermission,
+		"privilege not held": &os.LinkError{
+			Op:  "symlink",
+			Old: "target",
+			New: "link",
+			Err: windows.ERROR_PRIVILEGE_NOT_HELD,
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			if !isSymlinkPrivilegeUnavailable(err) {
+				t.Fatalf("isSymlinkPrivilegeUnavailable(%v) = false, want true", err)
+			}
+		})
+	}
+
+	unrelated := &os.LinkError{
+		Op:  "symlink",
+		Old: "target",
+		New: "link",
+		Err: windows.ERROR_FILENAME_EXCED_RANGE,
+	}
+	if isSymlinkPrivilegeUnavailable(unrelated) {
+		t.Fatal("unrelated Windows symlink failure was classified as a missing privilege")
+	}
+}