feat(security): refresh rotation+revocation, security headers, error scrub#58
Open
PenguinzTech wants to merge 1 commit into
Open
feat(security): refresh rotation+revocation, security headers, error scrub#58PenguinzTech wants to merge 1 commit into
PenguinzTech wants to merge 1 commit into
Conversation
…scrub
Three manager hardening items from the code-quality/security review:
1. Refresh-token rotation + revocation (real logout):
- Refresh tokens carry a jti; /auth/refresh is single-use — presented
token revoked, new access+refresh pair issued; reuse -> 401 (also
surfaces token theft). /auth/logout revokes the refresh token
server-side (was a no-op leaving 7-day tokens live).
- revoked_token denylist table (schema + Alembic 007); expired rows
purged opportunistically. Legacy jti-less refresh tokens rejected
(fail closed; one re-login after upgrade).
- Frontend: api.ts stores the rotated refresh token; useAuth logout
calls the API best-effort before clearing local state.
2. Security headers on every response (after_request): nosniff,
X-Frame-Options DENY, CSP default-src 'none' + frame-ancestors 'none',
Referrer-Policy no-referrer, HSTS.
3. Error-detail scrub: new app/utils/responses.internal_error() logs the
exception w/ traceback and returns a generic 500 — replaces 9
jsonify(str(e)) leaks in client_config.py; middleware server-token
error and handle_db_errors no longer echo exception text.
Also fixes a pre-existing test-pollution bug: test_rs256_token_accepted_
when_configured overwrote JWT_PUBLIC_KEY on the session-scoped app fixture
without restoring (now monkeypatch.setitem) — surfaced by the new tests.
Tests: +12 (rotation/reuse/logout/legacy-reject/purge/endpoint flows,
headers, scrub). Full manager suite: 142 passing. flake8 clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviewer's GuideImplements single-use refresh-token rotation with server-side revocation, adds a refresh-token denylist table and migration, wires frontend to store rotated tokens and call logout revocation, introduces global security response headers, and centralizes error handling to scrub internal exception details while fixing a test config leak. Sequence diagram for refresh-token rotation and revocationsequenceDiagram
actor Browser
participant FrontendApi as FrontendApi_interceptor
participant AuthBlueprint as Auth_blueprint
participant AuthService
participant DB
Browser->>FrontendApi: API request with accessToken
FrontendApi-->>FrontendApi: detect 401 from API
FrontendApi->>FrontendApi: localStorage.getItem refreshToken
FrontendApi->>AuthBlueprint: POST /api/v1/auth/refresh
AuthBlueprint->>AuthService: refresh_access_token refresh_token
AuthService->>AuthService: decode_token refresh_token
AuthService->>AuthService: is_refresh_token_revoked jti
AuthService->>DB: query auth_user by user_id
AuthService-->>AuthService: validate user.active
AuthService->>AuthService: create_access_token user_id username global_role team_roles
AuthService->>AuthService: create_refresh_token user_id
AuthService->>DB: _revoke_jti jti user_id expires_at reason=rotated
AuthService-->>AuthBlueprint: {access_token, refresh_token}
AuthBlueprint-->>FrontendApi: {accessToken, refreshToken}
FrontendApi->>FrontendApi: localStorage.setItem accessToken
FrontendApi->>FrontendApi: localStorage.setItem refreshToken
FrontendApi->>Browser: retry original request with new accessToken
Browser->>AuthBlueprint: POST /api/v1/auth/logout {refreshToken}
AuthBlueprint->>AuthService: revoke_refresh_token refresh_token reason=logout
AuthService->>AuthService: decode_token refresh_token
AuthService->>DB: _revoke_jti jti user_id expires_at reason=logout
AuthBlueprint-->>Browser: {message: Logged out successfully}
Entity relationship diagram for refresh-token revocation denylisterDiagram
auth_user {
int id
}
revoked_token {
int id
string jti
int user_id
string reason
datetime revoked_at
datetime expires_at
}
auth_user ||--o{ revoked_token : has_revocations
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
AuthService._revoke_jti, the opportunistic purge runs on every revocation; if refresh/logout becomes high-traffic, consider moving this cleanup to a periodic task or using a time-based index/partitioning to avoid adding latency to user-facing flows. - The
logoutendpoint silently ignores invalid or already-rotated refresh tokens (always returning 200); if callers need to distinguish successful revocation from a no-op, consider returning a flag in the response or adjustingrevoke_refresh_tokento reflect whether a token was actually added to the denylist.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AuthService._revoke_jti`, the opportunistic purge runs on every revocation; if refresh/logout becomes high-traffic, consider moving this cleanup to a periodic task or using a time-based index/partitioning to avoid adding latency to user-facing flows.
- The `logout` endpoint silently ignores invalid or already-rotated refresh tokens (always returning 200); if callers need to distinguish successful revocation from a no-op, consider returning a flag in the response or adjusting `revoke_refresh_token` to reflect whether a token was actually added to the denylist.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Stacked on #57. Sixth branch in the chain (from the code-quality/security review).
1. Refresh-token rotation + revocation (real logout)
jti;/auth/refreshis single-use — the presented token is revoked and a new access+refresh pair issued. Reuse → 401, which also surfaces token theft./auth/logoutrevokes the refresh token server-side (previously a no-op that left 7-day refresh tokens usable after logout).revoked_tokendenylist (schema + Alembic007); expired rows purged opportunistically. Legacy jti-less refresh tokens rejected (fail closed → one re-login after upgrade).api.tsstores the rotated refresh token;useAuthlogout calls the API best-effort.2. Security headers on every response
after_request:X-Content-Type-Options: nosniff,X-Frame-Options: DENY,CSP default-src 'none'; frame-ancestors 'none',Referrer-Policy: no-referrer, HSTS.3. Error-detail scrub (+ de-dup)
New
app/utils/responses.internal_error()logs the exception with traceback and returns a generic 500 — replaces 9 identicaljsonify(str(e))leaks inclient_config.py; the middleware server-token error andhandle_db_errorsno longer echo exception text to clients.Also fixes
Pre-existing test pollution:
test_rs256_token_accepted_when_configuredoverwroteJWT_PUBLIC_KEYon the session-scoped app fixture without restoring — surfaced by the new tests, fixed withmonkeypatch.setitem.Tests
+12 (
test_refresh_rotation.py,test_security_headers.py): rotation/reuse-blocked/logout/legacy-reject/inactive-user/purge + endpoint flows; headers stamped on error responses; scrub keeps detail out of the body. Full manager suite: 142 passing. flake8 clean. Documented indocs/ENTERPRISE_SECURITY.md.🤖 Generated with Claude Code
Summary by Sourcery
Implement secure refresh-token rotation with server-side revocation, strengthen API response security, and clean up error handling and tests.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: