From 561264521cef6fa73a737ec43840b5130746abd4 Mon Sep 17 00:00:00 2001 From: "Rodrigo Rodriguez (Pragmatismo)" Date: Sun, 28 Dec 2025 15:32:48 -0300 Subject: [PATCH] Add SECURITY_TASKS.md - security audit checklist, consolidate duplicate utils --- SECURITY_TASKS.md | 265 +++++++++++++++++++++++++ src/auto_task/app_generator.rs | 20 +- src/basic/keywords/app_server.rs | 34 +--- src/basic/keywords/data_operations.rs | 35 ++-- src/basic/keywords/db_api.rs | 7 +- src/basic/keywords/on_change.rs | 14 +- src/basic/keywords/table_definition.rs | 7 +- src/core/shared/mod.rs | 5 +- src/core/shared/utils.rs | 59 ++++++ src/designer/mod.rs | 16 +- 10 files changed, 351 insertions(+), 111 deletions(-) create mode 100644 SECURITY_TASKS.md diff --git a/SECURITY_TASKS.md b/SECURITY_TASKS.md new file mode 100644 index 000000000..8300b3fc4 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/src/auto_task/app_generator.rs b/src/auto_task/app_generator.rs index c53ee63a8..e967346b9 100644 --- a/src/auto_task/app_generator.rs +++ b/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> { 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> { 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], diff --git a/src/basic/keywords/app_server.rs b/src/basic/keywords/app_server.rs index c0eda5458..eb3ff1078 100644 --- a/src/basic/keywords/app_server.rs +++ b/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>) -> 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 { diff --git a/src/basic/keywords/data_operations.rs b/src/basic/keywords/data_operations.rs index e2db98aca..ddf169faf 100644 --- a/src/basic/keywords/data_operations.rs +++ b/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 = vec!["id".to_string()]; - let mut values: Vec = vec![format!("'{}'", sanitize_sql(&id_value))]; + let mut values: Vec = vec![format!("'{}'", sanitize_sql_value(&id_value))]; let mut update_sets: Vec = 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 { - 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() { - assert_eq!(sanitize_sql("hello"), "hello"); - assert_eq!(sanitize_sql("it's"), "it''s"); - assert_eq!(sanitize_sql("O'Brien"), "O''Brien"); + fn test_sanitize_sql_value() { + assert_eq!(sanitize_sql_value("hello"), "hello"); + assert_eq!(sanitize_sql_value("it's"), "it''s"); + assert_eq!(sanitize_sql_value("O'Brien"), "O''Brien"); } #[test] diff --git a/src/basic/keywords/db_api.rs b/src/basic/keywords/db_api.rs index 813dea76a..54d750dab 100644 --- a/src/basic/keywords/db_api.rs +++ b/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> { ) } -fn sanitize_identifier(name: &str) -> String { - name.chars() - .filter(|c| c.is_alphanumeric() || *c == '_') - .collect() -} - pub async fn list_records_handler( State(state): State>, headers: HeaderMap, diff --git a/src/basic/keywords/on_change.rs b/src/basic/keywords/on_change.rs index 8b2224b69..4a2a63e29 100644 --- a/src/basic/keywords/on_change.rs +++ b/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::() - .to_lowercase() -} + pub fn execute_on_change( conn: &mut diesel::PgConnection, diff --git a/src/basic/keywords/table_definition.rs b/src/basic/keywords/table_definition.rs index 445141ae5..642b0b8ec 100644 --- a/src/basic/keywords/table_definition.rs +++ b/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, diff --git a/src/core/shared/mod.rs b/src/core/shared/mod.rs index 4384bebf4..0244528e5 100644 --- a/src/core/shared/mod.rs +++ b/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, +}; diff --git a/src/core/shared/utils.rs b/src/core/shared/utils.rs index 3cd7152b6..9b6df254a 100644 --- a/src/core/shared/utils.rs +++ b/src/core/shared/utils.rs @@ -307,3 +307,62 @@ pub fn run_migrations(pool: &DbPool) -> Result<(), Box 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::() + .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('\'', "''") +} diff --git a/src/designer/mod.rs b/src/designer/mod.rs index 5033a8399..085c354e0 100644 --- a/src/designer/mod.rs +++ b/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", - } -}