diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index fc4c4c2578..662a6b6d2c 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -9,4 +9,17 @@ # message = "Fix typos in module documentation for generated crates" # references = ["smithy-rs#920"] # meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"} -# author = "rcoh" \ No newline at end of file +# author = "rcoh" +[[smithy-rs]] +message = """Fix request `Content-Type` header checking + +Two bugs related to how servers were checking the `Content-Type` header in incoming requests have been fixed: + +1. `Content-Type` header checking was incorrectly succeeding when no `Content-Type` header was present but one was expected. +2. When a shape was @httpPayload`-bound, `Content-Type` header checking occurred even when no payload was being sent. In this case it is not necessary to check the header, since there is no content. + +This is a breaking change in that servers are now stricter at enforcing the expected `Content-Type` header is being sent by the client in general, and laxer when the shape is bound with `@httpPayload`. +""" +references = ["smithy-rs#3690"] +meta = { "breaking" = true, "tada" = false, "bug" = true, "target" = "server"} +author = "david-perez" diff --git a/codegen-client-test/build.gradle.kts b/codegen-client-test/build.gradle.kts index de4b54d5b3..ac03a03490 100644 --- a/codegen-client-test/build.gradle.kts +++ b/codegen-client-test/build.gradle.kts @@ -67,7 +67,16 @@ val allCodegenTests = listOf( ClientTest( "aws.protocoltests.restjson#RestJsonExtras", "rest_json_extras", - dependsOn = listOf("rest-json-extras.smithy"), + dependsOn = listOf( + "rest-json-extras.smithy", + // TODO(https://github.com/smithy-lang/smithy/pull/2310): Can be deleted when consumed in next Smithy version. + "rest-json-extras-2310.smithy", + // TODO(https://github.com/smithy-lang/smithy/pull/2314): Can be deleted when consumed in next Smithy version. + "rest-json-extras-2314.smithy", + // TODO(https://github.com/smithy-lang/smithy/pull/2315): Can be deleted when consumed in next Smithy version. + // TODO(https://github.com/smithy-lang/smithy/pull/2331): Can be deleted when consumed in next Smithy version. + "rest-json-extras-2315.smithy", + ), ), ClientTest("aws.protocoltests.misc#MiscService", "misc", dependsOn = listOf("misc.smithy")), ClientTest("aws.protocoltests.restxml#RestXml", "rest_xml", addMessageToErrors = false), diff --git a/codegen-core/common-test-models/rest-json-extras-2310.smithy b/codegen-core/common-test-models/rest-json-extras-2310.smithy new file mode 100644 index 0000000000..afd8a1c443 --- /dev/null +++ b/codegen-core/common-test-models/rest-json-extras-2310.smithy @@ -0,0 +1,35 @@ +$version: "1.0" + +namespace aws.protocoltests.restjson + +use aws.protocols#restJson1 +use smithy.test#httpMalformedRequestTests + +@http(method: "POST", uri: "/MalformedContentTypeWithBody") +operation MalformedContentTypeWithBody2 { + input: GreetingStruct +} + +structure GreetingStruct { + salutation: String, +} + +apply MalformedContentTypeWithBody2 @httpMalformedRequestTests([ + { + id: "RestJsonWithBodyExpectsApplicationJsonContentTypeNoHeaders", + documentation: "When there is modeled input, the content type must be application/json", + protocol: restJson1, + request: { + method: "POST", + uri: "/MalformedContentTypeWithBody", + body: "{}", + }, + response: { + code: 415, + headers: { + "x-amzn-errortype": "UnsupportedMediaTypeException" + } + }, + tags: [ "content-type" ] + } +]) diff --git a/codegen-core/common-test-models/rest-json-extras-2314.smithy b/codegen-core/common-test-models/rest-json-extras-2314.smithy new file mode 100644 index 0000000000..3ff64f1b95 --- /dev/null +++ b/codegen-core/common-test-models/rest-json-extras-2314.smithy @@ -0,0 +1,39 @@ +$version: "1.0" + +namespace aws.protocoltests.restjson + +use aws.protocols#restJson1 +use smithy.test#httpRequestTests + +/// This example serializes a blob shape in the payload. +/// +/// In this example, no JSON document is synthesized because the payload is +/// not a structure or a union type. +@http(uri: "/HttpPayloadTraits", method: "POST") +operation HttpPayloadTraits2 { + input: HttpPayloadTraitsInputOutput, + output: HttpPayloadTraitsInputOutput +} + +apply HttpPayloadTraits2 @httpRequestTests([ + { + id: "RestJsonHttpPayloadTraitsWithBlobAcceptsNoContentType", + documentation: """ + Servers must accept no content type for blob inputs + without the media type trait.""", + protocol: restJson1, + method: "POST", + uri: "/HttpPayloadTraits", + body: "This is definitely a jpeg", + bodyMediaType: "application/octet-stream", + headers: { + "X-Foo": "Foo", + }, + params: { + foo: "Foo", + blob: "This is definitely a jpeg" + }, + appliesTo: "server", + tags: [ "content-type" ] + } +]) diff --git a/codegen-core/common-test-models/rest-json-extras-2315.smithy b/codegen-core/common-test-models/rest-json-extras-2315.smithy new file mode 100644 index 0000000000..955bb14c96 --- /dev/null +++ b/codegen-core/common-test-models/rest-json-extras-2315.smithy @@ -0,0 +1,133 @@ +$version: "2.0" + +namespace aws.protocoltests.restjson + +use smithy.test#httpRequestTests +use smithy.test#httpResponseTests +use smithy.test#httpMalformedRequestTests +use smithy.framework#ValidationException + +@http(uri: "/EnumPayload2", method: "POST") +@httpRequestTests([ + { + id: "RestJsonEnumPayloadRequest2", + uri: "/EnumPayload2", + headers: { "Content-Type": "text/plain" }, + body: "enumvalue", + params: { payload: "enumvalue" }, + method: "POST", + protocol: "aws.protocols#restJson1" + } +]) +@httpResponseTests([ + { + id: "RestJsonEnumPayloadResponse2", + headers: { "Content-Type": "text/plain" }, + body: "enumvalue", + params: { payload: "enumvalue" }, + protocol: "aws.protocols#restJson1", + code: 200 + } +]) +operation HttpEnumPayload2 { + input: EnumPayloadInput, + output: EnumPayloadInput + errors: [ValidationException] +} + +@http(uri: "/StringPayload2", method: "POST") +@httpRequestTests([ + { + id: "RestJsonStringPayloadRequest2", + uri: "/StringPayload2", + body: "rawstring", + bodyMediaType: "text/plain", + headers: { + "Content-Type": "text/plain", + }, + requireHeaders: [ + "Content-Length" + ], + params: { payload: "rawstring" }, + method: "POST", + protocol: "aws.protocols#restJson1" + } +]) +@httpResponseTests([ + { + id: "RestJsonStringPayloadResponse2", + headers: { "Content-Type": "text/plain" }, + body: "rawstring", + bodyMediaType: "text/plain", + params: { payload: "rawstring" }, + protocol: "aws.protocols#restJson1", + code: 200 + } +]) +@httpMalformedRequestTests([ + { + id: "RestJsonStringPayloadNoContentType2", + documentation: "Serializes a string in the HTTP payload without a content-type header", + protocol: "aws.protocols#restJson1", + request: { + method: "POST", + uri: "/StringPayload2", + body: "rawstring", + // We expect a `Content-Type` header but none was provided. + }, + response: { + code: 415, + headers: { + "x-amzn-errortype": "UnsupportedMediaTypeException" + } + }, + tags: [ "content-type" ] + }, + { + id: "RestJsonStringPayloadWrongContentType2", + documentation: "Serializes a string in the HTTP payload without the expected content-type header", + protocol: "aws.protocols#restJson1", + request: { + method: "POST", + uri: "/StringPayload2", + body: "rawstring", + headers: { + // We expect `text/plain`. + "Content-Type": "application/json", + }, + }, + response: { + code: 415, + headers: { + "x-amzn-errortype": "UnsupportedMediaTypeException" + } + }, + tags: [ "content-type" ] + }, + { + id: "RestJsonStringPayloadUnsatisfiableAccept2", + documentation: "Serializes a string in the HTTP payload with an unstatisfiable accept header", + protocol: "aws.protocols#restJson1", + request: { + method: "POST", + uri: "/StringPayload2", + body: "rawstring", + headers: { + "Content-Type": "text/plain", + // We can't satisfy this requirement; the server will return `text/plain`. + "Accept": "application/json", + }, + }, + response: { + code: 406, + headers: { + "x-amzn-errortype": "NotAcceptableException" + } + }, + tags: [ "accept" ] + }, +]) +operation HttpStringPayload2 { + input: StringPayloadInput, + output: StringPayloadInput +} diff --git a/codegen-core/common-test-models/rest-json-extras.smithy b/codegen-core/common-test-models/rest-json-extras.smithy index 73f45e7dfd..df1d8992ac 100644 --- a/codegen-core/common-test-models/rest-json-extras.smithy +++ b/codegen-core/common-test-models/rest-json-extras.smithy @@ -66,6 +66,13 @@ service RestJsonExtras { CaseInsensitiveErrorOperation, EmptyStructWithContentOnWireOp, QueryPrecedence, + // TODO(https://github.com/smithy-lang/smithy/pull/2314) + HttpPayloadTraits2, + // TODO(https://github.com/smithy-lang/smithy/pull/2310) + MalformedContentTypeWithBody2, + // TODO(https://github.com/smithy-lang/smithy/pull/2315) + HttpEnumPayload2, + HttpStringPayload2, ], errors: [ExtraError] } @@ -101,6 +108,7 @@ structure ExtraError {} id: "StringPayload", uri: "/StringPayload", body: "rawstring", + headers: { "Content-Type": "text/plain" }, params: { payload: "rawstring" }, method: "POST", protocol: "aws.protocols#restJson1" diff --git a/codegen-server-test/build.gradle.kts b/codegen-server-test/build.gradle.kts index cab36dbbcc..8c4d5618bc 100644 --- a/codegen-server-test/build.gradle.kts +++ b/codegen-server-test/build.gradle.kts @@ -64,7 +64,16 @@ val allCodegenTests = "../codegen-core/common-test-models".let { commonModels -> CodegenTest( "aws.protocoltests.restjson#RestJsonExtras", "rest_json_extras", - imports = listOf("$commonModels/rest-json-extras.smithy"), + imports = listOf( + "$commonModels/rest-json-extras.smithy", + // TODO(https://github.com/smithy-lang/smithy/pull/2310): Can be deleted when consumed in next Smithy version. + "$commonModels/rest-json-extras-2310.smithy", + // TODO(https://github.com/smithy-lang/smithy/pull/2314): Can be deleted when consumed in next Smithy version. + "$commonModels/rest-json-extras-2314.smithy", + // TODO(https://github.com/smithy-lang/smithy/pull/2315): Can be deleted when consumed in next Smithy version. + // TODO(https://github.com/smithy-lang/smithy/pull/2331): Can be deleted when consumed in next Smithy version. + "$commonModels/rest-json-extras-2315.smithy", + ), ), CodegenTest( "aws.protocoltests.restjson.validation#RestJsonValidation", diff --git a/codegen-server-test/python/build.gradle.kts b/codegen-server-test/python/build.gradle.kts index f9129a2fde..8fa340e7d4 100644 --- a/codegen-server-test/python/build.gradle.kts +++ b/codegen-server-test/python/build.gradle.kts @@ -61,7 +61,15 @@ val allCodegenTests = "../../codegen-core/common-test-models".let { commonModels CodegenTest( "aws.protocoltests.restjson#RestJsonExtras", "rest_json_extras", - imports = listOf("$commonModels/rest-json-extras.smithy"), + imports = listOf( + "$commonModels/rest-json-extras.smithy", + // TODO(https://github.com/smithy-lang/smithy/pull/2310): Can be deleted when consumed in next Smithy version. + "$commonModels/rest-json-extras-2310.smithy", + // TODO(https://github.com/smithy-lang/smithy/pull/2314): Can be deleted when consumed in next Smithy version. + "$commonModels/rest-json-extras-2314.smithy", + // TODO(https://github.com/smithy-lang/smithy/pull/2315): Can be deleted when consumed in next Smithy version. + "$commonModels/rest-json-extras-2315.smithy", + ), ), // TODO(https://github.com/smithy-lang/smithy-rs/issues/2477) // CodegenTest( diff --git a/codegen-server/python/src/test/kotlin/software/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonServerTypesTest.kt b/codegen-server/python/src/test/kotlin/software/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonServerTypesTest.kt index 44b83c3132..02c93347eb 100644 --- a/codegen-server/python/src/test/kotlin/software/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonServerTypesTest.kt +++ b/codegen-server/python/src/test/kotlin/software/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonServerTypesTest.kt @@ -130,6 +130,7 @@ internal class PythonServerTypesTest { let req = Request::builder() .method("POST") .uri("/echo") + .header("content-type", "application/json") .body(Body::from(${payload.dq()})) .unwrap(); @@ -222,6 +223,7 @@ internal class PythonServerTypesTest { let req = Request::builder() .method("POST") .uri("/echo") + .header("content-type", "application/json") .body(Body::from("{\"value\":1676298520}")) .unwrap(); let res = service.call(req).await.unwrap(); diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt index c35518319c..e06c81eb4d 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt @@ -812,6 +812,9 @@ class ServerProtocolTestGenerator( FailingTest(REST_JSON, "RestJsonEndpointTrait", TestType.Request), FailingTest(REST_JSON, "RestJsonEndpointTraitWithHostLabel", TestType.Request), FailingTest(REST_JSON, "RestJsonOmitsEmptyListQueryValues", TestType.Request), + // TODO(https://github.com/smithy-lang/smithy/pull/2315): Can be deleted when fixed tests are consumed in next Smithy version + FailingTest(REST_JSON, "RestJsonEnumPayloadRequest", TestType.Request), + FailingTest(REST_JSON, "RestJsonStringPayloadRequest", TestType.Request), // Tests involving `@range` on floats. // Pending resolution from the Smithy team, see https://github.com/smithy-lang/smithy-rs/issues/2007. FailingTest(REST_JSON_VALIDATION, "RestJsonMalformedRangeFloat_case0", TestType.MalformedRequest), 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 910acf4a70..f6ba45ab27 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 @@ -218,7 +218,7 @@ class ServerHttpBoundProtocolTraitImplGenerator( * we require the HTTP body to be fully read in memory before parsing or deserialization. * From a server perspective we need a way to parse an HTTP request from `Bytes` and serialize * an HTTP response to `Bytes`. - * These traits are the public entrypoint of the ser/de logic of the `aws-smithy-http-server` server. + * These traits are the public entrypoint of the ser/de logic of the generated server. */ private fun RustWriter.renderTraits( inputSymbol: Symbol, @@ -258,35 +258,6 @@ class ServerHttpBoundProtocolTraitImplGenerator( rustTemplate(init, *codegenScope) } } - // This checks for the expected `Content-Type` header if the `@httpPayload` trait is present, as dictated by - // the core Smithy library, which _does not_ require deserializing the payload. - // If no members have `@httpPayload`, the expected `Content-Type` header as dictated _by the protocol_ is - // checked later on for non-streaming operations, in `serverRenderShapeParser`: that check _does_ require at - // least buffering the entire payload, since the check must only be performed if the payload is empty. - val verifyRequestContentTypeHeader = - writable { - operationShape - .inputShape(model) - .members() - .find { it.hasTrait() } - ?.let { payload -> - val target = model.expectShape(payload.target) - if (!target.isBlobShape || target.hasTrait()) { - // `null` is only returned by Smithy when there are no members, but we know there's at least - // the one with `@httpPayload`, so `!!` is safe here. - val expectedRequestContentType = httpBindingResolver.requestContentType(operationShape)!! - rustTemplate( - """ - #{SmithyHttpServer}::protocol::content_type_header_classifier_http( - request.headers(), - Some("$expectedRequestContentType"), - )?; - """, - *codegenScope, - ) - } - } - } // Implement `from_request` trait for input types. val inputFuture = "${inputSymbol.name}Future" @@ -325,7 +296,6 @@ class ServerHttpBoundProtocolTraitImplGenerator( fn from_request(request: #{http}::Request) -> Self::Future { let fut = async move { #{verifyAcceptHeader:W} - #{verifyRequestContentTypeHeader:W} #{parse_request}(request) .await .map_err(Into::into) @@ -347,7 +317,6 @@ class ServerHttpBoundProtocolTraitImplGenerator( "parse_request" to serverParseRequest(operationShape), "verifyAcceptHeader" to verifyAcceptHeader, "verifyAcceptHeaderStaticContentTypeInit" to verifyAcceptHeaderStaticContentTypeInit, - "verifyRequestContentTypeHeader" to verifyRequestContentTypeHeader, ) // Implement `into_response` for output types. @@ -747,7 +716,6 @@ class ServerHttpBoundProtocolTraitImplGenerator( "RequestParts" to RuntimeType.smithyRuntimeApi(runtimeConfig).resolve("http::RequestParts"), ) val parser = structuredDataParser.serverInputParser(operationShape) - val noInputs = model.expectShape(operationShape.inputShape).expectTrait().originalId == null if (parser != null) { // `null` is only returned by Smithy when there are no members, but we know there's at least one, since @@ -768,41 +736,43 @@ class ServerHttpBoundProtocolTraitImplGenerator( ) } } + for (binding in bindings) { val member = binding.member val parsedValue = serverRenderBindingParser(binding, operationShape, httpBindingGenerator, structuredDataParser) + val valueToSet = + if (symbolProvider.toSymbol(binding.member).isOptional()) { + "Some(value)" + } else { + "value" + } if (parsedValue != null) { - rust("if let Some(value) = ") - parsedValue(this) - rust( + rustTemplate( """ - { - input = input.${member.setterName()}(${ - if (symbolProvider.toSymbol(binding.member).isOptional()) { - "Some(value)" - } else { - "value" - } - }); + if let Some(value) = #{ParsedValue:W} { + input = input.${member.setterName()}($valueToSet) } """, + "ParsedValue" to parsedValue, ) } } + serverRenderUriPathParser(this, operationShape) serverRenderQueryStringParser(this, operationShape) + // If there's no modeled operation input, some protocols require that `Content-Type` header not be present. + val noInputs = model.expectShape(operationShape.inputShape).expectTrait().originalId == null if (noInputs && protocol.serverContentTypeCheckNoModeledInput()) { - conditionalBlock("if body.is_empty() {", "}", conditional = parser != null) { - rustTemplate( - """ - #{SmithyHttpServer}::protocol::content_type_header_empty_body_no_modeled_input(&headers)?; - """, - *codegenScope, - ) - } + rustTemplate( + """ + #{SmithyHttpServer}::protocol::content_type_header_classifier_smithy(&headers, None)?; + """, + *codegenScope, + ) } + val err = if (ServerBuilderGenerator.hasFallibleBuilder( inputShape, @@ -850,14 +820,48 @@ class ServerHttpBoundProtocolTraitImplGenerator( *codegenScope, ) } else { + // This checks for the expected `Content-Type` header if the `@httpPayload` trait is present, as dictated by + // the core Smithy library, which _does not_ require deserializing the payload. + // If no members have `@httpPayload`, the expected `Content-Type` header as dictated _by the protocol_ is + // checked later on for non-streaming operations, in `serverRenderShapeParser`. + // Both checks require buffering the entire payload, since the check must only be performed if the payload is + // not empty. + val verifyRequestContentTypeHeader = + writable { + operationShape + .inputShape(model) + .members() + .find { it.hasTrait() } + ?.let { payload -> + val target = model.expectShape(payload.target) + if (!target.isBlobShape || target.hasTrait()) { + // `null` is only returned by Smithy when there are no members, but we know there's at least + // the one with `@httpPayload`, so `!!` is safe here. + val expectedRequestContentType = httpBindingResolver.requestContentType(operationShape)!! + rustTemplate( + """ + if !bytes.is_empty() { + #{SmithyHttpServer}::protocol::content_type_header_classifier_smithy( + &headers, + Some("$expectedRequestContentType"), + )?; + } + """, + *codegenScope, + ) + } + } + } rustTemplate( """ { let bytes = #{Hyper}::body::to_bytes(body).await?; + #{VerifyRequestContentTypeHeader:W} #{Deserializer}(&bytes)? } """, "Deserializer" to deserializer, + "VerifyRequestContentTypeHeader" to verifyRequestContentTypeHeader, *codegenScope, ) } diff --git a/rust-runtime/Cargo.lock b/rust-runtime/Cargo.lock index a42d64291b..4031ea6ebb 100644 --- a/rust-runtime/Cargo.lock +++ b/rust-runtime/Cargo.lock @@ -465,7 +465,7 @@ version = "0.60.3" [[package]] name = "aws-smithy-http-server" -version = "0.61.2" +version = "0.62.0" dependencies = [ "aws-smithy-http 0.60.8", "aws-smithy-json 0.60.7", diff --git a/rust-runtime/aws-smithy-http-server/Cargo.toml b/rust-runtime/aws-smithy-http-server/Cargo.toml index 18b12aaa01..c1b40cad68 100644 --- a/rust-runtime/aws-smithy-http-server/Cargo.toml +++ b/rust-runtime/aws-smithy-http-server/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "aws-smithy-http-server" -version = "0.61.2" +version = "0.62.0" authors = ["Smithy Rust Server "] edition = "2021" license = "Apache-2.0" diff --git a/rust-runtime/aws-smithy-http-server/src/protocol/mod.rs b/rust-runtime/aws-smithy-http-server/src/protocol/mod.rs index 0b3faf0136..e7eb963e98 100644 --- a/rust-runtime/aws-smithy-http-server/src/protocol/mod.rs +++ b/rust-runtime/aws-smithy-http-server/src/protocol/mod.rs @@ -13,7 +13,7 @@ pub mod rest_xml; use crate::rejection::MissingContentTypeReason; use aws_smithy_runtime_api::http::Headers as SmithyHeaders; use http::header::CONTENT_TYPE; -use http::{HeaderMap, HeaderValue}; +use http::HeaderMap; #[cfg(test)] pub mod test_helpers { @@ -46,81 +46,63 @@ fn parse_mime(content_type: &str) -> Result Result<(), MissingContentTypeReason> { - if headers.contains_key(http::header::CONTENT_TYPE) { - let found_mime = headers - .get(http::header::CONTENT_TYPE) - .unwrap() // The header is present, `unwrap` will not panic. - .parse::() - .map_err(MissingContentTypeReason::MimeParseError)?; - Err(MissingContentTypeReason::UnexpectedMimeType { - expected_mime: None, - found_mime: Some(found_mime), - }) - } else { - Ok(()) - } -} - -/// Checks that the `content-type` header is valid from a Smithy `Headers`. +/// Checks that the `content-type` header from a `SmithyHeaders` matches what we expect. #[allow(clippy::result_large_err)] pub fn content_type_header_classifier_smithy( headers: &SmithyHeaders, expected_content_type: Option<&'static str>, ) -> Result<(), MissingContentTypeReason> { - match headers.get(CONTENT_TYPE) { - Some(content_type) => content_type_header_classifier(content_type, expected_content_type), - None => Ok(()), - } + let actual_content_type = headers.get(CONTENT_TYPE); + content_type_header_classifier(actual_content_type, expected_content_type) } -/// Checks that the `content-type` header is valid from a `http::HeaderMap`. +/// Checks that the `content-type` header matches what we expect. #[allow(clippy::result_large_err)] -pub fn content_type_header_classifier_http( - headers: &HeaderMap, +fn content_type_header_classifier( + actual_content_type: Option<&str>, expected_content_type: Option<&'static str>, ) -> Result<(), MissingContentTypeReason> { - if let Some(content_type) = headers.get(http::header::CONTENT_TYPE) { - let content_type = content_type.to_str().map_err(MissingContentTypeReason::ToStrError)?; - content_type_header_classifier(content_type, expected_content_type) - } else { - Ok(()) + fn parse_expected_mime(expected_content_type: &str) -> mime::Mime { + let mime = expected_content_type + .parse::() + // `expected_content_type` comes from the codegen. + .expect("BUG: MIME parsing failed, `expected_content_type` is not valid; please file a bug report under https://github.com/smithy-lang/smithy-rs/issues"); + debug_assert_eq!( + mime, expected_content_type, + "BUG: expected `content-type` header value we own from codegen should coincide with its mime type; please file a bug report under https://github.com/smithy-lang/smithy-rs/issues", + ); + mime } -} -/// Checks that the `content-type` header is valid. -#[allow(clippy::result_large_err)] -fn content_type_header_classifier( - content_type: &str, - expected_content_type: Option<&'static str>, -) -> Result<(), MissingContentTypeReason> { - let found_mime = parse_mime(content_type)?; - // There is a `content-type` header. - // If there is an implied content type, they must match. - if let Some(expected_content_type) = expected_content_type { - let expected_mime = expected_content_type - .parse::() - // `expected_content_type` comes from the codegen. - .expect("BUG: MIME parsing failed, `expected_content_type` is not valid. Please file a bug report under https://github.com/smithy-lang/smithy-rs/issues"); - if expected_content_type != found_mime { - return Err(MissingContentTypeReason::UnexpectedMimeType { + match (actual_content_type, expected_content_type) { + (None, None) => Ok(()), + (None, Some(expected_content_type)) => { + let expected_mime = parse_expected_mime(expected_content_type); + Err(MissingContentTypeReason::UnexpectedMimeType { expected_mime: Some(expected_mime), + found_mime: None, + }) + } + (Some(actual_content_type), None) => { + let found_mime = parse_mime(actual_content_type)?; + Err(MissingContentTypeReason::UnexpectedMimeType { + expected_mime: None, found_mime: Some(found_mime), - }); + }) + } + (Some(actual_content_type), Some(expected_content_type)) => { + let expected_mime = parse_expected_mime(expected_content_type); + let found_mime = parse_mime(actual_content_type)?; + if expected_mime != found_mime { + Err(MissingContentTypeReason::UnexpectedMimeType { + expected_mime: Some(expected_mime), + found_mime: Some(found_mime), + }) + } else { + Ok(()) + } } - } else { - // `content-type` header and no modeled input (mismatch). - return Err(MissingContentTypeReason::UnexpectedMimeType { - expected_mime: None, - found_mime: Some(found_mime), - }); } - Ok(()) } pub fn accept_header_classifier(headers: &HeaderMap, content_type: &mime::Mime) -> bool { @@ -160,11 +142,10 @@ pub fn accept_header_classifier(headers: &HeaderMap, content_type: &mime::Mime) #[cfg(test)] mod tests { use super::*; - use aws_smithy_runtime_api::http::Headers; use http::header::{HeaderValue, ACCEPT, CONTENT_TYPE}; - fn req_content_type(content_type: &'static str) -> HeaderMap { - let mut headers = HeaderMap::new(); + fn req_content_type_smithy(content_type: &'static str) -> SmithyHeaders { + let mut headers = SmithyHeaders::new(); headers.insert(CONTENT_TYPE, HeaderValue::from_str(content_type).unwrap()); headers } @@ -175,75 +156,72 @@ mod tests { headers } - const EXPECTED_MIME_APPLICATION_JSON: Option<&'static str> = Some("application/json"); + const APPLICATION_JSON: Option<&'static str> = Some("application/json"); - #[test] - fn check_content_type_header_empty_body_no_modeled_input() { - assert!(content_type_header_empty_body_no_modeled_input(&Headers::new()).is_ok()); + // Validates the rejection type since we cannot implement `PartialEq` + // for `MissingContentTypeReason`. + fn assert_unexpected_mime_type( + result: Result<(), MissingContentTypeReason>, + actually_expected_mime: Option, + actually_found_mime: Option, + ) { + match result { + Ok(()) => panic!("content-type validation is expected to fail"), + Err(e) => match e { + MissingContentTypeReason::UnexpectedMimeType { + expected_mime, + found_mime, + } => { + assert_eq!(actually_expected_mime, expected_mime); + assert_eq!(actually_found_mime, found_mime); + } + _ => panic!("unexpected `MissingContentTypeReason`: {}", e), + }, + } } #[test] - fn check_invalid_content_type_header_empty_body_no_modeled_input() { - let mut valid = Headers::new(); - valid.insert(CONTENT_TYPE, "application/json"); - let result = content_type_header_empty_body_no_modeled_input(&valid).unwrap_err(); - assert!(matches!( - result, - MissingContentTypeReason::UnexpectedMimeType { - expected_mime: None, - found_mime: Some(_) - } - )); + fn check_valid_content_type() { + let headers = req_content_type_smithy("application/json"); + assert!(content_type_header_classifier_smithy(&headers, APPLICATION_JSON,).is_ok()); } #[test] fn check_invalid_content_type() { let invalid = vec!["application/jason", "text/xml"]; for invalid_mime in invalid { - let headers = req_content_type(invalid_mime); - let mut results = Vec::new(); - results.push(content_type_header_classifier_http( - &headers, - EXPECTED_MIME_APPLICATION_JSON, - )); - results.push(content_type_header_classifier_smithy( - &Headers::try_from(headers).unwrap(), - EXPECTED_MIME_APPLICATION_JSON, - )); + let headers = req_content_type_smithy(invalid_mime); + let results = vec![content_type_header_classifier_smithy(&headers, APPLICATION_JSON)]; - // Validates the rejection type since we cannot implement `PartialEq` - // for `MissingContentTypeReason`. + let actually_expected_mime = Some(parse_mime(APPLICATION_JSON.unwrap()).unwrap()); for result in results { - match result { - Ok(()) => panic!("Content-type validation is expected to fail"), - Err(e) => match e { - MissingContentTypeReason::UnexpectedMimeType { - expected_mime, - found_mime, - } => { - assert_eq!( - expected_mime.unwrap(), - "application/json".parse::().unwrap() - ); - assert_eq!(found_mime, invalid_mime.parse::().ok()); - } - _ => panic!("Unexpected `MissingContentTypeReason`: {}", e), - }, - } + let actually_found_mime = invalid_mime.parse::().ok(); + assert_unexpected_mime_type(result, actually_expected_mime.clone(), actually_found_mime); } } } #[test] - fn check_missing_content_type_is_allowed() { - let result = content_type_header_classifier_http(&HeaderMap::new(), EXPECTED_MIME_APPLICATION_JSON); - assert!(result.is_ok()); + fn check_missing_content_type_is_not_allowed() { + let actually_expected_mime = Some(parse_mime(APPLICATION_JSON.unwrap()).unwrap()); + let result = content_type_header_classifier_smithy(&SmithyHeaders::new(), APPLICATION_JSON); + assert_unexpected_mime_type(result, actually_expected_mime, None); + } + + #[test] + fn check_missing_content_type_is_expected() { + let headers = req_content_type_smithy(APPLICATION_JSON.unwrap()); + let actually_found_mime = Some(parse_mime(APPLICATION_JSON.unwrap()).unwrap()); + let actually_expected_mime = None; + + let result = content_type_header_classifier_smithy(&headers, None); + assert_unexpected_mime_type(result, actually_expected_mime, actually_found_mime); } #[test] fn check_not_parsable_content_type() { - let request = req_content_type("123"); - let result = content_type_header_classifier_http(&request, EXPECTED_MIME_APPLICATION_JSON); + let request = req_content_type_smithy("123"); + let result = content_type_header_classifier_smithy(&request, APPLICATION_JSON); assert!(matches!( result.unwrap_err(), MissingContentTypeReason::MimeParseError(_) @@ -252,9 +230,15 @@ mod tests { #[test] fn check_non_ascii_visible_characters_content_type() { - let request = req_content_type("application/💩"); - let result = content_type_header_classifier_http(&request, EXPECTED_MIME_APPLICATION_JSON); - assert!(matches!(result.unwrap_err(), MissingContentTypeReason::ToStrError(_))); + // Note that for Smithy headers, validation fails when attempting to parse the mime type, + // unlike with `http`'s `HeaderMap`, that would fail when checking the header value is + // valid (~ASCII string). + let request = req_content_type_smithy("application/💩"); + let result = content_type_header_classifier_smithy(&request, APPLICATION_JSON); + assert!(matches!( + result.unwrap_err(), + MissingContentTypeReason::MimeParseError(_) + )); } #[test] diff --git a/rust-runtime/aws-smithy-http-server/src/rejection.rs b/rust-runtime/aws-smithy-http-server/src/rejection.rs index 1f1e247435..ae3d2854fb 100644 --- a/rust-runtime/aws-smithy-http-server/src/rejection.rs +++ b/rust-runtime/aws-smithy-http-server/src/rejection.rs @@ -11,13 +11,9 @@ use thiserror::Error; pub enum MissingContentTypeReason { #[error("headers taken by another extractor")] HeadersTakenByAnotherExtractor, - #[error("no `Content-Type` header")] - NoContentTypeHeader, - #[error("`Content-Type` header value is not a valid HTTP header value: {0}")] - ToStrError(http::header::ToStrError), #[error("invalid `Content-Type` header value mime type: {0}")] MimeParseError(mime::FromStrError), - #[error("unexpected `Content-Type` header value; expected {expected_mime:?}, found {found_mime:?}")] + #[error("unexpected `Content-Type` header value; expected mime {expected_mime:?}, found mime {found_mime:?}")] UnexpectedMimeType { expected_mime: Option, found_mime: Option,