feat: comprehensive moderator system with role hierarchy, suspensions, appeals, and audit logging#17
feat: comprehensive moderator system with role hierarchy, suspensions, appeals, and audit logging#17Syme-6005 wants to merge 41 commits into
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 7 high |
| Security | 1 high |
🟢 Metrics 71 complexity · 0 duplication
Metric Results Complexity 71 Duplication 0
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds role and status fields to users, enforces active and moderator checks in auth and API flows, adds moderator APIs and UI, deletes sessions on ban/suspend, and resets message decryption state when entering threads. ChangesModerator System and Message Handling Enhancements
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthServer as /api/auth
participant ModServer as /api/mod
participant Database as SQLite
rect rgba(100, 150, 200, 0.5)
Client->>AuthServer: POST /register (email, password)
AuthServer->>AuthServer: parse CIPHER_MODERATOR_EMAILS -> MODERATOR_EMAILS
AuthServer->>Database: INSERT users (role, status=active)
Database-->>AuthServer: OK
end
rect rgba(200, 150, 100, 0.5)
Client->>AuthServer: POST /login (email, password)
AuthServer->>Database: SELECT status FROM users WHERE email
alt status == banned
AuthServer-->>Client: 403 Forbidden
else
AuthServer-->>Client: JWT token
end
end
rect rgba(150, 200, 100, 0.5)
Client->>ModServer: POST /mod/users/{id}/status (action=ban)
ModServer->>Database: UPDATE users SET status = banned
ModServer->>Database: DELETE FROM sessions WHERE user_id = id
Database-->>ModServer: OK
ModServer-->>Client: updated user
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
The PR introduces a moderation system but contains critical security regressions and logic oversights that prevent it from being production-ready. Most notably, the introduction of new authentication dependencies has inadvertently removed CSRF protection from several sensitive backend endpoints, and the new moderation dashboard is vulnerable to Cross-Site Scripting (XSS) due to unsafe HTML rendering. Additionally, Codacy has flagged the PR as not up to standards, specifically noting high complexity in 'backend/main.py' which lacks any accompanying test coverage. These security flaws and the lack of automated validation for critical authorization paths must be addressed before merging.
About this PR
- Systemic Security Issue: The replacement of 'auth_dep' with status-specific dependencies has systematically removed CSRF protection from multiple POST and DELETE endpoints. A global fix is required to ensure all protected routes maintain CSRF validation.
- Missing Tests: The PR contains significant logic for account banning and access control but lacks new unit or integration tests to verify these critical security paths.
1 comment outside of the diff
backend/main.py
line 1127🟡 MEDIUM RISK
Suspended users can still retrieve group messages. Userequire_active_userto enforce the suspension.\n\nsuggestion\ndef group_messages_list(group_id: int, limit: int = 200, user = Depends(require_active_user)):\n
Test suggestions
- Verify that a user listed in CIPHER_MODERATOR_EMAILS is assigned the 'moderator' role upon registration.\n- [x] Verify that a 'banned' user is immediately rejected by current_user even if they have an active session cookie.\n- [x] Verify that a 'suspended' user is blocked from sending messages by the require_active_user guard.\n- [x] Verify that the moderator dashboard tab is hidden for standard users and visible for moderators.\n- [ ] Verify CSRF protection on new moderator POST endpoints (e.g., /api/mod/users/{user_id}/status).\n- [x] Verify that message decryption state is reset when switching from one DM thread to another.\n- [ ] Add unit tests for the complex authorization logic in
backend/main.pyto address coverage gaps.
Low confidence findings
- Consistency gap: While 'require_active_user' protects endpoints, suspended users can still appear authenticated in UI components that only use 'require_user'. Consider standardizing the check across all authentication-dependent UI elements.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| if (reports.length === 0) { | ||
| reportsList.innerHTML = `<div class="empty"><p>No recent reports.</p></div>`; | ||
| } else { | ||
| reportsList.innerHTML = reports.map(r => ` |
There was a problem hiding this comment.
🔴 HIGH RISK
Avoid assigning dynamic HTML strings to innerHTML. This pattern is susceptible to Cross-Site Scripting (XSS) attacks. Properties like messageId and groupMessageId are concatenated without sanitization. For a safer implementation, use document.createElement and textContent.
| return user | ||
|
|
||
|
|
||
| def require_active_user(user = Depends(require_user)): |
There was a problem hiding this comment.
🔴 HIGH RISK
Security regression: by replacing 'auth_dep' with 'require_active_user', CSRF protection has been removed from multiple sensitive endpoints including message_send and group_join. This dependency must include a CSRF check.
| return user | ||
|
|
||
|
|
||
| def require_moderator(user = Depends(require_user)): |
There was a problem hiding this comment.
🔴 HIGH RISK
This dependency lacks CSRF protection, representing a security regression. Moderator actions are vulnerable to CSRF because this dependency does not perform CSRF token validation. It should incorporate 'require_csrf' or depend on 'auth_dep'.\n\nsuggestion\ndef require_moderator(user = Depends(auth_dep)):\n
|
|
||
|
|
||
| @app.post("/api/mod/users/{user_id}/status") | ||
| def mod_user_status(user_id: int, body: ModUserActionIn, user = Depends(require_moderator)): |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Moderators can currently ban other moderators or admins. This endpoint should verify that the target user is not a moderator before applying a status change. Given the complexity of this file and lack of test coverage, this logic gap is high risk.\n\nTry running the following prompt in your coding agent:\n> In backend/main.py, update the mod_user_status function to check if the target user_id belongs to a moderator (by fetching their role and checking if their email is in MODERATOR_EMAILS) and raise an HTTPException(403) if a moderator attempts to modify another moderator's account.
| if not target: | ||
| raise HTTPException(404, "User not found") | ||
| conn.execute("UPDATE users SET status = ? WHERE id = ?", (status, user_id)) | ||
| if status == "banned": |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Sessions should be revoked for suspended users as well to ensure they are immediately blocked from the system.\n\nsuggestion\n if status in ("banned", "suspended"):\n
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@backend/main.py`:
- Around line 386-395: The guards must be chained so CSRF and active-account
checks run for mutating/moderator routes: change require_active_user to take its
user via Depends(require_csrf) instead of Depends(require_user), and change
require_moderator to take its user via Depends(require_active_user) (so
moderator checks automatically include CSRF + active status); update the
parameter defaults/signatures for require_active_user and require_moderator
accordingly and keep the same error behavior.
In `@static/js/app.js`:
- Around line 786-788: The toast uses a naive template `${action}ed` which
yields incorrect past-tense strings; update the success message after
api.post(`/api/mod/users/${userId}/status`, { action }) to use an explicit
mapping or switch on the action (e.g., map 'ban'->'banned',
'restore'->'restored') and pass the mapped past-tense label into toast instead
of `${action}ed`, then call loadModeration() as before.
- Around line 247-250: The boot path sets state.user directly but doesn't
refresh UI; ensure the moderator tab is synced whenever state.user is assigned
on startup by calling updateModeratorTab() from the boot() flow (the same way
loadUser() does) or by extracting the state.user assignment into a helper that
sets state.user and then calls updateModeratorTab(); update references in
boot(), loadUser(), and any existing-session logic so updateModeratorTab() runs
after state.user is populated.
- Around line 738-792: The loadModeration function and the per-user click
handler (the addEventListener on elements found via usersList.querySelectorAll)
call api.get and api.post without try/catch; wrap the API calls in try/catch
blocks (both the initial await api.get("/api/mod/stats"), the reports/users
fetches, and the await api.post(`/api/mod/users/${userId}/status`)) and on error
show a user-visible error (e.g., toast with the error message), avoid leaving
the UI half-rendered by ensuring fallback content is set for
reportsList/usersList and by re-enabling or leaving buttons intact; also catch
errors around the confirmDialog/api.post in the per-user handler so failures
don’t produce unhandled rejections and call loadModeration() only after a
successful post.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c4ab34c8-6c17-4fff-aae6-3ff608a5eea8
📒 Files selected for processing (3)
backend/main.pystatic/index.htmlstatic/js/app.js
- Replace unsafe innerHTML assignments with safe DOM manipulation (createElement, appendChild, textContent) - Fix moderation reports list rendering (line 748-785) - Fix moderation users list rendering (line 787-843) - Add try-catch error handling to loadModeration() function - Add error handling to async user action event listeners - Use textContent for user-controlled data instead of innerHTML templates Fixes XSS issues reported in static/js/app.js related to reports and users lists
|
@Syme-6005 will you fix the security issues or shall i? |
|
you please, im busy |
- Replace unsafe innerHTML assignments with replaceChildren() - Use textContent instead of innerHTML for error messages - Eliminate unencoded HTML in error display - Improve XSS vulnerability prevention
|
nevermind i guess |
…e checks, and session revocation - Fix CSRF protection in require_active_user and require_moderator dependencies - Add moderator privilege escalation prevention in mod_user_status - Extend session revocation to suspended users (not just banned) - Sync moderator tab visibility on boot() for existing sessions
- Fix toast message grammar for account actions (ban->banned, suspend->suspended, restore->restored) - Prevent suspended users from accessing group messages by using require_active_user
|
@dbwg2009 i cant fix it. ngl idk what the code space copilot is up to but its hallucinating and says it fixes the errors and then the smae errors come up. soooooo |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/main.py (1)
1559-1564:⚠️ Potential issue | 🟠 MajorUse the stored role when protecting moderator accounts.
This still only checks
target["email"]againstMODERATOR_EMAILS. Any account already persisted withrole = "moderator"but no longer present in the env list is still treated as a moderator byis_moderator_row(), yet this endpoint will let another moderator change its status. Fetchrolehere and reuse the same moderator predicate before applying any status change.🤖 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 `@backend/main.py` around lines 1559 - 1564, The endpoint fetches only id/email into target but then uses MODERATOR_EMAILS to protect moderators; change the query to also select role (e.g., SELECT id, email, role FROM users), and replace the email-based check with the existing moderator predicate (call is_moderator_row(target) or the same logic that checks role/email) before applying status changes; if the predicate is true and status != "active" raise HTTPException(403, "Cannot modify moderator accounts") so persisted moderator rows remain protected even if their email is not in MODERATOR_EMAILS.
🤖 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 `@backend/main.py`:
- Around line 355-356: The session validation currently only blocks "banned"
users so suspended accounts can re-authenticate; update the checks in the
session lookup (the if that inspects row["sess_exp"] and row["status"]) to also
treat "suspended" as invalid, and add the same status check to the login() flow
so authentication is refused for users with status == "suspended"; also ensure
require_user()/current_user() uses the user's status (not just session
existence) to deny access for suspended accounts and keep the same behavior
where suspension deletes sessions so re-login is impossible.
---
Duplicate comments:
In `@backend/main.py`:
- Around line 1559-1564: The endpoint fetches only id/email into target but then
uses MODERATOR_EMAILS to protect moderators; change the query to also select
role (e.g., SELECT id, email, role FROM users), and replace the email-based
check with the existing moderator predicate (call is_moderator_row(target) or
the same logic that checks role/email) before applying status changes; if the
predicate is true and status != "active" raise HTTPException(403, "Cannot modify
moderator accounts") so persisted moderator rows remain protected even if their
email is not in MODERATOR_EMAILS.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19c01256-66dd-4285-9028-d69db4512977
📒 Files selected for processing (2)
backend/main.pystatic/js/app.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
static/js/app.js (1)
738-748: 💤 Low valueConsider parallelizing the three API calls for faster load.
The stats, reports, and users fetches are independent and could run concurrently. This would reduce total load time from ~3 sequential round-trips to ~1.
♻️ Suggested parallel fetch pattern
async function loadModeration() { try { - const stats = await api.get("/api/mod/stats"); + const [stats, reports, users] = await Promise.all([ + api.get("/api/mod/stats"), + api.get("/api/mod/reports?limit=100"), + api.get("/api/mod/users?limit=200"), + ]); + $("mod-total-users").textContent = stats.totalUsers; $("mod-active-users").textContent = stats.activeUsers; $("mod-suspended-users").textContent = stats.suspendedUsers; $("mod-banned-users").textContent = stats.bannedUsers; $("mod-total-messages").textContent = stats.totalMessages; $("mod-total-reports").textContent = stats.totalReports; - const reports = await api.get("/api/mod/reports?limit=100"); const reportsList = $("mod-reports-list"); // ... rest of reports rendering ... - const users = await api.get("/api/mod/users?limit=200"); const usersList = $("mod-users-list"); // ... rest of users rendering ...🤖 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 `@static/js/app.js` around lines 738 - 748, The loadModeration function currently awaits the three independent API calls sequentially; change it to fetch stats, reports and users in parallel using Promise.all so the three api.get calls (for "/api/mod/stats", "/api/mod/reports?limit=100", and the users endpoint you use elsewhere, e.g., "/api/mod/users?limit=100") run concurrently, then destructure the results into stats, reports and users variables and update the DOM nodes ("mod-total-users", "mod-active-users", etc. and report/user rendering code) from those results; keep the existing try/catch and error handling around the Promise.all to preserve failure behavior.
🤖 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 `@backend/main.py`:
- Around line 1559-1564: The current moderator-protection only fetches email and
checks MODERATOR_EMAILS, which is inconsistent with is_moderator_row (which also
checks role == "moderator"); update the SELECT to also fetch the role (e.g.,
"SELECT id, email, role FROM users WHERE id = ?") and then use the same helper
logic as is_moderator_row (or reproduce its condition) to decide whether to
block status changes — replace the email-only check with a call to
is_moderator_row(target) or equivalent (checking role == "moderator" OR email in
MODERATOR_EMAILS) in the handler that raises HTTPException(403).
---
Nitpick comments:
In `@static/js/app.js`:
- Around line 738-748: The loadModeration function currently awaits the three
independent API calls sequentially; change it to fetch stats, reports and users
in parallel using Promise.all so the three api.get calls (for "/api/mod/stats",
"/api/mod/reports?limit=100", and the users endpoint you use elsewhere, e.g.,
"/api/mod/users?limit=100") run concurrently, then destructure the results into
stats, reports and users variables and update the DOM nodes ("mod-total-users",
"mod-active-users", etc. and report/user rendering code) from those results;
keep the existing try/catch and error handling around the Promise.all to
preserve failure behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e6b6945-6027-4a37-aeef-5f562142e489
📒 Files selected for processing (2)
backend/main.pystatic/js/app.js
|
@Syme-6005 sorry, ive been bogged down. fixing now |
- New super_moderator role with higher privileges than moderators
- Regular moderators cannot ban/suspend other moderators
- Suspension system with duration (1-365 days) and auto-restore on expiry
- Added suspended_until column with migration
- Fixed suspended user login blocking
- New /api/mod/users/{id}/role endpoint for role management
- Complete HTML injection audit: all user data properly escaped
- Improved role-based access controls throughout
Fixes privilege escalation where moderators could ban each other.
Moderator System Enhancements - TODO
Generated by Claude Code |
|
@Syme-6005 I've fixed a lot of errors and added some things. tested it and it works all ok. just need to complete the to-do list |
ngl i made my own in the meantime https://xorcrypt-6005.web.app/ |
@Syme-6005 looks good! I'm almost done with the mods features on this |
…stead Restore now has its own flow with a simple prompt for restoration reason, helping other mods understand the context without the formal dialog format of ban actions.
Add a dedicated restore dialog with three preset reasons: - Appeal approved - Manual review - no violation found - Mistaken suspension Reason is now required for restore actions to help other mods understand context.
- Add warning message explaining message will be decrypted and sent unencrypted to mods - Show preview of message being reported in report dialog - Add confirmation dialog before submitting report - Decrypt message using user's key and send decrypted content with report - Store message_content in reports table - Display message content in mod reports view - Add migration for message_content column
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 9 high |
| Security | 1 high |
🟢 Metrics 82 complexity · 0 duplication
Metric Results Complexity 82 Duplication 0
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
- Item 1: Message decryption for reports ✅ - Item 7: Restore dialog with preset reasons ✅ https://claude.ai/code/session_01Tq7iYMZVHmyeByUxiUj2iH
- Add AppealSubmitIn model with appeal_type, email confirmation, min 50 char reason - Add appeal_type column migration and endpoint support - Add GET /api/appeals/status endpoint for users to check appeal status - Modify POST /api/appeals to validate email and accept appeal_type - Update login endpoint to return restriction info (banned/suspended) instead of throwing errors - Return timeRemaining for suspended accounts to display on suspension screen - Support auto-restore on appeal approval https://claude.ai/code/session_01Tq7iYMZVHmyeByUxiUj2iH
- Add suspension screen (replaces login when user is banned/suspended) - Show suspension time remaining with readable format - Add appeal form page with: - Appeal type dropdown (mistaken, violated by mistake, circumstances changed) - Email confirmation field - Reason textarea (min 50 char, max 2000) - Character count display - Handle appeal submission and show success screen - Route to /appeal from suspension screen - Update login flow to detect restrictions and show suspension screen https://claude.ai/code/session_01Tq7iYMZVHmyeByUxiUj2iH
The suspension and appeal views weren't being toggled by setView(), causing blank screen when user is suspended. Now properly shows suspension screen with appeal option. https://claude.ai/code/session_01Tq7iYMZVHmyeByUxiUj2iH
Summary
Complete moderator system implementation with role-based access control, suspension management with auto-restore, appeals workflow, comprehensive audit logging, and enhanced moderation UX with search, filtering, and action templates.
Core Features Implemented
User Role & Status System
rolecolumn to users table (user, moderator, super_moderator)statuscolumn to users table (active, suspended, banned)suspended_untilcolumn for time-based suspension auto-restoreSuper Moderator Role Hierarchy
is_super_moderator()and updatedis_moderator_row()helper functionsSuspension System
Audit Logging System
mod_actionstable tracks all moderator activitiesGET /api/mod/audit-log?limit=100&offset=0with paginationAppeal System
appealstable tracks suspension/ban appeals with status (pending/approved/rejected)POST /api/appealswith reasonGET /api/mod/appeals?status=pendingPOST /api/mod/appeals/{id}with decision and responseSearch & Filtering
Moderator Dashboard Enhancements
Action Templates & Custom Reasons
Security Enhancements
escapeHtml()to prevent XSSBug Fixes
Issue #16: Report as spam validation error ✅
XSS Prevention
createElement()andtextContentCSRF Protection
Database Schema
Modified Tables
users: Added role, status, suspended_until columnsNew Tables
mod_actions(id, mod_id, target_id, action, reason, created_at)with indexes on mod_id, target_id, created_atappeals(id, user_id, status, reason, response, created_at, reviewed_at, reviewed_by)with indexes on user_id, statusBackend Endpoints
Moderation Endpoints:
GET /api/mod/stats- Moderation statisticsGET /api/mod/reports- List reports with filteringGET /api/mod/users- List users for moderationPOST /api/mod/users/{user_id}/status- Suspend/ban/restore userPOST /api/mod/users/{user_id}/role- Change user role (super_moderator only)GET /api/mod/audit-log- View moderation action historyGET /api/mod/appeals- View pending appealsPOST /api/mod/appeals/{appeal_id}- Review and decide on appealUser Endpoints:
POST /api/appeals- Submit suspension/ban appealGET /api/auth/me- Returns user status and roleFrontend Components
Moderation Dashboard:
Modals:
UX Features:
Testing Checklist
https://claude.ai/code/session_01Tq7iYMZVHmyeByUxiUj2iH
Summary by CodeRabbit