fix: double-serialization of session state

Use `serde_json::Map<String, serde_json::Value>` instead of
`HashMap<String, String>`. Has the added benefit of fully type-safe json
storage in sessions.

see actix#366

Author: willothy

actix-session/src/middleware.rs

@@ -1,4 +1,4 @@
-use std::{collections::HashMap, fmt, future::Future, pin::Pin, rc::Rc};
+use std::{fmt, future::Future, pin::Pin, rc::Rc};
 
 use actix_utils::future::{ready, Ready};
 use actix_web::{
@@ -15,7 +15,7 @@ use crate::{
         self, Configuration, CookieConfiguration, CookieContentSecurity, SessionMiddlewareBuilder,
         TtlExtensionPolicy,
     },
-    storage::{LoadError, SessionKey, SessionStore},
+    storage::{LoadError, SessionKey, SessionState, SessionStore},
     Session, SessionStatus,
 };
 
@@ -358,7 +358,7 @@ fn extract_session_key(req: &ServiceRequest, config: &CookieConfiguration) -> Op
 async fn load_session_state<Store: SessionStore>(
     session_key: Option<SessionKey>,
     storage_backend: &Store,
-) -> Result<(Option<SessionKey>, HashMap<String, String>), actix_web::Error> {
+) -> Result<(Option<SessionKey>, SessionState), actix_web::Error> {
     if let Some(session_key) = session_key {
         match storage_backend.load(&session_key).await {
             Ok(state) => {
@@ -376,7 +376,7 @@ async fn load_session_state<Store: SessionStore>(
                         empty session."
                     );
 
-                    Ok((None, HashMap::new()))
+                    Ok((None, SessionState::new()))
                 }
             }
 
@@ -388,14 +388,14 @@ async fn load_session_state<Store: SessionStore>(
                         "Invalid session state, creating a new empty session."
                     );
 
-                    Ok((Some(session_key), HashMap::new()))
+                    Ok((Some(session_key), SessionState::new()))
                 }
 
                 LoadError::Other(err) => Err(e500(err)),
             },
         }
     } else {
-        Ok((None, HashMap::new()))
+        Ok((None, SessionState::new()))
     }
 }
 

actix-session/src/session.rs

@@ -1,6 +1,5 @@
 use std::{
     cell::{Ref, RefCell},
-    collections::HashMap,
     error::Error as StdError,
     mem,
     rc::Rc,
@@ -16,6 +15,9 @@ use actix_web::{
 use anyhow::Context;
 use derive_more::derive::{Display, From};
 use serde::{de::DeserializeOwned, Serialize};
+use serde_json::Value;
+
+use crate::storage::SessionState;
 
 /// The primary interface to access and modify session state.
 ///
@@ -70,7 +72,7 @@ pub enum SessionStatus {
 
 #[derive(Default)]
 struct SessionInner {
-    state: HashMap<String, String>,
+    state: SessionState,
     status: SessionStatus,
 }
 
@@ -79,9 +81,9 @@ impl Session {
     ///
     /// It returns an error if it fails to deserialize as `T` the JSON value associated with `key`.
     pub fn get<T: DeserializeOwned>(&self, key: &str) -> Result<Option<T>, SessionGetError> {
-        if let Some(val_str) = self.0.borrow().state.get(key) {
+        if let Some(value) = self.0.borrow().state.get(key) {
             Ok(Some(
-                serde_json::from_str(val_str)
+                serde_json::from_value(value.to_owned())
                     .with_context(|| {
                         format!(
                             "Failed to deserialize the JSON-encoded session data attached to key \
@@ -100,7 +102,7 @@ impl Session {
     /// Get all raw key-value data from the session.
     ///
     /// Note that values are JSON encoded.
-    pub fn entries(&self) -> Ref<'_, HashMap<String, String>> {
+    pub fn entries(&self) -> Ref<'_, SessionState> {
         Ref::map(self.0.borrow(), |inner| &inner.state)
     }
 
@@ -139,7 +141,7 @@ impl Session {
                 })
                 .map_err(SessionInsertError)?;
 
-            inner.state.insert(key, val);
+            inner.state.insert(key, Value::String(val));
         }
 
         Ok(())
@@ -148,7 +150,7 @@ impl Session {
     /// Remove value from the session.
     ///
     /// If present, the JSON encoded value is returned.
-    pub fn remove(&self, key: &str) -> Option<String> {
+    pub fn remove(&self, key: &str) -> Option<Value> {
         let mut inner = self.0.borrow_mut();
 
         if inner.status != SessionStatus::Purged {
@@ -165,9 +167,9 @@ impl Session {
     ///
     /// Returns `None` if key was not present in session. Returns `T` if deserialization succeeds,
     /// otherwise returns un-deserialized JSON string.
-    pub fn remove_as<T: DeserializeOwned>(&self, key: &str) -> Option<Result<T, String>> {
+    pub fn remove_as<T: DeserializeOwned>(&self, key: &str) -> Option<Result<T, Value>> {
         self.remove(key)
-            .map(|val_str| match serde_json::from_str(&val_str) {
+            .map(|val| match serde_json::from_value(val.clone()) {
                 Ok(val) => Ok(val),
                 Err(_err) => {
                     tracing::debug!(
@@ -176,7 +178,7 @@ impl Session {
                         std::any::type_name::<T>()
                     );
 
-                    Err(val_str)
+                    Err(val)
                 }
             })
     }
@@ -216,11 +218,13 @@ impl Session {
     #[allow(clippy::needless_pass_by_ref_mut)]
     pub(crate) fn set_session(
         req: &mut ServiceRequest,
-        data: impl IntoIterator<Item = (String, String)>,
+        data: impl IntoIterator<Item = (String, impl Into<Value>)>,
     ) {
         let session = Session::get_session(&mut req.extensions_mut());
         let mut inner = session.0.borrow_mut();
-        inner.state.extend(data);
+        inner
+            .state
+            .extend(data.into_iter().map(|(k, v)| (k, v.into())));
     }
 
     /// Returns session status and iterator of key-value pairs of changes.
@@ -229,9 +233,7 @@ impl Session {
     /// typemap, leaving behind a new empty map. It should only be used when the session is being
     /// finalised (i.e. in `SessionMiddleware`).
     #[allow(clippy::needless_pass_by_ref_mut)]
-    pub(crate) fn get_changes<B>(
-        res: &mut ServiceResponse<B>,
-    ) -> (SessionStatus, HashMap<String, String>) {
+    pub(crate) fn get_changes<B>(res: &mut ServiceResponse<B>) -> (SessionStatus, SessionState) {
         if let Some(s_impl) = res
             .request()
             .extensions()
@@ -240,7 +242,7 @@ impl Session {
             let state = mem::take(&mut s_impl.borrow_mut().state);
             (s_impl.borrow().status.clone(), state)
         } else {
-            (SessionStatus::Unchanged, HashMap::new())
+            (SessionStatus::Unchanged, SessionState::new())
         }
     }
 

actix-session/src/storage/interface.rs

@@ -1,11 +1,13 @@
-use std::{collections::HashMap, future::Future};
+use std::future::Future;
 
 use actix_web::cookie::time::Duration;
 use derive_more::derive::Display;
+use serde_json::{Map, Value};
 
 use super::SessionKey;
 
-pub(crate) type SessionState = HashMap<String, String>;
+/// Convenience type for the map structure backing session state.
+pub type SessionState = Map<String, Value>;
 
 /// The interface to retrieve and save the current session data from/to the chosen storage backend.
 ///

actix-session/src/storage/mod.rs

@@ -13,7 +13,7 @@ pub use self::cookie::CookieSessionStore;
 #[cfg(feature = "redis-session")]
 pub use self::redis_rs::{RedisSessionStore, RedisSessionStoreBuilder};
 pub use self::{
-    interface::{LoadError, SaveError, SessionStore, UpdateError},
+    interface::{LoadError, SaveError, SessionState, SessionStore, UpdateError},
     session_key::SessionKey,
     utils::generate_session_key,
 };