Skip to content

Fix port-selection TOCTOU race in e2e tests #6142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jhrozek merged 1 commit into from
Jul 29, 2026
Merged

Fix port-selection TOCTOU race in e2e tests #6142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 80 additions & 46 deletions test/e2e/api_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,31 @@ type Server struct {
stdout *strings.Builder
}

// RetryOnPortConflict runs attempt with port and returns its result. The
// port is only probed free at selection time (networking.FindOrUsePort);
// the real bind happens later, in a subprocess this test doesn't control, so
// another process can steal the port before that bind occurs. If attempt
// fails with an address-already-in-use signal, RetryOnPortConflict
// allocates a fresh port and retries attempt exactly once -- mirroring
// startEraBackendOnPort's handling of the same TOCTOU window for
// Docker-bound ports in test/e2e/vmcp_dual_era_helpers_test.go. It returns
// the port actually used (the original, or the fresh one on retry) along
// with attempt's result.
func RetryOnPortConflict(port int, attempt func(port int) error) (int, error) {
err := attempt(port)
if err == nil || !strings.Contains(err.Error(), "address already in use") {
return port, err
}

newPort, ferr := networking.FindOrUsePort(0)
if ferr != nil {
GinkgoWriter.Printf("failed to find a fresh port after port %d lost a bind race: %v\n", port, ferr)
return port, err
}
GinkgoWriter.Printf("port %d lost a bind race, retrying once with fresh port %d: %v\n", port, newPort, err)
return newPort, attempt(newPort)
}

// NewServer creates and starts a new API server instance by running `thv serve` as a subprocess.
func NewServer(config *ServerConfig) (*Server, error) {
testConfig := NewTestConfig()
Expand Down Expand Up @@ -87,54 +112,63 @@ func NewServer(config *ServerConfig) (*Server, error) {
// would fail because no client config paths exist in the temp home dir.
_ = os.WriteFile(filepath.Join(tempHome, ".claude.json"), []byte("{}"), 0600)

ctx, cancel := context.WithCancel(context.Background())

// Create string builders to capture output
var stdout, stderr strings.Builder

// Create the command: thv serve --host 127.0.0.1 --port <port>
//nolint:gosec // Intentional for e2e testing
cmd := exec.CommandContext(
ctx,
testConfig.THVBinary,
"serve",
"--host",
"127.0.0.1",
"--port",
strconv.Itoa(port),
)
// Set environment variables including temporary config paths
cmd.Env = append([]string{
"TOOLHIVE_DEV=true",
fmt.Sprintf("XDG_CONFIG_HOME=%s", tempXdgConfigHome),
fmt.Sprintf("HOME=%s", tempHome),
}, config.ExtraEnv...)
cmd.Stdout = &stdout
cmd.Stderr = &stderr

// Start the server process
if err := cmd.Start(); err != nil {
cancel()
return nil, fmt.Errorf("failed to start thv serve: %w", err)
}
var server *Server
// The returned port is discarded here: the closure below assigns the
// real port onto server.port via the Server struct literal.
_, err = RetryOnPortConflict(port, func(p int) error {
ctx, cancel := context.WithCancel(context.Background())

// Create string builders to capture output
var stdout, stderr strings.Builder

// Create the command: thv serve --host 127.0.0.1 --port <port>
//nolint:gosec // Intentional for e2e testing
cmd := exec.CommandContext(
ctx,
testConfig.THVBinary,
"serve",
"--host",
"127.0.0.1",
"--port",
strconv.Itoa(p),
)
// Set environment variables including temporary config paths
cmd.Env = append([]string{
"TOOLHIVE_DEV=true",
fmt.Sprintf("XDG_CONFIG_HOME=%s", tempXdgConfigHome),
fmt.Sprintf("HOME=%s", tempHome),
}, config.ExtraEnv...)
cmd.Stdout = &stdout
cmd.Stderr = &stderr

// Start the server process
if err := cmd.Start(); err != nil {
cancel()
return fmt.Errorf("failed to start thv serve: %w", err)
}

server := &Server{
config: config,
baseURL: fmt.Sprintf("http://127.0.0.1:%d", port),
cmd: cmd,
ctx: ctx,
cancel: cancel,
httpClient: &http.Client{
Timeout: config.RequestTimeout,
},
port: port,
stdout: &stdout,
stderr: &stderr,
}
server = &Server{
config: config,
baseURL: fmt.Sprintf("http://127.0.0.1:%d", p),
cmd: cmd,
ctx: ctx,
cancel: cancel,
httpClient: &http.Client{
Timeout: config.RequestTimeout,
},
port: p,
stdout: &stdout,
stderr: &stderr,
}

// Wait for server to be ready
if err := server.WaitForReady(); err != nil {
_ = server.Stop()
// Wait for server to be ready
if err := server.WaitForReady(); err != nil {
_ = server.Stop()
return err
}
return nil
})
if err != nil {
return nil, err
}

Expand Down
196 changes: 97 additions & 99 deletions test/e2e/cli_llm_all_clients_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,12 +254,9 @@ var _ = Describe("thv llm — all-client matrix", Label("cli", "llm", "clients",
binDir, err = e2e.CreateFakeBrowserDir(tempDir)
Expect(err).ToNot(HaveOccurred())

// Allocate a free port for the OIDC mock server.
oidcPort, err = networking.FindOrUsePort(0)
Expect(err).ToNot(HaveOccurred())

oidcServer, err = e2e.NewOIDCMockServer(oidcPort, clientID, clientSecret)
oidcServer, err = e2e.NewOIDCMockServer(0, clientID, clientSecret)
Expect(err).ToNot(HaveOccurred())
oidcPort = oidcServer.Port()
oidcServer.EnableAutoComplete()
Expect(oidcServer.Start()).To(Succeed())

Expand Down Expand Up @@ -541,39 +538,13 @@ var _ = Describe("thv llm — all-client matrix", Label("cli", "llm", "clients",
proxyPort, portErr := networking.FindOrUsePort(0)
Expect(portErr).ToNot(HaveOccurred())

By(fmt.Sprintf("setting proxy port to %d", proxyPort))
thvCmd("llm", "config", "set", "--proxy-port", fmt.Sprintf("%d", proxyPort)).ExpectSuccess()

By("starting the proxy in a goroutine")
type proxyResult struct {
stdout, stderr string
err error
}
done := make(chan proxyResult, 1)
proxyCmd := thvCmd("llm", "proxy", "start")
go func() {
out, serr, rerr := proxyCmd.RunWithTimeout(15 * time.Second)
done <- proxyResult{out, serr, rerr}
}()
DeferCleanup(func() {
_ = proxyCmd.Interrupt()
select {
case <-done:
case <-time.After(5 * time.Second):
}
By(fmt.Sprintf("starting the proxy on port %d", proxyPort))
_, portErr = e2e.RetryOnPortConflict(proxyPort, func(port int) error {
return startLLMProxy(thvCmd, func() *e2e.THVCommand {
return thvCmd("llm", "proxy", "start")
}, port, 15*time.Second)
})

By(fmt.Sprintf("waiting for proxy to listen on port %d", proxyPort))
proxyAddr := fmt.Sprintf("127.0.0.1:%d", proxyPort)
Eventually(func() error {
conn, err := net.DialTimeout("tcp", proxyAddr, 200*time.Millisecond)
if err != nil {
return err
}
_ = conn.Close()
return nil
}, 10*time.Second, 300*time.Millisecond).Should(Succeed(),
"proxy should be listening on %s", proxyAddr)
Expect(portErr).ToNot(HaveOccurred())
})
})

Expand Down Expand Up @@ -630,9 +601,7 @@ var _ = Describe("thv llm — all-client matrix", Label("cli", "llm", "clients",

// Start a local HTTPS mock gateway so the proxy can forward requests
// quickly rather than timing out on DNS resolution for a fake domain.
gwPort, portErr := networking.FindOrUsePort(0)
Expect(portErr).ToNot(HaveOccurred())
gw, gwErr := e2e.NewLLMGatewayMock(gwPort)
gw, gwErr := e2e.NewLLMGatewayMock(0)
Expect(gwErr).ToNot(HaveOccurred())
Expect(gw.Start()).To(Succeed())
defer func() { _ = gw.Stop() }()
Expand Down Expand Up @@ -660,36 +629,17 @@ var _ = Describe("thv llm — all-client matrix", Label("cli", "llm", "clients",
proxyPort, portErr2 := networking.FindOrUsePort(0)
Expect(portErr2).ToNot(HaveOccurred())

By(fmt.Sprintf("setting proxy port to %d and starting proxy", proxyPort))
thvCmd("llm", "config", "set", "--proxy-port", fmt.Sprintf("%d", proxyPort)).ExpectSuccess()

done := make(chan struct{})
proxyCmd := thvCmd("llm", "proxy", "start").WithEnv(
"SSL_CERT_FILE="+gwCertFile,
rebindEnvKey+"="+rebindToken,
)
go func() {
defer close(done)
_, _, _ = proxyCmd.RunWithTimeout(15 * time.Second)
}()
DeferCleanup(func() {
_ = proxyCmd.Interrupt()
select {
case <-done:
case <-time.After(5 * time.Second):
}
By(fmt.Sprintf("starting the proxy on port %d", proxyPort))
proxyPort, portErr2 = e2e.RetryOnPortConflict(proxyPort, func(port int) error {
return startLLMProxy(thvCmd, func() *e2e.THVCommand {
return thvCmd("llm", "proxy", "start").WithEnv(
"SSL_CERT_FILE="+gwCertFile,
rebindEnvKey+"="+rebindToken,
)
}, port, 15*time.Second)
})

Expect(portErr2).ToNot(HaveOccurred())
proxyAddr := fmt.Sprintf("127.0.0.1:%d", proxyPort)
Eventually(func() error {
conn, dialErr := net.DialTimeout("tcp", proxyAddr, 200*time.Millisecond)
if dialErr != nil {
return dialErr
}
_ = conn.Close()
return nil
}, 10*time.Second, 300*time.Millisecond).Should(Succeed(),
"proxy should be listening on %s", proxyAddr)

rebindClient := &http.Client{Timeout: 10 * time.Second}

Expand Down Expand Up @@ -765,16 +715,15 @@ var _ = Describe("thv llm — all-client matrix", Label("cli", "llm", "clients",
Skip(fmt.Sprintf("client %q not supported on %s", clientTC.name, runtime.GOOS))
}

// Allocate ports for the mock gateway and the proxy.
gatewayPort, portErr := networking.FindOrUsePort(0)
Expect(portErr).ToNot(HaveOccurred())
// Allocate a port for the proxy.
proxyPort, portErr := networking.FindOrUsePort(0)
Expect(portErr).ToNot(HaveOccurred())

// Start the mock LLM gateway (HTTPS with self-signed cert).
By(fmt.Sprintf("[%s] starting mock LLM gateway on port %d", clientTC.name, gatewayPort))
gateway, gwErr := e2e.NewLLMGatewayMock(gatewayPort)
gateway, gwErr := e2e.NewLLMGatewayMock(0)
Expect(gwErr).ToNot(HaveOccurred())
gatewayPort := gateway.Port()
By(fmt.Sprintf("[%s] starting mock LLM gateway on port %d", clientTC.name, gatewayPort))
Expect(gateway.Start()).To(Succeed())
defer func() { _ = gateway.Stop() }()

Expand Down Expand Up @@ -822,34 +771,14 @@ var _ = Describe("thv llm — all-client matrix", Label("cli", "llm", "clients",
}

// Configure the proxy port and start it.
By(fmt.Sprintf("[%s] setting proxy port to %d", clientTC.name, proxyPort))
thvCmd("llm", "config", "set", "--proxy-port", fmt.Sprintf("%d", proxyPort)).ExpectSuccess()

By(fmt.Sprintf("[%s] starting the proxy", clientTC.name))
done := make(chan struct{})
proxyCmd := thvCmdWithToken("llm", "proxy", "start")
go func() {
defer close(done)
_, _, _ = proxyCmd.RunWithTimeout(20 * time.Second)
}()
DeferCleanup(func() {
_ = proxyCmd.Interrupt()
select {
case <-done:
case <-time.After(5 * time.Second):
}
By(fmt.Sprintf("[%s] starting the proxy on port %d", clientTC.name, proxyPort))
proxyPort, portErr = e2e.RetryOnPortConflict(proxyPort, func(port int) error {
return startLLMProxy(thvCmd, func() *e2e.THVCommand {
return thvCmdWithToken("llm", "proxy", "start")
}, port, 20*time.Second)
})

Expect(portErr).ToNot(HaveOccurred())
proxyAddr := fmt.Sprintf("127.0.0.1:%d", proxyPort)
Eventually(func() error {
conn, dialErr := net.DialTimeout("tcp", proxyAddr, 200*time.Millisecond)
if dialErr != nil {
return dialErr
}
_ = conn.Close()
return nil
}, 10*time.Second, 300*time.Millisecond).Should(Succeed(),
"proxy should be listening on %s", proxyAddr)

// Send requests through the proxy and verify the gateway received them
// with the correct Bearer token. Use a client with an explicit timeout
Expand Down Expand Up @@ -1275,6 +1204,75 @@ var _ = Describe("thv llm — all-client matrix", Label("cli", "llm", "clients",
// Helpers
// ─────────────────────────────────────────────────────────────────────────────

// llmProxyResult captures a `thv llm proxy start` subprocess's outcome, so
// startLLMProxy can inspect it for an address-already-in-use signal if the
// subprocess exits before its listener comes up.
type llmProxyResult struct {
stdout, stderr string
err error
}

// startLLMProxy sets the proxy port via `thv llm config set --proxy-port`,
// starts `thv llm proxy start` (built by buildProxyCmd, so each call site
// can inject its own env/token wiring) in the background, registers a
// DeferCleanup to interrupt it, and waits for it to accept TCP connections
// on 127.0.0.1:port. The port is only probed free at selection time
// (networking.FindOrUsePort); the real bind happens later in this
// subprocess, so another process can steal it in between. On that race,
// this returns an error containing "address already in use" so callers can
// retry via e2e.RetryOnPortConflict.
func startLLMProxy(
thvCmd func(args ...string) *e2e.THVCommand,
buildProxyCmd func() *e2e.THVCommand,
port int,
runTimeout time.Duration,
) error {
thvCmd("llm", "config", "set", "--proxy-port", fmt.Sprintf("%d", port)).ExpectSuccess()

proxyCmd := buildProxyCmd()
done := make(chan llmProxyResult, 1)
go func() {
out, serr, rerr := proxyCmd.RunWithTimeout(runTimeout)
done <- llmProxyResult{out, serr, rerr}
}()
DeferCleanup(func() {
_ = proxyCmd.Interrupt()
select {
case <-done:
case <-time.After(5 * time.Second):
}
})

addr := fmt.Sprintf("127.0.0.1:%d", port)
deadline := time.After(10 * time.Second)
ticker := time.NewTicker(300 * time.Millisecond)
defer ticker.Stop()
for {
select {
case res := <-done:
return fmt.Errorf("proxy exited before listening on %s: err=%v stderr=%q", addr, res.err, res.stderr)
case <-deadline:
return fmt.Errorf("timed out waiting for proxy to listen on %s", addr)
case <-ticker.C:
conn, dialErr := net.DialTimeout("tcp", addr, 200*time.Millisecond)
if dialErr != nil {
continue
}
_ = conn.Close()
// A successful dial doesn't prove OUR subprocess is the listener -- if it
// lost the bind race, another process already listening on this exact
// port would also accept the connection. Give the subprocess's own exit
// (if any) a moment to land on `done` before trusting the dial.
select {
case res := <-done:
return fmt.Errorf("proxy exited before listening on %s: err=%v stderr=%q", addr, res.err, res.stderr)
case <-time.After(100 * time.Millisecond):
return nil
}
}
}
}

// createFakeBinary writes a minimal no-op shell script named `name` in dir.
// This satisfies the LLMBinaryName check in DetectedLLMGatewayClients.
func createFakeBinary(dir, name string) error {
Expand Down
Loading
Loading