From 3580ceb7a66fddf7426bba833081b46d1cba1f78 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 14 Apr 2026 12:53:05 +0000 Subject: [PATCH 1/7] fix(coderd/externalauth): save refreshed token before validation GitHub rotates refresh tokens on use, invalidating the old token immediately. If post-refresh validation fails (e.g. rate-limited 403 from /user), the new token was silently discarded because the DB save only happened after successful validation. The next refresh attempt would use the stale (now-invalid) refresh token, fail permanently, and destroy the token. Move the UpdateExternalAuthLink call from after validation to immediately after TokenSource.Token() succeeds. The existing post-validation save becomes a no-op when the early save fires. --- coderd/externalauth/externalauth.go | 49 ++++++-- coderd/externalauth/externalauth_test.go | 145 +++++++++++++++++++++++ 2 files changed, 182 insertions(+), 12 deletions(-) diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index 532c5b7e270f9..3e4715a4688da 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -261,6 +261,30 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu return externalAuthLink, xerrors.Errorf("generate token extra: %w", err) } + // Persist the refreshed token to the DB before validation. GitHub + // rotates refresh tokens on every use, so the old refresh token is + // already invalid on the IDP side. If we validated first and the + // validation endpoint was unavailable (e.g. rate-limited 403), the + // new token would be silently lost and the user would be forced to + // re-authenticate manually. + if db != nil && token.AccessToken != externalAuthLink.OAuthAccessToken { + updatedAuthLink, err := db.UpdateExternalAuthLink(ctx, database.UpdateExternalAuthLinkParams{ + ProviderID: c.ID, + UserID: externalAuthLink.UserID, + UpdatedAt: dbtime.Now(), + OAuthAccessToken: token.AccessToken, + OAuthAccessTokenKeyID: sql.NullString{}, // dbcrypt will update as required + OAuthRefreshToken: token.RefreshToken, + OAuthRefreshTokenKeyID: sql.NullString{}, // dbcrypt will update as required + OAuthExpiry: token.Expiry, + OAuthExtra: extra, + }) + if err != nil { + return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err) + } + externalAuthLink = updatedAuthLink + } + r := retry.New(50*time.Millisecond, 200*time.Millisecond) // See the comment below why the retry and cancel is required. retryCtx, retryCtxCancel := context.WithTimeout(ctx, time.Second) @@ -301,19 +325,20 @@ validate: return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err) } externalAuthLink = updatedAuthLink + } - // Update the associated users github.com username if the token is for github.com. - if IsGithubDotComURL(c.AuthCodeURL("")) && user != nil { - err = db.UpdateUserGithubComUserID(ctx, database.UpdateUserGithubComUserIDParams{ - ID: externalAuthLink.UserID, - GithubComUserID: sql.NullInt64{ - Int64: user.ID, - Valid: true, - }, - }) - if err != nil { - return externalAuthLink, xerrors.Errorf("update user github com user id: %w", err) - } + // Update the associated users github.com username if the token + // is for github.com and validation returned user info. + if IsGithubDotComURL(c.AuthCodeURL("")) && user != nil { + err = db.UpdateUserGithubComUserID(ctx, database.UpdateUserGithubComUserIDParams{ + ID: externalAuthLink.UserID, + GithubComUserID: sql.NullInt64{ + Int64: user.ID, + Valid: true, + }, + }) + if err != nil { + return externalAuthLink, xerrors.Errorf("update user github com user id: %w", err) } } diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index daf5927e21f77..7953ab8913d45 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -2,12 +2,14 @@ package externalauth_test import ( "context" + "database/sql" "encoding/json" "fmt" "net/http" "net/http/httptest" "net/url" "strings" + "sync/atomic" "testing" "time" @@ -26,6 +28,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbmock" "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/externalauth" "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/codersdk" @@ -379,6 +382,148 @@ func TestRefreshToken(t *testing.T) { require.True(t, ok) require.Equal(t, updated.OAuthAccessToken, mapping["access_token"]) }) + + // SaveBeforeValidate tests that a successfully refreshed token is + // persisted to the DB even when post-refresh validation fails. This + // prevents the data-loss scenario where GitHub rotates the refresh + // token on use but the new token is silently discarded because a + // rate-limited validation endpoint returns 403. + t.Run("SaveBeforeValidate", func(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + + // simulateRateLimit controls whether the validate endpoint + // returns 403 (true) or 200 (false). + var simulateRateLimit atomic.Bool + simulateRateLimit.Store(true) + + var refreshCalls atomic.Int64 + fake, config, link := setupOauth2Test(t, testConfig{ + FakeIDPOpts: []oidctest.FakeIDPOpt{ + oidctest.WithRefresh(func(_ string) error { + refreshCalls.Add(1) + return nil + }), + oidctest.WithDynamicUserInfo(func(_ string) (jwt.MapClaims, error) { + if simulateRateLimit.Load() { + return jwt.MapClaims{}, oidctest.StatusError(http.StatusForbidden, xerrors.New("rate limit exceeded")) + } + return jwt.MapClaims{}, nil + }), + }, + ExternalAuthOpt: func(cfg *externalauth.Config) { + cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String() + }, + DB: db, + }) + + ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil)) + + oldAccessToken := link.OAuthAccessToken + oldRefreshToken := link.OAuthRefreshToken + + // Expire the token to force a refresh. + link.OAuthExpiry = expired + + // --- First call: refresh succeeds, validation fails (403). --- + _, err := config.RefreshToken(ctx, db, link) + require.Error(t, err, "expected error because validation returned 403") + require.True(t, externalauth.IsInvalidTokenError(err)) + require.Equal(t, int64(1), refreshCalls.Load(), "IDP refresh should have been called exactly once") + + // Critical assertion: the DB must contain the NEW tokens from the + // successful refresh, not the old (now-stale) ones. + dbLink, err := db.GetExternalAuthLink(context.Background(), database.GetExternalAuthLinkParams{ + ProviderID: link.ProviderID, + UserID: link.UserID, + }) + require.NoError(t, err) + require.NotEqual(t, oldAccessToken, dbLink.OAuthAccessToken, + "DB should have the new access token from the successful refresh") + require.NotEqual(t, oldRefreshToken, dbLink.OAuthRefreshToken, + "DB should have the new refresh token (old one was rotated by the IDP)") + + // --- Second call: uses the saved token from DB, no re-refresh. --- + // The saved token has a future expiry, so TokenSource should return + // it without contacting the IDP. Validation should succeed now. + simulateRateLimit.Store(false) + updated, err := config.RefreshToken(ctx, db, dbLink) + require.NoError(t, err, "second call should succeed because rate limit lifted") + require.Equal(t, int64(1), refreshCalls.Load(), + "IDP refresh should NOT have been called again; the saved token is not expired") + require.Equal(t, dbLink.OAuthAccessToken, updated.OAuthAccessToken, + "returned token should match what was saved in the DB") + }) + + // ConcurrentRefreshOptimisticLock verifies that when caller A saves + // a successfully refreshed token (early save), caller B's subsequent + // attempt to destroy the refresh token via + // UpdateExternalAuthLinkRefreshToken is a no-op. The optimistic lock + // (WHERE oauth_refresh_token = @old) prevents B from overwriting + // A's valid token. + t.Run("ConcurrentRefreshOptimisticLock", func(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + + fake, config, link := setupOauth2Test(t, testConfig{ + FakeIDPOpts: []oidctest.FakeIDPOpt{ + oidctest.WithRefresh(func(_ string) error { + return nil + }), + oidctest.WithDynamicUserInfo(func(_ string) (jwt.MapClaims, error) { + return jwt.MapClaims{}, nil + }), + }, + ExternalAuthOpt: func(cfg *externalauth.Config) { + cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String() + }, + DB: db, + }) + + ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil)) + + // Snapshot the original tokens before any refresh. + oldRefreshToken := link.OAuthRefreshToken + + // Expire the token to force a refresh. + link.OAuthExpiry = expired + + // Caller A: refresh and save successfully. + updated, err := config.RefreshToken(ctx, db, link) + require.NoError(t, err) + require.NotEqual(t, oldRefreshToken, updated.OAuthRefreshToken, + "caller A should have a new refresh token") + + // Caller B had a stale read of the original link. It tries to + // destroy the refresh token using the OLD refresh token in the + // optimistic lock. Because caller A already wrote a different + // refresh token, this WHERE clause matches nothing. + err = db.UpdateExternalAuthLinkRefreshToken(ctx, database.UpdateExternalAuthLinkRefreshTokenParams{ + OauthRefreshFailureReason: "simulated failure from stale caller B", + OAuthRefreshToken: "", + OAuthRefreshTokenKeyID: sql.NullString{}.String, + UpdatedAt: dbtime.Now(), + ProviderID: link.ProviderID, + UserID: link.UserID, + OldOauthRefreshToken: oldRefreshToken, + }) + require.NoError(t, err, "optimistic lock write should not error, it is a no-op") + + // Verify DB still has caller A's valid 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, + "caller A's access token should still be in DB") + require.Equal(t, updated.OAuthRefreshToken, dbLink.OAuthRefreshToken, + "caller A's refresh token should still be in DB") + require.Empty(t, dbLink.OauthRefreshFailureReason, + "caller B's failure reason should not have been written") + }) } func TestRevokeToken(t *testing.T) { From 43159d6b46ad30cb137aefcce36253bd3abff99a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 15 Apr 2026 13:14:18 +0000 Subject: [PATCH 2/7] fix(coderd/externalauth): address review feedback Delete the dead post-validation save block (unreachable after early save). Gate UpdateUserGithubComUserID on whether a refresh occurred via originalAccessToken snapshot, restoring refresh-only semantics. Differentiate the early save error message, add a DB failure test, rename the optimistic lock test, and fix minor nits. --- coderd/externalauth/externalauth.go | 28 +++---------- coderd/externalauth/externalauth_test.go | 51 +++++++++++++++++++----- 2 files changed, 48 insertions(+), 31 deletions(-) diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index 3e4715a4688da..434e306777ebf 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -267,7 +267,9 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu // validation endpoint was unavailable (e.g. rate-limited 403), the // new token would be silently lost and the user would be forced to // re-authenticate manually. - if db != nil && token.AccessToken != externalAuthLink.OAuthAccessToken { + originalAccessToken := externalAuthLink.OAuthAccessToken + // db may be nil in unit tests that don't exercise persistence. + if db != nil && token.AccessToken != originalAccessToken { updatedAuthLink, err := db.UpdateExternalAuthLink(ctx, database.UpdateExternalAuthLinkParams{ ProviderID: c.ID, UserID: externalAuthLink.UserID, @@ -280,7 +282,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu OAuthExtra: extra, }) if err != nil { - return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err) + return updatedAuthLink, xerrors.Errorf("persist refreshed token: %w", err) } externalAuthLink = updatedAuthLink } @@ -309,27 +311,9 @@ validate: return externalAuthLink, InvalidTokenError("token failed to validate") } - if token.AccessToken != externalAuthLink.OAuthAccessToken { - updatedAuthLink, err := db.UpdateExternalAuthLink(ctx, database.UpdateExternalAuthLinkParams{ - ProviderID: c.ID, - UserID: externalAuthLink.UserID, - UpdatedAt: dbtime.Now(), - OAuthAccessToken: token.AccessToken, - OAuthAccessTokenKeyID: sql.NullString{}, // dbcrypt will update as required - OAuthRefreshToken: token.RefreshToken, - OAuthRefreshTokenKeyID: sql.NullString{}, // dbcrypt will update as required - OAuthExpiry: token.Expiry, - OAuthExtra: extra, - }) - if err != nil { - return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err) - } - externalAuthLink = updatedAuthLink - } - - // Update the associated users github.com username if the token + // Update the associated user's github.com user ID if the token // is for github.com and validation returned user info. - if IsGithubDotComURL(c.AuthCodeURL("")) && user != nil { + if originalAccessToken != token.AccessToken && IsGithubDotComURL(c.AuthCodeURL("")) && user != nil { err = db.UpdateUserGithubComUserID(ctx, database.UpdateUserGithubComUserIDParams{ ID: externalAuthLink.UserID, GithubComUserID: sql.NullInt64{ diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index 7953ab8913d45..b42ff947c4a5d 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -2,7 +2,6 @@ package externalauth_test import ( "context" - "database/sql" "encoding/json" "fmt" "net/http" @@ -456,13 +455,47 @@ func TestRefreshToken(t *testing.T) { "returned token should match what was saved in the DB") }) - // ConcurrentRefreshOptimisticLock verifies that when caller A saves - // a successfully refreshed token (early save), caller B's subsequent - // attempt to destroy the refresh token via - // UpdateExternalAuthLinkRefreshToken is a no-op. The optimistic lock - // (WHERE oauth_refresh_token = @old) prevents B from overwriting - // A's valid token. - t.Run("ConcurrentRefreshOptimisticLock", func(t *testing.T) { + // 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 + // issued by IDP but not persisted), and callers should see a + // real error, not a "please re-authenticate" prompt. + t.Run("SaveBeforeValidate_DBError", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mDB := dbmock.NewMockStore(ctrl) + + fake, config, link := setupOauth2Test(t, testConfig{ + FakeIDPOpts: []oidctest.FakeIDPOpt{ + oidctest.WithRefresh(func(_ string) error { + return nil + }), + }, + ExternalAuthOpt: func(cfg *externalauth.Config) { + cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String() + }, + }) + + ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil)) + link.OAuthExpiry = expired + + mDB.EXPECT(). + UpdateExternalAuthLink(gomock.Any(), gomock.Any()). + Return(database.ExternalAuthLink{}, xerrors.New("db connection lost")) + + _, err := config.RefreshToken(ctx, mDB, link) + require.Error(t, err) + require.Contains(t, err.Error(), "persist refreshed token") + require.False(t, externalauth.IsInvalidTokenError(err), + "DB errors should not be treated as invalid token") + }) + + // OptimisticLockPreventsStaleOverwrite verifies that the + // UpdateExternalAuthLinkRefreshToken WHERE clause prevents a + // stale caller from overwriting a valid refresh token saved + // by a concurrent winner. + t.Run("OptimisticLockPreventsStaleOverwrite", func(t *testing.T) { t.Parallel() db, _ := dbtestutil.NewDB(t) @@ -503,7 +536,7 @@ func TestRefreshToken(t *testing.T) { err = db.UpdateExternalAuthLinkRefreshToken(ctx, database.UpdateExternalAuthLinkRefreshTokenParams{ OauthRefreshFailureReason: "simulated failure from stale caller B", OAuthRefreshToken: "", - OAuthRefreshTokenKeyID: sql.NullString{}.String, + OAuthRefreshTokenKeyID: "", UpdatedAt: dbtime.Now(), ProviderID: link.ProviderID, UserID: link.UserID, From 5590a9ac0de168bd8f8885441778ae77d84da498 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 15 Apr 2026 15:50:35 +0000 Subject: [PATCH 3/7] fix(coderd/externalauth): add db != nil guard to UpdateUserGithubComUserID The early save's db != nil guard lets execution proceed past it when db is nil, making UpdateUserGithubComUserID reachable and panicking. Add the same guard. --- coderd/externalauth/externalauth.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index 434e306777ebf..485c4933e3113 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -313,7 +313,7 @@ validate: // Update the associated user's github.com user ID if the token // is for github.com and validation returned user info. - if originalAccessToken != token.AccessToken && IsGithubDotComURL(c.AuthCodeURL("")) && user != nil { + if db != nil && originalAccessToken != token.AccessToken && IsGithubDotComURL(c.AuthCodeURL("")) && user != nil { err = db.UpdateUserGithubComUserID(ctx, database.UpdateUserGithubComUserIDParams{ ID: externalAuthLink.UserID, GithubComUserID: sql.NullInt64{ From 78d71cb4290f96abdcb96c6bcf26791698ae8a0f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 15 Apr 2026 18:53:57 +0000 Subject: [PATCH 4/7] fix(coderd/externalauth): align operand order in access token check Match line 272's token.AccessToken != originalAccessToken order. --- coderd/externalauth/externalauth.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index 485c4933e3113..83d4d5fbd2fec 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -313,7 +313,7 @@ validate: // Update the associated user's github.com user ID if the token // is for github.com and validation returned user info. - if db != nil && originalAccessToken != token.AccessToken && IsGithubDotComURL(c.AuthCodeURL("")) && user != nil { + if db != nil && token.AccessToken != originalAccessToken && IsGithubDotComURL(c.AuthCodeURL("")) && user != nil { err = db.UpdateUserGithubComUserID(ctx, database.UpdateUserGithubComUserIDParams{ ID: externalAuthLink.UserID, GithubComUserID: sql.NullInt64{ From e4947c3ccbd14f1e6206a8028645d00bb4a8f2d1 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 15 Apr 2026 19:12:13 +0000 Subject: [PATCH 5/7] fix(coderd/externalauth): remove db != nil guards, fix tests Remove the db != nil guards from the early save and UpdateUserGithubComUserID. The db parameter is never nil in production. ValidateServerError and ValidateFailure now pass a mock DB instead of nil. --- coderd/externalauth/externalauth.go | 5 ++--- coderd/externalauth/externalauth_test.go | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index 83d4d5fbd2fec..eaa4e080a1ffe 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -268,8 +268,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu // new token would be silently lost and the user would be forced to // re-authenticate manually. originalAccessToken := externalAuthLink.OAuthAccessToken - // db may be nil in unit tests that don't exercise persistence. - if db != nil && token.AccessToken != originalAccessToken { + if token.AccessToken != originalAccessToken { updatedAuthLink, err := db.UpdateExternalAuthLink(ctx, database.UpdateExternalAuthLinkParams{ ProviderID: c.ID, UserID: externalAuthLink.UserID, @@ -313,7 +312,7 @@ validate: // Update the associated user's github.com user ID if the token // is for github.com and validation returned user info. - if db != nil && token.AccessToken != originalAccessToken && IsGithubDotComURL(c.AuthCodeURL("")) && user != nil { + if token.AccessToken != originalAccessToken && IsGithubDotComURL(c.AuthCodeURL("")) && user != nil { err = db.UpdateUserGithubComUserID(ctx, database.UpdateUserGithubComUserIDParams{ ID: externalAuthLink.UserID, GithubComUserID: sql.NullInt64{ diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index b42ff947c4a5d..1b33eff6ff208 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -121,6 +121,11 @@ func TestRefreshToken(t *testing.T) { t.Run("ValidateServerError", func(t *testing.T) { t.Parallel() + ctrl := gomock.NewController(t) + mDB := dbmock.NewMockStore(ctrl) + mDB.EXPECT().UpdateExternalAuthLink(gomock.Any(), gomock.Any()). + Return(database.ExternalAuthLink{}, nil).AnyTimes() + const staticError = "static error" validated := false fake, config, link := setupOauth2Test(t, testConfig{ @@ -137,7 +142,7 @@ func TestRefreshToken(t *testing.T) { ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil)) link.OAuthExpiry = expired - _, err := config.RefreshToken(ctx, nil, link) + _, err := config.RefreshToken(ctx, mDB, link) require.ErrorContains(t, err, staticError) // Unsure if this should be the correct behavior. It's an invalid token because // 'ValidateToken()' failed with a runtime error. This was the previous behavior, @@ -224,6 +229,11 @@ func TestRefreshToken(t *testing.T) { t.Run("ValidateFailure", func(t *testing.T) { t.Parallel() + ctrl := gomock.NewController(t) + mDB := dbmock.NewMockStore(ctrl) + mDB.EXPECT().UpdateExternalAuthLink(gomock.Any(), gomock.Any()). + Return(database.ExternalAuthLink{}, nil).AnyTimes() + const staticError = "static error" validated := false fake, config, link := setupOauth2Test(t, testConfig{ @@ -240,7 +250,7 @@ func TestRefreshToken(t *testing.T) { ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil)) link.OAuthExpiry = expired - _, err := config.RefreshToken(ctx, nil, link) + _, err := config.RefreshToken(ctx, mDB, link) require.ErrorContains(t, err, "token failed to validate") require.True(t, externalauth.IsInvalidTokenError(err)) require.True(t, validated, "token should have been attempted to be validated") From 8ebd45fa4def034d534c77f1dbdccb4e64d18746 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 15 Apr 2026 19:17:49 +0000 Subject: [PATCH 6/7] fix(coderd/externalauth): clean up test comments --- coderd/externalauth/externalauth_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index 1b33eff6ff208..592b95c6fd148 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -435,7 +435,7 @@ func TestRefreshToken(t *testing.T) { // Expire the token to force a refresh. link.OAuthExpiry = expired - // --- First call: refresh succeeds, validation fails (403). --- + // First call: refresh succeeds, validation fails (403). _, err := config.RefreshToken(ctx, db, link) require.Error(t, err, "expected error because validation returned 403") require.True(t, externalauth.IsInvalidTokenError(err)) @@ -453,7 +453,7 @@ func TestRefreshToken(t *testing.T) { require.NotEqual(t, oldRefreshToken, dbLink.OAuthRefreshToken, "DB should have the new refresh token (old one was rotated by the IDP)") - // --- Second call: uses the saved token from DB, no re-refresh. --- + // Second call: uses the saved token from DB, no re-refresh. // The saved token has a future expiry, so TokenSource should return // it without contacting the IDP. Validation should succeed now. simulateRateLimit.Store(false) From cbb926c03874648356dc0932c0f02662dcbd221c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 16 Apr 2026 10:18:55 +0000 Subject: [PATCH 7/7] fix(coderd/externalauth): use detached context for post-refresh DB save The IDP consumes the old refresh token during TokenSource.Token(). If the caller's request context is canceled before the DB write completes, the new token is lost. Use context.WithoutCancel for the UpdateExternalAuthLink call only. Validation and the GitHub user ID update still use the caller's context so they abort promptly when nobody is waiting for the result. --- coderd/externalauth/externalauth.go | 8 +++- coderd/externalauth/externalauth_test.go | 53 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index eaa4e080a1ffe..a2e624bf8dee9 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -267,9 +267,15 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu // validation endpoint was unavailable (e.g. rate-limited 403), the // new token would be silently lost and the user would be forced to // re-authenticate manually. + // Use a detached context for the DB write only. The IDP already + // consumed the old refresh token, so if the caller's request + // context is canceled mid-save, the new token would be lost. + persistCtx, persistCancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second) + defer persistCancel() + originalAccessToken := externalAuthLink.OAuthAccessToken if token.AccessToken != originalAccessToken { - updatedAuthLink, err := db.UpdateExternalAuthLink(ctx, database.UpdateExternalAuthLinkParams{ + updatedAuthLink, err := db.UpdateExternalAuthLink(persistCtx, database.UpdateExternalAuthLinkParams{ ProviderID: c.ID, UserID: externalAuthLink.UserID, UpdatedAt: dbtime.Now(), diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index 592b95c6fd148..5a17212125572 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -465,6 +465,59 @@ func TestRefreshToken(t *testing.T) { "returned token should match what was saved in the DB") }) + // SaveBeforeValidate_ContextCanceled verifies the early DB save + // uses a detached context. The parent context is canceled inside + // the refresh hook (after TokenSource.Token() but before the DB + // write), and the test asserts the new token is still persisted. + t.Run("SaveBeforeValidate_ContextCanceled", func(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + + var refreshCalls atomic.Int64 + cancelOnRefresh, cancel := context.WithCancel(context.Background()) + defer cancel() + + fake, config, link := setupOauth2Test(t, testConfig{ + FakeIDPOpts: []oidctest.FakeIDPOpt{ + oidctest.WithRefresh(func(_ string) error { + refreshCalls.Add(1) + // Cancel the parent context after refresh succeeds + // but before the DB save and validation. + cancel() + return nil + }), + oidctest.WithDynamicUserInfo(func(_ string) (jwt.MapClaims, error) { + return jwt.MapClaims{}, nil + }), + }, + ExternalAuthOpt: func(cfg *externalauth.Config) { + cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String() + }, + DB: db, + }) + + ctx := oidc.ClientContext(cancelOnRefresh, fake.HTTPClient(nil)) + + oldAccessToken := link.OAuthAccessToken + oldRefreshToken := link.OAuthRefreshToken + link.OAuthExpiry = expired + + _, err := config.RefreshToken(ctx, db, link) + require.NoError(t, err) + require.Equal(t, int64(1), refreshCalls.Load()) + + dbLink, err := db.GetExternalAuthLink(context.Background(), database.GetExternalAuthLinkParams{ + ProviderID: link.ProviderID, + UserID: link.UserID, + }) + require.NoError(t, err) + require.NotEqual(t, oldAccessToken, dbLink.OAuthAccessToken, + "DB should have the new access token despite context cancellation") + require.NotEqual(t, oldRefreshToken, dbLink.OAuthRefreshToken, + "DB should have the new refresh token despite context cancellation") + }) + // 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