Skip to content

fix(coderd/oauth2provider): answer invalid_grant when a refreshed token's key is gone - #28753

Draft
BobbyHo wants to merge 1 commit into
plat481-2-single-use-refreshfrom
plat481-3-revoked-token-error-class
Draft

fix(coderd/oauth2provider): answer invalid_grant when a refreshed token's key is gone#28753
BobbyHo wants to merge 1 commit into
plat481-2-single-use-refreshfrom
plat481-3-revoked-token-error-class

Conversation

@BobbyHo

@BobbyHo BobbyHo commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Last of three in PLAT-481, which closes PLAT-470. Refreshing a revoked token returned HTTP 500. refreshTokenGrant reads the api_keys row its token hangs off and returned that lookup's error raw, so sql.ErrNoRows fell past the sentinel dispatch to the generic handler, while every other missing row in the same function already answers invalid_grant.

PR What it does
#28751 A refresh may name a narrower scope.
#28752 Refresh token redemption is single-use under concurrency.
this A revoked token refreshes as invalid_grant, and the token endpoint's scope rejections stop emitting " and \.

The error class (FR12, AC13, AC14)

  • Seven lines: sql.ErrNoRows from GetAPIKeyByID becomes errBadToken.
  • The two revocation paths a client can reach both passed before this. Deleting the API key, and deleting the app secret, each cascade the oauth2_provider_app_tokens row away, so the prefix lookup answers errBadToken first. 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.
  • The case that moves is a token row whose api_key_id names no key. The FK cascade makes that unreachable through any API, so TestOAuth2RefreshKeyMissing disables the constraints to seed it, and takes a database of its own because disabling them applies to every table. 500 before, 400 after.
  • FR12 names a third path, deleting the app. It never reaches the grant: the client_id no longer resolves, so ExtractOAuth2ProviderAppWithOAuth2Errors answers 401 invalid_client first. AppDeleted pins that, and the spec is corrected.

The charset (spec.md §19.2 finding 5)

  • RFC 6749 §5.2 and OAuth 2.1 §3.2.4 restrict error_description to NQSCHAR, which excludes " (0x22) and \ (0x5C). %q emits both, and Tokens() passes err.Error() straight to WriteOAuth2Error, which does no rewriting.
  • Three sites switch to the '%s' form the newer sentinels already use: errNoGrantableScope and errStaleScope in scopeStillCoveredByAllowlist, and errCoverageUndecidable in the shared firstScopeNotCovered.
  • The authorize-only rejections keep %q. fix: deliver three more authorize errors to the client #28450 adds sanitizeErrorDescription on that side, which is where that endpoint's fix belongs; lifting it into WriteOAuth2Error for every endpoint is its own ticket.
  • TestOAuth2TokenScopeErrorCharset drives 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.

…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.
@linear-code

linear-code Bot commented Aug 29, 2026

Copy link
Copy Markdown

PLAT-470

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