From 437fe5b931e5f1528c8456f972222100e661c798 Mon Sep 17 00:00:00 2001 From: David Pedersen Date: Fri, 18 Mar 2022 16:53:39 +0100 Subject: [PATCH] Make status codes for `JsonRejection` more precise (#868) * Fix status codes for `JsonRejection` rejections Fixes #865 * Apply suggestions from code review Co-authored-by: Jonas Platte Co-authored-by: Jonas Platte --- axum/CHANGELOG.md | 4 ++ axum/src/docs/extract.md | 47 ++++++++++++------- axum/src/extract/rejection.rs | 19 +++++++- axum/src/json.rs | 37 ++++++++++++++- .../customize-extractor-error/src/main.rs | 2 +- 5 files changed, 88 insertions(+), 21 deletions(-) diff --git a/axum/CHANGELOG.md b/axum/CHANGELOG.md index 192a8d89..77064e03 100644 --- a/axum/CHANGELOG.md +++ b/axum/CHANGELOG.md @@ -71,6 +71,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 be accepted but most likely result in bugs ([#823]) - **breaking:** `Headers` has been removed. Arrays of tuples directly implement `IntoResponseParts` so `([("x-foo", "foo")], response)` now works ([#797]) +- **breaking:** `InvalidJsonBody` has been replaced with `JsonDataError` to clearly signal that the + request body was syntactically valid JSON but couldn't be deserialized into the target type +- **changed:** New `JsonSyntaxError` variant added to `JsonRejection`. This is returned when the + request body contains syntactically invalid JSON - **fixed:** Set `Allow` header when responding with `405 Method Not Allowed` ([#733]) - **fixed:** Correctly set the `Content-Length` header for response to `HEAD` requests ([#734]) diff --git a/axum/src/docs/extract.md b/axum/src/docs/extract.md index edab8bb1..238273fe 100644 --- a/axum/src/docs/extract.md +++ b/axum/src/docs/extract.md @@ -209,9 +209,12 @@ async fn create_user(payload: Result, JsonRejection>) { // Request didn't have `Content-Type: application/json` // header } - Err(JsonRejection::InvalidJsonBody(_)) => { + Err(JsonRejection::JsonDataError(_)) => { // Couldn't deserialize the body into the target type } + Err(JsonRejection::JsonSyntaxError(_)) => { + // Syntax error in the body + } Err(JsonRejection::BytesRejection(_)) => { // Failed to extract the request body } @@ -249,7 +252,7 @@ breaking the public API. For example that means while [`Json`] is implemented using [`serde_json`] it doesn't directly expose the [`serde_json::Error`] thats contained in -[`JsonRejection::InvalidJsonBody`]. However it is still possible to access via +[`JsonRejection::JsonDataError`]. However it is still possible to access via methods from [`std::error::Error`]: ```rust @@ -267,21 +270,11 @@ async fn handler(result: Result, JsonRejection>) -> impl IntoRespons Ok(Json(payload)) => Ok(Json(json!({ "payload": payload }))), Err(err) => match err { - // attempt to extract the inner `serde_json::Error`, if that - // succeeds we can provide a more specific error - JsonRejection::InvalidJsonBody(err) => { - if let Some(serde_json_err) = find_error_source::(&err) { - Err(( - StatusCode::BAD_REQUEST, - format!( - "Invalid JSON at line {} column {}", - serde_json_err.line(), - serde_json_err.column() - ), - )) - } else { - Err((StatusCode::BAD_REQUEST, "Unknown error".to_string())) - } + JsonRejection::JsonDataError(err) => { + Err(serde_json_error_response(err)) + } + JsonRejection::JsonSyntaxError(err) => { + Err(serde_json_error_response(err)) } // handle other rejections from the `Json` extractor JsonRejection::MissingJsonContentType(_) => Err(( @@ -302,6 +295,26 @@ async fn handler(result: Result, JsonRejection>) -> impl IntoRespons } } +// attempt to extract the inner `serde_json::Error`, if that succeeds we can +// provide a more specific error +fn serde_json_error_response(err: E) -> (StatusCode, String) +where + E: Error + 'static, +{ + if let Some(serde_json_err) = find_error_source::(&err) { + ( + StatusCode::BAD_REQUEST, + format!( + "Invalid JSON at line {} column {}", + serde_json_err.line(), + serde_json_err.column() + ), + ) + } else { + (StatusCode::BAD_REQUEST, "Unknown error".to_string()) + } +} + // attempt to downcast `err` into a `T` and if that fails recursively try and // downcast `err`'s source fn find_error_source<'a, T>(err: &'a (dyn Error + 'static)) -> Option<&'a T> diff --git a/axum/src/extract/rejection.rs b/axum/src/extract/rejection.rs index 737a70bd..fc3e6a54 100644 --- a/axum/src/extract/rejection.rs +++ b/axum/src/extract/rejection.rs @@ -9,10 +9,24 @@ pub use axum_core::extract::rejection::*; #[cfg(feature = "json")] define_rejection! { #[status = UNPROCESSABLE_ENTITY] + #[body = "Failed to deserialize the JSON body into the target type"] + #[cfg_attr(docsrs, doc(cfg(feature = "json")))] + /// Rejection type for [`Json`](super::Json). + /// + /// This rejection is used if the request body is syntactically valid JSON but couldn't be + /// deserialized into the target type. + pub struct JsonDataError(Error); +} + +#[cfg(feature = "json")] +define_rejection! { + #[status = BAD_REQUEST] #[body = "Failed to parse the request body as JSON"] #[cfg_attr(docsrs, doc(cfg(feature = "json")))] /// Rejection type for [`Json`](super::Json). - pub struct InvalidJsonBody(Error); + /// + /// This rejection is used if the request body didn't contain syntactically valid JSON. + pub struct JsonSyntaxError(Error); } #[cfg(feature = "json")] @@ -141,7 +155,8 @@ composite_rejection! { /// can fail. #[cfg_attr(docsrs, doc(cfg(feature = "json")))] pub enum JsonRejection { - InvalidJsonBody, + JsonDataError, + JsonSyntaxError, MissingJsonContentType, BytesRejection, } diff --git a/axum/src/json.rs b/axum/src/json.rs index c1e6c8f9..9fcb53cb 100644 --- a/axum/src/json.rs +++ b/axum/src/json.rs @@ -99,7 +99,27 @@ where if json_content_type(req) { let bytes = Bytes::from_request(req).await?; - let value = serde_json::from_slice(&bytes).map_err(InvalidJsonBody::from_err)?; + let value = match serde_json::from_slice(&bytes) { + Ok(value) => value, + Err(err) => { + let rejection = match err.classify() { + serde_json::error::Category::Data => JsonDataError::from_err(err).into(), + serde_json::error::Category::Syntax | serde_json::error::Category::Eof => { + JsonSyntaxError::from_err(err).into() + } + serde_json::error::Category::Io => { + if cfg!(debug_assertions) { + // we don't use `serde_json::from_reader` and instead always buffer + // bodies first, so we shouldn't encounter any IO errors + unreachable!() + } else { + JsonSyntaxError::from_err(err).into() + } + } + }; + return Err(rejection); + } + }; Ok(Json(value)) } else { @@ -244,4 +264,19 @@ mod tests { assert!(valid_json_content_type("application/cloudevents+json").await); assert!(!valid_json_content_type("text/json").await); } + + #[tokio::test] + async fn invalid_json_syntax() { + let app = Router::new().route("/", post(|_: Json| async {})); + + let client = TestClient::new(app); + let res = client + .post("/") + .body("{") + .header("content-type", "application/json") + .send() + .await; + + assert_eq!(res.status(), StatusCode::BAD_REQUEST); + } } diff --git a/examples/customize-extractor-error/src/main.rs b/examples/customize-extractor-error/src/main.rs index 119c4c08..fcfd1726 100644 --- a/examples/customize-extractor-error/src/main.rs +++ b/examples/customize-extractor-error/src/main.rs @@ -69,7 +69,7 @@ where Err(rejection) => { // convert the error from `axum::Json` into whatever we want let (status, body): (_, Cow<'_, str>) = match rejection { - JsonRejection::InvalidJsonBody(err) => ( + JsonRejection::JsonDataError(err) => ( StatusCode::BAD_REQUEST, format!("Invalid JSON request: {}", err).into(), ),