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 return error or not delete Host Header #45513

Closed
ianwoolf opened this issue Apr 12, 2021 · 9 comments
Closed

net/http: ReadRequest should return error or not delete Host Header #45513

ianwoolf opened this issue Apr 12, 2021 · 9 comments
Labels
Milestone

Comments

@ianwoolf
Copy link
Contributor

@ianwoolf ianwoolf commented Apr 12, 2021

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

$ go version
go version go1.16.2 darwin/amd64

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

go env Output
$ go env
GO111MODULE="auto"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/xx/Library/Caches/go-build"
GOENV="/Users/xx/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/xx/code/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/xx/code/go"
GOPRIVATE=""
GOPROXY="https://goproxy.io,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/xx/code/go/src/go/src/go.mod"
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/3f/x8jl0lgx629d_5bz0537gxnm0000gn/T/go-build908271374=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I noticed that http.readRequest has a param called deleteHostHeader to control whether to delete the Host of Request.Header. It seems to be designed so that http.ReadRequest can skip the Host check.

But if http.ReadRequest sets deleteHostHeader to true, the first Host Header is read in readRequest and the Host Header is deleted, which makes http.ReadRequest lose the rest of the Host Header.

I don't understand whether the processing of the Host Header is to be skipped, or the Host Header needs to be check and return error when there is multi Host Header. So I think the processing here can be more clear.

related to #45463

What did you expect to see?

Clear processing strategy for multiple Host Headers. For example, http.Request do not delete the Host Header, or reject multiple Host Header Requests

What did you see instead?

http.Request do not check the Host Header and delete the Host in the Header after getting the first Host Header

@ianwoolf ianwoolf changed the title net/http: net/http: http.ReadRequest should reject requests with multiple Host headers Apr 12, 2021
@ianwoolf ianwoolf changed the title net/http: http.ReadRequest should reject requests with multiple Host headers net/http: http.ReadRequest should return error or not delete Host Header Apr 12, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 12, 2021

Change https://golang.org/cl/308952 mentions this issue: net/http: ReadRequest return error when requests has multiple Host headers

@mknyszek mknyszek added this to the Backlog milestone Apr 12, 2021
@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Apr 12, 2021

If I understand correctly, this is more of just a cleanup, right? Are there any functional changes that your CL makes?

@ianwoolf
Copy link
Contributor Author

@ianwoolf ianwoolf commented Apr 14, 2021

There is a function called http.ReadRequest that is not called by other functions. It is exposed for specialized applications. I made a small change. When the Header has multiple Host, the return is changed from nil to error.

In fact, this function does not allow multiple Host Headers. Because this function only read the first Host Header, and directly discards the rest Host Header. So I agree with what Dave said in #45463, it is more appropriate to return an error when there are multiple Host Headers.

@networkimprov
Copy link

@networkimprov networkimprov commented May 16, 2021

@dmitshur we have a request to backport this, see #45463

@dmitshur dmitshur modified the milestones: Backlog, Go1.17 May 21, 2021
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 21, 2021

Thanks. I closed that issue as a duplicate, since this one is more up to date on the current state of this bug.

This bug is fixed in Go 1.17, I've updated the milestone accordingly. I don't see a backport request at this time. If you or someone believes this fix should be backported to Go 1.16 and 1.15 and meets the criteria described at https://golang.org/wiki/MinorReleases, please make a backport request using the process documented there.

@dmitshur dmitshur changed the title net/http: http.ReadRequest should return error or not delete Host Header net/http: ReadRequest should return error or not delete Host Header May 21, 2021
@networkimprov
Copy link

@networkimprov networkimprov commented May 22, 2021

@borncrusader also reported this and requested a backport.

@gopherbot
Copy link

@gopherbot gopherbot commented May 28, 2021

Change https://golang.org/cl/311569 mentions this issue: doc/go1.17: document text/template.DeferFuncCheck

@gopherbot
Copy link

@gopherbot gopherbot commented May 28, 2021

Change https://golang.org/cl/311569 mentions this issue: doc/go1.17: document text/template/parse.SkipFuncCheck

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

Successfully merging a pull request may close this issue.

None yet
5 participants