From: Pawan Hegde <pahegde@linkedin.com>
Date: Mon, 10 Jul 2023 20:44:14 +0000 (+0530)
Subject: Fix #3366: Wrap plain-text error responses from the API in JSON (#3559)
X-Git-Url: http://these/git/%22https:/lemmy.ml/u/Liwott/static/%7B?a=commitdiff_plain;h=ef9dc5d0b6f727cc78ab4df214e6e65ec77c3b9e;p=lemmy.git

Fix #3366: Wrap plain-text error responses from the API in JSON (#3559)

* Fix #3366: API does return plain HTML errors

* Fix Clippy errors

* Improve api response times by doing send_activity asynchronously (#3493)

* do send_activity after http response

* move to util function

* format

* fix prometheus

* make synchronous federation configurable

* cargo fmt

* empty

* empty

---------

Co-authored-by: Dessalines <dessalines@users.noreply.github.com>

* Updating `login.rs` with generic `incorrect_login` response. (#3549)

* Adding v0.18.1 and v0.18.0 release notes. (#3530)

* Update RELEASES.md (#3556)

added instruction to find the location of your docker directory (especially useful for those who used ansible since they never had to setup docker manually)

* Use async email sender (#3554)

* Upgrade all dependencies (#3526)

* Upgrade all dependencies

* as base64

* Adding phiresky to codeowners. (#3576)

* Error enum fixed (#3487)

* Create error type enum

* Replace magic string slices with LemmyErrorTypes

* Remove unused enum

* Add rename snake case to error enum

* Rename functions

* clippy

* Fix merge errors

* Serialize in PascalCase instead of snake_case

* Revert src/lib

* Add serialization tests

* Update translations

* Fix compilation error in test

* Fix another compilation error

* Add code for generating typescript types

* Various fixes to avoid breaking api

* impl From<LemmyErrorType> for LemmyError

* with_lemmy_type

* trigger ci

---------

Co-authored-by: SleeplessOne1917 <abias1122@gmail.com>

* Only update site_aggregates for local site (#3516)

* Fix #3501 - Fix aggregation counts for elements removed and deleted (#3543)

Two bugs were found and fixed:
- previously elements removal and deletion were counted as two separate disappearances
- removing comments did not affect post aggregations

* Use LemmyErrorType also make error_type compulsory

* Add missing import for jsonify_plain_text_errors

* Fix formatting

* Trying to make woodpecker run again

---------

Co-authored-by: phiresky <phireskyde+git@gmail.com>
Co-authored-by: Dessalines <dessalines@users.noreply.github.com>
Co-authored-by: rosenjcb <rosenjcb@gmail.com>
Co-authored-by: nixoye <12674582+nixoye@users.noreply.github.com>
Co-authored-by: dullbananas <dull.bananas0@gmail.com>
Co-authored-by: Nutomic <me@nutomic.com>
Co-authored-by: SleeplessOne1917 <abias1122@gmail.com>
Co-authored-by: Sander Saarend <sander@saarend.com>
Co-authored-by: Piotr Juszczyk <74842304+pijuszczyk@users.noreply.github.com>
---

diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs
index be8411e2..ed433ad9 100644
--- a/crates/api_crud/src/site/create.rs
+++ b/crates/api_crud/src/site/create.rs
@@ -371,7 +371,7 @@ mod tests {
           }
           Err(error) => {
             assert!(
-              error.error_type.eq(&Some(expected_err.clone())),
+              error.error_type.eq(&expected_err.clone()),
               "Got Err {:?}, but should have failed with message: {} for reason: {}. invalid_payloads.nth({})",
               error.error_type,
               expected_err,
diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs
index 1e354d9c..d1820d17 100644
--- a/crates/api_crud/src/site/update.rs
+++ b/crates/api_crud/src/site/update.rs
@@ -373,7 +373,7 @@ mod tests {
           }
           Err(error) => {
             assert!(
-              error.error_type.eq(&Some(expected_err.clone())),
+              error.error_type.eq(&expected_err.clone()),
               "Got Err {:?}, but should have failed with message: {} for reason: {}. invalid_payloads.nth({})",
               error.error_type,
               expected_err,
diff --git a/crates/utils/src/error.rs b/crates/utils/src/error.rs
index cdb48472..78590a6a 100644
--- a/crates/utils/src/error.rs
+++ b/crates/utils/src/error.rs
@@ -10,7 +10,7 @@ use ts_rs::TS;
 pub type LemmyResult<T> = Result<T, LemmyError>;
 
 pub struct LemmyError {
-  pub error_type: Option<LemmyErrorType>,
+  pub error_type: LemmyErrorType,
   pub inner: anyhow::Error,
   pub context: SpanTrace,
 }
@@ -20,9 +20,10 @@ where
   T: Into<anyhow::Error>,
 {
   fn from(t: T) -> Self {
+    let cause = t.into();
     LemmyError {
-      error_type: None,
-      inner: t.into(),
+      error_type: LemmyErrorType::Unknown(format!("{}", &cause)),
+      inner: cause,
       context: SpanTrace::capture(),
     }
   }
@@ -40,9 +41,7 @@ impl Debug for LemmyError {
 
 impl Display for LemmyError {
   fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-    if let Some(message) = &self.error_type {
-      write!(f, "{message}: ")?;
-    }
+    write!(f, "{}: ", &self.error_type)?;
     // print anyhow including trace
     // https://docs.rs/anyhow/latest/anyhow/struct.Error.html#display-representations
     // this will print the anyhow trace (only if it exists)
@@ -61,13 +60,7 @@ impl actix_web::error::ResponseError for LemmyError {
   }
 
   fn error_response(&self) -> actix_web::HttpResponse {
-    if let Some(message) = &self.error_type {
-      actix_web::HttpResponse::build(self.status_code()).json(message)
-    } else {
-      actix_web::HttpResponse::build(self.status_code())
-        .content_type("text/plain")
-        .body(self.inner.to_string())
-    }
+    actix_web::HttpResponse::build(self.status_code()).json(&self.error_type)
   }
 }
 
@@ -214,14 +207,14 @@ pub enum LemmyErrorType {
   CouldntCreateAudioCaptcha,
   InvalidUrlScheme,
   CouldntSendWebmention,
-  Unknown,
+  Unknown(String),
 }
 
 impl From<LemmyErrorType> for LemmyError {
   fn from(error_type: LemmyErrorType) -> Self {
     let inner = anyhow::anyhow!("{}", error_type);
     LemmyError {
-      error_type: Some(error_type),
+      error_type,
       inner,
       context: SpanTrace::capture(),
     }
@@ -235,7 +228,7 @@ pub trait LemmyErrorExt<T, E: Into<anyhow::Error>> {
 impl<T, E: Into<anyhow::Error>> LemmyErrorExt<T, E> for Result<T, E> {
   fn with_lemmy_type(self, error_type: LemmyErrorType) -> Result<T, LemmyError> {
     self.map_err(|error| LemmyError {
-      error_type: Some(error_type),
+      error_type,
       inner: error.into(),
       context: SpanTrace::capture(),
     })
@@ -248,7 +241,7 @@ pub trait LemmyErrorExt2<T> {
 impl<T> LemmyErrorExt2<T> for Result<T, LemmyError> {
   fn with_lemmy_type(self, error_type: LemmyErrorType) -> Result<T, LemmyError> {
     self.map_err(|mut e| {
-      e.error_type = Some(error_type);
+      e.error_type = error_type;
       e
     })
   }
diff --git a/crates/utils/src/lib.rs b/crates/utils/src/lib.rs
index 2c71c58d..9ca427cf 100644
--- a/crates/utils/src/lib.rs
+++ b/crates/utils/src/lib.rs
@@ -11,6 +11,7 @@ pub mod settings;
 pub mod claims;
 pub mod error;
 pub mod request;
+pub mod response;
 pub mod utils;
 pub mod version;
 
diff --git a/crates/utils/src/response.rs b/crates/utils/src/response.rs
new file mode 100644
index 00000000..cc0b7c70
--- /dev/null
+++ b/crates/utils/src/response.rs
@@ -0,0 +1,127 @@
+use crate::error::{LemmyError, LemmyErrorType};
+use actix_web::{
+  dev::ServiceResponse,
+  http::header,
+  middleware::ErrorHandlerResponse,
+  HttpResponse,
+};
+
+pub fn jsonify_plain_text_errors<BODY>(
+  res: ServiceResponse<BODY>,
+) -> actix_web::Result<ErrorHandlerResponse<BODY>> {
+  let maybe_error = res.response().error();
+
+  // This function is only expected to be called for errors, so if there is no error, return
+  if maybe_error.is_none() {
+    return Ok(ErrorHandlerResponse::Response(res.map_into_left_body()));
+  }
+  // We're assuming that any LemmyError is already in JSON format, so we don't need to do anything
+  if maybe_error
+    .expect("http responses with 400-599 statuses should have an error object")
+    .as_error::<LemmyError>()
+    .is_some()
+  {
+    return Ok(ErrorHandlerResponse::Response(res.map_into_left_body()));
+  }
+
+  let (req, res) = res.into_parts();
+  let error = res
+    .error()
+    .expect("expected an error object in the response");
+  let response = HttpResponse::build(res.status())
+    .append_header(header::ContentType::json())
+    .json(LemmyErrorType::Unknown(error.to_string()));
+
+  let service_response = ServiceResponse::new(req, response);
+  Ok(ErrorHandlerResponse::Response(
+    service_response.map_into_right_body(),
+  ))
+}
+
+#[cfg(test)]
+mod tests {
+  use super::*;
+  use crate::error::{LemmyError, LemmyErrorType};
+  use actix_web::{
+    error::ErrorInternalServerError,
+    middleware::ErrorHandlers,
+    test,
+    web,
+    App,
+    Error,
+    Handler,
+    Responder,
+  };
+  use http::StatusCode;
+
+  #[actix_web::test]
+  async fn test_non_error_responses_are_not_modified() {
+    async fn ok_service() -> actix_web::Result<String, Error> {
+      Ok("Oll Korrect".to_string())
+    }
+
+    check_for_jsonification(ok_service, StatusCode::OK, "Oll Korrect").await;
+  }
+
+  #[actix_web::test]
+  async fn test_lemmy_errors_are_not_modified() {
+    async fn lemmy_error_service() -> actix_web::Result<String, LemmyError> {
+      Err(LemmyError::from(LemmyErrorType::EmailAlreadyExists))
+    }
+
+    check_for_jsonification(
+      lemmy_error_service,
+      StatusCode::BAD_REQUEST,
+      "{\"error\":\"email_already_exists\"}",
+    )
+    .await;
+  }
+
+  #[actix_web::test]
+  async fn test_generic_errors_are_jsonified_as_unknown_errors() {
+    async fn generic_error_service() -> actix_web::Result<String, Error> {
+      Err(ErrorInternalServerError("This is not a LemmyError"))
+    }
+
+    check_for_jsonification(
+      generic_error_service,
+      StatusCode::INTERNAL_SERVER_ERROR,
+      "{\"error\":\"unknown\",\"message\":\"This is not a LemmyError\"}",
+    )
+    .await;
+  }
+
+  #[actix_web::test]
+  async fn test_anyhow_errors_wrapped_in_lemmy_errors_are_jsonified_correctly() {
+    async fn anyhow_error_service() -> actix_web::Result<String, LemmyError> {
+      Err(LemmyError::from(anyhow::anyhow!("This is the inner error")))
+    }
+
+    check_for_jsonification(
+      anyhow_error_service,
+      StatusCode::BAD_REQUEST,
+      "{\"error\":\"unknown\",\"message\":\"This is the inner error\"}",
+    )
+    .await;
+  }
+
+  async fn check_for_jsonification(
+    service: impl Handler<(), Output = impl Responder + 'static>,
+    expected_status_code: StatusCode,
+    expected_body: &str,
+  ) {
+    let app = test::init_service(
+      App::new()
+        .wrap(ErrorHandlers::new().default_handler(jsonify_plain_text_errors))
+        .route("/", web::get().to(service)),
+    )
+    .await;
+    let req = test::TestRequest::default().to_request();
+    let res = test::call_service(&app, req).await;
+
+    assert_eq!(res.status(), expected_status_code);
+
+    let body = test::read_body(res).await;
+    assert_eq!(body, expected_body);
+  }
+}
diff --git a/crates/utils/src/utils/validation.rs b/crates/utils/src/utils/validation.rs
index ec2d20b9..b42fe1ad 100644
--- a/crates/utils/src/utils/validation.rs
+++ b/crates/utils/src/utils/validation.rs
@@ -431,10 +431,7 @@ mod tests {
 
         assert!(result.is_err());
         assert!(
-          result
-            .unwrap_err()
-            .error_type
-            .eq(&Some(expected_err.clone())),
+          result.unwrap_err().error_type.eq(&expected_err.clone()),
           "Testing {}, expected error {}",
           invalid_name,
           expected_err
@@ -454,7 +451,7 @@ mod tests {
         && invalid_result
           .unwrap_err()
           .error_type
-          .eq(&Some(LemmyErrorType::BioLengthOverflow))
+          .eq(&LemmyErrorType::BioLengthOverflow)
     );
   }
 
@@ -478,7 +475,7 @@ mod tests {
         && invalid_result
           .unwrap_err()
           .error_type
-          .eq(&Some(LemmyErrorType::SiteDescriptionLengthOverflow))
+          .eq(&LemmyErrorType::SiteDescriptionLengthOverflow)
     );
   }
 
@@ -508,10 +505,7 @@ mod tests {
 
         assert!(result.is_err());
         assert!(
-          result
-            .unwrap_err()
-            .error_type
-            .eq(&Some(expected_err.clone())),
+          result.unwrap_err().error_type.eq(&expected_err.clone()),
           "Testing regex {:?}, expected error {}",
           regex_str,
           expected_err
diff --git a/src/lib.rs b/src/lib.rs
index b6fd64ec..9e081376 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -10,7 +10,13 @@ pub mod telemetry;
 use crate::{code_migrations::run_advanced_migrations, root_span_builder::QuieterRootSpanBuilder};
 use activitypub_federation::config::{FederationConfig, FederationMiddleware};
 use actix_cors::Cors;
-use actix_web::{middleware, web::Data, App, HttpServer, Result};
+use actix_web::{
+  middleware::{self, ErrorHandlers},
+  web::Data,
+  App,
+  HttpServer,
+  Result,
+};
 use lemmy_api_common::{
   context::LemmyContext,
   lemmy_db_views::structs::SiteView,
@@ -29,6 +35,7 @@ use lemmy_routes::{feeds, images, nodeinfo, webfinger};
 use lemmy_utils::{
   error::LemmyError,
   rate_limit::RateLimitCell,
+  response::jsonify_plain_text_errors,
   settings::SETTINGS,
   SYNCHRONOUS_FEDERATION,
 };
@@ -181,6 +188,7 @@ pub async fn start_lemmy_server() -> Result<(), LemmyError> {
       .wrap(middleware::Compress::default())
       .wrap(cors_config)
       .wrap(TracingLogger::<QuieterRootSpanBuilder>::new())
+      .wrap(ErrorHandlers::new().default_handler(jsonify_plain_text_errors))
       .app_data(Data::new(context.clone()))
       .app_data(Data::new(rate_limit_cell.clone()))
       .wrap(FederationMiddleware::new(federation_config.clone()));