finn/forgejo
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Improve template system and panic recovery (#24461)

Browse source 
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>
This commit is contained in:
wxiaoguang 2023-05-04 14:36:34 +08:00 • committed by GitHub
parent 75ea0d5dba
commit 5d77691d42
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
25 changed files with 277 additions and 364 deletions
Download patch file Download diff file Expand all files Collapse all files

2
routers/api/v1/admin/user.go
View file

@ -103,7 +103,6 @@ func CreateUser(ctx *context.APIContext) {
if err != nil {
log.Error(err.Error())
}
ctx.Data["Err_Password"] = true
ctx.Error(http.StatusBadRequest, "PasswordPwned", errors.New("PasswordPwned"))
return
}
@ -201,7 +200,6 @@ func EditUser(ctx *context.APIContext) {
if err != nil {
log.Error(err.Error())
}
ctx.Data["Err_Password"] = true
ctx.Error(http.StatusBadRequest, "PasswordPwned", errors.New("PasswordPwned"))
return
}

3
routers/api/v1/misc/markup_test.go
View file

@ -19,6 +19,7 @@ import (
"code.gitea.io/gitea/modules/templates"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/modules/web/middleware"
"github.com/stretchr/testify/assert"
)
@ -36,7 +37,7 @@ func createContext(req *http.Request) (*context.Context, *httptest.ResponseRecor
Req: req,
Resp: context.NewResponse(resp),
Render: rnd,
Data: make(map[string]interface{}),
Data: make(middleware.ContextData),
}
defer c.Close()

57
routers/common/errpage.go Normal file
View file

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

54
routers/common/middleware.go
View file

@ -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{}

30
routers/install/install.go
View file

@ -5,7 +5,6 @@
package install
import (
goctx "context"
"fmt"
"net/http"
"os"
@ -53,33 +52,32 @@ func getSupportedDbTypeNames() (dbTypeNames []map[string]string) {
return dbTypeNames
}
// Init prepare for rendering installation page
func Init(ctx goctx.Context) func(next http.Handler) http.Handler {
// Contexter prepare for rendering installation page
func Contexter() func(next http.Handler) http.Handler {
rnd := templates.HTMLRenderer()
dbTypeNames := getSupportedDbTypeNames()
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
locale := middleware.Locale(resp, req)
startTime := time.Now()
ctx := context.Context{
Resp: context.NewResponse(resp),
Flash: &middleware.Flash{},
Locale: locale,
Locale: middleware.Locale(resp, req),
Render: rnd,
Data: middleware.GetContextData(req.Context()),
Session: session.GetSession(req),
Data: map[string]interface{}{
"locale": locale,
"Title": locale.Tr("install.install"),
"PageIsInstall": true,
"DbTypeNames": dbTypeNames,
"AllLangs": translation.AllLangs(),
"PageStartTime": startTime,
"PasswordHashAlgorithms": hash.RecommendedHashAlgorithms,
},
}
defer ctx.Close()
ctx.Data.MergeFrom(middleware.CommonTemplateContextData())
ctx.Data.MergeFrom(middleware.ContextData{
"locale": ctx.Locale,
"Title": ctx.Locale.Tr("install.install"),
"PageIsInstall": true,
"DbTypeNames": dbTypeNames,
"AllLangs": translation.AllLangs(),
"PasswordHashAlgorithms": hash.RecommendedHashAlgorithms,
})
ctx.Req = context.WithContext(req, &ctx)
next.ServeHTTP(resp, ctx.Req)
})

66
routers/install/routes.go
View file

@ -9,76 +9,14 @@ import (
"html"
"net/http"
"code.gitea.io/gitea/modules/httpcache"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/public"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/templates"
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/modules/web/middleware"
"code.gitea.io/gitea/routers/common"
"code.gitea.io/gitea/routers/web/healthcheck"
"code.gitea.io/gitea/services/forms"
)
type dataStore map[string]interface{}
func (d *dataStore) GetData() map[string]interface{} {
return *d
}
func installRecovery(ctx goctx.Context) func(next http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
defer func() {
// Why we need this? The first recover will try to render a beautiful
// error page for user, but the process can still panic again, then
// we have to just recover twice and send a simple error page that
// should not panic anymore.
defer func() {
if err := recover(); err != nil {
combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, log.Stack(2))
log.Error("%s", combinedErr)
if setting.IsProd {
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
} else {
http.Error(w, combinedErr, http.StatusInternalServerError)
}
}
}()
if err := recover(); err != nil {
combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, log.Stack(2))
log.Error("%s", combinedErr)
lc := middleware.Locale(w, req)
store := dataStore{
"Language": lc.Language(),
"CurrentURL": setting.AppSubURL + req.URL.RequestURI(),
"locale": lc,
"SignedUserID": int64(0),
"SignedUserName": "",
}
httpcache.SetCacheControlInHeader(w.Header(), 0, "no-transform")
w.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions)
if !setting.IsProd {
store["ErrorMsg"] = combinedErr
}
rnd := templates.HTMLRenderer()
err = rnd.HTML(w, http.StatusInternalServerError, "status/500", templates.BaseVars().Merge(store))
if err != nil {
log.Error("%v", err)
}
}
}()
next.ServeHTTP(w, req)
})
}
}
// Routes registers the installation routes
func Routes(ctx goctx.Context) *web.Route {
base := web.NewRoute()
@ -86,9 +24,7 @@ func Routes(ctx goctx.Context) *web.Route {
base.RouteMethods("/assets/*", "GET, HEAD", public.AssetsHandlerFunc("/assets/"))
r := web.NewRoute()
r.Use(common.Sessioner())
r.Use(installRecovery(ctx))
r.Use(Init(ctx))
r.Use(common.Sessioner(), Contexter())
r.Get("/", Install) // it must be on the root, because the "install.js" use the window.location to replace the "localhost" AppURL
r.Post("/", web.Bind(forms.InstallForm{}), SubmitInstall)
r.Get("/post-install", InstallDone)

66
routers/web/base.go
View file

@ -4,7 +4,6 @@
package web
import (
goctx "context"
"errors"
"fmt"
"io"
@ -13,18 +12,12 @@ import (
"path"
"strings"
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/httpcache"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/storage"
"code.gitea.io/gitea/modules/templates"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web/middleware"
"code.gitea.io/gitea/modules/web/routing"
"code.gitea.io/gitea/services/auth"
"gitea.com/go-chi/session"
)
func storageHandler(storageSetting setting.Storage, prefix string, objStore storage.ObjectStorage) func(next http.Handler) http.Handler {
@ -110,62 +103,3 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
})
}
}
type dataStore map[string]interface{}
func (d *dataStore) GetData() map[string]interface{} {
return *d
}
// RecoveryWith500Page returns a middleware that recovers from any panics and writes a 500 and a log if so.
// This error will be created with the gitea 500 page.
func RecoveryWith500Page(ctx goctx.Context) func(next http.Handler) http.Handler {
rnd := templates.HTMLRenderer()
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
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("%s", combinedErr)
sessionStore := session.GetSession(req)
lc := middleware.Locale(w, req)
store := dataStore{
"Language": lc.Language(),
"CurrentURL": setting.AppSubURL + req.URL.RequestURI(),
"locale": lc,
}
// TODO: this recovery handler is usually 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 := context.GetContextUser(req) // almost always nil
if user == nil {
// Get user from session if logged in - do not attempt to sign-in
user = auth.SessionUser(sessionStore)
}
httpcache.SetCacheControlInHeader(w.Header(), 0, "no-transform")
w.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions)
if !setting.IsProd || (user != nil && user.IsAdmin) {
store["ErrorMsg"] = combinedErr
}
defer func() {
if err := recover(); err != nil {
log.Error("HTML render in Recovery handler panics again: %v", err)
}
}()
err = rnd.HTML(w, http.StatusInternalServerError, "status/500", templates.BaseVars().Merge(store))
if err != nil {
log.Error("HTML render in Recovery handler fails again: %v", err)
}
}
}()
next.ServeHTTP(w, req)
})
}
}

30
routers/web/web.go
View file

@ -116,38 +116,34 @@ func Routes(ctx gocontext.Context) *web.Route {
_ = templates.HTMLRenderer()
common := []any{
common.Sessioner(),
RecoveryWith500Page(ctx),
}
var mid []any
if setting.EnableGzip {
h, err := gziphandler.GzipHandlerWithOpts(gziphandler.MinSize(GzipMinSize))
if err != nil {
log.Fatal("GzipHandlerWithOpts failed: %v", err)
}
common = append(common, h)
mid = append(mid, h)
}
if setting.Service.EnableCaptcha {
// The captcha http.Handler should only fire on /captcha/* so we can just mount this on that url
routes.RouteMethods("/captcha/*", "GET,HEAD", append(common, captcha.Captchaer(context.GetImageCaptcha()))...)
routes.RouteMethods("/captcha/*", "GET,HEAD", append(mid, captcha.Captchaer(context.GetImageCaptcha()))...)
}
if setting.HasRobotsTxt {
routes.Get("/robots.txt", append(common, misc.RobotsTxt)...)
routes.Get("/robots.txt", append(mid, misc.RobotsTxt)...)
}
// prometheus metrics endpoint - do not need to go through contexter
if setting.Metrics.Enabled {
prometheus.MustRegister(metrics.NewCollector())
routes.Get("/metrics", append(common, Metrics)...)
routes.Get("/metrics", append(mid, Metrics)...)
}
routes.Get("/ssh_info", misc.SSHInfo)
routes.Get("/api/healthz", healthcheck.Check)
common = append(common, context.Contexter(ctx))
mid = append(mid, common.Sessioner(), context.Contexter())
group := buildAuthGroup()
if err := group.Init(ctx); err != nil {
@ -155,23 +151,23 @@ func Routes(ctx gocontext.Context) *web.Route {
}
// Get user from session if logged in.
common = append(common, auth_service.Auth(group))
mid = append(mid, auth_service.Auth(group))
// GetHead allows a HEAD request redirect to GET if HEAD method is not defined for that route
common = append(common, middleware.GetHead)
mid = append(mid, middleware.GetHead)
if setting.API.EnableSwagger {
// Note: The route is here but no in API routes because it renders a web page
routes.Get("/api/swagger", append(common, misc.Swagger)...) // Render V1 by default
routes.Get("/api/swagger", append(mid, misc.Swagger)...) // Render V1 by default
}
// TODO: These really seem like things that could be folded into Contexter or as helper functions
common = append(common, user.GetNotificationCount)
common = append(common, repo.GetActiveStopwatch)
common = append(common, goGet)
mid = append(mid, user.GetNotificationCount)
mid = append(mid, repo.GetActiveStopwatch)
mid = append(mid, goGet)
others := web.NewRoute()
others.Use(common...)
others.Use(mid...)
registerRoutes(others)
routes.Mount("", others)
return routes