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

Jump to bottom

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
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.17

Comments

@ianwoolf
Copy link
Contributor

ianwoolf commented Apr 12, 2021 •
edited

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

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

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 12, 2021
@mknyszek mknyszek added this to the Backlog milestone Apr 12, 2021
@mknyszek
Copy link
Contributor

CC @bradfitz @rsc @empijei via https://dev.golang.org/owners

@mknyszek
Copy link
Contributor

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

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.

@borncrusader borncrusader mentioned this issue Apr 19, 2021
Closed
@networkimprov
Copy link

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

@dmitshur dmitshur modified the milestones: Backlog, Go1.17 May 21, 2021
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 21, 2021
@dmitshur
Copy link
Contributor

dmitshur commented May 21, 2021 •
edited

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

@borncrusader also reported this and requested a backport.

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
5 participants
@networkimprov @mknyszek @dmitshur @gopherbot @ianwoolf