fix(auth): wrong password returns Ok(false), not Err
All checks were successful
Lint and Build / format (pull_request) Successful in 37s
Lint and Build / clippy (pull_request) Successful in 1m40s
Lint and Build / build (pull_request) Successful in 3m43s
Lint and Build / test (pull_request) Successful in 3m21s

verify_password() was mapping every password-hash failure — including a
plain wrong password (password_hash::Error::Password) — to Err, which
propagated to the login handler's Err arm and returned 500
'authentication error' instead of 401 'invalid credentials'.

All three callers (login, change_password, recover_password) already
match on Ok(true)/Ok(false)/Err and expect Ok(false) for a mismatch;
the function just wasn't honoring that contract. Map
Error::Password -> Ok(false) and propagate only genuine failures as Err.

This was a latent bug: CI's auth tests never ran before because the
Mongo service container couldn't schedule (port collision, fixed in
#8). Now that CI runs them for real, login_with_wrong_password_is_rejected
exposed it. Behavior improves on all three call sites — a wrong recovery
phrase now also correctly 401s instead of 500ing.
This commit is contained in:
goose 2026-07-19 02:29:09 -03:00
parent eb2c2aa546
commit 706df11f15

View file

@ -1,6 +1,9 @@
use anyhow::Result; use anyhow::Result;
use pbkdf2::{ use pbkdf2::{
password_hash::{rand_core::OsRng, PasswordHash, PasswordHasher, PasswordVerifier, SaltString}, password_hash::{
rand_core::OsRng, Error as PasswordHashError, PasswordHash, PasswordHasher,
PasswordVerifier, SaltString,
},
Pbkdf2, Pbkdf2,
}; };
@ -15,14 +18,24 @@ impl PasswordService {
Ok(password_hash.to_string()) Ok(password_hash.to_string())
} }
/// Verify a password against a stored PHC string.
///
/// Returns `Ok(true)` on a match, `Ok(false)` on a mismatch (wrong
/// password — an expected outcome, not an error), and `Err(...)` only for
/// genuine failures (unparseable hash, malformed input, etc.). Returning
/// `Ok(false)` for a wrong password lets the login handler distinguish
/// "invalid credentials" (401) from a real backend fault (500).
pub fn verify_password(password: &str, hash: &str) -> Result<bool> { pub fn verify_password(password: &str, hash: &str) -> Result<bool> {
let parsed_hash = PasswordHash::new(hash) let parsed_hash = PasswordHash::new(hash)
.map_err(|e| anyhow::anyhow!("Failed to parse password hash: {}", e))?; .map_err(|e| anyhow::anyhow!("Failed to parse password hash: {}", e))?;
Pbkdf2 match Pbkdf2.verify_password(password.as_bytes(), &parsed_hash) {
.verify_password(password.as_bytes(), &parsed_hash) Ok(()) => Ok(true),
.map(|_| true) // `Error::Password` is the password-hash crate's signal for a
.map_err(|e| anyhow::anyhow!("Password verification failed: {}", e)) // mismatch — the one expected "failure" during normal auth.
Err(PasswordHashError::Password) => Ok(false),
Err(e) => Err(anyhow::anyhow!("Password verification failed: {}", e)),
}
} }
} }