fix(auth): durably revoke sessions on user delete/demote (survives restart)#106
Merged
Conversation
…start) Closes the highest-severity audit finding (CashPilot-rqw). Deleting or demoting a user only bumped the in-memory auth._USER_PWD_EPOCH cache; startup warm-up restored epochs solely from users.password_changed_at. So after any UI restart (webhook GitOps deploy, crash, reboot) the epoch reset to 0 and the account's still-valid 30-day signed cookie was honored again — a deleted owner regained owner, a demoted owner kept owner — because the role is read from the token claim with no DB re-fetch. Fix (keeps the fast no-DB-in-request-path guard unchanged): - New session_revocations table (user_id, revoked_before). DELIBERATELY no FK to users, so a deleted user's revocation outlives the row. - database.revoke_user_sessions() persists the revocation (monotonic — never lowers an existing one; prunes rows whose 30-day window has fully elapsed) and list_session_revocations() reads them back. - delete/demote routes now persist the revocation durably in addition to the in-memory epoch bump. - Startup warm-up (_warm_session_epochs, factored out + testable) restores the epoch cache from BOTH password_changed_at and session_revocations, taking the later timestamp per user — so revocations survive restarts. Freshly-minted tokens (issued at login with the current DB role) are unaffected. Tests: durable store (persist/monotonic/prune/survives-deletion) + the restart restore (revoke -> clear cache -> warm-up -> old cookie rejected, fresh accepted) + password/revocation merge. Full suite 1149 pass, ruff clean.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDurable per-user session revocation is stored in SQLite, applied during role changes and user deletion, restored into the auth epoch cache at startup, and covered by persistence, restart, merging, and route tests. ChangesSession revocation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
+ Coverage 93.04% 93.55% +0.51%
==========================================
Files 31 31
Lines 3276 3303 +27
==========================================
+ Hits 3048 3090 +42
+ Misses 228 213 -15
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_main_routes.py (1)
1042-1051: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the durable revocation calls.
These tests only patch the new calls, so they do not fail if route integration regresses.
tests/test_main_routes.py#L1042-L1051: bind the mock and assert one awaited role-update revocation.tests/test_main_routes.py#L1086-L1095: bind the mock and assert one awaited deletion revocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_main_routes.py` around lines 1042 - 1051, Bind the revoke_user_sessions mock in test_api_update_user_role and assert it was awaited exactly once after the PATCH request. In tests/test_main_routes.py lines 1086-1095, bind the deletion-revocation mock and assert it was awaited exactly once as well, preserving the existing response assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/routers/users.py`:
- Around line 52-54: The user mutation and session revocation must commit
atomically in both affected routes: in app/routers/users.py lines 52-54, combine
the role update with the revocation upsert; in lines 73-75, combine the
revocation upsert with user deletion. Update the in-memory epoch only after the
transaction commits, and add appropriate error handling and logging for
transaction failures.
In `@tests/test_session_revocation.py`:
- Around line 79-88: Update test_survives_user_deletion so run creates user 42
before calling revoke_user_sessions and delete_user. Ensure the created user is
actually deleted, then retain the assertion that user ID 42 remains in
list_session_revocations.
---
Outside diff comments:
In `@tests/test_main_routes.py`:
- Around line 1042-1051: Bind the revoke_user_sessions mock in
test_api_update_user_role and assert it was awaited exactly once after the PATCH
request. In tests/test_main_routes.py lines 1086-1095, bind the
deletion-revocation mock and assert it was awaited exactly once as well,
preserving the existing response assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eef9b4ad-9a07-4e87-bbbb-70f86ecaeb31
📒 Files selected for processing (6)
CHANGELOG.mdapp/database.pyapp/main.pyapp/routers/users.pytests/test_main_routes.pytests/test_session_revocation.py
Comment on lines
+52
to
+54
| revoked_at = time.time() | ||
| await main.database.revoke_user_sessions(user_id, revoked_at) | ||
| main.auth.set_user_pwd_epoch(user_id, revoked_at) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Make user mutation and session revocation one transaction.
Both routes commit the user mutation before durable revocation, allowing partial failures to leave old privileged cookies valid.
app/routers/users.py#L52-L54: atomically update the role and upsert the revocation.app/routers/users.py#L73-L75: atomically upsert the revocation and delete the user.
After the transaction commits, update the in-memory epoch.
As per path instructions, “Check for proper error handling and logging.”
📍 Affects 1 file
app/routers/users.py#L52-L54(this comment)app/routers/users.py#L73-L75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routers/users.py` around lines 52 - 54, The user mutation and session
revocation must commit atomically in both affected routes: in
app/routers/users.py lines 52-54, combine the role update with the revocation
upsert; in lines 73-75, combine the revocation upsert with user deletion. Update
the in-memory epoch only after the transaction commits, and add appropriate
error handling and logging for transaction failures.
Source: Path instructions
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…e window) Address CodeRabbit review on #106: - delete/demote now persist the session revocation BEFORE deleting/updating the user row, so a crash between the two writes can't leave the account mutated-but-not-revoked (old privileged cookie still valid). Revocation is keyed by uid with no FK, so persisting it first is valid. - test_survives_user_deletion now creates a real user first, so it genuinely proves the revocation is not dropped when a real users row is deleted (would catch a future cascading FK).
Owner
Author
|
Thanks @coderabbitai — both addressed in the latest commit:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline app/routers/users.py --items all --type function
ast-grep outline tests/test_session_revocation.py --items all --type class,function
printf '\n--- session-revocation calls in user routes ---\n'
rg -n -C 14 'revoke_user_sessions|update_user_role|delete_user' app/routers/users.py
printf '\n--- deletion-survival test ---\n'
rg -n -i -C 18 'delet.*surviv|surviv.*delet|create_user|delete_user' tests/test_session_revocation.pyLength of output: 8182
🐇 ✅ |
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
1 participant
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.
Closes the highest-severity finding from the security audit (
CashPilot-rqw).The bug
Deleting or demoting a user only bumped the in-memory
auth._USER_PWD_EPOCHcache, and startup warm-up restored epochs solely fromusers.password_changed_at. So after any UI restart (webhook GitOps deploy, crash, reboot):password_changed_atwas never touched → epoch not restored → their old cookie is accepted;and because
require_rolereads the role from the token claim with no DB re-fetch, a removed/demoted owner regains owner (create users, deploy socket-privileged containers = host RCE, reveal the fleet key) for up to 30 days.The fix (surgical; the fast no-DB-in-request-path guard is unchanged)
session_revocations(user_id, revoked_before)table — deliberately no FK tousers, so a deleted user's revocation outlives the row.database.revoke_user_sessions()persists it (monotonic — an older timestamp can't lower an existing revocation; prunes rows whose 30-day window has elapsed, since those tokens are already expired) +list_session_revocations()reads them back._warm_session_epochs()) restores the epoch cache from bothpassword_changed_atandsession_revocations, taking the later timestamp per user. Freshly-minted tokens (issued at login with the current DB role) are unaffected — a demoted user simply re-logs in and gets their new role.Why revocation-epoch rather than DB role re-fetch: it keeps the deliberate "no DB call in the request path" design (an explicit comment in
decode_session_token); the demoted/deleted user's old token is rejected outright, so the stale role in the token is never honored.Verification
ruff check .+ruff format --check .clean; 1149 tests pass. Newtests/test_session_revocation.py: durable store (persist / monotonic / prune / survives user deletion) + the restart restore (revoke → clear cache → warm-up → old cookie rejected, fresh cookie accepted with the new role) + password/revocation merge.Forward-only schema (
CREATE TABLE IF NOT EXISTSin_SCHEMA) — existing DBs get the table on next start. Not auto-merging: this touches the session-auth path and adds a table, so flagging for your review.Summary by CodeRabbit
Security
Bug Fixes
Tests