fix(coderd/oauth2provider): answer invalid_grant when a refreshed token's key is gone - #28753
Draft
BobbyHo wants to merge 1 commit into
Draft
fix(coderd/oauth2provider): answer invalid_grant when a refreshed token's key is gone#28753BobbyHo wants to merge 1 commit into
BobbyHo wants to merge 1 commit into
Conversation
…en's key is gone refreshTokenGrant reads the api_keys row its refresh token hangs off and returned that lookup's error raw. A revoked token leaves no row, so the sql.ErrNoRows fell past the sentinel dispatch to the generic handler and a client that presented a revoked token got HTTP 500 instead of the RFC 6749 invalid_grant every other missing row in this function answers. Mapping it to errBadToken needs no new sentinel or dispatch case. The two revocation paths a client can reach both cascade the token row away before this lookup runs, so the case that moves is a token row whose api_key_id names no key; its test disables the constraints to seed one. Also switches the three scope rejections the token endpoint renders with %q to single quotes. RFC 6749 §5.2 and OAuth 2.1 §3.2.4 exclude " and \ from error_description, and %q emits both. The authorize-only rejections keep %q, which #28450 covers with a sanitizer on the way out.
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
Last of three in PLAT-481, which closes PLAT-470. Refreshing a revoked token returned HTTP 500.
refreshTokenGrantreads theapi_keysrow its token hangs off and returned that lookup's error raw, sosql.ErrNoRowsfell past the sentinel dispatch to the generic handler, while every other missing row in the same function already answersinvalid_grant.scope.invalid_grant, and the token endpoint's scope rejections stop emitting"and\.The error class (FR12, AC13, AC14)
sql.ErrNoRowsfromGetAPIKeyByIDbecomeserrBadToken.oauth2_provider_app_tokensrow away, so the prefix lookup answerserrBadTokenfirst. Their tests are here 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. 500 before, 400 after.client_idno longer resolves, soExtractOAuth2ProviderAppWithOAuth2Errorsanswers 401invalid_clientfirst.AppDeletedpins that, and the spec is corrected.The charset (
spec.md§19.2 finding 5)error_descriptionto NQSCHAR, which excludes"(0x22) and\(0x5C).%qemits both, andTokens()passeserr.Error()straight toWriteOAuth2Error, which does no rewriting.'%s'form the newer sentinels already use:errNoGrantableScopeanderrStaleScopeinscopeStillCoveredByAllowlist, anderrCoverageUndecidablein the sharedfirstScopeNotCovered.%q. fix: deliver three more authorize errors to the client #28450 addssanitizeErrorDescriptionon that side, which is where that endpoint's fix belongs; lifting it intoWriteOAuth2Errorfor every endpoint is its own ticket.TestOAuth2TokenScopeErrorCharsetdrives all six scope rejections the token endpoint can produce and requires neither character in the description. Three of the six fail without the switch, which is what makes the table worth its length.Stack: #28237, #28740, #28744, #28751, #28752, this PR.