5 files changed, 167 insertions, 51 deletions

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),