[actix-session] Opaque 500s (#236)

actix-session/CHANGES.md

@@ -1,7 +1,10 @@
 # Changes
 
 ## Unreleased - 2021-xx-xx
+### Fixed
+- Do not leak internal implementation details to callers when errors occur. [#236]
  
+[#236]: https://github.com/actix/actix-extras/pull/236
 
 ## 0.6.1 - 2022-03-21
 - No significant changes since `0.6.0`.

actix-session/src/middleware.rs

@@ -6,6 +6,7 @@ use actix_web::{
     cookie::{Cookie, CookieJar, Key, SameSite},
     dev::{forward_ready, ResponseHead, Service, ServiceRequest, ServiceResponse, Transform},
     http::header::{HeaderValue, SET_COOKIE},
+    HttpResponse,
 };
 use anyhow::Context;
 use time::Duration;
@@ -393,7 +394,17 @@ where
 /// Short-hand to create an `actix_web::Error` instance that will result in an `Internal Server
 /// Error` response while preserving the error root cause (e.g. in logs).
 fn e500<E: fmt::Debug + fmt::Display + 'static>(err: E) -> actix_web::Error {
-    actix_web::error::ErrorInternalServerError(err)
+    // We do not use `actix_web::error::ErrorInternalServerError` because we do not want to
+    // leak internal implementation details to the caller.
+    //
+    // `actix_web::error::ErrorInternalServerError` includes the error Display representation
+    // as body of the error responses, leading to messages like "There was an issue persisting
+    // the session state" reaching API clients. We don't want that, we want opaque 500s.
+    actix_web::error::InternalError::from_response(
+        err,
+        HttpResponse::InternalServerError().finish(),
+    )
+    .into()
 }
 
 #[doc(hidden)]

actix-session/tests/opaque_errors.rs

@@ -0,0 +1,85 @@
+use actix_session::storage::{LoadError, SaveError, SessionKey, SessionStore, UpdateError};
+use actix_session::{Session, SessionMiddleware};
+use actix_web::body::MessageBody;
+use actix_web::http::StatusCode;
+use actix_web::{
+    cookie::{time::Duration, Key},
+    dev::Service,
+    test, web, App, Responder,
+};
+use anyhow::Error;
+use std::collections::HashMap;
+use std::convert::TryInto;
+
+#[actix_web::test]
+async fn errors_are_opaque() {
+    let signing_key = Key::generate();
+    let app = test::init_service(
+        App::new()
+            .wrap(SessionMiddleware::new(MockStore, signing_key.clone()))
+            .route("/create_session", web::post().to(create_session))
+            .route(
+                "/load_session_with_error",
+                web::post().to(load_session_with_error),
+            ),
+    )
+    .await;
+
+    let req = test::TestRequest::post()
+        .uri("/create_session")
+        .to_request();
+    let response = test::call_service(&app, req).await;
+    let session_cookie = response.response().cookies().next().unwrap();
+
+    let req = test::TestRequest::post()
+        .cookie(session_cookie)
+        .uri("/load_session_with_error")
+        .to_request();
+    let response = app.call(req).await.unwrap_err().error_response();
+    assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR);
+    assert!(response.into_body().try_into_bytes().unwrap().is_empty());
+}
+
+struct MockStore;
+
+#[async_trait::async_trait(?Send)]
+impl SessionStore for MockStore {
+    async fn load(
+        &self,
+        _session_key: &SessionKey,
+    ) -> Result<Option<HashMap<String, String>>, LoadError> {
+        Err(LoadError::Other(anyhow::anyhow!(
+            "My error full of implementation details"
+        )))
+    }
+
+    async fn save(
+        &self,
+        _session_state: HashMap<String, String>,
+        _ttl: &Duration,
+    ) -> Result<SessionKey, SaveError> {
+        Ok("random_value".to_string().try_into().unwrap())
+    }
+
+    async fn update(
+        &self,
+        _session_key: SessionKey,
+        _session_state: HashMap<String, String>,
+        _ttl: &Duration,
+    ) -> Result<SessionKey, UpdateError> {
+        todo!()
+    }
+
+    async fn delete(&self, _session_key: &SessionKey) -> Result<(), Error> {
+        todo!()
+    }
+}
+
+async fn create_session(session: Session) -> impl Responder {
+    session.insert("user_id", "id").unwrap();
+    "Created"
+}
+
+async fn load_session_with_error(_session: Session) -> impl Responder {
+    "Loaded"
+}