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 <art27@cantab.net>
* fix test

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>

diff --git a/modules/web/route.go b/modules/web/route.go
index 1d9c92bd7a9b5006b0ad61b480294d409f091db0..2ee1f2572868afdb4eacc875e68f0f41deb354c3 100644 (file)
--- 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 a8470fec94f577799177ef19ff572d69fa4184fb..df00f074f1d74d1011dbb3fe6fffdcd7c9710fcd 100644 (file)
--- 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)
                        })
                })
        })