fix: make OAuth2 refresh token redemption single-use under concurrency - #28752
fix: make OAuth2 refresh token redemption single-use under concurrency#28752BobbyHo wants to merge 14 commits into
Conversation
BobbyHo
changed the title
717e257 to
3a3ed82
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 22 findings (3 P2, 11 P3, 2 P4, 4 Nit, 2 Note), COMMENT. Review Finding inventoryFinding inventory - PR #28752Findings
Contested and acknowledgedCRF-1 (P2, tokens.go:742) - deferred to #28753
CRF-5 (Note, apikeys.sql:95) - acknowledged
CRF-1 (P2, deferred to #28753) - panel re-examination (R2)
Round logRound 1Netero-only (pre-panel gate). P2 present, so panel deferred per Netero decision gate. 1 P2, 1 P3, 1 Nit, 2 Notes. Reviewed against 755b5a1..3a3ed82. P2 verified empirically by orchestrator against tokens.go:693-696 read path. Round 2Churn guard PROCEED: 3 addressed (CRF-2 1857da1, CRF-3 cb8b32f, CRF-4 6bebd3c), 1 acknowledged (CRF-5), 1 deferred (CRF-1 -> #28753, fix verified at 5a5e2da). First panel round (round 1 was Netero-only). Reviewed against 755b5a1..6bebd3c. Netero round 2: highest new finding P3 (CRF-6), so panel proceeds. New: CRF-6 (P3), CRF-7 (Note), CRF-8 (Note). About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review only. These are mechanical findings from Netero; the full review panel has not yet reviewed this PR and will do so after these are addressed. Netero found a P2 that gates the panel.
The fix is well-shaped: it mirrors the merged code-side precedent (#28744), the DELETE ... RETURNING shape maps cleanly to sql.ErrNoRows -> errBadToken -> invalid_grant, the seven unrelated DeleteAPIKeyByID call sites are left alone, and the test density is high (87 test lines against 19 production). The regression test genuinely fails against the pre-fix code (10/10 with DeleteAPIKeyByID).
The one blocker: the delete arbitrates the race, but the loser can lose one read earlier at the prevKey fetch, where the same "token already spent" condition surfaces as HTTP 500 rather than invalid_grant, and can flake the new test. Netero put it plainly: "Verified, not reasoned."
Severity count: 1 P2, 1 P3, 1 Nit, 2 Notes.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
0d45b6d to
f499e46
Compare
…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.
…he exchange test TestOAuth2TokenExchangeSingleUse kept its own copy of the harness that requireExactlyOneMinted factors out: same result struct, same goroutine and channel pair, same 200/400 counting. The only thing keeping it inline was the winner response it hands to requireTokenAuthenticates, so return that from the helper and let both callers share one definition of the single-use contract.
… not assign it Inside the InTx closure the delete assigned the captured outer err while the InsertAPIKey below it declares a fresh one, so err named two variables six lines apart. No behaviour change: the outer err is overwritten by the InTx assignment before anything reads it. Declaring here matches the sibling delete in authorizationCodeGrant.
The refresh test released both goroutines from a WaitGroup and relied on them overlapping. When they serialised, the loser was refused at the token read and the test passed without the delete ever arbitrating, so its coverage was measured rather than guaranteed. Give barrierStore a hold on GetOAuth2ProviderAppTokenByPrefix, the read refreshTokenGrant makes before its transaction, so both refreshes are released with the same view and the delete is what decides. The two holds are separate groups because a nil group leaves that read alone, which keeps the token hold off the code read the test makes while seeding. Verified: 50/50 pass under -race, and 10/10 fail against the non-arbitrating delete this test exists to catch.
… a transaction The delete comment described the race in terms of winners and losers without saying what the transaction buys. Say what actually happens: the second refresh blocks on the row until the first commits, then deletes nothing. Grouping the delete with the inserts is what makes that answer correct, since the loser is refused only if the winner really minted and a failure below puts the old key back. Drop the four test comments that restated their own code.
f499e46 to
81bad9e
Compare
There was a problem hiding this comment.
Panel round (round 1 was a Netero-only first pass). The core change is correct and unusually well-tested: the delete-arbitrates shape matches the merged code-side precedent line for line, the dbauthz swap is authorization-equivalent, and about ten reviewers independently confirmed the race test fails 10/10 against the pre-fix blind delete and passes 100-200/200 with it, with the new errBadToken branch covered. The barrier-at-the-token-read design and the value-returning requireExactlyOneMinted are the right shape for a concurrency test. Hisoka summed up the review's best find: "a security control that answers 200 while doing nothing is the shape I look for."
Prior round: CRF-2, CRF-3, CRF-4 verified fixed; CRF-5 accepted (with one correction, see below).
The panel's central concern is a framing one. The description says "the seven other DeleteAPIKeyByID call sites are untouched" and treats them as one safe class. They are not one class. Two of them contend for the same (user, app) session-key row this PR now arbitrates: the exchange's prev-key delete (CRF-9, tokens.go:601) and revocation (CRF-10, revoke.go:150). Both are blind deletes that treat a zero-row delete as success, so a refresh racing either leaves two live sessions (CRF-9, proven by two reviewers) or lets revocation report 200 while the rotated credential stays live (CRF-10, analytical). Neither is a regression this PR introduces, but the PR looked directly at tokens.go:601 and recorded "the code delete already arbitrates single use" against it, and that reasoning answers a different question (it arbitrates the code, not the key). At minimum the description's claim needs correcting; the fixes need migrations, so whether to fix here or file a ticket is a human decision. Do not accept these as permanent silently.
CRF-1 (deferred to #28753): the panel's disposition is that the deferral is acceptable only if the stack merges in order. Merged alone this PR answers HTTP 500 (not invalid_grant) on the prevKey-read interleaving, and Komugi forced that interleaving to hard-fail the test this PR adds, so merging before #28753 lands a flaky test on main. Meruem offers a subtractive in-PR alternative (CRF-13): the prevKey read at 693 is redundant with dbToken, and removing it collapses both the 500 window and the CRF-8 fetch window into the single arbitrating delete. Recommend pulling #28753's one-branch fix in or applying CRF-13, or confirm the stack merges atomically.
Three process/description items (no code change to this diff): the commit body and PR description cite RFC 6749 §10.5 (Authorization Codes) for refresh single-use; refresh tokens are §10.4, which is the stronger justification (rotation so replay is detectable), and the squash body inherits the wrong citation (CRF-23). The description names a test TestSingleUseDeleteByIDReturningRow that does not exist; it is TestSingleUseDelete (CRF-24). And the fix changes behaviour for a client that races itself (two working sessions become one invalid_grant): spec-correct, but worth a release-note line (CRF-25).
Pre-existing and out of this PR's scope, surfaced for tickets rather than to block: the unindexed api_key_id FK cascade (CRF-16, measured 197x on the hot path this PR creates), the refresh-vs-revoke deadlock (CRF-17), the missing confidential-client auth on refresh (CRF-18, a documented gap from #27712), and api_keys purge cascading refresh tokens away before their stated expiry (CRF-19).
Severity count (new this round): 2 P2, 10 P3, 2 P4, 4 Nit, 1 Note, plus CRF-1 re-raised. Ging-go found nothing.
coderd/oauth2provider/tokens.go:601
P2 [CRF-9] The exchange's blind prev-key delete arbitrates code redemption, not the shared (user, app) session key that both grants now contend for. (Kurapika P2, Takumi P3, Knuckle P3, Pariston P3)
The PR excludes this site because "the code delete already arbitrates single use." The code delete arbitrates the oauth2_provider_app_codes row; it does not serialize the api_keys row, and an exchange and a refresh (or two exchanges) for the same user and app race that row with only one side checking affected rows. Kurapika and Takumi both proved the interleaving leaves two live api_keys/tokens with the same token_name:
live key YtE0Qpmt2p name="…_oauth_session_token"
live key qUIqgIhkCq name="…_oauth_session_token"
No constraint catches it: idx_api_key_name is UNIQUE (user_id, token_name) WHERE login_type = 'token' and these keys are oauth2_provider_app. Consequence (Kurapika): a stolen refresh token survives the user re-consenting, and RFC 7009 revocation by the presented token reaches only one of the two. The full fix is a partial unique index plus mapping the 23505 (migration; the PR says it has none), so this is a human decision: fix here or file a ticket. Either way, correct the description's "already arbitrates" claim, which is false.
🤖
coderd/oauth2provider/revoke.go:150
P3 [CRF-10] revokeRefreshTokenInTx deletes the shared session key blind and treats a zero-row delete as success, so a revoke racing a refresh reports 200 while the rotated credential stays live. (Hisoka P2)
err = db.DeleteAPIKeyByID(dbauthz.AsSystemOAuth2(ctx), dbToken.APIKeyID)
if err != nil && !errors.Is(err, sql.ErrNoRows) {
return xerrors.Errorf("delete api key: %w", err)
}
return nilRevoke reads the token (K1); a concurrent refresh deletes K1, inserts K2/RT2, commits; revoke's delete removes nothing, the ErrNoRows branch swallows it, and RevokeToken answers 200. The client is told the grant is gone; K2 is live for its full lifetime and RT2 rotates forever (RFC 7009 §2.1). Hisoka rates this P2 but did not land the probe ("the window is four lines wide"); I am recording it as P3 pending verification and because it is pre-existing and outside this diff. This is the same class as CRF-9 and contradicts the description's treatment of the seven call sites as one safe class. Verify and fix, or file a ticket.
🤖
coderd/oauth2provider/revoke.go:229
P3 [CRF-17] Refresh and revoke take opposite lock orders on the same row pair, so a refresh racing a revoke deadlocks and returns 500. (Knuckle P3)
Refresh locks the api_keys row then the token via cascade (key→token); RevokeApp (DeleteOAuth2ProviderAppTokensByAppAndUserID) locks the token row then the key via trigger_delete_oauth2_provider_app_token (token→key). Knuckle reproduced 39 deadlocks racing 20k key deletes against 20k token deletes over the same ids; the victim gets SQLSTATE 40P01, which IsSerializedError does not match and InTx does not retry, so the refresh returns 500 (or the revoke silently no-ops) after up to deadlock_timeout. Pre-existing; reported because the PR's contract is that a concurrent redemption answers invalid_grant, and this is the pair that answers 500. Cheapest fix: make RevokeApp delete keys and let the cascade take the tokens, putting every path on key→token order. Ticket.
🤖
coderd/oauth2provider/tokens.go:644
P4 [CRF-18] refreshTokenGrant authenticates no client, so a confidential app's refresh token is redeemable by anyone holding the token and the public client_id. (Kurapika P3)
extractTokenRequest requires client_secret only for authorization_code, and refreshTokenGrant has no equivalent to authorizationCodeGrant's secret check for non-public apps (RFC 6749 §6). Kurapika verified a refresh with the secret removed returns 200 for a confidential app. The dbToken.AppID != app.ID binding stops cross-app replay, so this is not takeover on its own, but a refresh token from storage, a proxy log, or a crash dump is a complete credential and a confidential app is no safer than a public one here. Pre-existing and documented as a known gap from #27712 review; rated P4 as it predates this PR and is tracked. Surfaced because this is the exact endpoint the PR hardens.
🤖
coderd/database/dbpurge/dbpurge.go:249
P4 [CRF-19] api_keys purging cascades to oauth2_provider_app_tokens, so a refresh token dies at access-key expiry plus retention, not at its own expiry. (Mafuuu P4)
--default-oauth-refresh-lifetime defaults to 30 days and Validate enforces it strictly greater than the 24h session duration, so the design intends the refresh token to outlive its API key. But DeleteExpiredAPIKeys deletes every api_keys row past expires_at - retention (retention default 7 days) with no login_type filter, and the FK cascade drops the token. So an OAuth2 session idle ~8 days loses its refresh token 22 days early, and the next refresh answers invalid_grant. Pre-existing and outside this diff; filed because nothing else will. Either exclude keys still referenced by a live token row, or correct the flag's help text. Ticket.
🤖
coderd/database/queries/apikeys.sql:89
Nit [CRF-20] DeleteAPIKeyByID :exec carries no comment marking it as the twin that cannot arbitrate. (Zoro, Gon)
CRF-5 (the ReturningRow name) was accepted on the grounds that renaming touches seven call sites for no behaviour change. A comment on line 89 touches nothing and retires the same risk: -- Reports nothing when the row is absent. Use DeleteAPIKeyByIDReturningRow to arbitrate single use. The merged precedent had exactly this pointer before it was folded away. Note also that the recorded CRF-5 rationale is slightly off: converting this query to :one in place would be a behaviour change at apikey.go:419 and userauth.go:717,2056, which today treat a zero-row delete as success.
🤖
coderd/oauth2provider/tokens_test.go:557
Nit [CRF-22] The refusal assertion substring-matches the raw body instead of the file's own error helper. (Chopper)
require.Contains(t, result.body, string(codersdk.OAuth2ErrorCodeInvalidGrant)) accepts the code anywhere in the response, including inside error_description. requireTokenGrantError unmarshals codersdk.OAuth2Error and asserts oauthErr.Error equals the code; the sibling replay test uses it. This PR extends the pattern to a grant whose handler maps five distinct conditions to invalid_grant. Not a live bug (both goroutines post the identical form), hence Nit; swap for requireTokenGrantError(t, result.status, result.body).
🤖
🤖 This review was automatically generated with Coder Agents.
…e race refreshTokenGrant read the api_keys row before deleting it, for the user id. That row has carried nothing the token row lacks since migration 346 denormalized user_id onto oauth2_provider_app_tokens, and the read sat in the one window where a refresh that lost the race answered 500: the winner's cascade removed the key between the token read and this lookup, and the resulting sql.ErrNoRows was unmapped. Drop the read and take the user id and key id from the token row. The returning-row delete is now the first statement to touch the key, so a lost race can only surface there, where it already maps to invalid_grant.
…y is gone A refresh presented for a revoked token must answer invalid_grant, not a server fault. The two revocation paths a client can reach, deleting the API key and deleting the app secret, both cascade the token row away, so the prefix lookup refuses them; deleting the app never reaches the grant because the client_id stops resolving. All three are pinned as responses. The case that used to answer HTTP 500 is a token row whose api_key_id names no key. The refresh no longer reads that key before deleting it, so the returning-row delete finds nothing and answers invalid_grant. The FK cascade makes the row unreachable through any API, so its test disables the constraints to seed one.
CHECK (scope <> '') admits a whitespace-only scope, which scopeStringToAPIKeyScopes echoed into error_description as an empty pair of quotes. There is no name to report, so the rejection carries a fixed message instead. Tests widen the whitespace table and pin that the message does not vary with the value it rejected, and cover an unmintable stored scope reached through a refresh as well as through an authorization code.
…etes Both grants arbitrate a race with a delete that removes nothing when it loses, and map that sql.ErrNoRows to invalid_grant. That zero-row answer is READ COMMITTED behavior: under REPEATABLE READ or above the same delete raises a serialization failure, which nothing here maps and InTx does not retry, so every lost race would answer 500. The transactions were opened with nil options, which sends a bare BEGIN and inherits default_transaction_isolation from the server, database, or role. Name the level at both call sites so the dependency is visible and a raised server default cannot change the response.
…token The single-use delete finding nothing is the one place the server can see that a code or refresh token was presented twice. RFC 6749 §10.4 rotates refresh tokens for exactly this reason, so log it at warn with the app and the row involved. The client still receives the generic invalid_grant. Also cite §10.4 rather than §10.5 for the refresh delete, and shorten the comments around it.
… as its code sibling The only caller reaches it through a fetch-then-query wrapper, so "instead of reading first" was wrong, and the isolation note the sibling carries applies here too.
…sql.ErrNoRows The OAuth2 grants map that error to invalid_grant, so the wrapping in fetchAndQuery decides whether a refused single-use delete answers 400 or 500. Neither method suite case covered the miss.
…epted token The barrier was a WaitGroup sized in advance: one arrival short hung the package until the go test timeout, one too many panicked. It now releases on a closed channel or the request context, and the tests assert the arrival count instead. The refresh race seeded a narrowed scope copied from the scope tests, so the accepted token could not be checked against /users/me. Seed it unnarrowed, check the accepted token authenticates, and check the presented refresh token's row is gone. Assertion messages now say what is being asserted.
1 participant
TL;DR
Second and last in PLAT-481, which closes PLAT-470. 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. It also absorbs what was #28753: the revoked-token refresh is pinned asinvalid_grant, and a stored scope that names nothing stops being echoed back.scope.invalid_grant. A stored scope that names nothing stops being echoed back.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 (RFC 6749 §5.2). RFC 6749 §10.4 is why the presented token is invalidated at all: a second use of it must be detectable, which holds only if exactly one refresh succeeds. Both grants now log that refusal at warn with the app and the row involved; the client still receives the genericinvalid_grant.refreshTokenGrantno longer reads the previousapi_keysrow before deleting it. The token row has carrieduser_idsince migration 346, so the read supplied nothing, and it sat in the one window where a refresh that lost the race still answered 500: the winner's cascade removed the key between the token read and this lookup. The returning-row delete is now the first statement to touch the key, so a lost race can only surface where it already maps toinvalid_grant.READ COMMITTEDinstead of inheritingdefault_transaction_isolation. The zero-row delete the race relies on is READ COMMITTED behavior; under REPEATABLE READ or above the same delete raises a serialization failure that nothing maps andInTxdoes not retry, so a raised server default would have turned every lost race into a 500.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.The revoked-token error class (FR12, AC13, AC14)
GetAPIKeyByIDread returned itssql.ErrNoRowsraw and it fell past the sentinel dispatch to the generic handler. Removing the read is the fix; the delete already answersinvalid_grantwhen it finds nothing.oauth2_provider_app_tokensrow away, so the prefix lookup answerserrBadTokenfirst. Deleting the API key and deleting the app secret are pinned anyway: the property a client depends on is the response, not which statement notices it, and the cascades that make them pass are schema this function does not control.api_key_idnames no key. The FK cascade makes that unreachable through any API, soTestOAuth2RefreshKeyMissingdisables the constraints to seed it, and takes a database of its own because disabling them applies to every table. It now reaches the delete and answers 400.client_idno longer resolves, soExtractOAuth2ProviderAppWithOAuth2Errorsanswers 401invalid_clientfirst.AppDeletedpins that, and the spec is corrected.The scope that names nothing
CHECK (scope <> '')admits a whitespace-only scope, andscopeStringToAPIKeyScopesechoed it intoerror_descriptionas an empty pair of quotes. There is no name to report, so the rejection carries a fixed message instead.StoredScopeOutsideEnumRejectedOnRefreshcovers an unmintable stored scope reached through a refresh rather than through an authorization code, which is the likelier way a name dropped from the enum surfaces: a grant outlives the code that issued it.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 shared withTestOAuth2TokenExchangeSingleUseasrequireExactlyOneAccepted. Both tests check the accepted token authenticates and that exactly two requests reached the barriered read; the refresh test also checks the presented refresh token's row is gone.TestSingleUseDeleteNotFoundin dbauthz pins that a fetch miss in the fetch-then-query wrapper still matchessql.ErrNoRowsand never reaches the delete. That wrapping is what makes a refused single-use delete answerinvalid_grantrather than 500.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.