diff options
author | wxiaoguang <wxiaoguang@gmail.com> | 2023-05-04 14:36:34 +0800 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-05-04 14:36:34 +0800 |
commit | 5d77691d428d5302ee4df6c2a936b8e2ea9dca7e (patch) | |
tree | 4fa6b13f2ae81c5de94d84f2288ae1f5653c011f /routers/common | |
parent | 75ea0d5dba5dbf2f84cef2d12460fdd566d43e62 (diff) | |
download | gitea-5d77691d428d5302ee4df6c2a936b8e2ea9dca7e.tar.gz gitea-5d77691d428d5302ee4df6c2a936b8e2ea9dca7e.zip |
Improve template system and panic recovery (#24461)
Partially for #24457
Major changes:
1. The old `signedUserNameStringPointerKey` is quite hacky, use
`ctx.Data[SignedUser]` instead
2. Move duplicate code from `Contexter` to `CommonTemplateContextData`
3. Remove incorrect copying&pasting code `ctx.Data["Err_Password"] =
true` in API handlers
4. Use one unique `RenderPanicErrorPage` for panic error page rendering
5. Move `stripSlashesMiddleware` to be the first middleware
6. Install global panic recovery handler, it works for both `install`
and `web`
7. Make `500.tmpl` only depend minimal template functions/variables,
avoid triggering new panics
Screenshot:
<details>
![image](https://user-images.githubusercontent.com/2114189/235444895-cecbabb8-e7dc-4360-a31c-b982d11946a7.png)
</details>
Diffstat (limited to 'routers/common')
-rw-r--r-- | routers/common/errpage.go | 57 | ||||
-rw-r--r-- | routers/common/middleware.go | 54 |
2 files changed, 78 insertions, 33 deletions
diff --git a/routers/common/errpage.go b/routers/common/errpage.go new file mode 100644 index 0000000000..4cf3bf8357 --- /dev/null +++ b/routers/common/errpage.go @@ -0,0 +1,57 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package common + +import ( + "fmt" + "net/http" + + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/base" + "code.gitea.io/gitea/modules/httpcache" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/templates" + "code.gitea.io/gitea/modules/web/middleware" + "code.gitea.io/gitea/modules/web/routing" +) + +const tplStatus500 base.TplName = "status/500" + +// RenderPanicErrorPage renders a 500 page, and it never panics +func RenderPanicErrorPage(w http.ResponseWriter, req *http.Request, err any) { + combinedErr := fmt.Sprintf("%v\n%s", err, log.Stack(2)) + log.Error("PANIC: %s", combinedErr) + + defer func() { + if err := recover(); err != nil { + log.Error("Panic occurs again when rendering error page: %v", err) + } + }() + + routing.UpdatePanicError(req.Context(), err) + + httpcache.SetCacheControlInHeader(w.Header(), 0, "no-transform") + w.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions) + + data := middleware.GetContextData(req.Context()) + if data["locale"] == nil { + data = middleware.CommonTemplateContextData() + data["locale"] = middleware.Locale(w, req) + } + + // This recovery handler could be called without Gitea's web context, so we shouldn't touch that context too much. + // Otherwise, the 500-page may cause new panics, eg: cache.GetContextWithData, it makes the developer&users couldn't find the original panic. + user, _ := data[middleware.ContextDataKeySignedUser].(*user_model.User) + if !setting.IsProd || (user != nil && user.IsAdmin) { + data["ErrorMsg"] = "PANIC: " + combinedErr + } + + err = templates.HTMLRenderer().HTML(w, http.StatusInternalServerError, string(tplStatus500), data) + if err != nil { + log.Error("Error occurs again when rendering error page: %v", err) + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("Internal server error, please collect error logs and report to Gitea issue tracker")) + } +} diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 28ecf934e1..c1ee9dd765 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -10,9 +10,9 @@ import ( "code.gitea.io/gitea/modules/cache" "code.gitea.io/gitea/modules/context" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/web/middleware" "code.gitea.io/gitea/modules/web/routing" "gitea.com/go-chi/session" @@ -20,13 +20,26 @@ import ( chi "github.com/go-chi/chi/v5" ) -// ProtocolMiddlewares returns HTTP protocol related middlewares +// ProtocolMiddlewares returns HTTP protocol related middlewares, and it provides a global panic recovery func ProtocolMiddlewares() (handlers []any) { + // first, normalize the URL path + handlers = append(handlers, stripSlashesMiddleware) + + // prepare the ContextData and panic recovery handlers = append(handlers, func(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - // First of all escape the URL RawPath to ensure that all routing is done using a correctly escaped URL - req.URL.RawPath = req.URL.EscapedPath() + defer func() { + if err := recover(); err != nil { + RenderPanicErrorPage(resp, req, err) // it should never panic + } + }() + req = req.WithContext(middleware.WithContextData(req.Context())) + next.ServeHTTP(resp, req) + }) + }) + handlers = append(handlers, func(next http.Handler) http.Handler { + return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { ctx, _, finished := process.GetManager().AddTypedContext(req.Context(), fmt.Sprintf("%s: %s", req.Method, req.RequestURI), process.RequestProcessType, true) defer finished() next.ServeHTTP(context.NewResponse(resp), req.WithContext(cache.WithCacheContext(ctx))) @@ -47,9 +60,6 @@ func ProtocolMiddlewares() (handlers []any) { handlers = append(handlers, proxy.ForwardedHeaders(opt)) } - // Strip slashes. - handlers = append(handlers, stripSlashesMiddleware) - if !setting.Log.DisableRouterLog { handlers = append(handlers, routing.NewLoggerHandler()) } @@ -58,40 +68,18 @@ func ProtocolMiddlewares() (handlers []any) { handlers = append(handlers, context.AccessLogger()) } - handlers = append(handlers, func(next http.Handler) http.Handler { - return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - // Why we need this? The Recovery() will try to render a beautiful - // error page for user, but the process can still panic again, and other - // middleware like session also may panic then we have to recover twice - // and send a simple error page that should not panic anymore. - defer func() { - if err := recover(); err != nil { - routing.UpdatePanicError(req.Context(), err) - combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, log.Stack(2)) - log.Error("%v", combinedErr) - if setting.IsProd { - http.Error(resp, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) - } else { - http.Error(resp, combinedErr, http.StatusInternalServerError) - } - } - }() - next.ServeHTTP(resp, req) - }) - }) return handlers } func stripSlashesMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - var urlPath string + // First of all escape the URL RawPath to ensure that all routing is done using a correctly escaped URL + req.URL.RawPath = req.URL.EscapedPath() + + urlPath := req.URL.RawPath rctx := chi.RouteContext(req.Context()) if rctx != nil && rctx.RoutePath != "" { urlPath = rctx.RoutePath - } else if req.URL.RawPath != "" { - urlPath = req.URL.RawPath - } else { - urlPath = req.URL.Path } sanitizedPath := &strings.Builder{} |