fix: surface keyring access failures from ActiveToken

When ActiveToken's keyring lookup fails with a non-ErrNotFound error (typically *keyring.TimeoutError on macOS Keychain), the error was silently discarded and AddAuthTokenHeader.RoundTrip then sent the request without an Authorization header. Users saw a confusing HTTP 401 (REST) or HTTP 403 "API rate limit exceeded for <IP>" (GraphQL) with no signal that the local keyring was the cause. Add ActiveTokenWithError to gh.AuthConfig that surfaces these errors. Refactor ActiveToken as a thin wrapper that calls ActiveTokenWithError and discards the error so existing callers keep their signature. Require the new method on api/http_client.go's tokenGetter interface and update RoundTrip to fail the request with the resolution error rather than send unauthenticated. When the user-scoped keyring lookup fails with a real error and the legacy unkeyed fallback then returns ErrNotFound, preserve the original error rather than letting the fallback's "not found" mask it. Otherwise a timeout or permission failure on the canonical lookup would be reported as if no token were configured. ErrNotFound continues to be the legitimate "no token configured for this host" signal and is not surfaced as an error so genuinely anonymous requests against unconfigured hosts keep working. Fixes #13317
cli · Iron-Ham · Apr 29, 2026 · Apr 30, 2026 · 5213965abd657e35d61ff427d0b474ee30412879
commit 5213965abd657e35d61ff427d0b474ee30412879
@@ -15,6 +15,7 @@ import (
 
 type tokenGetter interface {
 	ActiveToken(string) (string, string)
+	ActiveTokenWithError(string) (string, string, error)
 }
 
 type HTTPClientOptions struct {
@@ -105,6 +106,11 @@ func AddCacheTTLHeader(rt http.RoundTripper, ttl time.Duration) http.RoundTrippe
 }
 
 // AddAuthTokenHeader adds an authentication token header for the host specified by the request.
+//
+// If the cfg's token resolution returns a non-nil error (typically a keyring access failure such as
+// a macOS Keychain timeout), the request is failed with that error rather than sent unauthenticated;
+// silently dropping the auth header would surface as a confusing upstream 401/403 instead of pointing
+// at the local-keyring root cause.
 func AddAuthTokenHeader(rt http.RoundTripper, cfg tokenGetter) http.RoundTripper {
 	return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) {
 		// If the header is already set in the request, don't overwrite it.
@@ -117,7 +123,11 @@ func AddAuthTokenHeader(rt http.RoundTripper, cfg tokenGetter) http.RoundTripper
 			// If the host has changed during a redirect do not add the authentication token header.
 			if !redirectHostnameChange {
 				hostname := ghauth.NormalizeHostname(getHost(req))
-				if token, _ := cfg.ActiveToken(hostname); token != "" {
+				token, _, err := cfg.ActiveTokenWithError(hostname)
+				if err != nil {
+					return nil, err
+				}
+				if token != "" {
 					req.Header.Set(authorization, fmt.Sprintf("token %s", token))
 				}
 			}

@@ -2,6 +2,7 @@ package api
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -395,6 +396,93 @@ func (c tinyConfig) ActiveToken(host string) (string, string) {
 	return c[fmt.Sprintf("%s:%s", host, "oauth_token")], "oauth_token"
 }
 
+func (c tinyConfig) ActiveTokenWithError(host string) (string, string, error) {
+	token, source := c.ActiveToken(host)
+	return token, source, nil
+}
+
+// resolvingTokenConfig is a tokenGetter that lets a test inject both the
+// resolved token and the resolution error. It covers (a) keyring failures
+// surfaced as request-level errors, (b) successful resolution through the
+// error-aware path, and (c) the empty-token-no-error anonymous case.
+type resolvingTokenConfig struct {
+	token string
+	err   error
+}
+
+func (c resolvingTokenConfig) ActiveToken(host string) (string, string) {
+	return c.token, ""
+}
+
+func (c resolvingTokenConfig) ActiveTokenWithError(host string) (string, string, error) {
+	return c.token, "", c.err
+}
+
+// TestAddAuthTokenHeaderResolutionPaths verifies the three outcomes of
+// token resolution at request time: an error fails the request loudly,
+// a resolved token is attached as Authorization, and an empty token with
+// no error proceeds anonymously (preserving support for unauthenticated
+// endpoints like /zen).
+func TestAddAuthTokenHeaderResolutionPaths(t *testing.T) {
+	resolutionErr := errors.New("simulated keyring failure")
+
+	tests := []struct {
+		name            string
+		cfg             resolvingTokenConfig
+		wantErr         error
+		wantInnerCalled bool
+		wantAuthHeader  string
+	}{
+		{
+			name:            "resolution error fails the request",
+			cfg:             resolvingTokenConfig{err: resolutionErr},
+			wantErr:         resolutionErr,
+			wantInnerCalled: false,
+		},
+		{
+			name:            "resolved token is attached as Authorization",
+			cfg:             resolvingTokenConfig{token: "RESOLVED-TOKEN"},
+			wantInnerCalled: true,
+			wantAuthHeader:  "token RESOLVED-TOKEN",
+		},
+		{
+			name:            "empty token with no error proceeds anonymously",
+			cfg:             resolvingTokenConfig{},
+			wantInnerCalled: true,
+			wantAuthHeader:  "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var innerCalled bool
+			var captured *http.Request
+			inner := funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) {
+				innerCalled = true
+				captured = req
+				return &http.Response{StatusCode: http.StatusOK}, nil
+			}}
+
+			rt := AddAuthTokenHeader(inner, tt.cfg)
+
+			req, err := http.NewRequest("GET", "https://api.github.com/", nil)
+			require.NoError(t, err)
+
+			_, err = rt.RoundTrip(req)
+			if tt.wantErr != nil {
+				require.ErrorIs(t, err, tt.wantErr)
+			} else {
+				require.NoError(t, err)
+			}
+			assert.Equal(t, tt.wantInnerCalled, innerCalled)
+			if tt.wantInnerCalled {
+				require.NotNil(t, captured)
+				assert.Equal(t, tt.wantAuthHeader, captured.Header.Get("Authorization"))
+			}
+		})
+	}
+}
+
 var requestAtRE = regexp.MustCompile(`(?m)^\* Request at .+`)
 var dateRE = regexp.MustCompile(`(?m)^< Date: .+`)
 var hostWithPortRE = regexp.MustCompile(`127\.0\.0\.1:\d+`)

@@ -123,6 +123,11 @@ func (c cfg) ActiveToken(hostname string) (string, string) {
 	return c.token, "oauth_token"
 }
 
+func (c cfg) ActiveTokenWithError(hostname string) (string, string, error) {
+	token, source := c.ActiveToken(hostname)
+	return token, source, nil
+}
+
 func getViewer(httpClient *http.Client, hostname, token string) (string, error) {
 	authedClient := *httpClient
 	authedClient.Transport = api.AddAuthTokenHeader(httpClient.Transport, cfg{token: token})

@@ -76,6 +76,190 @@ func TestHasNoActiveToken(t *testing.T) {
 	require.False(t, hasActiveToken, "expected there to be no active token")
 }
 
+func TestActiveTokenWithErrorSurfacesKeyringFailure(t *testing.T) {
+	// Given a host with a configured user whose token lives in the keyring
+	authCfg := newTestAuthConfig(t)
+	_, err := authCfg.Login("github.com", "test-user", "test-token", "", true)
+	require.NoError(t, err)
+
+	// And a keyring that subsequently fails on every access (simulating a
+	// macOS Keychain access timeout or other non-ErrNotFound failure)
+	keyringErr := errors.New("simulated keyring failure")
+	keyring.MockInitWithError(keyringErr)
+
+	// When we resolve the active token via ActiveTokenWithError
+	token, _, err := authCfg.ActiveTokenWithError("github.com")
+
+	// Then the failure is surfaced rather than silently producing an empty
+	// token, so callers can distinguish "no token here" from "keyring is
+	// inaccessible" and avoid sending unauthenticated requests.
+	require.Empty(t, token)
+	require.Error(t, err)
+	require.ErrorIs(t, err, keyringErr)
+	require.ErrorContains(t, err, "github.com")
+}
+
+func TestActiveTokenWithErrorSurfacesUserScopedFailureWhenUnkeyedFallbackIsAbsent(t *testing.T) {
+	// Given a host with a configured user whose user-scoped keyring lookup
+	// fails with a real keyring access error (not ErrNotFound), while the
+	// legacy unkeyed fallback slot is genuinely empty (ErrNotFound).
+	//
+	// This is the regression case for the silent-failure bug: the fallback's
+	// "not found" must not be allowed to mask the user-scoped lookup's real
+	// failure, otherwise AddAuthTokenHeader will proceed unauthenticated.
+	authCfg := newTestAuthConfig(t)
+	_, err := authCfg.Login("github.com", "test-user", "test-token", "", true)
+	require.NoError(t, err)
+
+	keyringErr := errors.New("simulated user-scoped keyring failure")
+	keyring.MockGetOverride(func(_, user string) (string, error) {
+		if user == "" {
+			return "", keyring.ErrNotFound
+		}
+		return "", keyringErr
+	})
+	t.Cleanup(func() { keyring.MockGetOverride(nil) })
+
+	// When we resolve the active token via ActiveTokenWithError
+	token, _, err := authCfg.ActiveTokenWithError("github.com")
+
+	// Then the user-scoped failure is surfaced rather than being masked by
+	// the fallback's ErrNotFound.
+	require.Empty(t, token)
+	require.Error(t, err)
+	require.ErrorIs(t, err, keyringErr)
+	require.ErrorContains(t, err, "github.com")
+}
+
+func TestActiveTokenWithErrorPrefersFallbackErrorWhenBothLookupsFail(t *testing.T) {
+	// Given a host with a configured user where the user-scoped keyring lookup
+	// fails with a real error AND the legacy unkeyed fallback also fails with
+	// a different real error. This locks in the current precedence: when both
+	// lookups produce non-ErrNotFound errors the fallback's error wins, since
+	// the swap at config.go only fires when the fallback returns ErrNotFound.
+	// Either error is "real" enough to surface; this test is characterization:
+	// flipping precedence would silently change which error message users see,
+	// so any future change should be deliberate and update this test.
+	authCfg := newTestAuthConfig(t)
+	_, err := authCfg.Login("github.com", "test-user", "test-token", "", true)
+	require.NoError(t, err)
+
+	userScopedErr := errors.New("simulated user-scoped failure")
+	unkeyedErr := errors.New("simulated unkeyed failure")
+	keyring.MockGetOverride(func(_, user string) (string, error) {
+		if user == "" {
+			return "", unkeyedErr
+		}
+		return "", userScopedErr
+	})
+	t.Cleanup(func() { keyring.MockGetOverride(nil) })
+
+	// When we resolve the active token via ActiveTokenWithError
+	token, _, err := authCfg.ActiveTokenWithError("github.com")
+
+	// Then a real failure is surfaced. The fallback's error is the one that
+	// reaches the wrapper; the user-scoped error is intentionally not
+	// preferred when both are real.
+	require.Empty(t, token)
+	require.Error(t, err)
+	require.ErrorIs(t, err, unkeyedErr)
+	require.NotErrorIs(t, err, userScopedErr)
+	require.ErrorContains(t, err, "github.com")
+}
+
+func TestActiveTokenWithErrorSurfacesUnkeyedFailureWhenUserScopedReturnsNotFound(t *testing.T) {
+	// Given a host with a configured user whose user-scoped keyring lookup
+	// returns ErrNotFound (legitimately empty) while the legacy unkeyed slot
+	// fails with a real keyring access error. The user-scoped ErrNotFound
+	// must not be stashed (it's not a real failure), so the unkeyed real
+	// error reaches the wrapper.
+	authCfg := newTestAuthConfig(t)
+	_, err := authCfg.Login("github.com", "test-user", "test-token", "", true)
+	require.NoError(t, err)
+
+	unkeyedErr := errors.New("simulated unkeyed failure")
+	keyring.MockGetOverride(func(_, user string) (string, error) {
+		if user == "" {
+			return "", unkeyedErr
+		}
+		return "", keyring.ErrNotFound
+	})
+	t.Cleanup(func() { keyring.MockGetOverride(nil) })
+
+	// When we resolve the active token via ActiveTokenWithError
+	token, _, err := authCfg.ActiveTokenWithError("github.com")
+
+	// Then the unkeyed real failure is surfaced.
+	require.Empty(t, token)
+	require.Error(t, err)
+	require.ErrorIs(t, err, unkeyedErr)
+	require.ErrorContains(t, err, "github.com")
+}
+
+func TestActiveTokenSilentlyDiscardsKeyringFailure(t *testing.T) {
+	// Given the same scenario as above
+	authCfg := newTestAuthConfig(t)
+	_, err := authCfg.Login("github.com", "test-user", "test-token", "", true)
+	require.NoError(t, err)
+	keyring.MockInitWithError(errors.New("simulated keyring failure"))
+
+	// When we resolve the active token via the original ActiveToken
+	token, _ := authCfg.ActiveToken("github.com")
+
+	// Then it returns empty without surfacing the error, preserving its
+	// historical signature for existing callers that have not migrated to
+	// ActiveTokenWithError.
+	require.Empty(t, token)
+}
+
+func TestActiveTokenWithErrorDoesNotSurfaceErrNotFound(t *testing.T) {
+	// Given a fresh config with no users and a clean keyring (ErrNotFound
+	// is the legitimate "no token configured for this host" signal)
+	authCfg := newTestAuthConfig(t)
+
+	// When we resolve the active token via ActiveTokenWithError
+	token, _, err := authCfg.ActiveTokenWithError("github.com")
+
+	// Then the empty token is returned without an error, so callers can
+	// proceed anonymously against unconfigured hosts.
+	require.Empty(t, token)
+	require.NoError(t, err)
+}
+
+func TestActiveTokenWithErrorReturnsEnvTokenWithoutKeyring(t *testing.T) {
+	// Given an environment-provided token and a keyring that would fail if
+	// it were consulted
+	t.Setenv("GH_TOKEN", "env-test-token")
+	authCfg := newTestAuthConfig(t)
+	keyring.MockInitWithError(errors.New("keyring should not be consulted"))
+
+	// When we resolve the active token via ActiveTokenWithError
+	token, source, err := authCfg.ActiveTokenWithError("github.com")
+
+	// Then the env-var path short-circuits keyring access entirely and
+	// returns successfully without error.
+	require.NoError(t, err)
+	require.Equal(t, "env-test-token", token)
+	require.Equal(t, "GH_TOKEN", source)
+}
+
+func TestActiveTokenWithErrorReturnsKeyringToken(t *testing.T) {
+	// Given a host with a configured user and a healthy keyring containing
+	// that user's token
+	authCfg := newTestAuthConfig(t)
+	_, err := authCfg.Login("github.com", "test-user", "test-token", "", true)
+	require.NoError(t, err)
+
+	// When we resolve the active token via ActiveTokenWithError
+	token, source, err := authCfg.ActiveTokenWithError("github.com")
+
+	// Then it returns the keyring-stored token with the "keyring" source
+	// label and no error.
+	require.NoError(t, err)
+	require.Equal(t, "test-token", token)
+	require.Equal(t, "keyring", source)
+}
+
 func TestTokenStoredInConfig(t *testing.T) {
 	// Given the user has logged in insecurely
 	authCfg := newTestAuthConfig(t)