net/http: 302 redirect of DELETE to GET isn't RFC compliant

[ncw]

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

$ go version
go version go1.15 linux/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=""
GOCACHE="/home/ncw/.cache/go-build"
GOENV="/home/ncw/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/ncw/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/ncw/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/go/go1.15"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/go/go1.15/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
AR="ar"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build061961387=/tmp/go-build -gno-record-gcc-switches"

What did you do?

We used http.Client against a Microsoft Ondrive service.

When we issued a DELETE response the server responded with a 302 redirect and http.Client retried with a GET request.

We issue a DELETE request

2020/09/12 22:44:36 DEBUG : HTTP REQUEST (req 0xc00066b300)
2020/09/12 22:44:36 DEBUG : DELETE /v1.0/drives/b!-Jr3ZPe__kCZ7xLdl2FMoffWFSmIUF1AoWygTwxjtywWlD7heAFZQpK_8QbDA7ff/items/01HSWNV2LEGZAH5A2BLJE3R3I4ACF3QDRK HTTP/1.1
Host: graph.microsoft.com
User-Agent: rclone/v1.53.0
Authorization: XXXX
Accept-Encoding: gzip

The server responds with a 302 redirect

2020/09/12 22:44:36 DEBUG : HTTP RESPONSE (req 0xc00066b300)
2020/09/12 22:44:36 DEBUG : HTTP/1.1 302 Found
Cache-Control: private
Client-Request-Id: 9f245298-ad4b-4c78-b6c3-478208784d04
Content-Type: text/plain
Date: Sun, 13 Sep 2020 02:44:35 GMT
Location: https://redacted-my.sharepoint.com/_vti_bin/client.svc/v2.0/drives('b!-Jr3ZPe__kCZ7xLdl2FMoffWFSmIUF1AoWygTwxjtywWlD7heAFZQpK_8QbDA7ff')/items/01HSWNV2LEGZAH5A2BLJE3R3I4ACF3QDRK
Request-Id: 9f245298-ad4b-4c78-b6c3-478208784d04
Strict-Transport-Security: max-age=31536000
X-Ms-Ags-Diagnostic: {"ServerInfo":{"DataCenter":"North Central US","Slice":"SliceC","Ring":"3","ScaleUnit":"003","RoleInstance":"AGSFE_IN_30"}}
Content-Length: 0

We then do a GET request on the redirected URL which doesn't work.

2020/09/12 22:44:36 DEBUG : HTTP REQUEST (req 0xc0004be400)
2020/09/12 22:44:36 DEBUG : GET /_vti_bin/client.svc/v2.0/drives('b!-Jr3ZPe__kCZ7xLdl2FMoffWFSmIUF1AoWygTwxjtywWlD7heAFZQpK_8QbDA7ff')/items/01HSWNV2LEGZAH5A2BLJE3R3I4ACF3QDRK HTTP/1.1
Host: redacted-my.sharepoint.com
User-Agent: rclone/v1.53.0
Authorization: XXXX
Referer: https://graph.microsoft.com/v1.0/drives/b!-Jr3ZPe__kCZ7xLdl2FMoffWFSmIUF1AoWygTwxjtywWlD7heAFZQpK_8QbDA7ff/items/01HSWNV2LEGZAH5A2BLJE3R3I4ACF3QDRK
Accept-Encoding: gzip

What did you expect to see?

I expected http.Client to redirect with a DELETE request.

According to RFC7231

6.4.3. 302 Found

The 302 (Found) status code indicates that the target resource
resides temporarily under a different URI. Since the redirection
might be altered on occasion, the client ought to continue to use the
effective request URI for future requests.

The server SHOULD generate a Location header field in the response
containing a URI reference for the different URI. The user agent MAY
use the Location field value for automatic redirection. The server's
response payload usually contains a short hypertext note with a
hyperlink to the different URI(s).

  Note: For historical reasons, a user agent MAY change the request
  method from POST to GET for the subsequent request.  If this
  behavior is undesired, the 307 (Temporary Redirect) status code
  can be used instead.

I note that this only talks about POST to GET and not other methods in the historical reasons section.

So I'm not sure Go's implementation is RFC compliant here - the RFC doesn't say that user agents may change any other sort of method from POST to GET.

The code is here:

go/src/net/http/client.go

Lines 496 to 512 in 66e66e7

	// redirectBehavior describes what should happen when the
	// client encounters a 3xx status code from the server
	func redirectBehavior(reqMethod string, resp *Response, ireq *Request) (redirectMethod string, shouldRedirect, includeBody bool) {
	switch resp.StatusCode {
	case 301, 302, 303:
	redirectMethod = reqMethod
	shouldRedirect = true
	includeBody = false
	// RFC 2616 allowed automatic redirection only with GET and
	// HEAD requests. RFC 7231 lifts this restriction, but we still
	// restrict other methods to GET to maintain compatibility.
	// See Issue 18570.
	if reqMethod != "GET" && reqMethod != "HEAD" {
	redirectMethod = "GET"
	}
	case 307, 308:

Which says for all methods other than GET or HEAD, change them to GET, whereas the RFC says you can change POST to GET only.

So I think backwards compatibility might demand that POST becomes GET so something like this would be more RFC compliant.

		if reqMethod == "POST" {
			redirectMethod = "GET"
		}

The current code seems to think the major distinction between 301/302 and 307/308 is whether the body is resent. I'm not sure the RFC supports that, but using that distinction I think the code code could look like

		if ireq.outgoingLength() != 0 {
			redirectMethod = "GET"
		}

Reading the RFC it says nothing about request bodies vs headers. So I think to be most useful and most RFC compliant the 301,302 redirects should work just like the 307,308 redirects now we have the capability to resend the body if needed.

See: #18570
See: https://forum.rclone.org/t/purge-command-fails-on-onedrive/19048

[odeke-em]

Thank you for filing this issue @ncw!

So back in 2016, I implemented this change (on a fun working weekend collaboration with @bradfitz) and the basis for that unconditional change of DELETE->GET was an advisory from Microsoft on how Internet Explorer and other implementations changed the game despite being non-RFC complaint, and here is what guided us https://docs.microsoft.com/en-us/archive/blogs/ieinternals/http-methods-and-redirect-status-codes

and RFC 2616 does talk about the UNCONDITIONAL->GET caveat as per https://tools.ietf.org/html/rfc2616#section-10.3.3

and for posterity here is our guiding research that led us down that road https://docs.google.com/document/d/1LnWicNarwSdVWQ5RcgUOdHEGVcdR--Lravn6G0Hkg6c/

Suggestions to fix

This case clearly lands on the caveat of a modern server not handling the raised caveat, but in the 4+ years since, no one had reported this problem. Thus:

• It would be nice if perhaps that server could return a response to an OPTIONS request listing the supported HTTP methods in the redirectURL (please help me try making an OPTIONS request and paste in here the methods it allows) -- if the OPTIONS request returns a response, we could perhaps check for supported methods and then switch if it doesn't support GET
• Perhaps some sort of hook in which a user can supply their confirmation of the redirect method which bypasses the normal mechanism and would comply with the suggestion of user confirmation before changing the request method

[ncw]

Thanks for the history of this change :-)

This case clearly lands on the caveat of a modern server not handling the raised caveat, but in the 4+ years since, no one had reported this problem.

My first instinct was to create a bug report for onedrive asking for them to change the redirect to 307. However on reading the RFC I decided that I didn't really have a strong case - they would surely say that the Go http client is behaving strangely.

What do you think?

Note that the RFC you referenced 2616 is obsoleted by the one I referenced 7231 and 7231 doesn't contain the wording here from 2616 as far as I can see

  Note: RFC 1945 and RFC 2068 specify that the client is not allowed
  to change the method on the redirected request.  However, most
  existing user agent implementations treat 302 as if it were a 303
  response, performing a GET on the Location field-value regardless
  of the original request method. The status codes 303 and 307 have
  been added for servers that wish to make unambiguously clear which
  kind of reaction is expected of the client.

It would be nice if perhaps that server could return a response to an OPTIONS request listing the supported HTTP methods in the redirectURL (please help me try making an OPTIONS request and paste in here the methods it allows) -- if the OPTIONS request returns a response, we could perhaps check for supported methods and then switch if it doesn't support GET

I don't think that will be a good solution in this case. The server responds to GET requests just fine, however it returns a very unexpected error.

2020/09/12 22:44:36 DEBUG : HTTP RESPONSE (req 0xc0004be400)
2020/09/12 22:44:36 DEBUG : HTTP/2.0 401 Unauthorized
Content-Length: 123
Date: Sun, 13 Sep 2020 02:44:37 GMT
Microsoftsharepointteamservices: 16.0.0.20502
Ms-Cv: n3l4Yu6AALBwpBYmq77maA.0
P3p: CP="ALL IND DSP COR ADM CONo CUR CUSo IVAo IVDo PSA PSD TAI TELo OUR SAMo CNT COM INT NAV ONL PHY PRE PUR UNI"
Request-Id: 6278799f-80ee-b000-70a4-1626abbee668
Spiislatency: 0
Sprequestduration: 12
Sprequestguid: 6278799f-80ee-b000-70a4-1626abbee668
Strict-Transport-Security: max-age=31536000
Www-Authenticate: Bearer realm="001ad6f8-df91-4a35-a943-fc2bd42ce603",client_id="00000003-0000-0ff1-ce00-000000000000",trusted_issuers="00000001-0000-0000-c000-000000000000@*,D3776938-3DBA-481F-A652-4BEDFCAB7CD8@*,https://sts.windows.net/*/,00000003-0000-0ff1-ce00-000000000000@90140122-8516-11e1-8eff-49304924019b",authorization_uri="https://login.windows.net/common/oauth2/authorize"
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Ms-Invokeapp: 1; RequireReadOnly
X-Ms-Suspended-Features: features=""
X-Msedge-Ref: Ref A: ECC6763BD9A84911930CFDE578B1E896 Ref B: BN3EDGE0415 Ref C: 2020-09-13T02:44:37Z
X-Powered-By: ASP.NET

{"error":{"innerError":{"code":"invalidSignature"},"code":"unauthenticated","message":"Token contains invalid signature."}}

Perhaps some sort of hook in which a user can supply their confirmation of the redirect method which bypasses the normal mechanism and would comply with the suggestion of user confirmation before changing the request method

I think you can do that with CheckRedirect in http.Client and that would be my workaround.