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.

Sign up for GitHub

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

http/client - edge case on POST and redirect #70174

Closed
Shnitzelil opened this issue Nov 2, 2024 · 7 comments
Closed

http/client - edge case on POST and redirect #70174

Shnitzelil opened this issue Nov 2, 2024 · 7 comments

Comments

@Shnitzelil
Copy link

Go version

1.23.2

Output of go env in your module/workspace:

set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\kgana\AppData\Local\go-build
set GOENV=C:\Users\kgana\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\<my user>\go\pkg\mod
set GONOPROXY=gitlab.<my org>.net
set GONOSUMDB=gitlab.<my org>.net
set GOOS=windows
set GOPATH=C:\Users\<my user>\go
set GOPRIVATE=gitlab.otxlab.net
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:/Program Files/Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=local
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.23.2
set GODEBUG=
set GOTELEMETRY=local
set GOTELEMETRYDIR=C:\Users\<my user>\AppData\Roaming\go\telemetry
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\scm\<my project>\go.mod
set GOWORK=
set CGO_CFLAGS=-IC:/scm/ZMQ/include
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-LC:/scm/ZMQ/bin
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\<my user>\AppData\Local\Temp\go-build2742211471=/tmp/go-build -gno-record-gcc-switches

What did you do?

Using simple http client to execute POST with body the server response with series of redirect and enter loop (till reached to 10th redirects).

I used Burp (Fiddler like) to see the communication b/w the client and server.

What did you see happen?

The go client received several redirect responses
302 -> 301 -> 308 -> 302 -> 301 -> 308... until it reached to the 10th redirects.

What did you expect to see?

It was supposed to stop after the 308.
The reason is simple it should send the last one w/o body and Content Length.

I was able to modify the Go SDK (net/http/client.go) with this change on function redirectBehavior

// 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:
		redirectMethod = reqMethod
		shouldRedirect = true
		includeBody = true

		if ireq.GetBody == nil && ireq.outgoingLength() != 0 {
			// We had a request body, and 307/308 require
			// re-sending it, but GetBody is not defined. So just
			// return this response to the user instead of an
			// error, like we did in Go 1.7 and earlier.
			shouldRedirect = false
		}
		
		if reqMethod == "GET" && ireq.Method == "POST" {
			includeBody = false
		}
	}
	return redirectMethod, shouldRedirect, includeBody
}
@gabyhelp
Copy link

gabyhelp commented Nov 2, 2024

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@seankhliao
Copy link
Member

see https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/308

The request method and the body will not be modified by the client in the redirected request. A 301 Moved Permanently requires the request method and the body to remain unchanged when redirection is performed, but this is incorrectly handled by older clients to use the GET method instead.

modifying the request is incorrect for 308

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2024
@Shnitzelil
Copy link
Author

So how it come and change my method to GET?

@seankhliao
Copy link
Member

that's the 301

@Shnitzelil
Copy link
Author

Expected behavior of handling HTTP 308 redirection is that the client must repeat the same type of request (either POST or GET) on the target location. And it works properly when server responds with HTTP 308 on POST request (the redirected request will be also POST with the same body and only different URL following value of Location header).
image
In our case the flow is different - after several "normal" redirections (following HTTP 302 and HTTP 301) client receives HTTP 308 response and following the logic should respond with the same method as in previous request (that was GET without body) to the new location.
This way it works in browsers.
Thus the handling of HTTP 308 response in our (and GO) code should refer the request that responded with HTTP 308 and not the initial one ("first") in the redirections sequence.

Please advise

@seankhliao
Copy link
Member

Our handling of redirects is consistent with other tools such as curl. If you need different behaviour, CheckRedirect is available for you to set.

@Shnitzelil
Copy link
Author

Shnitzelil commented Nov 3, 2024 •
edited
Loading

I've checked with curl...
Curl

  1. Send POST with Body and receive status code 302
  2. Send POST w/o Body and receive status code 200.

Go

  1. Send POST with Body and receive status code 302
  2. Send Get w/o Body and receive status code 301
  3. Send Get with Body and receive status code 308
  4. And back to 1

So you should check that.. there is something wrong with the redirections...

@gabyhelp gabyhelp mentioned this issue Nov 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Shnitzelil @seankhliao @gabyhelp