register the panic-recover handler after the activity logger

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.
 

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")

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.

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()
 

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

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 {