Set Windows DACL on discovery directory under LOCALAPPDATA

pkg/server/discovery/discovery.go

@@ -78,8 +78,10 @@ func writeServerInfoTo(dir string, info *ServerInfo) error {
 
 	// Tighten permissions on the directory in case it already existed with
 	// looser permissions. MkdirAll only applies mode to newly-created dirs.
-	if err := os.Chmod(dir, dirPermissions); err != nil {
-		return fmt.Errorf("failed to set discovery directory permissions: %w", err)
+	// On Windows this sets an explicit protected DACL (POSIX modes are
+	// advisory on NTFS); elsewhere it is os.Chmod(dirPermissions).
+	if err := restrictDiscoveryDirPermissions(dir); err != nil {
+		return err
 	}
 
 	path := filepath.Join(dir, "server.json")

pkg/server/discovery/discovery_test.go

@@ -6,6 +6,7 @@ package discovery
 import (
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 	"time"
 
@@ -115,6 +116,12 @@ func TestRemoveServerInfo_NotFound(t *testing.T) {
 
 func TestWriteServerInfo_FilePermissions(t *testing.T) {
 	t.Parallel()
+	// POSIX file modes are advisory on NTFS; Windows reports 0666 for
+	// newly written files regardless of the mode passed to AtomicWriteFile.
+	// Directory ACL coverage for Windows lives in permissions_windows_test.go.
+	if runtime.GOOS == "windows" {
+		t.Skip("POSIX file modes are not meaningful on Windows")
+	}
 	dir := t.TempDir()
 
 	info := &ServerInfo{
@@ -132,6 +139,11 @@ func TestWriteServerInfo_FilePermissions(t *testing.T) {
 
 func TestWriteServerInfo_CreatesDirectoryWithCorrectPermissions(t *testing.T) {
 	t.Parallel()
+	// On Windows, writeServerInfoTo sets an explicit DACL instead of relying
+	// on os.Chmod; see TestWriteServerInfo_WindowsDACL_NoOtherInteractiveUsers.
+	if runtime.GOOS == "windows" {
+		t.Skip("POSIX directory modes are not meaningful on Windows; see DACL tests")
+	}
 	parent := t.TempDir()
 	dir := filepath.Join(parent, "nested", "server")
 
@@ -195,6 +207,11 @@ func TestReadServerInfo_RejectsSymlink(t *testing.T) {
 
 func TestWriteServerInfo_TightensExistingDirPermissions(t *testing.T) {
 	t.Parallel()
+	// On Windows the equivalent "tighten existing" path is covered by
+	// TestRestrictDiscoveryDirPermissions_ReplacesExistingLooseACL.
+	if runtime.GOOS == "windows" {
+		t.Skip("POSIX directory modes are not meaningful on Windows; see DACL tests")
+	}
 
 	// Create a directory with deliberately too-loose permissions.
 	dir := t.TempDir()

pkg/server/discovery/permissions_other.go

@@ -0,0 +1,21 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+//go:build !windows
+
+package discovery
+
+import (
+	"fmt"
+	"os"
+)
+
+// restrictDiscoveryDirPermissions tightens POSIX mode bits on the discovery
+// directory. On non-Windows platforms this is the Chmod that previously lived
+// inline in writeServerInfoTo.
+func restrictDiscoveryDirPermissions(dir string) error {
+	if err := os.Chmod(dir, dirPermissions); err != nil {
+		return fmt.Errorf("failed to set discovery directory permissions: %w", err)
+	}
+	return nil
+}

pkg/server/discovery/permissions_windows.go

@@ -0,0 +1,79 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+//go:build windows
+
+package discovery
+
+import (
+	"fmt"
+
+	"golang.org/x/sys/windows"
+)
+
+// restrictDiscoveryDirPermissions replaces the discovery directory DACL with
+// an explicit ACL granting FILE-equivalent GenericAll only to the process
+// user and SYSTEM, and marks the DACL protected so parent ACEs cannot
+// re-inherit. os.Chmod is advisory on NTFS and does not strip inherited
+// ACEs under %LOCALAPPDATA%, which is the gap that lets another interactive
+// user rewrite server.json (for example to point at an attacker named pipe).
+//
+// Inheritance (OICI) is intentional: server.json and any future children
+// pick up the same restriction instead of inheriting a looser parent ACL.
+func restrictDiscoveryDirPermissions(dir string) error {
+	userSID, err := currentProcessUserSID()
+	if err != nil {
+		return fmt.Errorf("failed to resolve current user SID: %w", err)
+	}
+	systemSID, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid)
+	if err != nil {
+		return fmt.Errorf("failed to resolve SYSTEM SID: %w", err)
+	}
+
+	acl, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{
+		{
+			AccessPermissions: windows.GENERIC_ALL,
+			AccessMode:        windows.GRANT_ACCESS,
+			Inheritance:       windows.OBJECT_INHERIT_ACE | windows.CONTAINER_INHERIT_ACE,
+			Trustee: windows.TRUSTEE{
+				TrusteeForm:  windows.TRUSTEE_IS_SID,
+				TrusteeValue: windows.TrusteeValueFromSID(userSID),
+			},
+		},
+		{
+			AccessPermissions: windows.GENERIC_ALL,
+			AccessMode:        windows.GRANT_ACCESS,
+			Inheritance:       windows.OBJECT_INHERIT_ACE | windows.CONTAINER_INHERIT_ACE,
+			Trustee: windows.TRUSTEE{
+				TrusteeForm:  windows.TRUSTEE_IS_SID,
+				TrusteeType:  windows.TRUSTEE_IS_WELL_KNOWN_GROUP,
+				TrusteeValue: windows.TrusteeValueFromSID(systemSID),
+			},
+		},
+	}, nil)
+	if err != nil {
+		return fmt.Errorf("failed to build discovery directory DACL: %w", err)
+	}
+
+	if err := windows.SetNamedSecurityInfo(
+		dir,
+		windows.SE_FILE_OBJECT,
+		windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION,
+		nil,
+		nil,
+		acl,
+		nil,
+	); err != nil {
+		return fmt.Errorf("failed to set discovery directory DACL: %w", err)
+	}
+	return nil
+}
+
+func currentProcessUserSID() (*windows.SID, error) {
+	token := windows.GetCurrentProcessToken()
+	tokenUser, err := token.GetTokenUser()
+	if err != nil {
+		return nil, err
+	}
+	return tokenUser.User.Sid, nil
+}

pkg/server/discovery/permissions_windows_test.go

@@ -0,0 +1,167 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+//go:build windows
+
+package discovery
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+	"unsafe"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/windows"
+)
+
+// TestWriteServerInfo_WindowsDACL_NoOtherInteractiveUsers asserts the
+// acceptance criterion from #5217: after writeServerInfoTo, the discovery
+// directory DACL grants access only to the current user and SYSTEM, and does
+// not retain Everyone / Authenticated Users / other interactive-user ACEs
+// that MkdirAll would otherwise inherit (and that os.Chmod cannot strip).
+func TestWriteServerInfo_WindowsDACL_NoOtherInteractiveUsers(t *testing.T) {
+	t.Parallel()
+
+	parent := t.TempDir()
+	dir := filepath.Join(parent, "toolhive", "server")
+
+	// Seed a deliberately loose ACL on the parent so newly created children
+	// inherit Everyone. This models a shared / misconfigured LOCALAPPDATA
+	// tree better than relying on whatever TempDir happens to carry.
+	grantEveryone(t, parent)
+
+	info := &ServerInfo{
+		URL:       "npipe://thv-api",
+		PID:       1,
+		Nonce:     "dacl-nonce",
+		StartedAt: time.Now().UTC(),
+	}
+	require.NoError(t, writeServerInfoTo(dir, info))
+
+	assertDiscoveryDACLRestricted(t, dir)
+}
+
+// TestRestrictDiscoveryDirPermissions_ReplacesExistingLooseACL covers the
+// sibling failure mode: the discovery directory already exists with a loose
+// DACL (Everyone + inherited ACEs). restrictDiscoveryDirPermissions must
+// replace that ACL rather than merge, and block further inheritance.
+func TestRestrictDiscoveryDirPermissions_ReplacesExistingLooseACL(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	grantEveryone(t, dir)
+
+	before := discoveryDirSDDL(t, dir)
+	require.Contains(t, strings.ToUpper(before), "WD", "precondition: Everyone (WD) must be present before restrict")
+
+	require.NoError(t, restrictDiscoveryDirPermissions(dir))
+	assertDiscoveryDACLRestricted(t, dir)
+}
+
+// TestRestrictDiscoveryDirPermissions_NewDirectory covers the create path
+// (no pre-existing ACL to replace) so MkdirAll + restrict still lands a
+// protected owner/SYSTEM-only DACL.
+func TestRestrictDiscoveryDirPermissions_NewDirectory(t *testing.T) {
+	t.Parallel()
+
+	parent := t.TempDir()
+	grantEveryone(t, parent)
+	dir := filepath.Join(parent, "fresh-server")
+	require.NoError(t, os.MkdirAll(dir, dirPermissions))
+
+	require.NoError(t, restrictDiscoveryDirPermissions(dir))
+	assertDiscoveryDACLRestricted(t, dir)
+}
+
+func grantEveryone(t *testing.T, path string) {
+	t.Helper()
+	// icacls grants are the product-path way to introduce a loose ACE;
+	// quoting keeps PowerShell from expanding (OI)/(CI).
+	cmd := exec.Command("icacls", path, "/grant", "*S-1-1-0:(OI)(CI)M")
+	out, err := cmd.CombinedOutput()
+	require.NoError(t, err, "icacls grant Everyone failed: %s", out)
+}
+
+func assertDiscoveryDACLRestricted(t *testing.T, dir string) {
+	t.Helper()
+
+	userSID, err := currentProcessUserSID()
+	require.NoError(t, err)
+	systemSID, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid)
+	require.NoError(t, err)
+	everyoneSID, err := windows.CreateWellKnownSid(windows.WinWorldSid)
+	require.NoError(t, err)
+	authUsersSID, err := windows.CreateWellKnownSid(windows.WinAuthenticatedUserSid)
+	require.NoError(t, err)
+
+	sd, err := windows.GetNamedSecurityInfo(
+		dir,
+		windows.SE_FILE_OBJECT,
+		windows.DACL_SECURITY_INFORMATION,
+	)
+	require.NoError(t, err)
+
+	control, _, err := sd.Control()
+	require.NoError(t, err)
+	assert.NotZero(t, control&windows.SE_DACL_PROTECTED, "DACL must be protected against inheritance")
+
+	dacl, _, err := sd.DACL()
+	require.NoError(t, err)
+	require.NotNil(t, dacl)
+
+	aces, err := allowACEsFromACL(dacl)
+	require.NoError(t, err)
+
+	var userSeen, systemSeen bool
+	for _, ace := range aces {
+		sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart))
+		switch {
+		case userSID.Equals(sid):
+			userSeen = true
+		case systemSID.Equals(sid):
+			systemSeen = true
+		case everyoneSID.Equals(sid):
+			t.Fatalf("DACL still grants Everyone (%s)", sid)
+		case authUsersSID.Equals(sid):
+			t.Fatalf("DACL still grants Authenticated Users (%s)", sid)
+		default:
+			// Administrators / package SIDs / other interactive users must
+			// not remain after an explicit replace. Fail closed on anything
+			// that is not the process user or SYSTEM.
+			t.Fatalf("unexpected allow ACE for SID %s (want only current user + SYSTEM)", sid)
+		}
+	}
+	assert.True(t, userSeen, "DACL must grant the current process user")
+	assert.True(t, systemSeen, "DACL must grant SYSTEM")
+}
+
+func discoveryDirSDDL(t *testing.T, dir string) string {
+	t.Helper()
+	sd, err := windows.GetNamedSecurityInfo(
+		dir,
+		windows.SE_FILE_OBJECT,
+		windows.DACL_SECURITY_INFORMATION,
+	)
+	require.NoError(t, err)
+	return sd.String()
+}
+
+func allowACEsFromACL(acl *windows.ACL) ([]*windows.ACCESS_ALLOWED_ACE, error) {
+	aces := make([]*windows.ACCESS_ALLOWED_ACE, 0, acl.AceCount)
+	for i := uint16(0); i < acl.AceCount; i++ {
+		var ace *windows.ACCESS_ALLOWED_ACE
+		if err := windows.GetAce(acl, uint32(i), &ace); err != nil {
+			return nil, err
+		}
+		if ace.Header.AceType != windows.ACCESS_ALLOWED_ACE_TYPE {
+			continue
+		}
+		aces = append(aces, ace)
+	}
+	return aces, nil
+}