From 5213965abd657e35d61ff427d0b474ee30412879 Mon Sep 17 00:00:00 2001 From: Hesham Salman Date: Wed, 29 Apr 2026 17:58:34 -0400 Subject: [PATCH] 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 " (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 --- api/http_client.go | 12 +- api/http_client_test.go | 88 +++++++++++++ internal/authflow/flow.go | 5 + internal/config/auth_config_test.go | 184 ++++++++++++++++++++++++++++ internal/config/config.go | 90 +++++++++++--- internal/gh/gh.go | 6 + internal/keyring/keyring.go | 28 +++++ 7 files changed, 395 insertions(+), 18 deletions(-) diff --git a/api/http_client.go b/api/http_client.go index be7a6b8a71c..f04068d8ed2 100644 --- a/api/http_client.go +++ b/api/http_client.go @@ -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)) } } diff --git a/api/http_client_test.go b/api/http_client_test.go index 198c0849118..3aea4eb5f69 100644 --- a/api/http_client_test.go +++ b/api/http_client_test.go @@ -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+`) diff --git a/internal/authflow/flow.go b/internal/authflow/flow.go index 0a195168f26..21f7774b693 100644 --- a/internal/authflow/flow.go +++ b/internal/authflow/flow.go @@ -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}) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index ca5f7e584cb..9d7d9978e37 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -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) diff --git a/internal/config/config.go b/internal/config/config.go index a694ca65461..cf56c937ea9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -234,29 +234,85 @@ type AuthConfig struct { // ActiveToken will retrieve the active auth token for the given hostname, // searching environment variables, plain text config, and // lastly encrypted storage. +// +// Errors encountered while resolving the token (notably keyring access +// failures such as timeouts on macOS Keychain) are discarded so this +// method's signature stays unchanged for existing callers. Code that +// needs to distinguish "no token here" from "token resolution failed" +// should use ActiveTokenWithError instead. func (c *AuthConfig) ActiveToken(hostname string) (string, string) { + token, source, _ := c.ActiveTokenWithError(hostname) + return token, source +} + +// ActiveTokenWithError is like ActiveToken but additionally returns any +// error encountered while resolving the active token. The most important +// case is a keyring access failure (e.g. *keyring.TimeoutError): callers +// that receive a non-nil error should treat it as a real auth-resolution +// failure rather than silently proceeding without a token. +// +// A non-nil error is returned only when a keyring lookup fails with an +// error other than keyring.ErrNotFound. ErrNotFound is the legitimate +// "no token here" signal and is not surfaced as an error so that +// genuinely anonymous requests against unconfigured hosts keep working. +func (c *AuthConfig) ActiveTokenWithError(hostname string) (string, string, error) { if c.tokenOverride != nil { - return c.tokenOverride(hostname) + token, source := c.tokenOverride(hostname) + return token, source, nil } token, source := ghauth.TokenFromEnvOrConfig(hostname) - if token == "" { - var user string - var err error - if user, err = c.ActiveUser(hostname); err == nil { - token, err = c.TokenFromKeyringForUser(hostname, user) - } - if err != nil { - // We should generally be able to find a token for the active user, - // but in some cases such as if the keyring was set up in a very old - // version of the CLI, it may only have a unkeyed token, so fallback - // to it. - token, err = c.TokenFromKeyring(hostname) - } - if err == nil { - source = "keyring" + if token != "" { + return token, source, nil + } + + var user string + var err error + var primaryKeyringErr error + if user, err = c.ActiveUser(hostname); err == nil { + // Invariant: user is non-empty here, so TokenFromKeyringForUser + // will not return its "username cannot be blank" sentinel and any + // error we observe is a real keyring access result. + token, err = c.TokenFromKeyringForUser(hostname, user) + // Stash a real keyring access failure from the user-scoped lookup + // so the fallback below can't mask it by reporting its own + // ErrNotFound. We deliberately scope this to errors from the + // keyring call, since errors from ActiveUser are config-lookup + // failures rather than keyring-access failures: when ActiveUser + // errors (host has no configured user), falling through to the + // legacy unkeyed lookup and returning anonymous on its ErrNotFound + // is the historical, intended behavior for unconfigured hosts. + if err != nil && !errors.Is(err, keyring.ErrNotFound) { + primaryKeyringErr = err } } - return token, source + if err != nil { + // We should generally be able to find a token for the active user, + // but in some cases such as if the keyring was set up in a very old + // version of the CLI, it may only have a unkeyed token, so fallback + // to it. + token, err = c.TokenFromKeyring(hostname) + } + if err == nil { + source = "keyring" + return token, source, nil + } + if primaryKeyringErr != nil && errors.Is(err, keyring.ErrNotFound) { + // The legacy unkeyed lookup didn't recover a token, so the real + // failure to surface is the user-scoped one. Otherwise we'd treat + // a timeout or permission error as "no token here" and silently + // drop to anonymous requests. + err = primaryKeyringErr + } + if errors.Is(err, keyring.ErrNotFound) { + // No token configured for this host. Preserve the historical + // "return empty without error" behavior so callers can choose to + // proceed anonymously when appropriate. + return token, source, nil + } + // A real keyring access failure (timeout, permission denied, etc.). + // Surface it so the request layer can fail loudly instead of silently + // sending unauthenticated requests. + return token, source, fmt.Errorf("couldn't resolve auth token from keyring for %q: %w", hostname, err) } // HasActiveToken returns true when a token for the hostname is present. diff --git a/internal/gh/gh.go b/internal/gh/gh.go index 759a931f2b7..975e5bf24d7 100644 --- a/internal/gh/gh.go +++ b/internal/gh/gh.go @@ -110,6 +110,12 @@ type AuthConfig interface { // general configuration, and finally encrypted storage. ActiveToken(hostname string) (token string, source string) + // ActiveTokenWithError is like ActiveToken but additionally returns any error encountered while resolving + // the active token, such as a keyring access timeout. Callers that need to distinguish "no token configured + // for this host" from "token resolution failed" should prefer this method so a keyring failure does not + // silently produce an unauthenticated request. + ActiveTokenWithError(hostname string) (token string, source string, err error) + // HasEnvToken returns true when a token has been specified in an environment variable, else returns false. HasEnvToken() bool diff --git a/internal/keyring/keyring.go b/internal/keyring/keyring.go index f873c643600..66d94aa36ba 100644 --- a/internal/keyring/keyring.go +++ b/internal/keyring/keyring.go @@ -33,8 +33,20 @@ func Set(service, user, secret string) error { } } +// getOverride, when non-nil, intercepts Get calls before they reach the +// underlying provider. It exists solely to let tests simulate per-(service,user) +// failure modes that the upstream mock can only express globally. +var getOverride func(service, user string) (string, error) + // Get secret from keyring given service and user name. func Get(service, user string) (string, error) { + if fn := getOverride; fn != nil { + val, err := fn(service, user) + if errors.Is(err, keyring.ErrNotFound) { + return "", ErrNotFound + } + return val, err + } ch := make(chan struct { val string err error @@ -75,8 +87,24 @@ func Delete(service, user string) error { func MockInit() { keyring.MockInit() + getOverride = nil } func MockInitWithError(err error) { keyring.MockInitWithError(err) + getOverride = nil +} + +// MockGetOverride installs fn as an interceptor for Get calls. Pass nil to +// remove the override. Intended for tests that need per-(service,user) failure +// modes which the upstream mock cannot express. +// +// The override is stored in a package-level variable without synchronization, +// so callers must not use this with t.Parallel(). Either MockInit or +// MockInitWithError will clear the override, preserving the isolation +// contract for tests that reset keyring state at setup; callers should still +// prefer registering t.Cleanup to restore nil promptly when the override is +// only needed for one test. +func MockGetOverride(fn func(service, user string) (string, error)) { + getOverride = fn }