From 6f2fe91da57e4b88d9d6fd9c326c4c4babda1824 Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Fri, 18 Oct 2024 13:47:10 +0300 Subject: [PATCH] register the panic-recover handler after the activity logger --- CHANGELOG.md | 4 +++- apis/base.go | 3 ++- apis/middlewares.go | 40 ++++++++++++++++++++++++++++++++++++- apis/middlewares_test.go | 39 ++++++++++++++++++++++++++++++++++++ tools/router/router.go | 35 -------------------------------- tools/router/router_test.go | 5 ----- 6 files changed, 83 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fcd3399..9a7b671c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,9 @@ > [!CAUTION] > **This is a prerelease intended for test and experimental purposes only!** -- Updated the `BindBody` FormData type inferring rules to convert numeric strings into float64 only if the resulting minimal number string representation matches the initial FormData string value ([#5687](https://github.com/pocketbase/pocketbase/issues/5687)). +- Attach the default panic-recover middleware after the activity logger so that we can log the error. + +- Updated the `RequestEvent.BindBody` FormData type inferring rules to convert numeric strings into float64 only if the resulting minimal number string representation matches the initial FormData string value ([#5687](https://github.com/pocketbase/pocketbase/issues/5687)). - Fixed the JSVM types to include properly generated function declarations when the related Go functions have shortened/combined return values. diff --git a/apis/base.go b/apis/base.go index 5926a7eb..2c9bafc1 100644 --- a/apis/base.go +++ b/apis/base.go @@ -28,9 +28,10 @@ func NewRouter(app core.App) (*router.Router[*core.RequestEvent], error) { // register default middlewares pbRouter.Bind(activityLogger()) + pbRouter.Bind(panicRecover()) + pbRouter.Bind(rateLimit()) pbRouter.Bind(loadAuthToken()) pbRouter.Bind(securityHeaders()) - pbRouter.Bind(rateLimit()) pbRouter.Bind(BodyLimit(DefaultMaxBodySize)) apiGroup := pbRouter.Group("/api") diff --git a/apis/middlewares.go b/apis/middlewares.go index 77f9fe94..c5959125 100644 --- a/apis/middlewares.go +++ b/apis/middlewares.go @@ -1,10 +1,12 @@ package apis import ( + "errors" "fmt" "log/slog" "net/http" "net/url" + "runtime" "slices" "strings" "time" @@ -29,11 +31,14 @@ const ( DefaultWWWRedirectMiddlewarePriority = -99999 DefaultWWWRedirectMiddlewareId = "pbWWWRedirect" - DefaultActivityLoggerMiddlewarePriority = DefaultRateLimitMiddlewarePriority - 30 + DefaultActivityLoggerMiddlewarePriority = DefaultRateLimitMiddlewarePriority - 40 DefaultActivityLoggerMiddlewareId = "pbActivityLogger" DefaultSkipSuccessActivityLogMiddlewareId = "pbSkipSuccessActivityLog" DefaultEnableAuthIdActivityLog = "pbEnableAuthIdActivityLog" + DefaultPanicRecoverMiddlewarePriority = DefaultRateLimitMiddlewarePriority - 30 + DefaultPanicRecoverMiddlewareId = "pbPanicRecover" + DefaultLoadAuthTokenMiddlewarePriority = DefaultRateLimitMiddlewarePriority - 20 DefaultLoadAuthTokenMiddlewareId = "pbLoadAuthToken" @@ -252,6 +257,39 @@ func wwwRedirect(redirectHosts []string) *hook.Handler[*core.RequestEvent] { } } +// panicRecover returns a default panic-recover handler. +func panicRecover() *hook.Handler[*core.RequestEvent] { + return &hook.Handler[*core.RequestEvent]{ + Id: DefaultPanicRecoverMiddlewareId, + Priority: DefaultPanicRecoverMiddlewarePriority, + Func: func(e *core.RequestEvent) (err error) { + // panic-recover + defer func() { + recoverResult := recover() + if recoverResult == nil { + return + } + + recoverErr, ok := recoverResult.(error) + if !ok { + recoverErr = fmt.Errorf("%v", recoverResult) + } else if errors.Is(recoverErr, http.ErrAbortHandler) { + // don't recover ErrAbortHandler so the response to the client can be aborted + panic(recoverResult) + } + + stack := make([]byte, 2<<10) // 2 KB + length := runtime.Stack(stack, true) + err = e.InternalServerError("", fmt.Errorf("[PANIC RECOVER] %w %s", recoverErr, stack[:length])) + }() + + err = e.Next() + + return err + }, + } +} + // securityHeaders middleware adds common security headers to the response. // // This middleware is registered by default for all routes. diff --git a/apis/middlewares_test.go b/apis/middlewares_test.go index a8f44ef6..967c6434 100644 --- a/apis/middlewares_test.go +++ b/apis/middlewares_test.go @@ -9,6 +9,45 @@ import ( "github.com/pocketbase/pocketbase/tests" ) +func TestPanicRecover(t *testing.T) { + t.Parallel() + + scenarios := []tests.ApiScenario{ + { + Name: "panic from route", + Method: http.MethodGet, + URL: "/my/test", + BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { + e.Router.GET("/my/test", func(e *core.RequestEvent) error { + panic("123") + }) + }, + ExpectedStatus: 500, + ExpectedContent: []string{`"data":{}`}, + ExpectedEvents: map[string]int{"*": 0}, + }, + { + Name: "panic from middleware", + Method: http.MethodGet, + URL: "/my/test", + BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { + e.Router.GET("/my/test", func(e *core.RequestEvent) error { + return e.String(http.StatusOK, "test") + }).BindFunc(func(e *core.RequestEvent) error { + panic(123) + }) + }, + ExpectedStatus: 500, + ExpectedContent: []string{`"data":{}`}, + ExpectedEvents: map[string]int{"*": 0}, + }, + } + + for _, scenario := range scenarios { + scenario.Test(t) + } +} + func TestRequireGuestOnly(t *testing.T) { t.Parallel() diff --git a/tools/router/router.go b/tools/router/router.go index bf862fff..d789ff5f 100644 --- a/tools/router/router.go +++ b/tools/router/router.go @@ -4,12 +4,10 @@ import ( "bufio" "encoding/json" "errors" - "fmt" "io" "log" "net" "net/http" - "runtime" "github.com/pocketbase/pocketbase/tools/hook" ) @@ -129,12 +127,6 @@ func (r *Router[T]) loadMux(mux *http.ServeMux, group *RouterGroup[T], parents [ } } - // add global panic-recover middleware - routeHook.Bind(&hook.Handler[T]{ - Func: r.panicHandler, - Priority: -9999999, // before everything else - }) - mux.HandleFunc(pattern, func(resp http.ResponseWriter, req *http.Request) { // wrap the response to add write and status tracking resp = &ResponseWriter{ResponseWriter: resp} @@ -162,33 +154,6 @@ func (r *Router[T]) loadMux(mux *http.ServeMux, group *RouterGroup[T], parents [ return nil } -// panicHandler registers a default panic-recover handling. -func (r *Router[T]) panicHandler(event T) (err error) { - // panic-recover - defer func() { - recoverResult := recover() - if recoverResult == nil { - return - } - - recoverErr, ok := recoverResult.(error) - if !ok { - recoverErr = fmt.Errorf("%v", recoverResult) - } else if errors.Is(recoverErr, http.ErrAbortHandler) { - // don't recover ErrAbortHandler so the response to the client can be aborted - panic(recoverResult) - } - - stack := make([]byte, 2<<10) // 2 KB - length := runtime.Stack(stack, true) - err = NewInternalServerError("", fmt.Errorf("[PANIC RECOVER] %w %s", recoverErr, stack[:length])) - }() - - err = event.Next() - - return err -} - func ErrorHandler(resp http.ResponseWriter, req *http.Request, err error) { if err == nil { return diff --git a/tools/router/router_test.go b/tools/router/router_test.go index 82021c54..c71cb708 100644 --- a/tools/router/router_test.go +++ b/tools/router/router_test.go @@ -64,10 +64,6 @@ func TestRouter(t *testing.T) { calls += "/" + e.Request.PathValue("param") return errors.New("test") // should be normalized to an ApiError }) - g1.GET("/panic", func(e *router.Event) error { - calls += "/panic" - panic("test") - }) mux, err := r.BuildMux() if err != nil { @@ -98,7 +94,6 @@ func TestRouter(t *testing.T) { {http.MethodHead, "/a/b/1", "root_m:a_b_group_m:1_get_m:/1_get:cleanup"}, {http.MethodPost, "/a/b/1", "root_m:a_b_group_m:/1_post:cleanup"}, {http.MethodGet, "/a/b/456", "root_m:a_b_group_m:/456/error:cleanup"}, - {http.MethodGet, "/a/b/panic", "root_m:a_b_group_m:/panic:cleanup"}, } for _, s := range scenarios {