Skip to content

fix(auth): sweep refresh revocations until a pass finds nothing (#725) - #890

Merged
alex-dembele merged 2 commits into
masterfrom
fix/725-revocation-sweep
Oct 2, 2026
Merged

alex-dembele merged 2 commits into
masterfrom
fix/725-revocation-sweep

Conversation

@alex-dembele

Copy link
Copy Markdown
Member

Closes #725

What was wrong

#775 fixed the race #725 describes: after storing its successor, a rotation checks that its claimed row (the witness) still exists. That check only works once the revoking DELETE has committed. On Postgres, under READ COMMITTED, a DELETE reads from the snapshot taken when it starts, and its deletions stay invisible until it commits. So a rotation can store its successor after that snapshot, still see the witness, and hand the successor out, which survives the revocation.

This affects all three revokers:

  • revokeFamily, on reuse or lost membership;
  • RevokeAllUserTokens, on password change or reset;
  • RevokeUserTokensInTenant, on member deactivation or role change.

SQLite serialises writers, so the existing tests could not show it.

Mechanism

sweepRefreshTokens repeats the DELETE until a pass removes nothing, with at most 5 passes. Let P be the last, empty pass. A successor committed before P started was visible to an earlier pass, so it is gone. A successor committed after P started has its witness checked after the earlier passes committed, so the rotation finds no witness, revokes the family itself and refuses. The full argument is in token.go, next to the sweep and next to the witness check.

The refresh path is unchanged: no lock, no transaction, no extra query. Revocation costs one extra DELETE that removes nothing. There is no schema change and no new dependency.

Tests

  • token_pg_test.go (new, needs DATABASE_URL, runs on its own schema): parks the revoking DELETE behind a row lock to force the interleaving, for all three revokers. It fails on master (1 token survives in each case) and passes with the fix.
  • TestSweepRefreshTokens_TakesARowStoredBehindIt (SQLite): fails on master, passes with the fix.
$ go test ./internal/auth/ -run TestRefresh -count=50 -race
ok  	github.com/opendefender/openrisk/internal/auth	49.553s
$ DATABASE_URL=… go test ./internal/auth/ -run Postgres -count=20 -race
ok  	github.com/opendefender/openrisk/internal/auth	20.246s

go test ./...: 78 packages pass. internal/handler and cmd/server fail to build on master itself (saml2_handler.go, #886), and this branch does not touch them.

Spec: docs/725_REFRESH_REVOCATION_RACE.md. This touches auth, so it should get a tech-lead review before merge.

#775 closed the race on SQLite, but on Postgres a revoking DELETE only
sees rows committed before it started. Record the gap, the sweep that
closes it and the owner's scope decisions.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
Under READ COMMITTED a revoking DELETE misses a successor that a
rotation commits while the DELETE runs, and the rotation still sees its
witness because the DELETE has not committed. Family revocation,
logout-everywhere and per-tenant revocation all left a live token this
way. Repeating the DELETE until it removes nothing closes the gap; the
ordering argument sits next to the sweep and the witness check.

A Postgres test parks the DELETE behind a row lock to force the
interleaving for all three revokers, and a SQLite test checks that the
sweep takes a late row and stops.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
@alex-dembele alex-dembele self-assigned this Oct 2, 2026
@alex-dembele
alex-dembele merged commit dfa4599 into master Oct 2, 2026
15 of 30 checks passed
@alex-dembele
alex-dembele deleted the fix/725-revocation-sweep branch October 2, 2026 14:40
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.

security(auth): a refresh-token reuse can leave the winner’s new session alive (race)

1 participant