From c6d107a05fa0c45cd4ff7fb4f01f3492a2d65a4f Mon Sep 17 00:00:00 2001 From: Tamo Date: Tue, 5 Oct 2021 13:30:53 +0200 Subject: [PATCH 1/5] makes the content-type mandatory for every routes --- meilisearch-http/src/error.rs | 16 ++++++---- meilisearch-http/src/lib.rs | 29 +++++++++++++++++-- .../src/routes/indexes/documents.rs | 19 ++++++++++-- .../tests/documents/add_documents.rs | 11 +++++-- 4 files changed, 61 insertions(+), 14 deletions(-) diff --git a/meilisearch-http/src/error.rs b/meilisearch-http/src/error.rs index cbe963615..5a1c2064c 100644 --- a/meilisearch-http/src/error.rs +++ b/meilisearch-http/src/error.rs @@ -11,17 +11,21 @@ use serde::{Deserialize, Serialize}; #[derive(Debug, thiserror::Error)] pub enum MeilisearchHttpError { - #[error("A Content-Type header is missing. Accepted values for the Content-Type header are: \"application/json\", \"application/x-ndjson\", \"text/csv\"")] - MissingContentType, - #[error("The Content-Type \"{0}\" is invalid. Accepted values for the Content-Type header are: \"application/json\", \"application/x-ndjson\", \"text/csv\"")] - InvalidContentType(String), + #[error("A Content-Type header is missing. Accepted values for the Content-Type header are: {}", + .0.iter().map(|s| format!("\"{}\"", s)).collect::>().join(", "))] + MissingContentType(Vec), + #[error( + "The Content-Type \"{0}\" is invalid. Accepted values for the Content-Type header are: {}", + .1.iter().map(|s| format!("\"{}\"", s)).collect::>().join(", ") + )] + InvalidContentType(String, Vec), } impl ErrorCode for MeilisearchHttpError { fn error_code(&self) -> Code { match self { - MeilisearchHttpError::MissingContentType => Code::MissingContentType, - MeilisearchHttpError::InvalidContentType(_) => Code::InvalidContentType, + MeilisearchHttpError::MissingContentType(_) => Code::MissingContentType, + MeilisearchHttpError::InvalidContentType(_, _) => Code::InvalidContentType, } } } diff --git a/meilisearch-http/src/lib.rs b/meilisearch-http/src/lib.rs index 0b9dacbe0..870162516 100644 --- a/meilisearch-http/src/lib.rs +++ b/meilisearch-http/src/lib.rs @@ -11,7 +11,10 @@ pub mod routes; use std::path::Path; use std::time::Duration; +use crate::error::{MeilisearchHttpError, ResponseError}; use crate::extractors::authentication::AuthConfig; +use actix_web::error::JsonPayloadError; +use http::header::CONTENT_TYPE; pub use option::Opt; use actix_web::web; @@ -98,9 +101,28 @@ pub fn configure_data(config: &mut web::ServiceConfig, data: MeiliSearch, opt: & .app_data(data) .app_data( web::JsonConfig::default() - .limit(http_payload_size_limit) - .content_type(|_mime| true) // Accept all mime types - .error_handler(|err, _req| error::payload_error_handler(err).into()), + .content_type(|mime| mime == mime::APPLICATION_JSON) + .error_handler(|err, req| match err { + JsonPayloadError::ContentType if req.headers().get(CONTENT_TYPE).is_none() => { + ResponseError::from(MeilisearchHttpError::MissingContentType(vec![ + mime::APPLICATION_JSON.to_string(), + ])) + .into() + } + JsonPayloadError::ContentType => { + ResponseError::from(MeilisearchHttpError::InvalidContentType( + req.headers() + .get(CONTENT_TYPE) + .unwrap() + .to_str() + .unwrap_or("unknown") + .to_string(), + vec![mime::APPLICATION_JSON.to_string()], + )) + .into() + } + err => error::payload_error_handler(err).into(), + }), ) .app_data(PayloadConfig::new(http_payload_size_limit)) .app_data( @@ -180,6 +202,7 @@ macro_rules! create_app { use actix_web::middleware::TrailingSlash; use actix_web::App; use actix_web::{middleware, web}; + use meilisearch_http::error::{MeilisearchHttpError, ResponseError}; use meilisearch_http::routes; use meilisearch_http::{configure_auth, configure_data, dashboard}; diff --git a/meilisearch-http/src/routes/indexes/documents.rs b/meilisearch-http/src/routes/indexes/documents.rs index 50ba31c97..de53c3b3f 100644 --- a/meilisearch-http/src/routes/indexes/documents.rs +++ b/meilisearch-http/src/routes/indexes/documents.rs @@ -181,9 +181,24 @@ async fn document_addition( Some("application/x-ndjson") => DocumentAdditionFormat::Ndjson, Some("text/csv") => DocumentAdditionFormat::Csv, Some(other) => { - return Err(MeilisearchHttpError::InvalidContentType(other.to_string()).into()) + return Err(MeilisearchHttpError::InvalidContentType( + other.to_string(), + vec![ + "application/json".to_string(), + "application/x-ndjson".to_string(), + "application/csv".to_string(), + ], + ) + .into()) + } + None => { + return Err(MeilisearchHttpError::MissingContentType(vec![ + "application/json".to_string(), + "application/x-ndjson".to_string(), + "application/csv".to_string(), + ]) + .into()) } - None => return Err(MeilisearchHttpError::MissingContentType.into()), }; let update = Update::DocumentAddition { diff --git a/meilisearch-http/tests/documents/add_documents.rs b/meilisearch-http/tests/documents/add_documents.rs index 13265dcfd..f0b01c4bd 100644 --- a/meilisearch-http/tests/documents/add_documents.rs +++ b/meilisearch-http/tests/documents/add_documents.rs @@ -67,7 +67,6 @@ async fn add_documents_test_no_content_types() { /// any other content-type is must be refused #[actix_rt::test] -#[ignore] async fn add_documents_test_bad_content_types() { let document = json!([ { @@ -91,8 +90,14 @@ async fn add_documents_test_bad_content_types() { let res = test::call_service(&app, req).await; let status_code = res.status(); let body = test::read_body(res).await; - assert_eq!(status_code, 405); - assert!(body.is_empty()); + let response: Value = serde_json::from_slice(&body).unwrap_or_default(); + assert_eq!(status_code, 415); + assert_eq!( + response["message"], + json!( + r#"The Content-Type "text/plain" is invalid. Accepted values for the Content-Type header are: "application/json", "application/x-ndjson", "application/csv""# + ) + ); } #[actix_rt::test] From dfa199f98f0add9340d1a4187b9539e030cdba6e Mon Sep 17 00:00:00 2001 From: Tamo Date: Tue, 5 Oct 2021 17:07:44 +0200 Subject: [PATCH 2/5] add content-type tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix typo Co-authored-by: Clémentine Urquizar --- meilisearch-http/tests/content_type.rs | 109 +++++++++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 meilisearch-http/tests/content_type.rs diff --git a/meilisearch-http/tests/content_type.rs b/meilisearch-http/tests/content_type.rs new file mode 100644 index 000000000..d548777f4 --- /dev/null +++ b/meilisearch-http/tests/content_type.rs @@ -0,0 +1,109 @@ +mod common; + +use crate::common::{GetAllDocumentsOptions, Server}; +use actix_web::test; +use meilisearch_http::create_app; +use serde_json::{json, Value}; + +#[actix_rt::test] +async fn strict_json_bad_content_type() { + let routes = [ + // all the POST routes except the dumps that can be created without any body or content-type + // and the search that is not a strict json + "/indexes", + "/indexes/doggo/documents/delete-batch", + "/indexes/doggo/search", + "/indexes/doggo/settings", + "/indexes/doggo/settings/displayed-attributes", + "/indexes/doggo/settings/distinct-attribute", + "/indexes/doggo/settings/filterable-attributes", + "/indexes/doggo/settings/ranking-rules", + "/indexes/doggo/settings/searchable-attributes", + "/indexes/doggo/settings/sortable-attributes", + "/indexes/doggo/settings/stop-words", + "/indexes/doggo/settings/synonyms", + ]; + let bad_content_types = [ + "application/csv", + "application/x-ndjson", + "application/x-www-form-urlencoded", + "text/plain", + "json", + "application", + "json/application", + ]; + + let document = "{}"; + let server = Server::new().await; + let app = test::init_service(create_app!( + &server.service.meilisearch, + true, + &server.service.options + )) + .await; + for route in routes { + // Good content-type, we probably have an error since we didn't send anything in the json + // so we only ensure we didn't get a bad media type error. + let req = test::TestRequest::post() + .uri(route) + .set_payload(document) + .insert_header(("content-type", "application/json")) + .to_request(); + let res = test::call_service(&app, req).await; + let status_code = res.status(); + assert_ne!(status_code, 415, + "calling the route `{}` with a content-type of json isn't supposed to throw a bad media type error", route); + + // No content-type. + let req = test::TestRequest::post() + .uri(route) + .set_payload(document) + .to_request(); + let res = test::call_service(&app, req).await; + let status_code = res.status(); + let body = test::read_body(res).await; + let response: Value = serde_json::from_slice(&body).unwrap_or_default(); + assert_eq!(status_code, 415, "calling the route `{}` without content-type is supposed to throw a bad media type error", route); + assert_eq!( + response, + json!({ + "message": r#"A Content-Type header is missing. Accepted values for the Content-Type header are: "application/json""#, + "errorCode": "missing_content_type", + "errorType": "invalid_request_error", + "errorLink": "https://docs.meilisearch.com/errors#missing_content_type", + }), + "when calling the route `{}` with no content-type", + route, + ); + + for bad_content_type in bad_content_types { + // Always bad content-type + let req = test::TestRequest::post() + .uri(route) + .set_payload(document.to_string()) + .insert_header(("content-type", bad_content_type)) + .to_request(); + let res = test::call_service(&app, req).await; + let status_code = res.status(); + let body = test::read_body(res).await; + let response: Value = serde_json::from_slice(&body).unwrap_or_default(); + assert_eq!(status_code, 415); + let expected_error_message = format!( + r#"The Content-Type "{}" is invalid. Accepted values for the Content-Type header are: "application/json""#, + bad_content_type + ); + assert_eq!( + response, + json!({ + "message": expected_error_message, + "errorCode": "invalid_content_type", + "errorType": "invalid_request_error", + "errorLink": "https://docs.meilisearch.com/errors#invalid_content_type", + }), + "when calling the route `{}` with a content-type of `{}`", + route, + bad_content_type, + ); + } + } +} From 37b267ffb327ca3127dfde3ed3f61177319ecb68 Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 6 Oct 2021 12:49:58 +0200 Subject: [PATCH 3/5] duplicate the post document tests with the put verb --- .../tests/documents/add_documents.rs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/meilisearch-http/tests/documents/add_documents.rs b/meilisearch-http/tests/documents/add_documents.rs index f0b01c4bd..a9189a30c 100644 --- a/meilisearch-http/tests/documents/add_documents.rs +++ b/meilisearch-http/tests/documents/add_documents.rs @@ -22,6 +22,7 @@ async fn add_documents_test_json_content_types() { &server.service.options )) .await; + // post let req = test::TestRequest::post() .uri("/indexes/dog/documents") .set_payload(document.to_string()) @@ -33,6 +34,19 @@ async fn add_documents_test_json_content_types() { let response: Value = serde_json::from_slice(&body).unwrap_or_default(); assert_eq!(status_code, 202); assert_eq!(response, json!({ "updateId": 0 })); + + // put + let req = test::TestRequest::put() + .uri("/indexes/dog/documents") + .set_payload(document.to_string()) + .insert_header(("content-type", "application/json")) + .to_request(); + let res = test::call_service(&app, req).await; + let status_code = res.status(); + let body = test::read_body(res).await; + let response: Value = serde_json::from_slice(&body).unwrap_or_default(); + assert_eq!(status_code, 202); + assert_eq!(response, json!({ "updateId": 1 })); } /// no content type is still supposed to be accepted as json @@ -52,6 +66,7 @@ async fn add_documents_test_no_content_types() { &server.service.options )) .await; + // post let req = test::TestRequest::post() .uri("/indexes/dog/documents") .set_payload(document.to_string()) @@ -63,6 +78,19 @@ async fn add_documents_test_no_content_types() { let response: Value = serde_json::from_slice(&body).unwrap_or_default(); assert_eq!(status_code, 202); assert_eq!(response, json!({ "updateId": 0 })); + + // put + let req = test::TestRequest::put() + .uri("/indexes/dog/documents") + .set_payload(document.to_string()) + .insert_header(("content-type", "application/json")) + .to_request(); + let res = test::call_service(&app, req).await; + let status_code = res.status(); + let body = test::read_body(res).await; + let response: Value = serde_json::from_slice(&body).unwrap_or_default(); + assert_eq!(status_code, 202); + assert_eq!(response, json!({ "updateId": 1 })); } /// any other content-type is must be refused @@ -82,6 +110,7 @@ async fn add_documents_test_bad_content_types() { &server.service.options )) .await; + // post let req = test::TestRequest::post() .uri("/indexes/dog/documents") .set_payload(document.to_string()) @@ -98,6 +127,24 @@ async fn add_documents_test_bad_content_types() { r#"The Content-Type "text/plain" is invalid. Accepted values for the Content-Type header are: "application/json", "application/x-ndjson", "application/csv""# ) ); + + // put + let req = test::TestRequest::put() + .uri("/indexes/dog/documents") + .set_payload(document.to_string()) + .insert_header(("content-type", "text/plain")) + .to_request(); + let res = test::call_service(&app, req).await; + let status_code = res.status(); + let body = test::read_body(res).await; + let response: Value = serde_json::from_slice(&body).unwrap_or_default(); + assert_eq!(status_code, 415); + assert_eq!( + response["message"], + json!( + r#"The Content-Type "text/plain" is invalid. Accepted values for the Content-Type header are: "application/json", "application/x-ndjson", "application/csv""# + ) + ); } #[actix_rt::test] From 9a1e44dc7862f915e457e6c62831c98f9388c01b Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 6 Oct 2021 11:49:34 +0200 Subject: [PATCH 4/5] Apply suggestion - remove the payload_error_handler in favor of a PayloadError::from - merge the two match branch into one - makes the accepted content type a const instead of recalculating it for every error --- meilisearch-http/src/error.rs | 15 ++++--- meilisearch-http/src/lib.rs | 39 ++++++++----------- .../src/routes/indexes/documents.rs | 23 +++++------ 3 files changed, 38 insertions(+), 39 deletions(-) diff --git a/meilisearch-http/src/error.rs b/meilisearch-http/src/error.rs index 5a1c2064c..1f2f4742d 100644 --- a/meilisearch-http/src/error.rs +++ b/meilisearch-http/src/error.rs @@ -30,6 +30,12 @@ impl ErrorCode for MeilisearchHttpError { } } +impl From for aweb::Error { + fn from(other: MeilisearchHttpError) -> Self { + aweb::Error::from(ResponseError::from(other)) + } +} + #[derive(Debug, Serialize, Deserialize, Clone)] #[serde(rename_all = "camelCase")] pub struct ResponseError { @@ -125,9 +131,8 @@ impl From for PayloadError { } } -pub fn payload_error_handler(err: E) -> ResponseError -where - E: Into, -{ - err.into().into() +impl From for aweb::Error { + fn from(other: PayloadError) -> Self { + aweb::Error::from(ResponseError::from(other)) + } } diff --git a/meilisearch-http/src/lib.rs b/meilisearch-http/src/lib.rs index 870162516..cbb417ab1 100644 --- a/meilisearch-http/src/lib.rs +++ b/meilisearch-http/src/lib.rs @@ -11,13 +11,14 @@ pub mod routes; use std::path::Path; use std::time::Duration; -use crate::error::{MeilisearchHttpError, ResponseError}; +use crate::error::MeilisearchHttpError; use crate::extractors::authentication::AuthConfig; use actix_web::error::JsonPayloadError; +use error::PayloadError; use http::header::CONTENT_TYPE; pub use option::Opt; -use actix_web::web; +use actix_web::{web, HttpRequest}; use extractors::authentication::policies::*; use extractors::payload::PayloadConfig; @@ -102,32 +103,24 @@ pub fn configure_data(config: &mut web::ServiceConfig, data: MeiliSearch, opt: & .app_data( web::JsonConfig::default() .content_type(|mime| mime == mime::APPLICATION_JSON) - .error_handler(|err, req| match err { - JsonPayloadError::ContentType if req.headers().get(CONTENT_TYPE).is_none() => { - ResponseError::from(MeilisearchHttpError::MissingContentType(vec![ - mime::APPLICATION_JSON.to_string(), - ])) - .into() - } - JsonPayloadError::ContentType => { - ResponseError::from(MeilisearchHttpError::InvalidContentType( - req.headers() - .get(CONTENT_TYPE) - .unwrap() - .to_str() - .unwrap_or("unknown") - .to_string(), + .error_handler(|err, req: &HttpRequest| match err { + JsonPayloadError::ContentType => match req.headers().get(CONTENT_TYPE) { + Some(content_type) => MeilisearchHttpError::InvalidContentType( + content_type.to_str().unwrap_or("unknown").to_string(), vec![mime::APPLICATION_JSON.to_string()], - )) - .into() - } - err => error::payload_error_handler(err).into(), + ) + .into(), + None => MeilisearchHttpError::MissingContentType(vec![ + mime::APPLICATION_JSON.to_string(), + ]) + .into(), + }, + err => PayloadError::from(err).into(), }), ) .app_data(PayloadConfig::new(http_payload_size_limit)) .app_data( - web::QueryConfig::default() - .error_handler(|err, _req| error::payload_error_handler(err).into()), + web::QueryConfig::default().error_handler(|err, _req| PayloadError::from(err).into()), ); } diff --git a/meilisearch-http/src/routes/indexes/documents.rs b/meilisearch-http/src/routes/indexes/documents.rs index de53c3b3f..b3163f543 100644 --- a/meilisearch-http/src/routes/indexes/documents.rs +++ b/meilisearch-http/src/routes/indexes/documents.rs @@ -6,6 +6,7 @@ use log::debug; use meilisearch_lib::index_controller::{DocumentAdditionFormat, Update}; use meilisearch_lib::milli::update::IndexDocumentsMethod; use meilisearch_lib::MeiliSearch; +use once_cell::sync::Lazy; use serde::Deserialize; use serde_json::Value; use tokio::sync::mpsc; @@ -176,6 +177,13 @@ async fn document_addition( body: Payload, method: IndexDocumentsMethod, ) -> Result { + const ACCEPTED_CONTENT_TYPE: Lazy> = Lazy::new(|| { + vec![ + "application/json".to_string(), + "application/x-ndjson".to_string(), + "application/csv".to_string(), + ] + }); let format = match content_type { Some("application/json") => DocumentAdditionFormat::Json, Some("application/x-ndjson") => DocumentAdditionFormat::Ndjson, @@ -183,21 +191,14 @@ async fn document_addition( Some(other) => { return Err(MeilisearchHttpError::InvalidContentType( other.to_string(), - vec![ - "application/json".to_string(), - "application/x-ndjson".to_string(), - "application/csv".to_string(), - ], + ACCEPTED_CONTENT_TYPE.clone(), ) .into()) } None => { - return Err(MeilisearchHttpError::MissingContentType(vec![ - "application/json".to_string(), - "application/x-ndjson".to_string(), - "application/csv".to_string(), - ]) - .into()) + return Err( + MeilisearchHttpError::MissingContentType(ACCEPTED_CONTENT_TYPE.clone()).into(), + ) } }; From 66dbd3cd341c50a3fbb4de09935db1553e904e60 Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 6 Oct 2021 12:33:25 +0200 Subject: [PATCH 5/5] makes clippy happy --- meilisearch-http/src/routes/indexes/documents.rs | 2 +- meilisearch-http/tests/content_type.rs | 4 +++- meilisearch-lib/src/index_controller/dump_actor/loaders/v2.rs | 3 +-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/meilisearch-http/src/routes/indexes/documents.rs b/meilisearch-http/src/routes/indexes/documents.rs index b3163f543..e890bab24 100644 --- a/meilisearch-http/src/routes/indexes/documents.rs +++ b/meilisearch-http/src/routes/indexes/documents.rs @@ -177,7 +177,7 @@ async fn document_addition( body: Payload, method: IndexDocumentsMethod, ) -> Result { - const ACCEPTED_CONTENT_TYPE: Lazy> = Lazy::new(|| { + static ACCEPTED_CONTENT_TYPE: Lazy> = Lazy::new(|| { vec![ "application/json".to_string(), "application/x-ndjson".to_string(), diff --git a/meilisearch-http/tests/content_type.rs b/meilisearch-http/tests/content_type.rs index d548777f4..5402a7cd6 100644 --- a/meilisearch-http/tests/content_type.rs +++ b/meilisearch-http/tests/content_type.rs @@ -1,6 +1,8 @@ +#![allow(dead_code)] + mod common; -use crate::common::{GetAllDocumentsOptions, Server}; +use crate::common::Server; use actix_web::test; use meilisearch_http::create_app; use serde_json::{json, Value}; diff --git a/meilisearch-lib/src/index_controller/dump_actor/loaders/v2.rs b/meilisearch-lib/src/index_controller/dump_actor/loaders/v2.rs index 0040d4cea..35640aaef 100644 --- a/meilisearch-lib/src/index_controller/dump_actor/loaders/v2.rs +++ b/meilisearch-lib/src/index_controller/dump_actor/loaders/v2.rs @@ -4,7 +4,6 @@ use std::path::{Path, PathBuf}; use serde_json::{Deserializer, Value}; use tempfile::NamedTempFile; -use uuid::Uuid; use crate::index_controller::dump_actor::loaders::compat::{asc_ranking_rule, desc_ranking_rule}; use crate::index_controller::dump_actor::Metadata; @@ -200,7 +199,7 @@ impl From for Enqueued { method, // Just ignore if the uuid is no present. If it is needed later, an error will // be thrown. - content_uuid: content.unwrap_or_else(Uuid::default), + content_uuid: content.unwrap_or_default(), } } compat::UpdateMeta::ClearDocuments => Update::ClearDocuments,