fix: address PR review issues in provisioning session

+7 -2

crates/relay/src/app.rs

··· 37 37 use crate::routes::resolve_handle::resolve_handle_handler; 38 38 use crate::well_known::WellKnownResolver; 39 39 40 - /// In-memory store for failed login attempts per identifier (for createSession rate limiting). 41 - /// Maps identifier (DID or handle) → timestamps of recent failures. 40 + /// In-memory store for failed login attempts per identifier, shared across all login endpoints. 41 + /// Maps identifier string → timestamps of recent failures. 42 42 /// `std::sync::Mutex` is used because the critical section never awaits. 43 + /// 44 + /// **Known limitation:** `createSession` keys by DID or handle; `POST /v1/accounts/sessions` 45 + /// keys by email. Both share this store, so an attacker gets `RATE_LIMIT_MAX_FAILURES` attempts 46 + /// per endpoint independently against the same account. Acceptable for v0.1; a future revision 47 + /// should normalise all identifiers to DID before keying. 43 48 pub type FailedLoginStore = Arc<Mutex<HashMap<String, VecDeque<Instant>>>>; 44 49 45 50 /// Wraps an `axum::http::HeaderMap` as an OTel text-map [`Extractor`] so that

+3 -1

crates/relay/src/db/accounts.rs

··· 82 82 .fetch_optional(db) 83 83 .await 84 84 .map_err(|e| { 85 - tracing::error!(error = %e, "DB error resolving email"); 85 + // Logging the email domain aids ops triage without exposing the full address in logs. 86 + let domain = email.split('@').nth(1).unwrap_or("<unknown>"); 87 + tracing::error!(error = %e, email_domain = %domain, "DB error resolving email"); 86 88 ApiError::new(ErrorCode::InternalError, "failed to resolve identifier") 87 89 })?; 88 90

+8 -6

crates/relay/src/routes/create_session.rs

··· 206 206 // Clear failure history only after the session is fully committed. 207 207 // Doing this earlier would reset the counter even if JWT issuance or the 208 208 // DB transaction subsequently fails. 209 - { 210 - let mut attempts = state.failed_login_attempts.lock().map_err(|_| { 211 - tracing::error!("failed_login_attempts mutex is poisoned"); 212 - ApiError::new(ErrorCode::InternalError, "internal error") 213 - })?; 214 - clear_failures(&mut attempts, &payload.identifier); 209 + // Mutex poison here must not override a committed session — log and continue. 210 + match state.failed_login_attempts.lock() { 211 + Ok(mut attempts) => clear_failures(&mut attempts, &payload.identifier), 212 + Err(_) => tracing::error!( 213 + identifier = %payload.identifier, 214 + phase = "clear_failures", 215 + "mutex poisoned; rate-limit counter not cleared after successful login" 216 + ), 215 217 } 216 218 217 219 // ATProto spec: "handle.invalid" is the sentinel for accounts without a resolvable handle.

+95 -60

crates/relay/src/routes/provisioning_session.rs

··· 51 51 // Check before any DB work to shed load on targeted accounts. 52 52 { 53 53 let mut attempts = state.failed_login_attempts.lock().map_err(|_| { 54 - tracing::error!("failed_login_attempts mutex is poisoned"); 54 + tracing::error!(phase = "rate_limit_check", "failed_login_attempts mutex is poisoned"); 55 55 ApiError::new(ErrorCode::InternalError, "internal error") 56 56 })?; 57 57 if is_rate_limited(&mut attempts, &payload.email) { ··· 63 63 } 64 64 65 65 // --- Resolve email and verify password --- 66 - // Both "account not found" and "wrong password" surface as the same error to prevent 67 - // user enumeration via distinguishable error messages. 66 + // "Account not found" and "wrong password" return the same error to prevent user 67 + // enumeration. Note: CorruptHash is intentionally distinct — it returns InternalError 68 + // because a corrupted hash indicates DB corruption, not an auth failure. 68 69 let account_opt = resolve_by_email(&state.db, &payload.email).await?; 69 70 70 71 let account = match account_opt { 71 72 Some(row) => { 72 73 let result = match row.password_hash.as_deref() { 73 - // Accounts without a password_hash cannot use provisioning session login. 74 + // argon2id never produces an empty string; this guards against rows 75 + // created by a migration that stored "" instead of NULL. 74 76 None | Some("") => VerifyResult::WrongPassword, 75 77 Some(h) => verify_password(h, &payload.password), 76 78 }; ··· 78 80 VerifyResult::Ok => {} 79 81 VerifyResult::WrongPassword => { 80 82 let mut attempts = state.failed_login_attempts.lock().map_err(|_| { 81 - tracing::error!("failed_login_attempts mutex is poisoned"); 83 + tracing::error!( 84 + phase = "record_failure_wrong_password", 85 + "failed_login_attempts mutex is poisoned" 86 + ); 82 87 ApiError::new(ErrorCode::InternalError, "internal error") 83 88 })?; 84 89 record_failure(&mut attempts, &payload.email); ··· 89 94 } 90 95 VerifyResult::CorruptHash => { 91 96 tracing::error!( 97 + email = %payload.email, 92 98 "stored password_hash is not a valid PHC string; possible DB corruption" 93 99 ); 94 100 return Err(ApiError::new(ErrorCode::InternalError, "internal error")); ··· 98 104 } 99 105 None => { 100 106 let mut attempts = state.failed_login_attempts.lock().map_err(|_| { 101 - tracing::error!("failed_login_attempts mutex is poisoned"); 107 + tracing::error!( 108 + phase = "record_failure_unknown_email", 109 + "failed_login_attempts mutex is poisoned" 110 + ); 102 111 ApiError::new(ErrorCode::InternalError, "internal error") 103 112 })?; 104 113 record_failure(&mut attempts, &payload.email); ··· 128 137 })?; 129 138 130 139 // Clear failure history only after the session is fully committed. 131 - { 132 - let mut attempts = state.failed_login_attempts.lock().map_err(|_| { 133 - tracing::error!("failed_login_attempts mutex is poisoned"); 134 - ApiError::new(ErrorCode::InternalError, "internal error") 135 - })?; 136 - clear_failures(&mut attempts, &payload.email); 140 + // Doing this earlier would reset the counter even if the DB insert subsequently fails. 141 + // Mutex poison here must not override a committed session — log and continue. 142 + match state.failed_login_attempts.lock() { 143 + Ok(mut attempts) => clear_failures(&mut attempts, &payload.email), 144 + Err(_) => tracing::error!( 145 + email = %payload.email, 146 + phase = "clear_failures", 147 + "mutex poisoned; rate-limit counter not cleared after successful login" 148 + ), 137 149 } 138 150 139 151 Ok(( ··· 149 161 150 162 #[cfg(test)] 151 163 mod tests { 152 - use argon2::{ 153 - password_hash::{rand_core::OsRng, SaltString}, 154 - Argon2, PasswordHasher, 155 - }; 156 164 use axum::{ 157 165 body::Body, 158 166 http::{Request, StatusCode}, ··· 161 169 162 170 use crate::app::{app, test_state}; 163 171 use crate::auth::rate_limit::RATE_LIMIT_MAX_FAILURES; 172 + use crate::routes::test_utils::{body_json, insert_account_with_password}; 164 173 165 174 // ── Helpers ─────────────────────────────────────────────────────────────── 166 175 ··· 175 184 .unwrap() 176 185 } 177 186 178 - async fn insert_account_with_password( 179 - db: &sqlx::SqlitePool, 180 - did: &str, 181 - handle: &str, 182 - email: &str, 183 - password: &str, 184 - ) { 185 - let salt = SaltString::generate(&mut OsRng); 186 - let hash = Argon2::default() 187 - .hash_password(password.as_bytes(), &salt) 188 - .unwrap() 189 - .to_string(); 190 - 191 - sqlx::query( 192 - "INSERT INTO accounts (did, email, password_hash, created_at, updated_at) \ 193 - VALUES (?, ?, ?, datetime('now'), datetime('now'))", 194 - ) 195 - .bind(did) 196 - .bind(email) 197 - .bind(&hash) 198 - .execute(db) 199 - .await 200 - .unwrap(); 201 - 202 - sqlx::query("INSERT INTO handles (handle, did, created_at) VALUES (?, ?, datetime('now'))") 203 - .bind(handle) 204 - .bind(did) 205 - .execute(db) 206 - .await 207 - .unwrap(); 208 - } 209 - 210 - async fn body_json(response: axum::response::Response) -> serde_json::Value { 211 - let bytes = axum::body::to_bytes(response.into_body(), usize::MAX) 212 - .await 213 - .unwrap(); 214 - serde_json::from_slice(&bytes).unwrap() 215 - } 216 - 217 187 // ── Happy path ──────────────────────────────────────────────────────────── 218 188 219 189 #[tokio::test] ··· 307 277 .await 308 278 .unwrap(); 309 279 310 - assert_eq!(did.as_deref(), Some("did:plc:authcheck"), 311 - "require_session query must find the issued token"); 280 + assert_eq!( 281 + did.as_deref(), 282 + Some("did:plc:authcheck"), 283 + "require_session query must find the issued token" 284 + ); 312 285 } 313 286 314 287 // ── Auth failures ───────────────────────────────────────────────────────── ··· 370 343 } 371 344 372 345 #[tokio::test] 346 + async fn account_with_empty_string_password_hash_returns_401() { 347 + // Guards the Some("") arm — a migration quirk storing "" instead of NULL 348 + // must be treated identically to NULL (password auth not allowed). 349 + let state = test_state().await; 350 + sqlx::query( 351 + "INSERT INTO accounts (did, email, password_hash, created_at, updated_at) \ 352 + VALUES ('did:plc:emptypass', 'emptypass@example.com', '', datetime('now'), datetime('now'))", 353 + ) 354 + .execute(&state.db) 355 + .await 356 + .unwrap(); 357 + 358 + let response = app(state) 359 + .oneshot(post_provisioning_session( 360 + "emptypass@example.com", 361 + "anypassword", 362 + )) 363 + .await 364 + .unwrap(); 365 + 366 + assert_eq!(response.status(), StatusCode::UNAUTHORIZED); 367 + } 368 + 369 + #[tokio::test] 370 + async fn corrupt_password_hash_returns_500_and_does_not_increment_rate_limit() { 371 + // CorruptHash returns InternalError (not AuthenticationRequired) because 372 + // it indicates DB corruption, not an auth failure. The rate-limit counter 373 + // must not be incremented — the user should not be locked out. 374 + let state = test_state().await; 375 + sqlx::query( 376 + "INSERT INTO accounts (did, email, password_hash, created_at, updated_at) \ 377 + VALUES ('did:plc:corrupt', 'corrupt@example.com', 'not-a-valid-phc-string', \ 378 + datetime('now'), datetime('now'))", 379 + ) 380 + .execute(&state.db) 381 + .await 382 + .unwrap(); 383 + 384 + let response = app(state.clone()) 385 + .oneshot(post_provisioning_session("corrupt@example.com", "anypassword")) 386 + .await 387 + .unwrap(); 388 + 389 + assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); 390 + let json = body_json(response).await; 391 + assert_eq!(json["error"]["code"], "INTERNAL_ERROR"); 392 + 393 + // Rate-limit counter must not have been incremented. 394 + let attempts = state.failed_login_attempts.lock().unwrap(); 395 + let entry = attempts.get("corrupt@example.com"); 396 + assert!( 397 + entry.map_or(true, |q| q.is_empty()), 398 + "CorruptHash must not increment the rate-limit counter" 399 + ); 400 + } 401 + 402 + #[tokio::test] 373 403 async fn deactivated_account_returns_401() { 374 404 let state = test_state().await; 375 405 insert_account_with_password( ··· 409 439 .await; 410 440 411 441 let wrong_pw = app(state.clone()) 412 - .oneshot(post_provisioning_session("enumtest@example.com", "wrongpassword")) 442 + .oneshot(post_provisioning_session( 443 + "enumtest@example.com", 444 + "wrongpassword", 445 + )) 413 446 .await 414 447 .unwrap(); 415 448 let unknown = app(state) ··· 439 472 for i in 0..RATE_LIMIT_MAX_FAILURES { 440 473 let response = app(state.clone()) 441 474 .oneshot(post_provisioning_session( 442 - "did:plc:ratelimited@example.com", 475 + "ratelimited@example.com", 443 476 "wrongpassword", 444 477 )) 445 478 .await ··· 453 486 454 487 let response = app(state) 455 488 .oneshot(post_provisioning_session( 456 - "did:plc:ratelimited@example.com", 489 + "ratelimited@example.com", 457 490 "wrongpassword", 458 491 )) 459 492 .await 460 493 .unwrap(); 461 494 assert_eq!(response.status(), StatusCode::TOO_MANY_REQUESTS); 495 + let json = body_json(response).await; 496 + assert_eq!(json["error"]["code"], "RATE_LIMITED"); 462 497 } 463 498 464 499 #[tokio::test]

+50

crates/relay/src/routes/test_utils.rs

··· 1 1 use std::sync::Arc; 2 2 3 + use argon2::{ 4 + password_hash::{rand_core::OsRng, SaltString}, 5 + Argon2, PasswordHasher, 6 + }; 7 + 3 8 use crate::app::{test_state, AppState}; 4 9 5 10 /// Minimal test state with admin_token set to `"test-admin-token"`. ··· 24 29 failed_login_attempts: base.failed_login_attempts, 25 30 } 26 31 } 32 + 33 + /// Insert a fully provisioned account row with an argon2id-hashed password and a handle. 34 + /// 35 + /// Used across route tests that exercise password authentication 36 + /// (`createSession`, `POST /v1/accounts/sessions`). Using real Argon2 with production 37 + /// parameters means parameter drift would surface as a test failure. 38 + pub async fn insert_account_with_password( 39 + db: &sqlx::SqlitePool, 40 + did: &str, 41 + handle: &str, 42 + email: &str, 43 + password: &str, 44 + ) { 45 + let salt = SaltString::generate(&mut OsRng); 46 + let hash = Argon2::default() 47 + .hash_password(password.as_bytes(), &salt) 48 + .unwrap() 49 + .to_string(); 50 + 51 + sqlx::query( 52 + "INSERT INTO accounts (did, email, password_hash, created_at, updated_at) \ 53 + VALUES (?, ?, ?, datetime('now'), datetime('now'))", 54 + ) 55 + .bind(did) 56 + .bind(email) 57 + .bind(&hash) 58 + .execute(db) 59 + .await 60 + .unwrap(); 61 + 62 + sqlx::query("INSERT INTO handles (handle, did, created_at) VALUES (?, ?, datetime('now'))") 63 + .bind(handle) 64 + .bind(did) 65 + .execute(db) 66 + .await 67 + .unwrap(); 68 + } 69 + 70 + /// Deserialise a response body as `serde_json::Value`, consuming the response. 71 + pub async fn body_json(response: axum::response::Response) -> serde_json::Value { 72 + let bytes = axum::body::to_bytes(response.into_body(), usize::MAX) 73 + .await 74 + .unwrap(); 75 + serde_json::from_slice(&bytes).unwrap() 76 + }

Configure Feed

Configure Feed