Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix request Content-Type header checking in servers #3690

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
# 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"
11 changes: 10 additions & 1 deletion codegen-client-test/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
35 changes: 35 additions & 0 deletions codegen-core/common-test-models/rest-json-extras-2310.smithy
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
$version: "1.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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: "{}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

},
response: {
code: 415,
headers: {
"x-amzn-errortype": "UnsupportedMediaTypeException"
}
},
tags: [ "content-type" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a appliesTo = server?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, @httpMalformedRequestTests only apply to servers.

}
])
39 changes: 39 additions & 0 deletions codegen-core/common-test-models/rest-json-extras-2314.smithy
Original file line number Diff line number Diff line change
@@ -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" ]
}
])
133 changes: 133 additions & 0 deletions codegen-core/common-test-models/rest-json-extras-2315.smithy
Original file line number Diff line number Diff line change
@@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
}
8 changes: 8 additions & 0 deletions codegen-core/common-test-models/rest-json-extras.smithy
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
Expand Down Expand Up @@ -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"
Expand Down
11 changes: 10 additions & 1 deletion codegen-server-test/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
10 changes: 9 additions & 1 deletion codegen-server-test/python/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Loading