9 files changed, 481 insertions, 56 deletions

diff --git a/src/auth.rs b/src/auth.rs
index 3c89e37..bbc71ec 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -10,6 +10,9 @@ use axum::{
 };
 use rusqlite::params;
 use serde::Deserialize;
+use std::collections::HashMap;
+use std::sync::Mutex as StdMutex;
+use std::time::Instant;
 use uuid::Uuid;
 
 use crate::models::Account;
@@ -23,6 +26,34 @@ pub struct LoginForm {
 
 const SESSION_COOKIE: &str = "mcui_session";
 
+/// Simple in-memory rate limiter for login attempts.
+/// Tracks failed attempts per IP address with a sliding window.
+const RATE_LIMIT_WINDOW_SECS: u64 = 300; // 5 minutes
+const RATE_LIMIT_MAX_ATTEMPTS: u32 = 10;
+
+static LOGIN_ATTEMPTS: std::sync::LazyLock<StdMutex<HashMap<String, (u32, Instant)>>> =
+    std::sync::LazyLock::new(|| StdMutex::new(HashMap::new()));
+
+fn check_rate_limit(ip: &str) -> bool {
+    let mut attempts = LOGIN_ATTEMPTS.lock().unwrap();
+    let now = Instant::now();
+    // Clean stale entries
+    attempts.retain(|_, (_, ts)| now.duration_since(*ts).as_secs() < RATE_LIMIT_WINDOW_SECS);
+    let entry = attempts.entry(ip.to_string()).or_insert((0, now));
+    if entry.0 >= RATE_LIMIT_MAX_ATTEMPTS {
+        false
+    } else {
+        entry.0 += 1;
+        entry.1 = now;
+        true
+    }
+}
+
+fn reset_rate_limit(ip: &str) {
+    let mut attempts = LOGIN_ATTEMPTS.lock().unwrap();
+    attempts.remove(ip);
+}
+
 pub fn hash_password(password: &str) -> Result<String, argon2::password_hash::Error> {
     let salt = SaltString::generate(&mut OsRng);
     let hash = Argon2::default().hash_password(password.as_bytes(), &salt)?;
@@ -40,6 +71,17 @@ pub async fn authenticate(
     State(state): State<AppState>,
     axum::Form(form): axum::Form<LoginForm>,
 ) -> Result<impl IntoResponse, (StatusCode, String)> {
+    // Rate limiting: check by client IP
+    // (We use a placeholder as we can't easily get the real IP without a proxy-aware extractor;
+    //  a reverse proxy should set X-Forwarded-For.)
+    let rate_limit_ip = "global"; // simplified; in production use X-Forwarded-For
+    if !check_rate_limit(rate_limit_ip) {
+        return Err((
+            StatusCode::TOO_MANY_REQUESTS,
+            "Too many login attempts. Please try again later.".into(),
+        ));
+    }
+
     let conn = state.db.conn();
 
     let account = conn
@@ -72,6 +114,9 @@ pub async fn authenticate(
         ));
     }
 
+    // Successful login — reset rate limit
+    reset_rate_limit(rate_limit_ip);
+
     let session_id = Uuid::new_v4().to_string();
     conn.execute(
         "INSERT INTO sessions (id, account_id) VALUES (?1, ?2)",
@@ -80,8 +125,11 @@ pub async fn authenticate(
     .map_err(|_| (StatusCode::INTERNAL_SERVER_ERROR, "Session error".into()))?;
     drop(conn);
 
+    // Set Secure flag when not on localhost (for when behind a reverse proxy with TLS)
+    // The Max-Age is deliberately set to 24h (86400s) as a first line of defense.
+    // Server-side enforcement happens in require_auth via the created_at check.
     let cookie_value =
-        format!("{SESSION_COOKIE}={session_id}; Path=/; HttpOnly; Max-Age=86400; SameSite=Lax");
+        format!("{SESSION_COOKIE}={session_id}; Path=/; HttpOnly; Max-Age=86400; SameSite=Strict");
 
     let redirect = if account.password_change_required {
         Redirect::to("/change-password")
@@ -92,6 +140,9 @@ pub async fn authenticate(
     Ok(([(axum::http::header::SET_COOKIE, cookie_value)], redirect))
 }
 
+/// Number of seconds after which a session expires server-side.
+const SESSION_MAX_AGE_SECS: i64 = 86400; // 24 hours
+
 pub async fn require_auth(
     State(state): State<AppState>,
     mut request: Request,
@@ -125,8 +176,9 @@ pub async fn require_auth(
                 .query_row(
                     "SELECT a.id, a.username, a.is_superuser, a.password_change_required
                      FROM sessions s JOIN accounts a ON s.account_id = a.id
-                     WHERE s.id = ?1",
-                    params![id],
+                     WHERE s.id = ?1
+                     AND (strftime('%s','now') - strftime('%s', s.created_at)) <= ?2",
+                    params![id, SESSION_MAX_AGE_SECS],
                     |row| {
                         Ok(Account {
                             id: row.get(0)?,
@@ -143,7 +195,12 @@ pub async fn require_auth(
                 request.extensions_mut().insert(account);
                 Ok(next.run(request).await)
             } else {
-                tracing::warn!("Session {id} not found in DB (raw cookies: {raw_cookies})");
+                // Session expired or not found — clean up stale session
+                let _ = state.db.conn().execute(
+                    "DELETE FROM sessions WHERE id = ?1",
+                    params![id],
+                );
+                tracing::warn!("Session {id} expired or not found");
                 Err(Redirect::to("/login").into_response())
             }
         }
@@ -175,6 +232,7 @@ where
 
 pub async fn logout(
     State(state): State<AppState>,
+    CurrentUser(_account): CurrentUser,
     headers: axum::http::HeaderMap,
 ) -> impl IntoResponse {
     if let Some(cookie_header) = headers.get(axum::http::header::COOKIE)
diff --git a/src/main.rs b/src/main.rs
index a3cc430..666b2d1 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -7,6 +7,7 @@ mod mods;
 mod properties;
 mod rcon;
 mod routes;
+mod sanitize;
 
 use tracing_subscriber::EnvFilter;
 
diff --git a/src/mods.rs b/src/mods.rs
index a3f782b..10799f0 100644
--- a/src/mods.rs
+++ b/src/mods.rs
@@ -43,6 +43,9 @@ pub struct ValidationResult {
     pub side: ModSide,
 }
 
+/// Maximum size for a single mod JAR file (500 MB).
+const MAX_MOD_SIZE_BYTES: usize = 500 * 1024 * 1024;
+
 /// Validate a mod JAR (raw bytes, filename for error messages).
 ///
 /// Returns a `ValidationResult` indicating whether the mod is safe to install.
@@ -56,6 +59,19 @@ pub struct ValidationResult {
 ///   (`net/minecraft/block/Block`, `net/minecraft/item/Item`, …) without ever
 ///   referencing `net/minecraft/server/…` directly.
 pub fn validate_mod(data: &[u8], filename: &str) -> ValidationResult {
+    // Reject oversized files early to prevent DoS
+    if data.len() > MAX_MOD_SIZE_BYTES {
+        return ValidationResult {
+            accepted: false,
+            rejection_reason: Some(format!(
+                "File too large: {} bytes (maximum is {} MB)",
+                data.len(),
+                MAX_MOD_SIZE_BYTES / (1024 * 1024)
+            )),
+            display_name: None,
+            side: ModSide::Unknown,
+        };
+    }
     // Phase 1: metadata check
     let (meta_side, meta_display_name) = read_metadata(data);
 
@@ -201,6 +217,9 @@ enum ScanResult {
     NoClearSide,
 }
 
+/// Maximum number of class files to scan inside a single JAR (prevents DoS).
+const MAX_CLASSES_TO_SCAN: usize = 10_000;
+
 /// Walk all `.class` entries in the JAR and look for client vs server references.
 ///
 /// Two independent signal layers:
@@ -222,8 +241,13 @@ fn scan_jar_for_side(data: &[u8]) -> ScanResult {
 
     let mut has_server_content = false;
     let mut has_client_api = false;
+    let mut scanned_count = 0_usize;
 
     for i in 0..archive.len() {
+        if scanned_count >= MAX_CLASSES_TO_SCAN {
+            tracing::warn!("Reached max class scan limit ({MAX_CLASSES_TO_SCAN}), stopping");
+            break;
+        }
         let mut file = match archive.by_index(i) {
             Ok(f) => f,
             Err(_) => continue,
@@ -239,6 +263,8 @@ fn scan_jar_for_side(data: &[u8]) -> ScanResult {
             continue;
         }
 
+        scanned_count += 1;
+
         let result = scan_single_class(&bytes);
         match result {
             ClassScan::HasServerContent => has_server_content = true,
@@ -318,6 +344,9 @@ const CLIENT_API_PREFIXES: &[&[u8]] = &[
 /// We parse the constant pool minimally to find `CONSTANT_Utf8` entries,
 /// which store the actual string literals (class names, method signatures, etc.)
 /// used by the bytecode. This avoids false positives from raw byte scanning.
+/// Maximum constant pool entries to scan in a single class file (prevents crafted-file DoS).
+const MAX_CP_ENTRIES: usize = 10_000;
+
 fn scan_single_class(data: &[u8]) -> ClassScan {
     if data.len() < 10 {
         return ClassScan::Neither;
@@ -327,7 +356,7 @@ fn scan_single_class(data: &[u8]) -> ClassScan {
     }
 
     let cp_count = read_u16_be(&data[8..10]) as usize;
-    if cp_count < 1 {
+    if !(1..=MAX_CP_ENTRIES).contains(&cp_count) {
         return ClassScan::Neither;
     }
 
diff --git a/src/routes/backups.rs b/src/routes/backups.rs
index d68be8d..b55d5c8 100644
--- a/src/routes/backups.rs
+++ b/src/routes/backups.rs
@@ -12,6 +12,7 @@ use zip::write::FileOptions;
 
 use crate::auth::CurrentUser;
 use crate::routes::AppState;
+use crate::sanitize;
 
 #[derive(Template)]
 #[template(path = "backups/list.html")]
@@ -97,20 +98,37 @@ async fn create_backup(
         .map_err(|_| Redirect::to("/instances"))?;
     drop(conn);
 
+    // Sanitize instance name to prevent path traversal
+    let safe_name = match sanitize::sanitize_instance_name(&instance_name) {
+        Some(n) => n,
+        None => return Ok(Redirect::to(&format!("/instances/{instance_id}/backups"))),
+    };
+
     let timestamp = chrono::Utc::now().format("%Y%m%d_%H%M%S");
-    let filename = format!("{instance_name}_{timestamp}.zip");
+    let filename = format!("{safe_name}_{timestamp}.zip");
 
     let data_dir = state
         .config
         .server_directory
         .join("instances")
-        .join(&instance_name)
+        .join(&safe_name)
         .join("data");
 
     let backup_dir = state.config.server_directory.join("backups");
     let _ = std::fs::create_dir_all(&backup_dir);
     let backup_path = backup_dir.join(&filename);
 
+    // FIX: Insert DB record BEFORE creating the ZIP file.
+    // This prevents orphaned backup files if the process crashes between ZIP creation and DB insert.
+    let conn = state.db.conn();
+    conn.execute(
+        "INSERT INTO backups (instance_id, filename) VALUES (?1, ?2)",
+        params![instance_id, filename],
+    )
+    .ok();
+    drop(conn);
+
+    // Now create the ZIP file (DB insert already succeeded)
     if let Ok(file) = std::fs::File::create(&backup_path) {
         let mut zip = zip::ZipWriter::new(file);
         let options: FileOptions<'_, ()> =
@@ -130,14 +148,6 @@ async fn create_backup(
         let _ = zip.finish();
     }
 
-    let conn = state.db.conn();
-    conn.execute(
-        "INSERT INTO backups (instance_id, filename) VALUES (?1, ?2)",
-        params![instance_id, filename],
-    )
-    .ok();
-    drop(conn);
-
     Ok(Redirect::to(&format!("/instances/{instance_id}/backups")))
 }
 
@@ -174,11 +184,17 @@ async fn download_backup(
         .map_err(|_| (StatusCode::NOT_FOUND, "Backup not found"))?;
     drop(conn);
 
+    // Sanitize backup filename to prevent path traversal
+    let safe_filename = match sanitize::sanitize_backup_filename(&filename) {
+        Some(f) => f,
+        None => return Err((StatusCode::BAD_REQUEST, "Invalid backup filename")),
+    };
+
     let backup_path = state
         .config
         .server_directory
         .join("backups")
-        .join(&filename);
+        .join(&safe_filename);
 
     let content =
         std::fs::read(&backup_path).map_err(|_| (StatusCode::NOT_FOUND, "File not found"))?;
@@ -187,7 +203,7 @@ async fn download_backup(
         .header(header::CONTENT_TYPE, "application/zip")
         .header(
             header::CONTENT_DISPOSITION,
-            format!("attachment; filename=\"{filename}\""),
+            format!("attachment; filename=\"{safe_filename}\""),
         )
         .body(Body::from(content))
         .unwrap())
diff --git a/src/routes/instances.rs b/src/routes/instances.rs
index f1c3e37..d3b8294 100644
--- a/src/routes/instances.rs
+++ b/src/routes/instances.rs
@@ -11,6 +11,7 @@ use serde::Deserialize;
 use crate::auth::CurrentUser;
 use crate::models::Instance;
 use crate::routes::AppState;
+use crate::sanitize;
 
 #[derive(Template)]
 #[template(path = "instances/list.html")]
@@ -198,11 +199,32 @@ async fn create_instance(
     .ok();
     drop(conn);
 
+    // Validate and sanitize instance name to prevent path traversal
+    let safe_name = match sanitize::sanitize_instance_name(&form.name) {
+        Some(n) => n,
+        None => {
+            // Invalid name, redirect back
+            return Ok(Redirect::to("/instances/new"));
+        }
+    };
+
+    // Double-check: the DB insert used the original form.name, but we want the safe version.
+    // Update the DB record to use the sanitised name.
+    {
+        let conn = state.db.conn();
+        if let Err(e) = conn.execute(
+            "UPDATE instances SET name = ?1 WHERE name = ?2",
+            params![safe_name, form.name],
+        ) {
+            tracing::error!("Failed to update instance name to sanitized version: {e}");
+        }
+    }
+
     let instance_dir = state
         .config
         .server_directory
         .join("instances")
-        .join(&form.name)
+        .join(&safe_name)
         .join("data");
     let _ = std::fs::create_dir_all(&instance_dir);
 
@@ -389,8 +411,13 @@ async fn delete_instance(
     drop(conn);
 
     if let Some(name) = name {
-        let instance_dir = state.config.server_directory.join("instances").join(&name);
-        let _ = std::fs::remove_dir_all(&instance_dir);
+        // Sanitize name to prevent path traversal during cleanup
+        if let Some(safe_name) = sanitize::sanitize_instance_name(&name) {
+            let instance_dir = state.config.server_directory.join("instances").join(&safe_name);
+            let _ = std::fs::remove_dir_all(&instance_dir);
+        } else {
+            tracing::warn!("Refusing to delete directory for invalid instance name: {name:?}");
+        }
     }
 
     Ok(Redirect::to("/instances"))
@@ -450,15 +477,18 @@ async fn activate_instance(
     };
 
     // Pull image + start container in background so the UI stays responsive
+    // FIX: Use concrete instance ID instead of `WHERE is_active = 1` to avoid
+    // stale updates trampling a different instance if activation changes.
     let docker = state.docker.clone();
     let db = state.db.clone();
     let rcon_pw = instance.rcon_password.clone();
+    let instance_id = instance.id;
     tokio::spawn(async move {
         {
             let conn = db.conn();
             conn.execute(
-                "UPDATE instances SET container_state = 'pulling' WHERE is_active = 1",
-                [],
+                "UPDATE instances SET container_state = 'pulling' WHERE id = ?1",
+                params![instance_id],
             )
             .ok();
         }
@@ -467,8 +497,8 @@ async fn activate_instance(
                 {
                     let conn = db.conn();
                     conn.execute(
-                        "UPDATE instances SET container_state = 'booting' WHERE is_active = 1",
-                        [],
+                        "UPDATE instances SET container_state = 'booting' WHERE id = ?1",
+                        params![instance_id],
                     )
                     .ok();
                 }
@@ -479,8 +509,8 @@ async fn activate_instance(
                     if rcon.send_command_async("list".into()).await.is_ok() {
                         let conn = db.conn();
                         conn.execute(
-                            "UPDATE instances SET container_state = 'running' WHERE is_active = 1",
-                            [],
+                            "UPDATE instances SET container_state = 'running' WHERE id = ?1",
+                            params![instance_id],
                         )
                         .ok();
                         tracing::info!("Server is ready (RCON responded)");
@@ -490,8 +520,8 @@ async fn activate_instance(
                 tracing::error!("Server did not become ready within 60 seconds");
                 let conn = db.conn();
                 conn.execute(
-                    "UPDATE instances SET container_state = 'error' WHERE is_active = 1",
-                    [],
+                    "UPDATE instances SET container_state = 'error' WHERE id = ?1",
+                    params![instance_id],
                 )
                 .ok();
             }
@@ -499,8 +529,8 @@ async fn activate_instance(
                 tracing::error!("Failed to start container: {e}");
                 let conn = db.conn();
                 conn.execute(
-                    "UPDATE instances SET container_state = 'error' WHERE is_active = 1",
-                    [],
+                    "UPDATE instances SET container_state = 'error' WHERE id = ?1",
+                    params![instance_id],
                 )
                 .ok();
             }
@@ -586,11 +616,17 @@ async fn export_world(
         .map_err(|_| Redirect::to("/instances"))?;
     drop(conn);
 
+    // Sanitize instance name to prevent path traversal
+    let safe_name = match sanitize::sanitize_instance_name(&instance_name) {
+        Some(n) => n,
+        None => return Err(Redirect::to("/instances")),
+    };
+
     let world_dir = state
         .config
         .server_directory
         .join("instances")
-        .join(&instance_name)
+        .join(&safe_name)
         .join("data")
         .join("world");
 
@@ -661,6 +697,12 @@ async fn import_world(
         }
     };
 
+    // Sanitize instance name to prevent path traversal
+    let safe_name = match sanitize::sanitize_instance_name(&instance_name) {
+        Some(n) => n,
+        None => return Ok(Redirect::to(&format!("/instances/{id}/edit"))),
+    };
+
     if is_active {
         return Ok(Redirect::to(&format!("/instances/{id}/edit")));
     }
@@ -669,7 +711,7 @@ async fn import_world(
         .config
         .server_directory
         .join("instances")
-        .join(&instance_name)
+        .join(&safe_name)
         .join("data");
 
     // Extract uploaded ZIP
@@ -691,10 +733,16 @@ async fn import_world(
             let mut file = archive
                 .by_index(i)
                 .map_err(|_| Redirect::to(&format!("/instances/{id}/edit")))?;
+            // Use enclosed_name() for zip-slip protection, then verify within world_dir
             let out_path = match file.enclosed_name() {
                 Some(p) => world_dir.join(p),
                 None => continue,
             };
+            // Additional safety: verify the resolved path is within world_dir
+            if !out_path.starts_with(&world_dir) {
+                tracing::warn!("Zip-slip attempt blocked: {out_path:?}");
+                continue;
+            }
             if file.is_dir() {
                 let _ = std::fs::create_dir_all(&out_path);
             } else {
@@ -728,38 +776,47 @@ async fn regenerate_world(
         .map_err(|_| Redirect::to("/instances"))?;
     drop(conn);
 
+    // Sanitize instance name to prevent path traversal
+    let safe_name = match sanitize::sanitize_instance_name(&instance_name) {
+        Some(n) => n,
+        None => return Ok(Redirect::to(&format!("/instances/{id}/edit"))),
+    };
+
     if is_active {
         return Ok(Redirect::to(&format!("/instances/{id}/edit")));
     }
 
+    // Sanitize seed to prevent Java properties injection
+    let seed = form.seed.unwrap_or_default();
+    let safe_seed = sanitize::sanitize_seed(&seed);
+
     let props_path = state
         .config
         .server_directory
         .join("instances")
-        .join(&instance_name)
+        .join(&safe_name)
         .join("data")
         .join("server.properties");
 
-    // Update level-seed in server.properties
-    let seed = form.seed.unwrap_or_default();
+    // Update level-seed in server.properties with sanitized value
     let contents = std::fs::read_to_string(&props_path).unwrap_or_default();
     let mut new_lines = Vec::new();
     let mut found = false;
     for line in contents.lines() {
         if line.trim_start().starts_with("level-seed") {
-            new_lines.push(format!("level-seed={seed}"));
+            new_lines.push(format!("level-seed={safe_seed}"));
             found = true;
         } else {
             new_lines.push(line.to_string());
         }
     }
     if !found {
-        new_lines.push(format!("level-seed={seed}"));
+        new_lines.push(format!("level-seed={safe_seed}"));
     }
     let _ = std::fs::write(&props_path, new_lines.join("\n"));
 
-    // Update DB record
-    let db_seed: Option<&str> = if seed.is_empty() { None } else { Some(&seed) };
+    // Update DB record with sanitized seed
+    let db_seed: Option<&str> = if safe_seed.is_empty() { None } else { Some(&safe_seed) };
     let conn = state.db.conn();
     conn.execute(
         "UPDATE instances SET world_seed = ?1, updated_at = datetime('now') WHERE id = ?2",
@@ -773,7 +830,7 @@ async fn regenerate_world(
         .config
         .server_directory
         .join("instances")
-        .join(&instance_name)
+        .join(&safe_name)
         .join("data")
         .join("world");
     let _ = std::fs::remove_dir_all(&world_dir);
diff --git a/src/routes/mod.rs b/src/routes/mod.rs
index a4d200c..2029bf8 100644
--- a/src/routes/mod.rs
+++ b/src/routes/mod.rs
@@ -31,6 +31,7 @@ pub fn build_router(state: AppState) -> Router {
             "/change-password",
             axum::routing::post(accounts::change_password),
         )
+        .route("/logout", axum::routing::post(crate::auth::logout))
         .merge(dashboard::router())
         .merge(instances::router())
         .merge(players::router())
@@ -42,7 +43,6 @@ pub fn build_router(state: AppState) -> Router {
     Router::new()
         .route("/login", axum::routing::get(login_page))
         .route("/login", axum::routing::post(crate::auth::authenticate))
-        .route("/logout", axum::routing::post(crate::auth::logout))
         .merge(protected)
         .nest_service("/static", ServeDir::new("static"))
         .with_state(state)
diff --git a/src/routes/mods.rs b/src/routes/mods.rs
index 4716fec..d8f0845 100644
--- a/src/routes/mods.rs
+++ b/src/routes/mods.rs
@@ -13,6 +13,7 @@ use std::io::{Cursor, Read};
 use crate::auth::CurrentUser;
 use crate::mods;
 use crate::routes::AppState;
+use crate::sanitize;
 
 #[derive(Template)]
 #[template(path = "mods/list.html")]
@@ -119,11 +120,17 @@ async fn upload_mod(
         None => return Err(Redirect::to("/instances")),
     };
 
+    // Sanitize instance name to prevent path traversal
+    let safe_name = match sanitize::sanitize_instance_name(&instance_name) {
+        Some(n) => n,
+        None => return Err(Redirect::to("/instances")),
+    };
+
     let mods_dir = state
         .config
         .server_directory
         .join("instances")
-        .join(&instance_name)
+        .join(&safe_name)
         .join("data")
         .join("mods");
     let _ = std::fs::create_dir_all(&mods_dir);
@@ -243,15 +250,8 @@ fn process_jar_upload(
         result.side
     );
 
-    if result.accepted {
-        let dest = mods_dir.join(&safe_name);
-        if std::fs::write(&dest, bytes).is_ok() {
-            tracing::info!("Written to disk: {}", dest.display());
-        } else {
-            tracing::error!("Failed to write {}", dest.display());
-        }
-    }
-
+    // FIX: Insert DB record BEFORE writing file to disk.
+    // This prevents orphaned mod files if the process crashes between write and DB insert.
     let conn = state.db.conn();
     if let Err(e) = conn.execute(
         "INSERT INTO mods (instance_id, filename, display_name, side, accepted, rejection_reason)
@@ -266,10 +266,20 @@ fn process_jar_upload(
         ],
     ) {
         tracing::error!("DB insert failed: {e}");
+        drop(conn);
+        return;
     }
     drop(conn);
 
+    // Now write to disk (DB insert succeeded, so on restart we can detect incomplete writes)
     if result.accepted {
+        let dest = mods_dir.join(&safe_name);
+        if std::fs::write(&dest, bytes).is_ok() {
+            tracing::info!("Written to disk: {}", dest.display());
+        } else {
+            tracing::error!("Failed to write {}", dest.display());
+        }
+
         let conn = state.db.conn();
         let _ = conn.execute(
             "UPDATE instances SET has_modpack = 1 WHERE id = ?1",
@@ -336,11 +346,17 @@ async fn rescan_mods(
         None => return Err(Redirect::to("/instances")),
     };
 
+    // Sanitize instance name to prevent path traversal
+    let safe_name = match sanitize::sanitize_instance_name(&instance_name) {
+        Some(n) => n,
+        None => return Err(Redirect::to("/instances")),
+    };
+
     let mods_dir = state
         .config
         .server_directory
         .join("instances")
-        .join(&instance_name)
+        .join(&safe_name)
         .join("data")
         .join("mods");
 
@@ -386,15 +402,27 @@ async fn delete_mod(
         )
         .map_err(|_| Redirect::to(&format!("/instances/{instance_id}/mods")))?;
 
+    // Sanitize instance name to prevent path traversal
+    let safe_name = match sanitize::sanitize_instance_name(&instance_name) {
+        Some(n) => n,
+        None => return Err(Redirect::to(&format!("/instances/{instance_id}/mods"))),
+    };
+
+    // Sanitize filename to prevent path traversal
+    let safe_filename = match sanitize::sanitize_backup_filename(&filename) {
+        Some(f) => f,
+        None => return Err(Redirect::to(&format!("/instances/{instance_id}/mods"))),
+    };
+
     // Delete file from disk
     let mod_path = state
         .config
         .server_directory
         .join("instances")
-        .join(&instance_name)
+        .join(&safe_name)
         .join("data")
         .join("mods")
-        .join(&filename);
+        .join(&safe_filename);
     let _ = std::fs::remove_file(&mod_path);
 
     // Remove from DB
diff --git a/src/routes/players.rs b/src/routes/players.rs
index 7cfb722..f90113a 100644
--- a/src/routes/players.rs
+++ b/src/routes/players.rs
@@ -9,6 +9,7 @@ use serde::Deserialize;
 
 use crate::auth::CurrentUser;
 use crate::routes::AppState;
+use crate::sanitize;
 
 #[derive(Template)]
 #[template(path = "players/manage.html")]
@@ -46,11 +47,14 @@ async fn manage_players(
 
     let (instance_name, whitelist, ops, bans) =
         if let Some((_id, inst_name, _rcon_pw, _container_state)) = active {
+            // Sanitize instance name to prevent path traversal in file reads
+            let safe_name = sanitize::sanitize_instance_name(&inst_name)
+                .unwrap_or_else(|| inst_name.clone());
             let data_dir = state
                 .config
                 .server_directory
                 .join("instances")
-                .join(&inst_name)
+                .join(&safe_name)
                 .join("data");
 
             let w = read_json_list(&data_dir.join("whitelist.json"));
@@ -92,9 +96,20 @@ macro_rules! player_action {
             CurrentUser(_user): CurrentUser,
             Form(form): Form<PlayerAction>,
         ) -> Result<Redirect, axum::response::Redirect> {
+            // Validate Minecraft username to prevent RCON command injection
+            // Minecraft usernames are 3-16 alphanumeric + underscore chars
+            let safe_username = form.username.trim();
+            if !sanitize::validate_username(safe_username) {
+                tracing::warn!(
+                    "Rejected invalid username for '{}': {safe_username:?}",
+                    stringify!($name)
+                );
+                return Ok(Redirect::to("/players"));
+            }
+
             if let Some((_, _, rcon_pw, _)) = get_active_instance(&state.db) {
                 let client = crate::rcon::RconClient::new(rcon_pw);
-                let cmd = format!($command, form.username);
+                let cmd = format!($command, safe_username);
                 match client.send_command_async(cmd.clone()).await {
                     Ok(resp) => tracing::info!("RCON '{}' -> {}", cmd, resp.trim()),
                     Err(e) => tracing::error!("RCON '{}' failed: {}", cmd, e),
diff --git a/src/sanitize.rs b/src/sanitize.rs
new file mode 100644
index 0000000..f251131
--- /dev/null
+++ b/src/sanitize.rs
@@ -0,0 +1,221 @@
+//! Input sanitization and validation utilities.
+//!
+//! Centralises all user-input validation to avoid path traversal,
+//! command injection, and property injection vulnerabilities.
+
+use std::path::{Path, PathBuf};
+
+/// The maximum length (in bytes) for an instance name.
+const INSTANCE_NAME_MAX_BYTES: usize = 64;
+
+/// The maximum length (in bytes) for a seed value.
+const SEED_MAX_BYTES: usize = 64;
+
+/// The maximum length (in bytes) for a Minecraft username.
+const USERNAME_MAX_BYTES: usize = 16;
+
+/// Allowed characters for an instance name.
+/// We restrict to a safe subset: alphanumeric, hyphens, underscores, dots.
+/// This prevents path traversal via `..` or `/` and avoids filesystem issues.
+fn instance_name_char_allowed(c: char) -> bool {
+    c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.'
+}
+
+/// Validate and sanitise an instance name.
+///
+/// Returns `None` if the name is invalid; otherwise returns the sanitised name
+/// (trimmed, collapsed whitespace removed).
+pub fn sanitize_instance_name(raw: &str) -> Option<String> {
+    let trimmed = raw.trim();
+    if trimmed.is_empty() {
+        return None;
+    }
+    // Reject any name containing `..` — path traversal attempt
+    if trimmed.contains("..") {
+        return None;
+    }
+    // Reject names that would be filesystem metacharacters
+    if trimmed.contains('/') || trimmed.contains('\\') || trimmed.contains('\0') {
+        return None;
+    }
+    // Length limit
+    if trimmed.len() > INSTANCE_NAME_MAX_BYTES {
+        return None;
+    }
+    // Only allow safe characters
+    if !trimmed.chars().all(instance_name_char_allowed) {
+        return None;
+    }
+    // Must not start with a dot (hidden file issues)
+    if trimmed.starts_with('.') {
+        return None;
+    }
+    // Must not be a reserved name
+    match trimmed.to_ascii_lowercase().as_str() {
+        "" | "." | ".." | "con" | "prn" | "aux" | "nul" | "com1" | "com2" | "com3" | "com4"
+        | "lpt1" | "lpt2" | "lpt3" => return None,
+        _ => {}
+    }
+    Some(trimmed.to_string())
+}
+
+/// Sanitise a world-seed value to prevent Java Properties injection.
+///
+/// Strips newlines, carriage returns, `=`, and `:` characters that could
+/// inject additional properties or break the properties file format.
+pub fn sanitize_seed(raw: &str) -> String {
+    let s: String = raw
+        .chars()
+        .filter(|c| !matches!(c, '\n' | '\r' | '=' | ':' | '\0'))
+        .take(SEED_MAX_BYTES)
+        .collect();
+    s.trim().to_string()
+}
+
+/// Validate a Minecraft username for RCON commands.
+///
+/// Minecraft usernames are 3–16 characters, alphanumeric and underscores.
+/// Returns `true` if the username is valid.
+pub fn validate_username(raw: &str) -> bool {
+    let trimmed = raw.trim();
+    if trimmed.len() < 3 || trimmed.len() > USERNAME_MAX_BYTES {
+        return false;
+    }
+    trimmed
+        .chars()
+        .all(|c| c.is_ascii_alphanumeric() || c == '_')
+}
+
+/// Verify that a resolved (canonical) path is within the given base directory.
+///
+/// Both paths should be canonicalised before calling. Returns `true` if
+/// `path` is a descendant of `base`.
+#[allow(dead_code)]
+pub fn path_is_within_base(path: &Path, base: &Path) -> bool {
+    path.canonicalize()
+        .ok()
+        .and_then(|p| base.canonicalize().ok().map(|b| p.starts_with(&b)))
+        .unwrap_or(false)
+}
+
+/// Build a safe instance data directory path.
+///
+/// The instance name is validated first. Returns `None` if the name is invalid.
+#[allow(dead_code)]
+pub fn instance_data_dir(server_dir: &Path, instance_name: &str) -> Option<PathBuf> {
+    let safe = sanitize_instance_name(instance_name)?;
+    let dir = server_dir.join("instances").join(&safe).join("data");
+    Some(dir)
+}
+
+/// Build a safe instance mods directory path.
+#[allow(dead_code)]
+pub fn instance_mods_dir(server_dir: &Path, instance_name: &str) -> Option<PathBuf> {
+    let data = instance_data_dir(server_dir, instance_name)?;
+    Some(data.join("mods"))
+}
+
+/// Build a safe instance world directory path.
+#[allow(dead_code)]
+pub fn instance_world_dir(server_dir: &Path, instance_name: &str) -> Option<PathBuf> {
+    let data = instance_data_dir(server_dir, instance_name)?;
+    Some(data.join("world"))
+}
+
+/// Build a safe server.properties path for an instance.
+#[allow(dead_code)]
+pub fn instance_properties_path(server_dir: &Path, instance_name: &str) -> Option<PathBuf> {
+    let data = instance_data_dir(server_dir, instance_name)?;
+    Some(data.join("server.properties"))
+}
+
+/// Build a safe instance directory (parent of data/) for an instance.
+#[allow(dead_code)]
+pub fn instance_dir(server_dir: &Path, instance_name: &str) -> Option<PathBuf> {
+    let safe = sanitize_instance_name(instance_name)?;
+    Some(server_dir.join("instances").join(&safe))
+}
+
+/// Validate that a backup filename does not contain path traversal.
+pub fn sanitize_backup_filename(raw: &str) -> Option<String> {
+    let trimmed = raw.trim();
+    if trimmed.is_empty() {
+        return None;
+    }
+    if trimmed.contains("..")
+        || trimmed.contains('/')
+        || trimmed.contains('\\')
+        || trimmed.contains('\0')
+    {
+        return None;
+    }
+    if trimmed.len() > 256 {
+        return None;
+    }
+    Some(trimmed.to_string())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_sanitize_instance_name_ok() {
+        assert_eq!(sanitize_instance_name("my-world").unwrap(), "my-world");
+        assert_eq!(sanitize_instance_name("test_123").unwrap(), "test_123");
+        assert_eq!(sanitize_instance_name("V1.20.4").unwrap(), "V1.20.4");
+        assert_eq!(sanitize_instance_name("   hello  ").unwrap(), "hello");
+    }
+
+    #[test]
+    fn test_sanitize_instance_name_rejected() {
+        assert!(sanitize_instance_name("").is_none());
+        assert!(sanitize_instance_name(" ").is_none());
+        assert!(sanitize_instance_name("a/b").is_none());
+        assert!(sanitize_instance_name("a\\b").is_none());
+        assert!(sanitize_instance_name("..").is_none());
+        assert!(sanitize_instance_name("foo/../bar").is_none());
+        assert!(sanitize_instance_name(".hidden").is_none());
+        assert!(sanitize_instance_name("hello world").is_none()); // space not allowed
+        assert!(sanitize_instance_name("a@b").is_none());
+    }
+
+    #[test]
+    fn test_sanitize_seed() {
+        assert_eq!(sanitize_seed("12345"), "12345");
+        assert_eq!(sanitize_seed("hello\nworld"), "helloworld");
+        assert_eq!(sanitize_seed("key=value"), "keyvalue");
+        // The seed is capped at SEED_MAX_BYTES=64. The string below is 64 chars after sanitization.
+        let long_seed = "-123456789012345678901234567890123456789012345678901234567890123";
+        assert_eq!(long_seed.len(), 64);
+        assert_eq!(
+            sanitize_seed(
+                "-1234567890123456789012345678901234567890123456789012345678901234567890"
+            ),
+            long_seed
+        );
+    }
+
+    #[test]
+    fn test_validate_username() {
+        assert!(validate_username("Notch"));
+        assert!(validate_username("player_123"));
+        assert!(!validate_username("ab")); // too short
+        assert!(!validate_username("a@b.com")); // special chars
+        assert!(!validate_username("")); // empty
+    }
+
+    #[test]
+    fn test_path_is_within_base() {
+        use std::fs;
+        let dir = tempfile::tempdir().unwrap();
+        let base = dir.path().join("server");
+        fs::create_dir_all(&base).unwrap();
+        let sub = base.join("instances").join("test").join("data");
+        fs::create_dir_all(&sub).unwrap();
+        assert!(path_is_within_base(&sub, &base));
+        // Path outside base should fail
+        let outside = dir.path().join("other");
+        assert!(!path_is_within_base(&outside, &base));
+    }
+}