From 0d85c63fff05860b5b98dd4afcf66b2b05199fdb Mon Sep 17 00:00:00 2001
From: Daniel Lee <dan.limerick@gmail.com>
Date: Wed, 15 Nov 2017 13:51:15 +0100
Subject: [PATCH] middleware: recovery handles panics in all handlers

Also, changes the order of the middleware so that recovery is after the
gzip middleware. Otherwise, a 200 OK is returned instead of a 500 error.
---
 pkg/api/http_server.go            |  3 +-
 pkg/middleware/middleware_test.go |  1 +
 pkg/middleware/recovery.go        | 31 ++++++------
 pkg/middleware/recovery_test.go   | 79 +++++++++++++++++++++++++++++++
 4 files changed, 97 insertions(+), 17 deletions(-)
 create mode 100644 pkg/middleware/recovery_test.go

diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go
index 037d2ae98e5..eadaa117e86 100644
--- a/pkg/api/http_server.go
+++ b/pkg/api/http_server.go
@@ -146,12 +146,13 @@ func (hs *HttpServer) newMacaron() *macaron.Macaron {
 	m := macaron.New()
 
 	m.Use(middleware.Logger())
-	m.Use(middleware.Recovery())
 
 	if setting.EnableGzip {
 		m.Use(middleware.Gziper())
 	}
 
+	m.Use(middleware.Recovery())
+
 	for _, route := range plugins.StaticRoutes {
 		pluginRoute := path.Join("/public/plugins/", route.PluginId)
 		hs.log.Debug("Plugins: Adding route", "route", pluginRoute, "dir", route.Directory)
diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go
index a279743aed3..0d9e0e5b973 100644
--- a/pkg/middleware/middleware_test.go
+++ b/pkg/middleware/middleware_test.go
@@ -363,6 +363,7 @@ type scenarioContext struct {
 	respJson       map[string]interface{}
 	handlerFunc    handlerFunc
 	defaultHandler macaron.Handler
+	url            string
 
 	req *http.Request
 }
diff --git a/pkg/middleware/recovery.go b/pkg/middleware/recovery.go
index b63bc623549..0c9dc4670e2 100644
--- a/pkg/middleware/recovery.go
+++ b/pkg/middleware/recovery.go
@@ -123,23 +123,22 @@ func Recovery() macaron.Handler {
 					c.Data["ErrorMsg"] = string(stack)
 				}
 
-				c.HTML(500, "500")
+				ctx, ok := c.Data["ctx"].(*Context)
 
-				// // Lookup the current responsewriter
-				// val := c.GetVal(inject.InterfaceOf((*http.ResponseWriter)(nil)))
-				// res := val.Interface().(http.ResponseWriter)
-				//
-				// // respond with panic message while in development mode
-				// var body []byte
-				// if setting.Env == setting.DEV {
-				// 	res.Header().Set("Content-Type", "text/html")
-				// 	body = []byte(fmt.Sprintf(panicHtml, err, err, stack))
-				// }
-				//
-				// res.WriteHeader(http.StatusInternalServerError)
-				// if nil != body {
-				// 	res.Write(body)
-				// }
+				if ok && ctx.IsApiRequest() {
+					resp := make(map[string]interface{})
+					resp["message"] = "Internal Server Error - Check the Grafana server logs for the detailed error message."
+
+					if c.Data["ErrorMsg"] != nil {
+						resp["error"] = fmt.Sprintf("%v - %v", c.Data["Title"], c.Data["ErrorMsg"])
+					} else {
+						resp["error"] = c.Data["Title"]
+					}
+
+					c.JSON(500, resp)
+				} else {
+					c.HTML(500, "500")
+				}
 			}
 		}()
 
diff --git a/pkg/middleware/recovery_test.go b/pkg/middleware/recovery_test.go
new file mode 100644
index 00000000000..299186945ee
--- /dev/null
+++ b/pkg/middleware/recovery_test.go
@@ -0,0 +1,79 @@
+package middleware
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/go-macaron/session"
+	"github.com/grafana/grafana/pkg/bus"
+	. "github.com/smartystreets/goconvey/convey"
+	"gopkg.in/macaron.v1"
+)
+
+func TestRecoveryMiddleware(t *testing.T) {
+	Convey("Given an api route that panics", t, func() {
+		apiUrl := "/api/whatever"
+		recoveryScenario("recovery middleware should return json", apiUrl, func(sc *scenarioContext) {
+			sc.handlerFunc = PanicHandler
+			sc.fakeReq("GET", apiUrl).exec()
+			sc.req.Header.Add("content-type", "application/json")
+
+			So(sc.resp.Code, ShouldEqual, 500)
+			So(sc.respJson["message"], ShouldStartWith, "Internal Server Error - Check the Grafana server logs for the detailed error message.")
+			So(sc.respJson["error"], ShouldStartWith, "Server Error")
+		})
+	})
+
+	Convey("Given a non-api route that panics", t, func() {
+		apiUrl := "/whatever"
+		recoveryScenario("recovery middleware should return html", apiUrl, func(sc *scenarioContext) {
+			sc.handlerFunc = PanicHandler
+			sc.fakeReq("GET", apiUrl).exec()
+
+			So(sc.resp.Code, ShouldEqual, 500)
+			So(sc.resp.Header().Get("content-type"), ShouldEqual, "text/html; charset=UTF-8")
+			So(sc.resp.Body.String(), ShouldContainSubstring, "<title>Grafana - Error</title>")
+		})
+	})
+}
+
+func PanicHandler(c *Context) {
+	panic("Handler has panicked")
+}
+
+func recoveryScenario(desc string, url string, fn scenarioFunc) {
+	Convey(desc, func() {
+		defer bus.ClearBusHandlers()
+
+		sc := &scenarioContext{
+			url: url,
+		}
+		viewsPath, _ := filepath.Abs("../../public/views")
+
+		sc.m = macaron.New()
+		sc.m.Use(Recovery())
+
+		sc.m.Use(macaron.Renderer(macaron.RenderOptions{
+			Directory: viewsPath,
+			Delims:    macaron.Delims{Left: "[[", Right: "]]"},
+		}))
+
+		sc.m.Use(GetContextHandler())
+		// mock out gc goroutine
+		startSessionGC = func() {}
+		sc.m.Use(Sessioner(&session.Options{}))
+		sc.m.Use(OrgRedirect())
+		sc.m.Use(AddDefaultResponseHeaders())
+
+		sc.defaultHandler = func(c *Context) {
+			sc.context = c
+			if sc.handlerFunc != nil {
+				sc.handlerFunc(sc.context)
+			}
+		}
+
+		sc.m.Get(url, sc.defaultHandler)
+
+		fn(sc)
+	})
+}