fix(oauth): surface refresh failures, bound refresh, prefer device fl…

…ow when headless Addresses pre-merge review of the OAuth stdio core: - Log a one-time warning when token refresh fails instead of silently returning an empty access token, so a forced re-login isn't a surprise. - Bound each background token refresh with a 30s HTTP client timeout so a stalled GitHub token endpoint can't block tool calls indefinitely. - On a headless host (no display server) with a random callback port, fall back to the device-code flow — the only channel reachable from a browser on another machine — instead of dead-ending on an unreachable localhost redirect. A generic browser-open failure still offers the manual URL. - Mark the callback bind failure with a sentinel so the fixed-port-busy fatal path can't misreport an unrelated error as a port conflict. - Export NormalizeHost so callers can recognize the default github.com host (consumed by the build-time baked-in credential guard). 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 64afe523f1302c7b13c7a076e7f9171b7cebb323
@@ -1,6 +1,7 @@
 package oauth
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -9,6 +10,12 @@ import (
 	"strings"
 )
 
+// errNoDisplay reports that the host has no display server, so no browser can be
+// launched. It is a definitive headless signal (unlike a generic launch error),
+// which lets the flow prefer device authorization — the only channel reachable
+// from a browser on another machine (e.g. a remote SSH session).
+var errNoDisplay = errors.New("no display server detected")
+
 // openBrowser tries to open url in the user's default browser. It returns an
 // error when no browser can plausibly be launched so the caller can fall back
 // to elicitation. On Linux it treats a headless session (no display server) as
@@ -18,7 +25,7 @@ func openBrowser(url string) error {
 	switch runtime.GOOS {
 	case "linux":
 		if os.Getenv("DISPLAY") == "" && os.Getenv("WAYLAND_DISPLAY") == "" {
-			return fmt.Errorf("no display server detected")
+			return errNoDisplay
 		}
 		cmd = exec.Command("xdg-open", url)
 	case "darwin":

@@ -2,6 +2,7 @@ package oauth
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -12,6 +13,11 @@ import (
 // preparing the device flow (before any waiting on the user).
 const deviceAuthTimeout = 30 * time.Second
 
+// errCallbackBind marks a failure to bind the local OAuth callback listener, so
+// begin can treat a busy fixed port as fatal without mislabeling unrelated
+// errors (e.g. a failure to generate the state parameter) as a port conflict.
+var errCallbackBind = errors.New("OAuth callback listener could not bind")
+
 // flowPlan is a prepared authorization flow ready to run in the background.
 type flowPlan struct {
 	// run performs the blocking part of the flow (await callback + exchange, or
@@ -41,8 +47,9 @@ func (m *Manager) begin(prompter Prompter) (*flowPlan, error) {
 		// The port was chosen deliberately (and registered with the OAuth app), so
 		// a bind failure means another process holds it — possibly one positioned
 		// to intercept the authorization redirect. Silently switching to device
-		// flow would mask that, so stop and make the user resolve it.
-		if m.config.CallbackPort != 0 {
+		// flow would mask that, so stop and make the user resolve it. Only genuine
+		// bind failures qualify; other errors fall through to device flow.
+		if m.config.CallbackPort != 0 && errors.Is(err, errCallbackBind) {
 			return nil, fmt.Errorf("OAuth callback port %d is not available; another process may be using it — free the port or set a different --oauth-callback-port: %w", m.config.CallbackPort, err)
 		}
 		m.logger.Info("PKCE flow unavailable, falling back to device flow", "reason", err)
@@ -53,8 +60,10 @@ func (m *Manager) begin(prompter Prompter) (*flowPlan, error) {
 }
 
 // beginPKCE prepares the authorization-code + PKCE flow. It binds the callback
-// server and selects the most secure available display channel:
-// browser auto-open, then URL elicitation, then a tool-response message.
+// server and selects the most secure available display channel: browser
+// auto-open, then URL elicitation, then a tool-response message. On a headless
+// host with a random callback port it diverts to device flow, whose redirect
+// does not depend on reaching this machine's localhost.
 func (m *Manager) beginPKCE(prompter Prompter) (*flowPlan, error) {
 	state, err := randomState()
 	if err != nil {
@@ -66,7 +75,7 @@ func (m *Manager) beginPKCE(prompter Prompter) (*flowPlan, error) {
 	// 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
+		return nil, fmt.Errorf("%w: %w", errCallbackBind, err)
 	}
 	if m.inDocker() {
 		// Inside a container the callback binds all interfaces so the published
@@ -92,11 +101,27 @@ func (m *Manager) beginPKCE(prompter Prompter) (*flowPlan, error) {
 		return tok, nil
 	}
 
-	if browserErr := m.openURL(authURL); browserErr != nil {
-		m.logger.Debug("browser auto-open unavailable", "reason", browserErr)
-	} else {
+	browserErr := m.openURL(authURL)
+	switch {
+	case browserErr == nil:
 		m.logger.Info("opened browser for GitHub authorization")
 		return &flowPlan{run: run}, nil
+	case errors.Is(browserErr, errNoDisplay) && m.config.CallbackPort == 0:
+		// Headless host with a random callback port: every PKCE channel ends in a
+		// redirect to this machine's localhost, which a browser on another machine
+		// (e.g. a remote SSH client) cannot reach — so even URL elicitation would
+		// dead-end. Device flow is the only channel reachable from elsewhere, so
+		// prefer it when the app supports it; otherwise fall through to the manual
+		// authorization URL below for a same-machine browser.
+		plan, deviceErr := m.beginDevice(prompter)
+		if deviceErr == nil {
+			cs.close()
+			m.logger.Info("no display server; using device flow")
+			return plan, nil
+		}
+		m.logger.Debug("device flow unavailable on headless host; offering manual authorization URL", "reason", deviceErr)
+	default:
+		m.logger.Debug("browser auto-open unavailable", "reason", browserErr)
 	}
 
 	if canPromptURL(prompter) {

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"log/slog"
+	"net/http"
 	"os"
 	"sync"
 	"time"
@@ -15,6 +16,10 @@ import (
 // the user to complete the browser or device flow.
 const DefaultAuthTimeout = 5 * time.Minute
 
+// tokenRefreshTimeout bounds each background refresh of an expiring token so a
+// stalled GitHub token endpoint cannot block a tool call indefinitely.
+const tokenRefreshTimeout = 30 * time.Second
+
 // flowStatus tracks the manager's single-flight authorization state.
 type flowStatus int
 
@@ -58,12 +63,13 @@ type Manager struct {
 	openURL  func(string) error
 	inDocker func() bool
 
-	mu      sync.Mutex
-	source  oauth2.TokenSource // refreshing source, set once authorized
-	status  flowStatus
-	pending *UserAction
-	done    chan struct{}
-	lastErr error
+	mu               sync.Mutex
+	source           oauth2.TokenSource // refreshing source, set once authorized
+	status           flowStatus
+	pending          *UserAction
+	done             chan struct{}
+	lastErr          error
+	refreshErrLogged bool // true once a refresh failure has been logged, reset on re-auth
 }
 
 // NewManager builds a Manager for the given configuration. A nil logger logs to
@@ -93,8 +99,24 @@ func (m *Manager) AccessToken() string {
 	if src == nil {
 		return ""
 	}
+	// Refresh (if needed) happens here, off the lock, because ReuseTokenSource may
+	// make a blocking network call and holding m.mu would serialize every tool call.
 	tok, err := src.Token()
-	if err != nil || !tok.Valid() {
+	if err != nil {
+		// A refresh failure (expired GitHub App refresh token, revoked grant, or a
+		// network blip) leaves the session unauthorized and forces a re-login.
+		// Surface it once, otherwise it only manifests as a surprise re-authorization
+		// prompt. The oauth2 error carries the token endpoint's response, not the
+		// access or refresh token.
+		m.mu.Lock()
+		if !m.refreshErrLogged {
+			m.refreshErrLogged = true
+			m.logger.Warn("OAuth token refresh failed; re-authorization required", "error", err)
+		}
+		m.mu.Unlock()
+		return ""
+	}
+	if !tok.Valid() {
 		return ""
 	}
 	return tok.AccessToken
@@ -201,8 +223,11 @@ func (m *Manager) complete(tok *oauth2.Token, err error) {
 		m.lastErr = nil
 		// Config.TokenSource returns a ReuseTokenSource that refreshes expired
 		// tokens using the refresh token — this is what makes GitHub App
-		// (expiring) tokens work transparently.
-		m.source = m.refreshConfig.TokenSource(context.Background(), tok)
+		// (expiring) tokens work transparently. The refresh uses a bounded HTTP
+		// client so a stalled token endpoint can't block a tool call forever.
+		refreshCtx := context.WithValue(context.Background(), oauth2.HTTPClient, &http.Client{Timeout: tokenRefreshTimeout})
+		m.source = m.refreshConfig.TokenSource(refreshCtx, tok)
+		m.refreshErrLogged = false
 		m.logger.Info("github authorization complete")
 	}
 	if m.done != nil {

@@ -185,6 +185,29 @@ func TestAuthenticateDevicePollingPending(t *testing.T) {
 	assert.Equal(t, "gho_device_token", m.AccessToken())
 }
 
+func TestAuthenticateHeadlessPrefersDeviceFlow(t *testing.T) {
+	f := newFakeGitHub(t)
+	f.deviceToken = "gho_device_token"
+	m := newManager(t, f)
+	// A headless host (no display server) with a random callback port: a PKCE
+	// redirect to this machine's localhost is unreachable from a browser on
+	// another machine, so device flow must be chosen even though the client can
+	// elicit a URL (which would otherwise win over device flow).
+	m.openURL = func(string) error { return errNoDisplay }
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+
+	out, err := m.Authenticate(ctx, &fakePrompter{urlCapable: true})
+	require.NoError(t, err)
+	assert.Nil(t, out)
+	assert.Equal(t, "gho_device_token", m.AccessToken())
+	grants := f.recordedGrants()
+	assert.Contains(t, grants, "urn:ietf:params:oauth:grant-type:device_code")
+	assert.NotContains(t, grants, "authorization_code",
+		"headless host must skip the unreachable PKCE authorization-code flow")
+}
+
 func TestAuthenticateFixedCallbackPortUnavailableIsFatal(t *testing.T) {
 	f := newFakeGitHub(t)
 	m := newManager(t, f)

@@ -53,18 +53,20 @@ func NewGitHubConfig(clientID, clientSecret string, scopes []string, host string
 // GitHubEndpoint returns the OAuth authorization, token, and device endpoints
 // for a GitHub host. An empty host targets github.com.
 func GitHubEndpoint(host string) oauth2.Endpoint {
-	base := normalizeHost(host)
+	base := NormalizeHost(host)
 	return oauth2.Endpoint{
 		AuthURL:       base + "/login/oauth/authorize",
 		TokenURL:      base + "/login/oauth/access_token",
 		DeviceAuthURL: base + "/login/device/code",
 	}
 }
 
-// normalizeHost turns a user-supplied host into a scheme+host base URL with no
+// NormalizeHost turns a user-supplied host into a scheme+host base URL with no
 // trailing slash. The API subdomain is stripped because OAuth endpoints live on
-// the web host, not the API host (api.github.com -> github.com).
-func normalizeHost(host string) string {
+// the web host, not the API host (api.github.com -> github.com). An empty host
+// yields the github.com default, so callers can also use it to recognize the
+// default host (NormalizeHost(host) == "https://github.com").
+func NormalizeHost(host string) string {
 	host = strings.TrimSpace(host)
 	if host == "" {
 		return "https://github.com"

@@ -25,7 +25,7 @@ func TestNormalizeHost(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			assert.Equal(t, tt.want, normalizeHost(tt.host))
+			assert.Equal(t, tt.want, NormalizeHost(tt.host))
 		})
 	}
 }