From d96023ece83cba779edc99fdb78ebb0f20a423c1 Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 20 Apr 2023 13:54:00 +0200 Subject: [PATCH] Provide a reason for not acceptable server request rejections This commit makes it so that when an incoming request's `Accept` header value set cannot be satisfied by the server, the unsatisfiable value set is stored in the `RequestRejection`. The rejection reason will thus be `DEBUG`-logged. --- .../ServerHttpBoundProtocolGenerator.kt | 4 +- .../src/proto/aws_json/rejection.rs | 6 +- .../src/proto/rest_json_1/rejection.rs | 10 +- .../src/proto/rest_json_1/runtime_error.rs | 2 +- .../src/proto/rest_xml/rejection.rs | 6 +- .../aws-smithy-http-server/src/protocols.rs | 149 ++++++++++++------ .../aws-smithy-http-server/src/rejection.rs | 8 + 7 files changed, 119 insertions(+), 66 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt index 85b624b567..510146508d 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt @@ -182,9 +182,7 @@ class ServerHttpBoundProtocolTraitImplGenerator( httpBindingResolver.responseContentType(operationShape)?.also { contentType -> rustTemplate( """ - if !#{SmithyHttpServer}::protocols::accept_header_classifier(request.headers(), ${contentType.dq()}) { - return Err(#{RequestRejection}::NotAcceptable); - } + #{SmithyHttpServer}::protocols::accept_header_classifier(request.headers(), ${contentType.dq()})?; """, *codegenScope, ) diff --git a/rust-runtime/aws-smithy-http-server/src/proto/aws_json/rejection.rs b/rust-runtime/aws-smithy-http-server/src/proto/aws_json/rejection.rs index 491e865dd6..d241984310 100644 --- a/rust-runtime/aws-smithy-http-server/src/proto/aws_json/rejection.rs +++ b/rust-runtime/aws-smithy-http-server/src/proto/aws_json/rejection.rs @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -use crate::rejection::MissingContentTypeReason; +use crate::rejection::{MissingContentTypeReason, NotAcceptableReason}; use thiserror::Error; #[derive(Debug, Error)] @@ -18,8 +18,8 @@ pub enum ResponseRejection { pub enum RequestRejection { #[error("error converting non-streaming body to bytes: {0}")] BufferHttpBodyBytes(crate::Error), - #[error("request contains invalid value for `Accept` header")] - NotAcceptable, + #[error("request is not acceptable: {0}")] + NotAcceptable(#[from] NotAcceptableReason), #[error("expected `Content-Type` header not found: {0}")] MissingContentType(#[from] MissingContentTypeReason), #[error("error deserializing request HTTP body as JSON: {0}")] diff --git a/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/rejection.rs b/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/rejection.rs index 5ccf0225f7..fca9d67ffe 100644 --- a/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/rejection.rs +++ b/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/rejection.rs @@ -47,7 +47,7 @@ //! //! Consult `crate::proto::$protocolName::rejection` for rejection types for other protocols. -use crate::rejection::MissingContentTypeReason; +use crate::rejection::{MissingContentTypeReason, NotAcceptableReason}; use std::num::TryFromIntError; use thiserror::Error; @@ -109,10 +109,10 @@ pub enum RequestRejection { #[error("error converting non-streaming body to bytes: {0}")] BufferHttpBodyBytes(crate::Error), - /// Used when the request contained an `Accept` header with a MIME type, and the server cannot - /// return a response body adhering to that MIME type. - #[error("request contains invalid value for `Accept` header")] - NotAcceptable, + /// Used when the request contained `Accept` headers with certain MIME types, and the server cannot + /// return a response body adhering to _any_ of those MIME types. + #[error("request is not acceptable: {0}")] + NotAcceptable(#[from] NotAcceptableReason), /// Used when checking the `Content-Type` header. #[error("expected `Content-Type` header not found: {0}")] diff --git a/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/runtime_error.rs b/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/runtime_error.rs index 0525d9b576..bb62d7c9eb 100644 --- a/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/runtime_error.rs +++ b/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/runtime_error.rs @@ -115,7 +115,7 @@ impl From for RuntimeError { match err { RequestRejection::MissingContentType(_reason) => Self::UnsupportedMediaType, RequestRejection::ConstraintViolation(reason) => Self::Validation(reason), - RequestRejection::NotAcceptable => Self::NotAcceptable, + RequestRejection::NotAcceptable(_reason) => Self::NotAcceptable, _ => Self::Serialization(crate::Error::new(err)), } } diff --git a/rust-runtime/aws-smithy-http-server/src/proto/rest_xml/rejection.rs b/rust-runtime/aws-smithy-http-server/src/proto/rest_xml/rejection.rs index 51b0ac0ad6..7e13e2d5f9 100644 --- a/rust-runtime/aws-smithy-http-server/src/proto/rest_xml/rejection.rs +++ b/rust-runtime/aws-smithy-http-server/src/proto/rest_xml/rejection.rs @@ -7,7 +7,7 @@ //! [`crate::proto::rest_json_1::rejection::RequestRejection::JsonDeserialize`] is swapped for //! [`RequestRejection::XmlDeserialize`]. -use crate::rejection::MissingContentTypeReason; +use crate::rejection::{MissingContentTypeReason, NotAcceptableReason}; use std::num::TryFromIntError; use thiserror::Error; @@ -28,8 +28,8 @@ pub enum RequestRejection { #[error("error converting non-streaming body to bytes: {0}")] BufferHttpBodyBytes(crate::Error), - #[error("request contains invalid value for `Accept` header")] - NotAcceptable, + #[error("request is not acceptable: {0}")] + NotAcceptable(#[from] NotAcceptableReason), #[error("expected `Content-Type` header not found: {0}")] MissingContentType(#[from] MissingContentTypeReason), diff --git a/rust-runtime/aws-smithy-http-server/src/protocols.rs b/rust-runtime/aws-smithy-http-server/src/protocols.rs index 2db5e7163d..0e655215c7 100644 --- a/rust-runtime/aws-smithy-http-server/src/protocols.rs +++ b/rust-runtime/aws-smithy-http-server/src/protocols.rs @@ -4,8 +4,8 @@ */ //! Protocol helpers. -use crate::rejection::MissingContentTypeReason; -use http::HeaderMap; +use crate::rejection::{MissingContentTypeReason, NotAcceptableReason}; +use http::{HeaderMap, HeaderValue}; /// When there are no modeled inputs, /// a request body is empty and the content-type request header must not be set @@ -66,17 +66,18 @@ pub fn content_type_header_classifier( Ok(()) } -pub fn accept_header_classifier(headers: &HeaderMap, content_type: &'static str) -> bool { +pub fn accept_header_classifier(headers: &HeaderMap, content_type: &'static str) -> Result<(), NotAcceptableReason> { if !headers.contains_key(http::header::ACCEPT) { - return true; + return Ok(()); } - // Must be of the form: type/subtype + // Must be of the form: `type/subtype`. let content_type = content_type .parse::() - .expect("BUG: MIME parsing failed, content_type is not valid"); - headers - .get_all(http::header::ACCEPT) - .into_iter() + // `expect` safety: content_type` is sent in from the generated server SDK and we know it's valid. + .expect("MIME parsing failed, `content_type` is not valid; please file a bug report under https://github.com/awslabs/smithy-rs/issues"); + let accept_headers = headers.get_all(http::header::ACCEPT); + let can_satisfy_some_accept_header = accept_headers + .iter() .flat_map(|header| { header .to_str() @@ -88,20 +89,30 @@ pub fn accept_header_classifier(headers: &HeaderMap, content_type: &'static str) * and remove the optional "; q=x" parameters * NOTE: the `unwrap`() is safe, because it takes the first element (if there's nothing to split, returns the string) */ - .flat_map(|s| s.split(',').map(|typ| typ.split(';').next().unwrap().trim())) + .flat_map(|s| s.split(',').map(|type_| type_.split(';').next().unwrap().trim())) }) .filter_map(|h| h.parse::().ok()) - .any(|mim| { - let typ = content_type.type_(); + .any(|mime| { + let type_ = content_type.type_(); let subtype = content_type.subtype(); // Accept: */*, type/*, type/subtype - match (mim.type_(), mim.subtype()) { - (t, s) if t == typ && s == subtype => true, - (t, mime::STAR) if t == typ => true, + match (mime.type_(), mime.subtype()) { + (t, s) if t == type_ && s == subtype => true, + (t, mime::STAR) if t == type_ => true, (mime::STAR, mime::STAR) => true, _ => false, } - }) + }); + if can_satisfy_some_accept_header { + Ok(()) + } else { + // We can't make `NotAcceptableReason`/`RequestRejection` borrow the header values because + // non-static lifetimes are not allowed in the source of an error, because + // `std::error::Error` requires the source is `dyn Error + 'static`. So we clone them into + // a vector in the case of a request rejection. + let cloned_accept_headers: Vec = accept_headers.into_iter().cloned().collect(); + Err(NotAcceptableReason::CannotSatisfyAcceptHeaders(cloned_accept_headers)) + } } #[cfg(test)] @@ -115,9 +126,9 @@ mod tests { headers } - fn req_accept(accept: &'static str) -> HeaderMap { + fn req_accept(accept: &str) -> HeaderMap { let mut headers = HeaderMap::new(); - headers.insert(ACCEPT, HeaderValue::from_static(accept)); + headers.insert(ACCEPT, HeaderValue::from_str(accept).unwrap()); headers } @@ -192,44 +203,80 @@ mod tests { assert!(matches!(result.unwrap_err(), MissingContentTypeReason::ToStrError(_))); } - #[test] - fn valid_accept_header_classifier_multiple_values() { - let valid_request = req_accept("text/strings, application/json, invalid"); - assert!(accept_header_classifier(&valid_request, "application/json")); - } + mod accept_header_classifier { + use super::*; - #[test] - fn invalid_accept_header_classifier() { - let invalid_request = req_accept("text/invalid, invalid, invalid/invalid"); - assert!(!accept_header_classifier(&invalid_request, "application/json")); - } + #[test] + fn valid_single_value() { + let valid_request = req_accept("application/json"); + assert!(accept_header_classifier(&valid_request, "application/json").is_ok()); + } - #[test] - fn valid_accept_header_classifier_star() { - let valid_request = req_accept("application/*"); - assert!(accept_header_classifier(&valid_request, "application/json")); - } + #[test] + fn valid_multiple_values() { + let valid_request = req_accept("text/strings, application/json, invalid"); + assert!(accept_header_classifier(&valid_request, "application/json").is_ok()); + } - #[test] - fn valid_accept_header_classifier_star_star() { - let valid_request = req_accept("*/*"); - assert!(accept_header_classifier(&valid_request, "application/json")); - } + #[test] + fn subtype_star() { + let valid_request = req_accept("application/*"); + assert!(accept_header_classifier(&valid_request, "application/json").is_ok()); + } - #[test] - fn valid_empty_accept_header_classifier() { - assert!(accept_header_classifier(&HeaderMap::new(), "application/json")); - } + #[test] + fn type_star_subtype_star() { + let valid_request = req_accept("*/*"); + assert!(accept_header_classifier(&valid_request, "application/json").is_ok()); + } - #[test] - fn valid_accept_header_classifier_with_params() { - let valid_request = req_accept("application/json; q=30, */*"); - assert!(accept_header_classifier(&valid_request, "application/json")); - } + #[test] + fn empty() { + assert!(accept_header_classifier(&HeaderMap::new(), "application/json").is_ok()); + } - #[test] - fn valid_accept_header_classifier() { - let valid_request = req_accept("application/json"); - assert!(accept_header_classifier(&valid_request, "application/json")); + #[test] + fn valid_with_params() { + let valid_request = req_accept("application/json; q=30, */*"); + assert!(accept_header_classifier(&valid_request, "application/json").is_ok()); + } + + #[test] + fn unstatisfiable_multiple_values() { + let accept_header_values = ["text/invalid, invalid, invalid/invalid"]; + let joined = accept_header_values.join(", "); + let invalid_request = req_accept(&joined); + match accept_header_classifier(&invalid_request, "application/json").unwrap_err() { + NotAcceptableReason::CannotSatisfyAcceptHeaders(returned_accept_header_values) => { + for header_value in accept_header_values { + let header_value = HeaderValue::from_str(header_value).unwrap(); + assert!(returned_accept_header_values.contains(&header_value)); + } + } + } + } + + #[test] + fn unstatisfiable_unparseable() { + let header_value = "foo_"; // Not a valid MIME type. + assert!(header_value.parse::().is_err()); + let invalid_request = req_accept(header_value); + match accept_header_classifier(&invalid_request, "application/json").unwrap_err() { + NotAcceptableReason::CannotSatisfyAcceptHeaders(returned_accept_header_values) => { + let header_value = HeaderValue::from_str(header_value).unwrap(); + assert!(returned_accept_header_values.contains(&header_value)); + } + } + } + + #[test] + #[should_panic] + fn panic_if_content_type_not_parseable() { + let header_value = "foo_"; // Not a valid MIME type. + let mut headers = HeaderMap::new(); + headers.insert(ACCEPT, HeaderValue::from_str(header_value).unwrap()); + assert!(header_value.parse::().is_err()); + let _res = accept_header_classifier(&headers, header_value); + } } } diff --git a/rust-runtime/aws-smithy-http-server/src/rejection.rs b/rust-runtime/aws-smithy-http-server/src/rejection.rs index 1f1e247435..2f114bc59e 100644 --- a/rust-runtime/aws-smithy-http-server/src/rejection.rs +++ b/rust-runtime/aws-smithy-http-server/src/rejection.rs @@ -4,6 +4,7 @@ */ use crate::response::IntoResponse; +use http::HeaderValue; use thiserror::Error; // This is used across different protocol-specific `rejection` modules. @@ -24,6 +25,13 @@ pub enum MissingContentTypeReason { }, } +// This is used across different protocol-specific `rejection` modules. +#[derive(Debug, Error)] +pub enum NotAcceptableReason { + #[error("cannot satisfy any of `Accept` header values: {0:?}")] + CannotSatisfyAcceptHeaders(Vec), +} + pub mod any_rejections { //! This module hosts enums, up to size 8, which implement [`IntoResponse`] when their variants implement //! [`IntoResponse`].