From ef9dc5d0b6f727cc78ab4df214e6e65ec77c3b9e Mon Sep 17 00:00:00 2001 From: Pawan Hegde Date: Tue, 11 Jul 2023 02:14:14 +0530 Subject: [PATCH] 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 * 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 for LemmyError * with_lemmy_type * trigger ci --------- Co-authored-by: SleeplessOne1917 * 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 Co-authored-by: Dessalines Co-authored-by: rosenjcb Co-authored-by: nixoye <12674582+nixoye@users.noreply.github.com> Co-authored-by: dullbananas Co-authored-by: Nutomic Co-authored-by: SleeplessOne1917 Co-authored-by: Sander Saarend Co-authored-by: Piotr Juszczyk <74842304+pijuszczyk@users.noreply.github.com> --- crates/api_crud/src/site/create.rs | 2 +- crates/api_crud/src/site/update.rs | 2 +- crates/utils/src/error.rs | 27 +++--- crates/utils/src/lib.rs | 1 + crates/utils/src/response.rs | 127 +++++++++++++++++++++++++++ crates/utils/src/utils/validation.rs | 14 +-- src/lib.rs | 10 ++- 7 files changed, 153 insertions(+), 30 deletions(-) create mode 100644 crates/utils/src/response.rs diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index be8411e24..ed433ad9d 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 1e354d9cd..d1820d177 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 cdb484722..78590a6a7 100644 --- a/crates/utils/src/error.rs +++ b/crates/utils/src/error.rs @@ -10,7 +10,7 @@ use ts_rs::TS; pub type LemmyResult = Result; pub struct LemmyError { - pub error_type: Option, + pub error_type: LemmyErrorType, pub inner: anyhow::Error, pub context: SpanTrace, } @@ -20,9 +20,10 @@ where T: Into, { 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 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> { impl> LemmyErrorExt for Result { fn with_lemmy_type(self, error_type: LemmyErrorType) -> Result { 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 { impl LemmyErrorExt2 for Result { fn with_lemmy_type(self, error_type: LemmyErrorType) -> Result { 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 2c71c58d0..9ca427cf9 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 000000000..cc0b7c707 --- /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( + res: ServiceResponse, +) -> actix_web::Result> { + 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::() + .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 { + 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 { + 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 { + 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 { + 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 ec2d20b97..b42fe1add 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 b6fd64ec8..9e081376f 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::::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()));