Merge b516280981 into eee23e2326

actix-web/CHANGES.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+- Add `Error::add_response_mapper()` to allow middleware to modify error-generated responses before they are sent. [#2979]
+
+[#2979]: https://github.com/actix/actix-web/pull/2979
+
 ## 4.14.0
 
 - Add `HttpRequest::{cookies_raw,cookie_raw}` and `ServiceRequest::{cookies_raw,cookie_raw}` for reading request cookies without percent-decoding names and values. [#3542]

actix-web/README.md

@@ -41,13 +41,9 @@
 
 ### Experimental features
 
-To enable faster release iterations, we mark some features as experimental.
-These features are prefixed with `experimental` and a breaking change may happen at any release.
-Please use them in a production environment at your own risk.
+To enable faster release iterations, we mark some features as experimental. These features are prefixed with `experimental` and a breaking change may happen at any release. Please use them in a production environment at your own risk.
 
-- `experimental-introspection`: exposes route and method reporting helpers for local diagnostics
-  and tooling. See [`examples/introspection.rs`](examples/introspection.rs) and
-  [`examples/introspection_multi_servers.rs`](examples/introspection_multi_servers.rs).
+- `experimental-introspection`: exposes route and method reporting helpers for local diagnostics and tooling. See [`examples/introspection.rs`](examples/introspection.rs) and [`examples/introspection_multi_servers.rs`](examples/introspection_multi_servers.rs).
 
 ## Documentation
 

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<dyn ResponseError>,
+    response_mappers: Vec<Box<dyn Fn(HttpResponse) -> 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<F>(&mut self, mapper: F)
+    where
+        F: Fn(HttpResponse) -> HttpResponse + 'static,
+    {
+        self.response_mappers.push(Box::new(mapper))
     }
 }
 
@@ -56,13 +117,17 @@ impl<T: ResponseError + 'static> From<T> for Error {
     fn from(err: T) -> Error {
         Error {
             cause: Box::new(err),
+            response_mappers: Vec::new(),
         }
     }
 }
 
 impl From<Box<dyn ResponseError>> for Error {
     fn from(value: Box<dyn ResponseError>) -> Self {
-        Error { cause: value }
+        Error {
+            cause: value,
+            response_mappers: Vec::new(),
+        }
     }
 }
 

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<B>(
+    req: ServiceRequest,
+    next: Next<B>,
+) -> Result<ServiceResponse, Error> {
+    next.call(req).await?;
+    Err(actix_web::error::ErrorBadRequest("inner middleware error"))
+}
+
+async fn cors_like_middleware<B>(
+    req: ServiceRequest,
+    next: Next<B>,
+) -> Result<ServiceResponse<B>, 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;
+}