fix: make OAuth2 refresh token redemption single-use under concurrency - #28752
Draft
BobbyHo wants to merge 1 commit into
Draft
fix: make OAuth2 refresh token redemption single-use under concurrency#28752BobbyHo wants to merge 1 commit into
BobbyHo wants to merge 1 commit into
Conversation
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.scope.The fix
:execwith no affected-rows check, so the request that lost the race deleted nothing and minted anyway.DeleteAPIKeyByIDReturningRowis the sameDELETE ... RETURNING *shape as its code-side sibling: a delete that removed nothing surfacessql.ErrNoRows.RETURNING *becausefetchAndQueryneeds anrbac.Objecter.refreshTokenGrantmaps that toerrBadToken, theinvalid_grantit already returns for a token it cannot find, per RFC 6749 §10.5.DeleteAPIKeyByIDcall sites are untouched, includingauthorizationCodeGrant's previous-key delete, where the code delete already arbitrates single use and a returning-row delete would imply otherwise.Tests
TestOAuth2RefreshSingleUseraces two refreshes on one barrier and requires exactly one 200 and one 400invalid_grant. A sequential pair passes pre-fix, so the race is the test; the barrier shape is now shared withTestOAuth2TokenExchangeSingleUseasrequireExactlyOneMinted.APIKeysubtest in the existingTestSingleUseDeleteByIDReturningRowpins the second delete returningsql.ErrNoRows.MethodTestSuitefails on any untesteddatabase.Storemethod, soDeleteAPIKeyByIDReturningRowgets a case alongsideDeleteAPIKeyByID.Stack: #28237, #28740, #28744, #28751, this PR.