From daf14b275a52a01c801e89039dece2307fec5306 Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 3 Jun 2022 22:38:29 +0100 Subject: [PATCH] Ensure responses are context.ResponseWriters (#19843) (#19859) * Ensure responses are context.ResponseWriters (#19843) Backport #19843 In order for web.Wrap to be able to detect if a response has been written we need to wrap any non-context.ResponseWriters as a such. Otherwise responses will be incorrectly detected as non-written to and handlers can double run. In the case of GZip this handler will change the response to a non-context.RW and this failure to correctly detect response writing causes fallthrough and a NPE. Fix #19839 Signed-off-by: Andrew Thornton * fix test Signed-off-by: Andrew Thornton Co-authored-by: Lunny Xiao Co-authored-by: techknowlogick --- modules/web/route.go | 42 ++++++++++++++++++++++----------------- modules/web/route_test.go | 5 +++-- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/modules/web/route.go b/modules/web/route.go index 1d9c92bd7a..2ee1f25728 100644 --- a/modules/web/route.go +++ b/modules/web/route.go @@ -42,11 +42,17 @@ func Wrap(handlers ...interface{}) http.HandlerFunc { handler := handlers[i] switch t := handler.(type) { case http.HandlerFunc: + if _, ok := resp.(context.ResponseWriter); !ok { + resp = context.NewResponse(resp) + } t(resp, req) if r, ok := resp.(context.ResponseWriter); ok && r.Status() > 0 { return } case func(http.ResponseWriter, *http.Request): + if _, ok := resp.(context.ResponseWriter); !ok { + resp = context.NewResponse(resp) + } t(resp, req) if r, ok := resp.(context.ResponseWriter); ok && r.Status() > 0 { return @@ -88,7 +94,7 @@ func Wrap(handlers ...interface{}) http.HandlerFunc { return } case func(http.Handler) http.Handler: - var next = http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) if len(handlers) > i+1 { next = Wrap(handlers[i+1:]...) } @@ -148,7 +154,7 @@ func MiddleAPI(f func(ctx *context.APIContext)) func(netx http.Handler) http.Han // Bind binding an obj to a handler func Bind(obj interface{}) http.HandlerFunc { - var tp = reflect.TypeOf(obj) + tp := reflect.TypeOf(obj) if tp.Kind() == reflect.Ptr { tp = tp.Elem() } @@ -156,7 +162,7 @@ func Bind(obj interface{}) http.HandlerFunc { panic("Only structs are allowed to bind") } return Wrap(func(ctx *context.Context) { - var theObj = reflect.New(tp).Interface() // create a new form obj for every request but not use obj directly + theObj := reflect.New(tp).Interface() // create a new form obj for every request but not use obj directly binding.Bind(ctx.Req, theObj) SetForm(ctx, theObj) middleware.AssignForm(theObj, ctx.Data) @@ -214,8 +220,8 @@ func (r *Route) Use(middlewares ...interface{}) { // Group mounts a sub-Router along a `pattern` string. func (r *Route) Group(pattern string, fn func(), middlewares ...interface{}) { - var previousGroupPrefix = r.curGroupPrefix - var previousMiddlewares = r.curMiddlewares + previousGroupPrefix := r.curGroupPrefix + previousMiddlewares := r.curMiddlewares r.curGroupPrefix += pattern r.curMiddlewares = append(r.curMiddlewares, middlewares...) @@ -238,7 +244,7 @@ func (r *Route) getPattern(pattern string) string { // Mount attaches another Route along ./pattern/* func (r *Route) Mount(pattern string, subR *Route) { - var middlewares = make([]interface{}, len(r.curMiddlewares)) + middlewares := make([]interface{}, len(r.curMiddlewares)) copy(middlewares, r.curMiddlewares) subR.Use(middlewares...) r.R.Mount(r.getPattern(pattern), subR.R) @@ -246,7 +252,7 @@ func (r *Route) Mount(pattern string, subR *Route) { // Any delegate requests for all methods func (r *Route) Any(pattern string, h ...interface{}) { - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) r.R.HandleFunc(r.getPattern(pattern), Wrap(middlewares...)) } @@ -254,7 +260,7 @@ func (r *Route) Any(pattern string, h ...interface{}) { func (r *Route) Route(pattern, methods string, h ...interface{}) { p := r.getPattern(pattern) ms := strings.Split(methods, ",") - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) for _, method := range ms { r.R.MethodFunc(strings.TrimSpace(method), p, Wrap(middlewares...)) } @@ -262,12 +268,12 @@ func (r *Route) Route(pattern, methods string, h ...interface{}) { // Delete delegate delete method func (r *Route) Delete(pattern string, h ...interface{}) { - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) r.R.Delete(r.getPattern(pattern), Wrap(middlewares...)) } func (r *Route) getMiddlewares(h []interface{}) []interface{} { - var middlewares = make([]interface{}, len(r.curMiddlewares), len(r.curMiddlewares)+len(h)) + middlewares := make([]interface{}, len(r.curMiddlewares), len(r.curMiddlewares)+len(h)) copy(middlewares, r.curMiddlewares) middlewares = append(middlewares, h...) return middlewares @@ -275,51 +281,51 @@ func (r *Route) getMiddlewares(h []interface{}) []interface{} { // Get delegate get method func (r *Route) Get(pattern string, h ...interface{}) { - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) r.R.Get(r.getPattern(pattern), Wrap(middlewares...)) } // Options delegate options method func (r *Route) Options(pattern string, h ...interface{}) { - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) r.R.Options(r.getPattern(pattern), Wrap(middlewares...)) } // GetOptions delegate get and options method func (r *Route) GetOptions(pattern string, h ...interface{}) { - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) r.R.Get(r.getPattern(pattern), Wrap(middlewares...)) r.R.Options(r.getPattern(pattern), Wrap(middlewares...)) } // PostOptions delegate post and options method func (r *Route) PostOptions(pattern string, h ...interface{}) { - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) r.R.Post(r.getPattern(pattern), Wrap(middlewares...)) r.R.Options(r.getPattern(pattern), Wrap(middlewares...)) } // Head delegate head method func (r *Route) Head(pattern string, h ...interface{}) { - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) r.R.Head(r.getPattern(pattern), Wrap(middlewares...)) } // Post delegate post method func (r *Route) Post(pattern string, h ...interface{}) { - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) r.R.Post(r.getPattern(pattern), Wrap(middlewares...)) } // Put delegate put method func (r *Route) Put(pattern string, h ...interface{}) { - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) r.R.Put(r.getPattern(pattern), Wrap(middlewares...)) } // Patch delegate patch method func (r *Route) Patch(pattern string, h ...interface{}) { - var middlewares = r.getMiddlewares(h) + middlewares := r.getMiddlewares(h) r.R.Patch(r.getPattern(pattern), Wrap(middlewares...)) } diff --git a/modules/web/route_test.go b/modules/web/route_test.go index a8470fec94..df00f074f1 100644 --- a/modules/web/route_test.go +++ b/modules/web/route_test.go @@ -53,6 +53,7 @@ func TestRoute2(t *testing.T) { tp := chi.URLParam(req, "type") assert.EqualValues(t, "issues", tp) route = 0 + resp.WriteHeader(200) }) r.Get("/{type:issues|pulls}/{index}", func(resp http.ResponseWriter, req *http.Request) { @@ -65,9 +66,8 @@ func TestRoute2(t *testing.T) { index := chi.URLParam(req, "index") assert.EqualValues(t, "1", index) route = 1 + resp.WriteHeader(200) }) - }, func(resp http.ResponseWriter, req *http.Request) { - resp.WriteHeader(200) }) r.Group("/issues/{index}", func() { @@ -79,6 +79,7 @@ func TestRoute2(t *testing.T) { index := chi.URLParam(req, "index") assert.EqualValues(t, "1", index) route = 2 + resp.WriteHeader(200) }) }) })