Id collision mitigation (#181)

This patch introduces a new method, `create`, to the `SessionStore` trait to distinguish between creating a new session and updating an existing one. This distinction is crucial for mitigating the potential for session ID collisions.

Although the probability of session ID collisions is statistically low, given that IDs are composed of securely-random `i128` values, such collisions pose a significant security risk. A store that does not differentiate between session creation and updates could inadvertently allow an existing session to be accessed, leading to potential session takeovers.

To prevent this, stores must ensure the uniqueness of session IDs during creation. The new `create` method is designed to allow session store implementers to handle any conflicts and resolve them.

This change is a breaking interface update. As a transitional measure, we have provided a default implementation of create that wraps the existing save method. However, this default is not immune to the original issue. Therefore, it is imperative that stores override the create method with an implementation that adheres to the required uniqueness semantics, thereby effectively mitigating the risk of session ID collisions.

Author: maxcountryman

memory-store/src/lib.rs

@@ -19,15 +19,22 @@ use tower_sessions_core::{
 /// MemoryStore::default();
 /// ```
 #[derive(Clone, Debug, Default)]
-pub struct MemoryStore(Arc<Mutex<HashMap<Id, (Record, OffsetDateTime)>>>);
+pub struct MemoryStore(Arc<Mutex<HashMap<Id, Record>>>);
 
 #[async_trait]
 impl SessionStore for MemoryStore {
+    async fn create(&self, record: &mut Record) -> session_store::Result<()> {
+        let mut store_guard = self.0.lock().await;
+        while store_guard.contains_key(&record.id) {
+            // Session ID collision mitigation.
+            record.id = Id::default();
+        }
+        store_guard.insert(record.id, record.clone());
+        Ok(())
+    }
+
     async fn save(&self, record: &Record) -> session_store::Result<()> {
-        self.0
-            .lock()
-            .await
-            .insert(record.id, (record.clone(), record.expiry_date));
+        self.0.lock().await.insert(record.id, record.clone());
         Ok(())
     }
 
@@ -37,8 +44,7 @@ impl SessionStore for MemoryStore {
             .lock()
             .await
             .get(session_id)
-            .filter(|(_, expiry_date)| is_active(*expiry_date))
-            .map(|(session, _)| session)
+            .filter(|Record { expiry_date, .. }| is_active(*expiry_date))
             .cloned())
     }
 
@@ -51,3 +57,78 @@ impl SessionStore for MemoryStore {
 fn is_active(expiry_date: OffsetDateTime) -> bool {
     expiry_date > OffsetDateTime::now_utc()
 }
+
+#[cfg(test)]
+mod tests {
+    use time::Duration;
+
+    use super::*;
+
+    #[tokio::test]
+    async fn test_create() {
+        let store = MemoryStore::default();
+        let mut record = Record {
+            id: Default::default(),
+            data: Default::default(),
+            expiry_date: OffsetDateTime::now_utc() + Duration::minutes(30),
+        };
+        assert!(store.create(&mut record).await.is_ok());
+    }
+
+    #[tokio::test]
+    async fn test_save() {
+        let store = MemoryStore::default();
+        let record = Record {
+            id: Default::default(),
+            data: Default::default(),
+            expiry_date: OffsetDateTime::now_utc() + Duration::minutes(30),
+        };
+        assert!(store.save(&record).await.is_ok());
+    }
+
+    #[tokio::test]
+    async fn test_load() {
+        let store = MemoryStore::default();
+        let mut record = Record {
+            id: Default::default(),
+            data: Default::default(),
+            expiry_date: OffsetDateTime::now_utc() + Duration::minutes(30),
+        };
+        store.create(&mut record).await.unwrap();
+        let loaded_record = store.load(&record.id).await.unwrap();
+        assert_eq!(Some(record), loaded_record);
+    }
+
+    #[tokio::test]
+    async fn test_delete() {
+        let store = MemoryStore::default();
+        let mut record = Record {
+            id: Default::default(),
+            data: Default::default(),
+            expiry_date: OffsetDateTime::now_utc() + Duration::minutes(30),
+        };
+        store.create(&mut record).await.unwrap();
+        assert!(store.delete(&record.id).await.is_ok());
+        assert_eq!(None, store.load(&record.id).await.unwrap());
+    }
+
+    #[tokio::test]
+    async fn test_create_id_collision() {
+        let store = MemoryStore::default();
+        let expiry_date = OffsetDateTime::now_utc() + Duration::minutes(30);
+        let mut record1 = Record {
+            id: Default::default(),
+            data: Default::default(),
+            expiry_date,
+        };
+        let mut record2 = Record {
+            id: Default::default(),
+            data: Default::default(),
+            expiry_date,
+        };
+        store.create(&mut record1).await.unwrap();
+        record2.id = record1.id; // Set the same ID for record2
+        store.create(&mut record2).await.unwrap();
+        assert_ne!(record1.id, record2.id); // IDs should be different
+    }
+}

src/lib.rs

@@ -394,34 +394,26 @@
 //!
 //! Cookies hold a pointer to the session, rather than the session's data, and
 //! because of this, the `tower` middleware is focused on managing the process
-//! of hydrating a session from the store and managing its life cycle.
+//! of initializing a session which can later be used in code to transparently
+//! interact with the store.
 //!
-//! We load a session by looking for a cookie that matches our configured
-//! session cookie name. If no such cookie is found or a cookie is found but the
-//! store has no such session or the session is no longer active, we create a
-//! new session.
-//!
-//! It's important to note that creating a session **does not** save the session
-//! to the store. In fact, the session store is not used at all unless the
-//! session is read from or written to. In other words, the middleware only
-//! introduces session store overhead when the session is actually used.
+//! A session is initialized by looking for a cookie that matches the configured
+//! session cookie name. If no such cookie is found or a cookie is found but is
+//! malformed, an empty session is initialized.
 //!  
-//! Modified sessions will invoke the session store's
-//! [`save`](SessionStore::save) method as well as send a `Set-Cookie` header.
-//! While deleted sessions will either be:
-//!
-//! 1. Deleted, invoking the [`delete`](SessionStore::delete) method and setting
-//!    a removal cookie or,
-//! 2. Cycled, invoking the `delete` method but setting a new ID on the session;
-//!    the session will have been marked as modified and so this will also set a
-//!    `Set-Cookie` header on the response.
+//! Modified sessions will invoke the session's [`save`](Session::save) method
+//! as well as append to the `Set-Cookie` header of the response.
 //!
-//! Empty sessions are considered to be deleted and are removed from the session
-//! store as well as the user agent.
+//! Empty sessions are considered deleted and will set a removal cookie
+//! on the response but are not removed from the store directly.
 //!
-//! Sessions also carry with them a configurable expiry and will be deleted in
+//! Sessions also carry with them a configurable expiry and will be removed in
 //! accordance with this.
 //!
+//! Notably, the session life cycle minimizes overhead with the store. All
+//! session store methods are deferred until the point [`Session`] is used in
+//! code and more specifically one of its methods requiring the store is called.
+//!
 //! [^json]: Using JSON allows us to translate arbitrary types to virtually
 //! any backend and gives us a nice interface with which to interact with the
 //! session.

tower-sessions-core/Cargo.toml

@@ -34,3 +34,4 @@ tracing = { version = "0.1.40", features = ["log"] }
 tower-sessions = { workspace = true, features = ["memory-store"] }
 tokio-test = "0.4.3"
 tokio = { workspace = true, features = ["rt", "macros"] }
+mockall = "0.12.1"

tower-sessions-core/src/session.rs

@@ -649,19 +649,21 @@ impl Session {
     pub async fn save(&self) -> Result<()> {
         let mut record_guard = self.get_record().await?;
         record_guard.expiry_date = self.expiry_date();
-        {
-            let mut session_id_guard = self.session_id.lock();
-            if session_id_guard.is_none() {
-                // Generate a new ID here since e.g. flush may have been called, which will
-                // not directly update the record ID.
-                let id = Id::default();
-                *session_id_guard = Some(id);
-                record_guard.id = id;
-            }
-        }
-
-        self.store.save(&record_guard).await.map_err(Error::Store)?;
 
+        // Session ID is `None` if:
+        //
+        //  1. No valid cookie was found on the request or,
+        //  2. No valid session was found in the store.
+        //
+        // In either case, we must create a new session via the store interface.
+        //
+        // Potential ID collisions must be handled by session store implementers.
+        if self.session_id.lock().is_none() {
+            self.store.create(&mut record_guard).await?;
+            *self.session_id.lock() = Some(record_guard.id);
+        } else {
+            self.store.save(&record_guard).await?;
+        }
         Ok(())
     }