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: How should net http handler behave when Content-Length header is incorrect? #45801

Closed
richzw opened this issue Apr 27, 2021 · 6 comments
Closed

net/http: How should net http handler behave when Content-Length header is incorrect? #45801

richzw opened this issue Apr 27, 2021 · 6 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Unreleased

Comments

@richzw
Copy link

richzw commented Apr 27, 2021

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

$ go version 
1.15.11

Does this issue reproduce with the latest release?

maybe

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/zangw/Library/Caches/go-build"
GOENV="/Users/zangw/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/zangw/go/pkg/mod"
GONOPROXY="github.com/Zentertain"
GONOSUMDB="github.com/Zentertain"
GOOS="darwin"
GOPATH="/Users/zangw/go"
GOPRIVATE="github.com/Zentertain"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/zangw/go115/go1.15.11"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/zangw/go115/go1.15.11/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/zangw/go/src/test/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 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/x3/bcr5s9sj1jqcxd19w9fk4j1c0000gn/T/go-build269433144=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

For server codes

import "net/http"

func defaultHandler(w http.ResponseWriter, r *http.Request) {
	bodyBytes, _ := ioutil.ReadAll(r.Body)
	fmt.Printf("Get request body %+v", string(bodyBytes))
	fmt.Fprint(w, "Hello Golang")
}

http.HandleFunc("/", defaultHandler)
http.ListenAndServe(":8080", nil)

Post HTTP packet with incorrect Content-Length through curl

time curl -v -X POST -H "Content-Type: application/json" -H "Content-Length: 1000" -d '{"key": "value"}' 'http://localhost:8080/'

Output

* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> POST / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.54.0
> Accept: */*
> Content-Type: application/json
> Content-Length: 1000
> 
* upload completely sent off: 16 out of 16 bytes
^C
curl -v -X POST -H "Content-Type: application/json" -H "Content-Length: 1000"  0.01s user 0.01s system 0% cpu 26.100 total

Server log output:

Get request body {"key": "value"}

enter image description here

Through Wireshark packet capture, we can see response OK is given from server in line 11. IMO, since the client send TCP segment of a reassembled PDU (line 5), server should NOT handle it in defaultHandler until the whole packet is received. However, server handle this partial packet. Is it correct behavior?

What did you expect to see?

The server should not give a 200 OK response

What did you see instead?

The server give a 200 OK response
@gopherbot gopherbot added this to the Unreleased milestone Apr 27, 2021
@richzw richzw changed the title x/net: Should handle incomplete http packet in net http handler? x/net: How should net http handler behave when Content-Length header is incorrect? Apr 27, 2021
@davecheney
Copy link
Contributor

bodyBytes, _ := ioutil.ReadAll(r.Body)

Your sample code ignores the error consuming the body. What happens if you consult the error?

@richzw
Copy link
Author

richzw commented Apr 27, 2021

@davecheney

Add error handling codes as

func defaultHandler(w http.ResponseWriter, r *http.Request) {
	bodyBytes, err := ioutil.ReadAll(r.Body)
	if err != nil {
		fmt.Printf("defaultHandler with err %+v \n", err)
		w.WriteHeader(400)
		return
	}
	fmt.Printf("Get request body %+v", string(bodyBytes))
	fmt.Fprint(w, "Hello Golang")
}

Now the HTTP code 400 is captured in WireShark.

@cherrymui
Copy link
Member

@richzw in the issue title you mentioned x/net, but the code snippets above uses the net/http package. Does your code use golang.org/x/net packages? Which package do you think the issue is in? Thanks.

@cherrymui cherrymui added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 27, 2021
@richzw
Copy link
Author

richzw commented Apr 27, 2021

@cherrymui the net/http is used in my test code. Sorry for misleading you.

@richzw richzw changed the title x/net: How should net http handler behave when Content-Length header is incorrect? net/http: How should net http handler behave when Content-Length header is incorrect? Apr 27, 2021
@davecheney
Copy link
Contributor

@richzw is there anything left to do in this ticket? It seems that if the error from io.ReadAll(r.Body) is checked the truncated request body is not an issue.

@richzw
Copy link
Author

richzw commented Apr 28, 2021

@davecheney It is not an issue after checking the error from io.ReadAll, it is my bad not checking this error. I will close it.

@richzw richzw closed this as completed Apr 28, 2021
@golang golang locked and limited conversation to collaborators Apr 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 WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@davecheney @richzw @gopherbot @cherrymui