fix(oauth): reap browser launcher and keep native callback on loopback

SamMorrowDrums · Copilot · SamMorrowDrums · commit f707dba0fd5d · 2026-06-16T11:03:24.000+02:00
Address code review:
- openBrowser: reap the launcher process asynchronously so it does not
  linger as a zombie for the lifetime of the server.
- listenCallback: take an explicit bindAll flag and bind to all interfaces
  only inside a container (where the published port arrives via eth0).
  A native run, even with a fixed callback port, now stays on 127.0.0.1
  instead of 0.0.0.0.

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;
diff --git a/internal/oauth/callback.go b/internal/oauth/callback.go
@@ -36,14 +36,15 @@ type callbackServer struct {
 
 // listenCallback binds the local callback listener.
 //
-// A random port (port == 0) binds to 127.0.0.1 only: the redirect target is
-// loopback and never reachable off-host. A fixed port binds to all interfaces
-// because Docker's published-port DNAT delivers traffic to the container's eth0
-// rather than to loopback; exposure is still constrained by the host-side
-// publish (e.g. -p 127.0.0.1:8085:8085).
-func listenCallback(port int) (net.Listener, error) {
+// It binds to loopback (127.0.0.1) by default so the callback server is never
+// exposed on other interfaces. bindAll is set only inside a container, where
+// Docker's published-port DNAT delivers traffic to the container's eth0 rather
+// than to loopback; host-side exposure is still constrained by the publish
+// (e.g. -p 127.0.0.1:8085:8085). A native run — even with a fixed port — stays
+// on loopback.
+func listenCallback(port int, bindAll bool) (net.Listener, error) {
 	host := "127.0.0.1"
-	if port > 0 {
+	if bindAll {
 		host = "0.0.0.0"
 	}
 	addr := fmt.Sprintf("%s:%d", host, port)
diff --git a/internal/oauth/callback_test.go b/internal/oauth/callback_test.go
@@ -71,12 +71,22 @@ func TestCallbackHandlerEscapesError(t *testing.T) {
 }
 
 func TestListenCallbackRandomPortIsLoopback(t *testing.T) {
-	listener, err := listenCallback(0)
+	listener, err := listenCallback(0, false)
 	require.NoError(t, err)
 	defer listener.Close()
 
 	addr, ok := listener.Addr().(*net.TCPAddr)
 	require.True(t, ok)
-	assert.True(t, addr.IP.IsLoopback(), "random port must bind loopback only, got %s", addr.IP)
+	assert.True(t, addr.IP.IsLoopback(), "default bind must be loopback only, got %s", addr.IP)
 	assert.NotZero(t, addr.Port)
 }
+
+func TestListenCallbackBindAllForContainer(t *testing.T) {
+	listener, err := listenCallback(0, true)
+	require.NoError(t, err)
+	defer listener.Close()
+
+	addr, ok := listener.Addr().(*net.TCPAddr)
+	require.True(t, ok)
+	assert.True(t, addr.IP.IsUnspecified(), "bindAll must bind all interfaces, got %s", addr.IP)
+}
diff --git a/internal/oauth/env.go b/internal/oauth/env.go
@@ -31,7 +31,14 @@ func openBrowser(url string) error {
 
 	cmd.Stdout = io.Discard
 	cmd.Stderr = io.Discard
-	return cmd.Start()
+	if err := cmd.Start(); err != nil {
+		return err
+	}
+	// The launcher (xdg-open/open/rundll32) exits as soon as it hands off to the
+	// browser. Reap it asynchronously so it does not linger as a zombie for the
+	// lifetime of this long-running server.
+	go func() { _ = cmd.Wait() }()
+	return nil
 }
 
 // isRunningInDocker reports whether the process is running inside a Docker (or
diff --git a/internal/oauth/flow.go b/internal/oauth/flow.go
@@ -54,7 +54,9 @@ func (m *Manager) beginPKCE(prompter Prompter) (*flowPlan, error) {
 	}
 	verifier := oauth2.GenerateVerifier()
 
-	listener, err := listenCallback(m.config.CallbackPort)
+	// Bind to all interfaces only inside a container, where the published port
+	// is delivered via eth0 rather than loopback. Native runs stay on loopback.
+	listener, err := listenCallback(m.config.CallbackPort, m.inDocker())
 	if err != nil {
 		return nil, err
 	}

Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,9 @@ func (m Manager) beginPKCE(prompter Prompter) (flowPlan, error) {`
`54`	`54`	`}`
`55`	`55`	`verifier := oauth2.GenerateVerifier()`
`56`	`56`
`57`		`- listener, err := listenCallback(m.config.CallbackPort)`
	`57`	`+ // Bind to all interfaces only inside a container, where the published port`
	`58`	`+ // is delivered via eth0 rather than loopback. Native runs stay on loopback.`
	`59`	`+ listener, err := listenCallback(m.config.CallbackPort, m.inDocker())`
`58`	`60`	`if err != nil {`
`59`	`61`	`return nil, err`
`60`	`62`	`}`