From 2ce56535ba29b277224bae59cf5b7ad8c1dd932b Mon Sep 17 00:00:00 2001 From: Anten Skrabec Date: Wed, 29 Jul 2026 14:45:39 -0600 Subject: [PATCH] =?UTF-8?q?fix:=20harden=20RBAC=20=E2=80=94=20profile=20ac?= =?UTF-8?q?cess=20control,=20group=20permissions,=20role=20sync?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three security fixes: 1. Mod group handlers (groups_partial, new_group_card, save_groups) used Permission::ModsInstall instead of Permission::ConvoyManage — any user with mod install rights could manage convoy groups. 2. Profile and raid detail handlers had no ownership checks — any authenticated user could view any other user's profile, stash, quests, traders, hideout, and raid history. Now restricted to own data unless the user has UsersManage permission. 3. sync_builtin_role_permissions only synced admin role on upgrade, leaving moderator without permissions added in later migrations (items.give, mods.config_edit, notes.edit). Extended to sync moderator with its expected permission set. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/db/rbac.rs | 85 +++++++++++++++++++++++++++++------- src/web/auth.rs | 39 +++++++++++++++++ src/web/handlers/mods.rs | 6 +-- src/web/handlers/profiles.rs | 15 ++++--- src/web/handlers/raids.rs | 5 ++- 5 files changed, 125 insertions(+), 25 deletions(-) diff --git a/src/db/rbac.rs b/src/db/rbac.rs index b90d7608..f567ac90 100644 --- a/src/db/rbac.rs +++ b/src/db/rbac.rs @@ -478,24 +478,54 @@ pub enum DeleteRoleResult { HasUsers(i64), } -/// Idempotently ensure the admin role has all permissions from Permission::ALL. +/// Permissions that the moderator role should have. +/// Everything except admin-only permissions (UsersManage, SettingsManage, NotesManage). +const MODERATOR_PERMISSIONS: &[Permission] = &[ + Permission::ModsInstall, + Permission::ModsUpdate, + Permission::ModsRemove, + Permission::ModsDisable, + Permission::ModsConfigEdit, + Permission::ConvoyManage, + Permission::SvmEdit, + Permission::RequestsResolve, + Permission::ServerControl, + Permission::ServerLogs, + Permission::ServerMetrics, + Permission::HeadlessManage, + Permission::QueueManage, + Permission::ItemsGive, + Permission::NotesEdit, +]; + +/// Idempotently ensure built-in roles have their expected permissions. /// Called after migrations to pick up newly added permissions on upgrade. /// Additive only — never removes permissions. pub fn sync_builtin_role_permissions(conn: &Connection) -> rusqlite::Result<()> { - let admin_id: Option = conn - .query_row("SELECT id FROM roles WHERE name = 'admin'", [], |row| { - row.get(0) - }) - .optional()?; - - if let Some(id) = admin_id { - for perm in Permission::ALL { - conn.execute( - "INSERT OR IGNORE INTO role_permissions (role_id, permission) VALUES (?1, ?2)", - params![id, perm.as_str()], - )?; + // ponytail: single query per role, INSERT OR IGNORE is idempotent + let sync_role = |role_name: &str, perms: &[Permission]| -> rusqlite::Result<()> { + let role_id: Option = conn + .query_row( + "SELECT id FROM roles WHERE name = ?1", + params![role_name], + |row| row.get(0), + ) + .optional()?; + + if let Some(id) = role_id { + for perm in perms { + conn.execute( + "INSERT OR IGNORE INTO role_permissions (role_id, permission) VALUES (?1, ?2)", + params![id, perm.as_str()], + )?; + } } - } + Ok(()) + }; + + sync_role("admin", Permission::ALL)?; + sync_role("moderator", MODERATOR_PERMISSIONS)?; + // player intentionally has no permissions Ok(()) } @@ -534,10 +564,15 @@ mod tests { fn moderator_permissions() { let db = Database::open_in_memory().unwrap(); let perms = db.get_permissions_for_role("moderator").unwrap(); - assert!(perms.contains(&Permission::ModsInstall)); - assert!(perms.contains(&Permission::ServerControl)); + // Moderator should have exactly the MODERATOR_PERMISSIONS set + assert_eq!(perms.len(), MODERATOR_PERMISSIONS.len()); + for p in MODERATOR_PERMISSIONS { + assert!(perms.contains(p), "moderator missing {:?}", p); + } + // Admin-only permissions must be absent assert!(!perms.contains(&Permission::UsersManage)); assert!(!perms.contains(&Permission::SettingsManage)); + assert!(!perms.contains(&Permission::NotesManage)); } #[test] @@ -670,4 +705,22 @@ mod tests { assert_eq!(perms_after_sync.len(), Permission::ALL.len()); assert!(perms_after_sync.contains(&Permission::SettingsManage)); } + + #[test] + fn sync_adds_new_moderator_permissions() { + let db = Database::open_in_memory().unwrap(); + // Simulate a pre-upgrade state: remove a permission that sync should restore + db.conn().execute( + "DELETE FROM role_permissions WHERE role_id = (SELECT id FROM roles WHERE name = 'moderator') AND permission = 'items.give'", + [], + ).unwrap(); + let perms = db.get_permissions_for_role("moderator").unwrap(); + assert!(!perms.contains(&Permission::ItemsGive)); + + sync_builtin_role_permissions(db.conn()).unwrap(); + let perms = db.get_permissions_for_role("moderator").unwrap(); + assert!(perms.contains(&Permission::ItemsGive)); + // Still no admin-only permissions + assert!(!perms.contains(&Permission::UsersManage)); + } } diff --git a/src/web/auth.rs b/src/web/auth.rs index e1df8490..eb546a6a 100644 --- a/src/web/auth.rs +++ b/src/web/auth.rs @@ -99,6 +99,17 @@ pub fn require_permission( Ok(()) } +/// Require that the requesting user is viewing their own data or has UsersManage. +pub fn require_self_or_admin( + user: &SessionUser, + target_username: &str, +) -> std::result::Result<(), WebError> { + if user.username != target_username && !user.has_permission(Permission::UsersManage) { + return Err(WebError::Forbidden); + } + Ok(()) +} + pub fn set_session_user(session: &Session, user: &SessionUser) -> Result<()> { session .insert("user_id", user.user_id) @@ -313,6 +324,34 @@ mod tests { assert!(!user.can("nonexistent.perm")); } + #[test] + fn require_self_or_admin_allows_own_data() { + let user = SessionUser { + user_id: 1, + username: "alice".into(), + role_name: "player".into(), + role_display_name: "Player".into(), + permissions: HashSet::new(), + has_password: true, + }; + assert!(require_self_or_admin(&user, "alice").is_ok()); + assert!(require_self_or_admin(&user, "bob").is_err()); + } + + #[test] + fn require_self_or_admin_allows_users_manage() { + let user = SessionUser { + user_id: 1, + username: "admin".into(), + role_name: "admin".into(), + role_display_name: "Admin".into(), + permissions: HashSet::from([Permission::UsersManage]), + has_password: true, + }; + assert!(require_self_or_admin(&user, "alice").is_ok()); + assert!(require_self_or_admin(&user, "bob").is_ok()); + } + #[test] fn password_complexity_validation() { // Too short diff --git a/src/web/handlers/mods.rs b/src/web/handlers/mods.rs index ec9d45e2..0b7c83d0 100644 --- a/src/web/handlers/mods.rs +++ b/src/web/handlers/mods.rs @@ -3348,7 +3348,7 @@ pub async fn groups_partial( session: Session, ) -> actix_web::Result { let user = require_auth(&req)?; - require_permission(&user, Permission::ModsInstall)?; + require_permission(&user, Permission::ConvoyManage)?; let csrf_token = crate::web::csrf::get_or_create_token(&session); let html = render_groups_tab(&state, &csrf_token).await?; @@ -3370,7 +3370,7 @@ pub async fn new_group_card( query: Query, ) -> actix_web::Result { let user = require_auth(&req)?; - require_permission(&user, Permission::ModsInstall)?; + require_permission(&user, Permission::ConvoyManage)?; let _csrf_token = crate::web::csrf::get_or_create_token(&session); let all_mods = fetch_mods_with_client_files(&state).await?; @@ -3447,7 +3447,7 @@ pub async fn save_groups( body: web::Json, ) -> actix_web::Result { let user = require_auth(&req)?; - require_permission(&user, Permission::ModsInstall)?; + require_permission(&user, Permission::ConvoyManage)?; if !crate::web::csrf::validate_token(&session, &body.csrf_token) { return Err(WebError::Forbidden.into()); diff --git a/src/web/handlers/profiles.rs b/src/web/handlers/profiles.rs index 096d622d..127e6276 100644 --- a/src/web/handlers/profiles.rs +++ b/src/web/handlers/profiles.rs @@ -7,7 +7,7 @@ use askama::Template; use serde::Deserialize; use crate::spt::profiles::{load_profile_detail, load_stash_items, ProfileDetail, QuestState}; -use crate::web::auth::require_auth; +use crate::web::auth::{require_auth, require_self_or_admin}; use crate::web::csrf; use crate::web::error::WebError; use crate::web::flash::{take_flash, FlashMessage}; @@ -125,6 +125,7 @@ pub async fn profile_page( let flash = take_flash(&session); let csrf_token = csrf::get_or_create_token(&session); let profile_username = path.into_inner(); + require_self_or_admin(&user, &profile_username)?; let db = state.db.clone(); let dirs = Arc::clone(&state.dirs); @@ -252,8 +253,9 @@ pub async fn quests_partial( path: Path, query: Query, ) -> actix_web::Result { - require_auth(&req)?; + let user = require_auth(&req)?; let profile_username = path.into_inner(); + require_self_or_admin(&user, &profile_username)?; let detail = load_detail_for_user(&state, &profile_username).await?; let total = detail.quests.len(); @@ -306,8 +308,9 @@ pub async fn traders_partial( req: HttpRequest, path: Path, ) -> actix_web::Result { - require_auth(&req)?; + let user = require_auth(&req)?; let profile_username = path.into_inner(); + require_self_or_admin(&user, &profile_username)?; let detail = load_detail_for_user(&state, &profile_username).await?; let trader_displays: Vec = detail @@ -338,8 +341,9 @@ pub async fn hideout_partial( req: HttpRequest, path: Path, ) -> actix_web::Result { - require_auth(&req)?; + let user = require_auth(&req)?; let profile_username = path.into_inner(); + require_self_or_admin(&user, &profile_username)?; let detail = load_detail_for_user(&state, &profile_username).await?; let area_displays: Vec = detail @@ -405,8 +409,9 @@ pub async fn stash_partial( path: Path, query: Query, ) -> actix_web::Result { - require_auth(&req)?; + let user = require_auth(&req)?; let profile_username = path.into_inner(); + require_self_or_admin(&user, &profile_username)?; let db = state.db.clone(); let lookup_username = profile_username.clone(); diff --git a/src/web/handlers/raids.rs b/src/web/handlers/raids.rs index fb3cb7bc..da1e999b 100644 --- a/src/web/handlers/raids.rs +++ b/src/web/handlers/raids.rs @@ -5,7 +5,7 @@ use askama::Template; use serde::Deserialize; use crate::db::raids::{LeaderboardEntry, Raid, RaidKill, ServerRaidStats, UserRaidStats}; -use crate::web::auth::require_auth; +use crate::web::auth::{require_auth, require_self_or_admin}; use crate::web::csrf; use crate::web::error::WebError; use crate::web::flash::{take_flash, FlashMessage}; @@ -175,6 +175,7 @@ pub async fn player_raids_page( let flash = take_flash(&session); let csrf_token = csrf::get_or_create_token(&session); let profile_username = path.into_inner(); + require_self_or_admin(&user, &profile_username)?; let offset = query.offset.unwrap_or(0); let db = state.db.clone(); @@ -250,6 +251,8 @@ pub async fn raid_detail_page( .map_err(WebError::from)? .map_err(|_| WebError::NotFound)?; + require_self_or_admin(&user, &raid_username)?; + let tmpl = RaidDetailPageTemplate { user, flash,