Skip to content

Commit bbe8913

Browse files
committed
Security: Add transaction wrapper to prevent TOCTOU vulnerabilities
Wrap role-based data access in a database transaction to eliminate Time-of-Check to Time-of-Use (TOCTOU) race conditions. Changes: 1. Update entity_api coaching relationship functions to accept ConnectionTrait - find_by_organization_with_user_names: &DatabaseConnection -> &impl ConnectionTrait - find_by_user_and_organization_with_user_names: &DatabaseConnection -> &impl ConnectionTrait - Now compatible with both database connections and transactions 2. Wrap domain::coaching_relationship function in transaction - Begin transaction before role check - Execute all queries within transaction scope - Commit transaction after data retrieval - Ensures atomic snapshot of database state Security Benefits: - Prevents race condition where user roles change between check and data access - Provides REPEATABLE READ isolation level guarantees (PostgreSQL default) - Ensures consistent view of user permissions and accessible data - Eliminates window for privilege escalation during query execution The transaction ensures that the role check and coaching relationship retrieval see the same consistent snapshot of the database, preventing scenarios where a user's admin privileges are revoked mid-query but they still receive admin-level data. All existing tests pass with no behavioral changes.
1 parent 9a58f54 commit bbe8913

File tree

2 files changed

+26
-6
lines changed

2 files changed

+26
-6
lines changed

domain/src/coaching_relationship.rs

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use crate::coaching_relationships::Model;
22
use crate::error::{DomainErrorKind, EntityErrorKind, Error, InternalErrorKind};
33
use entity_api::query::{IntoQueryFilterMap, QuerySort};
44
use entity_api::{coaching_relationships, query};
5-
use sea_orm::DatabaseConnection;
5+
use sea_orm::{DatabaseConnection, TransactionTrait};
66

77
pub use entity_api::coaching_relationship::{
88
create, find_by_id, find_by_organization_with_user_names, find_by_user,
@@ -27,13 +27,25 @@ where
2727
/// - SuperAdmins (global role with organization_id = NULL) see all relationships in the organization
2828
/// - Admins (organization-specific role) see all relationships in their organization
2929
/// - Regular users see only relationships where they are the coach or coachee
30+
///
31+
/// This function uses a database transaction to prevent Time-of-Check to Time-of-Use (TOCTOU)
32+
/// vulnerabilities by ensuring the role check and data retrieval happen atomically within
33+
/// a consistent database snapshot.
3034
pub async fn find_by_organization_for_user_with_user_names(
3135
db: &DatabaseConnection,
3236
user_id: crate::Id,
3337
organization_id: crate::Id,
3438
) -> Result<Vec<CoachingRelationshipWithUserNames>, Error> {
39+
// Begin transaction to ensure atomicity and prevent TOCTOU vulnerabilities
40+
let txn = db.begin().await.map_err(|e| Error {
41+
source: Some(Box::new(e)),
42+
error_kind: DomainErrorKind::Internal(InternalErrorKind::Entity(
43+
EntityErrorKind::DbTransaction,
44+
)),
45+
})?;
46+
3547
// Check if user has admin access using entity_api layer
36-
let is_admin = entity_api::user::has_admin_access(db, user_id, organization_id)
48+
let is_admin = entity_api::user::has_admin_access(&txn, user_id, organization_id)
3749
.await
3850
.map_err(|e| Error {
3951
source: Some(Box::new(e)),
@@ -44,11 +56,19 @@ pub async fn find_by_organization_for_user_with_user_names(
4456

4557
let coaching_relationships = if is_admin {
4658
// Admin users see all relationships in the organization
47-
find_by_organization_with_user_names(db, organization_id).await?
59+
find_by_organization_with_user_names(&txn, organization_id).await?
4860
} else {
4961
// Regular users see only relationships they're associated with (as coach or coachee)
50-
find_by_user_and_organization_with_user_names(db, user_id, organization_id).await?
62+
find_by_user_and_organization_with_user_names(&txn, user_id, organization_id).await?
5163
};
5264

65+
// Commit transaction
66+
txn.commit().await.map_err(|e| Error {
67+
source: Some(Box::new(e)),
68+
error_kind: DomainErrorKind::Internal(InternalErrorKind::Entity(
69+
EntityErrorKind::DbTransaction,
70+
)),
71+
})?;
72+
5373
Ok(coaching_relationships)
5474
}

entity_api/src/coaching_relationship.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ pub async fn find_by_organization(
125125
}
126126

127127
pub async fn find_by_organization_with_user_names(
128-
db: &DatabaseConnection,
128+
db: &impl ConnectionTrait,
129129
organization_id: Id,
130130
) -> Result<Vec<CoachingRelationshipWithUserNames>, Error> {
131131
let coaches = Alias::new("coaches");
@@ -160,7 +160,7 @@ pub async fn find_by_organization_with_user_names(
160160
}
161161

162162
pub async fn find_by_user_and_organization_with_user_names(
163-
db: &DatabaseConnection,
163+
db: &impl ConnectionTrait,
164164
user_id: Id,
165165
organization_id: Id,
166166
) -> Result<Vec<CoachingRelationshipWithUserNames>, Error> {

0 commit comments

Comments
 (0)