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: Content-Length: 0 is not set for PATCH requests with empty body #40978

Closed
tharinduwijewardane opened this issue Aug 22, 2020 · 12 comments
Closed
Labels
Milestone

Comments

@tharinduwijewardane
Copy link

@tharinduwijewardane tharinduwijewardane commented Aug 22, 2020

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

1.15

Does this issue reproduce with the latest release?

yes

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

darwin amd64

go env Output
GO111MODULE=""
GOARCH="amd64"
GOBIN="/Users/tharindu/GoCode/bin"
GOCACHE="/Users/tharindu/Library/Caches/go-build"
GOENV="/Users/tharindu/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/tharindu/GoCode/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/tharindu/GoCode"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.15/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.15/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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/3l/cw1f6lvn6p1c_vcg0cnbmqwc0000gp/T/go-build547496291=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

package main

import (
	"fmt"
	"io/ioutil"
	"net/http"
	"strings"
)

func main() {

	url := "https://myapi.net/logs/file/a?action=aa"
	method := "PATCH"

	payload := strings.NewReader("")
	client := &http.Client {
	}
	req, err := http.NewRequest(method, url, payload)

	if err != nil {
		fmt.Println(err)
	}
	req.Header.Set("Authorization", "Bearer my-token")
	req.Header.Set("Content-Length", "0")

	res, err := client.Do(req)
	defer res.Body.Close()
	body, err := ioutil.ReadAll(res.Body)

	fmt.Println(string(body))
}

What did you expect to see?

Content-Length header getting set to 0 when the request is observed from the server side.

What did you see instead?

Content-Length header is not getting set for PATCH requests with empty/nil payload. It is getting set without any issue for other http methods (POST, PUT).
It is also getting set for PATCH requests if the payload is non empty.
Also setting req.Header.Set("Content-Length", "0") seems to have no effect on the actual content length getting set in the request.

@odeke-em odeke-em changed the title [Bug] Content-Length is not set for PATCH requests with empty body net/http: Content-Length is not set for PATCH requests with empty body Aug 24, 2020
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Aug 24, 2020

Thank you for filing this issue @tharinduwijewardane and welcome to the Go project!

I’d like to understand the motivation here. Was there a failure on some intermediate or front end server, say Nginx or is your service breaking due to lack of that 0? I ask because in 9+ years no one had reported this being a problem. Did you discover this perhaps just by a code audit, or an actual failure? Please let us know. The impetus for adding that 0 for POST and PUT is that RFC 2616 says we can include 0 only if the method doesn’t prohibit it, and Nginx apparently needs that too. However, nothing has been reported/requested for DELETE nor PATCH in the past 9 years (almost decade).

@odeke-em odeke-em changed the title net/http: Content-Length is not set for PATCH requests with empty body net/http: Content-Length: 0 is not set for PATCH requests with empty body Aug 24, 2020
@tharinduwijewardane
Copy link
Author

@tharinduwijewardane tharinduwijewardane commented Aug 24, 2020

@odeke-em I came across this when invoking the Azure data lake storage gen2 API (https://docs.microsoft.com/en-us/rest/api/storageservices/datalakestoragegen2/path/update) for flushing the content of a file which was uploaded in a previous request.
They deliberately reject the PATCH request when the Content-Length is not set to 0 even if there is no content. Maybe that is something they should not do.

However, at-least we should be able to manually set the content-length header right?
In this case req.Header.Set("Content-Length", "0") is not honoured by http package.
I think we should be able set whatever the header we need.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Aug 24, 2020

Thank you for providing the background, @tharinduwijewardane! I have cited your case in the fixing CL https://go-review.googlesource.com/c/go/+/250039. To get around this, firstly am away from my computer for a couple of hours but I’ve written for you a repro with a stand-alone server using my iPad here https://play.golang.org/p/43_KR2riBiT, please let me know what it prints out on your computer.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 24, 2020

Change https://golang.org/cl/250039 mentions this issue: net/http: set Content-Length:0 for empty PATCH requests as with POST, PATCH

@tharinduwijewardane
Copy link
Author

@tharinduwijewardane tharinduwijewardane commented Aug 24, 2020

@odeke-em "In server" the content-length header is not received.

PreRequest PATCH / HTTP/1.1
Host: 127.0.0.1:65209
Content-Length: 0


In server PATCH / HTTP/1.1
Host: 127.0.0.1:65209
Accept-Encoding: gzip
User-Agent: Go-http-client/1.1


Response HTTP/1.1 200 OK
Date: Mon, 24 Aug 2020 08:17:40 GMT
Content-Length: 0
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Aug 24, 2020

Thank you for running them! Given that you are the sole reporter in the past 9 years, I don’t think that this could qualify for a backport, but would it be alright for you to use go tip for the next 3+ months until Go1.16 is released? If not, let me know and I can craft up for you a way of hacking in that Content-Length:0 or perhaps request backports to the last 2 Go releases

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 24, 2020

Are you sure that rfc2616 defines patch? From my research it was proposed in 2010 in https://tools.ietf.org/html/rfc5789

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Aug 24, 2020

@tharinduwijewardane
Copy link
Author

@tharinduwijewardane tharinduwijewardane commented Aug 24, 2020

@odeke-em Thank you. But this is no longer a blocker for me as there is a workaround/hack.
By setting req.TransferEncoding = []string{"identity"} I can get it to add the header.

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 24, 2020

It just says that as long as the method doesn’t forbid sending the Content-Length, it can be sent which is my interpretation to say that it doesn’t forbid it.

That’s probably for connection reuse to work.

What’s the use case for sending a PATCH with an empty body, surely the server will reject it?

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Aug 24, 2020

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 24, 2020

Ok, thanks for confirming

@odeke-em odeke-em added the NeedsFix label Aug 24, 2020
@odeke-em odeke-em added this to the Go1.16 milestone Aug 24, 2020
@gopherbot gopherbot closed this in fb5c3ea Aug 24, 2020
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.

4 participants
You can’t perform that action at this time.