Skip to main content

max / makenotwork

server: govern OAuth read routes; harden passkey + password-verify paths - SEC-S3: /oauth/userinfo (DB-amplifying) and the discovery endpoint were merged in after the governed sub-routers with no rate limit. Move both behind an API-read GovernorLayer so every public OAuth route is governed. - SEC-S2: webauthn-rs already rejects a non-zero sign-count regression (CredentialPossibleCompromise) before we persist, and update_credential is monotonic — but the rejection was swallowed into a generic error. Log it as a distinct passkey_counter_regression security event. - verify_password: an unparseable / non-Argon2 stored hash returned 500, which is an availability bug and a third-order account oracle. Treat it as a logged non-match (Ok(false)) instead. Ultra-fuzz Run #23, Security axis. (SEC-S1 failed-2FA-lockout was a false positive — verify_two_factor already feeds failures into increment_failed_login.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-18 21:44 UTC
Commit: 75b315bff03a4d1304d99e4c936834972c499ae4
Parent: 73fc8c6
3 files changed, +65 insertions, -18 deletions
M server/src/auth.rs +37 -13
@@ -426,19 +426,34 @@ pub fn hash_password(password: &str) -> Result<String, AppError> {
426 426 /// future algorithm migration — when one lands, add a dispatch table
427 427 /// instead of swapping the default instance under the verifier's feet.
428 428 pub fn verify_password(password: &str, hash: &str) -> Result<bool, AppError> {
429 - let parsed_hash = PasswordHash::new(hash)
430 - .map_err(|e| AppError::Internal(anyhow::anyhow!("parse password hash: {e}")))?;
431 -
432 - let algorithm = Algorithm::try_from(parsed_hash.algorithm)
433 - .map_err(|e| AppError::Internal(anyhow::anyhow!("unexpected password hash algorithm: {e}")))?;
434 - let version = parsed_hash
435 - .version
436 - .map(Version::try_from)
437 - .transpose()
438 - .map_err(|e| AppError::Internal(anyhow::anyhow!("unexpected password hash version: {e}")))?
439 - .unwrap_or(Version::V0x13);
440 - let params = Params::try_from(&parsed_hash)
441 - .map_err(|e| AppError::Internal(anyhow::anyhow!("parse password hash params: {e}")))?;
429 + // A stored hash that won't parse (corruption, or a non-Argon2 algorithm we
430 + // don't verify) is a server-side data problem, not a 500 for the user: treat
431 + // it as a non-match so login simply fails, and log it for ops. Returning an
432 + // Internal error here would also be a (third-order) account oracle — it
433 + // distinguishes "valid account, bad stored hash" from "valid account, wrong
434 + // password" by status code (SEC minor, Run #23).
435 + let reject = |what: &str, e: &dyn std::fmt::Display| {
436 + tracing::error!(event = "password_hash_unverifiable", reason = %what, error = %e,
437 + "stored password hash could not be parsed; treating as non-match");
438 + Ok(false)
439 + };
440 +
441 + let parsed_hash = match PasswordHash::new(hash) {
442 + Ok(h) => h,
443 + Err(e) => return reject("parse", &e),
444 + };
445 + let algorithm = match Algorithm::try_from(parsed_hash.algorithm) {
446 + Ok(a) => a,
447 + Err(e) => return reject("algorithm", &e),
448 + };
449 + let version = match parsed_hash.version.map(Version::try_from).transpose() {
450 + Ok(v) => v.unwrap_or(Version::V0x13),
451 + Err(e) => return reject("version", &e),
452 + };
453 + let params = match Params::try_from(&parsed_hash) {
454 + Ok(p) => p,
455 + Err(e) => return reject("params", &e),
456 + };
442 457
443 458 Ok(Argon2::new(algorithm, version, params)
444 459 .verify_password(password.as_bytes(), &parsed_hash)
@@ -649,6 +664,15 @@ mod tests {
649 664 }
650 665
651 666 #[test]
667 + fn verify_password_unparseable_hash_is_non_match_not_error() {
668 + // A corrupt / non-Argon2 stored hash must fail login cleanly (Ok(false)),
669 + // not 500 — avoids an availability bug and an account oracle (SEC, Run #23).
670 + for bad in ["", "not-a-phc-string", "$argon2id$garbage", "$2y$10$abcdefghijklmnopqrstuv"] {
671 + assert_eq!(verify_password("any", bad).unwrap(), false, "hash {bad:?} should be a non-match");
672 + }
673 + }
674 +
675 + #[test]
652 676 fn hash_password_different_each_time() {
653 677 let h1 = hash_password("same_password").unwrap();
654 678 let h2 = hash_password("same_password").unwrap();
M server/src/routes/auth.rs +17 -3
@@ -446,13 +446,27 @@ async fn passkey_auth_finish(
446 446 .context("deserialize passkey credential")?;
447 447 let discoverable_key = DiscoverableKey::from(&passkey);
448 448
449 - // Verify the authentication response
449 + // Verify the authentication response. webauthn-rs rejects a non-zero
450 + // signature-counter regression (CredentialPossibleCompromise) here, before
451 + // we ever persist — surface that as a distinct, auditable security event
452 + // rather than a generic verification failure (SEC-S2, Run #23).
450 453 let auth_result = state
451 454 .webauthn
452 455 .finish_discoverable_authentication(&auth, auth_state, &[discoverable_key])
453 - .map_err(|e| AppError::BadRequest(format!("Passkey verification failed: {}", e)))?;
456 + .map_err(|e| {
457 + if matches!(e, WebauthnError::CredentialPossibleCompromise) {
458 + tracing::warn!(
459 + user_id = %user_id,
460 + event = "passkey_counter_regression",
461 + "passkey sign-count regressed; authenticator may be cloned — authentication rejected"
462 + );
463 + }
464 + AppError::BadRequest(format!("Passkey verification failed: {}", e))
465 + })?;
454 466
455 - // Update the credential counter to prevent cloning attacks
467 + // Persist any counter/backup-state advance. update_credential only ever
468 + // raises the stored counter (never downgrades), and the regression case was
469 + // already rejected above — so this cannot record a cloned counter.
456 470 passkey.update_credential(&auth_result);
457 471 let updated_json = serde_json::to_value(&passkey)
458 472 .context("serialize passkey credential")?;
M server/src/routes/oauth.rs +11 -2
@@ -960,8 +960,17 @@ pub fn oauth_routes() -> CsrfRouter<AppState> {
960 960 config: token_rate_limit,
961 961 });
962 962
963 - authorize_routes
964 - .merge(token_routes)
963 + // userinfo is DB-amplifying (user + creator-tier lookup) and discovery is a
964 + // public read; govern both so every public OAuth route carries a rate limit
965 + // (SEC-S3, Run #23 — they were previously merged in ungoverned).
966 + let read_rate_limit =
967 + crate::helpers::rate_limiter_ms(constants::API_READ_RATE_LIMIT_MS, constants::API_READ_RATE_LIMIT_BURST);
968 + let read_routes = CsrfRouter::new()
965 969 .route_get("/oauth/userinfo", get(userinfo))
966 970 .route_get("/.well-known/oauth-authorization-server", get(discovery_metadata))
971 + .route_layer(GovernorLayer {
972 + config: read_rate_limit,
973 + });
974 +
975 + authorize_routes.merge(token_routes).merge(read_routes)
967 976 }