diff --git a/actix-session/CHANGES.md b/actix-session/CHANGES.md index aaafad650..ba4a99160 100644 --- a/actix-session/CHANGES.md +++ b/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`. diff --git a/actix-session/src/middleware.rs b/actix-session/src/middleware.rs index f1e791f0d..fa14ef260 100644 --- a/actix-session/src/middleware.rs +++ b/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(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)] diff --git a/actix-session/tests/opaque_errors.rs b/actix-session/tests/opaque_errors.rs new file mode 100644 index 000000000..cb8e9164c --- /dev/null +++ b/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>, LoadError> { + Err(LoadError::Other(anyhow::anyhow!( + "My error full of implementation details" + ))) + } + + async fn save( + &self, + _session_state: HashMap, + _ttl: &Duration, + ) -> Result { + Ok("random_value".to_string().try_into().unwrap()) + } + + async fn update( + &self, + _session_key: SessionKey, + _session_state: HashMap, + _ttl: &Duration, + ) -> Result { + 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" +}