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: Redirect conditions are incorrect, support 307/308 #10767

Closed
anacrolix opened this issue May 10, 2015 · 9 comments
Closed

net/http: Redirect conditions are incorrect, support 307/308 #10767

anacrolix opened this issue May 10, 2015 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@anacrolix
Copy link
Contributor

I believe the functions Client.doFollowingRedirects and shouldRedirect{Post,Get} are incorrect according to http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html. Of the 4 redirect codes considered in this code, 301, 302, and 307 should redirect for GET and HEAD only. 303 should always redirect, and always convert the method to GET.

Currently 302 and 303 are redirecting POST and PUT, and converting to GET in doing so. I doubt anyone is relying on the current broken behaviour as it doesn't work as expected for due to #3665, which prematurely consumes the original request body when using POST/PUT in a safe way.

@rsc
Copy link
Contributor

rsc commented Jun 29, 2015

Too late for Go 1.5. See also #9348.

@harshavardhana
Copy link
Contributor

I believe the functions Client.doFollowingRedirects and shouldRedirect{Post,Get} are incorrect according to http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html. Of the 4 redirect codes considered in this code, 301, 302, and 307 should redirect for GET and HEAD only. 303 should always redirect, and always convert the method to GET.

This document has been superseded. In 2014, RFC2616 was replaced by multiple RFCs (7230-7237). See IETF Documents for more information.

This document has been superseded by https://tools.ietf.org/html/rfc7231#section-6.4

@harshavardhana
Copy link
Contributor

   HTTP defines a set of status codes for the purpose of redirecting a
   request to a different URI ([RFC3986]).  The history of these status
   codes is summarized in Section 6.4 of [RFC7231], which also
   classifies the existing status codes into four categories.

   The first of these categories contains the status codes 301 (Moved
   Permanently), 302 (Found), and 307 (Temporary Redirect), which can be
   classified as below:

   +-------------------------------------------+-----------+-----------+
   |                                           | Permanent | Temporary |
   +-------------------------------------------+-----------+-----------+
   | Allows changing the request method from   | 301       | 302       |
   | POST to GET                               |           |           |
   | Does not allow changing the request       | -         | 307       |
   | method from POST to GET                   |           |           |
   +-------------------------------------------+-----------+-----------+

   Section 6.4.7 of [RFC7231] states that HTTP does not define a
   permanent variant of status code 307; this specification adds the
   status code 308, defining this missing variant (Section 3).

@lgarron
Copy link

lgarron commented Jun 27, 2016

this specification adds the
status code 308, defining this missing variant (Section 3).

It seems Go doesn't have a constant for 308, and explicitly does not consider it for redirects. This led to unexpected behaviour for someone using a program implemented in Go.

I can't find a technical reason this is not supported; is it just a matter of making Go know about 308's?

@bradfitz
Copy link
Contributor

@lgarron, the constant was added during the Go 1.7 dev cycle. You can see it at https://tip.golang.org/pkg/net/http/#pkg-constants

It seems that 308 should be followed.

@bradfitz bradfitz removed their assignment Jun 27, 2016
@bradfitz bradfitz modified the milestones: Go1.8, Unplanned Jun 27, 2016
@bradfitz bradfitz changed the title net/http: Redirect conditions are incorrect net/http: Redirect conditions are incorrect, support 308 Jun 27, 2016
@lgarron
Copy link

lgarron commented Jun 27, 2016

@lgarron, the constant was added during the Go 1.7 dev cycle. You can see it at https://tip.golang.org/pkg/net/http/#pkg-constants

It seems that 308 should be followed.

Ah, excellent! I didn't know about tip.golang.org, thanks for the link.

@bradfitz bradfitz self-assigned this Sep 26, 2016
@bradfitz bradfitz changed the title net/http: Redirect conditions are incorrect, support 308 net/http: Redirect conditions are incorrect, support 307/308 Sep 29, 2016
@gopherbot
Copy link

CL https://golang.org/cl/29852 mentions this issue.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2016
@gopherbot
Copy link

CL https://golang.org/cl/31733 mentions this issue.

gopherbot pushed a commit that referenced this issue Oct 22, 2016
Updates #10767

Change-Id: I197535f71bc2dc45e783f38d8031aa717d50fd80
Reviewed-on: https://go-review.googlesource.com/31733
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

CL https://golang.org/cl/32595 mentions this issue.

gopherbot pushed a commit that referenced this issue Nov 4, 2016
This CL tweaks the new (unreleased) 307/308 support added in
https://golang.org/cl/29852 for #10767.

Change 1: if a 307/308 response doesn't have a Location header in its
response (as observed in the wild in #17773), just do what we used to
do in Go 1.7 and earlier, and don't try to follow that redirect.

Change 2: don't follow a 307/308 if we sent a body on the first
request and the caller's Request.GetBody func is nil so we can't
"rewind" the body to send it again.

Updates #17773 (will be fixed more elsewhere)

Change-Id: I183570f7346917828a4b6f7f1773094122a30406
Reviewed-on: https://go-review.googlesource.com/32595
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Nov 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

9 participants