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

net/http: include Content-Length header for 400 responses #61348

Closed
kenballus opened this issue Jul 14, 2023 · 9 comments
Closed

net/http: include Content-Length header for 400 responses #61348

kenballus opened this issue Jul 14, 2023 · 9 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@kenballus
Copy link

What version of Go are you using (go version)?

$ go version
go version devel go1.21-089e37a931 Thu Jul 13 23:11:42 2023 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/root/.cache/go-build'
GOENV='/root/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/root/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/root/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/app/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/app/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.21-089e37a931 Thu Jul 13 23:11:42 2023 +0000'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='0'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2856339654=/tmp/go-build -gno-record-gcc-switches'

What did you do?

  1. Run the example server from the Go docs.
  2. Send it the following request:
GET / HTTP/1.1\r\n
\r\n

What did you expect to see?

A valid 400 response informing me that my request needs a Host header.

What did you see instead?

The following 400 response that is invalid because it is missing its Content-Length header:

HTTP/1.1 400 Bad Request: missing required Host header\r\n
Content-Type: text/plain; charset=utf-8\r\n
Connection: close\r\n
\r\n
400 Bad Request: missing required Host header
@kenballus
Copy link
Author

Caddy also has this bug, but I'm assuming that's because they use net/http.

@bcmills
Copy link
Contributor

bcmills commented Jul 14, 2023

invalid because it is missing its Content-Length header:

Is a Content-Length header field actually required for a response that specifies Connection: close? (What leads you to believe that it is invalid?)

RFC 9110 says:

A user agent SHOULD send Content-Length in a request when the method defines a meaning for enclosed content and it is not sending Transfer-Encoding.

(Note that that is “SHOULD”, not “MUST”.)

And RFC 9112 says:

Otherwise [if no Content-Length or Transfer-Encoding header field is present], this is a response message without a declared message body length, so the message body length is determined by the number of octets received prior to the server closing the connection.

As far as I can tell, per RFC 9112 this is a valid HTTP/1.1 response.

(CC @neild)

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 14, 2023
@bcmills bcmills added this to the Unplanned milestone Jul 14, 2023
@neild
Copy link
Contributor

neild commented Jul 14, 2023

I concur.

In addition, RFC 9110 says:

[...] in the absence of Transfer-Encoding, an origin server SHOULD send a Content-Length header field when the content size is known prior to sending the complete header section. This will allow downstream recipients to measure transfer progress, know when a received message is complete, and potentially reuse the connection for additional requests.

This response is small enough that measuring progress is unnecessary (the response should fit in a single packet), and the connection is closed immediately after the response so connection reuse is impossible. It would not be incorrect for us to send a Content-Length here, but I believe this is a valid HTTP/1.1 response.

@kenballus
Copy link
Author

kenballus commented Jul 14, 2023

Thank you for the quick responses.

I agree that the spec only suggests the inclusion of a TE or CL header; I was mistaken.

That said, net/http is the only HTTP library of which I am aware that does not send a TE or CL header in its responses.

Among those that do send TE or CL headers in 400 responses are

  • AIOHTTP
  • Apache HTTPD
  • Boost::Beast
  • Gunicorn
  • H2O
  • IIS
  • Jetty
  • Ligghtpd
  • NGINX
  • Node.js
  • OpenLitespeed
  • Puma
  • Tomcat
  • Tornado

Given that the spec says that you SHOULD do this, I stand by my suggestion that we move away from net/http's current behavior.

EDIT: Typo

@bcmills bcmills changed the title net/http: Content-Length header missing from 400 responses net/http: include Content-Length header for 400 responses Jul 14, 2023
@bcmills bcmills added FeatureRequest and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jul 14, 2023
@neild
Copy link
Contributor

neild commented Jul 14, 2023

What's the benefit in sending Content-Length here?

If there are existing broken client implementations in common use that can't cope with this response, then that might be an argument for change, but I'm dubious that this particular form of buggy client is common.

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jul 14, 2023
@kenballus
Copy link
Author

kenballus commented Jul 17, 2023

The benefit of sending CL is for clarity and ease of parsing.

This is the top answer to a StackOverflow question asking what happens when an HTTP response has no CL header:

In HTTP/1.0 - server response without content-length is when the stream closes
In HTTP/1.1 - server response without content-length is when the response is chunked encoded

Believing this answer would lead to exactly the sort of bug you describe.

That said, I think I'm convinced that the best course of action is not to add a CL header, but to remove the message bodies from the default 400 responses. They're redundant because of the reason string, and removing them should improve performance marginally, and improve compatibility with other servers.

@neild
Copy link
Contributor

neild commented Jul 17, 2023

The Stack Overflow response is wrong.

In the absence of evidence that the current behavior is causing real world problems, I believe that not only is there no good reason to change it, but there is a small but real value in preserving it as a minor discouragement to the development of new, buggy HTTP/1.1 client implementations that cannot handle this valid response.

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jul 17, 2023
@kenballus
Copy link
Author

I am aware that the SO response is wrong; I sent it to demonstrate that my misunderstanding of the standard is a widespread one.

If you'd like net/http to be the lone upholder of a little-known part of the HTTP standard, then that is your choice, and I will close the issue.

@kenballus
Copy link
Author

Here's another portion of the standard that encourages being explicit about lengths, from RFC 9112 Section 6.3:

Since there is no way to distinguish a successfully completed, close-delimited response message from a partially received message interrupted by network failure, a server SHOULD generate encoding or length-delimited messages whenever possible. The close-delimiting feature exists primarily for backwards compatibility with HTTP/1.0.

Thus, a client will have no way of determining with reasonable certainty that it received the entirety of net/http's error response when it receives a response with no Content-Length.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

3 participants