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: add more flexible Redirect #25166

Closed
SamWhited opened this issue Apr 29, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@SamWhited
Copy link
Member

commented Apr 29, 2018

The http.Redirect function sets the content header to text/html and writes a simple link. Since the body is normally not seen anyways except by legacy clients with one of the newer redirect status codes, this is generally fine if you're using HTML. However, being a general HTTP package, redirects are often useful in places that don't use HTML, such as web APIs, HTTP based instant messengers, etc.

Since an HTTP redirect is performed by setting the Location header and a status code, it seems easy enough to do on your own. However, the built in Redirect handler has several branches that cover for a variety of edge cases, and uses a special cased form of URL escaping that is not exported, so it actually requires copying a lot of code to get it right.

Since this is non-trivial, and potentially a security problem (if you don't escape your URLs properly you may wind up with header injection or similar), I would like to see the net/http package include a more general form of redirect that does not try to write the body itself. I am using a middleware-esq approach in some of my projects:

// RedirectHandlerFunc replies to the request with a redirect to url,
// which may be a path relative to the request path.
//
// The handler should write a status code in the 3xx range, usually
// http.StatusMovedPermanently, http.StatusFound or http.StatusSeeOther.
func RedirectHandlerFunc(w http.ResponseWriter, r *http.Request, url string, h http.HandlerFunc) { /**/ }

An alternative might be to create a function similar to the existing redirect that is meant to be called from within handlers:

// RedirectHeader replies to the request with a redirect to url,
// by writing the Location header which may be a path relative
// to the request path.
//
// RedirectHeader does not write a status code or body.
// The user should write a status code in the 3xx range, usually
// http.StatusMovedPermanently, http.StatusFound or http.StatusSeeOther.
func RedirectHeader(w http.ResponseWriter, r *http.Request, url string) { /**/ }

The usual litmus test of "would this be useful elsewhere in the standard library" doesn't seem to apply here since this is more an application specific concern, but this would potentially be useful in all web APIs, and that seems like a rather large number of projects.

@SamWhited SamWhited added the Proposal label Apr 29, 2018

@SamWhited SamWhited added this to the Proposal milestone Apr 29, 2018

@namusyaka

This comment has been minimized.

Copy link
Member

commented Apr 30, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 30, 2018

How about this:

  • no new API
  • modify the existing func Redirect to say that if the Content-Type header is already defined in the ResponseWriter.Header() map (even if it's set to nil, which means to not write it), then Redirect will not set a Content-Type or write a body.
@SamWhited

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2018

@bradfitz that seems reasonable to me, though also a little less obvious when reading code.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 30, 2018

Change https://golang.org/cl/110296 mentions this issue: net/http: don't write redirect body if content-type is set

@gopherbot gopherbot closed this in dc4b9cf May 3, 2018

@SamWhited SamWhited modified the milestones: Proposal, Go1.11 May 3, 2018

@gopherbot

This comment has been minimized.

Copy link

commented May 4, 2018

Change https://golang.org/cl/111517 mentions this issue: net/http: write status code in Redirect when Content-Type header set

gopherbot pushed a commit that referenced this issue May 4, 2018

net/http: write status code in Redirect when Content-Type header set
This is a followup to CL 110296. That change added a new behavior
to Redirect, where the short HTML body is not written if the
Content-Type header is already set. It was implemented by doing
an early return. That unintentionally prevented the correct status
code from being written, so it would always default to 200.
Existing tests didn't catch this because they don't check status code.

This change fixes that issue by removing the early return and
moving the code to write a short HTML body behind an if statement.
It adds written status code checks to Redirect tests.

It also tries to improve the documentation wording and code style
in TestRedirect_contentTypeAndBody.

Updates #25166.

Change-Id: Idce004baa88e278d098661c03c9523426c5eb898
Reviewed-on: https://go-review.googlesource.com/111517
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@golang golang locked and limited conversation to collaborators May 4, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.