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: ReadRequest should reject requests with multiple Host headers #45463

Closed
borncrusader opened this issue Apr 9, 2021 · 10 comments
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@borncrusader
Copy link

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

$ go version
go version go1.16 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/Users//go-workspace/bin"
GOCACHE="/Users//Library/Caches/go-build"
GOENV="/Users//Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users//go-workspace/pkg/mod"
GOOS="darwin"
GOPATH="/Users//go-workspace"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users//.asdf/installs/golang/1.16/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users//.asdf/installs/golang/1.16/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/_z/0tl9x4tj43981q938qh7xqhw0000gp/T/go-build492873196=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

http.ReadRequest doesn't account for potentially multiple Host headers in the request. This is a snippet of the code to reproduce this issue - https://play.golang.org/p/vZ71i09DAHb

According to the HTTP Spec (https://tools.ietf.org/html/rfc7230#section-5.4),

A server MUST respond with a 400 (Bad Request) status code to any
HTTP/1.1 request message that lacks a Host header field and to any
request message that contains more than one Host header field or a
Host header field with an invalid field-value.

However, this is appropriately handled in https://github.com/golang/go/blob/master/src/net/http/server.go#L1007. But clients attempting to parse an HTTP request using http.ReadRequest have no means to detect the presence of multiple Host headers since the Host header is promoted to http.Request.Host and is not part of the http.Request.Header map.

What did you expect to see?

Expect to have a means by which one can detect the presence of multiple Host headers in the HTTP request when using http.ReadRequest.

What did you see instead?

The http.Request returned by the method does not have any indication to the presence of multiple Host headers. It also returns a nil error.

@davecheney
Copy link
Contributor

Does ReadRequest return an error if there are multiple Host headers in the request?

@davecheney
Copy link
Contributor

Thank you for your sample. Given that ReadRequest rejects the request, are you asking for the error to be changed to perhaps indicate the multiple header values presented?

Asked another way, if we changed http.Request to capture multiple Host headers, how would that change the program you are trying to write?

@borncrusader
Copy link
Author

@davecheney - it does not return an error when there are multiple Host headers present. The code sample's err == nil code runs.

The issue is it does not return an error nor reflect in the req.Header fields that there were multiple Host headers present.

@borncrusader
Copy link
Author

To answer your latter question, I want to be able to use the API to parse an incoming byte stream and return an error if it has multiple Host headers.

@davecheney
Copy link
Contributor

@borncrusader thank you for your reply. Given this is a violation of the RFC, maybe the best resolution of this issue is to make ReadRequest reject such requests.

@davecheney davecheney added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 9, 2021
@borncrusader
Copy link
Author

Thanks @davecheney for your prompt reply!

@davecheney davecheney changed the title http.ReadRequest ignores the presence of multiple HTTP host headers http.ReadRequest should reject requests with multiple Host headers Apr 9, 2021
@seankhliao seankhliao changed the title http.ReadRequest should reject requests with multiple Host headers net/http: ReadRequest should reject requests with multiple Host headers Apr 9, 2021
@borncrusader
Copy link
Author

Looks like #45513 addresses this as well. I believe this will be considered for 1.16.4. Please correct me if I'm wrong, @davecheney

@networkimprov
Copy link

Missed by triage?

cc @neild @fraenkel @empijei

@fraenkel
Copy link
Contributor

1.17 returns an error, too many Host headers
issue #45513

@dmitshur
Copy link
Contributor

dmitshur commented May 21, 2021

I'll close this issue because it is a duplicate of issue #45513. Let's continue the conversation there. (Please let me know if I misunderstood or missed something.)

@golang golang locked and limited conversation to collaborators May 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants