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

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

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()
 

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()