From 1926b7e6586d1e1fd24548f1d17f9e53061d7d78 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 28 Apr 2026 18:03:35 +0300 Subject: [PATCH] fix(coderd/externalauth): detect rate-limit 403/429 and narrow isFailedRefresh (#24334) ValidateToken treated all 403 responses as "token invalid," including GitHub rate limits. isFailedRefresh included 403 in the status code fallthrough, destroying tokens on rate-limited refresh attempts. Split the combined 401/403 check in ValidateToken into a switch on status code. On 403, inspect X-RateLimit-Remaining and Retry-After headers; if either indicates a rate limit, return optimistically valid. Handle 429 the same way. Plain 403 without rate-limit headers preserves the existing invalid-token behavior. Add incorrect_client_credentials and invalid_client to isFailedRefresh error code switch. Remove 403 from the status code fallthrough since no known provider returns 403 from the token endpoint. --- coderd/externalauth/externalauth.go | 79 +++++- .../externalauth_internal_test.go | 85 +++++++ coderd/externalauth/externalauth_test.go | 230 ++++++++++++++++++ 3 files changed, 385 insertions(+), 9 deletions(-) diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index a2e624bf8d..fd21b37a92 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -334,8 +334,12 @@ validate: return externalAuthLink, nil } -// ValidateToken ensures the Git token provided is valid! +// ValidateToken checks if the Git token provided is valid. // The user is optionally returned if the provider supports it. +// Returns valid=true when: the provider confirmed the token, +// no ValidateURL is configured, or the validation endpoint +// returned a rate-limited response (403 with rate-limit headers +// or 429). func (c *Config) ValidateToken(ctx context.Context, link *oauth2.Token) (bool, *codersdk.ExternalAuthUser, error) { if link == nil { return false, nil, xerrors.New("validate external auth token: token is nil") @@ -359,11 +363,36 @@ func (c *Config) ValidateToken(ctx context.Context, link *oauth2.Token) (bool, * return false, nil, err } defer res.Body.Close() - if res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden { + switch res.StatusCode { + case http.StatusUnauthorized: // The token is no longer valid! return false, nil, nil - } - if res.StatusCode != http.StatusOK { + + case http.StatusForbidden: + // Some providers (notably GitHub) use 403 for both "token + // revoked" and "rate limit exceeded." If standard rate-limit + // headers are present, the token may still be valid and the + // validation endpoint is rejecting for a transient reason. + // Treat it as optimistically valid rather than discarding + // the token. + if isRateLimited(res) { + return true, nil, nil + } + // No rate-limit headers: genuine token revocation or + // permission error. + return false, nil, nil + + case http.StatusTooManyRequests: + // GitHub can return either 403 or 429 for rate limits. + // Treat 429 the same as a rate-limited 403: optimistically + // valid. The token was likely just issued by the IDP; the + // validation endpoint is transiently overloaded. + return true, nil, nil + + case http.StatusOK: + // Success, handled below. + + default: data, _ := io.ReadAll(res.Body) return false, nil, xerrors.Errorf("status %d: body: %s", res.StatusCode, data) } @@ -1254,6 +1283,32 @@ func IsGithubDotComURL(str string) bool { return ghURL.Host == "github.com" } +// isRateLimited checks whether an HTTP response indicates a rate +// limit rather than a genuine authorization failure. It returns +// true if either X-RateLimit-Remaining is "0" (primary) or +// Retry-After is present (secondary). OR logic is intentional: +// GitHub secondary limits can include Retry-After without +// X-RateLimit-Remaining: 0 (the remaining count tracks the +// primary quota, not secondary). +// +// Does not catch every secondary rate limit. GitHub can return +// 403 with positive X-RateLimit-Remaining and no Retry-After. +// Reliable detection of those requires response body inspection. +// Missing them is not a regression since all 403s were previously +// treated as invalid. +func isRateLimited(resp *http.Response) bool { + if resp == nil { + return false + } + if resp.Header.Get("Retry-After") != "" { + return true + } + if resp.Header.Get("X-RateLimit-Remaining") == "0" { + return true + } + return false +} + // isFailedRefresh returns true if the error returned by the TokenSource.Token() // is due to a failed refresh. The failure being the refresh token itself. // If this returns true, no amount of retries will fix the issue. @@ -1282,15 +1337,21 @@ func isFailedRefresh(existingToken *oauth2.Token, err error) bool { // Known error codes that indicate a failed refresh. // 'Spec' means the code is defined in the spec. case "bad_refresh_token", // Github - "invalid_grant", // Gitlab & Spec - "unauthorized_client", // Gitea & Spec - "unsupported_grant_type": // Spec, refresh not supported + "invalid_grant", // Gitlab & Spec + "unauthorized_client", // Gitea & Spec + "unsupported_grant_type", // Spec, refresh not supported + "incorrect_client_credentials", // GitHub, wrong client_id/secret (HTTP 200) + "invalid_client": // RFC 6749 Section 5.2, client auth failed return true } switch oauthErr.Response.StatusCode { - case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, http.StatusOK: - // Status codes that indicate the request was processed, and rejected. + case http.StatusBadRequest, http.StatusUnauthorized, http.StatusOK: + // Status codes that indicate the request was processed + // and rejected. 403 is intentionally excluded: no known + // provider returns 403 from the token endpoint, and the + // previous 403 case caused token destruction on + // rate-limited refresh attempts. return true case http.StatusInternalServerError, http.StatusTooManyRequests: // These do not indicate a failed refresh, but could be a temporary issue. diff --git a/coderd/externalauth/externalauth_internal_test.go b/coderd/externalauth/externalauth_internal_test.go index d845d92a86..363225f362 100644 --- a/coderd/externalauth/externalauth_internal_test.go +++ b/coderd/externalauth/externalauth_internal_test.go @@ -1,9 +1,13 @@ package externalauth import ( + "net/http" "testing" + "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/oauth2" "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/codersdk" @@ -124,6 +128,87 @@ func TestGitlabDefaults(t *testing.T) { } } +func TestIsFailedRefresh(t *testing.T) { + t.Parallel() + + expiredToken := &oauth2.Token{ + RefreshToken: "refresh-token", + // isFailedRefresh returns early at the existingToken.Valid() + // guard if the token is valid. Valid() requires + // AccessToken != "" AND not expired. This fixture has no + // AccessToken so Valid() is always false, but we set an + // expired time as a safety net in case someone later adds + // an AccessToken field. + Expiry: time.Now().Add(-time.Hour), + } + + tests := []struct { + name string + err error + expected bool + }{ + { + name: "IncorrectClientCredentials_StatusOK", + err: &oauth2.RetrieveError{ + Response: &http.Response{StatusCode: http.StatusOK}, + ErrorCode: "incorrect_client_credentials", + }, + // StatusOK fallthrough also returns true, so this test + // documents the combined behavior. See the 403-status + // variant below for error-code-only isolation. + expected: true, + }, + { + // Uses 403 status (excluded from the status code switch) + // so the only path to true is the error code switch. + name: "IncorrectClientCredentials_Status403", + err: &oauth2.RetrieveError{ + Response: &http.Response{StatusCode: http.StatusForbidden}, + ErrorCode: "incorrect_client_credentials", + }, + expected: true, + }, + { + name: "InvalidClient_Status401", + err: &oauth2.RetrieveError{ + Response: &http.Response{StatusCode: http.StatusUnauthorized}, + ErrorCode: "invalid_client", + }, + // StatusUnauthorized fallthrough also returns true, so + // this test documents the combined behavior. + expected: true, + }, + { + // Uses 403 status (excluded from the status code switch) + // so the only path to true is the error code switch. + name: "InvalidClient_Status403", + err: &oauth2.RetrieveError{ + Response: &http.Response{StatusCode: http.StatusForbidden}, + ErrorCode: "invalid_client", + }, + expected: true, + }, + { + name: "UnknownErrorCode_Status403_Transient", + err: &oauth2.RetrieveError{ + Response: &http.Response{StatusCode: http.StatusForbidden}, + ErrorCode: "unknown_code", + }, + // 403 with unknown error code should be transient (safe + // default: retry rather than destroy the token). + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := isFailedRefresh(expiredToken, tt.err) + assert.Equal(t, tt.expected, got) + }) + } +} + func Test_bitbucketServerConfigDefaults(t *testing.T) { t.Parallel() diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index 5a17212125..3ebca77614 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -518,6 +518,73 @@ func TestRefreshToken(t *testing.T) { "DB should have the new refresh token despite context cancellation") }) + // SaveBeforeValidate_RateLimited tests the full path: refresh + // succeeds, early save persists the token, validation returns + // rate-limited optimistic true, and RefreshToken returns success + // with no InvalidTokenError. Uses httptest.NewServer for the + // validate endpoint to set rate-limit headers that the FakeIDP's + // WithDynamicUserInfo hook cannot control. + t.Run("SaveBeforeValidate_RateLimited", func(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + + var refreshCalls atomic.Int64 + // rateLimitValidate returns 403 with rate-limit headers. + rateLimitValidate := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Limit", "5000") + w.WriteHeader(http.StatusForbidden) + })) + t.Cleanup(rateLimitValidate.Close) + + fake, config, link := setupOauth2Test(t, testConfig{ + FakeIDPOpts: []oidctest.FakeIDPOpt{ + oidctest.WithRefresh(func(_ string) error { + refreshCalls.Add(1) + return nil + }), + }, + ExternalAuthOpt: func(cfg *externalauth.Config) { + cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String() + cfg.ValidateURL = rateLimitValidate.URL + }, + DB: db, + }) + + // Use a real HTTP transport for non-IDP requests so the + // validate request can reach the httptest server. + ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(&http.Client{ + Transport: http.DefaultTransport, + })) + + oldAccessToken := link.OAuthAccessToken + oldRefreshToken := link.OAuthRefreshToken + + // Expire the token to force a refresh. + link.OAuthExpiry = expired + + // RefreshToken should succeed: the IDP refresh works, the + // early save persists the token, and ValidateToken returns + // (true, nil, nil) because the 403 has rate-limit headers. + updated, err := config.RefreshToken(ctx, db, link) + require.NoError(t, err, "RefreshToken should succeed when validation is rate-limited") + require.Equal(t, int64(1), refreshCalls.Load(), "IDP refresh should have been called") + require.NotEqual(t, oldAccessToken, updated.OAuthAccessToken, + "returned token should be the new one from the refresh") + + // Verify the DB has the new token. + dbLink, err := db.GetExternalAuthLink(context.Background(), database.GetExternalAuthLinkParams{ + ProviderID: link.ProviderID, + UserID: link.UserID, + }) + require.NoError(t, err) + require.Equal(t, updated.OAuthAccessToken, dbLink.OAuthAccessToken, + "DB should have the refreshed access token") + require.NotEqual(t, oldRefreshToken, dbLink.OAuthRefreshToken, + "DB should have the new refresh token (old one was rotated by the IDP)") + }) + // SaveBeforeValidate_DBError tests that when the early DB save // fails after a successful IDP refresh, the error is surfaced // as a non-InvalidTokenError. This is a degraded state (token @@ -622,6 +689,169 @@ func TestRefreshToken(t *testing.T) { }) } +func TestValidateToken(t *testing.T) { + t.Parallel() + + // These tests use httptest.NewServer to control response headers + // (X-RateLimit-Remaining, Retry-After) that the FakeIDP's + // WithDynamicUserInfo hook does not expose. + + newValidateConfig := func(t *testing.T, validateURL string) *externalauth.Config { + t.Helper() + f := promoauth.NewFactory(prometheus.NewRegistry()) + return &externalauth.Config{ + InstrumentedOAuth2Config: f.New("test-validate", &oauth2.Config{}), + ID: "test-validate", + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + ValidateURL: validateURL, + } + } + + newToken := func() *oauth2.Token { + return &oauth2.Token{ + AccessToken: "test-access-token", + Expiry: time.Now().Add(time.Hour), + } + } + + // RateLimitRemaining: 403 with X-RateLimit-Remaining: 0 should be + // treated as rate-limited, not as an invalid token. + t.Run("RateLimitRemaining", func(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Limit", "5000") + w.WriteHeader(http.StatusForbidden) + })) + t.Cleanup(srv.Close) + + config := newValidateConfig(t, srv.URL) + valid, user, err := config.ValidateToken(context.Background(), newToken()) + + require.NoError(t, err) + assert.True(t, valid, "rate-limited 403 should be treated as optimistically valid") + assert.Nil(t, user) + }) + + // RetryAfter: 403 with Retry-After header (secondary rate limit) + // should be treated as rate-limited. + t.Run("RetryAfter", func(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "60") + w.WriteHeader(http.StatusForbidden) + })) + t.Cleanup(srv.Close) + + config := newValidateConfig(t, srv.URL) + valid, user, err := config.ValidateToken(context.Background(), newToken()) + + require.NoError(t, err) + assert.True(t, valid, "rate-limited 403 with Retry-After should be optimistically valid") + assert.Nil(t, user) + }) + + // Forbidden_WithNonZeroRateLimit: a 403 with non-zero + // X-RateLimit-Remaining is a genuine token revocation, not a + // rate limit. GitHub includes X-RateLimit-* headers on all + // authenticated responses; the value matters, not the presence. + t.Run("Forbidden_WithNonZeroRateLimit", func(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-RateLimit-Remaining", "5000") + w.Header().Set("X-RateLimit-Limit", "5000") + w.WriteHeader(http.StatusForbidden) + })) + t.Cleanup(srv.Close) + + config := newValidateConfig(t, srv.URL) + valid, user, err := config.ValidateToken(context.Background(), newToken()) + + require.NoError(t, err) + assert.False(t, valid, "403 with non-zero rate limit remaining means token is invalid") + assert.Nil(t, user) + }) + + // Forbidden_NoRateLimitHeaders: a plain 403 without rate-limit + // headers is a genuine token revocation / permission error. + t.Run("Forbidden_NoRateLimitHeaders", func(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + t.Cleanup(srv.Close) + + config := newValidateConfig(t, srv.URL) + valid, user, err := config.ValidateToken(context.Background(), newToken()) + + require.NoError(t, err) + assert.False(t, valid, "plain 403 without rate-limit headers means token is invalid") + assert.Nil(t, user) + }) + + // Unauthorized: 401 is always a token revocation regardless of + // rate-limit headers. + t.Run("Unauthorized", func(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + t.Cleanup(srv.Close) + + config := newValidateConfig(t, srv.URL) + valid, user, err := config.ValidateToken(context.Background(), newToken()) + + require.NoError(t, err) + assert.False(t, valid, "401 always means token is invalid") + assert.Nil(t, user) + }) + + // Unauthorized_WithRateLimitHeaders: 401 is always a revocation, + // even when rate-limit headers are present. Locks the ordering + // invariant that the 401 branch precedes the rate-limit check. + t.Run("Unauthorized_WithRateLimitHeaders", func(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("Retry-After", "60") + w.WriteHeader(http.StatusUnauthorized) + })) + t.Cleanup(srv.Close) + + config := newValidateConfig(t, srv.URL) + valid, user, err := config.ValidateToken(context.Background(), newToken()) + + require.NoError(t, err) + assert.False(t, valid, "401 is always invalid, even with rate-limit headers") + assert.Nil(t, user) + }) + + // TooManyRequests: 429 is treated optimistically, same as a + // rate-limited 403. GitHub can return either status code for + // rate limits. + t.Run("TooManyRequests", func(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + })) + t.Cleanup(srv.Close) + + config := newValidateConfig(t, srv.URL) + valid, user, err := config.ValidateToken(context.Background(), newToken()) + + require.NoError(t, err) + assert.True(t, valid, "429 should be treated as optimistically valid") + assert.Nil(t, user) + }) +} + func TestRevokeToken(t *testing.T) { t.Parallel()