Skip to content

fix: make OAuth2 refresh token redemption single-use under concurrency - #28752

Draft
BobbyHo wants to merge 1 commit into
plat481-1-narrow-refresh-scopefrom
plat481-2-single-use-refresh
Draft

fix: make OAuth2 refresh token redemption single-use under concurrency#28752
BobbyHo wants to merge 1 commit into
plat481-1-narrow-refresh-scopefrom
plat481-2-single-use-refresh

Conversation

@BobbyHo

@BobbyHo BobbyHo commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Second of three in PLAT-481. Two concurrent refreshes of one refresh token both minted a replacement. #28744 fixed the same bug on the authorization code; this applies it to api_keys, so the review is a comparison against a merged precedent.

PR What it does
#28744 Code redemption is single-use under concurrency.
#28751 A refresh may name a narrower scope.
this Refresh token redemption is single-use under concurrency.

The fix

  • A refresh deletes the API key the presented token hangs off and mints a replacement. That delete was a blind :exec with no affected-rows check, so the request that lost the race deleted nothing and minted anyway.
  • DeleteAPIKeyByIDReturningRow is the same DELETE ... RETURNING * shape as its code-side sibling: a delete that removed nothing surfaces sql.ErrNoRows. RETURNING * because fetchAndQuery needs an rbac.Objecter.
  • refreshTokenGrant maps that to errBadToken, the invalid_grant it already returns for a token it cannot find, per RFC 6749 §10.5.
  • The seven other DeleteAPIKeyByID call sites are untouched, including authorizationCodeGrant's previous-key delete, where the code delete already arbitrates single use and a returning-row delete would imply otherwise.

Tests

  • TestOAuth2RefreshSingleUse races two refreshes on one barrier and requires exactly one 200 and one 400 invalid_grant. A sequential pair passes pre-fix, so the race is the test; the barrier shape is now shared with TestOAuth2TokenExchangeSingleUse as requireExactlyOneMinted.
  • An APIKey subtest in the existing TestSingleUseDeleteByIDReturningRow pins the second delete returning sql.ErrNoRows.
  • MethodTestSuite fails on any untested database.Store method, so DeleteAPIKeyByIDReturningRow gets a case alongside DeleteAPIKeyByID.

Stack: #28237, #28740, #28744, #28751, this PR.

…ncurrency

Two concurrent refreshes of one refresh token both minted a replacement.
The refresh deletes the API key the presented token hangs off, but that
delete was a blind :exec, so the request that lost the race deleted
nothing and minted anyway.

DeleteAPIKeyByIDReturningRow returns the row it removed, so a delete
that removed nothing surfaces sql.ErrNoRows. The refresh maps that to
the invalid_grant it already returns for an unknown token, which makes
the delete the arbiter of single use (RFC 6749 §10.5). The other
DeleteAPIKeyByID call sites are unchanged, including the exchange's
previous-key delete, where the code delete already arbitrates.
@BobbyHo BobbyHo changed the title fix(coderd): make OAuth2 refresh token redemption single-use under concurrency fix: make OAuth2 refresh token redemption single-use under concurrency Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant