Fix request `Content-Type` header checking in servers #3690

david-perez · 2024-06-11T16:33:13Z

This fixes two bugs:

Content-Type header checking was succeeding when no Content-Type
header was present but one was expected.
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.

Code has been refactored and cleaned up. The crux of the logic is now
easier to understand, and contained in content_type_header_classifier.

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This fixes two bugs: 1. `Content-Type` header checking was 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. Code has been refactored and cleaned up. The crux of the logic is now easier to understand, and contained in `content_type_header_classifier`.

github-actions · 2024-06-12T15:17:39Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
Client Test (ignoring whitespace)
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
Server Test Typescript (ignoring whitespace)

A new doc preview is ready to view.

drganjoo · 2024-06-13T12:28:13Z

codegen-core/common-test-models/rest-json-extras-2310.smithy

+        request: {
+            method: "POST",
+            uri: "/MalformedContentTypeWithBody",
+            body: "{}",


Do we need an additional test to verify that it's acceptable to omit the Content-Type header when the body is empty?

No, that's tested by https://github.com/david-perez/smithy/blob/bb492a6d16238f8f859b9a61ae2321dcc064787f/smithy-aws-protocol-tests/model/restJson1/empty-input-output.smithy#L50-L50.

drganjoo · 2024-06-13T12:30:00Z

codegen-core/common-test-models/rest-json-extras-2310.smithy

+                "x-amzn-errortype": "UnsupportedMediaTypeException"
+            }
+        },
+        tags: [ "content-type" ]


Do we need a appliesTo = server?

No, @httpMalformedRequestTests only apply to servers.

drganjoo · 2024-06-13T12:32:27Z

codegen-core/common-test-models/rest-json-extras-2310.smithy

@@ -0,0 +1,36 @@
+$version: "1.0"


A general question: are we sticking to version 1.0 or do we write all new tests in version 2.0 syntax?

We should write all new tests in 2.0 syntax. But since the tests in this file are copied from the ones upstream in Smithy (and they'll go away in the next Smithy release), this is OK, I just made the same modifications that I did upstream: smithy-lang/smithy#2310

drganjoo · 2024-06-13T12:32:55Z

codegen-core/common-test-models/rest-json-extras-2310.smithy

+    {
+        id: "RestJsonWithBodyExpectsApplicationJsonContentTypeNoHeaders",
+        documentation: """
+        When there is modeled input, the content type must be application/json""",


Nit: indentation

drganjoo · 2024-06-13T12:59:05Z

codegen-core/common-test-models/rest-json-extras-2315.smithy

+@httpRequestTests([
+    {
+        id: "RestJsonEnumPayloadRequest2",
+        uri: "/EnumPayload2",


Could we please add documentation key to the tests that are defined in this file?

These tests come from upstream, we should make the changes there.

drganjoo · 2024-06-13T13:26:41Z

rust-runtime/aws-smithy-http-server/src/protocol/mod.rs

+        let mime = expected_content_type
+                .parse::<mime::Mime>()
+                // `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");


Should we print the value of expected_content_type to make it easier to debug?

I'd rather not incur an unconditional format! heap-allocating call for something that should never happen.

drganjoo · 2024-06-13T14:20:24Z

...tware/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

+                        // 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


I'm curious, can't we simply check the Content-Length instead of buffering the entire payload?

No, Content-Length is best-effort, we should not rely on it for this. We could use it to fail early in some scenarios like these when it's set though.

drganjoo · 2024-06-13T14:33:40Z

rust-runtime/aws-smithy-http-server/src/protocol/mod.rs

-        content_type_header_classifier(content_type, expected_content_type)
-    } else {
-        Ok(())
+    fn parse_expected_mime(expected_content_type: &str) -> mime::Mime {


I'm curious, considering each protocol has a specific Content-Type MIME type, why do we parse the &str to Mime at runtime? Wouldn't it be more efficient to simply pass the mime::Mime to the function instead?

Yes, we could codegen a const Mime value for each expected Content-Type in a request. We already do this to check the Accept header (against the Content-Type of the response):

smithy-rs/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

Lines 243 to 261 in e912cf5

val verifyAcceptHeaderStaticContentTypeInit =

writable {

httpBindingResolver.responseContentType(operationShape)?.also { contentType ->

val init =

when (contentType) {

"application/json" -> "const $staticContentType: #{Mime}::Mime = #{Mime}::APPLICATION_JSON;"

"application/octet-stream" -> "const $staticContentType: #{Mime}::Mime = #{Mime}::APPLICATION_OCTET_STREAM;"

"application/x-www-form-urlencoded" -> "const $staticContentType: #{Mime}::Mime = #{Mime}::APPLICATION_WWW_FORM_URLENCODED;"

else ->

"""

static $staticContentType: #{OnceCell}::sync::Lazy<#{Mime}::Mime> = #{OnceCell}::sync::Lazy::new(|| {

${contentType.dq()}.parse::<#{Mime}::Mime>().expect("BUG: MIME parsing failed, content_type is not valid")

});

"""

}

rustTemplate(init, *codegenScope)

}

}

I opened #3703.

drganjoo · 2024-06-13T14:38:14Z

rust-runtime/aws-smithy-http-server/src/protocol/mod.rs

-            return Err(MissingContentTypeReason::UnexpectedMimeType {
+    match (actual_content_type, expected_content_type) {
+        (None, None) => Ok(()),
+        (None, Some(expected_content_type)) => {


What if the input is optional, and instead of sending us an empty body, the client sends no content at all? Since it's permissible to skip the Content-Type when the body is empty, the client also omits setting the Content-Type header. Wouldn't this scenario lead to failure?

No, because the codegen is such that we only call content_type_header_classifier_smithy when !bytes.is_empty().

drganjoo · 2024-06-13T14:57:03Z

...tware/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

+                            writable {
+                                operationShape
+                                    .inputShape(model)
+                                    .members()


Does Smithy allow decorating a member of a target shape with @httpPayload? If so, will members() return all descendants or just the immediate members of the input structure?

For instance,

structure ParentInput { x: DescendantShape } structure DescendantShape { @httpPayload y: String }

No, see smithy-lang/smithy#1379 (comment).

…-content-type-checking-in-servers

david-perez · 2024-06-18T11:31:36Z

A critical thing I missed was to bump aws-smithy-http-server's minor version to 0.62.0. Despite the changes having been made to #[doc(hidden)] functions, they are backwards-incompatible and used by the code-generated sSDK, so we can't publish them under 0.61.x.

github-actions · 2024-06-18T11:46:08Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
Client Test (ignoring whitespace)
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
Server Test Typescript (ignoring whitespace)

A new doc preview is ready to view.

## Motivation and Context This PR upgrades Smithy to 1.50.0. The majority of the changes follow `TODO` added in #3690. Other than that, a few adjustments needed to be made: - for the client - added two failing tests `RestJsonClientPopulatesDefaultValuesInInput` and `RestJsonClientUsesExplicitlyProvidedMemberValuesOverDefaults` to known failing tests for the same reason [here](https://github.com/smithy-lang/smithy-rs/blob/main/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ClientProtocolTestGenerator.kt#L72) - added one broken test (i.e. the upstream test definition is incorrect but our implementation is correct) to known broken tests per ([smithy#2341](smithy-lang/smithy#2341), [smithy-rs#3726](#3726 (comment))) - for the server - removed `rest-xml-extras.smithy` since `RestXmlMustSupportParametersInContentType` is now available upstream Smithy 1.50.0 - added the following to known failing tests (since the `awsJson1_0` counterparts are already in the list, but we need the server team to verify this assumption & provide additional `TODO` comments if necessary) - `RestJsonServerPopulatesDefaultsWhenMissingInRequestBody` - `RestJsonServerPopulatesDefaultsInResponseWhenMissingInParams`, - `RestJsonServerPopulatesNestedDefaultValuesWhenMissingInInResponseParams` ## Testing Existing tests in CI ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Zelda Hessler <zhessler@amazon.com>

@jjant

RPC v2 CBOR is a new protocol that ~is being added~ has [recently been added](https://smithy.io/2.0/additional-specs/protocols/smithy-rpc-v2.html) to the Smithy specification. _(I'll add more details here as the patchset evolves)_ Credit goes to @jjant for initial implementation of the router, which I built on top of from his [`jjant/smithy-rpc-v2-exploration`](https://github.com/awslabs/smithy-rs/tree/jjant/smithy-rpc-v2-exploration) branch. Tracking issue: #3573. ## Caveats `TODO`s are currently exhaustively sprinkled throughout the patch documenting what remains to be done. Most of these need to be addressed before this can be merged in; some can be punted on to not make this PR bigger. However, I'd like to call out the major caveats and blockers here. I'll keep updating this list as the patchset evolves. - [x] RPC v2 has still not been added to the Smithy specification. It is currently being worked on over in the [`smithy-rpc-v2`](https://github.com/awslabs/smithy/tree/smithy-rpc-v2) branch. The following are prerrequisites for this PR to be merged; **until they are done CI on this PR will fail**: - [x] Smithy merges in RPC v2 support. - [x] Smithy releases a new version incorporating RPC v2 support. - Released in [Smithy v1.47](https://github.com/smithy-lang/smithy/releases/tag/1.47.0) - [x] smithy-rs updates to the new version. - Updated in #3552 - [x] ~Protocol tests for the protocol do not currently exist in Smithy. Until those get written~, this PR resorts to Rust unit tests and integration tests that use `serde` to round-trip messages and compare `serde`'s encoders and decoders with ours for correctness. - Protocol tests are under the [`smithy-protocol-tests`](https://github.com/smithy-lang/smithy/tree/main/smithy-protocol-tests/model/rpcv2Cbor) directory in Smithy. - We're keeping the `serde_cbor` round-trip tests for defense in depth. - [ ] #3709 - Currently only server-side support has been implemented, because that's what I'm most familiar. However, we're almost all the way there to add client-side support. - ~[ ] [Smithy `document` shapes](https://smithy.io/2.0/spec/simple-types.html#document) are not supported. RPC v2's specification currently doesn't indicate how to implement them.~ - [The spec](https://smithy.io/2.0/additional-specs/protocols/smithy-rpc-v2.html#shape-serialization) ended up leaving them as unsupported: "Document types are not currently supported in this protocol." ## Prerequisite PRs This section lists prerequisite PRs and issues that would make the diff for this one lighter or easier to understand. It's preferable that these PRs be merged prior to this one; some are hard prerequisites. They mostly relate to parts of the codebase I've had to touch or ~pilfer~ inspect in this PR where I've made necessary changes, refactors and "drive-by improvements" that are mostly unrelated, although some directly unlock things I've needed in this patchset. It makes sense to pull them out to ease reviewability and make this patch more semantically self-contained. - #2516 - #2517 - #2522 - #2524 - #2528 - #2536 - #2537 - #2531 - #2538 - #2539 - #2542 - #3684 - #3678 - #3690 - #3713 - #3726 - #3752 ## Testing    ~RPC v2 has still not been added to the Smithy specification. It is currently being worked on over in the [`smithy-rpc-v2`](https://github.com/awslabs/smithy/tree/smithy-rpc-v2) branch.~ This can only be tested _locally_ following these steps: ~1. Clone [the Smithy repository](https://github.com/smithy-lang/smithy/tree/smithy-rpc-v2) and checkout the `smithy-rpc-v2` branch. 2. Inside your local checkout of smithy-rs pointing to this PR's branch, make sure you've added `mavenLocal()` as a repository in the `build.gradle.kts` files. [Example](8df82fd). 4. Inside your local checkout of Smithy's `smithy-rpc-v2` branch: 1. Set `VERSION` to the current Smithy version used in smithy-rs (1.28.1 as of writing, but [check here](https://github.com/awslabs/smithy-rs/blob/main/gradle.properties#L21)). 2. Run `./gradlew clean build pTML`.~ ~6.~ 1. In your local checkout of the smithy-rs's `smithy-rpc-v2` branch, run `./gradlew codegen-server-test:build -P modules='rpcv2Cbor'`. ~You can troubleshoot whether you have Smithy correctly set up locally by inspecting `~/.m2/repository/software/amazon/smithy/smithy-protocols-traits`.~ ## Checklist  - [ ] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._

david-perez added bug Something isn't working server Rust server SDK labels Jun 11, 2024

david-perez requested review from a team as code owners June 11, 2024 16:33

Fix things

e912cf5

drganjoo reviewed Jun 13, 2024

View reviewed changes

david-perez mentioned this pull request Jun 18, 2024

Code-generate const Mime values for each expected Content-Type in a request #3703

Open

david-perez added 3 commits June 18, 2024 13:23

Address comments

9f05a93

Merge remote-tracking branch 'upstream/main' into davidpz/fix-request…

95a58b6

…-content-type-checking-in-servers

Bump aws-smithy-http-server to 0.62.0

3d108fb

david-perez added the breaking-change This will require a breaking change label Jun 18, 2024

drganjoo approved these changes Jun 18, 2024

View reviewed changes

Velfi approved these changes Jun 18, 2024

View reviewed changes

david-perez added this pull request to the merge queue Jun 18, 2024

Merged via the queue into main with commit b583a2f Jun 18, 2024
44 checks passed

david-perez deleted the davidpz/fix-request-content-type-checking-in-servers branch June 18, 2024 16:39

david-perez mentioned this pull request Jun 19, 2024

Add server RPC v2 CBOR support #2544

Merged

7 tasks

ysaito1001 mentioned this pull request Jun 28, 2024

Upgrade Smithy to 1.50.0 #3728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix request `Content-Type` header checking in servers #3690

Fix request `Content-Type` header checking in servers #3690

david-perez commented Jun 11, 2024

github-actions bot commented Jun 12, 2024

drganjoo Jun 13, 2024

david-perez Jun 18, 2024

drganjoo Jun 13, 2024

david-perez Jun 18, 2024

drganjoo Jun 13, 2024

david-perez Jun 18, 2024

drganjoo Jun 13, 2024

drganjoo Jun 13, 2024

david-perez Jun 18, 2024

drganjoo Jun 13, 2024

david-perez Jun 18, 2024 •

edited

Loading

drganjoo Jun 13, 2024

david-perez Jun 18, 2024

drganjoo Jun 13, 2024

david-perez Jun 18, 2024

david-perez Jun 18, 2024

drganjoo Jun 13, 2024

david-perez Jun 18, 2024

drganjoo Jun 13, 2024

david-perez Jun 18, 2024

david-perez commented Jun 18, 2024

github-actions bot commented Jun 18, 2024

	val verifyAcceptHeaderStaticContentTypeInit =
	writable {
	httpBindingResolver.responseContentType(operationShape)?.also { contentType ->
	val init =
	when (contentType) {
	"application/json" -> "const $staticContentType: #{Mime}::Mime = #{Mime}::APPLICATION_JSON;"
	"application/octet-stream" -> "const $staticContentType: #{Mime}::Mime = #{Mime}::APPLICATION_OCTET_STREAM;"
	"application/x-www-form-urlencoded" -> "const $staticContentType: #{Mime}::Mime = #{Mime}::APPLICATION_WWW_FORM_URLENCODED;"
	else ->
	"""
	static $staticContentType: #{OnceCell}::sync::Lazy<#{Mime}::Mime> = #{OnceCell}::sync::Lazy::new(\|\| {
	${contentType.dq()}.parse::<#{Mime}::Mime>().expect("BUG: MIME parsing failed, content_type is not valid")
	});
	"""
	}
	rustTemplate(init, *codegenScope)
	}
	}

Fix request Content-Type header checking in servers #3690

Fix request Content-Type header checking in servers #3690

Conversation

david-perez commented Jun 11, 2024

Checklist

github-actions bot commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-perez Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-perez commented Jun 18, 2024

github-actions bot commented Jun 18, 2024

Fix request `Content-Type` header checking in servers #3690

Fix request `Content-Type` header checking in servers #3690

david-perez Jun 18, 2024 •

edited

Loading