HTTP 406 when request header includes "accept: */*"

[mcmasn-amzn]

It seems the generated server code is overly strict about requiring Accept: application/json. By default, clients like curl often send Accept: */*, but this produces a HTTP 406 response. This doesn't seem like the right behavior.

HTTP 1.1 RFC for Accept header - https://httpwg.org/specs/rfc7231.html#header.accept

Details

This occurs consistently with aws-smithy-http-server = "0.44". If I use the generated code for an simple service like this

Smithy

namespace test1
use aws.protocols#restJson1

@restJson1
service MySampleService {
    version: "2022-06-01",
    operations: [EmptyOperation]
}

@readonly
@http(uri: "/empty-operation", method: "GET")
operation EmptyOperation {
    input: EmptyOperationInput,
    output: EmptyOperationOutput,
}

@input
structure EmptyOperationInput { }

@output
structure EmptyOperationOutput { }

I believe the origin of the problem is the generated code I found in operation.rs

#[derive(Debug)]
pub(crate) struct EmptyOperationOperationInputWrapper(crate::input::EmptyOperationInput);
#[async_trait::async_trait]
impl<B> aws_smithy_http_server::request::FromRequest<B> for EmptyOperationOperationInputWrapper
where
    B: aws_smithy_http_server::body::HttpBody + Send, 
    B::Data: Send,
    aws_smithy_http_server::rejection::RequestRejection : From<<B as aws_smithy_http_server::body::HttpBody>::Error>
{
    type Rejection = aws_smithy_http_server::runtime_error::RuntimeError;
    async fn from_request(req: &mut aws_smithy_http_server::request::RequestParts<B>) -> Result<Self, Self::Rejection> {
        if let Some(headers) = req.headers() {
                        if let Some(accept) = headers.get(http::header::ACCEPT) {
                            if accept != "application/json" {
                                return Err(Self::Rejection {
                                    protocol: aws_smithy_http_server::protocols::Protocol::RestJson1,
                                    kind: aws_smithy_http_server::runtime_error::RuntimeErrorKind::NotAcceptable,
                                })
                            }
                        }
                    }
        crate::operation_deser::parse_empty_operation_request(req)
            .await
            .map(EmptyOperationOperationInputWrapper)
            .map_err(
                |err| aws_smithy_http_server::runtime_error::RuntimeError {
                    protocol: aws_smithy_http_server::protocols::Protocol::RestJson1,
                    kind: err.into()
                }
            )
    }
}

Fails with Accept: */*

curl -v http://localhost:3000/empty-operation
*   Trying 127.0.0.1:3000...
* Connected to localhost (127.0.0.1) port 3000 (#0)
> GET /empty-operation HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.79.1
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 406 Not Acceptable
< content-type: application/json
< x-amzn-errortype: NotAcceptableException
< content-length: 2
< date: Fri, 08 Jul 2022 21:44:02 GMT
< 
* Connection #0 to host localhost left intact
{}%    

Passes with Accept: application/json

curl -H 'Accept: application/json'  -v http://localhost:3000/empty-operation
*   Trying 127.0.0.1:3000...
* Connected to localhost (127.0.0.1) port 3000 (#0)
> GET /empty-operation HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.79.1
> Accept: application/json
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< content-type: application/json
< content-length: 2
< date: Fri, 08 Jul 2022 21:49:51 GMT
< 
* Connection #0 to host localhost left intact
{}

[jamesls]

Oh nice, I just ran into this exact same issue.

To give a concrete use case, I am unable to use the generated typescript clients from smithy-typescript in the browser, they fail with a 406 response because they send Accept: */*.

On whatever end this gets fixed, it seems like the smithy-typescript clients should be able to talk to the smithy-rs servers when generated against the same model.

[dmcallas]

Just to add in one more failing case: the client I'm using sends accept:application/json, text/javascript, */*. Even though this contains the correct application/json type, it also returns a 406 error since the generated server code is looking for an exact string match with application/json.

[david-perez]

I'm thinking that to fix this, we could implement a tower::Layer that correctly rejects requests when the Accept header does not include a configured value. This parsing logic is beneficial to the entire Rust Tower community, not only smithy-rs users.

[82marbag]

With the PR to tower http, it would become like this. Should we still have the check on our side, instead of asking users to add that layer manually?

[david-perez]

@82marbag We can't rely on users wrapping their Smithy service in the appropriate layer. It's also something that needs to be applied in codegen at the operation level. My idea is that we eventually vend it as middleware, which implements both tower::Layer and tower::Service. External users would apply the layer. In smithy-rs, we would call is_ready and call on the Service right where we're performing the current check.

We'd also need to map from whatever response the Service returns into the smithy-rs response body type, honoring the protocol too, which would be tricky.

Take the RequestBodyLimit middleware from tower_http as an example; it's likely that our middleware would be very similar, since both middlewares reject requests based on an HTTP header. If you look at the Service implementation, notice that if the request is rejected, a ResponseFuture::payload_too_large() response is returned, which just returns a text response with body length limit exceeded and a 413 status code. smithy-rs can't use an off-the-shelf layer like this one, it has additional requirements: it needs to return a protocol-specific response.

The more I think about it the more I think we should just have a simple fn accept_header_classifier(req: http:Request<B>, content_type: String) -> bool that says whether the request should be rejected in smithy-rs. The Tower middleware could then be written in terms of that logic.

[82marbag]

Fixed now, you can pull from main or wait for the next release. Keeping this open for future refactoring with tower

[david-perez]

Can we add some protocol tests?

[82marbag]

https://github.com/awslabs/smithy/pull/1365, https://github.com/awslabs/smithy-rs/pull/1648

[david-perez]

We're rejecting requests for operations that don't have modeled operation output if Accept is not set to the default protocol's response type (or a superset e.g. */*): is this correct? I think so, because {} is returned in this case, but I'm not sure. For example, the Healthcheck operation from the Pokémon Service currently generates:

impl HealthCheckOperationOperationInputWrapper {
    pub async fn from_request<B>(
        req: &mut aws_smithy_http_server::request::RequestParts<B>,
    ) -> Result<Self, aws_smithy_http_server::runtime_error::RuntimeError>
    where
        B: aws_smithy_http_server::body::HttpBody + Send,
        B::Data: Send,
        aws_smithy_http_server::rejection::RequestRejection:
            From<<B as aws_smithy_http_server::body::HttpBody>::Error>,
    {
        if !aws_smithy_http_server::protocols::accept_header_classifier(req, "application/json") {
            return Err(aws_smithy_http_server::runtime_error::RuntimeError {
                protocol: aws_smithy_http_server::protocols::Protocol::RestJson1,
                kind: aws_smithy_http_server::runtime_error::RuntimeErrorKind::NotAcceptable,
            });
        }
        crate::operation_deser::parse_health_check_operation_request(req)  » impl Future<Output = Result<…, …>>
            .await  » Result<HealthCheckOperationInput, …>
            .map(HealthCheckOperationOperationInputWrapper)  » Result<HealthCheckOperationOperationInputWrapper, …>
            .map_err(|err| aws_smithy_http_server::runtime_error::RuntimeError {
                protocol: aws_smithy_http_server::protocols::Protocol::RestJson1,
                kind: err.into(),
            })
    }
}

[david-perez]

I opened a consultation with the Smithy team: https://github.com/awslabs/smithy/issues/1376