feat: session listing/revocation and refresh token rotation#6
Merged
Conversation
- Add Session type, SessionStore interface, and ErrSessionRevoked to auth/types.go - Add CreateTokenWithSession (jti claim) and ParseTokenClaims (ignores expiry) to JWTManager - Add Sessions field to auth.Config; middleware validates JWT jti against SessionStore - Update resolveUser to return (userID, sessionID, error); API key auth returns empty sessionID - Add Sessions, RefreshTokenTTL, RefreshCookieName fields to AuthHandler - Login/Signup create sessions and return refresh_token when Sessions configured - Logout revokes session by parsing jti from access token - Add RefreshToken handler: validates refresh token, atomically rotates to new session+tokens - Add SessionHandler with List, Revoke, RevokeAll endpoints - Add SetRefreshCookie, ClearRefreshCookie, tokenFromRequest helpers - Comprehensive tests for all new functionality - Update README with SessionStore interface, SessionHandler docs, and security notes Agent-Logs-Url: https://github.com/amalgamated-tools/goauth/sessions/f2f7c45e-24f9-4796-b940-f4939c68db9a Co-authored-by: veverkap <22348+veverkap@users.noreply.github.com>
… error message Agent-Logs-Url: https://github.com/amalgamated-tools/goauth/sessions/f2f7c45e-24f9-4796-b940-f4939c68db9a Co-authored-by: veverkap <22348+veverkap@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
veverkap
April 16, 2026 23:53
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Adds optional server-side session tracking and refresh token rotation to enable session listing/revocation and mitigate long-lived token risk, while keeping existing JWT-only deployments working when no SessionStore is configured.
Changes:
- Introduces
auth.Session/auth.SessionStoreand embeds session IDs into JWTjtiviaCreateTokenWithSession. - Adds session-aware auth flows in
handler.AuthHandler(refresh token issuance, logout revocation, refresh rotation) plus a newSessionHandlerfor list/revoke endpoints. - Updates middleware to optionally validate
jtiagainst a session store (API keys bypass), and expands tests/docs accordingly.
Show a summary per file
| File | Description |
|---|---|
| handler/session.go | New session management HTTP handler (list/revoke/revoke-all). |
| handler/session_test.go | Tests for SessionHandler list and revocation behaviors. |
| handler/helpers.go | Adds refresh-cookie helpers and a token extractor used by logout. |
| handler/helpers_test.go | Adds a mockSessionStore and a helper for session-enabled auth handler tests. |
| handler/auth.go | Adds session-aware token issuance, logout session revocation, and refresh rotation endpoint. |
| handler/auth_test.go | Adds tests for session-enabled login/signup/logout/refresh flows. |
| auth/types.go | Adds Session model, SessionStore interface, and ErrSessionRevoked. |
| auth/middleware.go | Extends middleware to return sessionID from JWT and validate sessions via store. |
| auth/middleware_test.go | Adds mocks and test coverage for session validation + updated resolveUser signature. |
| auth/jwt.go | Adds CreateTokenWithSession and ParseTokenClaims (signature-validated, time-claims ignored). |
| auth/jwt_test.go | Adds test coverage for session jti embedding and ParseTokenClaims. |
| README.md | Documents session store wiring, refresh rotation, and new session endpoints. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
handler/auth.go:301
- In RefreshToken, if two refresh requests race on the same refresh token, the second request can reach DeleteSession after the first has already deleted it; per SessionStore docs DeleteSession should then return sql.ErrNoRows. The current code treats any DeleteSession error as a 500, which would incorrectly surface a server error for an already-consumed/invalid refresh token. Handle sql.ErrNoRows from DeleteSession as an unauthorized "invalid or expired refresh token" response (and consider adjusting the "revoked atomically" comment unless the store guarantees atomic rotation).
// Revoke the consumed session before issuing a new one.
if err := h.Sessions.DeleteSession(r.Context(), sess.ID, sess.UserID); err != nil {
slog.ErrorContext(r.Context(), "failed to revoke old session on refresh", slog.Any("error", err))
writeError(r.Context(), w, http.StatusInternalServerError, "internal server error")
return
}
auth/middleware.go:168
- Session validation treats any SessionStore error (including transient DB outages) as a 401 "session expired or revoked". That can cause hard-to-diagnose auth failures and prevents correct 5xx handling/alerting. Consider: (1) treating sql.ErrNoRows / missing session / expired session as 401, but (2) returning 500 for other store errors (and logging them).
if cfg.Sessions != nil && sessionID != "" {
sess, serr := cfg.Sessions.FindSessionByID(r.Context(), sessionID)
if serr != nil || sess == nil || time.Now().After(sess.ExpiresAt) {
jsonError(w, http.StatusUnauthorized, "session expired or revoked")
return
}
- Files reviewed: 12/12 changed files
- Comments generated: 4
Resolved conflicts in auth/types.go (combined ErrSessionRevoked with ErrNotFound/ErrTOTPNotFound/ErrInvalidTOTPCode from main) and handler/helpers_test.go (kept both mockSessionStore and mockPasswordResetStore). Updated sql.ErrNoRows references to auth.ErrNotFound throughout handler package.
…ing, and cache headers - middleware: add sess.UserID==userID check to prevent cross-user session mixups - middleware: return 500 (with logging) for session store errors, 401 only for not-found/expired - middleware: replace database/sql dependency with ErrNotFound sentinel - jwt: validate issuer and audience in ParseTokenClaims even when skipping time checks - jwt: add token.Valid guard in ParseTokenClaims - handler: add Cache-Control: no-store and Pragma: no-cache to Signup, Login, and RefreshToken responses
Resolved conflicts: - auth/types.go: combined ErrSessionRevoked (HEAD) with ErrEmailNotVerified (main) - handler/auth.go: merged session fields + RequireVerification/Verifications fields; Login now checks email verification before issuing tokens - auth/middleware_test.go: adopted main's proper error-checking pattern for IsAdmin calls - smtp/smtp_test.go: adopted main's loop-with-error-check pattern for os.Unsetenv
Resolved conflict in handler/helpers_test.go: kept both mockSessionStore (this branch) and mockMagicLinkStore (main), each with their own deleteExpiredFunc method body.
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
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.
Stateless JWTs can't be revoked server-side, and long-lived tokens are a liability. This adds opt-in server-side session tracking with refresh token rotation — both features are strictly additive; existing code without a
SessionStoreis unaffected.authpackageSession/SessionStore— new type and 7-method interface for session CRUD; only the SHA-256 hash of the refresh token is persistedCreateTokenWithSession(ctx, userID, sessionID)— embeds session ID as the JWTjticlaimParseTokenClaims(tokenString)— validates signature, ignores expiry (used by logout to parse potentially expired access tokens)Config.Sessions SessionStore— when set,MiddlewareandAdminMiddlewarevalidatejtiagainst the store on every request; API key auth bypasses the checkhandlerpackageAuthHandlergainsSessions,RefreshTokenTTL,RefreshCookieNamefieldsLogin/Signupcreate a session and returnrefresh_tokenin the responseLogoutrevokes the session by extractingjtifrom the (possibly expired) access tokenRefreshTokenhandler: validates the refresh token hash, atomically deletes the old session, issues a new session + rotated token pairSessionHandler(new) —List(200),Revoke(204),RevokeAll(204)Usage