From d275443eb05d6ac6322944074a21038f98e7c6f8 Mon Sep 17 00:00:00 2001 From: Maple Xu Date: Sat, 4 Apr 2026 10:23:18 -0400 Subject: [PATCH 1/5] Add per-user request locks to prevent race conditions Use DashMap>> to serialize all mutating requests per user. Covers create_user, update_user, add_stored_password, change_stored_password, and delete_user. Read-only handlers are unaffected. Closes #6. Co-Authored-By: Claude Opus 4.6 (1M context) --- passwords/api/Cargo.lock | 1 + passwords/api/Cargo.toml | 1 + passwords/api/src/lib.rs | 61 +++++++++++++--- passwords/api/tests/concurrency_tests.rs | 88 ++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 11 deletions(-) create mode 100644 passwords/api/tests/concurrency_tests.rs diff --git a/passwords/api/Cargo.lock b/passwords/api/Cargo.lock index 2b0d004..8a21c72 100644 --- a/passwords/api/Cargo.lock +++ b/passwords/api/Cargo.lock @@ -1305,6 +1305,7 @@ version = "0.1.0" dependencies = [ "anyhow", "axum", + "dashmap", "data-encoding", "dotenv", "http", diff --git a/passwords/api/Cargo.toml b/passwords/api/Cargo.toml index cc7f7bb..79e18a4 100644 --- a/passwords/api/Cargo.toml +++ b/passwords/api/Cargo.toml @@ -26,3 +26,4 @@ serde = "1.0" serde_json = "1.0" http = "1.0" tower_governor = "0.8.0" +dashmap = "6" diff --git a/passwords/api/src/lib.rs b/passwords/api/src/lib.rs index b31de07..0f1d3a4 100644 --- a/passwords/api/src/lib.rs +++ b/passwords/api/src/lib.rs @@ -3,21 +3,33 @@ pub mod encrypt; pub mod env; use axum::{ - extract::{rejection::PathRejection, FromRequestParts, Path}, + extract::{rejection::PathRejection, Extension, FromRequestParts, Path}, http::{header::HeaderName, request::Parts, HeaderValue, Method, StatusCode}, response::IntoResponse, routing::{get, post}, Json, Router, }; +use dashmap::DashMap; use db::DbError; -use encrypt::{generate_password, Credentials, CryptoError}; +use encrypt::{generate_password, user2oid, Credentials, CryptoError}; use env::EnvVars; +use mongodb::bson::oid::ObjectId; use serde::Deserialize; +use std::sync::Arc; use tower_governor::governor::GovernorConfigBuilder; use tower_governor::GovernorLayer; use tower_http::cors::CorsLayer; use tower_http::trace::TraceLayer; +// --------------------------------------------------------------------------- +// Per-user lock map for serializing mutating requests +// --------------------------------------------------------------------------- + +/// A shared map from user ObjectId to a per-user async mutex. +/// All mutating (write) handlers acquire the lock for the target user before +/// proceeding, preventing race conditions on concurrent writes. +pub type UserLocks = Arc>>>; + const MAX_KEY_LENGTH: usize = 128; // --------------------------------------------------------------------------- @@ -167,8 +179,14 @@ async fn generate() -> Result, Error> { Ok(Json(pw)) } -#[tracing::instrument(skip(creds))] -async fn create_user(creds: Credentials) -> Result { +#[tracing::instrument(skip(creds, locks))] +async fn create_user( + Extension(locks): Extension, + creds: Credentials, +) -> Result { + let oid = user2oid(&creds.username); + let mutex = locks.entry(oid).or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))).clone(); + let _guard = mutex.lock().await; db::add_user(creds).await?; tracing::info!("ok"); Ok(StatusCode::OK) @@ -181,11 +199,15 @@ async fn verify_user(creds: Credentials) -> Result { Ok(StatusCode::OK) } -#[tracing::instrument(skip(creds, payload))] +#[tracing::instrument(skip(creds, locks, payload))] async fn update_user( + Extension(locks): Extension, creds: Credentials, Json(payload): Json, ) -> Result { + let oid = user2oid(&creds.username); + let mutex = locks.entry(oid).or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))).clone(); + let _guard = mutex.lock().await; db::change_master_password(creds, payload.new_password, payload.passwords).await?; tracing::info!("ok"); Ok(StatusCode::OK) @@ -215,23 +237,31 @@ async fn get_stored_passwords(creds: Credentials) -> Result>, E Ok(Json(pws)) } -#[tracing::instrument(skip(creds, payload))] +#[tracing::instrument(skip(creds, locks, payload))] async fn add_stored_password( + Extension(locks): Extension, creds: Credentials, ValidatedKey(key): ValidatedKey, Json(payload): Json, ) -> Result { + let oid = user2oid(&creds.username); + let mutex = locks.entry(oid).or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))).clone(); + let _guard = mutex.lock().await; db::add_stored_password(creds, key, payload.encrypted_password).await?; tracing::info!("ok"); Ok(StatusCode::OK) } -#[tracing::instrument(skip(creds, payload))] +#[tracing::instrument(skip(creds, locks, payload))] async fn change_stored_password( + Extension(locks): Extension, creds: Credentials, ValidatedKey(key): ValidatedKey, Json(payload): Json, ) -> Result { + let oid = user2oid(&creds.username); + let mutex = locks.entry(oid).or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))).clone(); + let _guard = mutex.lock().await; db::change_stored_password(creds, key, payload.encrypted_password).await?; tracing::info!("ok"); Ok(StatusCode::OK) @@ -244,8 +274,14 @@ async fn root() -> &'static str { /// Delete a user. Only available in debug/test builds for cleanup. #[cfg(any(test, debug_assertions, feature = "test-helpers"))] -#[tracing::instrument(skip(creds))] -async fn delete_user(creds: Credentials) -> Result { +#[tracing::instrument(skip(creds, locks))] +async fn delete_user( + Extension(locks): Extension, + creds: Credentials, +) -> Result { + let oid = user2oid(&creds.username); + let mutex = locks.entry(oid).or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))).clone(); + let _guard = mutex.lock().await; db::delete_user(creds.username).await?; tracing::info!("ok"); Ok(StatusCode::OK) @@ -260,6 +296,8 @@ async fn delete_user(creds: Credentials) -> Result { /// [`RATE_LIMIT_BURST_SIZE`]. Tests may pass a much larger value to avoid /// accidental 429s during their busy request sequences. pub fn build_router_with_burst(burst_size: u32) -> Router { + let user_locks: UserLocks = Arc::new(DashMap::new()); + let app = Router::new() .route("/api/v2/generate", get(generate)) .route("/api/v2/user", post(create_user).put(update_user)) @@ -286,10 +324,11 @@ pub fn build_router_with_burst(burst_size: u32) -> Router { .expect("invalid rate-limit configuration"); // Layers wrap routes that were registered *before* the .layer() call. - // Order (outermost → innermost): CORS → rate-limit → tracing → handler. + // Order (outermost → innermost): CORS → rate-limit → tracing → Extension → handler. // CORS must be outermost so preflight OPTIONS responses are never blocked // by the rate limiter. - app.layer(TraceLayer::new_for_http()) + app.layer(Extension(user_locks)) + .layer(TraceLayer::new_for_http()) .layer(GovernorLayer::new(rate_limit_config)) .layer(cors_layer()) } diff --git a/passwords/api/tests/concurrency_tests.rs b/passwords/api/tests/concurrency_tests.rs new file mode 100644 index 0000000..2d9605c --- /dev/null +++ b/passwords/api/tests/concurrency_tests.rs @@ -0,0 +1,88 @@ +//! Concurrency tests for the MapoPass API. +//! +//! Verifies that per-user locking serializes concurrent mutations correctly. +//! +//! ## Running +//! +//! ```sh +//! # From passwords/api/: +//! cargo test --test concurrency_tests --features test-helpers +//! ``` + +mod common; + +use axum::body::Body; +use common::{app, body_string, parse_json, run, TestUser, WithAuth}; +use http::{Request, StatusCode}; +use std::sync::Arc; +use tower::ServiceExt; + +#[test] +fn test_concurrent_password_adds_are_serialized() { + run(async { + let t = TestUser::new(); + let (user, pw) = (t.user(), t.pw()); + + // Create user + let req = Request::builder() + .method("POST") + .uri("/api/v2/user") + .auth(user, pw) + .body(Body::empty()) + .unwrap(); + let res = app().oneshot(req).await.unwrap(); + assert_eq!(res.status(), StatusCode::OK, "create user"); + + // Fire N concurrent add-password requests with distinct keys + let n = 10; + let user = Arc::new(user.to_string()); + let pw = Arc::new(pw.to_string()); + + let mut handles = Vec::with_capacity(n); + for i in 0..n { + let user = Arc::clone(&user); + let pw = Arc::clone(&pw); + let handle = tokio::spawn(async move { + let key = format!("concurrent_key_{i}"); + let val = format!("enc_val_{i}"); + let req = Request::builder() + .method("POST") + .uri(format!("/api/v2/passwords/{key}")) + .header("x-username", user.as_str()) + .header("x-password", pw.as_str()) + .header("content-type", "application/json") + .body(Body::from(format!(r#"{{"encrypted_password":"{val}"}}"#))) + .unwrap(); + let res = app().oneshot(req).await.unwrap(); + assert_eq!( + res.status(), + StatusCode::OK, + "concurrent add key {key} failed" + ); + }); + handles.push(handle); + } + + for h in handles { + h.await.expect("task panicked"); + } + + // Verify all N keys are present + let req = Request::builder() + .method("GET") + .uri("/api/v2/keys") + .header("x-username", user.as_str()) + .header("x-password", pw.as_str()) + .body(Body::empty()) + .unwrap(); + let res = app().oneshot(req).await.unwrap(); + assert_eq!(res.status(), StatusCode::OK); + let keys: Vec = parse_json(&body_string(res).await); + assert_eq!( + keys.len(), + n, + "expected {n} keys after concurrent adds, got {}: {keys:?}", + keys.len() + ); + }); +} From c1cafffc9a8d8922257797519f942b9b4b694eaa Mon Sep 17 00:00:00 2001 From: Maple Xu Date: Sat, 4 Apr 2026 13:54:18 -0400 Subject: [PATCH 2/5] Use Axum State instead of Extension for UserLocks State provides compile-time guarantees that the state was provided, unlike Extension which panics at runtime if missing. Co-Authored-By: Claude Opus 4.6 (1M context) --- passwords/api/src/lib.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/passwords/api/src/lib.rs b/passwords/api/src/lib.rs index 0f1d3a4..fbc6f60 100644 --- a/passwords/api/src/lib.rs +++ b/passwords/api/src/lib.rs @@ -3,7 +3,7 @@ pub mod encrypt; pub mod env; use axum::{ - extract::{rejection::PathRejection, Extension, FromRequestParts, Path}, + extract::{rejection::PathRejection, FromRequestParts, Path, State}, http::{header::HeaderName, request::Parts, HeaderValue, Method, StatusCode}, response::IntoResponse, routing::{get, post}, @@ -181,7 +181,7 @@ async fn generate() -> Result, Error> { #[tracing::instrument(skip(creds, locks))] async fn create_user( - Extension(locks): Extension, + State(locks): State, creds: Credentials, ) -> Result { let oid = user2oid(&creds.username); @@ -201,7 +201,7 @@ async fn verify_user(creds: Credentials) -> Result { #[tracing::instrument(skip(creds, locks, payload))] async fn update_user( - Extension(locks): Extension, + State(locks): State, creds: Credentials, Json(payload): Json, ) -> Result { @@ -239,7 +239,7 @@ async fn get_stored_passwords(creds: Credentials) -> Result>, E #[tracing::instrument(skip(creds, locks, payload))] async fn add_stored_password( - Extension(locks): Extension, + State(locks): State, creds: Credentials, ValidatedKey(key): ValidatedKey, Json(payload): Json, @@ -254,7 +254,7 @@ async fn add_stored_password( #[tracing::instrument(skip(creds, locks, payload))] async fn change_stored_password( - Extension(locks): Extension, + State(locks): State, creds: Credentials, ValidatedKey(key): ValidatedKey, Json(payload): Json, @@ -276,7 +276,7 @@ async fn root() -> &'static str { #[cfg(any(test, debug_assertions, feature = "test-helpers"))] #[tracing::instrument(skip(creds, locks))] async fn delete_user( - Extension(locks): Extension, + State(locks): State, creds: Credentials, ) -> Result { let oid = user2oid(&creds.username); @@ -324,10 +324,10 @@ pub fn build_router_with_burst(burst_size: u32) -> Router { .expect("invalid rate-limit configuration"); // Layers wrap routes that were registered *before* the .layer() call. - // Order (outermost → innermost): CORS → rate-limit → tracing → Extension → handler. + // Order (outermost → innermost): CORS → rate-limit → tracing → handler. // CORS must be outermost so preflight OPTIONS responses are never blocked - // by the rate limiter. - app.layer(Extension(user_locks)) + // by the rate limiter. UserLocks are provided via .with_state() (not a layer). + app.with_state(user_locks) .layer(TraceLayer::new_for_http()) .layer(GovernorLayer::new(rate_limit_config)) .layer(cors_layer()) From 348d85e84a9501939855f0a35376f31ae745be39 Mon Sep 17 00:00:00 2001 From: Maple Xu Date: Sat, 4 Apr 2026 17:20:41 -0400 Subject: [PATCH 3/5] Move per-user locks from handlers into db module Locking protects DB operations, so it belongs in the db layer. Handlers no longer need to know about locks, and the duplicated user2oid calls are eliminated. Co-Authored-By: Claude Opus 4.6 (1M context) --- passwords/api/src/db.rs | 34 ++++++++++++++++++++++ passwords/api/src/lib.rs | 61 ++++++++-------------------------------- 2 files changed, 45 insertions(+), 50 deletions(-) diff --git a/passwords/api/src/db.rs b/passwords/api/src/db.rs index 663c982..2803d09 100644 --- a/passwords/api/src/db.rs +++ b/passwords/api/src/db.rs @@ -1,5 +1,6 @@ use crate::encrypt::{user2oid, Credentials, CryptoError, MasterKey}; use crate::env::EnvVars; +use dashmap::DashMap; use mongodb::{ bson::{doc, oid::ObjectId, to_bson, Bson}, error::Error as MongoError, @@ -7,9 +8,30 @@ use mongodb::{ Client, Collection, }; use serde::{Deserialize, Serialize}; +use std::sync::Arc; static DB: tokio::sync::OnceCell> = tokio::sync::OnceCell::const_new(); +// --------------------------------------------------------------------------- +// Per-user lock map for serializing mutating requests +// --------------------------------------------------------------------------- + +/// A shared map from user ObjectId to a per-user async mutex. +/// All mutating (write) db functions acquire the lock for the target user +/// before proceeding, preventing race conditions on concurrent writes. +static USER_LOCKS: std::sync::LazyLock>>> = + std::sync::LazyLock::new(DashMap::new); + +/// Acquire the per-user async mutex for the given OID, returning the guard. +/// The guard must be held for the duration of the mutating operation. +async fn acquire_user_lock(oid: ObjectId) -> tokio::sync::OwnedMutexGuard<()> { + let mutex = USER_LOCKS + .entry(oid) + .or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))) + .clone(); + mutex.lock_owned().await +} + pub static OID_LEN: usize = 12; pub type OID = ObjectId; @@ -82,6 +104,8 @@ pub async fn add_user(creds: Credentials) -> Result<(), DbError> { let db = DB.get().unwrap(); let en_user = user2oid(&creds.username); + let _guard = acquire_user_lock(en_user).await; + if find_user(&creds.username, en_user).await.is_ok() { return Err(DbError::GenericError { error_msg: "Cannot add user because username already exists".to_owned(), @@ -160,6 +184,9 @@ pub async fn add_stored_password( key: String, encrypted_password: String, ) -> Result<(), DbError> { + let en_user = user2oid(&creds.username); + let _guard = acquire_user_lock(en_user).await; + let (db, user, en_user) = authenticate_user(creds).await?; if user @@ -194,6 +221,9 @@ pub async fn change_stored_password( key: String, encrypted_password: String, ) -> Result<(), DbError> { + let en_user = user2oid(&creds.username); + let _guard = acquire_user_lock(en_user).await; + let (db, user, en_user) = authenticate_user(creds).await?; user.stored_passwords @@ -223,6 +253,9 @@ pub async fn change_master_password( new_password: String, updated_stored_passwords: Vec, ) -> Result<(), DbError> { + let en_user = user2oid(&creds.username); + let _guard = acquire_user_lock(en_user).await; + let (db, user, en_user) = authenticate_user(creds).await?; if user.stored_passwords.len() != updated_stored_passwords.len() { @@ -262,6 +295,7 @@ pub async fn change_master_password( pub async fn delete_user(username: String) -> Result<(), DbError> { let db = DB.get().unwrap(); let en_user = user2oid(&username); + let _guard = acquire_user_lock(en_user).await; db.delete_one(doc! { "_id": en_user }).await?; Ok(()) } diff --git a/passwords/api/src/lib.rs b/passwords/api/src/lib.rs index fbc6f60..b31de07 100644 --- a/passwords/api/src/lib.rs +++ b/passwords/api/src/lib.rs @@ -3,33 +3,21 @@ pub mod encrypt; pub mod env; use axum::{ - extract::{rejection::PathRejection, FromRequestParts, Path, State}, + extract::{rejection::PathRejection, FromRequestParts, Path}, http::{header::HeaderName, request::Parts, HeaderValue, Method, StatusCode}, response::IntoResponse, routing::{get, post}, Json, Router, }; -use dashmap::DashMap; use db::DbError; -use encrypt::{generate_password, user2oid, Credentials, CryptoError}; +use encrypt::{generate_password, Credentials, CryptoError}; use env::EnvVars; -use mongodb::bson::oid::ObjectId; use serde::Deserialize; -use std::sync::Arc; use tower_governor::governor::GovernorConfigBuilder; use tower_governor::GovernorLayer; use tower_http::cors::CorsLayer; use tower_http::trace::TraceLayer; -// --------------------------------------------------------------------------- -// Per-user lock map for serializing mutating requests -// --------------------------------------------------------------------------- - -/// A shared map from user ObjectId to a per-user async mutex. -/// All mutating (write) handlers acquire the lock for the target user before -/// proceeding, preventing race conditions on concurrent writes. -pub type UserLocks = Arc>>>; - const MAX_KEY_LENGTH: usize = 128; // --------------------------------------------------------------------------- @@ -179,14 +167,8 @@ async fn generate() -> Result, Error> { Ok(Json(pw)) } -#[tracing::instrument(skip(creds, locks))] -async fn create_user( - State(locks): State, - creds: Credentials, -) -> Result { - let oid = user2oid(&creds.username); - let mutex = locks.entry(oid).or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))).clone(); - let _guard = mutex.lock().await; +#[tracing::instrument(skip(creds))] +async fn create_user(creds: Credentials) -> Result { db::add_user(creds).await?; tracing::info!("ok"); Ok(StatusCode::OK) @@ -199,15 +181,11 @@ async fn verify_user(creds: Credentials) -> Result { Ok(StatusCode::OK) } -#[tracing::instrument(skip(creds, locks, payload))] +#[tracing::instrument(skip(creds, payload))] async fn update_user( - State(locks): State, creds: Credentials, Json(payload): Json, ) -> Result { - let oid = user2oid(&creds.username); - let mutex = locks.entry(oid).or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))).clone(); - let _guard = mutex.lock().await; db::change_master_password(creds, payload.new_password, payload.passwords).await?; tracing::info!("ok"); Ok(StatusCode::OK) @@ -237,31 +215,23 @@ async fn get_stored_passwords(creds: Credentials) -> Result>, E Ok(Json(pws)) } -#[tracing::instrument(skip(creds, locks, payload))] +#[tracing::instrument(skip(creds, payload))] async fn add_stored_password( - State(locks): State, creds: Credentials, ValidatedKey(key): ValidatedKey, Json(payload): Json, ) -> Result { - let oid = user2oid(&creds.username); - let mutex = locks.entry(oid).or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))).clone(); - let _guard = mutex.lock().await; db::add_stored_password(creds, key, payload.encrypted_password).await?; tracing::info!("ok"); Ok(StatusCode::OK) } -#[tracing::instrument(skip(creds, locks, payload))] +#[tracing::instrument(skip(creds, payload))] async fn change_stored_password( - State(locks): State, creds: Credentials, ValidatedKey(key): ValidatedKey, Json(payload): Json, ) -> Result { - let oid = user2oid(&creds.username); - let mutex = locks.entry(oid).or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))).clone(); - let _guard = mutex.lock().await; db::change_stored_password(creds, key, payload.encrypted_password).await?; tracing::info!("ok"); Ok(StatusCode::OK) @@ -274,14 +244,8 @@ async fn root() -> &'static str { /// Delete a user. Only available in debug/test builds for cleanup. #[cfg(any(test, debug_assertions, feature = "test-helpers"))] -#[tracing::instrument(skip(creds, locks))] -async fn delete_user( - State(locks): State, - creds: Credentials, -) -> Result { - let oid = user2oid(&creds.username); - let mutex = locks.entry(oid).or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))).clone(); - let _guard = mutex.lock().await; +#[tracing::instrument(skip(creds))] +async fn delete_user(creds: Credentials) -> Result { db::delete_user(creds.username).await?; tracing::info!("ok"); Ok(StatusCode::OK) @@ -296,8 +260,6 @@ async fn delete_user( /// [`RATE_LIMIT_BURST_SIZE`]. Tests may pass a much larger value to avoid /// accidental 429s during their busy request sequences. pub fn build_router_with_burst(burst_size: u32) -> Router { - let user_locks: UserLocks = Arc::new(DashMap::new()); - let app = Router::new() .route("/api/v2/generate", get(generate)) .route("/api/v2/user", post(create_user).put(update_user)) @@ -326,9 +288,8 @@ pub fn build_router_with_burst(burst_size: u32) -> Router { // Layers wrap routes that were registered *before* the .layer() call. // Order (outermost → innermost): CORS → rate-limit → tracing → handler. // CORS must be outermost so preflight OPTIONS responses are never blocked - // by the rate limiter. UserLocks are provided via .with_state() (not a layer). - app.with_state(user_locks) - .layer(TraceLayer::new_for_http()) + // by the rate limiter. + app.layer(TraceLayer::new_for_http()) .layer(GovernorLayer::new(rate_limit_config)) .layer(cors_layer()) } From 3b02a07e0428877882869707ffff028a4504acd5 Mon Sep 17 00:00:00 2001 From: Maple Xu Date: Sat, 4 Apr 2026 19:21:20 -0400 Subject: [PATCH 4/5] Reuse en_user from authenticate_user instead of duplicating Grab the per-user lock after authenticate_user returns, using the en_user it already computed. Removes the duplicate user2oid calls without needing a separate function. Co-Authored-By: Claude Opus 4.6 (1M context) --- passwords/api/src/db.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/passwords/api/src/db.rs b/passwords/api/src/db.rs index 2803d09..30c3675 100644 --- a/passwords/api/src/db.rs +++ b/passwords/api/src/db.rs @@ -184,10 +184,8 @@ pub async fn add_stored_password( key: String, encrypted_password: String, ) -> Result<(), DbError> { - let en_user = user2oid(&creds.username); - let _guard = acquire_user_lock(en_user).await; - let (db, user, en_user) = authenticate_user(creds).await?; + let _guard = acquire_user_lock(en_user).await; if user .stored_passwords @@ -221,10 +219,8 @@ pub async fn change_stored_password( key: String, encrypted_password: String, ) -> Result<(), DbError> { - let en_user = user2oid(&creds.username); - let _guard = acquire_user_lock(en_user).await; - let (db, user, en_user) = authenticate_user(creds).await?; + let _guard = acquire_user_lock(en_user).await; user.stored_passwords .into_iter() @@ -253,10 +249,8 @@ pub async fn change_master_password( new_password: String, updated_stored_passwords: Vec, ) -> Result<(), DbError> { - let en_user = user2oid(&creds.username); - let _guard = acquire_user_lock(en_user).await; - let (db, user, en_user) = authenticate_user(creds).await?; + let _guard = acquire_user_lock(en_user).await; if user.stored_passwords.len() != updated_stored_passwords.len() { return Err(DbError::GenericError { From 1a709f739ae0d89869f668a0aac53432e436a897 Mon Sep 17 00:00:00 2001 From: Maple Xu Date: Fri, 10 Apr 2026 20:25:22 -0400 Subject: [PATCH 5/5] Fix TOCTOU: re-verify master password after acquiring lock After acquiring the per-user lock in change_master_password and add_stored_password, re-read the user from DB and verify the master key hasn't changed since authentication. Prevents concurrent requests from proceeding with stale credentials after another request changed the master password. Co-Authored-By: Claude Opus 4.6 (1M context) --- passwords/api/src/db.rs | 20 ++ passwords/api/tests/concurrency_tests.rs | 266 +++++++++++++++++++++++ 2 files changed, 286 insertions(+) diff --git a/passwords/api/src/db.rs b/passwords/api/src/db.rs index 30c3675..4512398 100644 --- a/passwords/api/src/db.rs +++ b/passwords/api/src/db.rs @@ -187,6 +187,16 @@ pub async fn add_stored_password( let (db, user, en_user) = authenticate_user(creds).await?; let _guard = acquire_user_lock(en_user).await; + // Re-read the user after acquiring the lock to detect TOCTOU: if another + // request changed the master password between our authenticate_user call + // and the lock acquisition, the stored master_pw will have changed. + let current_user = find_user("", en_user).await?; + if current_user.master_key.master_pw != user.master_key.master_pw { + return Err(DbError::GenericError { + error_msg: "Master password was changed by a concurrent request".to_owned(), + }); + } + if user .stored_passwords .iter() @@ -252,6 +262,16 @@ pub async fn change_master_password( let (db, user, en_user) = authenticate_user(creds).await?; let _guard = acquire_user_lock(en_user).await; + // Re-read the user after acquiring the lock to detect TOCTOU: if another + // request changed the master password between our authenticate_user call + // and the lock acquisition, the stored master_pw will have changed. + let current_user = find_user("", en_user).await?; + if current_user.master_key.master_pw != user.master_key.master_pw { + return Err(DbError::GenericError { + error_msg: "Master password was changed by a concurrent request".to_owned(), + }); + } + if user.stored_passwords.len() != updated_stored_passwords.len() { return Err(DbError::GenericError { error_msg: format!( diff --git a/passwords/api/tests/concurrency_tests.rs b/passwords/api/tests/concurrency_tests.rs index 2d9605c..afc4bb2 100644 --- a/passwords/api/tests/concurrency_tests.rs +++ b/passwords/api/tests/concurrency_tests.rs @@ -15,6 +15,7 @@ use axum::body::Body; use common::{app, body_string, parse_json, run, TestUser, WithAuth}; use http::{Request, StatusCode}; use std::sync::Arc; +use tokio::sync::Barrier; use tower::ServiceExt; #[test] @@ -86,3 +87,268 @@ fn test_concurrent_password_adds_are_serialized() { ); }); } + +/// Exposes a TOCTOU bug: two requests both authenticate with the current +/// password *before* either acquires the per-user lock, so the second one +/// succeeds with stale credentials that should have been invalidated by +/// the first request's password change. +#[test] +fn test_stale_credentials_rejected_after_concurrent_master_password_change() { + run(async { + let pw1 = "original_password"; + let pw2 = "changed_to_pw2"; + let pw3 = "changed_to_pw3"; + + // --- Setup: create user with pw1 and add one stored password --- + let t = TestUser::new(); + let user = t.user().to_string(); + + let req = Request::builder() + .method("POST") + .uri("/api/v2/user") + .auth(&user, pw1) + .body(Body::empty()) + .unwrap(); + let res = app().oneshot(req).await.unwrap(); + assert_eq!(res.status(), StatusCode::OK, "create user"); + + let req = Request::builder() + .method("POST") + .uri("/api/v2/passwords/site1") + .auth(&user, pw1) + .header("content-type", "application/json") + .body(Body::from(r#"{"encrypted_password":"enc_value_1"}"#)) + .unwrap(); + let res = app().oneshot(req).await.unwrap(); + assert_eq!(res.status(), StatusCode::OK, "add stored password"); + + // --- Fire two concurrent change-master-password requests --- + // Both authenticate with pw1 (current). Request A changes to pw2, + // request B changes to pw3. The barrier ensures both tasks reach + // the request point before either one completes, maximizing the + // window for both to pass authentication before the lock is acquired. + let barrier = Arc::new(Barrier::new(2)); + let user_a = user.clone(); + let barrier_a = Arc::clone(&barrier); + let handle_a = tokio::spawn(async move { + barrier_a.wait().await; + let req = Request::builder() + .method("PUT") + .uri("/api/v2/user") + .auth(&user_a, pw1) + .header("content-type", "application/json") + .body(Body::from(format!( + r#"{{"new_password":"{pw2}","passwords":["reenc_a"]}}"# + ))) + .unwrap(); + app().oneshot(req).await.unwrap() + }); + + let user_b = user.clone(); + let barrier_b = Arc::clone(&barrier); + let handle_b = tokio::spawn(async move { + barrier_b.wait().await; + let req = Request::builder() + .method("PUT") + .uri("/api/v2/user") + .auth(&user_b, pw1) + .header("content-type", "application/json") + .body(Body::from(format!( + r#"{{"new_password":"{pw3}","passwords":["reenc_b"]}}"# + ))) + .unwrap(); + app().oneshot(req).await.unwrap() + }); + + let res_a = handle_a.await.expect("task A panicked"); + let res_b = handle_b.await.expect("task B panicked"); + + let status_a = res_a.status(); + let status_b = res_b.status(); + + // Exactly one should succeed and one should fail. The second request + // to acquire the lock should discover that pw1 is no longer valid + // (the first request changed it) and return 404. + let (ok_count, fail_count) = [status_a, status_b] + .iter() + .fold((0u32, 0u32), |(ok, fail), s| { + if *s == StatusCode::OK { + (ok + 1, fail) + } else { + assert_eq!( + *s, + StatusCode::NOT_FOUND, + "failing request should return 404, got {s}" + ); + (ok, fail + 1) + } + }); + + assert_eq!( + ok_count, 1, + "expected exactly 1 success, got {ok_count} (A={status_a}, B={status_b})" + ); + assert_eq!( + fail_count, 1, + "expected exactly 1 failure, got {fail_count} (A={status_a}, B={status_b})" + ); + + // --- Verify final state is consistent --- + // The winning password (pw2 or pw3) should work; the other should not. + // pw1 should definitely not work anymore. + let winning_pw = if status_a == StatusCode::OK { + pw2 + } else { + pw3 + }; + let losing_pw = if status_a == StatusCode::OK { + pw3 + } else { + pw2 + }; + + // Winner's password works + let req = Request::builder() + .method("GET") + .uri("/api/v2/user/verify") + .auth(&user, winning_pw) + .body(Body::empty()) + .unwrap(); + let res = app().oneshot(req).await.unwrap(); + assert_eq!( + res.status(), + StatusCode::OK, + "winning password should verify" + ); + + // Loser's password does not work + let req = Request::builder() + .method("GET") + .uri("/api/v2/user/verify") + .auth(&user, losing_pw) + .body(Body::empty()) + .unwrap(); + let res = app().oneshot(req).await.unwrap(); + assert_eq!( + res.status(), + StatusCode::NOT_FOUND, + "losing password should not verify" + ); + + // Original password (pw1) does not work + let req = Request::builder() + .method("GET") + .uri("/api/v2/user/verify") + .auth(&user, pw1) + .body(Body::empty()) + .unwrap(); + let res = app().oneshot(req).await.unwrap(); + assert_eq!( + res.status(), + StatusCode::NOT_FOUND, + "original password should not verify after master change" + ); + + // Clean up: update TestUser's password so Drop can delete with correct creds + // (TestUser::drop uses the original password, but we changed it) + // Actually, TestUser::drop calls db::delete_user which takes just the username, + // so cleanup will work regardless. + }); +} + +/// Verifies that add_stored_password rejects stale credentials after a +/// concurrent master password change. Thread A changes the master password, +/// thread B tries to add a password using the old (now-invalid) credentials. +/// Thread B should fail because its pre-auth credentials are stale. +#[test] +fn test_stale_credentials_rejected_on_add_password_after_master_change() { + run(async { + let pw_old = "original_password"; + let pw_new = "new_password"; + + // --- Setup: create user with pw_old --- + let t = TestUser::new(); + let user = t.user().to_string(); + + let req = Request::builder() + .method("POST") + .uri("/api/v2/user") + .auth(&user, pw_old) + .body(Body::empty()) + .unwrap(); + let res = app().oneshot(req).await.unwrap(); + assert_eq!(res.status(), StatusCode::OK, "create user"); + + // --- Fire two concurrent requests --- + // A: change master password (pw_old -> pw_new) + // B: add a stored password using pw_old + // Both authenticate with pw_old before either acquires the lock. + let barrier = Arc::new(Barrier::new(2)); + + let user_a = user.clone(); + let barrier_a = Arc::clone(&barrier); + let handle_a = tokio::spawn(async move { + barrier_a.wait().await; + let req = Request::builder() + .method("PUT") + .uri("/api/v2/user") + .auth(&user_a, pw_old) + .header("content-type", "application/json") + .body(Body::from(format!( + r#"{{"new_password":"{pw_new}","passwords":[]}}"# + ))) + .unwrap(); + app().oneshot(req).await.unwrap() + }); + + let user_b = user.clone(); + let barrier_b = Arc::clone(&barrier); + let handle_b = tokio::spawn(async move { + barrier_b.wait().await; + let req = Request::builder() + .method("POST") + .uri("/api/v2/passwords/sneaky_key") + .auth(&user_b, pw_old) + .header("content-type", "application/json") + .body(Body::from(r#"{"encrypted_password":"enc_val"}"#)) + .unwrap(); + app().oneshot(req).await.unwrap() + }); + + let res_a = handle_a.await.expect("task A panicked"); + let res_b = handle_b.await.expect("task B panicked"); + + let status_a = res_a.status(); + let status_b = res_b.status(); + + // If the master password change wins the lock first, the add-password + // request should be rejected because its credentials are stale. + // If add-password wins the lock first, both succeed (add happens + // before the password changes, and change_master_password re-encrypts + // all passwords including the newly added one — though the test sends + // an empty passwords array, so the change would fail on count mismatch). + // + // In either ordering, at most one can succeed. If A wins the lock: + // A succeeds, B should fail (stale creds). + // If B wins the lock: + // B succeeds (adds password), A fails (count mismatch: 1 stored vs 0 sent). + // Either way: exactly one success. + let ok_count = [status_a, status_b] + .iter() + .filter(|s| **s == StatusCode::OK) + .count(); + let fail_count = [status_a, status_b] + .iter() + .filter(|s| **s == StatusCode::NOT_FOUND) + .count(); + + assert_eq!( + ok_count, 1, + "expected exactly 1 success, got {ok_count} (A={status_a}, B={status_b})" + ); + assert_eq!( + fail_count, 1, + "expected exactly 1 failure, got {fail_count} (A={status_a}, B={status_b})" + ); + }); +}