From 5d77691d428d5302ee4df6c2a936b8e2ea9dca7e Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 4 May 2023 14:36:34 +0800 Subject: [PATCH] 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:
![image](https://user-images.githubusercontent.com/2114189/235444895-cecbabb8-e7dc-4360-a31c-b982d11946a7.png)
--- modules/context/access_log.go | 14 +++-- modules/context/api.go | 13 +---- modules/context/context.go | 73 +++++++++-------------- modules/context/package.go | 3 +- modules/context/private.go | 3 +- modules/templates/base.go | 31 ---------- modules/test/context_tests.go | 2 +- modules/web/middleware/data.go | 62 +++++++++++++++++++- modules/web/middleware/flash.go | 4 +- modules/web/middleware/request.go | 5 -- modules/web/route.go | 4 +- routers/api/v1/admin/user.go | 2 - routers/api/v1/misc/markup_test.go | 3 +- routers/common/errpage.go | 57 ++++++++++++++++++ routers/common/middleware.go | 54 +++++++---------- routers/install/install.go | 30 +++++----- routers/install/routes.go | 66 +-------------------- routers/web/base.go | 66 --------------------- routers/web/web.go | 30 +++++----- services/auth/interface.go | 2 +- services/auth/middleware.go | 4 +- templates/base/head_script.tmpl | 1 - templates/status/500.tmpl | 93 +++++++++++++++++++----------- templates/user/profile.tmpl | 6 +- web_src/js/bootstrap.js | 11 ---- 25 files changed, 276 insertions(+), 363 deletions(-) create mode 100644 routers/common/errpage.go diff --git a/modules/context/access_log.go b/modules/context/access_log.go index 64d204733b8..b6468d139bf 100644 --- a/modules/context/access_log.go +++ b/modules/context/access_log.go @@ -5,7 +5,6 @@ package context import ( "bytes" - "context" "fmt" "net" "net/http" @@ -13,8 +12,10 @@ import ( "text/template" "time" + user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/web/middleware" ) type routerLoggerOptions struct { @@ -26,8 +27,6 @@ type routerLoggerOptions struct { RequestID *string } -var signedUserNameStringPointerKey interface{} = "signedUserNameStringPointerKey" - const keyOfRequestIDInTemplate = ".RequestID" // According to: @@ -60,8 +59,6 @@ func AccessLogger() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { start := time.Now() - identity := "-" - r := req.WithContext(context.WithValue(req.Context(), signedUserNameStringPointerKey, &identity)) var requestID string if needRequestID { @@ -73,9 +70,14 @@ func AccessLogger() func(http.Handler) http.Handler { reqHost = req.RemoteAddr } - next.ServeHTTP(w, r) + next.ServeHTTP(w, req) rw := w.(ResponseWriter) + identity := "-" + data := middleware.GetContextData(req.Context()) + if signedUser, ok := data[middleware.ContextDataKeySignedUser].(*user_model.User); ok { + identity = signedUser.Name + } buf := bytes.NewBuffer([]byte{}) err = logTemplate.Execute(buf, routerLoggerOptions{ req: req, diff --git a/modules/context/api.go b/modules/context/api.go index ae245ec1cb6..e263dcbe8de 100644 --- a/modules/context/api.go +++ b/modules/context/api.go @@ -222,7 +222,7 @@ func APIContexter() func(http.Handler) http.Handler { ctx := APIContext{ Context: &Context{ Resp: NewResponse(w), - Data: map[string]interface{}{}, + Data: middleware.GetContextData(req.Context()), Locale: locale, Cache: cache.GetCache(), Repo: &Repository{ @@ -250,17 +250,6 @@ func APIContexter() func(http.Handler) http.Handler { ctx.Data["Context"] = &ctx next.ServeHTTP(ctx.Resp, ctx.Req) - - // Handle adding signedUserName to the context for the AccessLogger - usernameInterface := ctx.Data["SignedUserName"] - identityPtrInterface := ctx.Req.Context().Value(signedUserNameStringPointerKey) - if usernameInterface != nil && identityPtrInterface != nil { - username := usernameInterface.(string) - identityPtr := identityPtrInterface.(*string) - if identityPtr != nil && username != "" { - *identityPtr = username - } - } }) } } diff --git a/modules/context/context.go b/modules/context/context.go index cd7fcebe55d..d73a26e5b6f 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -55,7 +55,7 @@ type Render interface { type Context struct { Resp ResponseWriter Req *http.Request - Data map[string]interface{} // data used by MVC templates + Data middleware.ContextData // data used by MVC templates PageData map[string]interface{} // data used by JavaScript modules in one page, it's `window.config.pageData` Render Render translation.Locale @@ -97,7 +97,7 @@ func (ctx *Context) TrHTMLEscapeArgs(msg string, args ...string) string { } // GetData returns the data -func (ctx *Context) GetData() map[string]interface{} { +func (ctx *Context) GetData() middleware.ContextData { return ctx.Data } @@ -219,6 +219,7 @@ const tplStatus500 base.TplName = "status/500" // HTML calls Context.HTML and renders the template to HTTP response func (ctx *Context) HTML(status int, name base.TplName) { log.Debug("Template: %s", name) + tmplStartTime := time.Now() if !setting.IsProd { ctx.Data["TemplateName"] = name @@ -226,13 +227,19 @@ func (ctx *Context) HTML(status int, name base.TplName) { ctx.Data["TemplateLoadTimes"] = func() string { return strconv.FormatInt(time.Since(tmplStartTime).Nanoseconds()/1e6, 10) + "ms" } - if err := ctx.Render.HTML(ctx.Resp, status, string(name), templates.BaseVars().Merge(ctx.Data)); err != nil { - if status == http.StatusInternalServerError && name == tplStatus500 { - ctx.PlainText(http.StatusInternalServerError, "Unable to find HTML templates, the template system is not initialized, or Gitea can't find your template files.") - return - } + + err := ctx.Render.HTML(ctx.Resp, status, string(name), ctx.Data) + if err == nil { + return + } + + // if rendering fails, show error page + if name != tplStatus500 { err = fmt.Errorf("failed to render template: %s, error: %s", name, templates.HandleTemplateRenderingError(err)) - ctx.ServerError("Render failed", err) + ctx.ServerError("Render failed", err) // show the 500 error page + } else { + ctx.PlainText(http.StatusInternalServerError, "Unable to render status/500 page, the template system is broken, or Gitea can't find your template files.") + return } } @@ -676,7 +683,7 @@ func getCsrfOpts() CsrfOptions { } // Contexter initializes a classic context for a request. -func Contexter(ctx context.Context) func(next http.Handler) http.Handler { +func Contexter() func(next http.Handler) http.Handler { rnd := templates.HTMLRenderer() csrfOpts := getCsrfOpts() if !setting.IsProd { @@ -684,34 +691,30 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler { } 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() - link := setting.AppSubURL + strings.TrimSuffix(req.URL.EscapedPath(), "/") - ctx := Context{ Resp: NewResponse(resp), Cache: mc.GetCache(), - Locale: locale, - Link: link, + Locale: middleware.Locale(resp, req), + Link: setting.AppSubURL + strings.TrimSuffix(req.URL.EscapedPath(), "/"), Render: rnd, Session: session.GetSession(req), Repo: &Repository{ PullRequest: &PullRequest{}, }, - Org: &Organization{}, - Data: map[string]interface{}{ - "CurrentURL": setting.AppSubURL + req.URL.RequestURI(), - "PageStartTime": startTime, - "Link": link, - "RunModeIsProd": setting.IsProd, - }, + Org: &Organization{}, + Data: middleware.GetContextData(req.Context()), } defer ctx.Close() + ctx.Data.MergeFrom(middleware.CommonTemplateContextData()) + ctx.Data["Context"] = &ctx + ctx.Data["CurrentURL"] = setting.AppSubURL + req.URL.RequestURI() + ctx.Data["Link"] = ctx.Link + ctx.Data["locale"] = ctx.Locale + // PageData is passed by reference, and it will be rendered to `window.config.pageData` in `head.tmpl` for JavaScript modules - ctx.PageData = map[string]interface{}{} + ctx.PageData = map[string]any{} ctx.Data["PageData"] = ctx.PageData - ctx.Data["Context"] = &ctx ctx.Req = WithContext(req, &ctx) ctx.Csrf = PrepareCSRFProtector(csrfOpts, &ctx) @@ -755,16 +758,6 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler { ctx.Data["CsrfTokenHtml"] = template.HTML(``) // FIXME: do we really always need these setting? There should be someway to have to avoid having to always set these - ctx.Data["IsLandingPageHome"] = setting.LandingPageURL == setting.LandingPageHome - ctx.Data["IsLandingPageExplore"] = setting.LandingPageURL == setting.LandingPageExplore - ctx.Data["IsLandingPageOrganizations"] = setting.LandingPageURL == setting.LandingPageOrganizations - - ctx.Data["ShowRegistrationButton"] = setting.Service.ShowRegistrationButton - ctx.Data["ShowMilestonesDashboardPage"] = setting.Service.ShowMilestonesDashboardPage - ctx.Data["ShowFooterVersion"] = setting.Other.ShowFooterVersion - - ctx.Data["EnableSwagger"] = setting.API.EnableSwagger - ctx.Data["EnableOpenIDSignIn"] = setting.Service.EnableOpenIDSignIn ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations ctx.Data["DisableStars"] = setting.Repository.DisableStars ctx.Data["EnableActions"] = setting.Actions.Enabled @@ -777,21 +770,9 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler { ctx.Data["UnitProjectsGlobalDisabled"] = unit.TypeProjects.UnitGlobalDisabled() ctx.Data["UnitActionsGlobalDisabled"] = unit.TypeActions.UnitGlobalDisabled() - ctx.Data["locale"] = locale ctx.Data["AllLangs"] = translation.AllLangs() next.ServeHTTP(ctx.Resp, ctx.Req) - - // Handle adding signedUserName to the context for the AccessLogger - usernameInterface := ctx.Data["SignedUserName"] - identityPtrInterface := ctx.Req.Context().Value(signedUserNameStringPointerKey) - if usernameInterface != nil && identityPtrInterface != nil { - username := usernameInterface.(string) - identityPtr := identityPtrInterface.(*string) - if identityPtr != nil && username != "" { - *identityPtr = username - } - } }) } } diff --git a/modules/context/package.go b/modules/context/package.go index 6c418b31646..fe5bdac19d6 100644 --- a/modules/context/package.go +++ b/modules/context/package.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/templates" + "code.gitea.io/gitea/modules/web/middleware" ) // Package contains owner, access mode and optional the package descriptor @@ -136,7 +137,7 @@ func PackageContexter(ctx gocontext.Context) func(next http.Handler) http.Handle return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { ctx := Context{ Resp: NewResponse(resp), - Data: map[string]interface{}{}, + Data: middleware.GetContextData(req.Context()), Render: rnd, } defer ctx.Close() diff --git a/modules/context/private.go b/modules/context/private.go index 24f50fa4713..f621dd68390 100644 --- a/modules/context/private.go +++ b/modules/context/private.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/process" + "code.gitea.io/gitea/modules/web/middleware" ) // PrivateContext represents a context for private routes @@ -62,7 +63,7 @@ func PrivateContexter() func(http.Handler) http.Handler { ctx := &PrivateContext{ Context: &Context{ Resp: NewResponse(w), - Data: map[string]interface{}{}, + Data: middleware.GetContextData(req.Context()), }, } defer ctx.Close() diff --git a/modules/templates/base.go b/modules/templates/base.go index 4254a569764..ef28cc03f45 100644 --- a/modules/templates/base.go +++ b/modules/templates/base.go @@ -5,43 +5,12 @@ package templates import ( "strings" - "time" "code.gitea.io/gitea/modules/assetfs" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" ) -// Vars represents variables to be render in golang templates -type Vars map[string]interface{} - -// Merge merges another vars to the current, another Vars will override the current -func (vars Vars) Merge(another map[string]interface{}) Vars { - for k, v := range another { - vars[k] = v - } - return vars -} - -// BaseVars returns all basic vars -func BaseVars() Vars { - startTime := time.Now() - return map[string]interface{}{ - "IsLandingPageHome": setting.LandingPageURL == setting.LandingPageHome, - "IsLandingPageExplore": setting.LandingPageURL == setting.LandingPageExplore, - "IsLandingPageOrganizations": setting.LandingPageURL == setting.LandingPageOrganizations, - - "ShowRegistrationButton": setting.Service.ShowRegistrationButton, - "ShowMilestonesDashboardPage": setting.Service.ShowMilestonesDashboardPage, - "ShowFooterVersion": setting.Other.ShowFooterVersion, - "DisableDownloadSourceArchives": setting.Repository.DisableDownloadSourceArchives, - - "EnableSwagger": setting.API.EnableSwagger, - "EnableOpenIDSignIn": setting.Service.EnableOpenIDSignIn, - "PageStartTime": startTime, - } -} - func AssetFS() *assetfs.LayeredFS { return assetfs.Layered(CustomAssets(), BuiltinAssets()) } diff --git a/modules/test/context_tests.go b/modules/test/context_tests.go index 4af76512505..35dd920bb92 100644 --- a/modules/test/context_tests.go +++ b/modules/test/context_tests.go @@ -30,7 +30,7 @@ func MockContext(t *testing.T, path string) *context.Context { resp := &mockResponseWriter{} ctx := context.Context{ Render: &mockRender{}, - Data: make(map[string]interface{}), + Data: make(middleware.ContextData), Flash: &middleware.Flash{ Values: make(url.Values), }, diff --git a/modules/web/middleware/data.go b/modules/web/middleware/data.go index 43189940ee2..c1f0516d7d2 100644 --- a/modules/web/middleware/data.go +++ b/modules/web/middleware/data.go @@ -3,7 +3,63 @@ package middleware -// DataStore represents a data store -type DataStore interface { - GetData() map[string]interface{} +import ( + "context" + "time" + + "code.gitea.io/gitea/modules/setting" +) + +// ContextDataStore represents a data store +type ContextDataStore interface { + GetData() ContextData +} + +type ContextData map[string]any + +func (ds ContextData) GetData() map[string]any { + return ds +} + +func (ds ContextData) MergeFrom(other ContextData) ContextData { + for k, v := range other { + ds[k] = v + } + return ds +} + +const ContextDataKeySignedUser = "SignedUser" + +type contextDataKeyType struct{} + +var contextDataKey contextDataKeyType + +func WithContextData(c context.Context) context.Context { + return context.WithValue(c, contextDataKey, make(ContextData, 10)) +} + +func GetContextData(c context.Context) ContextData { + if ds, ok := c.Value(contextDataKey).(ContextData); ok { + return ds + } + return nil +} + +func CommonTemplateContextData() ContextData { + return ContextData{ + "IsLandingPageHome": setting.LandingPageURL == setting.LandingPageHome, + "IsLandingPageExplore": setting.LandingPageURL == setting.LandingPageExplore, + "IsLandingPageOrganizations": setting.LandingPageURL == setting.LandingPageOrganizations, + + "ShowRegistrationButton": setting.Service.ShowRegistrationButton, + "ShowMilestonesDashboardPage": setting.Service.ShowMilestonesDashboardPage, + "ShowFooterVersion": setting.Other.ShowFooterVersion, + "DisableDownloadSourceArchives": setting.Repository.DisableDownloadSourceArchives, + + "EnableSwagger": setting.API.EnableSwagger, + "EnableOpenIDSignIn": setting.Service.EnableOpenIDSignIn, + "PageStartTime": time.Now(), + + "RunModeIsProd": setting.IsProd, + } } diff --git a/modules/web/middleware/flash.go b/modules/web/middleware/flash.go index f2d7cc692d2..fa29ddeffc7 100644 --- a/modules/web/middleware/flash.go +++ b/modules/web/middleware/flash.go @@ -18,7 +18,7 @@ var FlashNow bool // Flash represents a one time data transfer between two requests. type Flash struct { - DataStore + DataStore ContextDataStore url.Values ErrorMsg, WarningMsg, InfoMsg, SuccessMsg string } @@ -34,7 +34,7 @@ func (f *Flash) set(name, msg string, current ...bool) { } if isShow { - f.GetData()["Flash"] = f + f.DataStore.GetData()["Flash"] = f } else { f.Set(name, msg) } diff --git a/modules/web/middleware/request.go b/modules/web/middleware/request.go index 34add27214b..0bb155df703 100644 --- a/modules/web/middleware/request.go +++ b/modules/web/middleware/request.go @@ -12,8 +12,3 @@ import ( func IsAPIPath(req *http.Request) bool { return strings.HasPrefix(req.URL.Path, "/api/") } - -// IsInternalPath returns true if the specified URL is an internal API path -func IsInternalPath(req *http.Request) bool { - return strings.HasPrefix(req.URL.Path, "/api/internal/") -} diff --git a/modules/web/route.go b/modules/web/route.go index 6fd60f4ca73..d801f1025c9 100644 --- a/modules/web/route.go +++ b/modules/web/route.go @@ -25,12 +25,12 @@ func Bind[T any](_ T) any { } // SetForm set the form object -func SetForm(data middleware.DataStore, obj interface{}) { +func SetForm(data middleware.ContextDataStore, obj interface{}) { data.GetData()["__form"] = obj } // GetForm returns the validate form information -func GetForm(data middleware.DataStore) interface{} { +func GetForm(data middleware.ContextDataStore) interface{} { return data.GetData()["__form"] } diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 8649a982b08..8afa83aa94f 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -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 } diff --git a/routers/api/v1/misc/markup_test.go b/routers/api/v1/misc/markup_test.go index 32de2956b79..68776613b27 100644 --- a/routers/api/v1/misc/markup_test.go +++ b/routers/api/v1/misc/markup_test.go @@ -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() diff --git a/routers/common/errpage.go b/routers/common/errpage.go new file mode 100644 index 00000000000..4cf3bf83578 --- /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 28ecf934e1c..c1ee9dd765a 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{} diff --git a/routers/install/install.go b/routers/install/install.go index c838db65822..714ddd55484 100644 --- a/routers/install/install.go +++ b/routers/install/install.go @@ -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) }) diff --git a/routers/install/routes.go b/routers/install/routes.go index 88c7e996952..52c07cfa26e 100644 --- a/routers/install/routes.go +++ b/routers/install/routes.go @@ -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) diff --git a/routers/web/base.go b/routers/web/base.go index 73607cad065..b9b59583896 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -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) - }) - } -} diff --git a/routers/web/web.go b/routers/web/web.go index 5357c55500f..8cdc10935ef 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -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 diff --git a/services/auth/interface.go b/services/auth/interface.go index f2f1aaf39cb..c4a8a20d010 100644 --- a/services/auth/interface.go +++ b/services/auth/interface.go @@ -13,7 +13,7 @@ import ( ) // DataStore represents a data store -type DataStore middleware.DataStore +type DataStore middleware.ContextDataStore // SessionStore represents a session store type SessionStore session.Store diff --git a/services/auth/middleware.go b/services/auth/middleware.go index abeafed9cb4..3b2f883d009 100644 --- a/services/auth/middleware.go +++ b/services/auth/middleware.go @@ -51,13 +51,11 @@ func authShared(ctx *context.Context, authMethod Method) error { ctx.IsBasicAuth = ctx.Data["AuthedMethod"].(string) == BasicMethodName ctx.IsSigned = true ctx.Data["IsSigned"] = ctx.IsSigned - ctx.Data["SignedUser"] = ctx.Doer + ctx.Data[middleware.ContextDataKeySignedUser] = ctx.Doer ctx.Data["SignedUserID"] = ctx.Doer.ID - ctx.Data["SignedUserName"] = ctx.Doer.Name ctx.Data["IsAdmin"] = ctx.Doer.IsAdmin } else { ctx.Data["SignedUserID"] = int64(0) - ctx.Data["SignedUserName"] = "" } return nil } diff --git a/templates/base/head_script.tmpl b/templates/base/head_script.tmpl index d19fae629bd..670d146b56a 100644 --- a/templates/base/head_script.tmpl +++ b/templates/base/head_script.tmpl @@ -6,7 +6,6 @@ If you introduce mistakes in it, Gitea JavaScript code wouldn't run correctly. -{{template "base/footer" .}} + + {{/* When a sub-template triggers an 500 error, its parent template has been partially rendered, then the 500 page + will be rendered after that partially rendered page, the HTML/JS are totally broken. Use this inline script to try to move it to main viewport. + And this page shouldn't include any other JS file, avoid duplicate JS execution (still due to the partial rendering).*/}} + + + diff --git a/templates/user/profile.tmpl b/templates/user/profile.tmpl index 9f454a82f19..5463270854e 100644 --- a/templates/user/profile.tmpl +++ b/templates/user/profile.tmpl @@ -5,7 +5,7 @@
- {{if eq .SignedUserName .ContextUser.Name}} + {{if eq .SignedUserID .ContextUser.ID}} {{avatar $.Context .ContextUser 290}} @@ -30,7 +30,7 @@ {{if .ContextUser.Location}}
  • {{svg "octicon-location"}} {{.ContextUser.Location}}
    • {{end}} - {{if (eq .SignedUserName .ContextUser.Name)}} + {{if (eq .SignedUserID .ContextUser.ID)}}
  • {{svg "octicon-mail"}} {{.ContextUser.Email}} @@ -100,7 +100,7 @@
    • {{end}} - {{if and .IsSigned (ne .SignedUserName .ContextUser.Name)}} + {{if and .IsSigned (ne .SignedUserID .ContextUser.ID)}}