fix(oauth2): revoke tokens issued from a replayed authorization code by euskadi31 · Pull Request #93 · hyperscale-stack/security

euskadi31 · 2026-10-10T02:26:39Z

Closes #80.

Problem

RFC 6749 §4.1.2: if an authorization code is used more than once, the
authorization server MUST deny the request and SHOULD revoke all tokens
previously issued based on that code. The MUST held — consumption is atomic
in every store — but nothing implemented the SHOULD:

Consumption deleted the code — memory (delete), SQL (DELETE),
Redis (GET + DEL script): a replay looked exactly like an unknown code,
and nothing was left to say which client the code was issued to.
Nothing linked a code to its tokens — issuance minted a random token
family after the code was consumed.
The grant answered a replay with the store's error and left the tokens of
the first exchange alive. This is the case RFC 6749 §10.5 and RFC 9700
§4.2.4 guard against: an attacker who redeems an intercepted code first
keeps an access token and a refresh token it can rotate at will, and the
legitimate client, whose own exchange is refused, has no way to cut it off.

Reproductions — the red phase of the committed tests, run on the pre-fix
base:

	Scenario	Observed
(a)	HTTP: exchange a code (200), replay it	400 `invalid_grant`, but `/introspect` still answers `{"active":true,…}` for the first access and refresh token, and `LookupAccessToken` finds the access token
(b)	The real grant over memory, SQLite and Redis (storetest)	after the replay, the first access token is still found in every store
(c)	A proven replay landing between the first exchange's consumption and its issuance	the first exchange answers 200 and issues
(d)	Redis: go-redis resends the consume script after a lost reply	the legitimate exchange fails with `invalid_grant` (the resent `GET` + `DEL` found nothing)

Also found on the way: the memory store kept the raw code in its record,
which the model says no store may persist; and the Storage / model godoc
claimed the storage layer hashes the code, while the authorization endpoint
sets CodeHash.

Fix

The family is derived from the code. codeFamilyID(hash) =
base64url(SHA-256("oauth2/code-family:" + hash)[:16]) — 22 characters like
a random family ID, domain-separated — is computed before the code is
consumed. The first exchange and every replay agree on it with no storage for
the link, and a replay can revoke it even before the first exchange has
issued into it. The legacy password grant, which has no code, keeps a random
family.

Consumed codes are retained. A store flags a consumed code instead of
deleting it and keeps the record — hash only, never the raw code — at least
until ExpiresAt. A replay gets nil and a new *oauth2.CodeReplayError
carrying the record; it reads and matches as ErrCodeAlreadyUsed
(errors.Is, IsCode, the wire). An unknown code still gets the bare
sentinel, and exactly one caller gets the record with a nil error.

SQL — new oauth2_auth_codes.consumed SMALLINT column. Consumption is
write-first in one transaction (inTx, hence READ COMMITTED on PostgreSQL,
now true of every store transaction): UPDATE … SET consumed = 1 WHERE code_hash = ? AND consumed = 0 picks the winner through RowsAffected,
the record is read in the same transaction, losers roll back. Rows stay
until the operator's cleanup.
Redis — one single-key script: GET, cjson.decode; a payload whose
consumed == true is returned as is (a replay); otherwise the script sets
consumed and consumed_by = the call's nonce (crypto/rand.Text()) with
SET … KEEPTTL — the key expires with the code — and returns the payload
as it found it. The nonce lets a go-redis resend after a lost reply
recognize its own win instead of reading as a replay; the explicit
== true matters because cjson decodes a JSON null as a truthy value.
Memory — a consumed-hash set; the raw code is no longer kept.

A replay revokes only when it proves possession (OAuth 2.1 §4.1.3,
§7.5.3). validateExchange — expiry, client, redirect_uri, PKCE (#79's
verifyPKCE, unchanged), grant type, in their existing order — runs for the
exchange and for the replay; the code's family is revoked only when it
passes. Unknown codes, other clients, missing, wrong or malformed verifiers,
another redirect_uri, an expired code and a client no longer allowed the
grant revoke nothing: a public client_id is no secret, and anyone holding
a leaked code could otherwise cut the client off. No oracle: every replay,
proven or not, answers the bare ErrCodeAlreadyUsed ("authorization code
already consumed"), the answer of an unknown code, so the response never
tells which check failed.

Detached revocation. The revocation runs on #82's detached context
(context.WithoutCancel plus a 10s timeout), so a client hanging up cannot
abort it. A failure reaches grant.Config.OnError as one server_error
(revoke token family failed after authorization code replay, its cause
naming the family); the code record is retained, so the next proven replay
retries. A replay without a record — a CodeReplayError without one, or a
typed nil — is refused like an unknown code.

Race outcomes. The consumption and the family record are independently
linearizable; #82's family gate orders the replay's revocation V and the
first exchange's SaveTokenPair S:

S before V — the tokens are persisted, then V makes them unusable (I1);
V before S — S fails with ErrTokenFamilyRevoked and persists nothing: the
first exchange answers 400 invalid_grant "token family revoked".

Either way no token of the code stays usable — the second outcome as long as
the store keeps the revoked family record (I5, I8), which the deployment
requirements #82 introduced protect (Redis noeviction, no SQL family row
deleted before its expires_at).

Docs: the AuthorizationCodeStore contract (retention, CodeReplayError,
save each CodeHash at most once, wrappers propagate the error), the
"Token families" note on Storage, the grant flow, the SQL and Redis package
docs (consumed flag, cleanup and an expires_at index, KEEPTTL, evicted code
keys, resend safety), CHANGELOG.md, MIGRATION.md, LIMITATIONS.md,
docs/security-considerations.md (new "Authorization-code replay" bullet;
offline JWTs are out of reach), docs/observability.md,
docs/architecture.md, the example README.

Breaking changes

Nothing is tagged yet, but code tracking master is affected — see
MIGRATION.md.

AuthorizationCodeStore contract — ConsumeAuthorizationCode keeps
the consumed record at least until ExpiresAt and answers a replay with
nil and a *oauth2.CodeReplayError; an unknown code still gets
ErrCodeAlreadyUsed. Callers test err first and never issue from a
record that comes with an error; decorators must return the store's error
unchanged or wrapped with %w (normalizing it disables replay
revocation). Custom stores must pass storetest. New exported type:
oauth2.CodeReplayError.
SQL — oauth2_auth_codes gains consumed SMALLINT NOT NULL DEFAULT 0,
which Migrate does not add to an existing table: run ALTER TABLE oauth2_auth_codes ADD COLUMN consumed SMALLINT NOT NULL DEFAULT 0
(PostgreSQL, MySQL, SQLite) or drop the table and Migrate again. Until
then every code exchange fails closed with server_error. Consumed codes
stay until the operator deletes the rows past expires_at.
Redis — the code payload gains consumed / consumed_by; a payload
without them is an unconsumed code. Redis ≥ 6.0 and
maxmemory-policy noeviction are already required by oauth2/store: make refresh-family revocation reliable across races and storage failures #82.
Memory — no longer keeps the raw Code.
Upgrade every instance at once — an instance still running an earlier
build ignores the flag and would accept a code an upgraded instance already
consumed.
Wire — a replay is still 400 invalid_grant; a first exchange losing
the race to a proven replay now gets 400 invalid_grant ("token family
revoked") instead of 200; tokens issued from a replayed code become
inactive.

Tests

storetest — run by memory, the suite itself, SQLite and Redis: a
consumed code comes back without its raw value; every replay — sequential,
and every loser of 50 concurrent consumers — gets a CodeReplayError
carrying the full record (all fields compared), never a code; an unknown
code is no replay; an empty hash is refused. A new (linted)
storetest/code_replay.go drives the real authorization_code grant
over each store: the replayed code's tokens become unusable while another
code's tokens and a family-less token stay usable — reproduction (b), per
backend.

SQL (store_code_test.go) — the consumed row is kept, flagged; the
documented cleanup drops expired codes (their replay reads as unknown) and
keeps the others recognizable; a read failing after the write rolls the
consumption back.

Redis (store_code_test.go) — the consumed key keeps its TTL and expires
with the code (FastForward); payloads without the flag or with a null one
are consumed once; an undecodable payload is an error; a consume script
resent by a hook (same nonce) still wins — reproduction (d).

Grant (authorization_code_replay_test.go, a decorator over memory) —
the issue's reproduction (G1) including a refresh refused afterwards;
rotated tokens and access-only issuance revoked; a proven replay injected
between consumption and issuance refuses the exchange, which persists
nothing; eight unproven-replay rows each revoke nothing and answer exactly
ErrCodeAlreadyUsed, then the positive control revokes; isolation between
two codes (distinct 22-character families); a failed revocation reported
once, tokens still usable, then revoked by the next proven replay; a
canceled request still revokes, on a context bounded by the 10s timeout;
record-less and typed-nil replay errors; 16 concurrent redemptions (at most
one success, every error invalid_grant, every loser revokes, a winner ends
up revoked); the password grant keeps fresh families.

HTTP (code_replay_test.go) — reproduction (a) over /token,
/introspect, the store and a refresh; five unproven replays of a public
client's code (a second public client, no or a wrong verifier, a
confidential client, an unknown code) keep the token active and answer the
exact "authorization code already consumed"; the losing first exchange
answers "token family revoked" with no access_token, and the hooks see two
invalid_grant, no server_error. Plus CodeReplayError unit tests.

Red phase — the tests were written first and run against the previous
behavior; the failures are the reproductions (a)–(d) plus: memory kept the
raw code, replays carried no record, rows and keys were deleted, no
revocation and no hook report, the losing exchange issued. The review-round
pins (the no-oracle answer, the timeout) hold on the fix and were checked
against their mutants instead; the typed-nil case was red (a panic).

Mutation — my runs killed: revoke without proof, a random family,
revocation on the request context, no Redis nonce check, no nil / typed-nil
guard, a replay answering with the validation error, a revocation without
timeout. Reviewer A ran 19 mutants, 16 on the stores and 3 on the grant (17
caught; the two misses are a truthy null-flag check, which miniredis cannot
show and which was verified on a real Redis, and a defensive guard with no
observable effect); reviewer B ran 17 grant/HTTP mutants, and the survivors
it reported (the validation-error answer, the missing timeout) are killed by
the last commit.

Real engines (reviewer A) — the conformance suite and concurrent-consume
races on PostgreSQL 17 (database default isolation READ COMMITTED,
REPEATABLE READ and SERIALIZABLE; the store pins READ COMMITTED), MySQL 9.6
(REPEATABLE READ, READ COMMITTED, SERIALIZABLE), Redis 8.8 and a
multi-connection file SQLite; a consumer blocked behind a holder that
commits or rolls back; on Redis, cjson's truthy null, the null-flag payload
and a lost-reply retry (go-redis retries 0 and 3); the ALTER TABLE
migration on PostgreSQL and MySQL (an unaltered table fails closed).

Stability — the race-sensitive tests pass with -race -count=30
(memory, storetest, grant, HTTP, SQLite, Redis).

Checks

make build — OK.

make test — every package passes with -race:

Package	Before	After
`oauth2`	93.1%	93.1%
`oauth2/grant`	98.0%	98.2%
`oauth2/pkce`	100%	100%
`oauth2/storage/memory`	98.7%	100%
`oauth2/storetest`	97.5%	97.3%
`oauth2/store/sql`	95.0%	96.3%
`oauth2/store/redis`	94.4%	94.5%

The storetest figure moves by a few tenths between runs with the outcomes
of the token-family race cases. What remains uncovered in the new code is
unreachable: the moved newFamilyID error branch (crypto/rand cannot
fail), SQL RowsAffected, a non-string Redis script reply.

make lint — 0 issues; gofmt -l clean on every changed file.

Follow-ups (out of scope)

ServerConfig.OnError cannot tell a validated replay — a possible code
theft — from a garbage code: both are an invalid_grant. A dedicated
security-event signal could follow.
/authorize stores the resolved redirect URI when the request omitted
redirect_uri, and /token then requires it, while RFC 6749 §4.1.3
requires it only if it was sent.
A wrong client can still burn a leaked code, since consumption precedes the
client checks (pre-existing, RFC-compliant); this PR guarantees that such
a burn revokes nothing.
The SQL store is only tested on SQLite in CI (PostgreSQL and MySQL were
exercised by the review, not by a job).

Every store deleted an authorization code as it consumed it. A replay then looked exactly like an unknown code, and nothing was left to tell the server which client the code was issued to or what its first exchange had issued, although RFC 6749 §4.1.2 says the tokens issued from a replayed code SHOULD be revoked. The memory store also kept the raw code in its record, which the model says no store may persist. Consuming a code now flags it instead of deleting it. The record stays, without the raw code, at least until the code expires, and every later consumption gets nil and an oauth2.CodeReplayError carrying it: a new error type that reads and matches as ErrCodeAlreadyUsed, so callers that test err first, IsCode and the wire response are unchanged. An unknown code still gets the bare ErrCodeAlreadyUsed, and exactly one caller gets the record with a nil error, as before. The SQL store flags the row write-first in one transaction (inTx, so READ COMMITTED on PostgreSQL): an UPDATE ... WHERE consumed = 0 picks the winner through RowsAffected, the record is then read in the same transaction, and losers roll back. oauth2_auth_codes gains a consumed SMALLINT column, which Migrate does not add to an existing table. The Redis script flags the code's JSON with SET ... KEEPTTL, so the record expires with the code, and records a per-call nonce: a script go-redis resends after a lost reply finds its own mark and still wins, where the former GET+DEL script made such a resend fail the exchange with invalid_grant. The script compares the flag with true because cjson decodes a JSON null as a truthy value. The storage contract, the SQL cleanup guidance (consumed and abandoned codes stay until an operator deletes the rows past expires_at, which deserves an index), the PostgreSQL READ COMMITTED requirement, which now covers code consumption, and the memory store's growth are documented; MIGRATION.md lists what custom stores, callers, SQL deployments and rolling upgrades need. The grant still refuses a replay without revoking anything: that comes next. Refs #80.

RFC 6749 §4.1.2 says the tokens previously issued from a replayed authorization code SHOULD be revoked, and RFC 9700 §4.2.4 relies on it against stolen codes. The grant refused a replay but left the tokens of the first exchange alive: an attacker who redeemed an intercepted code first kept an access token and a refresh token it could rotate at will. Nothing linked a code to its tokens either: issuance picked a random token family after the code was consumed. The authorization_code grant now derives the token family from the code hash — a domain-separated SHA-256, 22 characters like a random family ID — before consuming the code, so the first exchange and every replay agree on it without storing the link. A replay the store reports with its retained record goes through the validation an exchange goes through (expiry, client, redirect_uri, PKCE, grant type, in their existing order). Only when it passes, i.e. when the request would itself have been accepted (OAuth 2.1 §4.1.3, §7.5.3), does the grant revoke the family, on a context detached from the request; a failure is reported to grant.Config.OnError ("revoke token family failed after authorization code replay"), and the next proven replay retries it. A public client_id is no secret, so unknown codes, other clients and missing, wrong or malformed verifiers revoke nothing. The replay still answers the bare ErrCodeAlreadyUsed. Revoking the family revokes every token the code issued and every later rotation. Since the family is known before the code is consumed, a proven replay landing between the first exchange's consumption and its issuance revokes the family first, and the family gate of SaveTokenPair then makes that exchange fail with invalid_grant ("token family revoked") instead of issuing — as long as the store keeps the revoked family record. The legacy password grant, which has no code, keeps a random family. storetest drives the real grant over every backend: a replayed code's tokens become unusable, while another code's tokens and a family-less token stay usable. The storage, grant and package docs, the security considerations (with the offline JWT caveat and the deployment requirements the race guarantee rests on), observability, architecture, LIMITATIONS and the example README describe the behavior. Refs #80.

…contract Review follow-ups for the authorization-code replay revocation. An unproven replay — another client, a missing, wrong or malformed verifier, another redirect_uri, an expired code, a client no longer allowed the grant — now has its answer pinned to the bare ErrCodeAlreadyUsed ("authorization code already consumed"), the answer of any replay and of an unknown code, so the response tells nothing about which check failed. The former pins only checked the invalid_grant code, which a grant returning the validation error would have passed. The revocation a replay triggers is now checked to be bounded by the detached revocation timeout, not only detached from the request, and the replay guard also refuses a typed-nil CodeReplayError, which used to panic. The storage contract now says that a given CodeHash is saved at most once — the Redis store overwrites, so a consumed code saved again becomes redeemable again — and that a decorator of ConsumeAuthorizationCode returns the store's error unchanged or wrapped with %w, since normalizing it to ErrCodeAlreadyUsed silently disables replay revocation. The docs describe both outcomes of a replay racing the first exchange — it fails that exchange or revokes its tokens — say that a late replay is refused like any replay, and cite RFC 9700 §4.2.4. The Redis comments explain what the null-flag test checks and the non-string script reply, and the test doubles lose their dead nolint directives. Refs #80.

coveralls · 2026-10-10T02:28:48Z

Coverage Report for CI Build 38016983323

Coverage increased (+0.3%) to 94.768%

Details

Coverage increased (+0.3%) from the base build.
Patch coverage: 6 uncovered changes across 3 files (230 of 236 lines covered, 97.46%).
No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
oauth2/grant/legacy_password.go	5	3	60.0%
oauth2/store/redis/store.go	9	7	77.78%
oauth2/store/sql/store.go	30	28	93.33%
Total (11 files)	236	230	97.46%

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	5103
Covered Lines:	4836
Line Coverage:	94.77%
Coverage Strength:	75.68 hits per line

💛 - Coveralls

euskadi31 added 3 commits October 10, 2026 04:10

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

euskadi31 deleted the feature/80-oauth2-code-replay-revocation branch October 10, 2026 02:33

Navigation Menu

Repository navigation

fix(oauth2): revoke tokens issued from a replayed authorization code - #93

fix(oauth2): revoke tokens issued from a replayed authorization code#93
euskadi31 merged 3 commits into
masterfrom
feature/80-oauth2-code-replay-revocation

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

Breaking changes

Tests

Checks

Follow-ups (out of scope)

Uh oh!

coveralls commented Oct 10, 2026

Coverage Report for CI Build 38016983323

Coverage increased (+0.3%) to 94.768%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants