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

Open
borncrusader opened this issue Apr 9, 2021 · 7 comments
Open

Comments

@borncrusader
Copy link

@borncrusader borncrusader commented Apr 9, 2021

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

@davecheney davecheney commented Apr 9, 2021

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

@davecheney
Copy link
Contributor

@davecheney davecheney commented Apr 9, 2021

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

@borncrusader borncrusader commented Apr 9, 2021

@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

@borncrusader borncrusader commented Apr 9, 2021

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

@davecheney davecheney commented Apr 9, 2021

@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.

@borncrusader
Copy link
Author

@borncrusader borncrusader commented Apr 9, 2021

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

@borncrusader borncrusader commented Apr 19, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants