diff --git a/actix-web/src/error/error.rs b/actix-web/src/error/error.rs index 670a58a00..a75ef924d 100644 --- a/actix-web/src/error/error.rs +++ b/actix-web/src/error/error.rs @@ -6,7 +6,7 @@ use crate::{HttpResponse, ResponseError}; /// General purpose Actix Web error. /// -/// An Actix Web error is used to carry errors from `std::error` through actix in a convenient way. +/// An Actix Web error is used to carry errors from `std::error` through Actix in a convenient way. /// It can be created through converting errors with `into()`. /// /// Whenever it is created from an external object a response error is created for it that can be @@ -14,6 +14,7 @@ use crate::{HttpResponse, ResponseError}; /// you can always get a `ResponseError` reference from it. pub struct Error { cause: Box, + response_mappers: Vec HttpResponse>>, } impl Error { @@ -29,7 +30,67 @@ impl Error { /// Shortcut for creating an `HttpResponse`. pub fn error_response(&self) -> HttpResponse { - self.cause.error_response() + let mut res = self.cause.error_response(); + + for mapper in &self.response_mappers { + res = (mapper)(res); + } + + res + } + + /// Adds a function that maps the HTTP response generated for this error. + /// + /// Mappers are called in the order they are added each time + /// [`error_response`](Self::error_response) is called. A mapper may receive a response already + /// modified by other mappers, so it should avoid relying on a particular position in the chain. + /// + /// Prefer narrowly mutating the provided response. Preserve fields the mapper does not own, + /// insert default headers only when absent, and merge list-valued headers without introducing + /// duplicates. Mappers should also be deterministic and safe to call more than once. + /// + /// # Good + /// + /// This mapper preserves the error response and adds a default only when it is absent: + /// + /// ``` + /// use actix_web::{ + /// error, + /// http::header::{self, HeaderValue}, + /// }; + /// + /// let mut err = error::ErrorBadRequest("bad request"); + /// err.add_response_mapper(|mut res| { + /// if !res.headers().contains_key(header::CACHE_CONTROL) { + /// res.headers_mut() + /// .insert(header::CACHE_CONTROL, HeaderValue::from_static("no-store")); + /// } + /// + /// res + /// }); + /// + /// assert_eq!( + /// err.error_response().headers().get(header::CACHE_CONTROL), + /// Some(&HeaderValue::from_static("no-store")), + /// ); + /// ``` + /// + /// # Bad + /// + /// Replacing the response discards the original status, body, headers, extensions, and any + /// changes made by earlier mappers: + /// + /// ``` + /// use actix_web::{error, HttpResponse}; + /// + /// let mut err = error::ErrorBadRequest("bad request"); + /// err.add_response_mapper(|_| HttpResponse::InternalServerError().finish()); + /// ``` + pub fn add_response_mapper(&mut self, mapper: F) + where + F: Fn(HttpResponse) -> HttpResponse + 'static, + { + self.response_mappers.push(Box::new(mapper)) } } @@ -56,13 +117,17 @@ impl From for Error { fn from(err: T) -> Error { Error { cause: Box::new(err), + response_mappers: Vec::new(), } } } impl From> for Error { fn from(value: Box) -> Self { - Error { cause: value } + Error { + cause: value, + response_mappers: Vec::new(), + } } } diff --git a/actix-web/tests/test_error_propagation.rs b/actix-web/tests/test_error_propagation.rs index 958276b62..c546ade56 100644 --- a/actix-web/tests/test_error_propagation.rs +++ b/actix-web/tests/test_error_propagation.rs @@ -4,8 +4,10 @@ use actix_utils::future::{ok, Ready}; use actix_web::{ dev::{forward_ready, Service, ServiceRequest, ServiceResponse, Transform}, get, + http::{header, StatusCode}, + middleware::{from_fn, Next}, test::{call_service, init_service, TestRequest}, - ResponseError, + web, App, Error, HttpResponse, ResponseError, }; use futures_core::future::LocalBoxFuture; use futures_util::lock::Mutex; @@ -97,3 +99,70 @@ async fn error_cause_should_be_propagated_to_middlewares() { let was_error_captured = lock.lock().await.unwrap(); assert!(was_error_captured); } + +async fn inner_error_middleware( + req: ServiceRequest, + next: Next, +) -> Result { + next.call(req).await?; + Err(actix_web::error::ErrorBadRequest("inner middleware error")) +} + +async fn cors_like_middleware( + req: ServiceRequest, + next: Next, +) -> Result, Error> { + let origin = req.headers().get(header::ORIGIN).cloned(); + + match next.call(req).await { + Ok(mut res) => { + if let Some(origin) = origin { + res.headers_mut() + .insert(header::ACCESS_CONTROL_ALLOW_ORIGIN, origin); + } + + Ok(res) + } + Err(mut err) => { + if let Some(origin) = origin { + err.add_response_mapper(move |mut res| { + res.headers_mut() + .insert(header::ACCESS_CONTROL_ALLOW_ORIGIN, origin.clone()); + res + }); + } + + Err(err) + } + } +} + +#[actix_rt::test] +async fn error_response_mapper_allows_cors_like_middleware_to_augment_errors() { + let srv = actix_test::start(|| { + App::new() + // Emulate an inner middleware that propagates an error rather than constructing a + // response. This currently prevents response middleware such as actix-cors from + // augmenting the eventual error response. + .wrap(from_fn(inner_error_middleware)) + // Emulate actix-cors as the outer middleware. Successful responses are augmented + // directly; errors carry a mapper that applies the same header during conversion. + .wrap(from_fn(cors_like_middleware)) + .route("/", web::get().to(HttpResponse::Ok)) + }); + + let res = srv + .get("/") + .insert_header((header::ORIGIN, "https://example.com")) + .send() + .await + .unwrap(); + + assert_eq!(res.status(), StatusCode::BAD_REQUEST); + assert_eq!( + res.headers().get(header::ACCESS_CONTROL_ALLOW_ORIGIN), + Some(&header::HeaderValue::from_static("https://example.com")), + ); + + srv.stop().await; +}