PPT-2536: refuse revoked refresh tokens (IR-07) - #20
Merged
Conversation
`POST /auth/revoke` and `/auth/logout` were effectively no-ops for refresh
tokens. Upstream `RefreshToken#validate_code!` only checks that the JWT
decodes; it never consults the token store, and the refresh `jti` is never
written by `store_token_metadata` (refresh tokens are stateless JWTs even with
persist_jwt_tokens on). So a revoked token kept minting access tokens for its
full 30-day TTL — revocation and logout did not end a session, and a stolen
refresh token could not be revoked short of rotating JWT_SECRET. Doorkeeper had
no such gap: both tokens were columns of one row.
The complication is that ROTATION also revokes — `revoke_old_refresh_token`
marks the presented token before returning its replacement. A naive
"revoked => reject" therefore breaks RF-05 (P0): the ts-client boot race
legitimately submits the same refresh token twice within milliseconds, and the
second arrival would be refused, logging every SPA out on startup.
So the two are now recorded distinctly. Rotation writes
TokenStore::ROTATED_MARKER into `token_type` (revocation-marker rows never
carry one, so no migration is needed) and stays redeemable for
REFRESH_REVOCATION_GRACE_SECONDS, default 30. A deliberate revocation gets no
grace at all and takes effect immediately — otherwise logout would still mean
nothing. An already-revoked token is never downgraded to a rotation by a later
redemption.
This is no weaker than what it replaces: Doorkeeper's previous_refresh_token
kept the old token usable until its successor was used, typically a longer
window than 30s.
Three specs move from pinning the gap to asserting the fix:
* explicit revoke then refresh -> 400 invalid_grant (was 200)
* revocation-endpoint case un-pended
* rotation replay now covers BOTH sides of the boundary — immediate replay
still succeeds (RF-05), and the same replay with the window collapsed to
zero is refused (RFC 6819 §5.2.2.3)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard was written but the patch that scoped it to rotations silently failed to apply — crystal tool format had normalised the whitespace the string match relied on, so the grace stayed unconditional and an explicitly revoked token was still accepted for 30 seconds. CI caught it: both explicit-revoke specs expected 400 and got 200. Grace is now gated on TokenStore::ROTATED_MARKER, so a deliberate revocation takes effect immediately while the ts-client boot race (RF-05) keeps its tolerance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
camreeves
added a commit
that referenced
this pull request
Aug 1, 2026
Ameba `notice` introduced by #20 that has been failing the Ameba check on master since it merged — the check is separate from the crystal-style job in ci.yml, which is why it slipped through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
The gap
POST /auth/revokeand/auth/logoutwere effectively no-ops for refresh tokens. UpstreamRefreshToken#validate_code!only checks that the JWT decodes — it never consults the token store, and the refreshjtiis never written bystore_token_metadata(refresh tokens are stateless JWTs even withpersist_jwt_tokenson).So a revoked token kept minting access tokens for its full 30-day TTL: revocation and logout did not end a session, and a stolen refresh token could not be revoked short of rotating
JWT_SECRET. Doorkeeper had no such gap — access and refresh were columns of one row.Found independently by two reviewers. Verified as upstream
authlybehaviour, not introduced by PR #10's override of that method.Why it isn't a one-liner
Rotation also revokes.
revoke_old_refresh_tokenmarks the presented token before returning its replacement, so a naive "revoked ⇒ reject" breaks RF-05 (P0): the ts-client boot race legitimately submits the same refresh token twice within milliseconds, and the second arrival would be refused — logging every SPA out on startup.The two are now recorded distinctly:
token_type = "refresh_rotated"REFRESH_REVOCATION_GRACE_SECONDS(default 30)token_typeis used as the discriminator because revocation-marker rows never carry one — no migration required. An already-revoked token is never downgraded to a rotation by a later redemption.No weaker than what it replaces: Doorkeeper's
previous_refresh_tokenkept the old token usable until its successor was used, typically a longer window than 30s.Specs
Three move from pinning the gap to asserting the fix:
400 invalid_grant(was200)238 examples, 0 failures, 5 pending.Note
The first push failed CI (
expected 400, got 200) because the patch scoping the grace to rotations silently no-op'd —crystal tool formathad normalised the whitespace the string match relied on, leaving the grace unconditional so logout still didn't take effect. Caught by the very specs meant to prove it, which is the argument for flipping pinned gaps rather than deleting them.🤖 Generated with Claude Code