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: can't read 301 response without a Location header #49281

Closed
stefansundin opened this issue Nov 2, 2021 · 6 comments
Closed

net/http: can't read 301 response without a Location header #49281

stefansundin opened this issue Nov 2, 2021 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@stefansundin
Copy link

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

$ go version
go version go1.17.2 darwin/arm64

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
GOARCH="arm64"
GOOS="darwin"

What did you do?

I am trying to use net/http to send a request to https://example.s3.us-west-1.amazonaws.com/ and read the response in order to get the actual bucket location (the correct AWS region). This response is missing a Location header, so net/http just returns an error.

package main

import (
	"errors"
	"fmt"
	"net/http"
)

func getBucketRegion(bucket string) (string, error) {
	// Construct client that makes one request and does not follow redirects
	client := &http.Client{
		CheckRedirect: func(req *http.Request, via []*http.Request) error {
			return http.ErrUseLastResponse
		},
	}
	resp, err := client.Get("https://" + bucket + ".s3.us-west-1.amazonaws.com/")
	if err != nil {
		return "", err // <-- function will return here
	}

	if resp.StatusCode == 200 {
		return "us-west-1", nil
	} else if resp.StatusCode == 404 {
		return "", errors.New("Bucket does not exist.")
	}

	return resp.Header.Get("x-amz-bucket-region"), nil
}

func main() {
	region, err := getBucketRegion("test")
	fmt.Printf("region: %s\n", region)
	fmt.Printf("err: %v\n", err)
}

This does not run on play.golang.org because there is no network access. https://play.golang.org/p/BiEmt_5iEvJ

Output will be:

region:
err: Get "https://example.s3.us-west-1.amazonaws.com/": 301 response missing Location header

It is impossible to get the response to read the x-amz-bucket-region header and get the actual region. One could argue that S3 does not conform to HTTP specs by returning a 301 response code without a Location header. But Go's refusal to give me back the response means that I'm not able to request and handle this particular case.

It would be nice to just return the request in this case, since there is no redirect to follow.

https://cs.opensource.google/go/go/+/master:src/net/http/client.go;l=643-646;drc=aa4e0f528e1e018e2847decb549cfc5ac07ecf20

			if loc == "" {
				resp.closeBody()
				return nil, uerr(fmt.Errorf("%d response missing Location header", resp.StatusCode))
			}

A little bit below that, there's a place where both a response and an error is returned (the comment says that it is to maintain Go 1 compatibility). Perhaps a similar handling could be added here?

Something like:

			if loc == "" {
				return resp, uerr(fmt.Errorf("%d response missing Location header", resp.StatusCode))
			}

This is just an idea, I'm sure there may be a better way!

What did you expect to see?

$ curl -v https://example.s3.us-west-1.amazonaws.com/
[...]

< HTTP/1.1 301 Moved Permanently
< x-amz-bucket-region: us-east-1
[...]

I want the ability to get x-amz-bucket-region: us-east-1 header from the response. But since the Location header is missing it is not possible to use net/http to get it.

What did you see instead?

Get "https://example.s3.us-west-1.amazonaws.com/": 301 response missing Location header
@seankhliao
Copy link
Member

You could use http.Transport.Roundtrip directly or add one that modifies the response to suit your needs

@seankhliao seankhliao changed the title Impossible to read certain net/http responses due to "301 response missing Location header" error net/http: can't read 301 response without a Location header Nov 2, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 2, 2021
@seankhliao
Copy link
Member

cc @neild

@neild
Copy link
Contributor

neild commented Nov 2, 2021

RFC 7231 says that "the server SHOULD generate a Location header field in the response" for a 301 or 302. I'm not sure why we're treating it as an error for the Location header to be missing, since it is not required.

My first thought is that we should just return the response with no error for any 3xx status with no Location header.

@caleb-artifact
Copy link

caleb-artifact commented Dec 30, 2021

@neild It seems like this exact issue has occurred in the past with status codes 307 and 308 as referenced in issue #17773.

https://cs.opensource.google/go/go/+/master:src/net/http/client.go;l=524-532;drc=76fbd6167364fb98e3ebe946cfc16b5b84d4240e

		if resp.Header.Get("Location") == "" {
			// 308s have been observed in the wild being served
			// without Location headers. Since Go 1.7 and earlier
			// didn't follow these codes, just stop here instead
			// of returning an error.
			// See Issue 17773.
			shouldRedirect = false
			break
		}

Couldn't this be applied to the 301, 302, and 303 status codes as well?

Something like:

	switch resp.StatusCode {
	case 301, 302, 303:
		redirectMethod = reqMethod
		shouldRedirect = true
		includeBody = false

		if resp.Header.Get("Location") == "" {
			shouldRedirect = false
			break
		}

@gopherbot
Copy link

Change https://golang.org/cl/375354 mentions this issue: net/http: handle 3xx responses with no Location

@stefansundin
Copy link
Author

I just tested the fix using gotip and it works great. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants