From 1e116f059512107060c0b6b097f987d156edacd2 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Sat, 29 Aug 2026 04:23:59 +0000 Subject: [PATCH] fix(coderd/oauth2provider): answer invalid_grant when a refreshed token's key is gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- coderd/oauth2provider/authorize.go | 8 +- .../oauth2provider/authorize_internal_test.go | 12 +- coderd/oauth2provider/tokens.go | 10 +- coderd/oauth2provider/tokens_test.go | 198 ++++++++++++++++++ 4 files changed, 218 insertions(+), 10 deletions(-) diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 93ec023693810..3bf738d7579d7 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -29,8 +29,10 @@ import ( ) // Rejection reasons from scope negotiation, rendered into error_description. -// Each is wrapped as `%q: %w` with the offending value: xerrors repeats the -// sentinel's own text unless %w is the final verb. +// Each names the offending value ahead of `%w`: xerrors repeats the sentinel's +// own text unless %w is the final verb. Anything the token endpoint can render +// is quoted with '%s' rather than %q, since RFC 6749 §5.2 excludes " and \ +// from that parameter. var ( // A requested name outside the external scope catalog: unrecognized, or // recognized but internal-only. @@ -108,7 +110,7 @@ func firstScopeNotCovered(ctx context.Context, logger slog.Logger, app database. slog.F("app_id", app.ID.String()), slog.F("ceiling", strings.Join(ceiling, " ")), slog.F("scope", s)) - return "", xerrors.Errorf("%q: %w", s, errCoverageUndecidable) + return "", xerrors.Errorf("'%s': %w", s, errCoverageUndecidable) } if !covered { return s, nil diff --git a/coderd/oauth2provider/authorize_internal_test.go b/coderd/oauth2provider/authorize_internal_test.go index 7a9c5c7e13bc1..c4df0e7af3576 100644 --- a/coderd/oauth2provider/authorize_internal_test.go +++ b/coderd/oauth2provider/authorize_internal_test.go @@ -251,11 +251,13 @@ func requirePersistableScope(t *testing.T, scope string) { // Rejection reasons for the package's black-box tests, which cannot reach the // sentinels. var ( - ReasonUnknownScope = errUnknownScope.Error() - ReasonNoGrantableScope = errNoGrantableScope.Error() - ReasonScopeNotAllowed = errScopeNotAllowed.Error() - ReasonStaleScope = errStaleScope.Error() - ReasonScopeNotGranted = errScopeNotGranted.Error() + ReasonUnknownScope = errUnknownScope.Error() + ReasonNoGrantableScope = errNoGrantableScope.Error() + ReasonScopeNotAllowed = errScopeNotAllowed.Error() + ReasonStaleScope = errStaleScope.Error() + ReasonScopeNotGranted = errScopeNotGranted.Error() + ReasonUnmintableScope = errUnmintableScope.Error() + ReasonCoverageUndecidable = errCoverageUndecidable.Error() ) func TestNoScopeAllowlist(t *testing.T) { diff --git a/coderd/oauth2provider/tokens.go b/coderd/oauth2provider/tokens.go index 1e4f1919f2bd4..7b118fec871de 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -67,7 +67,7 @@ func scopeStillCoveredByAllowlist(ctx context.Context, logger slog.Logger, app d allowlist := grantableScopes(app.Scope.String) if len(allowlist) == 0 { // Named verbatim for the same reason as in negotiateScope. - return xerrors.Errorf("%q: %w", app.Scope.String, errNoGrantableScope) + return xerrors.Errorf("'%s': %w", app.Scope.String, errNoGrantableScope) } // Canonicalized rather than assumed: the row may have been written by @@ -77,7 +77,7 @@ func scopeStillCoveredByAllowlist(ctx context.Context, logger slog.Logger, app d return err } if outside != "" { - return xerrors.Errorf("%q: %w", outside, errStaleScope) + return xerrors.Errorf("'%s': %w", outside, errStaleScope) } return nil } @@ -629,6 +629,12 @@ func refreshTokenGrant(ctx context.Context, db database.Store, logger slog.Logge // Grab the user roles so we can perform the refresh as the user. //nolint:gocritic // OAuth2 system context, need to read the previous API key prevKey, err := db.GetAPIKeyByID(dbauthz.AsSystemOAuth2(ctx), dbToken.APIKeyID) + // A revoked token: the key is gone and the refresh cannot mint from it. The + // other lookups in this function already answer errBadToken, and without + // this one a revocation reads as a server fault. + if errors.Is(err, sql.ErrNoRows) { + return codersdk.OAuth2TokenResponse{}, errBadToken + } if err != nil { return codersdk.OAuth2TokenResponse{}, err } diff --git a/coderd/oauth2provider/tokens_test.go b/coderd/oauth2provider/tokens_test.go index 752c574cd3b50..a9f3df67acfea 100644 --- a/coderd/oauth2provider/tokens_test.go +++ b/coderd/oauth2provider/tokens_test.go @@ -361,6 +361,196 @@ func requireExactlyOneMinted(ctx context.Context, t *testing.T, client *codersdk require.Equal(t, 1, rejected) } +// A revoked token must refresh as invalid_grant rather than as a server fault. +// Both cases here pass before the GetAPIKeyByID mapping, because deleting +// either row cascades the token row away and the prefix lookup answers first. +// They stay because the property a client depends on is the response, not which +// statement notices, and the cascades that make them pass are schema the +// refresh does not control. +func TestOAuth2RefreshRevokedToken(t *testing.T) { + t.Parallel() + + db, pubsub := dbtestutil.NewDB(t) + client := coderdtest.New(t, &coderdtest.Options{ + Database: db, + Pubsub: pubsub, + }) + owner := coderdtest.CreateFirstUser(t, client) + + t.Run("KeyDeleted", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + code, verifier := authorizeCode(ctx, t, client, app.ID.String(), "workspace:ssh") + token := exchangeCode(ctx, t, client, app, code, verifier) + + keyID := tokenRow(ctx, t, db, token.RefreshToken).APIKeyID + require.NoError(t, client.DeleteAPIKey(ctx, owner.UserID.String(), keyID)) + + status, body := postTokenRequest(ctx, t, client, refreshForm(app, token.RefreshToken)) + requireTokenGrantError(t, status, body) + }) + + t.Run("AppSecretDeleted", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + code, verifier := authorizeCode(ctx, t, client, app.ID.String(), "workspace:ssh") + token := exchangeCode(ctx, t, client, app, code, verifier) + + require.NoError(t, client.DeleteOAuth2ProviderAppSecret(ctx, app.ID, app.SecretID)) + + status, body := postTokenRequest(ctx, t, client, refreshForm(app, token.RefreshToken)) + requireTokenGrantError(t, status, body) + }) + + // The third revocation path FR12 names. It never reaches the grant: the + // client_id no longer resolves, so authentication refuses first. + t.Run("AppDeleted", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + code, verifier := authorizeCode(ctx, t, client, app.ID.String(), "workspace:ssh") + token := exchangeCode(ctx, t, client, app, code, verifier) + + require.NoError(t, client.DeleteOAuth2ProviderApp(ctx, app.ID)) + + status, body := postTokenRequest(ctx, t, client, refreshForm(app, token.RefreshToken)) + require.Equal(t, http.StatusUnauthorized, status, body) + require.Contains(t, body, string(codersdk.OAuth2ErrorCodeInvalidClient), body) + }) +} + +// The case the mapping actually moves: a token row whose api_key_id names no +// key. The FK cascade makes that unreachable through any API, so the +// constraints come off to seed it, and this test takes a database of its own +// because disabling them applies to every table in it. Without the mapping the +// refresh answers HTTP 500. +func TestOAuth2RefreshKeyMissing(t *testing.T) { + t.Parallel() + + db, pubsub := dbtestutil.NewDB(t) + client := coderdtest.New(t, &coderdtest.Options{ + Database: db, + Pubsub: pubsub, + }) + owner := coderdtest.CreateFirstUser(t, client) + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + refreshToken := seedRefreshToken(ctx, t, db, app, owner.UserID, "workspace:ssh") + + dbtestutil.DisableForeignKeysAndTriggers(t, db) + require.NoError(t, db.DeleteAPIKeyByID(dbauthz.AsSystemRestricted(ctx), + tokenRow(ctx, t, db, refreshToken).APIKeyID)) + + status, body := postTokenRequest(ctx, t, client, refreshForm(app, refreshToken)) + requireTokenGrantError(t, status, body) +} + +// RFC 6749 §5.2 excludes " and \ from error_description, and %q emits both. +// Every scope rejection names the offending value, so each is a place they can +// return. +func TestOAuth2TokenScopeErrorCharset(t *testing.T) { + t.Parallel() + + db, pubsub := dbtestutil.NewDB(t) + client := coderdtest.New(t, &coderdtest.Options{ + Database: db, + Pubsub: pubsub, + }) + owner := coderdtest.CreateFirstUser(t, client) + + cases := []struct { + name string + reason string + reject func(ctx context.Context, t *testing.T) (int, string) + }{ + { + name: "UnmintableScope", + reason: oauth2provider.ReasonUnmintableScope, + reject: func(ctx context.Context, t *testing.T) (int, string) { + app := seedAppWithSecret(t, db, sql.NullString{}) + verifier, challenge := oauth2providertest.GeneratePKCE(t) + code := seedCode(ctx, t, db, app.ID, owner.UserID, challenge, scopeOutOfCatalog) + return postTokenRequest(ctx, t, client, tokenExchangeForm(app, code, verifier)) + }, + }, + { + name: "StaleScope", + reason: oauth2provider.ReasonStaleScope, + reject: func(ctx context.Context, t *testing.T) (int, string) { + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + code, verifier := authorizeCode(ctx, t, client, app.ID.String(), "workspace:ssh") + setAppAllowlist(ctx, t, db, app, sql.NullString{String: scopeAlsoInCatalog, Valid: true}) + return postTokenRequest(ctx, t, client, tokenExchangeForm(app, code, verifier)) + }, + }, + { + name: "NoGrantableScope", + reason: oauth2provider.ReasonNoGrantableScope, + reject: func(ctx context.Context, t *testing.T) (int, string) { + app := seedAppWithSecret(t, db, sql.NullString{String: scopeOutOfCatalog, Valid: true}) + verifier, challenge := oauth2providertest.GeneratePKCE(t) + code := seedCode(ctx, t, db, app.ID, owner.UserID, challenge, "workspace:ssh") + return postTokenRequest(ctx, t, client, tokenExchangeForm(app, code, verifier)) + }, + }, + { + name: "CoverageUndecidable", + reason: oauth2provider.ReasonCoverageUndecidable, + reject: func(ctx context.Context, t *testing.T) (int, string) { + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + verifier, challenge := oauth2providertest.GeneratePKCE(t) + // RBAC cannot expand the stored name, so the comparison against + // the allowlist fails rather than returning uncovered. + code := seedCode(ctx, t, db, app.ID, owner.UserID, challenge, scopeOutOfCatalog) + return postTokenRequest(ctx, t, client, tokenExchangeForm(app, code, verifier)) + }, + }, + { + name: "UnknownScope", + reason: oauth2provider.ReasonUnknownScope, + reject: func(ctx context.Context, t *testing.T) (int, string) { + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + code, verifier := authorizeCode(ctx, t, client, app.ID.String(), "workspace:ssh") + token := exchangeCode(ctx, t, client, app, code, verifier) + form := refreshForm(app, token.RefreshToken) + form.Set("scope", scopeOutOfCatalog) + return postTokenRequest(ctx, t, client, form) + }, + }, + { + name: "ScopeNotGranted", + reason: oauth2provider.ReasonScopeNotGranted, + reject: func(ctx context.Context, t *testing.T) (int, string) { + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + code, verifier := authorizeCode(ctx, t, client, app.ID.String(), "workspace:ssh") + token := exchangeCode(ctx, t, client, app, code, verifier) + form := refreshForm(app, token.RefreshToken) + form.Set("scope", scopeAlsoInCatalog) + return postTokenRequest(ctx, t, client, form) + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + status, body := tc.reject(ctx, t) + description := requireTokenScopeError(t, status, body) + require.Contains(t, description, tc.reason) + require.NotContains(t, description, `"`) + require.NotContains(t, description, `\`) + }) + } +} + // appWithSecret is seeded directly because the management API registers no // scope allowlist, and the allowlist is what these tests turn. type appWithSecret struct { @@ -549,6 +739,14 @@ func requireTokenResponse(t *testing.T, status int, body string) codersdk.OAuth2 return token } +// requireTokenGrantError asserts an RFC 6749 §5.2 invalid_grant response. +func requireTokenGrantError(t *testing.T, status int, body string) { + t.Helper() + + require.Equal(t, http.StatusBadRequest, status, body) + require.Contains(t, body, string(codersdk.OAuth2ErrorCodeInvalidGrant), body) +} + // requireTokenScopeError asserts an RFC 6749 §5.2 invalid_scope response and // returns its description. func requireTokenScopeError(t *testing.T, status int, body string) string {