Skip to content

fix(externalauth): prevent race condition in token refresh with optimistic locking (#22904) (backport to 2.29)#24901

Merged
f0ssel merged 1 commit into
release/2.29from
backport/22904-to-2.29
May 1, 2026
Merged

fix(externalauth): prevent race condition in token refresh with optimistic locking (#22904) (backport to 2.29)#24901
f0ssel merged 1 commit into
release/2.29from
backport/22904-to-2.29

Conversation

@f0ssel
Copy link
Copy Markdown
Member

@f0ssel f0ssel commented May 1, 2026

Backport of #22904 to release/2.29.

Adds an optimistic lock to UpdateExternalAuthLinkRefreshToken so that a concurrent caller that lost a token-refresh race cannot overwrite a valid token stored by the winner. The SQL WHERE clause now includes AND oauth_refresh_token = @old_oauth_refresh_token.

Original PR: #22904
Merge commit: 53e52ae

Cherry-pick applied cleanly with no conflicts.

Generated by Coder Agents

…istic locking (#22904)

## Problem

When multiple concurrent callers (e.g., parallel workspace builds) read
the same single-use OAuth2 refresh token from the database and race to
exchange it with the provider, the first caller succeeds but subsequent
callers get `bad_refresh_token`. The losing caller then **clears the
valid new token** from the database, permanently breaking the auth link
until the user manually re-authenticates.

This is reliably reproducible when launching multiple workspaces
simultaneously with GitHub App external auth and user-to-server token
expiration enabled.

## Solution

Two layers of protection:

### 1. Singleflight deduplication (`Config.RefreshToken` +
`ObtainOIDCAccessToken`)

Concurrent callers for the same user/provider share a single refresh
call via `golang.org/x/sync/singleflight`, keyed by `userID`. The
singleflight callback re-reads the link from the database to pick up any
token already refreshed by a prior in-flight call, avoiding redundant
IDP round-trips entirely.

### 2. Optimistic locking on `UpdateExternalAuthLinkRefreshToken`

The SQL `WHERE` clause now includes `AND oauth_refresh_token =
@old_oauth_refresh_token`, so if two replicas (HA) race past
singleflight, the loser's destructive UPDATE is a harmless no-op rather
than overwriting the winner's valid token.

## Changes

| File | Change |
|------|--------|
| `coderd/externalauth/externalauth.go` | Added `singleflight.Group` to
`Config`; split `RefreshToken` into public wrapper +
`refreshTokenInner`; pass `OldOauthRefreshToken` to DB update |
| `coderd/provisionerdserver/provisionerdserver.go` | Wrapped OIDC
refresh in `ObtainOIDCAccessToken` with package-level singleflight |
| `coderd/database/queries/externalauth.sql` | Added optimistic lock
(`WHERE ... AND oauth_refresh_token = @old_oauth_refresh_token`) |
| `coderd/database/queries.sql.go` | Regenerated |
| `coderd/database/querier.go` | Regenerated |
| `coderd/database/dbauthz/dbauthz_test.go` | Updated test params for
new field |
| `coderd/externalauth/externalauth_test.go` | Added
`ConcurrentRefreshDedup` test; updated existing tests for singleflight
DB re-read |

## Testing

- **New test `ConcurrentRefreshDedup`**: 5 goroutines call
`RefreshToken` concurrently, asserts IDP refresh called exactly once,
all callers get same token.
- All existing `TestRefreshToken/*` subtests updated and passing.
- `TestObtainOIDCAccessToken` passing.
- `dbauthz` tests passing.

(cherry picked from commit 53e52ae)
@f0ssel f0ssel force-pushed the backport/22904-to-2.29 branch from 3e09ae9 to d5fa784 Compare May 1, 2026 18:14
@f0ssel f0ssel merged commit 63a9280 into release/2.29 May 1, 2026
24 of 25 checks passed
@f0ssel f0ssel deleted the backport/22904-to-2.29 branch May 1, 2026 18:37
@github-actions github-actions Bot locked and limited conversation to collaborators May 1, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants