Skip to content

fix(auth): prune expired sessions on every verify to prevent memory leak - #196

Merged
nesquena-hermes merged 2 commits into
nesquena:masterfrom
iRonin:security/prune-expired-sessions
Apr 10, 2026
Merged

fix(auth): prune expired sessions on every verify to prevent memory leak#196
nesquena-hermes merged 2 commits into
nesquena:masterfrom
iRonin:security/prune-expired-sessions

Conversation

@iRonin

@iRonin iRonin commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

The in-memory _sessions dictionary in api/auth.py accumulated expired session tokens indefinitely. Each login added a new entry with a 24-hour TTL, but only the specific token being verified was ever checked for expiry. All other expired entries persisted in memory for the lifetime of the server process.

Fix

Added _prune_expired_sessions() which removes all entries past their TTL. Called at the top of verify_session() so cleanup happens lazily during normal authenticated traffic — no background thread needed, zero overhead when no sessions have expired.

Testing

  • Existing tests pass (no behavioural change for valid sessions)
  • Manual verification: len(_sessions) no longer grows monotonically after multiple login cycles

Fixes #192

The in-memory _sessions dict accumulated expired tokens indefinitely —
entries were only removed when that specific token was verified. Add a
lazy _prune_expired_sessions() call at the top of verify_session() so
all expired entries are swept during normal traffic.

Addresses nesquena#192.
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Thanks for this — the fix is correct and the approach is right. A few notes from reviewing api/auth.py:

What's good:

  • Lazy cleanup on verify_session() is exactly the right pattern — no background thread, no lock contention, cleanup happens naturally during normal traffic
  • The _sessions.pop(t, None) with the default is safe even if a token was removed between the list comprehension and the pop

One thing to check:
The cleanup list comprehension iterates _sessions.items() without holding a lock. In CPython this is safe in practice (GIL protects dict iteration), but it's worth confirming whether _sessions is accessed from multiple threads simultaneously in this codebase. Looking at the existing code, verify_session() is called from check_auth() which is called per-request — under ThreadingHTTPServer, multiple request threads can call this concurrently. The existing code doesn't lock around _sessions reads/writes either (the _sessions.pop on line 121 is also unprotected), so this PR is consistent with the existing concurrency model. If a lock were added in the future, _prune_expired_sessions() would need to be inside it.

Tests: The PR description says existing tests pass, but I don't see new unit tests specifically for the prune behavior added in tests/. A test that calls verify_session() after inserting an expired entry and then checks len(_sessions) would be a nice addition, though not a blocker.

Overall this is a clean, minimal fix for a real issue. Ready for maintainer review.

Tests verify:
- Fresh session creation and validation
- Expired entries are pruned during verify_session() calls
- Valid sessions are never removed by pruning
- Empty dict is safe for pruning
- Session TTL matches expected 24-hour window
- invalidate_session() actually removes the token
- Invalidating non-existent tokens is safe
@nesquena

nesquena commented Apr 9, 2026

Copy link
Copy Markdown
Owner

Full Review: PR #196 — prune expired auth sessions

Thanks @iRonin! Solid fix for a real memory leak.

Security Audit

Clean. The change is limited to session cleanup logic. No new endpoints, no external resources, no injection vectors.

Code Review

The _prune_expired_sessions() implementation is correct — list comprehension creates a snapshot of expired tokens, then removes them. Calling it at the top of verify_session() is the right lazy-cleanup pattern: zero overhead when auth is disabled, automatic cleanup during normal authenticated traffic.

One edge case to note: under very high concurrent auth load, the dict iteration in _prune_expired_sessions() could race with create_session() modifying _sessions in another thread. In practice this is unlikely to matter for a WebUI (low concurrency), but a threading.Lock would make it bulletproof. Not a blocker.

Tests

Excellent test suite — 8 unit tests covering: valid session creation, expired session pruning, valid sessions preserved, prune-before-verify ordering, empty dict safety, TTL verification, invalidation, and unknown-token invalidation. Well-structured with proper setUp cleanup.

One concern with the test setup: os.environ["HERMES_WEBUI_STATE_DIR"] is set at import time which could leak into other test files if pytest imports them in the same process. The conftest.py in the main test suite already handles this for the integration tests, but unit tests importing api.auth directly with importlib could cause import-order issues. Worth watching but not blocking.

Test Results

514 passed, 0 failed, 41 skipped. No regressions.

Verdict

Approved. Ready to merge.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Thank you, @iRonin!

Six security PRs in a row — each one catching a real production issue. Session memory leak, CSP headers, connection timeout, HTTPS/TLS support, update branch tracking, CLI session API. This kind of focused, no-nonsense security hardening is exactly what a self-hosted tool needs. We've added you to the Contributors section in the README. Thank you.

JKJameson pushed a commit to JKJameson/hermes-webui that referenced this pull request Apr 25, 2026
…eak (nesquena#196)

* fix(auth): prune expired sessions on every verify to prevent memory leak

The in-memory _sessions dict accumulated expired tokens indefinitely —
entries were only removed when that specific token was verified. Add a
lazy _prune_expired_sessions() call at the top of verify_session() so
all expired entries are swept during normal traffic.

Addresses nesquena#192.

* test(auth): add 8 unit tests for session lifecycle and lazy pruning

Tests verify:
- Fresh session creation and validation
- Expired entries are pruned during verify_session() calls
- Valid sessions are never removed by pruning
- Empty dict is safe for pruning
- Session TTL matches expected 24-hour window
- invalidate_session() actually removes the token
- Invalidating non-existent tokens is safe
SysAdminDoc pushed a commit to SysAdminDoc/hermes-webui that referenced this pull request Jun 26, 2026
…eak (nesquena#196)

* fix(auth): prune expired sessions on every verify to prevent memory leak

The in-memory _sessions dict accumulated expired tokens indefinitely —
entries were only removed when that specific token was verified. Add a
lazy _prune_expired_sessions() call at the top of verify_session() so
all expired entries are swept during normal traffic.

Addresses nesquena#192.

* test(auth): add 8 unit tests for session lifecycle and lazy pruning

Tests verify:
- Fresh session creation and validation
- Expired entries are pruned during verify_session() calls
- Valid sessions are never removed by pruning
- Empty dict is safe for pruning
- Session TTL matches expected 24-hour window
- invalidate_session() actually removes the token
- Invalidating non-existent tokens is safe
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.

Security: Session dict memory leak — expired auth tokens never pruned

3 participants