go-safeweb
go-safeweb copied to clipboard
google
ResponseWriter.Redirect checks whether the given status is in the right range (300-399), this should be removed to be consistent with WriteError
Currently the ReponseWriter.Redirect method still checks whether the given status code is in the right range. We have removed this check for WriteError and should remove it for Redirect to.
After some digging, it looks like this issue references https://github.com/google/go-safeweb/commit/06c0696df68349adc017c7e4388fcfbe6bd76691.
Current situation seems to be that the decision is still outstanding, and the relevant comment now lives with func (f *flight) SetCode, which currently checks code < 100 || code >= 600.
Seems that the current net/http behaviour for func Redirect(w ResponseWriter, r *Request, url string, code int) is to eventually call checkWriteHeaderCode (https://golang.org/src/net/http/server.go?s=64858:64923#L1097), which panics on code < 100 || code > 999.
My intuition is to either
- do the same (i.e. leave the panic-ing to the underlying
net/http) or - in functions that we do know better than the user, enforce status code consistently (e.g. redirects only with 3xx, errors only with 4xx/5xx).
func (f *flight) NoContent() currently seems to set StatusNoContent regardless of user-set status, which corresponds to the "knowing better than the user".
@kele wdyt?
I find it hard to imagine a scenario where writing w.Redirect(..., 200) is not a bug. Similarily for w.WriteError(foo) where foo.StatusCode() == 200.
Because of this I'm leaning towards adding the panic check consistently.
@empijei for second opinion.
Some interesting history for how net/http decided to panic when an invalid code is set server-side: https://github.com/golang/go/issues/22880#issuecomment-347327044 (TL;DR: users accidentally setting status 0 and subsequently making clients crash).
So +1 to your comment.
