Skip to content

feat(security): refresh rotation+revocation, security headers, error scrub#58

Open
PenguinzTech wants to merge 1 commit into
feature/spiffe-service-authfrom
feature/manager-security-hardening
Open

feat(security): refresh rotation+revocation, security headers, error scrub#58
PenguinzTech wants to merge 1 commit into
feature/spiffe-service-authfrom
feature/manager-security-hardening

Conversation

@PenguinzTech

@PenguinzTech PenguinzTech commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Stacked on #57. Sixth branch in the chain (from the code-quality/security review).

1. Refresh-token rotation + revocation (real logout)

  • Refresh tokens now carry a jti; /auth/refresh is single-use — the presented token is revoked and a new access+refresh pair issued. Reuse → 401, which also surfaces token theft.
  • /auth/logout revokes the refresh token server-side (previously a no-op that left 7-day refresh tokens usable after logout).
  • revoked_token denylist (schema + Alembic 007); expired rows purged opportunistically. Legacy jti-less refresh tokens rejected (fail closed → one re-login after upgrade).
  • Frontend updated in the same branch: api.ts stores the rotated refresh token; useAuth logout 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 identical jsonify(str(e)) leaks in client_config.py; the middleware server-token error and handle_db_errors no longer echo exception text to clients.

Also fixes

Pre-existing test pollution: test_rs256_token_accepted_when_configured overwrote JWT_PUBLIC_KEY on the session-scoped app fixture without restoring — surfaced by the new tests, fixed with monkeypatch.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 in docs/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:

  • Make refresh tokens single-use with rotation that issues new access and refresh token pairs and rejects legacy jti-less tokens.
  • Add server-side logout that revokes refresh tokens and wire the frontend to call the logout API and store rotated refresh tokens.

Bug Fixes:

  • Prevent test-suite configuration leakage from RS256 JWT tests that previously modified global app config without restoring it.

Enhancements:

  • Introduce a refresh-token revocation denylist table and migration with opportunistic purging of expired entries.
  • Stamp consistent security headers, including CSP, HSTS, and clickjacking protections, on all API responses.
  • Centralize internal error handling in a shared helper that logs exceptions with full detail while returning generic 500 responses.
  • Scrub exception text from middleware and database error responses so internal details are never exposed to clients.

Documentation:

  • Document refresh-token rotation and revocation semantics and API response hygiene measures in ENTERPRISE_SECURITY.md.

Tests:

  • Add tests covering refresh-token rotation, revocation, legacy token rejection, endpoint flows, and denylist purging.
  • Add tests verifying security headers on error responses and that internal error responses omit exception details.
  • Fix JWT RS256 test pollution by using monkeypatch to avoid leaking configuration changes across tests.

…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>
@sourcery-ai

sourcery-ai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Reviewer's Guide

Implements 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 revocation

sequenceDiagram
    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}
Loading

Entity relationship diagram for refresh-token revocation denylist

erDiagram
    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
Loading

File-Level Changes

Change Details Files
Implement single-use refresh-token rotation and server-side revocation using a refresh-token denylist.
  • Add jti claim to issued refresh tokens to uniquely identify each token instance.
  • Introduce helper methods to revoke refresh tokens by jti, check revocation status, and opportunistically purge expired denylist entries.
  • Change refresh_access_token to return both new access and refresh tokens, enforcing jti presence and denylist checks, and revoking the presented token on successful rotation.
  • Update the /auth/refresh and /auth/logout endpoints to use the new rotation and revocation logic and adjust response and request schemas accordingly.
  • Define revoked_token table in the schema and Alembic migration, including indexes for jti and expires_at, and extend schema tests to assert its presence.
manager/backend/app/services/auth_service.py
manager/backend/app/blueprints/auth.py
manager/backend/app/schema.py
manager/backend/alembic/versions/007_revoked_token.py
manager/backend/tests/test_refresh_rotation.py
manager/backend/tests/test_schema.py
Propagate rotation semantics to the frontend so clients store rotated refresh tokens and trigger server-side revocation on logout.
  • Update API interceptor to capture both accessToken and refreshToken from refresh responses and persist the rotated refresh token.
  • Modify useAuth.logout to best-effort call the backend logout endpoint with the stored refresh token before clearing local state and redirecting.
  • Ensure local logout behavior remains non-blocking even if the revocation call fails.
manager/frontend/src/services/api.ts
manager/frontend/src/hooks/useAuth.ts
Apply strict security headers to all HTTP responses from the backend application.
  • Register an after_request hook that stamps X-Content-Type-Options, X-Frame-Options, Content-Security-Policy, Referrer-Policy, and Strict-Transport-Security on every response.
  • Document the behavior and rationale for these headers in the enterprise security documentation.
  • Add tests that assert the presence and exact values of these headers on error responses.
manager/backend/app/__init__.py
docs/ENTERPRISE_SECURITY.md
manager/backend/tests/test_security_headers.py
Centralize internal error handling and scrub exception details from API responses.
  • Introduce an internal_error helper that logs exceptions with full detail and returns a generic 500 JSON body.
  • Refactor multiple client_config endpoints to use internal_error instead of jsonify(str(e)) or similar patterns that leak exception text.
  • Update database error-handling decorator to log full stack traces with logger.exception and remove the exception message from the JSON response.
  • Adjust auth middleware token validation error handling to log details and return a generic error string without interpolating the exception.
manager/backend/app/utils/responses.py
manager/backend/app/blueprints/client_config.py
manager/backend/app/utils/decorators.py
manager/backend/app/middleware/auth.py
manager/backend/tests/test_security_headers.py
Fix a test configuration leak related to JWT public key modification and ensure tests reflect new security behavior.
  • Change RS256 acceptance test to use monkeypatch.setitem when modifying app.config.JWT_PUBLIC_KEY so the value does not leak across the session-scoped app fixture.
  • Add tests for refresh rotation, revocation behavior, legacy jti-less token rejection, inactive user handling, denylist purging, and endpoint flows.
  • Extend security tests to validate internal error response content scrubbing in addition to header behavior.
manager/backend/tests/test_auth_jwt.py
manager/backend/tests/test_refresh_rotation.py
manager/backend/tests/test_security_headers.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.
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.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

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.

1 participant