Ensure that API endpoints return JSON errors

src/middleware.rs

@@ -4,6 +4,7 @@ mod block_traffic;
 mod common_headers;
 mod debug;
 mod ember_html;
+mod errors;
 pub mod log_request;
 pub mod normalize_path;
 pub mod real_ip;
@@ -25,6 +26,7 @@ use tower_http::compression::{CompressionLayer, CompressionLevel};
 use tower_http::timeout::{RequestBodyTimeoutLayer, TimeoutLayer};
 
 use crate::app::AppState;
+use crate::middleware::errors::ensure_json_errors;
 use crate::Env;
 
 pub fn apply_axum_middleware(state: AppState, router: Router<()>) -> Router {
@@ -62,6 +64,7 @@ pub fn apply_axum_middleware(state: AppState, router: Router<()>) -> Router {
         }));
 
     let middlewares_2 = tower::ServiceBuilder::new()
+        .layer(from_fn(ensure_json_errors))
         .layer(from_fn_with_state(state.clone(), session::attach_session))
         .layer(from_fn_with_state(
             state.clone(),

src/middleware/errors.rs

@@ -0,0 +1,153 @@
+use axum::extract::Request;
+use axum::middleware::Next;
+use axum::response::{IntoResponse, Response};
+use axum::Json;
+use http::{header, StatusCode};
+
+/// Convert plain text errors into JSON errors.
+///
+/// The built-in extractors in [axum] return plain text errors, but our API
+/// contract promises JSON errors. This middleware converts such plain text
+/// errors into corresponding JSON errors, allowing us to use the [axum]
+/// extractors without having to care about the error responses.
+pub async fn ensure_json_errors(req: Request, next: Next) -> Response {
+    let is_api_request = req.uri().path().starts_with("/api/");
+
+    let res = next.run(req).await;
+    if !is_api_request {
+        return res;
+    }
+
+    let status = res.status();
+    if !status.is_client_error() && !status.is_server_error() {
+        return res;
+    }
+
+    let content_type = res.headers().get("content-type");
+    if !matches!(content_type, Some(content_type) if content_type == "text/plain; charset=utf-8") {
+        return res;
+    }
+
+    convert_to_json_response(res).await.unwrap_or_else(|error| {
+        error!(%error, "Failed to convert response to JSON");
+        StatusCode::INTERNAL_SERVER_ERROR.into_response()
+    })
+}
+
+async fn convert_to_json_response(res: Response) -> anyhow::Result<Response> {
+    let (mut parts, body) = res.into_parts();
+
+    // The `Json` struct is somehow not able to override these headers of the
+    // `Parts` struct, so we remove them here to avoid the conflict.
+    parts.headers.remove(header::CONTENT_TYPE);
+    parts.headers.remove(header::CONTENT_LENGTH);
+
+    let bytes = axum::body::to_bytes(body, 1_000_000).await?;
+    let text = std::str::from_utf8(&bytes)?;
+
+    let json = serde_json::json!({ "errors": [{ "detail": text }] });
+
+    Ok((parts, Json(json)).into_response())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use axum::body::Body;
+    use axum::middleware::from_fn;
+    use axum::routing::get;
+    use axum::Router;
+    use bytes::Bytes;
+    use http::response::Parts;
+    use http::{Request, StatusCode};
+    use insta::assert_debug_snapshot;
+    use tower::ServiceExt;
+
+    fn build_app() -> Router {
+        let okay = get(|| async { "Everything is okay" });
+        let teapot = get(|| async { (StatusCode::IM_A_TEAPOT, "I'm a teapot") });
+        let internal =
+            get(|| async { (StatusCode::INTERNAL_SERVER_ERROR, "Internal Server Error") });
+
+        Router::new()
+            .route("/api/ok", okay.clone())
+            .route("/api/teapot", teapot.clone())
+            .route("/teapot", teapot)
+            .route("/api/500", internal.clone())
+            .route("/500", internal)
+            .layer(from_fn(ensure_json_errors))
+    }
+
+    async fn request(path: &str) -> anyhow::Result<(Parts, Bytes)> {
+        let request = Request::get(path).body(Body::empty())?;
+        let response = build_app().oneshot(request).await?;
+        let (parts, body) = response.into_parts();
+        let bytes = axum::body::to_bytes(body, usize::MAX).await?;
+        Ok((parts, bytes))
+    }
+
+    /// Check that successful text responses are **not** converted to JSON even
+    /// for `/api/` requests.
+    #[tokio::test]
+    async fn test_success_responses() {
+        let (parts, bytes) = request("/api/ok").await.unwrap();
+        assert_eq!(parts.status, StatusCode::OK);
+        assert_debug_snapshot!(parts.headers, @r###"
+        {
+            "content-type": "text/plain; charset=utf-8",
+            "content-length": "18",
+        }
+        "###);
+        assert_debug_snapshot!(bytes, @r###"b"Everything is okay""###);
+    }
+
+    /// Check that 4xx text responses **are** converted to JSON, but only
+    /// for `/api/` requests.
+    #[tokio::test]
+    async fn test_client_errors() {
+        let (parts, bytes) = request("/api/teapot").await.unwrap();
+        assert_eq!(parts.status, StatusCode::IM_A_TEAPOT);
+        assert_debug_snapshot!(parts.headers, @r###"
+        {
+            "content-type": "application/json",
+            "content-length": "38",
+        }
+        "###);
+        assert_debug_snapshot!(bytes, @r###"b"{\"errors\":[{\"detail\":\"I'm a teapot\"}]}""###);
+
+        let (parts, bytes) = request("/teapot").await.unwrap();
+        assert_eq!(parts.status, StatusCode::IM_A_TEAPOT);
+        assert_debug_snapshot!(parts.headers, @r###"
+        {
+            "content-type": "text/plain; charset=utf-8",
+            "content-length": "12",
+        }
+        "###);
+        assert_debug_snapshot!(bytes, @r###"b"I'm a teapot""###);
+    }
+
+    /// Check that 5xx text responses **are** converted to JSON, but only
+    /// for `/api/` requests.
+    #[tokio::test]
+    async fn test_server_errors() {
+        let (parts, bytes) = request("/api/500").await.unwrap();
+        assert_eq!(parts.status, StatusCode::INTERNAL_SERVER_ERROR);
+        assert_debug_snapshot!(parts.headers, @r###"
+        {
+            "content-type": "application/json",
+            "content-length": "47",
+        }
+        "###);
+        assert_debug_snapshot!(bytes, @r###"b"{\"errors\":[{\"detail\":\"Internal Server Error\"}]}""###);
+
+        let (parts, bytes) = request("/500").await.unwrap();
+        assert_eq!(parts.status, StatusCode::INTERNAL_SERVER_ERROR);
+        assert_debug_snapshot!(parts.headers, @r###"
+        {
+            "content-type": "text/plain; charset=utf-8",
+            "content-length": "21",
+        }
+        "###);
+        assert_debug_snapshot!(bytes, @r###"b"Internal Server Error""###);
+    }
+}

src/tests/routes/crates/versions/download.rs

@@ -1,7 +1,7 @@
 use crate::builders::{CrateBuilder, VersionBuilder};
 use crate::util::{RequestHelper, TestApp};
 use http::StatusCode;
-use insta::assert_snapshot;
+use insta::assert_json_snapshot;
 
 #[test]
 fn download_nonexistent_version_of_existing_crate_404s() {
@@ -52,7 +52,7 @@ fn rejected_non_canonical_download() {
     // and assert that the correct download link is returned.
     let response = anon.get::<()>("/api/v1/crates/foo-download/1.0.0/download");
     assert_eq!(response.status(), StatusCode::NOT_FOUND);
-    assert_snapshot!(response.into_text());
+    assert_json_snapshot!(response.into_json());
 }
 
 #[test]

src/tests/routes/crates/versions/snapshots/all__routes__crates__versions__download__rejected_non_canonical_download.snap

@@ -1,5 +1,11 @@
 ---
 source: src/tests/routes/crates/versions/download.rs
-expression: response.into_text()
+expression: response.into_json()
 ---
-Your request is for a version of the `foo-download` crate, but that crate is actually named `foo_download`. Support for "non-canonical" downloads has been deprecated and disabled. See https://blog.rust-lang.org/2023/10/27/crates-io-non-canonical-downloads.html for more detail.
+{
+  "errors": [
+    {
+      "detail": "Your request is for a version of the `foo-download` crate, but that crate is actually named `foo_download`. Support for \"non-canonical\" downloads has been deprecated and disabled. See https://blog.rust-lang.org/2023/10/27/crates-io-non-canonical-downloads.html for more detail."
+    }
+  ]
+}

src/tests/routes/session/authorize.rs

@@ -7,7 +7,7 @@ fn access_token_needs_data() {
     let response = anon.get::<()>("/api/private/session/authorize");
     assert_eq!(response.status(), StatusCode::BAD_REQUEST);
     assert_eq!(
-        response.into_text(),
-        "Failed to deserialize query string: missing field `code`"
+        response.into_json(),
+        json!({ "errors": [{ "detail": "Failed to deserialize query string: missing field `code`" }] })
     );
 }

src/tests/server.rs

@@ -2,7 +2,7 @@ use crate::builders::*;
 use crate::util::*;
 use std::collections::HashSet;
 
-use ::insta::assert_display_snapshot;
+use ::insta::assert_json_snapshot;
 use http::{header, Request, StatusCode};
 
 #[test]
@@ -74,7 +74,7 @@ fn block_traffic_via_arbitrary_header_and_value() {
 
     let resp = anon.run::<()>(req);
     assert_eq!(resp.status(), StatusCode::FORBIDDEN);
-    assert_display_snapshot!(resp.into_text());
+    assert_json_snapshot!(resp.into_json());
 
     let req = Request::get("/api/v1/crates/dl_no_ua/0.99.0/download")
         // A request with a header value we don't want to block is allowed, even though there might
@@ -100,5 +100,5 @@ fn block_traffic_via_ip() {
 
     let resp = anon.get::<()>("/api/v1/crates");
     assert_eq!(resp.status(), StatusCode::FORBIDDEN);
-    assert_display_snapshot!(resp.into_text());
+    assert_json_snapshot!(resp.into_json());
 }

src/tests/snapshots/all__server__block_traffic_via_arbitrary_header_and_value.snap

@@ -1,6 +1,11 @@
 ---
 source: src/tests/server.rs
-assertion_line: 67
-expression: resp.into_text()
+expression: resp.into_json()
 ---
-We are unable to process your request at this time. This usually means that you are in violation of our crawler policy (https://crates.io/policies#crawlers). Please open an issue at https://github.com/rust-lang/crates.io or email help@crates.io and provide the request id abcd
+{
+  "errors": [
+    {
+      "detail": "We are unable to process your request at this time. This usually means that you are in violation of our crawler policy (https://crates.io/policies#crawlers). Please open an issue at https://github.com/rust-lang/crates.io or email help@crates.io and provide the request id abcd"
+    }
+  ]
+}

src/tests/snapshots/all__server__block_traffic_via_ip.snap

@@ -1,5 +1,11 @@
 ---
 source: src/tests/server.rs
-expression: resp.into_text()
+expression: resp.into_json()
 ---
-We are unable to process your request at this time. This usually means that you are in violation of our crawler policy (https://crates.io/policies#crawlers). Please open an issue at https://github.com/rust-lang/crates.io or email help@crates.io and provide the request id 
+{
+  "errors": [
+    {
+      "detail": "We are unable to process your request at this time. This usually means that you are in violation of our crawler policy (https://crates.io/policies#crawlers). Please open an issue at https://github.com/rust-lang/crates.io or email help@crates.io and provide the request id "
+    }
+  ]
+}