Skip to content

net/http: should not reuse body following redirects after method is changed to GET #70180

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

Closed
Shnitzelil opened this issue Nov 4, 2024 · 12 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.

Comments

@Shnitzelil
Copy link

Shnitzelil commented Nov 4, 2024

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\<username>\AppData\Local\go-build
set GOENV=C:\Users\<username>\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.<no longer exists>.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.

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.

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

Also tested with Python and Java and they (Curl, Java and Python received 200 OK)

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

@seankhliao
Copy link
Member

please include a reproducer with server code

@seankhliao seankhliao added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 4, 2024
@seankhliao seankhliao changed the title net/http/client - Endless loop on redirect net/http: endless loop on redirect Nov 4, 2024
@Shnitzelil
Copy link
Author

ok, will try to build snippet for the server side

@seankhliao seankhliao added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Nov 4, 2024
@Shnitzelil
Copy link
Author

Hi,

Here is the server like (this is not the origin but this how it behave)

package main

import (
	"fmt"
	"net/http"
)

const paramName = "token" 

const baseRedirectionUrlTemplate = "http://%v/location?token=%v"


func getHostnameFromHttpRequest(request *http.Request) string {
	host := request.Host
	//request.Host is empty on redirected requests
	if host == "" {
		host = request.URL.Host
	}
	return host
}
// Handler for the initial POST request
func handleLocation(w http.ResponseWriter, r *http.Request) {
	host := getHostnameFromHttpRequest(r)
	if r.ContentLength > 0 {
		w.Header().Set("Location", fmt.Sprintf(baseRedirectionUrlTemplate, host, "1"))
		w.WriteHeader(http.StatusFound)
		return
	}

	value := r.URL.Query().Get(paramName)
	switch value {
		case "1":
			w.Header().Set("Location", fmt.Sprintf(baseRedirectionUrlTemplate, host, "2"))
			w.WriteHeader(http.StatusMovedPermanently)
		case "2":
			w.Header().Set("Location", fmt.Sprintf(baseRedirectionUrlTemplate, host, "3"))
			w.WriteHeader(http.StatusPermanentRedirect)
		case "3":
			w.WriteHeader(http.StatusOK)
			_, _ = fmt.Fprintf(w, "Finally you complete the redirections")
	default:
		w.WriteHeader(http.StatusNotFound)
	}
}

func main() {
	// Set up the routes
	http.HandleFunc("/location", handleLocation)

	// Start the server
	fmt.Println("Server is running on port 8081...")
	if err := http.ListenAndServe(":8081", nil); err != nil {
		fmt.Println("Error starting server:", err)
	}
}

It worked with curl (I used the -x http://localhost:8080 to see the conversation on local proxy like Fiddler or Burp)

curl -X POST http://localhost:8081/location -H "Content-Type: application/json" -d "Dummay body" -x http://localhost:8080 -L

Or you can use Python (also here I've used proxy to see the conversation)

import requests

# URL and data
url = "http://localhost:8081/location"
data = {
    "emplid": 333333333,
    "userpassword": "Trinet@123"
}

# Proxy configuration
proxies = {
    "http": "http://localhost:8080",
    "https": "http://localhost:8080"
}

# Sending the POST request
response = requests.post(url, json=data, proxies=proxies)

# Printing the response
print("Status Code:", response.status_code)
print("Response Body:", response.text)

@Shnitzelil
Copy link
Author

But if you use golang you will fail with this error
Error sending request: Post "http://localhost:8081/location?token=1": stopped after 10 redirects

package main

import (
	"bytes"
	"encoding/json"
	"fmt"
	"net/http"
	"net/http/cookiejar"
	"net/url"
)

func main() {
	// Define the URL and JSON body
	targetURL := "http://localhost:8081/location"
	jsonBody := map[string]interface{}{
		"emplid":       333333333,
		"userpassword": "Trinet@123",
	}

	// Convert JSON body to bytes
	jsonData, err := json.Marshal(jsonBody)
	if err != nil {
		fmt.Println("Error marshalling JSON:", err)
		return
	}

	// Set up the proxy
	proxyURL, err := url.Parse("http://localhost:8080")
	if err != nil {
		fmt.Println("Error parsing proxy URL:", err)
		return
	}

	// Create transport with the proxy
	transport := &http.Transport{
		Proxy: http.ProxyURL(proxyURL),
	}

	// Create an HTTP client with the transport
	jar, _ := cookiejar.New(nil)
	client := &http.Client{
		Transport: transport,
		Jar:       jar,
	}

	// Create a new POST request
	req, err := http.NewRequest(http.MethodPost, targetURL, bytes.NewBuffer(jsonData))
	if err != nil {
		fmt.Println("Error creating request:", err)
		return
	}

	// Set the content type to application/json
	req.Header.Set("Content-Type", "application/json")

	// Send the request
	resp, err := client.Do(req)
	if err != nil {
		fmt.Println("Error sending request:", err)
		return
	}
	defer func() {
		if err = resp.Body.Close(); err != nil {
			fmt.Println(err)
		}
	}()

	// Print the response status
	fmt.Println("Response status:", resp.Status)

}

`Error sending request: Post "http://localhost:8081/location?token=1": stopped after 10 redirects`

@seankhliao seankhliao changed the title net/http: endless loop on redirect net/http: should not reuse body following redirects after method is changed to GET Nov 6, 2024
@seankhliao seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Nov 6, 2024
@seankhliao
Copy link
Member

cc @neild

I've modified the client invocations so they should be doing the same thing.
Both Go and curl do POST->GET, but Go reuses the body further down the chain.

curl:

curl http://localhost:8081/location -H "Content-Type: application/json" -d "request body" -v -L
curl request log
Server is running on port 8081...
POST /location HTTP/1.1
Host: localhost:8081
Accept: */*
Content-Length: 12
Content-Type: application/json
User-Agent: curl/8.10.1

request body
====== done ======
GET /location?token=1 HTTP/1.1
Host: localhost:8081
Accept: */*
Content-Type: application/json
User-Agent: curl/8.10.1


====== done ======
GET /location?token=2 HTTP/1.1
Host: localhost:8081
Accept: */*
Content-Type: application/json
User-Agent: curl/8.10.1


====== done ======
GET /location?token=3 HTTP/1.1
Host: localhost:8081
Accept: */*
Content-Type: application/json
User-Agent: curl/8.10.1


====== done ======

go

go client
package main

import (
	"context"
	"fmt"
	"net/http"
	"strings"
)

func main() {
	// Define the URL and JSON body
	targetURL := "http://localhost:8081/location"

	// Create an HTTP client with the transport
	client := &http.Client{}

	ctx := context.Background()

	// Create a new POST request
	req, err := http.NewRequestWithContext(ctx, http.MethodPost, targetURL, strings.NewReader("request body"))
	if err != nil {
		fmt.Println("Error creating request:", err)
		return
	}

	// Set the content type to application/json
	req.Header.Set("Content-Type", "application/json")

	// Send the request
	resp, err := client.Do(req)
	if err != nil {
		fmt.Println("Error sending request:", err)
		return
	}
	defer func() {
		if err = resp.Body.Close(); err != nil {
			fmt.Println(err)
		}
	}()

	// Print the response status
	fmt.Println("Response status:", resp.Status)
}
go request log
Server is running on port 8081...
POST /location HTTP/1.1
Host: localhost:8081
Accept-Encoding: gzip
Content-Length: 12
Content-Type: application/json
User-Agent: Go-http-client/1.1

request body
====== done ======
GET /location?token=1 HTTP/1.1
Host: localhost:8081
Accept-Encoding: gzip
Content-Type: application/json
Referer: http://localhost:8081/location
User-Agent: Go-http-client/1.1


====== done ======
GET /location?token=2 HTTP/1.1
Host: localhost:8081
Accept-Encoding: gzip
Content-Type: application/json
Referer: http://localhost:8081/location?token=1
User-Agent: Go-http-client/1.1


====== done ======
GET /location?token=3 HTTP/1.1
Host: localhost:8081
Accept-Encoding: gzip
Content-Length: 12
Content-Type: application/json
Referer: http://localhost:8081/location?token=2
User-Agent: Go-http-client/1.1

request body
====== done ======
GET /location?token=1 HTTP/1.1
Host: localhost:8081
Accept-Encoding: gzip
Content-Type: application/json
Referer: http://localhost:8081/location?token=3
User-Agent: Go-http-client/1.1


====== done ======
GET /location?token=2 HTTP/1.1
Host: localhost:8081
Accept-Encoding: gzip
Content-Type: application/json
Referer: http://localhost:8081/location?token=1
User-Agent: Go-http-client/1.1


====== done ======
GET /location?token=3 HTTP/1.1
Host: localhost:8081
Accept-Encoding: gzip
Content-Length: 12
Content-Type: application/json
Referer: http://localhost:8081/location?token=2
User-Agent: Go-http-client/1.1

request body
====== done ======
GET /location?token=1 HTTP/1.1
Host: localhost:8081
Accept-Encoding: gzip
Content-Type: application/json
Referer: http://localhost:8081/location?token=3
User-Agent: Go-http-client/1.1


====== done ======
GET /location?token=2 HTTP/1.1
Host: localhost:8081
Accept-Encoding: gzip
Content-Type: application/json
Referer: http://localhost:8081/location?token=1
User-Agent: Go-http-client/1.1


====== done ======
GET /location?token=3 HTTP/1.1
Host: localhost:8081
Accept-Encoding: gzip
Content-Length: 12
Content-Type: application/json
Referer: http://localhost:8081/location?token=2
User-Agent: Go-http-client/1.1

request body
====== done ======

@neild
Copy link
Contributor

neild commented Nov 6, 2024

To summarize:

  • When a client receives a 301 in response to a POST, the redirect request should be a GET with no body.
  • When a client receives a 308 in response to a POST, the redirect request should be a POST with the original body.

This behavior is specified at: https://fetch.spec.whatwg.org/#http-redirect-fetch

When the net/http client follows a chain of redirects (for example, a 301 leading to a 308), it handles a 308 by sending the original request rather than the modified one:

  • Send POST with body
  • Get 301, send GET with no body.
  • Get 308, send GET with a body: Incorrect, this should be a GET with no body.

Seems like a bug.

Edit: I described the current behavior incorrectly; after the 308, we send a GET, not a POST. The problem is that the GET includes a body.

@neild neild 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 Nov 6, 2024
@seankhliao
Copy link
Member

It looks like right now it's:

  • Get 308, send GET, but with original POST body.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/626055 mentions this issue: net/http: 308 redirects should use the previous hop's body

@Shnitzelil
Copy link
Author

@seankhliao do you know when this fix will be shipped?

@seankhliao
Copy link
Member

1.24 ~ next February

@jupenur
Copy link

jupenur commented Nov 15, 2024

Should this be treated as a security issue? Consider e.g. the following scenario:

  1. Go program authenticates to an auth server at https://login.example.com/ by POSTing credentials
  2. Auth server checks credentials, sets a cookie on the example.com domain, and 302-redirects to an app at https://app.example.com/
  3. The app has been compromised and does a 307-redirect to itself to harvest the credentials from the first step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants