From ba3421534a0165b413607343c4e2606a95e5db39 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Wed, 6 May 2026 12:58:31 +0200 Subject: [PATCH 1/5] Support npipe URLs in discovery health check discovery.HTTPClientForURL knew http:// and unix:// only, so a discovery file URL written for a Windows named pipe would be unparseable. Every thv serve startup runs discovery.Discover to detect a previous instance via the discovery URL, which routes through HTTPClientForURL, so without an npipe arm two concurrent thv processes on Windows could not see each other and would silently overwrite the discovery file. Add an npipe:// arm whose transport DialContext goes through winio.DialPipeContext, plus a ParseNamedPipeURL helper that strips the scheme and validates the remaining segment (non-empty, no path separators, no ".."). Split the actual dialer into pipe_windows.go (winio-backed) and pipe_other.go (returns a clear "only supported on Windows" error), so non-Windows builds do not import winio and a misconfigured discovery URL surfaces a useful message instead of an obscure dial syscall failure. go-winio is already a direct dependency at v0.6.2, so no go.mod change is required. --- pkg/server/discovery/health.go | 50 +++++++++++++++++++++++++--- pkg/server/discovery/pipe_other.go | 20 +++++++++++ pkg/server/discovery/pipe_windows.go | 19 +++++++++++ 3 files changed, 85 insertions(+), 4 deletions(-) create mode 100644 pkg/server/discovery/pipe_other.go create mode 100644 pkg/server/discovery/pipe_windows.go diff --git a/pkg/server/discovery/health.go b/pkg/server/discovery/health.go index cf80cb718d..abaeb4fb86 100644 --- a/pkg/server/discovery/health.go +++ b/pkg/server/discovery/health.go @@ -23,8 +23,13 @@ const ( NonceHeader = "X-Toolhive-Nonce" ) +// namedPipePrefix is the Windows named-pipe namespace prefix used to +// reconstruct addresses for winio.DialPipeContext. +const namedPipePrefix = `\\.\pipe\` + // CheckHealth verifies that a server at the given URL is healthy and optionally -// matches the expected nonce. It supports http:// and unix:// URL schemes. +// matches the expected nonce. It supports http://, unix://, and npipe:// URL +// schemes (npipe:// only resolves on Windows). func CheckHealth(ctx context.Context, serverURL string, expectedNonce string) error { client, requestURL, err := buildHealthClient(serverURL) if err != nil { @@ -76,9 +81,10 @@ func buildHealthClient(serverURL string) (*http.Client, string, error) { // HTTPClientForURL returns an HTTP client configured for the given server URL // and the base URL to use for requests. For unix:// URLs it creates a client // with a Unix socket transport and returns "http://localhost" as the base URL. -// For http:// URLs it validates the host is a loopback address and returns a -// default client. The returned client has no timeout set; callers should apply -// their own timeout via context or client.Timeout. +// For npipe:// URLs (Windows only) it dials the named pipe via the platform +// dialNamedPipe helper. For http:// URLs it validates the host is a loopback +// address and returns a default client. The returned client has no timeout +// set; callers should apply their own timeout via context or client.Timeout. func HTTPClientForURL(serverURL string) (*http.Client, string, error) { switch { case strings.HasPrefix(serverURL, "unix://"): @@ -95,6 +101,20 @@ func HTTPClientForURL(serverURL string) (*http.Client, string, error) { } return client, "http://localhost", nil + case strings.HasPrefix(serverURL, "npipe://"): + pipePath, err := ParseNamedPipeURL(serverURL) + if err != nil { + return nil, "", err + } + client := &http.Client{ + Transport: &http.Transport{ + DialContext: func(ctx context.Context, _, _ string) (net.Conn, error) { + return dialNamedPipe(ctx, pipePath) + }, + }, + } + return client, "http://localhost", nil + case strings.HasPrefix(serverURL, "http://"): if err := ValidateLoopbackURL(serverURL); err != nil { return nil, "", err @@ -144,3 +164,25 @@ func ParseUnixSocketPath(rawURL string) (string, error) { return path, nil } + +// ParseNamedPipeURL extracts and validates the pipe name from an npipe:// URL +// and returns the full Windows pipe path (e.g. \\.\pipe\thv-api). The name +// portion must be a single segment with no path separators or traversal +// components, since the toolhive listener only ever publishes local pipes +// under the \\.\pipe\ namespace. +func ParseNamedPipeURL(rawURL string) (string, error) { + if !strings.HasPrefix(rawURL, "npipe://") { + return "", fmt.Errorf("named pipe URL must start with npipe://: %s", rawURL) + } + name := strings.TrimPrefix(rawURL, "npipe://") + if name == "" { + return "", fmt.Errorf("empty named pipe name") + } + if strings.ContainsAny(name, `/\`) { + return "", fmt.Errorf("named pipe name must not contain path separators: %s", name) + } + if strings.Contains(name, "..") { + return "", fmt.Errorf("named pipe name must not contain '..': %s", name) + } + return namedPipePrefix + name, nil +} diff --git a/pkg/server/discovery/pipe_other.go b/pkg/server/discovery/pipe_other.go new file mode 100644 index 0000000000..b16b046cac --- /dev/null +++ b/pkg/server/discovery/pipe_other.go @@ -0,0 +1,20 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package discovery + +import ( + "context" + "fmt" + "net" +) + +// dialNamedPipe is a no-op stub on non-Windows platforms. The npipe:// scheme +// is reachable here only via a misconfigured discovery file or a hand-crafted +// URL; surface a clear error rather than fail with a confusing dial syscall +// result. +func dialNamedPipe(_ context.Context, name string) (net.Conn, error) { + return nil, fmt.Errorf("named pipes are only supported on Windows: %s", name) +} diff --git a/pkg/server/discovery/pipe_windows.go b/pkg/server/discovery/pipe_windows.go new file mode 100644 index 0000000000..bf08e8f6c6 --- /dev/null +++ b/pkg/server/discovery/pipe_windows.go @@ -0,0 +1,19 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package discovery + +import ( + "context" + "net" + + "github.com/Microsoft/go-winio" +) + +// dialNamedPipe opens a connection to the Windows named pipe at name. The +// caller is expected to have validated name via ParseNamedPipeURL. +func dialNamedPipe(ctx context.Context, name string) (net.Conn, error) { + return winio.DialPipeContext(ctx, name) +} From 9eeceab71a0f8c785dbba42dcff8f73b1beb8bd9 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Wed, 6 May 2026 12:58:31 +0200 Subject: [PATCH 2/5] Add Windows named-pipe support to API listener setupUnixSocket assumed the address was a filesystem path -- it ran os.Stat, os.Remove, os.MkdirAll, and os.Chmod around net.Listen("unix", ...). None of that applies to a Windows named pipe (\.\pipe\thv-api), and net.Listen("unix", ...) is not a substitute since named pipes are a different kernel object. As a result, toolhive-studio could not run thv with a pipe address on Windows. Split setupUnixSocket and cleanupUnixSocket into per-platform files via build tags, mirroring pkg/container/docker/sdk. On Windows, branch on the \.\pipe\ prefix and use winio.ListenPipe with InputBufferSize and OutputBufferSize at 64 KiB; MessageMode stays at byte-stream because HTTP requires byte framing. Skip stat/remove/mkdir/chmod for pipes since they are not files; keep stat/remove/mkdir for the AF_UNIX fallback (Win10 1803+) but drop chmod because POSIX modes do not apply on Windows. ListenURL emits npipe:// for pipe addresses so the discovery file URL stays unambiguous. createListener labels the listener as "Windows named pipe" and rejects pipe addresses on non-Windows up front, rather than creating a literal-backslash file via AF_UNIX. --- pkg/api/server.go | 74 ++++++++++++++------------------- pkg/api/socket_other.go | 54 ++++++++++++++++++++++++ pkg/api/socket_windows.go | 87 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 44 deletions(-) create mode 100644 pkg/api/socket_other.go create mode 100644 pkg/api/socket_windows.go diff --git a/pkg/api/server.go b/pkg/api/server.go index bc23dcd490..07d8f1cc48 100644 --- a/pkg/api/server.go +++ b/pkg/api/server.go @@ -27,6 +27,7 @@ import ( "net/http" "os" "path/filepath" + stdruntime "runtime" "strings" "time" @@ -366,41 +367,20 @@ func (b *ServerBuilder) setupDefaultRoutes(r *chi.Mux) { } } -func setupTCPListener(address string) (net.Listener, error) { - return net.Listen("tcp", address) -} - -func setupUnixSocket(address string) (net.Listener, error) { - // Remove the socket file if it already exists - if _, err := os.Stat(address); err == nil { - if err := os.Remove(address); err != nil { - return nil, fmt.Errorf("failed to remove existing socket: %w", err) - } - } - - // Create the directory for the socket file if it doesn't exist - if err := os.MkdirAll(filepath.Dir(address), 0750); err != nil { - return nil, fmt.Errorf("failed to create socket directory: %w", err) - } - - // Create UNIX socket listener - listener, err := net.Listen("unix", address) - if err != nil { - return nil, fmt.Errorf("failed to create UNIX socket listener: %w", err) - } +// namedPipePrefix is the Windows named-pipe namespace prefix. Both per-platform +// socket files use it, and ListenURL/createListener inspect the address prefix +// to choose between AF_UNIX and named-pipe semantics. +const namedPipePrefix = `\\.\pipe\` - // Set file permissions on the socket to allow other local processes to connect - if err := os.Chmod(address, socketPermissions); err != nil { - return nil, fmt.Errorf("failed to set socket permissions: %w", err) - } - - return listener, nil +// isNamedPipeAddress reports whether address is a Windows named-pipe path. +// The check is platform-agnostic so callers on non-Windows can fail fast with +// a clear error before reaching the listener code. +func isNamedPipeAddress(address string) bool { + return strings.HasPrefix(address, namedPipePrefix) } -func cleanupUnixSocket(address string) { - if err := os.Remove(address); err != nil && !os.IsNotExist(err) { - slog.Warn("failed to remove socket file", "error", err) - } +func setupTCPListener(address string) (net.Listener, error) { + return net.Listen("tcp", address) } func headersMiddleware(next http.Handler) http.Handler { @@ -592,7 +572,7 @@ func NewServer(ctx context.Context, builder *ServerBuilder) (*Server, error) { // bound address from the listener (important when binding to port 0). func (s *Server) ListenURL() string { if s.isUnixSocket { - return fmt.Sprintf("unix://%s", s.address) + return socketURL(s.address) } return fmt.Sprintf("http://%s", s.listener.Addr().String()) } @@ -715,24 +695,30 @@ func (s *Server) cleanup() { } } -// createListener creates the appropriate listener based on the configuration +// createListener creates the appropriate listener based on the configuration. +// Named-pipe addresses are only supported on Windows; other platforms reject +// them up front rather than creating a literal-backslash file via AF_UNIX. func createListener(address string, isUnixSocket bool) (net.Listener, string, error) { - var listener net.Listener - var addrType string - var err error + if !isUnixSocket { + listener, err := setupTCPListener(address) + if err != nil { + return nil, "", err + } + return listener, "HTTP", nil + } - if isUnixSocket { - listener, err = setupUnixSocket(address) - addrType = "UNIX socket" - } else { - listener, err = setupTCPListener(address) - addrType = "HTTP" + addrType := "UNIX socket" + if isNamedPipeAddress(address) { + if stdruntime.GOOS != "windows" { + return nil, "", fmt.Errorf("named pipe addresses are only supported on Windows: %s", address) + } + addrType = "Windows named pipe" } + listener, err := setupUnixSocket(address) if err != nil { return nil, "", err } - return listener, addrType, nil } diff --git a/pkg/api/socket_other.go b/pkg/api/socket_other.go new file mode 100644 index 0000000000..e34169298a --- /dev/null +++ b/pkg/api/socket_other.go @@ -0,0 +1,54 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package api + +import ( + "fmt" + "log/slog" + "net" + "os" + "path/filepath" +) + +// setupUnixSocket creates a UNIX domain socket listener at the given path. +// On non-Windows platforms named-pipe addresses are not supported; callers +// guard against that in createListener. +func setupUnixSocket(address string) (net.Listener, error) { + if _, err := os.Stat(address); err == nil { + if err := os.Remove(address); err != nil { + return nil, fmt.Errorf("failed to remove existing socket: %w", err) + } + } + + if err := os.MkdirAll(filepath.Dir(address), 0750); err != nil { + return nil, fmt.Errorf("failed to create socket directory: %w", err) + } + + listener, err := net.Listen("unix", address) + if err != nil { + return nil, fmt.Errorf("failed to create UNIX socket listener: %w", err) + } + + if err := os.Chmod(address, socketPermissions); err != nil { + return nil, fmt.Errorf("failed to set socket permissions: %w", err) + } + + return listener, nil +} + +// cleanupUnixSocket removes the socket file at address. Missing files are not +// an error since cleanup may run after a partial startup. +func cleanupUnixSocket(address string) { + if err := os.Remove(address); err != nil && !os.IsNotExist(err) { + slog.Warn("failed to remove socket file", "error", err) + } +} + +// socketURL returns the URL form of a Unix-socket address for the discovery +// file. Non-Windows platforms only ever produce unix:// URLs. +func socketURL(address string) string { + return "unix://" + address +} diff --git a/pkg/api/socket_windows.go b/pkg/api/socket_windows.go new file mode 100644 index 0000000000..fc3068bd65 --- /dev/null +++ b/pkg/api/socket_windows.go @@ -0,0 +1,87 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package api + +import ( + "fmt" + "log/slog" + "net" + "os" + "path/filepath" + "strings" + + "github.com/Microsoft/go-winio" +) + +// namedPipeBufferSize is the size of the input/output buffers winio allocates +// per pipe instance. 64 KiB matches what go-winio uses in similar consumers +// (Docker, containerd, Podman) and is well above any single HTTP header chunk. +const namedPipeBufferSize = 64 * 1024 + +// setupUnixSocket creates either a Windows named-pipe listener (when address +// has the \\.\pipe\ prefix) or an AF_UNIX listener at a filesystem path. +// +// Named pipes are kernel objects rather than files, so the os.Stat / os.Remove +// precheck, os.MkdirAll, and os.Chmod steps are skipped: the pipe namespace +// has no parent directory, and access control is governed by the security +// descriptor on the listener (winio's default restricts access to the +// creating user, which matches the toolhive-studio same-user use case). +// +// AF_UNIX is supported on Windows 10 1803+. The chmod step is dropped on this +// path because POSIX file modes do not apply on Windows. +func setupUnixSocket(address string) (net.Listener, error) { + if strings.HasPrefix(address, namedPipePrefix) { + // MessageMode is left at false (byte stream) explicitly because HTTP + // requires byte-oriented framing. + listener, err := winio.ListenPipe(address, &winio.PipeConfig{ + MessageMode: false, + InputBufferSize: namedPipeBufferSize, + OutputBufferSize: namedPipeBufferSize, + }) + if err != nil { + return nil, fmt.Errorf("failed to create named pipe listener: %w", err) + } + return listener, nil + } + + if _, err := os.Stat(address); err == nil { + if err := os.Remove(address); err != nil { + return nil, fmt.Errorf("failed to remove existing socket: %w", err) + } + } + + if err := os.MkdirAll(filepath.Dir(address), 0750); err != nil { + return nil, fmt.Errorf("failed to create socket directory: %w", err) + } + + listener, err := net.Listen("unix", address) + if err != nil { + return nil, fmt.Errorf("failed to create UNIX socket listener: %w", err) + } + + return listener, nil +} + +// cleanupUnixSocket removes the AF_UNIX socket file at address, or no-ops for +// named pipes (the pipe is destroyed when the listener closes). +func cleanupUnixSocket(address string) { + if strings.HasPrefix(address, namedPipePrefix) { + return + } + if err := os.Remove(address); err != nil && !os.IsNotExist(err) { + slog.Warn("failed to remove socket file", "error", err) + } +} + +// socketURL returns the URL form of a Unix-socket or named-pipe address for +// the discovery file. Named pipes are emitted as npipe:// where +// is everything after the \\.\pipe\ prefix. +func socketURL(address string) string { + if strings.HasPrefix(address, namedPipePrefix) { + return "npipe://" + strings.TrimPrefix(address, namedPipePrefix) + } + return "unix://" + address +} From bdfd53e314218751142769cb3c09053834207c7c Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Wed, 6 May 2026 12:58:31 +0200 Subject: [PATCH 3/5] Test npipe URL parsing and discovery health TestParseNamedPipeURL covers the success case plus every rejection branch (missing scheme, wrong scheme, empty name, forward slash, backslash, ".."). A non-Windows guard test asserts CheckHealth("npipe://...") surfaces a clear "Windows" error rather than a misleading dial syscall failure. The Windows-only health_windows_test.go spins up a winio listener with an http.Server.Serve goroutine and asserts CheckHealth("npipe://", expectedNonce) succeeds with a matching nonce, plus a not-found case that expects "health check failed". An atomic counter disambiguates parallel pipe names, since the Windows pipe namespace is global. --- pkg/server/discovery/health_test.go | 44 +++++++++++++++++ pkg/server/discovery/health_windows_test.go | 53 +++++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 pkg/server/discovery/health_windows_test.go diff --git a/pkg/server/discovery/health_test.go b/pkg/server/discovery/health_test.go index 4a18169103..d45c64cb3a 100644 --- a/pkg/server/discovery/health_test.go +++ b/pkg/server/discovery/health_test.go @@ -10,6 +10,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -44,6 +45,49 @@ func TestParseUnixSocketPath_Empty(t *testing.T) { assert.Contains(t, err.Error(), "empty") } +func TestParseNamedPipeURL(t *testing.T) { + t.Parallel() + tests := []struct { + name string + raw string + expect string + wantErr bool + errSubstr string + }{ + {name: "valid", raw: "npipe://thv-api", expect: `\\.\pipe\thv-api`}, + {name: "valid with hyphen and digits", raw: "npipe://thv-api-1", expect: `\\.\pipe\thv-api-1`}, + {name: "missing scheme", raw: "thv-api", wantErr: true, errSubstr: "must start with npipe://"}, + {name: "wrong scheme", raw: "unix://thv-api", wantErr: true, errSubstr: "must start with npipe://"}, + {name: "empty name", raw: "npipe://", wantErr: true, errSubstr: "empty"}, + {name: "forward slash", raw: "npipe://thv/api", wantErr: true, errSubstr: "path separators"}, + {name: "backslash", raw: `npipe://thv\api`, wantErr: true, errSubstr: "path separators"}, + {name: "dot dot", raw: "npipe://..thv", wantErr: true, errSubstr: "'..'"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := ParseNamedPipeURL(tt.raw) + if tt.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errSubstr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.expect, got) + }) + } +} + +func TestCheckHealth_NamedPipe_Unsupported_OnNonWindows(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("non-Windows guard test") + } + t.Parallel() + err := CheckHealth(context.Background(), "npipe://thv-api", "") + require.Error(t, err) + assert.Contains(t, err.Error(), "Windows") +} + func TestCheckHealth_TCP_Success(t *testing.T) { t.Parallel() expectedNonce := "test-nonce-123" diff --git a/pkg/server/discovery/health_windows_test.go b/pkg/server/discovery/health_windows_test.go new file mode 100644 index 0000000000..bc7c1edc5b --- /dev/null +++ b/pkg/server/discovery/health_windows_test.go @@ -0,0 +1,53 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package discovery + +import ( + "context" + "fmt" + "net/http" + "sync/atomic" + "testing" + + "github.com/Microsoft/go-winio" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// pipeNameSeq disambiguates concurrent test pipes so parallel runs don't +// collide on the global pipe namespace. +var pipeNameSeq atomic.Uint64 + +func TestCheckHealth_NamedPipe_Success(t *testing.T) { + t.Parallel() + + pipeName := fmt.Sprintf("thv-test-%d", pipeNameSeq.Add(1)) + pipePath := `\\.\pipe\` + pipeName + + listener, err := winio.ListenPipe(pipePath, &winio.PipeConfig{}) + require.NoError(t, err) + t.Cleanup(func() { _ = listener.Close() }) + + expectedNonce := "pipe-nonce" + mux := http.NewServeMux() + mux.HandleFunc("/health", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set(NonceHeader, expectedNonce) + w.WriteHeader(http.StatusNoContent) + }) + srv := &http.Server{Handler: mux} //nolint:gosec // test server, ReadHeaderTimeout not relevant + go func() { _ = srv.Serve(listener) }() + t.Cleanup(func() { _ = srv.Close() }) + + err = CheckHealth(context.Background(), "npipe://"+pipeName, expectedNonce) + require.NoError(t, err) +} + +func TestCheckHealth_NamedPipe_NotFound(t *testing.T) { + t.Parallel() + err := CheckHealth(context.Background(), "npipe://nonexistent-pipe-thv-test", "") + require.Error(t, err) + assert.Contains(t, err.Error(), "health check failed") +} From 261fcce903ede8f4ae0b6e65dbe1d9fca4fd1840 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Wed, 6 May 2026 12:58:31 +0200 Subject: [PATCH 4/5] Test API listener split and pipe URL forms Cover the per-platform refactor of setupUnixSocket. On all platforms, assert socketURL emits unix:// for filesystem addresses and that isNamedPipeAddress detects \.\pipe\ prefixes. On Windows, additionally assert socketURL emits npipe://, that setupUnixSocket against a unique pipe path returns a winio listener that accepts a winio dial within a 2 s timeout (proving the listener is wired to the named-pipe namespace, not AF_UNIX), and that cleanupUnixSocket no-ops for pipe addresses without panicking. Tests are split via build tags so the Windows test file is only compiled on GOOS=windows and pulls in winio there. --- pkg/api/socket_other_test.go | 36 ++++++++++++++ pkg/api/socket_windows_test.go | 89 ++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+) create mode 100644 pkg/api/socket_other_test.go create mode 100644 pkg/api/socket_windows_test.go diff --git a/pkg/api/socket_other_test.go b/pkg/api/socket_other_test.go new file mode 100644 index 0000000000..5341f73fa5 --- /dev/null +++ b/pkg/api/socket_other_test.go @@ -0,0 +1,36 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package api + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSocketURL_Unix(t *testing.T) { + t.Parallel() + assert.Equal(t, "unix:///tmp/test.sock", socketURL("/tmp/test.sock")) +} + +func TestIsNamedPipeAddress(t *testing.T) { + t.Parallel() + tests := []struct { + name string + address string + want bool + }{ + {"plain socket", "/tmp/thv.sock", false}, + {"named pipe", `\\.\pipe\thv-api`, true}, + {"empty", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, isNamedPipeAddress(tt.address)) + }) + } +} diff --git a/pkg/api/socket_windows_test.go b/pkg/api/socket_windows_test.go new file mode 100644 index 0000000000..f6caa5b4f4 --- /dev/null +++ b/pkg/api/socket_windows_test.go @@ -0,0 +1,89 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package api + +import ( + "context" + "fmt" + "sync/atomic" + "testing" + "time" + + "github.com/Microsoft/go-winio" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// pipeNameSeq disambiguates concurrent test pipes so parallel runs don't +// collide on the global Windows pipe namespace. +var pipeNameSeq atomic.Uint64 + +func uniqueTestPipe() string { + return fmt.Sprintf(`\\.\pipe\thv-api-test-%d`, pipeNameSeq.Add(1)) +} + +func TestSocketURL_Windows(t *testing.T) { + t.Parallel() + tests := []struct { + name string + address string + want string + }{ + {"named pipe", `\\.\pipe\thv-api`, "npipe://thv-api"}, + {"af_unix windows path", `C:\path\thv.sock`, `unix://C:\path\thv.sock`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, socketURL(tt.address)) + }) + } +} + +func TestSetupUnixSocket_NamedPipe(t *testing.T) { + t.Parallel() + pipePath := uniqueTestPipe() + + listener, err := setupUnixSocket(pipePath) + require.NoError(t, err) + t.Cleanup(func() { _ = listener.Close() }) + + // The listener should accept a winio dial within a short timeout, proving + // it is wired to the named-pipe namespace and not to AF_UNIX. + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + connCh := make(chan error, 1) + go func() { + conn, dialErr := winio.DialPipeContext(ctx, pipePath) + if conn != nil { + _ = conn.Close() + } + connCh <- dialErr + }() + + go func() { + conn, _ := listener.Accept() + if conn != nil { + _ = conn.Close() + } + }() + + select { + case err := <-connCh: + require.NoError(t, err) + case <-ctx.Done(): + t.Fatal("dial against named-pipe listener timed out") + } +} + +func TestCleanupUnixSocket_NamedPipe_NoOp(t *testing.T) { + t.Parallel() + // Passing a pipe address to cleanup must not error or panic. There is no + // file to remove; the assertion here is simply that the call returns + // cleanly. + cleanupUnixSocket(`\\.\pipe\thv-api-cleanup-noop`) +} From 785ddc343b345c934a4bfaa6dc9ab7d636612bcf Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 7 May 2026 14:27:30 +0200 Subject: [PATCH 5/5] Tighten Windows named-pipe handling per review Address aponcedeleonch's review on PR #5201: - Update --socket help to mention named pipes and regen CLI docs. - Promote namedPipePrefix to discovery.NamedPipePrefix as the canonical definition; pkg/api re-aliases it locally so listener and dialer cannot drift. - Make isNamedPipeAddress case-insensitive via strings.EqualFold; the Windows pipe namespace is case-insensitive at the kernel layer, so \\.\Pipe\foo must not silently fall through to AF_UNIX. - Collapse stat-then-remove into a single os.Remove that tolerates fs.ErrNotExist on both POSIX and the Windows AF_UNIX fallback. - Close the listener and remove the socket file when Chmod fails, rather than leaking a bound AF_UNIX listener. - Replace stdruntime.GOOS with a per-platform supportsNamedPipe() helper, dropping the runtime-package alias and its collision with pkg/container/runtime. - Rename socket_other.{go,_test.go} to socket_unix.{go,_test.go} to match the client_unix/client_windows convention used by pkg/container/docker/sdk. - Add TestCreateListener_NamedPipe_Unsupported to round out the listener-side reject coverage on non-Windows builds. Co-authored-by: Cursor --- cmd/thv/app/server.go | 5 ++-- docs/cli/thv_serve.md | 2 +- pkg/api/server.go | 20 +++++++++------- pkg/api/{socket_other.go => socket_unix.go} | 20 ++++++++++++---- ...cket_other_test.go => socket_unix_test.go} | 12 ++++++++++ pkg/api/socket_windows.go | 24 +++++++++++-------- pkg/server/discovery/health.go | 10 ++++---- 7 files changed, 63 insertions(+), 30 deletions(-) rename pkg/api/{socket_other.go => socket_unix.go} (63%) rename pkg/api/{socket_other_test.go => socket_unix_test.go} (57%) diff --git a/cmd/thv/app/server.go b/cmd/thv/app/server.go index 75bb56da81..f01cbd10fd 100644 --- a/cmd/thv/app/server.go +++ b/cmd/thv/app/server.go @@ -192,8 +192,9 @@ func init() { serveCmd.Flags().IntVar(&port, "port", 8080, "Port to bind the server to") serveCmd.Flags().BoolVar(&enableDocs, "openapi", false, "Enable OpenAPI documentation endpoints (/api/openapi.json and /api/doc)") - serveCmd.Flags().StringVar(&socketPath, "socket", "", "UNIX socket path to bind the "+ - "server to (overrides host and port if provided)") + serveCmd.Flags().StringVar(&socketPath, "socket", "", + `UNIX socket path or, on Windows, a named pipe (\\.\pipe\) to bind the `+ + "server to (overrides host and port if provided)") // Add experimental MCP server flags serveCmd.Flags().BoolVar(&enableMCPServer, "experimental-mcp", false, diff --git a/docs/cli/thv_serve.md b/docs/cli/thv_serve.md index c455142639..e7a0c95d87 100644 --- a/docs/cli/thv_serve.md +++ b/docs/cli/thv_serve.md @@ -41,7 +41,7 @@ thv serve [flags] --sentry-dsn string Sentry DSN for error tracking and distributed tracing (falls back to SENTRY_DSN env var) --sentry-environment string Sentry environment name, e.g. production or development (falls back to SENTRY_ENVIRONMENT env var) --sentry-traces-sample-rate float Sentry traces sample rate (0.0-1.0) for performance monitoring (default 1) - --socket string UNIX socket path to bind the server to (overrides host and port if provided) + --socket string UNIX socket path or, on Windows, a named pipe (\\.\pipe\) to bind the server to (overrides host and port if provided) ``` ### Options inherited from parent commands diff --git a/pkg/api/server.go b/pkg/api/server.go index 07d8f1cc48..31d873ac07 100644 --- a/pkg/api/server.go +++ b/pkg/api/server.go @@ -27,7 +27,6 @@ import ( "net/http" "os" "path/filepath" - stdruntime "runtime" "strings" "time" @@ -367,16 +366,21 @@ func (b *ServerBuilder) setupDefaultRoutes(r *chi.Mux) { } } -// namedPipePrefix is the Windows named-pipe namespace prefix. Both per-platform -// socket files use it, and ListenURL/createListener inspect the address prefix -// to choose between AF_UNIX and named-pipe semantics. -const namedPipePrefix = `\\.\pipe\` +// namedPipePrefix is the Windows named-pipe namespace prefix. The canonical +// definition lives in pkg/server/discovery so the listener and dialer cannot +// drift; pkg/api re-aliases it here so per-platform socket files do not need +// to import discovery directly. +const namedPipePrefix = discovery.NamedPipePrefix // isNamedPipeAddress reports whether address is a Windows named-pipe path. // The check is platform-agnostic so callers on non-Windows can fail fast with -// a clear error before reaching the listener code. +// a clear error before reaching the listener code. The comparison is +// case-insensitive because the Windows pipe namespace is case-insensitive at +// the kernel layer; without EqualFold an address like \\.\Pipe\foo would +// silently fall through to AF_UNIX and then fail to bind. func isNamedPipeAddress(address string) bool { - return strings.HasPrefix(address, namedPipePrefix) + return len(address) >= len(namedPipePrefix) && + strings.EqualFold(address[:len(namedPipePrefix)], namedPipePrefix) } func setupTCPListener(address string) (net.Listener, error) { @@ -709,7 +713,7 @@ func createListener(address string, isUnixSocket bool) (net.Listener, string, er addrType := "UNIX socket" if isNamedPipeAddress(address) { - if stdruntime.GOOS != "windows" { + if !supportsNamedPipe() { return nil, "", fmt.Errorf("named pipe addresses are only supported on Windows: %s", address) } addrType = "Windows named pipe" diff --git a/pkg/api/socket_other.go b/pkg/api/socket_unix.go similarity index 63% rename from pkg/api/socket_other.go rename to pkg/api/socket_unix.go index e34169298a..3bd6403be4 100644 --- a/pkg/api/socket_other.go +++ b/pkg/api/socket_unix.go @@ -6,21 +6,26 @@ package api import ( + "errors" "fmt" + "io/fs" "log/slog" "net" "os" "path/filepath" ) +// supportsNamedPipe reports whether the current build target can host a +// Windows named-pipe listener. Used by createListener to reject pipe addresses +// before reaching the per-platform setupUnixSocket implementation. +func supportsNamedPipe() bool { return false } + // setupUnixSocket creates a UNIX domain socket listener at the given path. // On non-Windows platforms named-pipe addresses are not supported; callers // guard against that in createListener. func setupUnixSocket(address string) (net.Listener, error) { - if _, err := os.Stat(address); err == nil { - if err := os.Remove(address); err != nil { - return nil, fmt.Errorf("failed to remove existing socket: %w", err) - } + if err := os.Remove(address); err != nil && !errors.Is(err, fs.ErrNotExist) { + return nil, fmt.Errorf("failed to remove existing socket: %w", err) } if err := os.MkdirAll(filepath.Dir(address), 0750); err != nil { @@ -33,6 +38,11 @@ func setupUnixSocket(address string) (net.Listener, error) { } if err := os.Chmod(address, socketPermissions); err != nil { + // Roll back the bound listener and the socket file rather than leaking + // either: the listener owns the AF_UNIX file and net.Listen will not + // rebind the same path while the file exists. + _ = listener.Close() + _ = os.Remove(address) return nil, fmt.Errorf("failed to set socket permissions: %w", err) } @@ -42,7 +52,7 @@ func setupUnixSocket(address string) (net.Listener, error) { // cleanupUnixSocket removes the socket file at address. Missing files are not // an error since cleanup may run after a partial startup. func cleanupUnixSocket(address string) { - if err := os.Remove(address); err != nil && !os.IsNotExist(err) { + if err := os.Remove(address); err != nil && !errors.Is(err, fs.ErrNotExist) { slog.Warn("failed to remove socket file", "error", err) } } diff --git a/pkg/api/socket_other_test.go b/pkg/api/socket_unix_test.go similarity index 57% rename from pkg/api/socket_other_test.go rename to pkg/api/socket_unix_test.go index 5341f73fa5..27ca7dc691 100644 --- a/pkg/api/socket_other_test.go +++ b/pkg/api/socket_unix_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestSocketURL_Unix(t *testing.T) { @@ -25,6 +26,7 @@ func TestIsNamedPipeAddress(t *testing.T) { }{ {"plain socket", "/tmp/thv.sock", false}, {"named pipe", `\\.\pipe\thv-api`, true}, + {"named pipe mixed case", `\\.\Pipe\thv-api`, true}, {"empty", "", false}, } for _, tt := range tests { @@ -34,3 +36,13 @@ func TestIsNamedPipeAddress(t *testing.T) { }) } } + +// TestCreateListener_NamedPipe_Unsupported asserts that createListener rejects +// pipe addresses on non-Windows up front, mirroring the dialer-side guard +// covered by TestCheckHealth_NamedPipe_Unsupported_OnNonWindows. +func TestCreateListener_NamedPipe_Unsupported(t *testing.T) { + t.Parallel() + _, _, err := createListener(`\\.\pipe\thv-api`, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "only supported on Windows") +} diff --git a/pkg/api/socket_windows.go b/pkg/api/socket_windows.go index fc3068bd65..494a82a550 100644 --- a/pkg/api/socket_windows.go +++ b/pkg/api/socket_windows.go @@ -6,12 +6,13 @@ package api import ( + "errors" "fmt" + "io/fs" "log/slog" "net" "os" "path/filepath" - "strings" "github.com/Microsoft/go-winio" ) @@ -21,6 +22,11 @@ import ( // (Docker, containerd, Podman) and is well above any single HTTP header chunk. const namedPipeBufferSize = 64 * 1024 +// supportsNamedPipe reports whether the current build target can host a +// Windows named-pipe listener. Used by createListener to choose between the +// pipe and AF_UNIX paths without dragging the runtime package into server.go. +func supportsNamedPipe() bool { return true } + // setupUnixSocket creates either a Windows named-pipe listener (when address // has the \\.\pipe\ prefix) or an AF_UNIX listener at a filesystem path. // @@ -33,7 +39,7 @@ const namedPipeBufferSize = 64 * 1024 // AF_UNIX is supported on Windows 10 1803+. The chmod step is dropped on this // path because POSIX file modes do not apply on Windows. func setupUnixSocket(address string) (net.Listener, error) { - if strings.HasPrefix(address, namedPipePrefix) { + if isNamedPipeAddress(address) { // MessageMode is left at false (byte stream) explicitly because HTTP // requires byte-oriented framing. listener, err := winio.ListenPipe(address, &winio.PipeConfig{ @@ -47,10 +53,8 @@ func setupUnixSocket(address string) (net.Listener, error) { return listener, nil } - if _, err := os.Stat(address); err == nil { - if err := os.Remove(address); err != nil { - return nil, fmt.Errorf("failed to remove existing socket: %w", err) - } + if err := os.Remove(address); err != nil && !errors.Is(err, fs.ErrNotExist) { + return nil, fmt.Errorf("failed to remove existing socket: %w", err) } if err := os.MkdirAll(filepath.Dir(address), 0750); err != nil { @@ -68,10 +72,10 @@ func setupUnixSocket(address string) (net.Listener, error) { // cleanupUnixSocket removes the AF_UNIX socket file at address, or no-ops for // named pipes (the pipe is destroyed when the listener closes). func cleanupUnixSocket(address string) { - if strings.HasPrefix(address, namedPipePrefix) { + if isNamedPipeAddress(address) { return } - if err := os.Remove(address); err != nil && !os.IsNotExist(err) { + if err := os.Remove(address); err != nil && !errors.Is(err, fs.ErrNotExist) { slog.Warn("failed to remove socket file", "error", err) } } @@ -80,8 +84,8 @@ func cleanupUnixSocket(address string) { // the discovery file. Named pipes are emitted as npipe:// where // is everything after the \\.\pipe\ prefix. func socketURL(address string) string { - if strings.HasPrefix(address, namedPipePrefix) { - return "npipe://" + strings.TrimPrefix(address, namedPipePrefix) + if isNamedPipeAddress(address) { + return "npipe://" + address[len(namedPipePrefix):] } return "unix://" + address } diff --git a/pkg/server/discovery/health.go b/pkg/server/discovery/health.go index abaeb4fb86..6f7d24c185 100644 --- a/pkg/server/discovery/health.go +++ b/pkg/server/discovery/health.go @@ -23,9 +23,11 @@ const ( NonceHeader = "X-Toolhive-Nonce" ) -// namedPipePrefix is the Windows named-pipe namespace prefix used to -// reconstruct addresses for winio.DialPipeContext. -const namedPipePrefix = `\\.\pipe\` +// NamedPipePrefix is the Windows named-pipe namespace prefix. The discovery +// dialer uses it to reconstruct addresses for winio.DialPipeContext, and the +// API listener (pkg/api) imports it as the canonical definition so the two +// sides cannot drift. +const NamedPipePrefix = `\\.\pipe\` // CheckHealth verifies that a server at the given URL is healthy and optionally // matches the expected nonce. It supports http://, unix://, and npipe:// URL @@ -184,5 +186,5 @@ func ParseNamedPipeURL(rawURL string) (string, error) { if strings.Contains(name, "..") { return "", fmt.Errorf("named pipe name must not contain '..': %s", name) } - return namedPipePrefix + name, nil + return NamedPipePrefix + name, nil }