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

Terminating HTTP CONNECT 200 response should not send "Transfer-Encoding: chunked" header #11227

Closed
chradcliffe opened this issue May 16, 2020 · 1 comment · Fixed by #11245
Closed
Assignees
Labels

Comments

@chradcliffe
Copy link
Contributor

chradcliffe commented May 16, 2020

When Envoy terminates an HTTP CONNECT tunnel (see #1451) and sends a 200 response, it sends a Transfer-Encoding: chunked header. This is a violation of the RFC-7231 which states:

A server MUST NOT send any Transfer-Encoding or Content-Length header fields in a 2xx (Successful) response to CONNECT.

It looks as if the request path knows to strip the transfer-encoding header, since in Http1::RequestEncoderImpl::encodeHeaders, there is a branch that disables chunked encoding when the method is CONNECT:

  } else if (method->value() == Headers::get().MethodValues.Connect) {
    disableChunkEncoding();
    connect_request_ = true;
  }

There is no such check in Http1::ResponseEncoderImpl::encodeHeaders but if I unconditionally call disableChunkEncoding() in that method, the the transfer-encoding header goes away.

Edit: it looks like is_response_to_connect_request_ does the right thing, so this is probably an easy fix. I can submit a PR for this.

@chradcliffe
Copy link
Contributor Author

cc @alyssawilk

alyssawilk added a commit that referenced this issue May 19, 2020
Risk Level: low (connect only)
Testing: UT, IT
Docs Changes: n/a
Release Notes: n/a
Fixes #11227
Part of #1451

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants