Implement critical security improvements for authentication
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 <noreply@anthropic.com>
This commit is contained in:
51
src/auth.rs
51
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<D: UserDao>(
|
||||
user: Json<CreateAccountRequest>,
|
||||
user_dao: web::Data<Mutex<D>>,
|
||||
) -> 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<D: UserDao>(
|
||||
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()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -50,7 +50,9 @@ impl FromStr for Claims {
|
||||
type Err = jsonwebtoken::errors::Error;
|
||||
|
||||
fn from_str(s: &str) -> Result<Self, Self::Err> {
|
||||
let token = *(s.split("Bearer ").collect::<Vec<_>>().last().unwrap_or(&""));
|
||||
let token = s.strip_prefix("Bearer ").ok_or_else(|| {
|
||||
jsonwebtoken::errors::Error::from(jsonwebtoken::errors::ErrorKind::InvalidToken)
|
||||
})?;
|
||||
|
||||
match decode::<Claims>(
|
||||
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() {
|
||||
|
||||
14
src/main.rs
14
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::<SqliteUserDao>)))
|
||||
.service(
|
||||
web::resource("/login")
|
||||
.wrap(Governor::new(&governor_conf))
|
||||
.route(web::post().to(login::<SqliteUserDao>)),
|
||||
)
|
||||
.service(
|
||||
web::resource("/photos")
|
||||
.route(web::get().to(files::list_photos::<SqliteTagDao, RealFileSystem>)),
|
||||
|
||||
Reference in New Issue
Block a user