Skip to content

Commit 5d77691

Browse files
wxiaoguangwxiaoguang
authored
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>
1 parent 75ea0d5 commit 5d77691

File tree

25 files changed

+276
-363
lines changed

25 files changed

+276
-363
lines changed

‎modules/context/access_log.go

+8-6
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,17 @@ package context
55

66
import (
77
"bytes"
8-
"context"
98
"fmt"
109
"net"
1110
"net/http"
1211
"strings"
1312
"text/template"
1413
"time"
1514

15+
user_model "code.gitea.io/gitea/models/user"
1616
"code.gitea.io/gitea/modules/log"
1717
"code.gitea.io/gitea/modules/setting"
18+
"code.gitea.io/gitea/modules/web/middleware"
1819
)
1920

2021
type routerLoggerOptions struct {
@@ -26,8 +27,6 @@ type routerLoggerOptions struct {
2627
RequestID *string
2728
}
2829

29-
var signedUserNameStringPointerKey interface{} = "signedUserNameStringPointerKey"
30-
3130
const keyOfRequestIDInTemplate = ".RequestID"
3231

3332
// According to:
@@ -60,8 +59,6 @@ func AccessLogger() func(http.Handler) http.Handler {
6059
return func(next http.Handler) http.Handler {
6160
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
6261
start := time.Now()
63-
identity := "-"
64-
r := req.WithContext(context.WithValue(req.Context(), signedUserNameStringPointerKey, &identity))
6562

6663
var requestID string
6764
if needRequestID {
@@ -73,9 +70,14 @@ func AccessLogger() func(http.Handler) http.Handler {
7370
reqHost = req.RemoteAddr
7471
}
7572

76-
next.ServeHTTP(w, r)
73+
next.ServeHTTP(w, req)
7774
rw := w.(ResponseWriter)
7875

76+
identity := "-"
77+
data := middleware.GetContextData(req.Context())
78+
if signedUser, ok := data[middleware.ContextDataKeySignedUser].(*user_model.User); ok {
79+
identity = signedUser.Name
80+
}
7981
buf := bytes.NewBuffer([]byte{})
8082
err = logTemplate.Execute(buf, routerLoggerOptions{
8183
req: req,

‎modules/context/api.go

+1-12
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ func APIContexter() func(http.Handler) http.Handler {
222222
ctx := APIContext{
223223
Context: &Context{
224224
Resp: NewResponse(w),
225-
Data: map[string]interface{}{},
225+
Data: middleware.GetContextData(req.Context()),
226226
Locale: locale,
227227
Cache: cache.GetCache(),
228228
Repo: &Repository{
@@ -250,17 +250,6 @@ func APIContexter() func(http.Handler) http.Handler {
250250
ctx.Data["Context"] = &ctx
251251

252252
next.ServeHTTP(ctx.Resp, ctx.Req)
253-
254-
// Handle adding signedUserName to the context for the AccessLogger
255-
usernameInterface := ctx.Data["SignedUserName"]
256-
identityPtrInterface := ctx.Req.Context().Value(signedUserNameStringPointerKey)
257-
if usernameInterface != nil && identityPtrInterface != nil {
258-
username := usernameInterface.(string)
259-
identityPtr := identityPtrInterface.(*string)
260-
if identityPtr != nil && username != "" {
261-
*identityPtr = username
262-
}
263-
}
264253
})
265254
}
266255
}

‎modules/context/context.go

+27-46
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ type Render interface {
5555
type Context struct {
5656
Resp ResponseWriter
5757
Req *http.Request
58-
Data map[string]interface{} // data used by MVC templates
58+
Data middleware.ContextData // data used by MVC templates
5959
PageData map[string]interface{} // data used by JavaScript modules in one page, it's `window.config.pageData`
6060
Render Render
6161
translation.Locale
@@ -97,7 +97,7 @@ func (ctx *Context) TrHTMLEscapeArgs(msg string, args ...string) string {
9797
}
9898

9999
// GetData returns the data
100-
func (ctx *Context) GetData() map[string]interface{} {
100+
func (ctx *Context) GetData() middleware.ContextData {
101101
return ctx.Data
102102
}
103103

@@ -219,20 +219,27 @@ const tplStatus500 base.TplName = "status/500"
219219
// HTML calls Context.HTML and renders the template to HTTP response
220220
func (ctx *Context) HTML(status int, name base.TplName) {
221221
log.Debug("Template: %s", name)
222+
222223
tmplStartTime := time.Now()
223224
if !setting.IsProd {
224225
ctx.Data["TemplateName"] = name
225226
}
226227
ctx.Data["TemplateLoadTimes"] = func() string {
227228
return strconv.FormatInt(time.Since(tmplStartTime).Nanoseconds()/1e6, 10) + "ms"
228229
}
229-
if err := ctx.Render.HTML(ctx.Resp, status, string(name), templates.BaseVars().Merge(ctx.Data)); err != nil {
230-
if status == http.StatusInternalServerError && name == tplStatus500 {
231-
ctx.PlainText(http.StatusInternalServerError, "Unable to find HTML templates, the template system is not initialized, or Gitea can't find your template files.")
232-
return
233-
}
230+
231+
err := ctx.Render.HTML(ctx.Resp, status, string(name), ctx.Data)
232+
if err == nil {
233+
return
234+
}
235+
236+
// if rendering fails, show error page
237+
if name != tplStatus500 {
234238
err = fmt.Errorf("failed to render template: %s, error: %s", name, templates.HandleTemplateRenderingError(err))
235-
ctx.ServerError("Render failed", err)
239+
ctx.ServerError("Render failed", err) // show the 500 error page
240+
} else {
241+
ctx.PlainText(http.StatusInternalServerError, "Unable to render status/500 page, the template system is broken, or Gitea can't find your template files.")
242+
return
236243
}
237244
}
238245

@@ -676,42 +683,38 @@ func getCsrfOpts() CsrfOptions {
676683
}
677684

678685
// Contexter initializes a classic context for a request.
679-
func Contexter(ctx context.Context) func(next http.Handler) http.Handler {
686+
func Contexter() func(next http.Handler) http.Handler {
680687
rnd := templates.HTMLRenderer()
681688
csrfOpts := getCsrfOpts()
682689
if !setting.IsProd {
683690
CsrfTokenRegenerationInterval = 5 * time.Second // in dev, re-generate the tokens more aggressively for debug purpose
684691
}
685692
return func(next http.Handler) http.Handler {
686693
return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
687-
locale := middleware.Locale(resp, req)
688-
startTime := time.Now()
689-
link := setting.AppSubURL + strings.TrimSuffix(req.URL.EscapedPath(), "/")
690-
691694
ctx := Context{
692695
Resp: NewResponse(resp),
693696
Cache: mc.GetCache(),
694-
Locale: locale,
695-
Link: link,
697+
Locale: middleware.Locale(resp, req),
698+
Link: setting.AppSubURL + strings.TrimSuffix(req.URL.EscapedPath(), "/"),
696699
Render: rnd,
697700
Session: session.GetSession(req),
698701
Repo: &Repository{
699702
PullRequest: &PullRequest{},
700703
},
701-
Org: &Organization{},
702-
Data: map[string]interface{}{
703-
"CurrentURL": setting.AppSubURL + req.URL.RequestURI(),
704-
"PageStartTime": startTime,
705-
"Link": link,
706-
"RunModeIsProd": setting.IsProd,
707-
},
704+
Org: &Organization{},
705+
Data: middleware.GetContextData(req.Context()),
708706
}
709707
defer ctx.Close()
710708

709+
ctx.Data.MergeFrom(middleware.CommonTemplateContextData())
710+
ctx.Data["Context"] = &ctx
711+
ctx.Data["CurrentURL"] = setting.AppSubURL + req.URL.RequestURI()
712+
ctx.Data["Link"] = ctx.Link
713+
ctx.Data["locale"] = ctx.Locale
714+
711715
// PageData is passed by reference, and it will be rendered to `window.config.pageData` in `head.tmpl` for JavaScript modules
712-
ctx.PageData = map[string]interface{}{}
716+
ctx.PageData = map[string]any{}
713717
ctx.Data["PageData"] = ctx.PageData
714-
ctx.Data["Context"] = &ctx
715718

716719
ctx.Req = WithContext(req, &ctx)
717720
ctx.Csrf = PrepareCSRFProtector(csrfOpts, &ctx)
@@ -755,16 +758,6 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler {
755758
ctx.Data["CsrfTokenHtml"] = template.HTML(`<input type="hidden" name="_csrf" value="` + ctx.Data["CsrfToken"].(string) + `">`)
756759

757760
// FIXME: do we really always need these setting? There should be someway to have to avoid having to always set these
758-
ctx.Data["IsLandingPageHome"] = setting.LandingPageURL == setting.LandingPageHome
759-
ctx.Data["IsLandingPageExplore"] = setting.LandingPageURL == setting.LandingPageExplore
760-
ctx.Data["IsLandingPageOrganizations"] = setting.LandingPageURL == setting.LandingPageOrganizations
761-
762-
ctx.Data["ShowRegistrationButton"] = setting.Service.ShowRegistrationButton
763-
ctx.Data["ShowMilestonesDashboardPage"] = setting.Service.ShowMilestonesDashboardPage
764-
ctx.Data["ShowFooterVersion"] = setting.Other.ShowFooterVersion
765-
766-
ctx.Data["EnableSwagger"] = setting.API.EnableSwagger
767-
ctx.Data["EnableOpenIDSignIn"] = setting.Service.EnableOpenIDSignIn
768761
ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations
769762
ctx.Data["DisableStars"] = setting.Repository.DisableStars
770763
ctx.Data["EnableActions"] = setting.Actions.Enabled
@@ -777,21 +770,9 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler {
777770
ctx.Data["UnitProjectsGlobalDisabled"] = unit.TypeProjects.UnitGlobalDisabled()
778771
ctx.Data["UnitActionsGlobalDisabled"] = unit.TypeActions.UnitGlobalDisabled()
779772

780-
ctx.Data["locale"] = locale
781773
ctx.Data["AllLangs"] = translation.AllLangs()
782774

783775
next.ServeHTTP(ctx.Resp, ctx.Req)
784-
785-
// Handle adding signedUserName to the context for the AccessLogger
786-
usernameInterface := ctx.Data["SignedUserName"]
787-
identityPtrInterface := ctx.Req.Context().Value(signedUserNameStringPointerKey)
788-
if usernameInterface != nil && identityPtrInterface != nil {
789-
username := usernameInterface.(string)
790-
identityPtr := identityPtrInterface.(*string)
791-
if identityPtr != nil && username != "" {
792-
*identityPtr = username
793-
}
794-
}
795776
})
796777
}
797778
}

‎modules/context/package.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"code.gitea.io/gitea/modules/setting"
1717
"code.gitea.io/gitea/modules/structs"
1818
"code.gitea.io/gitea/modules/templates"
19+
"code.gitea.io/gitea/modules/web/middleware"
1920
)
2021

2122
// 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
136137
return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
137138
ctx := Context{
138139
Resp: NewResponse(resp),
139-
Data: map[string]interface{}{},
140+
Data: middleware.GetContextData(req.Context()),
140141
Render: rnd,
141142
}
142143
defer ctx.Close()

‎modules/context/private.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"code.gitea.io/gitea/modules/graceful"
1313
"code.gitea.io/gitea/modules/process"
14+
"code.gitea.io/gitea/modules/web/middleware"
1415
)
1516

1617
// PrivateContext represents a context for private routes
@@ -62,7 +63,7 @@ func PrivateContexter() func(http.Handler) http.Handler {
6263
ctx := &PrivateContext{
6364
Context: &Context{
6465
Resp: NewResponse(w),
65-
Data: map[string]interface{}{},
66+
Data: middleware.GetContextData(req.Context()),
6667
},
6768
}
6869
defer ctx.Close()

‎modules/templates/base.go

-31
Original file line numberDiff line numberDiff line change
@@ -5,43 +5,12 @@ package templates
55

66
import (
77
"strings"
8-
"time"
98

109
"code.gitea.io/gitea/modules/assetfs"
1110
"code.gitea.io/gitea/modules/setting"
1211
"code.gitea.io/gitea/modules/util"
1312
)
1413

15-
// Vars represents variables to be render in golang templates
16-
type Vars map[string]interface{}
17-
18-
// Merge merges another vars to the current, another Vars will override the current
19-
func (vars Vars) Merge(another map[string]interface{}) Vars {
20-
for k, v := range another {
21-
vars[k] = v
22-
}
23-
return vars
24-
}
25-
26-
// BaseVars returns all basic vars
27-
func BaseVars() Vars {
28-
startTime := time.Now()
29-
return map[string]interface{}{
30-
"IsLandingPageHome": setting.LandingPageURL == setting.LandingPageHome,
31-
"IsLandingPageExplore": setting.LandingPageURL == setting.LandingPageExplore,
32-
"IsLandingPageOrganizations": setting.LandingPageURL == setting.LandingPageOrganizations,
33-
34-
"ShowRegistrationButton": setting.Service.ShowRegistrationButton,
35-
"ShowMilestonesDashboardPage": setting.Service.ShowMilestonesDashboardPage,
36-
"ShowFooterVersion": setting.Other.ShowFooterVersion,
37-
"DisableDownloadSourceArchives": setting.Repository.DisableDownloadSourceArchives,
38-
39-
"EnableSwagger": setting.API.EnableSwagger,
40-
"EnableOpenIDSignIn": setting.Service.EnableOpenIDSignIn,
41-
"PageStartTime": startTime,
42-
}
43-
}
44-
4514
func AssetFS() *assetfs.LayeredFS {
4615
return assetfs.Layered(CustomAssets(), BuiltinAssets())
4716
}

‎modules/test/context_tests.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func MockContext(t *testing.T, path string) *context.Context {
3030
resp := &mockResponseWriter{}
3131
ctx := context.Context{
3232
Render: &mockRender{},
33-
Data: make(map[string]interface{}),
33+
Data: make(middleware.ContextData),
3434
Flash: &middleware.Flash{
3535
Values: make(url.Values),
3636
},

‎modules/web/middleware/data.go

+59-3
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,63 @@
33

44
package middleware
55

6-
// DataStore represents a data store
7-
type DataStore interface {
8-
GetData() map[string]interface{}
6+
import (
7+
"context"
8+
"time"
9+
10+
"code.gitea.io/gitea/modules/setting"
11+
)
12+
13+
// ContextDataStore represents a data store
14+
type ContextDataStore interface {
15+
GetData() ContextData
16+
}
17+
18+
type ContextData map[string]any
19+
20+
func (ds ContextData) GetData() map[string]any {
21+
return ds
22+
}
23+
24+
func (ds ContextData) MergeFrom(other ContextData) ContextData {
25+
for k, v := range other {
26+
ds[k] = v
27+
}
28+
return ds
29+
}
30+
31+
const ContextDataKeySignedUser = "SignedUser"
32+
33+
type contextDataKeyType struct{}
34+
35+
var contextDataKey contextDataKeyType
36+
37+
func WithContextData(c context.Context) context.Context {
38+
return context.WithValue(c, contextDataKey, make(ContextData, 10))
39+
}
40+
41+
func GetContextData(c context.Context) ContextData {
42+
if ds, ok := c.Value(contextDataKey).(ContextData); ok {
43+
return ds
44+
}
45+
return nil
46+
}
47+
48+
func CommonTemplateContextData() ContextData {
49+
return ContextData{
50+
"IsLandingPageHome": setting.LandingPageURL == setting.LandingPageHome,
51+
"IsLandingPageExplore": setting.LandingPageURL == setting.LandingPageExplore,
52+
"IsLandingPageOrganizations": setting.LandingPageURL == setting.LandingPageOrganizations,
53+
54+
"ShowRegistrationButton": setting.Service.ShowRegistrationButton,
55+
"ShowMilestonesDashboardPage": setting.Service.ShowMilestonesDashboardPage,
56+
"ShowFooterVersion": setting.Other.ShowFooterVersion,
57+
"DisableDownloadSourceArchives": setting.Repository.DisableDownloadSourceArchives,
58+
59+
"EnableSwagger": setting.API.EnableSwagger,
60+
"EnableOpenIDSignIn": setting.Service.EnableOpenIDSignIn,
61+
"PageStartTime": time.Now(),
62+
63+
"RunModeIsProd": setting.IsProd,
64+
}
965
}

‎modules/web/middleware/flash.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ var FlashNow bool
1818

1919
// Flash represents a one time data transfer between two requests.
2020
type Flash struct {
21-
DataStore
21+
DataStore ContextDataStore
2222
url.Values
2323
ErrorMsg, WarningMsg, InfoMsg, SuccessMsg string
2424
}
@@ -34,7 +34,7 @@ func (f *Flash) set(name, msg string, current ...bool) {
3434
}
3535

3636
if isShow {
37-
f.GetData()["Flash"] = f
37+
f.DataStore.GetData()["Flash"] = f
3838
} else {
3939
f.Set(name, msg)
4040
}

0 commit comments

Comments
 (0)