Security fix: invalidate panel sessions on logout and re-login

[cursor]

Security review finding

Severity: Medium
Location: app.py
Issue: #55

Impact

Flask signed session cookies remained valid for up to 8 hours after logout. An attacker who captured a session cookie before logout could replay it for full admin access even after the victim believed the session was terminated.

Fix

• Add session_store.py with Redis-backed (production) and in-memory (single-worker dev) session token tracking.
• Issue a server-side session token on login; validate it on every @login_required request.
• Revoke the active token on logout and replace it on subsequent login so only the latest session remains valid.

Tests

• test_logout_invalidates_stolen_session_cookie
• test_login_invalidates_previous_session_cookie

All 83 non-integration tests pass.

  

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

[qodo-code-review]

PR Summary by Qodo

Invalidate server-panel sessions on logout and re-login via server-side token tracking

🐞 Bug fix 🧪 Tests ✨ Enhancement 🕐 40+ Minutes

AI Description

• Add server-side session token validation to prevent replay of stolen signed cookies.
• Revoke active session tokens on logout and rotate tokens on each login.
• Add unit tests for cookie invalidation and Memory/Redis session-store behavior.

Diagram

graph TD
  C([Client]) --> A["app.py (Flask routes)"] --> R["Protected endpoints"] --> D{"Session token valid?"} --> S["session_store.py"] --> X[(Redis)]
  C --> A --> K["Signed session cookie"]

  subgraph Legend
    direction LR
    _svc["Service/Module"] ~~~ _dec{"Decision"} ~~~ _db[("Database")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Server-side session storage (Flask-Session) instead of signed cookies

• ➕ Centralizes all session state server-side (no long-lived auth material in cookies)
• ➕ Built-in invalidation semantics and multi-worker support
• ➖ Larger behavioral change; more migration and operational considerations
• ➖ Still requires careful TTL/configuration to meet security goals

2. Adopt Flask-Login with session protection / fresh logins

• ➕ Standardized auth/session management patterns and extension support
• ➕ Clearer separation between authentication and session lifecycle
• ➖ Additional dependency and refactor of current minimal auth approach
• ➖ May still need custom server-side token revocation to meet 'logout invalidates old cookie' requirement

3. Shorten permanent_session_lifetime and rotate secret key on logout

• ➕ Simple configuration-only mitigation; reduces replay window
• ➖ Does not guarantee immediate invalidation after logout
• ➖ Rotating secret key is disruptive (invalidates all sessions) and not user-specific

Recommendation: The PR’s approach (server-side per-user active token tracking) is the most targeted fix for the reported replay risk: it guarantees immediate invalidation on logout and enforces single-active-session on re-login, without converting the app to fully server-side sessions. Consider a future move to Flask-Session/Flask-Login if broader auth features are planned, but for this security finding the current solution is appropriately scoped and test-covered.

Files changed (4) +321 / -6

Enhancement (1) +102 / -0

session_store.pyAdd Memory and Redis session-token stores with per-user active session mapping +102/-0

Add Memory and Redis session-token stores with per-user active session mapping

• Introduces a session store abstraction with an in-process MemorySessionStore for single-worker dev and a RedisSessionStore for shared, multi-worker deployments. Supports replace-on-login semantics, TTL-based expiration, best-effort revoke, and URI-scheme-based backend selection.

session_store.py

Bug fix (1) +38 / -6

app.pyEnforce server-side session token validation and revoke/rotate tokens +38/-6

Enforce server-side session token validation and revoke/rotate tokens

• Creates a session store instance and adds helpers to establish, validate, and revoke server-side session tokens. Updates login_required and rate-limit exemptions to require token validity, revokes tokens on logout, and rotates tokens on each successful login to invalidate prior cookies.

app.py

Tests (2) +181 / -0

test_login_routes.pyAdd regression tests for logout and re-login invalidating prior cookies +54/-0

Add regression tests for logout and re-login invalidating prior cookies

• Extends login route tests to assert session_token/username are set on login. Adds regression coverage proving that a captured session cookie becomes unusable after logout or after a subsequent login from another client.

tests/test_login_routes.py

test_session_store.pyAdd unit tests for Memory/Redis session-store behaviors and failure modes +127/-0

Add unit tests for Memory/Redis session-store behaviors and failure modes

• Adds targeted tests verifying token replacement, expiration, and revocation behavior for the in-memory store. Mocks Redis to validate connection timeout settings, key/TTL operations, validation semantics, and fail-closed behavior when Redis is unavailable.

tests/test_session_store.py

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 Logout does not invalidate stolen session cookies, allowing post-logout admin access

✅ Compliance rules (platform): 6 rules

✅ Cross-repo context

  Not relevant to this PR: OpenRTMP/librtmp2-server

1. Redis login can 500 ✓ Resolved 🐞 Bug ☼ Reliability

Description

RedisSessionStore.replace_user_session() performs Redis calls without exception handling, so a
Redis timeout/connection error will propagate and turn a successful credential check into an HTTP
500. This makes login unavailable during Redis issues and can also break re-login token rotation.

Code

session_store.py[R61-70]

+    def replace_user_session(self, username, token, ttl_seconds):
+        user_key = f"{self._user_prefix}{username}"
+        old = self._client.get(user_key)
+        if old:
+            old_token = old.decode() if isinstance(old, bytes) else old
+            self._client.delete(f"{self._token_prefix}{old_token}")
+        pipe = self._client.pipeline()
+        pipe.setex(f"{self._token_prefix}{token}", ttl_seconds, "1")
+        pipe.setex(user_key, ttl_seconds, token)
+        pipe.execute()

Evidence

/login calls _establish_logged_in_session() on successful credentials, and that calls
session_store.replace_user_session(...). In the Redis-backed implementation,
replace_user_session() executes multiple Redis commands (get, delete, pipeline setex,
execute) without a try/except, so any Redis error will bubble up as an unhandled exception (500).
Docker Compose defaults RATELIMIT_STORAGE_URI to Redis, making this path common in real
deployments.

app.py[229-240]
session_store.py[61-70]
docker-compose.yml[14-33]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`RedisSessionStore.replace_user_session()` does network I/O without any exception handling, so Redis outages/timeouts can raise and produce a 500 during `/login` after credentials are validated.

## Issue Context
This is invoked by the login flow after `user_ok`/`pass_ok` succeeds.

## Fix Focus Areas
- session_store.py[61-70]
- app.py[229-240]

## Suggested fix
- Add exception handling around the Redis operations in `replace_user_session()` (preferably catching `redis.exceptions.RedisError`), and either:
 - re-raise a small custom exception (e.g., `SessionBackendUnavailable`) that `app.py` catches to return a controlled response (e.g., render login with an error or return 503), OR
 - return a boolean and have `_establish_logged_in_session()` only mutate/clear the Flask session after the server-side token write succeeds.
- Ensure the login route does not partially clear/modify session state when the backend write fails.
- Log the failure (see separate finding) so operators can diagnose Redis outages quickly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Non-atomic Redis revoke ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

RedisSessionStore.revoke() does a GET of the active token and then deletes the user mapping in a
later pipeline based on that stale value, so a concurrent login can update the mapping between the
GET and EXEC and still have its new mapping deleted. This can cause unexpected logouts/failed logins
under concurrent login/logout requests (race), especially in multi-worker deployments.

Code

session_store.py[R110-120]

+    def revoke(self, username, token):
+        try:
+            user_key = f"{self._user_prefix}{username}"
+            active = self._client.get(user_key)
+            active_token = active.decode() if isinstance(active, bytes) else active
+
+            pipe = self._client.pipeline()
+            if active_token == token:
+                pipe.delete(user_key)
+            pipe.delete(f"{self._token_prefix}{token}")
+            pipe.execute()

Relevance

⭐⭐⭐ High

Team accepted similar Redis atomicity/race hardening work recently (multi-worker Redis reliability
fixes) in PRs #47/#51.

PR-#47
PR-#51

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The revoke flow reads the current user mapping with GET and then later deletes user_key in a
pipeline based on that stale read, which is not an atomic compare-and-delete. Since login
replacement updates user_key in a separate operation, a concurrent replace can occur between the
revoke GET and EXEC, allowing revoke to delete the newer mapping.

session_store.py[69-84]
session_store.py[110-120]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RedisSessionStore.revoke()` reads `user_key` outside the pipeline and conditionally queues `DEL user_key` based on that earlier read. If another request replaces the user mapping between the `GET` and `EXEC`, `revoke()` can still delete the *new* mapping, invalidating a freshly established session.

### Issue Context
This is a cross-request race in Redis because the check (`active_token == token`) is performed client-side, not atomically in Redis.

### Fix Focus Areas
- session_store.py[110-120]

### Implementation direction
Use an atomic server-side compare-and-delete, e.g.:
- A small Lua script (`EVAL`) that:
 - `if redis.call('GET', user_key) == token then redis.call('DEL', user_key) end`
 - always deletes `panel:session:<token>`
- Or use `WATCH user_key` + re-read + `MULTI/EXEC` with retry on `WatchError`.

Add/adjust tests to cover the race behavior (ensure a revoke of an older token cannot delete a newly written `user_key`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Silent Redis auth failures ✓ Resolved 🐞 Bug ◔ Observability

Description

RedisSessionStore.is_valid() and revoke() swallow all exceptions and return False/None without
logging, so Redis outages will present as unexplained logouts and best-effort revokes silently
failing. This makes diagnosing auth/session incidents significantly harder.

Code

session_store.py[R72-95]

+    def is_valid(self, username, token):
+        try:
+            user_key = f"{self._user_prefix}{username}"
+            active = self._client.get(user_key)
+            if active is None:
+                return False
+            active_token = active.decode() if isinstance(active, bytes) else active
+            if active_token != token:
+                return False
+            return bool(self._client.exists(f"{self._token_prefix}{token}"))
+        except Exception:
+            return False
+
+    def revoke(self, username, token):
+        try:
+            user_key = f"{self._user_prefix}{username}"
+            active = self._client.get(user_key)
+            if active is not None:
+                active_token = active.decode() if isinstance(active, bytes) else active
+                if active_token == token:
+                    self._client.delete(user_key)
+            self._client.delete(f"{self._token_prefix}{token}")
+        except Exception:
+            return

Evidence

_session_is_authenticated() calls session_store.is_valid(...) on every @login_required route.
When is_valid() catches and suppresses all exceptions, any Redis issue becomes indistinguishable
from a real invalid token (redirect-to-login) with no server-side signal. Similarly, revoke()
suppresses exceptions, so logout revocation failures are silent.

app.py[147-156]
session_store.py[72-95]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`RedisSessionStore.is_valid()` and `RedisSessionStore.revoke()` catch `Exception` and suppress it with no logs/metrics. This hides Redis connectivity/timeout problems and also masks unexpected programming errors.

## Issue Context
These methods are on the hot path for every authenticated request via `_session_is_authenticated()`.

## Fix Focus Areas
- session_store.py[72-95]
- app.py[147-156]

## Suggested fix
- Catch narrower exceptions (e.g., `redis.exceptions.RedisError`) instead of `Exception`.
- Add logging with `exc_info=True` when a backend exception occurs (consider warning-level for `is_valid`, debug/info for `revoke` if you’re worried about noise).
- Optionally emit a counter/metric (if you have a metrics system) for backend errors so you can alert on auth-backend degradation.
- Keep the “fail closed” behavior (returning False) but make it observable.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit e4ea623

Results up to commit cc732b9

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Redis login can 500 ✓ Resolved 🐞 Bug ☼ Reliability

Description

RedisSessionStore.replace_user_session() performs Redis calls without exception handling, so a
Redis timeout/connection error will propagate and turn a successful credential check into an HTTP
500. This makes login unavailable during Redis issues and can also break re-login token rotation.

Code

session_store.py[R61-70]

+    def replace_user_session(self, username, token, ttl_seconds):
+        user_key = f"{self._user_prefix}{username}"
+        old = self._client.get(user_key)
+        if old:
+            old_token = old.decode() if isinstance(old, bytes) else old
+            self._client.delete(f"{self._token_prefix}{old_token}")
+        pipe = self._client.pipeline()
+        pipe.setex(f"{self._token_prefix}{token}", ttl_seconds, "1")
+        pipe.setex(user_key, ttl_seconds, token)
+        pipe.execute()

Evidence

/login calls _establish_logged_in_session() on successful credentials, and that calls
session_store.replace_user_session(...). In the Redis-backed implementation,
replace_user_session() executes multiple Redis commands (get, delete, pipeline setex,
execute) without a try/except, so any Redis error will bubble up as an unhandled exception (500).
Docker Compose defaults RATELIMIT_STORAGE_URI to Redis, making this path common in real
deployments.

app.py[229-240]
session_store.py[61-70]
docker-compose.yml[14-33]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`RedisSessionStore.replace_user_session()` does network I/O without any exception handling, so Redis outages/timeouts can raise and produce a 500 during `/login` after credentials are validated.

## Issue Context
This is invoked by the login flow after `user_ok`/`pass_ok` succeeds.

## Fix Focus Areas
- session_store.py[61-70]
- app.py[229-240]

## Suggested fix
- Add exception handling around the Redis operations in `replace_user_session()` (preferably catching `redis.exceptions.RedisError`), and either:
 - re-raise a small custom exception (e.g., `SessionBackendUnavailable`) that `app.py` catches to return a controlled response (e.g., render login with an error or return 503), OR
 - return a boolean and have `_establish_logged_in_session()` only mutate/clear the Flask session after the server-side token write succeeds.
- Ensure the login route does not partially clear/modify session state when the backend write fails.
- Log the failure (see separate finding) so operators can diagnose Redis outages quickly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Silent Redis auth failures ✓ Resolved 🐞 Bug ◔ Observability

Description

RedisSessionStore.is_valid() and revoke() swallow all exceptions and return False/None without
logging, so Redis outages will present as unexplained logouts and best-effort revokes silently
failing. This makes diagnosing auth/session incidents significantly harder.

Code

session_store.py[R72-95]

+    def is_valid(self, username, token):
+        try:
+            user_key = f"{self._user_prefix}{username}"
+            active = self._client.get(user_key)
+            if active is None:
+                return False
+            active_token = active.decode() if isinstance(active, bytes) else active
+            if active_token != token:
+                return False
+            return bool(self._client.exists(f"{self._token_prefix}{token}"))
+        except Exception:
+            return False
+
+    def revoke(self, username, token):
+        try:
+            user_key = f"{self._user_prefix}{username}"
+            active = self._client.get(user_key)
+            if active is not None:
+                active_token = active.decode() if isinstance(active, bytes) else active
+                if active_token == token:
+                    self._client.delete(user_key)
+            self._client.delete(f"{self._token_prefix}{token}")
+        except Exception:
+            return

Evidence

_session_is_authenticated() calls session_store.is_valid(...) on every @login_required route.
When is_valid() catches and suppresses all exceptions, any Redis issue becomes indistinguishable
from a real invalid token (redirect-to-login) with no server-side signal. Similarly, revoke()
suppresses exceptions, so logout revocation failures are silent.

app.py[147-156]
session_store.py[72-95]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`RedisSessionStore.is_valid()` and `RedisSessionStore.revoke()` catch `Exception` and suppress it with no logs/metrics. This hides Redis connectivity/timeout problems and also masks unexpected programming errors.

## Issue Context
These methods are on the hot path for every authenticated request via `_session_is_authenticated()`.

## Fix Focus Areas
- session_store.py[72-95]
- app.py[147-156]

## Suggested fix
- Catch narrower exceptions (e.g., `redis.exceptions.RedisError`) instead of `Exception`.
- Add logging with `exc_info=True` when a backend exception occurs (consider warning-level for `is_valid`, debug/info for `revoke` if you’re worried about noise).
- Optionally emit a counter/metric (if you have a metrics system) for backend errors so you can alert on auth-backend degradation.
- Keep the “fail closed” behavior (returning False) but make it observable.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 922f338

[sonarqubecloud]

Quality Gate failed

Failed conditions
D Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

💡 Need a hand with PR review? Try Gitar by Sonar!

[qodo-code-review]

Code review by qodo was updated up to the latest commit e4ea623