Add SECURITY_TASKS.md - security audit checklist, consolidate duplicate utils

SECURITY_TASKS.md

@@ -0,0 +1,265 @@
+# Security Audit Tasks - botserver
+
+**Priority:** CRITICAL
+**Auditor Focus:** Rust Security Best Practices
+
+---
+
+## 🔴 CRITICAL - Fix Immediately
+
+### 1. Remove All `.unwrap()` Calls (403 occurrences)
+
+```bash
+grep -rn "unwrap()" src --include="*.rs" | wc -l
+# Result: 403
+```
+
+**Action:** Replace every `.unwrap()` with:
+- `?` operator for propagating errors
+- `.unwrap_or_default()` for safe defaults
+- `.ok_or_else(|| Error::...)?` for custom errors
+
+**Files with highest count:**
+```bash
+grep -rn "unwrap()" src --include="*.rs" -c | sort -t: -k2 -rn | head -20
+```
+
+---
+
+### 2. Remove All `.expect()` Calls (76 occurrences)
+
+```bash
+grep -rn "\.expect(" src --include="*.rs" | wc -l
+# Result: 76
+```
+
+**Action:** Same as unwrap - use `?` or proper error handling.
+
+---
+
+### 3. SQL Injection Vectors - Dynamic Query Building
+
+**Location:** Multiple files build SQL with `format!`
+
+```
+src/basic/keywords/db_api.rs:168 - format!("SELECT COUNT(*) as count FROM {}", table_name)
+src/basic/keywords/db_api.rs:603 - format!("DELETE FROM {} WHERE id = $1", table_name)
+src/basic/keywords/db_api.rs:665 - format!("SELECT COUNT(*) as count FROM {}", table_name)
+```
+
+**Action:**
+- Validate `table_name` against whitelist of allowed tables
+- Use parameterized queries exclusively
+- Add schema validation before query execution
+
+---
+
+### 4. Command Injection Risk - External Process Execution
+
+**Locations:**
+```
+src/security/antivirus.rs - Command::new("powershell")
+src/core/kb/document_processor.rs - Command::new("pdftotext"), Command::new("pandoc")
+src/core/bot/manager.rs - Command::new("mc")
+src/nvidia/mod.rs - Command::new("nvidia-smi")
+```
+
+**Action:**
+- Never pass user input to command arguments
+- Use absolute paths for executables
+- Validate/sanitize all inputs before shell execution
+- Consider sandboxing or containerization
+
+---
+
+## 🟠 HIGH - Fix This Sprint
+
+### 5. Secrets in Memory
+
+**Concern:** API keys, passwords, tokens may persist in memory
+
+**Action:**
+- Use `secrecy` crate for sensitive data (`SecretString`, `SecretVec`)
+- Implement `Zeroize` trait for structs holding secrets
+- Clear secrets from memory after use
+
+---
+
+### 6. Missing Input Validation on API Endpoints
+
+**Action:** Add validation for all handler inputs:
+- Length limits on strings
+- Range checks on numbers
+- Format validation (emails, URLs, UUIDs)
+- Use `validator` crate with derive macros
+
+---
+
+### 7. Rate Limiting Missing
+
+**Action:**
+- Add rate limiting middleware to all public endpoints
+- Implement per-IP and per-user limits
+- Use `tower-governor` or similar
+
+---
+
+### 8. Missing Authentication Checks
+
+**Action:** Audit all handlers for:
+- Session validation
+- Permission checks (RBAC)
+- Bot ownership verification
+
+---
+
+### 9. CORS Configuration Review
+
+**Action:**
+- Restrict allowed origins (no wildcard `*` in production)
+- Validate Origin header
+- Set appropriate headers
+
+---
+
+### 10. File Path Traversal
+
+**Locations:** File serving, upload handlers
+
+**Action:**
+- Canonicalize paths before use
+- Validate paths are within allowed directories
+- Use `sanitize_path_component` consistently
+
+---
+
+## 🟡 MEDIUM - Fix Next Sprint
+
+### 11. Logging Sensitive Data
+
+**Action:**
+- Audit all `log::*` calls for sensitive data
+- Never log passwords, tokens, API keys
+- Redact PII in logs
+
+---
+
+### 12. Error Message Information Disclosure
+
+**Action:**
+- Return generic errors to clients
+- Log detailed errors server-side only
+- Never expose stack traces to users
+
+---
+
+### 13. Cryptographic Review
+
+**Action:**
+- Verify TLS 1.3 minimum
+- Check certificate validation
+- Review encryption algorithms used
+- Ensure secure random number generation (`rand::rngs::OsRng`)
+
+---
+
+### 14. Dependency Audit
+
+```bash
+cargo audit
+cargo deny check
+```
+
+**Action:**
+- Fix all reported vulnerabilities
+- Remove unused dependencies
+- Pin versions in Cargo.lock
+
+---
+
+### 15. TODO/FIXME Comments (Security-Related)
+
+```
+src/auto_task/autotask_api.rs:1829 - TODO: Fetch from database
+src/auto_task/autotask_api.rs:1849 - TODO: Implement recommendation
+```
+
+**Action:** Complete or remove all TODO comments.
+
+---
+
+## 🟢 LOW - Backlog
+
+### 16. Add Security Headers
+
+- `X-Content-Type-Options: nosniff`
+- `X-Frame-Options: DENY`
+- `Content-Security-Policy`
+- `Strict-Transport-Security`
+
+### 17. Implement Request ID Tracking
+
+- Add unique ID to each request
+- Include in logs for tracing
+
+### 18. Database Connection Pool Hardening
+
+- Set max connections
+- Implement connection timeouts
+- Add health checks
+
+### 19. Add Panic Handler
+
+- Catch panics at boundaries
+- Log and return 500, don't crash
+
+### 20. Memory Limits
+
+- Set max request body size
+- Limit file upload sizes
+- Implement streaming for large files
+
+---
+
+## Verification Commands
+
+```bash
+# Check for unwrap
+grep -rn "unwrap()" src --include="*.rs" | wc -l
+
+# Check for expect
+grep -rn "\.expect(" src --include="*.rs" | wc -l
+
+# Check for panic
+grep -rn "panic!" src --include="*.rs" | wc -l
+
+# Check for unsafe
+grep -rn "unsafe" src --include="*.rs"
+
+# Check for SQL injection vectors
+grep -rn "format!.*SELECT\|format!.*INSERT\|format!.*UPDATE\|format!.*DELETE" src --include="*.rs"
+
+# Check for command execution
+grep -rn "Command::new\|std::process::Command" src --include="*.rs"
+
+# Run security audit
+cargo audit
+
+# Check dependencies
+cargo deny check
+```
+
+---
+
+## Acceptance Criteria
+
+- [ ] 0 `.unwrap()` calls in production code (tests excluded)
+- [ ] 0 `.expect()` calls in production code
+- [ ] 0 `panic!` macros
+- [ ] 0 `unsafe` blocks (or documented justification)
+- [ ] All SQL uses parameterized queries
+- [ ] All external commands validated
+- [ ] `cargo audit` shows 0 vulnerabilities
+- [ ] Rate limiting on all public endpoints
+- [ ] Input validation on all handlers
+- [ ] Secrets use `secrecy` crate

src/auto_task/app_generator.rs

@@ -2,6 +2,7 @@ use crate::auto_task::app_logs::{log_generator_error, log_generator_info};
 use crate::basic::keywords::table_definition::{
     generate_create_table_sql, FieldDefinition, TableDefinition,
 };
+use crate::core::shared::get_content_type;
 use crate::core::shared::models::UserSession;
 use crate::core::shared::state::AppState;
 use aws_sdk_s3::primitives::ByteStream;
@@ -685,7 +686,7 @@ Respond with valid JSON only."#
     ) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
         if let Some(ref s3) = self.state.s3_client {
             let body = ByteStream::from(content.as_bytes().to_vec());
-            let content_type = Self::get_content_type(path);
+            let content_type = get_content_type(path);
 
             s3.put_object()
                 .bucket(bucket)
@@ -710,7 +711,7 @@ Respond with valid JSON only."#
         content: &str,
     ) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
         let mut conn = self.state.conn.get()?;
-        let content_type = Self::get_content_type(path);
+        let content_type = get_content_type(path);
 
         sql_query(
             "INSERT INTO drive_files (id, bucket, path, content, content_type, size, created_at, updated_at)
@@ -733,21 +734,6 @@ Respond with valid JSON only."#
         Ok(())
     }
 
-    fn get_content_type(path: &str) -> &'static str {
-        let ext = path.rsplit('.').next().unwrap_or("").to_lowercase();
-        match ext.as_str() {
-            "html" | "htm" => "text/html; charset=utf-8",
-            "css" => "text/css; charset=utf-8",
-            "js" => "application/javascript; charset=utf-8",
-            "json" => "application/json; charset=utf-8",
-            "bas" => "text/plain; charset=utf-8",
-            "png" => "image/png",
-            "jpg" | "jpeg" => "image/jpeg",
-            "svg" => "image/svg+xml",
-            _ => "application/octet-stream",
-        }
-    }
-
     fn sync_tables_to_database(
         &self,
         tables: &[TableDefinition],

src/basic/keywords/app_server.rs

@@ -1,3 +1,4 @@
+use crate::core::shared::{get_content_type, sanitize_path_component};
 use crate::shared::state::AppState;
 use axum::{
     body::Body,
@@ -136,40 +137,7 @@ pub async fn list_all_apps(State(state): State<Arc<AppState>>) -> impl IntoRespo
         .into_response()
 }
 
-fn sanitize_path_component(component: &str) -> String {
-    component
-        .replace("..", "")
-        .replace("//", "/")
-        .trim_start_matches('/')
-        .trim_end_matches('/')
-        .chars()
-        .filter(|c| c.is_alphanumeric() || *c == '-' || *c == '_' || *c == '.' || *c == '/')
-        .collect()
-}
 
-fn get_content_type(file_path: &str) -> &'static str {
-    let ext = file_path.rsplit('.').next().unwrap_or("").to_lowercase();
-
-    match ext.as_str() {
-        "html" | "htm" => "text/html; charset=utf-8",
-        "css" => "text/css; charset=utf-8",
-        "js" => "application/javascript; charset=utf-8",
-        "json" => "application/json; charset=utf-8",
-        "png" => "image/png",
-        "jpg" | "jpeg" => "image/jpeg",
-        "gif" => "image/gif",
-        "svg" => "image/svg+xml",
-        "ico" => "image/x-icon",
-        "woff" => "font/woff",
-        "woff2" => "font/woff2",
-        "ttf" => "font/ttf",
-        "eot" => "application/vnd.ms-fontobject",
-        "txt" => "text/plain; charset=utf-8",
-        "xml" => "application/xml; charset=utf-8",
-        "pdf" => "application/pdf",
-        _ => "application/octet-stream",
-    }
-}
 
 #[cfg(test)]
 mod tests {

src/basic/keywords/data_operations.rs

@@ -1,4 +1,5 @@
 use super::table_access::{check_table_access, AccessType, UserRoles};
+use crate::core::shared::{sanitize_identifier, sanitize_sql_value};
 use crate::shared::models::UserSession;
 use crate::shared::state::AppState;
 use crate::shared::utils::{json_value_to_dynamic, to_array};
@@ -455,12 +456,12 @@ fn execute_save(
     let id_value = id.to_string();
 
     let mut columns: Vec<String> = vec!["id".to_string()];
-    let mut values: Vec<String> = vec![format!("'{}'", sanitize_sql(&id_value))];
+    let mut values: Vec<String> = vec![format!("'{}'", sanitize_sql_value(&id_value))];
     let mut update_sets: Vec<String> = Vec::new();
 
     for (key, value) in &data_map {
         let sanitized_key = sanitize_identifier(key);
-        let sanitized_value = format!("'{}'", sanitize_sql(&value.to_string()));
+        let sanitized_value = format!("'{}'", sanitize_sql_value(&value.to_string()));
         columns.push(sanitized_key.clone());
         values.push(sanitized_value.clone());
         update_sets.push(format!("{} = {}", sanitized_key, sanitized_value));
@@ -501,7 +502,7 @@ fn execute_insert(
 
     for (key, value) in &data_map {
         columns.push(sanitize_identifier(key));
-        values.push(format!("'{}'", sanitize_sql(&value.to_string())));
+        values.push(format!("'{}'", sanitize_sql_value(&value.to_string())));
     }
 
     let query = format!(
@@ -557,7 +558,7 @@ fn execute_update(
         update_sets.push(format!(
             "{} = '{}'",
             sanitize_identifier(key),
-            sanitize_sql(&value.to_string())
+            sanitize_sql_value(&value.to_string())
         ));
     }
 
@@ -627,7 +628,7 @@ fn execute_merge(
             "SELECT COUNT(*) as cnt FROM {} WHERE {} = '{}'",
             sanitize_identifier(table),
             sanitize_identifier(key_field),
-            sanitize_sql(&key_value)
+            sanitize_sql_value(&key_value)
         );
 
         #[derive(QueryableByName)]
@@ -648,7 +649,7 @@ fn execute_merge(
                     update_sets.push(format!(
                         "{} = '{}'",
                         sanitize_identifier(key),
-                        sanitize_sql(&value.to_string())
+                        sanitize_sql_value(&value.to_string())
                     ));
                 }
             }
@@ -659,7 +660,7 @@ fn execute_merge(
                     sanitize_identifier(table),
                     update_sets.join(", "),
                     sanitize_identifier(key_field),
-                    sanitize_sql(&key_value)
+                    sanitize_sql_value(&key_value)
                 );
                 let _ = sql_query(&update_query).execute(conn);
                 updated += 1;
@@ -670,7 +671,7 @@ fn execute_merge(
 
             for (key, value) in &item_map {
                 columns.push(sanitize_identifier(key));
-                values.push(format!("'{}'", sanitize_sql(&value.to_string())));
+                values.push(format!("'{}'", sanitize_sql_value(&value.to_string())));
             }
 
             let insert_query = format!(
@@ -968,7 +969,7 @@ fn parse_filter_clause(filter: &str) -> Result<String, Box<dyn Error + Send + Sy
         "{} {} '{}'",
         sanitize_identifier(&field),
         sql_operator,
-        sanitize_sql(&value)
+        sanitize_sql_value(&value)
     ))
 }
 
@@ -997,15 +998,7 @@ fn parse_condition_internal(
     Err("Invalid condition format".into())
 }
 
-fn sanitize_identifier(name: &str) -> String {
-    name.chars()
-        .filter(|c| c.is_ascii_alphanumeric() || *c == '_')
-        .collect()
-}
 
-fn sanitize_sql(value: &str) -> String {
-    value.replace('\'', "''")
-}
 
 #[cfg(test)]
 mod tests {
@@ -1022,10 +1015,10 @@ mod tests {
     }
 
     #[test]
-    fn test_sanitize_sql() {
+    fn test_sanitize_sql_value() {
-        assert_eq!(sanitize_sql("hello"), "hello");
+        assert_eq!(sanitize_sql_value("hello"), "hello");
-        assert_eq!(sanitize_sql("it's"), "it''s");
+        assert_eq!(sanitize_sql_value("it's"), "it''s");
-        assert_eq!(sanitize_sql("O'Brien"), "O''Brien");
+        assert_eq!(sanitize_sql_value("O'Brien"), "O''Brien");
     }
 
     #[test]

src/basic/keywords/db_api.rs

@@ -2,6 +2,7 @@ use super::table_access::{
     check_field_write_access, check_table_access, filter_fields_by_role, AccessType, UserRoles,
 };
 use crate::core::shared::state::AppState;
+use crate::core::shared::sanitize_identifier;
 use crate::core::urls::ApiUrls;
 use axum::{
     extract::{Path, Query, State},
@@ -110,12 +111,6 @@ pub fn configure_db_routes() -> Router<Arc<AppState>> {
         )
 }
 
-fn sanitize_identifier(name: &str) -> String {
-    name.chars()
-        .filter(|c| c.is_alphanumeric() || *c == '_')
-        .collect()
-}
-
 pub async fn list_records_handler(
     State(state): State<Arc<AppState>>,
     headers: HeaderMap,

src/basic/keywords/on_change.rs

@@ -1,3 +1,4 @@
+use crate::core::shared::sanitize_path_for_filename;
 use diesel::prelude::*;
 use log::{error, info, trace};
 use rhai::{Dynamic, Engine};
@@ -289,18 +290,7 @@ fn register_on_change_with_events(state: &AppState, user: UserSession, engine: &
         .unwrap();
 }
 
-pub fn sanitize_path_for_filename(path: &str) -> String {
+
-    let mut result = path.to_string();
-    for ch in ['/', '\\', ':'] {
-        result = result.replace(ch, "_");
-    }
-    result
-        .replace([' ', '.'], "_")
-        .chars()
-        .filter(|c| c.is_alphanumeric() || *c == '_' || *c == '-')
-        .collect::<String>()
-        .to_lowercase()
-}
 
 pub fn execute_on_change(
     conn: &mut diesel::PgConnection,

src/basic/keywords/table_definition.rs

@@ -28,6 +28,7 @@
 |                                                                             |
 \*****************************************************************************/
 
+use crate::core::shared::sanitize_identifier;
 use crate::shared::models::UserSession;
 use crate::shared::state::AppState;
 use diesel::prelude::*;
@@ -399,12 +400,6 @@ pub fn generate_create_table_sql(table: &TableDefinition, driver: &str) -> Strin
     sql
 }
 
-fn sanitize_identifier(name: &str) -> String {
-    name.chars()
-        .filter(|c| c.is_alphanumeric() || *c == '_')
-        .collect()
-}
-
 pub fn load_connection_config(
     state: &AppState,
     bot_id: Uuid,

src/core/shared/mod.rs

@@ -43,7 +43,10 @@ pub use models::{
     Task, TriggerKind, User, UserLoginToken, UserPreference, UserSession,
 };
 
-pub use utils::{create_conn, DbPool};
+pub use utils::{
+    create_conn, get_content_type, sanitize_identifier, sanitize_path_component,
+    sanitize_path_for_filename, sanitize_sql_value, DbPool,
+};
 
 
 

src/core/shared/utils.rs

@@ -307,3 +307,62 @@ pub fn run_migrations(pool: &DbPool) -> Result<(), Box<dyn std::error::Error + S
     )?;
     Ok(())
 }
+
+pub fn sanitize_identifier(name: &str) -> String {
+    name.chars()
+        .filter(|c| c.is_ascii_alphanumeric() || *c == '_')
+        .collect()
+}
+
+pub fn sanitize_path_component(component: &str) -> String {
+    component
+        .chars()
+        .filter(|c| c.is_alphanumeric() || *c == '_' || *c == '-' || *c == '.')
+        .collect::<String>()
+        .trim_start_matches('.')
+        .to_string()
+}
+
+pub fn sanitize_path_for_filename(path: &str) -> String {
+    path.chars()
+        .map(|c| if c.is_alphanumeric() || c == '_' || c == '-' { c } else { '_' })
+        .collect()
+}
+
+pub fn get_content_type(path: &str) -> &'static str {
+    let ext = std::path::Path::new(path)
+        .extension()
+        .and_then(|e| e.to_str())
+        .map(|e| e.to_lowercase());
+
+    match ext.as_deref() {
+        Some("html") | Some("htm") => "text/html; charset=utf-8",
+        Some("css") => "text/css; charset=utf-8",
+        Some("js") => "application/javascript; charset=utf-8",
+        Some("json") => "application/json; charset=utf-8",
+        Some("bas") => "text/plain; charset=utf-8",
+        Some("png") => "image/png",
+        Some("jpg") | Some("jpeg") => "image/jpeg",
+        Some("gif") => "image/gif",
+        Some("svg") => "image/svg+xml",
+        Some("ico") => "image/x-icon",
+        Some("woff") => "font/woff",
+        Some("woff2") => "font/woff2",
+        Some("ttf") => "font/ttf",
+        Some("eot") => "application/vnd.ms-fontobject",
+        Some("otf") => "font/otf",
+        Some("txt") => "text/plain; charset=utf-8",
+        Some("xml") => "application/xml; charset=utf-8",
+        Some("pdf") => "application/pdf",
+        Some("zip") => "application/zip",
+        Some("webp") => "image/webp",
+        Some("mp3") => "audio/mpeg",
+        Some("mp4") => "video/mp4",
+        Some("webm") => "video/webm",
+        _ => "application/octet-stream",
+    }
+}
+
+pub fn sanitize_sql_value(value: &str) -> String {
+    value.replace('\'', "''")
+}

src/designer/mod.rs

@@ -1,4 +1,5 @@
 use crate::auto_task::get_designer_error_context;
+use crate::core::shared::get_content_type;
 use crate::core::urls::ApiUrls;
 use crate::shared::state::AppState;
 use axum::{
@@ -1325,18 +1326,3 @@ async fn apply_file_change(
 
     Ok(())
 }
-
-fn get_content_type(filename: &str) -> &'static str {
-    match Path::new(filename)
-        .extension()
-        .and_then(|e| e.to_str())
-        .map(|e| e.to_lowercase())
-        .as_deref()
-    {
-        Some("html") => "text/html",
-        Some("css") => "text/css",
-        Some("js") => "application/javascript",
-        Some("json") => "application/json",
-        _ => "text/plain",
-    }
-}