fix(security): invalidate sessions on disable, password change and 2FA reset - #131
Open
fjaeckel wants to merge 2 commits into
Open
fix(security): invalidate sessions on disable, password change and 2FA reset#131fjaeckel wants to merge 2 commits into
fjaeckel wants to merge 2 commits into
Conversation
…A reset
AuthMiddleware only checked an access token's signature and expiry. It never
verified the user still existed, was enabled, or that the token predated a
security event. Deleting refresh tokens -- the only revocation that existed --
merely stops a session being EXTENDED; the outstanding 15-minute access token
kept working.
Verified against a running instance:
- After an admin disabled a user (disabled=t, refresh tokens deleted), the
victim's access token still returned 200 from GET /users/me and GET
/flights, and created a flight (POST /flights -> 201).
- After a password change (204), the OLD access token still returned 200/201.
A user rotating their password because they suspect compromise did not
evict the attacker.
Adds a session epoch: users.tokens_valid_after (migration 000050). Access
tokens issued at or before that instant are rejected. IssuedAt has second
resolution, so a token minted in the same second as the event is also rejected
rather than slipping through.
AuthMiddlewareWithState additionally rejects tokens for disabled or deleted
users. AuthMiddleware keeps its old signature and delegates with a nil state
function, so existing callers and tests are unaffected.
The epoch is bumped on password change (AuthService.ChangePassword), admin
account disable, and admin 2FA reset. Admin disable and 2FA reset now also
delete refresh tokens.
Tradeoff: the state callback performs an indexed primary-key lookup per
authenticated request. The admin path already did one per request. If this
shows up in profiling, a short-TTL cache keyed by user ID is the natural next
step -- deliberately left out here to keep the change reviewable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GukWfyJMY28qv2CJjxFvKF
…nvalidation # Conflicts: # internal/api/handlers/admin_users.go # internal/api/handlers/handlers_test.go # internal/repository/interfaces.go # internal/repository/postgres/user.go # internal/service/auth_test.go # internal/service/flight_signature_test.go # internal/service/notification_custom_test.go # internal/service/twofactor_encryption_test.go
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
2 participants
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.
Summary
Fixes a Medium severity session-lifecycle gap: nothing could revoke an access token early.
AuthMiddlewareonly checked a token's signature and expiry. It never verified the user still existed, was enabled, or that the token predated a security event. Deleting refresh tokens — the only revocation that existed — merely stops a session being extended; the outstanding 15-minute access token kept working.Verified against a running instance
Account disable does not cut access. After an admin disabled a user (
disabled=t, refresh tokens deleted), the victim's access token still returned:GET /users/me→ 200GET /flights→ 200POST /flights→ 201 (created a flight)Password change does not cut existing sessions. After
change-password(204), the old access token still returned 200/201. A user rotating their password because they suspect compromise did not evict the attacker.Changes
000050addsusers.tokens_valid_after— a session epoch. Access tokens issued at or before that instant are rejected.IssuedAthas second resolution, so a token minted in the same second as the event is also rejected rather than slipping through.AuthMiddlewareWithStateadditionally rejects tokens for disabled (403) or deleted (401) users.AuthMiddlewarekeeps its old signature and delegates with anilstate function, so existing callers and tests are unaffected.The state callback performs an indexed primary-key lookup per authenticated request. The admin path already did one per request, and the connection pool is bounded — but this is a real change to the hot path.
If it shows up in profiling, a short-TTL (a few seconds) cache keyed by user ID is the natural next step. I deliberately left it out to keep this change reviewable; happy to add it if you'd prefer it upfront.
Tests
403; enabled user →200401; issued after →200401nilstate preserves prior behaviour (soAuthMiddlewarecallers are unchanged)All existing user-repo mocks gained the new interface method; sqlmock column lists updated for the new column.
Full suite: 24 packages passing,
gofmtclean.Related
Pairs with the logout PR (#130): that revokes the refresh token, this one makes the access token die with it. Together they give a user a reliable way to end a session.
🤖 Generated with Claude Code
Generated by Claude Code