refactor: apply clippy let-chains and harden password validation
- Auto-fix 29 clippy warnings by converting nested if-lets to let-chains - Harden password strength validator to reject restricted substrings - Simplify rate_limiter state checks using is_none_or - Improves idiomatic Rust style and overall security posture
This commit is contained in:
1 parent
f2f615b456
commit
b25a015898
17 files changed
+81
-109
No files matched your search
Generated
+1
-1
@@ -1239,7 +1239,7 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "nx9-auth"
|
||||
version = "0.3.0"
|
||||
version = "0.4.0"
|
||||
dependencies = [
|
||||
"anyhow",
|
||||
"argon2",
|
||||
|
||||
+6
-9
@@ -68,11 +68,10 @@ pub async fn login(
|
||||
}
|
||||
|
||||
// Rate limit check (per IP)
|
||||
if let Some(ip_str) = &ctx.ip_address {
|
||||
if let Ok(ip_addr) = ip_str.parse::<std::net::IpAddr>() {
|
||||
if let Some(ip_str) = &ctx.ip_address
|
||||
&& let Ok(ip_addr) = ip_str.parse::<std::net::IpAddr>() {
|
||||
state.rate_limiter.check(ip_addr)?;
|
||||
}
|
||||
}
|
||||
|
||||
// Look up user — always run comparable work on failure paths (timing).
|
||||
let user_opt = state
|
||||
@@ -103,11 +102,10 @@ pub async fn login(
|
||||
|
||||
if !is_authed {
|
||||
record_login_failure(&state, body.username.trim(), ip, ctx.user_agent.as_deref()).await;
|
||||
if let Some(ip_str) = &ctx.ip_address {
|
||||
if let Ok(ip_addr) = ip_str.parse::<std::net::IpAddr>() {
|
||||
if let Some(ip_str) = &ctx.ip_address
|
||||
&& let Ok(ip_addr) = ip_str.parse::<std::net::IpAddr>() {
|
||||
state.rate_limiter.record_failure(ip_addr);
|
||||
}
|
||||
}
|
||||
// Non-enumerating error for both unknown user and bad password.
|
||||
return Err(AppError::InvalidCredentials);
|
||||
}
|
||||
@@ -118,11 +116,10 @@ pub async fn login(
|
||||
};
|
||||
|
||||
// Clear rate limit on success
|
||||
if let Some(ip_str) = &ctx.ip_address {
|
||||
if let Ok(ip_addr) = ip_str.parse::<std::net::IpAddr>() {
|
||||
if let Some(ip_str) = &ctx.ip_address
|
||||
&& let Ok(ip_addr) = ip_str.parse::<std::net::IpAddr>() {
|
||||
state.rate_limiter.record_success(ip_addr);
|
||||
}
|
||||
}
|
||||
|
||||
// Session fixation mitigation: revoke prior sessions + refresh tokens.
|
||||
let _ = state
|
||||
|
||||
+2
-3
@@ -103,13 +103,12 @@ pub async fn terminate_session(
|
||||
Path(id): Path<String>,
|
||||
) -> Result<Json<Value>> {
|
||||
// If the user is trying to terminate the current session, disallow it
|
||||
if let Some(current_id) = auth.session_id.as_deref() {
|
||||
if id == current_id {
|
||||
if let Some(current_id) = auth.session_id.as_deref()
|
||||
&& id == current_id {
|
||||
return Err(AppError::InvalidInput(
|
||||
"Cannot terminate current session".into(),
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
// Admins can terminate any session, users can only terminate their own
|
||||
let is_admin = state
|
||||
|
||||
+4
-6
@@ -53,11 +53,10 @@ pub async fn create_tenant(
|
||||
) -> Result<Json<Value>> {
|
||||
require(&state.provider, &auth.user.id, "roles:manage").await?;
|
||||
|
||||
if let Some(ref s) = body.slug {
|
||||
if !s.trim().is_empty() {
|
||||
if let Some(ref s) = body.slug
|
||||
&& !s.trim().is_empty() {
|
||||
crate::identity::slug::validate_slug(s)?;
|
||||
}
|
||||
}
|
||||
|
||||
let id = uuid::Uuid::new_v4().to_string();
|
||||
let tenant = state
|
||||
@@ -124,11 +123,10 @@ pub async fn update_tenant(
|
||||
) -> Result<Json<Value>> {
|
||||
require(&state.provider, &auth.user.id, "roles:manage").await?;
|
||||
|
||||
if let Some(ref s) = body.slug {
|
||||
if !s.trim().is_empty() {
|
||||
if let Some(ref s) = body.slug
|
||||
&& !s.trim().is_empty() {
|
||||
crate::identity::slug::validate_slug(s)?;
|
||||
}
|
||||
}
|
||||
|
||||
state
|
||||
.provider
|
||||
|
||||
+2
-3
@@ -26,8 +26,8 @@ pub fn ui_dist_dir() -> PathBuf {
|
||||
return c.clone();
|
||||
}
|
||||
}
|
||||
if let Ok(exe) = std::env::current_exe() {
|
||||
if let Some(dir) = exe.parent() {
|
||||
if let Ok(exe) = std::env::current_exe()
|
||||
&& let Some(dir) = exe.parent() {
|
||||
for rel in ["ui/dist", "../ui/dist", "../../ui/dist"] {
|
||||
let candidate = dir.join(rel);
|
||||
if candidate.exists() {
|
||||
@@ -35,7 +35,6 @@ pub fn ui_dist_dir() -> PathBuf {
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("ui/dist")
|
||||
}
|
||||
|
||||
|
||||
+8
-12
@@ -566,11 +566,10 @@ async fn cmd_init(
|
||||
let sqlite_path = config.database.sqlite_path();
|
||||
let db_path = std::path::Path::new(&sqlite_path);
|
||||
println!("Creating database directory...");
|
||||
if let Some(parent) = db_path.parent() {
|
||||
if !parent.as_os_str().is_empty() {
|
||||
if let Some(parent) = db_path.parent()
|
||||
&& !parent.as_os_str().is_empty() {
|
||||
std::fs::create_dir_all(parent)?;
|
||||
}
|
||||
}
|
||||
|
||||
// Create state directory
|
||||
if let Ok(home) = std::env::var("HOME") {
|
||||
@@ -664,11 +663,10 @@ async fn run_init_validation(config: &Config, admin_skipped: bool) -> anyhow::Re
|
||||
let sqlite_path = config.database.sqlite_path();
|
||||
let db_path = std::path::Path::new(&sqlite_path);
|
||||
let mut dirs_ok = true;
|
||||
if let Some(parent) = db_path.parent() {
|
||||
if !parent.as_os_str().is_empty() && std::fs::create_dir_all(parent).is_err() {
|
||||
if let Some(parent) = db_path.parent()
|
||||
&& !parent.as_os_str().is_empty() && std::fs::create_dir_all(parent).is_err() {
|
||||
dirs_ok = false;
|
||||
}
|
||||
}
|
||||
if let Ok(home) = std::env::var("HOME") {
|
||||
let state_dir = std::path::Path::new(&home).join(".local/state/nx9-auth");
|
||||
if std::fs::create_dir_all(&state_dir).is_err() {
|
||||
@@ -892,11 +890,10 @@ async fn cmd_backup(config: &Config, path: &std::path::Path) -> anyhow::Result<(
|
||||
);
|
||||
}
|
||||
|
||||
if let Some(parent) = path.parent() {
|
||||
if !parent.as_os_str().is_empty() {
|
||||
if let Some(parent) = path.parent()
|
||||
&& !parent.as_os_str().is_empty() {
|
||||
std::fs::create_dir_all(parent)?;
|
||||
}
|
||||
}
|
||||
|
||||
if path.exists() {
|
||||
std::fs::remove_file(path)?;
|
||||
@@ -958,11 +955,10 @@ async fn cmd_restore(config: &Config, path: &std::path::Path) -> anyhow::Result<
|
||||
crate::config::DatabaseBackend::Sqlite => {
|
||||
let sqlite_path = config.database.sqlite_path();
|
||||
let target_path = std::path::Path::new(&sqlite_path);
|
||||
if let Some(parent) = target_path.parent() {
|
||||
if !parent.as_os_str().is_empty() {
|
||||
if let Some(parent) = target_path.parent()
|
||||
&& !parent.as_os_str().is_empty() {
|
||||
std::fs::create_dir_all(parent)?;
|
||||
}
|
||||
}
|
||||
std::fs::copy(path, target_path).with_context(|| {
|
||||
format!("failed to restore backup to {}", target_path.display())
|
||||
})?;
|
||||
|
||||
+6
-9
@@ -293,14 +293,13 @@ impl Default for ShutdownConfig {
|
||||
// ── Helpers ──────────────────────────────────────────────────────────────────
|
||||
|
||||
fn resolve_home_path(path: &str) -> String {
|
||||
if let Some(stripped) = path.strip_prefix("~/") {
|
||||
if let Ok(home) = std::env::var("HOME") {
|
||||
if let Some(stripped) = path.strip_prefix("~/")
|
||||
&& let Ok(home) = std::env::var("HOME") {
|
||||
return Path::new(&home)
|
||||
.join(stripped)
|
||||
.to_string_lossy()
|
||||
.into_owned();
|
||||
}
|
||||
}
|
||||
path.to_string()
|
||||
}
|
||||
|
||||
@@ -312,12 +311,11 @@ impl Config {
|
||||
if let Some(ref mut path) = self.database.path {
|
||||
*path = resolve_home_path(path);
|
||||
}
|
||||
if let Some(ref mut url) = self.database.url {
|
||||
if let Some(stripped) = url.strip_prefix("sqlite://") {
|
||||
if let Some(ref mut url) = self.database.url
|
||||
&& let Some(stripped) = url.strip_prefix("sqlite://") {
|
||||
let clean = resolve_home_path(stripped);
|
||||
*url = format!("sqlite://{clean}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Load and parse config from a TOML file.
|
||||
@@ -372,11 +370,10 @@ impl Config {
|
||||
|
||||
/// Default user configuration path (~/.config/nx9-auth/config.toml)
|
||||
pub fn default_user_config_path() -> Option<PathBuf> {
|
||||
if let Ok(xdg) = std::env::var("XDG_CONFIG_HOME") {
|
||||
if !xdg.is_empty() {
|
||||
if let Ok(xdg) = std::env::var("XDG_CONFIG_HOME")
|
||||
&& !xdg.is_empty() {
|
||||
return Some(PathBuf::from(xdg).join("nx9-auth/config.toml"));
|
||||
}
|
||||
}
|
||||
if let Ok(home) = std::env::var("HOME") {
|
||||
return Some(PathBuf::from(home).join(".config/nx9-auth/config.toml"));
|
||||
}
|
||||
|
||||
+4
-6
@@ -57,13 +57,12 @@ pub async fn init_provider(
|
||||
#[cfg(feature = "sqlite")]
|
||||
DatabaseBackend::Sqlite => {
|
||||
let path = config.database.sqlite_path();
|
||||
if let Some(parent) = std::path::Path::new(&path).parent() {
|
||||
if !parent.as_os_str().is_empty() {
|
||||
if let Some(parent) = std::path::Path::new(&path).parent()
|
||||
&& !parent.as_os_str().is_empty() {
|
||||
std::fs::create_dir_all(parent).with_context(|| {
|
||||
format!("failed to create database directory: {}", parent.display())
|
||||
})?;
|
||||
}
|
||||
}
|
||||
|
||||
let max_conn = config.database.max_connections.unwrap_or(16);
|
||||
let min_conn = config.database.min_connections.unwrap_or(1);
|
||||
@@ -177,13 +176,12 @@ pub async fn init_provider(
|
||||
/// Helper function to create an SQLite pool for legacy CLI commands or tests.
|
||||
#[cfg(feature = "sqlite")]
|
||||
pub async fn create_pool(path: &str) -> Result<SqlitePool> {
|
||||
if let Some(parent) = std::path::Path::new(path).parent() {
|
||||
if !parent.as_os_str().is_empty() {
|
||||
if let Some(parent) = std::path::Path::new(path).parent()
|
||||
&& !parent.as_os_str().is_empty() {
|
||||
std::fs::create_dir_all(parent).with_context(|| {
|
||||
format!("failed to create database directory: {}", parent.display())
|
||||
})?;
|
||||
}
|
||||
}
|
||||
let url = if path.starts_with("sqlite://") {
|
||||
path.to_string()
|
||||
} else {
|
||||
|
||||
@@ -221,8 +221,8 @@ pub async fn update(
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(new_enabled) = enabled {
|
||||
if new_enabled != existing.enabled {
|
||||
if let Some(new_enabled) = enabled
|
||||
&& new_enabled != existing.enabled {
|
||||
let action = if new_enabled {
|
||||
"application.member_enabled"
|
||||
} else {
|
||||
@@ -255,7 +255,6 @@ pub async fn update(
|
||||
.await
|
||||
.map_err(AppError::Database)?;
|
||||
}
|
||||
}
|
||||
|
||||
provider
|
||||
.application_members()
|
||||
|
||||
@@ -324,11 +324,9 @@ pub async fn update(
|
||||
.find_by_slug(slug)
|
||||
.await
|
||||
.map_err(AppError::Database)?
|
||||
{
|
||||
if other.entity_id != id || other.entity_type != "application" {
|
||||
&& (other.entity_id != id || other.entity_type != "application") {
|
||||
return Err(AppError::Conflict(format!("slug '{slug}' already exists")));
|
||||
}
|
||||
}
|
||||
|
||||
let redirect_json = redirect_uris.map(|v| serde_json::to_string(&v).unwrap_or_default());
|
||||
let scopes_json = scopes.map(|v| serde_json::to_string(&v).unwrap_or_default());
|
||||
|
||||
@@ -191,11 +191,9 @@ pub async fn update_role(
|
||||
.find_by_name(name)
|
||||
.await
|
||||
.map_err(AppError::Database)?
|
||||
{
|
||||
if other.id != id {
|
||||
&& other.id != id {
|
||||
return Err(AppError::Conflict(format!("role '{name}' already exists")));
|
||||
}
|
||||
}
|
||||
|
||||
provider
|
||||
.roles()
|
||||
|
||||
@@ -72,9 +72,9 @@ where
|
||||
// 2. Try Authorization: Bearer — PAT first, then session token.
|
||||
// Session tokens are returned from /auth/login for SPA clients that
|
||||
// cannot rely solely on the HttpOnly cookie.
|
||||
if let Some(auth_header) = parts.headers.get(axum::http::header::AUTHORIZATION) {
|
||||
if let Ok(value) = auth_header.to_str() {
|
||||
if let Some(raw) = value.strip_prefix("Bearer ") {
|
||||
if let Some(auth_header) = parts.headers.get(axum::http::header::AUTHORIZATION)
|
||||
&& let Ok(value) = auth_header.to_str()
|
||||
&& let Some(raw) = value.strip_prefix("Bearer ") {
|
||||
let raw = raw.trim();
|
||||
|
||||
// 2a. Personal access token
|
||||
@@ -125,8 +125,6 @@ where
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Err(AppError::Unauthorized)
|
||||
}
|
||||
|
||||
@@ -162,13 +162,12 @@ impl Lifecycle for Application {
|
||||
self.pool_handle = Some(pool_handle);
|
||||
}
|
||||
|
||||
if self.router.is_none() {
|
||||
if let Some(provider) = &self.provider {
|
||||
if self.router.is_none()
|
||||
&& let Some(provider) = &self.provider {
|
||||
let app_state = crate::state::AppState::new(provider.clone(), config);
|
||||
let router = crate::api::router::build(app_state);
|
||||
self.router = Some(router);
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
+27
-25
@@ -17,10 +17,10 @@ pub fn hash_password(password: &str, cfg: &SecurityConfig) -> Result<String, App
|
||||
cfg.argon2_parallelism,
|
||||
None,
|
||||
)
|
||||
.map_err(|e| {
|
||||
tracing::error!(error = %e, "invalid argon2 params");
|
||||
AppError::Internal
|
||||
})?,
|
||||
.map_err(|e| {
|
||||
tracing::error!(error = %e, "invalid argon2 params");
|
||||
AppError::Internal
|
||||
})?,
|
||||
);
|
||||
|
||||
let salt = SaltString::generate(&mut OsRng);
|
||||
@@ -55,7 +55,10 @@ pub fn verify_password(password: &str, hash: &str) -> Result<bool, AppError> {
|
||||
pub fn verify_dummy(cfg: &SecurityConfig) -> Result<(), AppError> {
|
||||
// We hash a constant string to ensure the time taken is consistent
|
||||
// and aligns with the cost parameters defined in the config.
|
||||
let _ = hash_password("dummy_password_for_timing_attacks_constant_time_alignment", cfg)?;
|
||||
let _ = hash_password(
|
||||
"dummy_password_for_timing_attacks_constant_time_alignment",
|
||||
cfg,
|
||||
)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -70,32 +73,31 @@ pub fn validate_password_strength(password: &str, is_admin: bool) -> Result<(),
|
||||
|
||||
let normalized = password.to_lowercase();
|
||||
|
||||
// Use a HashSet for O(1) exact matching against compromised/weak passwords
|
||||
let weak_passwords: HashSet<&str> = [
|
||||
"password",
|
||||
"admin123",
|
||||
"qwerty",
|
||||
"12345678",
|
||||
"123456789",
|
||||
"administrator",
|
||||
"nx9-auth",
|
||||
"nx9auth",
|
||||
"password123",
|
||||
"admin",
|
||||
"letmein",
|
||||
"welcome",
|
||||
]
|
||||
.iter()
|
||||
.copied()
|
||||
.collect();
|
||||
// 1. Exact matches for highly common passwords
|
||||
let exact_weak: HashSet<&str> = [
|
||||
"password", "admin123", "qwerty", "12345678", "123456789",
|
||||
"administrator", "nx9-auth", "nx9auth", "password123", "admin",
|
||||
"letmein", "welcome", "password12345"
|
||||
].into_iter().collect();
|
||||
|
||||
if weak_passwords.contains(normalized.as_str()) {
|
||||
if exact_weak.contains(normalized.as_str()) {
|
||||
return Err(AppError::InvalidInput(
|
||||
"password is too common or weak".to_string(),
|
||||
));
|
||||
}
|
||||
|
||||
// Additional check: reject if it's a simple sequence or pattern
|
||||
// 2. Substring matches for highly restricted roots
|
||||
// We ban these substrings because "password12345" or "qwerty2024" are trivially guessable.
|
||||
let banned_substrings = ["password", "qwerty", "123456"];
|
||||
for banned in &banned_substrings {
|
||||
if normalized.contains(banned) {
|
||||
return Err(AppError::InvalidInput(
|
||||
"password contains a restricted sequence".to_string(),
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
// 3. Reject single repeated characters
|
||||
if password.chars().all(|c| c == password.chars().next().unwrap()) {
|
||||
return Err(AppError::InvalidInput(
|
||||
"password cannot be a single repeated character".to_string(),
|
||||
|
||||
@@ -31,7 +31,7 @@ impl IpState {
|
||||
|
||||
/// Returns true if this state is no longer active and can be removed.
|
||||
fn is_stale(&self, now: Instant) -> bool {
|
||||
self.window.is_empty() && self.locked_until.map_or(true, |until| now >= until)
|
||||
self.window.is_empty() && self.locked_until.is_none_or(|until| now >= until)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -71,13 +71,11 @@ impl RateLimiter {
|
||||
|
||||
/// Check if the given IP is currently allowed to attempt a login.
|
||||
pub fn check(&self, ip: IpAddr) -> Result<(), AppError> {
|
||||
if let Some(s) = self.state.get(&ip) {
|
||||
if let Some(until) = s.locked_until {
|
||||
if Instant::now() < until {
|
||||
if let Some(s) = self.state.get(&ip)
|
||||
&& let Some(until) = s.locked_until
|
||||
&& Instant::now() < until {
|
||||
return Err(AppError::RateLimited);
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -87,11 +85,10 @@ impl RateLimiter {
|
||||
let now = Instant::now();
|
||||
|
||||
// Clear the lockout if it has expired
|
||||
if let Some(until) = s.locked_until {
|
||||
if now >= until {
|
||||
if let Some(until) = s.locked_until
|
||||
&& now >= until {
|
||||
s.locked_until = None;
|
||||
}
|
||||
}
|
||||
|
||||
// Prune old failures outside the window
|
||||
let cutoff = now - self.window;
|
||||
@@ -142,4 +139,4 @@ impl Default for RateLimiter {
|
||||
max_failures: 5,
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -77,8 +77,8 @@ pub async fn validate_session(
|
||||
let now = chrono::Utc::now();
|
||||
|
||||
// Check absolute expiry
|
||||
if let Ok(expires) = chrono::DateTime::parse_from_rfc3339(&session.expires_at) {
|
||||
if now > expires {
|
||||
if let Ok(expires) = chrono::DateTime::parse_from_rfc3339(&session.expires_at)
|
||||
&& now > expires {
|
||||
provider
|
||||
.sessions()
|
||||
.revoke(&session.id)
|
||||
@@ -86,7 +86,6 @@ pub async fn validate_session(
|
||||
.map_err(AppError::Database)?;
|
||||
return Ok(None);
|
||||
}
|
||||
}
|
||||
|
||||
// Check idle timeout
|
||||
if let Ok(last_seen) = chrono::DateTime::parse_from_rfc3339(&session.last_seen_at) {
|
||||
|
||||
@@ -130,13 +130,11 @@ pub async fn validate_token(
|
||||
};
|
||||
|
||||
// Check expiry if set
|
||||
if let Some(ref exp) = token.expires_at {
|
||||
if let Ok(expires) = chrono::DateTime::parse_from_rfc3339(exp) {
|
||||
if chrono::Utc::now() > expires {
|
||||
if let Some(ref exp) = token.expires_at
|
||||
&& let Ok(expires) = chrono::DateTime::parse_from_rfc3339(exp)
|
||||
&& chrono::Utc::now() > expires {
|
||||
return Ok(None);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Touch last_used_at (fire-and-forget)
|
||||
let _ = provider.tokens().update_last_used(&token.id).await;
|
||||
|
||||
Reference in new issue
Block a user