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

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 <223556219+Copilot@users.noreply.github.com>
SamMorrowDrums · Jun 26, 2026 · Jun 16, 2026 · Jun 16, 2026 · Jun 17, 2026 · Jun 18, 2026
commit f707dba0fd5dd7cdbd8963fb67003dcc7fd175d1
@@ -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)

@@ -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)
+}
@@ -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

@@ -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
 	}