From 2c52cffd65dc6b9902f94cc865f16a45cc2ef199 Mon Sep 17 00:00:00 2001 From: Cameron Date: Fri, 26 Dec 2025 23:53:54 -0500 Subject: [PATCH] Implement critical security improvements for authentication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit addresses several security vulnerabilities in the authentication and authorization system: 1. JWT Encoding Panic Fix (Critical) - Replace .unwrap() with proper error handling in JWT token generation - Prevents server crashes from encoding failures - Returns HTTP 500 with error logging instead of panicking 2. Rate Limiting for Login Endpoint (Critical) - Add actix-governor dependency (v0.5) - Configure rate limiter: 2 requests/sec with burst of 5 - Protects against brute-force authentication attacks 3. Strengthen Password Requirements - Minimum length increased from 6 to 12 characters - Require uppercase, lowercase, numeric, and special characters - Add comprehensive validation with clear error messages 4. Fix Token Parsing Vulnerability - Replace unsafe split().last().unwrap_or() pattern - Use strip_prefix() for proper Bearer token validation - Return InvalidToken error for malformed Authorization headers 5. Improve Authentication Logging - Sanitize error messages to avoid leaking user existence - Change from "User not found or incorrect password" to "Failed login attempt" All changes tested and verified with existing test suite (65/65 tests passing). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- Cargo.lock | 127 +++++++++++++++++++++++++++++++++++++++++++++++- Cargo.toml | 1 + src/auth.rs | 51 ++++++++++++++----- src/data/mod.rs | 8 +-- src/main.rs | 14 +++++- 5 files changed, 184 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c390518..44cb8ea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -82,6 +82,18 @@ dependencies = [ "v_htmlescape", ] +[[package]] +name = "actix-governor" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2e7b88f3804e01bd4191fdb08650430bbfcb43d3d9b2890064df3551ec7d25b" +dependencies = [ + "actix-http", + "actix-web", + "futures", + "governor", +] + [[package]] name = "actix-http" version = "3.11.1" @@ -874,6 +886,19 @@ dependencies = [ "syn", ] +[[package]] +name = "dashmap" +version = "5.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "978747c1d849a7d2ee5e8adc0159961c48fb7e5db2f06af6723b80123bb53856" +dependencies = [ + "cfg-if", + "hashbrown 0.14.5", + "lock_api", + "once_cell", + "parking_lot_core", +] + [[package]] name = "deranged" version = "0.5.3" @@ -1218,6 +1243,12 @@ version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f90f7dce0722e95104fcb095585910c0977252f286e354b5e3bd38902cd99988" +[[package]] +name = "futures-timer" +version = "3.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f288b0a4f20f9a56b5d1da57e2227c661b7b16168e2f72365f57b63326e29b24" + [[package]] name = "futures-util" version = "0.3.31" @@ -1277,6 +1308,26 @@ version = "0.31.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f" +[[package]] +name = "governor" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "68a7f542ee6b35af73b06abc0dad1c1bae89964e4e253bc4b587b91c9637867b" +dependencies = [ + "cfg-if", + "dashmap", + "futures", + "futures-timer", + "no-std-compat", + "nonzero_ext", + "parking_lot", + "portable-atomic", + "quanta", + "rand 0.8.5", + "smallvec", + "spinning_top", +] + [[package]] name = "h2" version = "0.3.27" @@ -1315,6 +1366,12 @@ dependencies = [ "tracing", ] +[[package]] +name = "hashbrown" +version = "0.14.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e5274423e17b7c9fc20b6e7e208532f9b19825d82dfd615708b70edd83df41f1" + [[package]] name = "hashbrown" version = "0.15.5" @@ -1610,6 +1667,7 @@ dependencies = [ "actix", "actix-cors", "actix-files", + "actix-governor", "actix-multipart", "actix-rt", "actix-web", @@ -1666,7 +1724,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f2481980430f9f78649238835720ddccc57e52df14ffce1c6f37391d61b563e9" dependencies = [ "equivalent", - "hashbrown", + "hashbrown 0.15.5", ] [[package]] @@ -2018,6 +2076,12 @@ version = "1.0.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "650eef8c711430f1a879fdd01d4745a7deea475becfb90269c06775983bbf086" +[[package]] +name = "no-std-compat" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b93853da6d84c2e3c7d730d6473e8817692dd89be387eb01b94d7f108ecb5b8c" + [[package]] name = "nom" version = "7.1.3" @@ -2028,6 +2092,12 @@ dependencies = [ "minimal-lexical", ] +[[package]] +name = "nonzero_ext" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "38bf9645c8b145698bb0b18a4637dcacbc421ea49bef2317e4fd8065a387cf21" + [[package]] name = "noop_proc_macro" version = "0.3.0" @@ -2448,6 +2518,21 @@ dependencies = [ "num-traits", ] +[[package]] +name = "quanta" +version = "0.12.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f3ab5a9d756f0d97bdc89019bd2e4ea098cf9cde50ee7564dde6b81ccc8f06c7" +dependencies = [ + "crossbeam-utils", + "libc", + "once_cell", + "raw-cpuid", + "wasi 0.11.1+wasi-snapshot-preview1", + "web-sys", + "winapi", +] + [[package]] name = "quick-error" version = "2.0.1" @@ -2578,6 +2663,15 @@ dependencies = [ "rgb", ] +[[package]] +name = "raw-cpuid" +version = "11.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "498cd0dc59d73224351ee52a95fee0f1a617a2eae0e7d9d720cc622c73a54186" +dependencies = [ + "bitflags", +] + [[package]] name = "rayon" version = "1.11.0" @@ -2919,6 +3013,15 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "spinning_top" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d96d2d1d716fb500937168cc09353ffdc7a012be8475ac7308e1bdf0e3923300" +dependencies = [ + "lock_api", +] + [[package]] name = "stable_deref_trait" version = "1.2.0" @@ -3546,6 +3649,22 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "winapi" +version = "0.3.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419" +dependencies = [ + "winapi-i686-pc-windows-gnu", + "winapi-x86_64-pc-windows-gnu", +] + +[[package]] +name = "winapi-i686-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" + [[package]] name = "winapi-util" version = "0.1.10" @@ -3555,6 +3674,12 @@ dependencies = [ "windows-sys 0.60.2", ] +[[package]] +name = "winapi-x86_64-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" + [[package]] name = "windows-core" version = "0.61.2" diff --git a/Cargo.toml b/Cargo.toml index 481c713..c754c6d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,6 +17,7 @@ tokio = { version = "1.42.0", features = ["default", "process", "sync"] } actix-files = "0.6" actix-cors = "0.7" actix-multipart = "0.7.2" +actix-governor = "0.5" futures = "0.3.5" jsonwebtoken = "9.3.0" serde = "1" diff --git a/src/auth.rs b/src/auth.rs index 9ee09bf..ffc58d8 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -13,22 +13,47 @@ use crate::{ database::UserDao, }; +/// Validate password meets security requirements +fn validate_password(password: &str) -> Result<(), String> { + if password.len() < 12 { + return Err("Password must be at least 12 characters".into()); + } + if !password.chars().any(|c| c.is_uppercase()) { + return Err("Password must contain at least one uppercase letter".into()); + } + if !password.chars().any(|c| c.is_lowercase()) { + return Err("Password must contain at least one lowercase letter".into()); + } + if !password.chars().any(|c| c.is_numeric()) { + return Err("Password must contain at least one number".into()); + } + if !password.chars().any(|c| !c.is_alphanumeric()) { + return Err("Password must contain at least one special character".into()); + } + Ok(()) +} + #[allow(dead_code)] async fn register( user: Json, user_dao: web::Data>, ) -> impl Responder { - if !user.username.is_empty() && user.password.len() > 5 && user.password == user.confirmation { + // Validate password strength + if let Err(msg) = validate_password(&user.password) { + return HttpResponse::BadRequest().body(msg); + } + + if !user.username.is_empty() && user.password == user.confirmation { let mut dao = user_dao.lock().expect("Unable to get UserDao"); if dao.user_exists(&user.username) { - HttpResponse::BadRequest() + HttpResponse::BadRequest().finish() } else if let Some(_user) = dao.create_user(&user.username, &user.password) { - HttpResponse::Ok() + HttpResponse::Ok().finish() } else { - HttpResponse::InternalServerError() + HttpResponse::InternalServerError().finish() } } else { - HttpResponse::BadRequest() + HttpResponse::BadRequest().finish() } } @@ -45,19 +70,21 @@ pub async fn login( sub: user.id.to_string(), exp: (Utc::now() + Duration::days(5)).timestamp(), }; - let token = encode( + let token = match encode( &Header::default(), &claims, &EncodingKey::from_secret(secret_key().as_bytes()), - ) - .unwrap(); + ) { + Ok(t) => t, + Err(e) => { + error!("Failed to encode JWT: {}", e); + return HttpResponse::InternalServerError().finish(); + } + }; HttpResponse::Ok().json(Token { token: &token }) } else { - error!( - "User not found during login or incorrect password: '{}'", - creds.username - ); + error!("Failed login attempt for user: '{}'", creds.username); HttpResponse::NotFound().finish() } } diff --git a/src/data/mod.rs b/src/data/mod.rs index cf279b7..010e7f5 100644 --- a/src/data/mod.rs +++ b/src/data/mod.rs @@ -50,7 +50,9 @@ impl FromStr for Claims { type Err = jsonwebtoken::errors::Error; fn from_str(s: &str) -> Result { - let token = *(s.split("Bearer ").collect::>().last().unwrap_or(&"")); + let token = s.strip_prefix("Bearer ").ok_or_else(|| { + jsonwebtoken::errors::Error::from(jsonwebtoken::errors::ErrorKind::InvalidToken) + })?; match decode::( token, @@ -341,7 +343,7 @@ mod tests { }; let c = Claims::from_str( - "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiI5IiwiZXhwIjoxNjEzNjE2NDc5MH0.9wwK4l8vhvq55YoueEljMbN_5uVTaAsGLLRPr0AuymE") + "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiI5IiwiZXhwIjoxNjEzNjE2NDc5MH0.9wwK4l8vhvq55YoueEljMbN_5uVTaAsGLLRPr0AuymE") .unwrap(); assert_eq!(claims.sub, c.sub); @@ -351,7 +353,7 @@ mod tests { #[test] fn test_expired_token() { let err = Claims::from_str( - "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiI5IiwiZXhwIjoxNn0.eZnfaNfiD54VMbphIqeBICeG9SzAtwNXntLwtTBihjY", + "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiI5IiwiZXhwIjoxNn0.eZnfaNfiD54VMbphIqeBICeG9SzAtwNXntLwtTBihjY", ); match err.unwrap_err().into_kind() { diff --git a/src/main.rs b/src/main.rs index ddb24b3..dcd6a72 100644 --- a/src/main.rs +++ b/src/main.rs @@ -21,6 +21,7 @@ use walkdir::{DirEntry, WalkDir}; use actix_cors::Cors; use actix_files::NamedFile; +use actix_governor::{Governor, GovernorConfigBuilder}; use actix_multipart as mp; use actix_web::{ App, HttpRequest, HttpResponse, HttpServer, Responder, delete, get, middleware, post, put, @@ -760,10 +761,21 @@ fn main() -> std::io::Result<()> { .supports_credentials() .max_age(3600); + // Configure rate limiting for login endpoint (2 requests/sec, burst of 5) + let governor_conf = GovernorConfigBuilder::default() + .per_second(2) + .burst_size(5) + .finish() + .unwrap(); + App::new() .wrap(middleware::Logger::default()) .wrap(cors) - .service(web::resource("/login").route(web::post().to(login::))) + .service( + web::resource("/login") + .wrap(Governor::new(&governor_conf)) + .route(web::post().to(login::)), + ) .service( web::resource("/photos") .route(web::get().to(files::list_photos::)),