fix(oauth2): enforce the refresh-token rotation the profile requires by euskadi31 · Pull Request #89 · hyperscale-stack/security

euskadi31 · 2026-10-10T00:10:36Z

Closes #78.

Problem

Profile20BCP (the zero value, hence the default) and Profile21Draft
advertise mandatory refresh-token rotation, and
Profile.RequiresRefreshRotation() says so, but nothing enforced it: its
only caller was a test. The refresh_token grant rotated only when both the
flag and a generator were set:

if !g.cfg.RotateRefreshTokens || g.cfg.RefreshTokens == nil {
	return resp, nil
}

With the zero configuration a refresh therefore returned a new access token,
no replacement refresh token, and left the presented token live: it could be
replayed until it expired, under the very profile that promises rotation
and reuse detection (RFC 9700 §4.14.2). TestRefreshTokenWithoutRotation
codified the behavior (it set no profile, i.e. BCP), and every end-to-end
wiring — tests, integrations, the example — hid it by setting
RotateRefreshTokens: true explicitly.

Two related defects on the same path:

Profile20 with RotateRefreshTokens: true but no generator silently
dropped the requested rotation.
RFC 6749 §6: "If a new refresh token is issued, the refresh token scope
MUST be identical to that of the refresh token included by the client in
the request." The rotated token inherited the narrowed request scope, so
narrowing one refresh shrank the grant for good (asking for the original
scope again got invalid_scope). Pre-existing, but this PR makes rotation
the default path, so it is fixed here (own commit).

Fix

One private predicate decides rotation. The profile can only tighten the
configuration, never relax it — the rule authorization_code already
applies to PKCE:

func (g *RefreshToken) rotates(p oauth2.Profile) (bool, error) {
	if !g.cfg.RotateRefreshTokens && !p.RequiresRefreshRotation() {
		return false, nil
	}

	if g.cfg.RefreshTokens == nil {
		return false, ErrRotationRequiresGenerator
	}

	return true, nil
}

It is enforced twice:

Runtime guard — first statement of issueRotated: after request
validation (missing parameter, lookup, reuse detection with family
revocation, expiry, client binding, scope) and before any access token is
generated or persisted. A rotation that cannot be honored fails with
server_error carrying the new grant.ErrRotationRequiresGenerator as
cause; the wire only gets
{"error":"server_error","error_description":"internal server error"},
the cause reaches ServerConfig.OnError only. Since the guard follows
validation, a misconfigured server still answers invalid or replayed
tokens precisely and still revokes a reused family.
Boot check — a new optional capability, oauth2.ProfileValidator
(ValidateProfile(Profile) error), following the SecretMatcher pattern.
NewServer calls it on every registered grant implementing it, under every
profile (the legacy-grant ban is unchanged), and refuses to build the
server:
oauth2: NewServer: grant "refresh_token" is inconsistent with profile oauth2.0-bcp: oauth2/grant: refresh-token rotation requires a RefreshTokens generator.
*grant.RefreshToken implements it with the same rotates predicate, so
the policy lives in one place, and a compile-time assertion keeps a rename
from silently disabling the check.

Profile	`RotateRefreshTokens`	Generator	Runtime	`NewServer`
BCP / 2.1 (incl. zero value)	false	set	rotates; replay → `invalid_grant` + family revoked	ok
BCP / 2.1	any	nil	`server_error` before minting; token not consumed	refused
`Profile20`	false	any	unchanged: no rotation (RFC 6749 §6 "MAY")	ok
`Profile20`	true	set	rotates	ok
`Profile20`	true	nil	`server_error` before minting (used to skip silently)	refused

The scope fix makes the replacement refresh token keep the presented token's
scope; narrowing a refresh only narrows the access token it issues.

Design notes:

The shipped grant's runtime guard is the actual guarantee.
ProfileValidator only moves the failure to boot: a grant wrapped by a
decorator that does not forward ValidateProfile is not checked at
construction (the godoc asks decorators to forward it) and still fails
closed at runtime. The "no optional-interface fallbacks" rule agreed for
this series is about the Storage guarantees (oauth2/store: make refresh-family revocation reliable across races and storage failures #82, oauth2: retain authorization-code replay metadata to revoke previously issued tokens #80), not this
boot-time check.
Refusing Profile20 + RotateRefreshTokens: true + no generator goes
slightly beyond what was decided for this issue (rotation forced under
BCP / 2.1, non-rotation kept as an explicit Profile20 choice), while
staying consistent with it: an explicit rotation request is a security
opt-in and must not be dropped silently.
Client-visible change: under the default profile every refresh now
returns a new refresh token. Clients MUST keep it (RFC 6749 §6) and must
not refresh concurrently with the same token, or they trip reuse detection
and lose the family. Documented in docs/security-considerations.md.

Docs: godoc (grant.Config.RefreshTokens / RotateRefreshTokens /
RequirePKCE, Profile, GrantRequest.Profile, package docs, and the stale
AccessToken.FamilyID / TokenPair comments), CHANGELOG.md (Added,
Security, Fixed), docs/security-considerations.md, docs/architecture.md.
The canonical example drops its now-redundant RotateRefreshTokens: true.

Tests

New oauth2/grant/refresh_token_test.go and oauth2/refresh_rotation_test.go
(the pre-existing gofmt-dirty test files are untouched), plus extensions of
grant_more_test.go and examples/oauth2/main_test.go.

Grant level:

TestRefreshTokenRotationMandatedByProfile (BCP, 2.1; flag false,
generator set) — rotates in the same family and consumes the presented
token; a replay gets ErrRefreshTokenReused (invalid_grant) and revokes
the family (replacement consumed, its access token gone).
TestRefreshTokenRotationOptInUnderProfile20 — the explicit opt-in
rotates; a replay is refused as reuse.
TestRefreshTokenRotationWithoutGeneratorFailsBeforeIssuance (BCP, 2.1,
Profile20 + opt-in) — server_error with the sentinel as cause,
zero access-token generations, presented token untouched.
TestRefreshTokenReuseDetectedWithoutGenerator (pin) — a server that
cannot rotate still answers reuse with invalid_grant and revokes the
family.
TestRefreshTokenWithoutRotationUnderProfile20 (pin, replaces
TestRefreshTokenWithoutRotation) — legacy non-rotation, with and
without a generator: reusable, never consumed.
TestRefreshTokenValidateProfile — 12-row truth table
{2.0, BCP, 2.1} × flag × generator, plus an unknown profile that fails
closed.
TestRefreshTokenNarrowsScope (extended) — the rotated token keeps
read:mail write:mail, returned and persisted; the next exchange without
scope gets the full scope back.

HTTP level:

TestTokenEndpointRefreshRotationMandatedByProfile (BCP, 2.1) — 200 with
a new refresh_token; replay → 400 invalid_grant; the replacement and
its access token are revoked.
TestTokenEndpointRefreshRotationOptInUnderProfile20 and
TestTokenEndpointRefreshWithoutRotationUnderProfile20 (pin).
TestTokenEndpointRefreshFailsClosedWhenRotationUnavailable — the grant
hidden behind a decorator: 500 with the exact generic body, the cause on
OnError only, zero generations, token not consumed.
TestNewServerRefusesRefreshGrantThatCannotRotate and
TestNewServerChecksEveryGrantAgainstProfile — refusal wrapping the
sentinel, whatever the grant's position among the registered grants.
TestExampleOAuth2AuthorizationCodeFlow (extended) — the canonical
example rotates end to end without the flag; replaying the old token
→ 400.

Red phase, before each fix:

Rotation tests (grant, HTTP, example): no replacement refresh token, and
the replay was accepted.
No-generator test: an access token was minted and returned, without
error.
Fail-closed HTTP test: 200 with an access token, nothing on OnError.
ValidateProfile truth table and NewServer refusal: the 7 and 3 error
rows were accepted (nil stub, then unwired server).
Scope: the rotated token carried read:mail (returned, persisted, and on
the next exchange).
The original TestRefreshTokenWithoutRotation (zero profile) fails once
rotation is enforced, as the issue predicted; the pin tests pass before
and after.

Mutation testing on a scratch copy — each mutant fails at least one test:

Mutant	Caught by
runtime enforcement reverted	rotation, no-generator, fail-closed and example tests
profile ignored once a generator exists	rotation (grant, HTTP) and example tests
`Profile20` opt-in dropped (in `rotates` or in `issueRotated`)	the two opt-in tests
guard moved after `SaveAccessToken`	no-generator and fail-closed tests
guard moved to the top of `Handle`	4 existing refresh error tests + the reuse pin
cause leaked into `error_description`	fail-closed HTTP test (exact body)
`profileConstraints` reverted, `ValidateProfile` returning nil	`NewServer` refusal tests (+ truth table)
validators skipped under `Profile20`	`NewServer` refusal test
only the first grant checked / return after the first validator / `return v.ValidateProfile(p)`	every-grant test
legacy ban never / always applied	profile-constraint tests, legacy password tests
scope fix reverted	`TestRefreshTokenNarrowsScope`

Checks

make build — OK.
make test — every package passes with -race; oauth2/grant
coverage 91.2% → 91.7%, oauth2 unchanged at 91.1%. New code is 100%
covered (rotates, ValidateProfile, the guard, profileConstraints);
the lines of issueRotated still uncovered are the pre-existing
generator/store error branches.
make lint — 0 issues.

Notes for the maintainer

CLAUDE.md is not edited here. Proposed replacement for the Profile
bullet of the "OAuth2 server" section:

Profile (2.0 / 2.0-BCP / 2.1-draft) is enforced at runtime on the
grants — PKCE required, plain PKCE refused and refresh-token rotation
forced under BCP/2.1; legacy password and implicit flows refused
outside Profile20. NewServer also refuses a grant that rejects the
profile through the optional ProfileValidator capability (e.g. a
refresh_token grant without a RefreshTokens generator).
Left out of scope: an access token can still be orphaned when the refresh
generator or RotateRefreshToken fails after SaveAccessToken
(pre-existing, oauth2/store: make refresh-family revocation reliable across races and storage failures #82 makes issuance atomic — the up-front rotation decision
is what it needs); a zero RefreshTTL with forced rotation mints
immediately-expiring refresh tokens (fails closed); the PKCE / Profiles
bullets of docs/security-considerations.md and the "BCP §8.10"
citations elsewhere are stale.

Profile20BCP, the zero value, and Profile21Draft advertise mandatory refresh-token rotation, yet the refresh_token grant rotated only when grant.Config.RotateRefreshTokens was set and a RefreshTokens generator was present; Profile.RequiresRefreshRotation had no caller outside a test. The zero configuration therefore answered a refresh with a new access token, no replacement refresh token, and a presented token that stayed live, so it could be replayed until it expired. With the flag set but no generator, even Profile20 dropped the requested rotation silently. The grant now decides rotation before minting anything, the way authorization_code applies the profile to PKCE: the profile can only tighten RotateRefreshTokens. Under BCP and 2.1 every exchange consumes the presented token and returns a new one in the same family, and a replayed token is refused with invalid_grant and revokes the family (RFC 9700 §4.14.2). A rotation that cannot be honored, for want of a RefreshTokens generator, fails with server_error carrying the new grant.ErrRotationRequiresGenerator as cause, before any token is generated or persisted. The guard runs after request validation, so a misconfigured server still answers invalid or replayed tokens precisely and still revokes a reused family. Non-rotating refresh tokens remain an explicit Profile20 choice. The example drops its now-redundant RotateRefreshTokens, and the godoc, the security considerations and the CHANGELOG describe the mandate. Refs #78.

The refresh_token grant now fails closed when it has to rotate without a RefreshTokens generator, but a server wired that way still booted and only broke on the first refresh, answering server_error to every client. The mismatch between the grant and the profile is a configuration error, knowable at construction. oauth2.ProfileValidator is a new optional Grant capability, following the SecretMatcher pattern: NewServer calls ValidateProfile on every registered grant implementing it, under every profile, and refuses to build the server on error. The refresh_token grant implements it with the very rotation decision it applies at runtime, so the policy lives in one place, and a compile-time assertion keeps a renamed method from silently disabling the check. NewServer therefore rejects the grant without a generator under Profile20BCP and Profile21Draft, and under Profile20 when RotateRefreshTokens is set, wrapping grant.ErrRotationRequiresGenerator; the legacy-grant ban is unchanged. A grant hidden behind a decorator that does not forward ValidateProfile is not checked at construction; the grant's runtime guard still keeps the exchange closed, and the godoc asks decorators to forward the method. Refs #78.

RFC 6749 §6 requires the scope of a newly issued refresh token to be identical to that of the refresh token the client presented. The refresh_token grant minted the replacement with the scope of the request instead, so a client that narrowed one refresh permanently shrank its grant: the next exchange inherited the narrowed scope, and asking for the original scope again was refused with invalid_scope. Rotation being now the default under Profile20BCP and Profile21Draft, every narrowed refresh hit this path. The replacement token now carries the presented token's scope; narrowing only affects the access token issued by the exchange. Refs #78.

Two behaviors of the rotation change had no test that would notice a regression. An explicit RotateRefreshTokens opt-in under Profile20 must still rotate at runtime, yet the suite kept passing when rotation followed the profile alone or when Profile20 skipped it: the opt-in was only exercised without a generator, where it fails closed. NewServer must check every registered grant, yet the suite kept passing when the check stopped after the first grant or returned on the first validator that accepted the profile. New tests rotate under Profile20 with the opt-in, both at the grant and through the token endpoint: the replacement is returned, the presented token is consumed, and replaying it is refused as reuse. Another builds servers whose offending grant comes second, a legacy grant after a refresh grant that can rotate and a refresh grant that cannot rotate after client_credentials, and expects NewServer to refuse both. The CHANGELOG now names sender-constrained refresh tokens as the alternative RFC 9700 §2.2.2 allows for public clients, and the security considerations say that NewServer also refuses the grant under Profile20 when RotateRefreshTokens is set. Refs #78.

coveralls · 2026-10-10T00:15:01Z

Coverage Report for CI Build 38007901732

Coverage increased (+0.04%) to 91.812%

Details

Coverage increased (+0.04%) from the base build.
Patch coverage: 28 of 28 lines across 2 files are fully covered (100%).
No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	4287
Covered Lines:	3936
Line Coverage:	91.81%
Coverage Strength:	13.19 hits per line

💛 - Coveralls

euskadi31 added 4 commits October 10, 2026 01:40

euskadi31 merged commit 90a38d1 into master Oct 10, 2026
2 checks passed

euskadi31 deleted the feature/78-oauth2-refresh-rotation-profile branch October 10, 2026 00:15

euskadi31 mentioned this pull request Oct 10, 2026

fix(oauth2): validate PKCE verifier and challenge syntax #92

Merged

Navigation Menu

Repository navigation

fix(oauth2): enforce the refresh-token rotation the profile requires - #89

fix(oauth2): enforce the refresh-token rotation the profile requires#89
euskadi31 merged 4 commits into
masterfrom
feature/78-oauth2-refresh-rotation-profile

euskadi31 commented Oct 10, 2026

Uh oh!

coveralls commented Oct 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

euskadi31 commented Oct 10, 2026

Problem

Fix

Tests

Checks

Notes for the maintainer

Uh oh!

coveralls commented Oct 10, 2026

Coverage Report for CI Build 38007901732

Coverage increased (+0.04%) to 91.812%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants