aboutsummaryrefslogtreecommitdiffstats
path: root/modules
diff options
context:
space:
mode:
authorwxiaoguang <wxiaoguang@gmail.com>2023-04-14 13:19:11 +0800
committerGitHub <noreply@github.com>2023-04-14 13:19:11 +0800
commit1c8bc4081a4f4d0d921ac218cb724ce97924d410 (patch)
tree038587701606c7abb11b29f5b63a14e12cb2239e /modules
parent5768bafeb28e4e4212ae0e2abc7f22c9c8b7c653 (diff)
downloadgitea-1c8bc4081a4f4d0d921ac218cb724ce97924d410.tar.gz
gitea-1c8bc4081a4f4d0d921ac218cb724ce97924d410.zip
Show friendly 500 error page to users and developers (#24110)
Close #24104 This also introduces many tests to cover many complex error handling functions. ### Before The details are never shown in production. <details> ![image](https://user-images.githubusercontent.com/2114189/231805004-13214579-4fbe-465a-821c-be75c2749097.png) </details> ### After The details could be shown to site admin users. It is safe. ![image](https://user-images.githubusercontent.com/2114189/231803912-d5660994-416f-4b27-a4f1-a4cc962091d4.png)
Diffstat (limited to 'modules')
-rw-r--r--modules/context/context.go44
-rw-r--r--modules/templates/htmlrenderer.go251
-rw-r--r--modules/templates/htmlrenderer_test.go106
3 files changed, 242 insertions, 159 deletions
diff --git a/modules/context/context.go b/modules/context/context.go
index cee533e42a..2507cc10c0 100644
--- a/modules/context/context.go
+++ b/modules/context/context.go
@@ -16,10 +16,8 @@ import (
"net/http"
"net/url"
"path"
- "regexp"
"strconv"
"strings"
- texttemplate "text/template"
"time"
"code.gitea.io/gitea/models/db"
@@ -216,7 +214,7 @@ func (ctx *Context) RedirectToFirst(location ...string) {
ctx.Redirect(setting.AppSubURL + "/")
}
-var templateExecutingErr = regexp.MustCompile(`^template: (.*):([1-9][0-9]*):([1-9][0-9]*): executing (?:"(.*)" at <(.*)>: )?`)
+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) {
@@ -229,34 +227,11 @@ func (ctx *Context) HTML(status int, name base.TplName) {
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 == base.TplName("status/500") {
+ 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
}
- if execErr, ok := err.(texttemplate.ExecError); ok {
- if groups := templateExecutingErr.FindStringSubmatch(err.Error()); len(groups) > 0 {
- errorTemplateName, lineStr, posStr := groups[1], groups[2], groups[3]
- target := ""
- if len(groups) == 6 {
- target = groups[5]
- }
- line, _ := strconv.Atoi(lineStr) // Cannot error out as groups[2] is [1-9][0-9]*
- pos, _ := strconv.Atoi(posStr) // Cannot error out as groups[3] is [1-9][0-9]*
- assetLayerName := templates.AssetFS().GetFileLayerName(errorTemplateName + ".tmpl")
- filename := fmt.Sprintf("(%s) %s", assetLayerName, errorTemplateName)
- if errorTemplateName != string(name) {
- filename += " (subtemplate of " + string(name) + ")"
- }
- err = fmt.Errorf("failed to render %s, error: %w:\n%s", filename, err, templates.GetLineFromTemplate(errorTemplateName, line, target, pos))
- } else {
- assetLayerName := templates.AssetFS().GetFileLayerName(execErr.Name + ".tmpl")
- filename := fmt.Sprintf("(%s) %s", assetLayerName, execErr.Name)
- if execErr.Name != string(name) {
- filename += " (subtemplate of " + string(name) + ")"
- }
- err = fmt.Errorf("failed to render %s, error: %w", filename, err)
- }
- }
+ err = fmt.Errorf("failed to render template: %s, error: %s", name, templates.HandleTemplateRenderingError(err))
ctx.ServerError("Render failed", err)
}
}
@@ -324,24 +299,25 @@ func (ctx *Context) serverErrorInternal(logMsg string, logErr error) {
return
}
- if !setting.IsProd {
+ // it's safe to show internal error to admin users, and it helps
+ if !setting.IsProd || (ctx.Doer != nil && ctx.Doer.IsAdmin) {
ctx.Data["ErrorMsg"] = logErr
}
}
ctx.Data["Title"] = "Internal Server Error"
- ctx.HTML(http.StatusInternalServerError, base.TplName("status/500"))
+ ctx.HTML(http.StatusInternalServerError, tplStatus500)
}
// NotFoundOrServerError use error check function to determine if the error
// is about not found. It responds with 404 status code for not found error,
// or error context description for logging purpose of 500 server error.
-func (ctx *Context) NotFoundOrServerError(logMsg string, errCheck func(error) bool, err error) {
- if errCheck(err) {
- ctx.notFoundInternal(logMsg, err)
+func (ctx *Context) NotFoundOrServerError(logMsg string, errCheck func(error) bool, logErr error) {
+ if errCheck(logErr) {
+ ctx.notFoundInternal(logMsg, logErr)
return
}
- ctx.serverErrorInternal(logMsg, err)
+ ctx.serverErrorInternal(logMsg, logErr)
}
// PlainTextBytes renders bytes as plain text
diff --git a/modules/templates/htmlrenderer.go b/modules/templates/htmlrenderer.go
index 26dd365e4c..833c2acdca 100644
--- a/modules/templates/htmlrenderer.go
+++ b/modules/templates/htmlrenderer.go
@@ -4,6 +4,7 @@
package templates
import (
+ "bufio"
"bytes"
"context"
"errors"
@@ -18,19 +19,13 @@ import (
"sync/atomic"
texttemplate "text/template"
+ "code.gitea.io/gitea/modules/assetfs"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
)
-var (
- rendererKey interface{} = "templatesHtmlRenderer"
-
- templateError = regexp.MustCompile(`^template: (.*):([0-9]+): (.*)`)
- notDefinedError = regexp.MustCompile(`^template: (.*):([0-9]+): function "(.*)" not defined`)
- unexpectedError = regexp.MustCompile(`^template: (.*):([0-9]+): unexpected "(.*)" in operand`)
- expectedEndError = regexp.MustCompile(`^template: (.*):([0-9]+): expected end; found (.*)`)
-)
+var rendererKey interface{} = "templatesHtmlRenderer"
type HTMLRender struct {
templates atomic.Pointer[template.Template]
@@ -107,11 +102,12 @@ func HTMLRenderer(ctx context.Context) (context.Context, *HTMLRender) {
renderer := &HTMLRender{}
if err := renderer.CompileTemplates(); err != nil {
- wrapFatal(handleNotDefinedPanicError(err))
- wrapFatal(handleUnexpected(err))
- wrapFatal(handleExpectedEnd(err))
- wrapFatal(handleGenericTemplateError(err))
- log.Fatal("HTMLRenderer error: %v", err)
+ p := &templateErrorPrettier{assets: AssetFS()}
+ wrapFatal(p.handleFuncNotDefinedError(err))
+ wrapFatal(p.handleUnexpectedOperandError(err))
+ wrapFatal(p.handleExpectedEndError(err))
+ wrapFatal(p.handleGenericTemplateError(err))
+ log.Fatal("HTMLRenderer CompileTemplates error: %v", err)
}
if !setting.IsProd {
go AssetFS().WatchLocalChanges(ctx, func() {
@@ -123,148 +119,153 @@ func HTMLRenderer(ctx context.Context) (context.Context, *HTMLRender) {
return context.WithValue(ctx, rendererKey, renderer), renderer
}
-func wrapFatal(format string, args []interface{}) {
- if format == "" {
+func wrapFatal(msg string) {
+ if msg == "" {
return
}
- log.FatalWithSkip(1, format, args...)
+ log.FatalWithSkip(1, "Unable to compile templates, %s", msg)
}
-func handleGenericTemplateError(err error) (string, []interface{}) {
- groups := templateError.FindStringSubmatch(err.Error())
- if len(groups) != 4 {
- return "", nil
- }
-
- templateName, lineNumberStr, message := groups[1], groups[2], groups[3]
- filename := fmt.Sprintf("%s (provided by %s)", templateName, AssetFS().GetFileLayerName(templateName+".tmpl"))
- lineNumber, _ := strconv.Atoi(lineNumberStr)
- line := GetLineFromTemplate(templateName, lineNumber, "", -1)
-
- return "PANIC: Unable to compile templates!\n%s in template file %s at line %d:\n\n%s\nStacktrace:\n\n%s", []interface{}{message, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)}
+type templateErrorPrettier struct {
+ assets *assetfs.LayeredFS
}
-func handleNotDefinedPanicError(err error) (string, []interface{}) {
- groups := notDefinedError.FindStringSubmatch(err.Error())
- if len(groups) != 4 {
- return "", nil
- }
-
- templateName, lineNumberStr, functionName := groups[1], groups[2], groups[3]
- functionName, _ = strconv.Unquote(`"` + functionName + `"`)
- filename := fmt.Sprintf("%s (provided by %s)", templateName, AssetFS().GetFileLayerName(templateName+".tmpl"))
- lineNumber, _ := strconv.Atoi(lineNumberStr)
- line := GetLineFromTemplate(templateName, lineNumber, functionName, -1)
+var reGenericTemplateError = regexp.MustCompile(`^template: (.*):([0-9]+): (.*)`)
- return "PANIC: Unable to compile templates!\nUndefined function %q in template file %s at line %d:\n\n%s", []interface{}{functionName, filename, lineNumber, log.NewColoredValue(line, log.Reset)}
-}
-
-func handleUnexpected(err error) (string, []interface{}) {
- groups := unexpectedError.FindStringSubmatch(err.Error())
+func (p *templateErrorPrettier) handleGenericTemplateError(err error) string {
+ groups := reGenericTemplateError.FindStringSubmatch(err.Error())
if len(groups) != 4 {
- return "", nil
+ return ""
}
-
- templateName, lineNumberStr, unexpected := groups[1], groups[2], groups[3]
- unexpected, _ = strconv.Unquote(`"` + unexpected + `"`)
- filename := fmt.Sprintf("%s (provided by %s)", templateName, AssetFS().GetFileLayerName(templateName+".tmpl"))
- lineNumber, _ := strconv.Atoi(lineNumberStr)
- line := GetLineFromTemplate(templateName, lineNumber, unexpected, -1)
-
- return "PANIC: Unable to compile templates!\nUnexpected %q in template file %s at line %d:\n\n%s", []interface{}{unexpected, filename, lineNumber, log.NewColoredValue(line, log.Reset)}
+ tmplName, lineStr, message := groups[1], groups[2], groups[3]
+ return p.makeDetailedError(message, tmplName, lineStr, -1, "")
}
-func handleExpectedEnd(err error) (string, []interface{}) {
- groups := expectedEndError.FindStringSubmatch(err.Error())
- if len(groups) != 4 {
- return "", nil
- }
-
- templateName, lineNumberStr, unexpected := groups[1], groups[2], groups[3]
- filename := fmt.Sprintf("%s (provided by %s)", templateName, AssetFS().GetFileLayerName(templateName+".tmpl"))
- lineNumber, _ := strconv.Atoi(lineNumberStr)
- line := GetLineFromTemplate(templateName, lineNumber, unexpected, -1)
+var reFuncNotDefinedError = regexp.MustCompile(`^template: (.*):([0-9]+): (function "(.*)" not defined)`)
- return "PANIC: Unable to compile templates!\nMissing end with unexpected %q in template file %s at line %d:\n\n%s", []interface{}{unexpected, filename, lineNumber, log.NewColoredValue(line, log.Reset)}
+func (p *templateErrorPrettier) handleFuncNotDefinedError(err error) string {
+ groups := reFuncNotDefinedError.FindStringSubmatch(err.Error())
+ if len(groups) != 5 {
+ return ""
+ }
+ tmplName, lineStr, message, funcName := groups[1], groups[2], groups[3], groups[4]
+ funcName, _ = strconv.Unquote(`"` + funcName + `"`)
+ return p.makeDetailedError(message, tmplName, lineStr, -1, funcName)
}
-const dashSeparator = "----------------------------------------------------------------------\n"
+var reUnexpectedOperandError = regexp.MustCompile(`^template: (.*):([0-9]+): (unexpected "(.*)" in operand)`)
-// GetLineFromTemplate returns a line from a template with some context
-func GetLineFromTemplate(templateName string, targetLineNum int, target string, position int) string {
- bs, err := AssetFS().ReadFile(templateName + ".tmpl")
- if err != nil {
- return fmt.Sprintf("(unable to read template file: %v)", err)
+func (p *templateErrorPrettier) handleUnexpectedOperandError(err error) string {
+ groups := reUnexpectedOperandError.FindStringSubmatch(err.Error())
+ if len(groups) != 5 {
+ return ""
}
+ tmplName, lineStr, message, unexpected := groups[1], groups[2], groups[3], groups[4]
+ unexpected, _ = strconv.Unquote(`"` + unexpected + `"`)
+ return p.makeDetailedError(message, tmplName, lineStr, -1, unexpected)
+}
- sb := &strings.Builder{}
-
- // Write the header
- sb.WriteString(dashSeparator)
+var reExpectedEndError = regexp.MustCompile(`^template: (.*):([0-9]+): (expected end; found (.*))`)
- var lineBs []byte
+func (p *templateErrorPrettier) handleExpectedEndError(err error) string {
+ groups := reExpectedEndError.FindStringSubmatch(err.Error())
+ if len(groups) != 5 {
+ return ""
+ }
+ tmplName, lineStr, message, unexpected := groups[1], groups[2], groups[3], groups[4]
+ return p.makeDetailedError(message, tmplName, lineStr, -1, unexpected)
+}
- // Iterate through the lines from the asset file to find the target line
- for start, currentLineNum := 0, 1; currentLineNum <= targetLineNum && start < len(bs); currentLineNum++ {
- // Find the next new line
- end := bytes.IndexByte(bs[start:], '\n')
+var (
+ reTemplateExecutingError = regexp.MustCompile(`^template: (.*):([1-9][0-9]*):([1-9][0-9]*): (executing .*)`)
+ reTemplateExecutingErrorMsg = regexp.MustCompile(`^executing "(.*)" at <(.*)>: `)
+)
- // adjust the end to be a direct pointer in to []byte
- if end < 0 {
- end = len(bs)
- } else {
- end += start
+func (p *templateErrorPrettier) handleTemplateRenderingError(err error) string {
+ if groups := reTemplateExecutingError.FindStringSubmatch(err.Error()); len(groups) > 0 {
+ tmplName, lineStr, posStr, msgPart := groups[1], groups[2], groups[3], groups[4]
+ target := ""
+ if groups = reTemplateExecutingErrorMsg.FindStringSubmatch(msgPart); len(groups) > 0 {
+ target = groups[2]
}
+ return p.makeDetailedError(msgPart, tmplName, lineStr, posStr, target)
+ } else if execErr, ok := err.(texttemplate.ExecError); ok {
+ layerName := p.assets.GetFileLayerName(execErr.Name + ".tmpl")
+ return fmt.Sprintf("asset from: %s, %s", layerName, err.Error())
+ } else {
+ return err.Error()
+ }
+}
- // set lineBs to the current line []byte
- lineBs = bs[start:end]
+func HandleTemplateRenderingError(err error) string {
+ p := &templateErrorPrettier{assets: AssetFS()}
+ return p.handleTemplateRenderingError(err)
+}
- // move start to after the current new line position
- start = end + 1
+const dashSeparator = "----------------------------------------------------------------------"
- // Write 2 preceding lines + the target line
- if targetLineNum-currentLineNum < 3 {
- _, _ = sb.Write(lineBs)
- _ = sb.WriteByte('\n')
- }
+func (p *templateErrorPrettier) makeDetailedError(errMsg, tmplName string, lineNum, posNum any, target string) string {
+ code, layer, err := p.assets.ReadLayeredFile(tmplName + ".tmpl")
+ if err != nil {
+ return fmt.Sprintf("template error: %s, and unable to find template file %q", errMsg, tmplName)
+ }
+ line, err := util.ToInt64(lineNum)
+ if err != nil {
+ return fmt.Sprintf("template error: %s, unable to parse template %q line number %q", errMsg, tmplName, lineNum)
+ }
+ pos, err := util.ToInt64(posNum)
+ if err != nil {
+ return fmt.Sprintf("template error: %s, unable to parse template %q pos number %q", errMsg, tmplName, posNum)
}
+ detail := extractErrorLine(code, int(line), int(pos), target)
- // FIXME: this algorithm could provide incorrect results and mislead the developers.
- // For example: Undefined function "file" in template .....
- // {{Func .file.Addition file.Deletion .file.Addition}}
- // ^^^^ ^(the real error is here)
- // The pointer is added to the first one, but the second one is the real incorrect one.
- //
- // If there is a provided target to look for in the line add a pointer to it
- // e.g. ^^^^^^^
- if target != "" {
- targetPos := bytes.Index(lineBs, []byte(target))
- if targetPos >= 0 {
- position = targetPos
- }
+ var msg string
+ if pos >= 0 {
+ msg = fmt.Sprintf("template error: %s:%s:%d:%d : %s", layer, tmplName, line, pos, errMsg)
+ } else {
+ msg = fmt.Sprintf("template error: %s:%s:%d : %s", layer, tmplName, line, errMsg)
}
- if position >= 0 {
- // take the current line and replace preceding text with whitespace (except for tab)
- for i := range lineBs[:position] {
- if lineBs[i] != '\t' {
- lineBs[i] = ' '
+ return msg + "\n" + dashSeparator + "\n" + detail + "\n" + dashSeparator
+}
+
+func extractErrorLine(code []byte, lineNum, posNum int, target string) string {
+ b := bufio.NewReader(bytes.NewReader(code))
+ var line []byte
+ var err error
+ for i := 0; i < lineNum; i++ {
+ if line, err = b.ReadBytes('\n'); err != nil {
+ if i == lineNum-1 && errors.Is(err, io.EOF) {
+ err = nil
}
+ break
}
+ }
+ if err != nil {
+ return fmt.Sprintf("unable to find target line %d", lineNum)
+ }
- // write the preceding "space"
- _, _ = sb.Write(lineBs[:position])
-
- // Now write the ^^ pointer
- targetLen := len(target)
- if targetLen == 0 {
- targetLen = 1
+ line = bytes.TrimRight(line, "\r\n")
+ var indicatorLine []byte
+ targetBytes := []byte(target)
+ targetLen := len(targetBytes)
+ for i := 0; i < len(line); {
+ if posNum == -1 && target != "" && bytes.HasPrefix(line[i:], targetBytes) {
+ for j := 0; j < targetLen && i < len(line); j++ {
+ indicatorLine = append(indicatorLine, '^')
+ i++
+ }
+ } else if i == posNum {
+ indicatorLine = append(indicatorLine, '^')
+ i++
+ } else {
+ if line[i] == '\t' {
+ indicatorLine = append(indicatorLine, '\t')
+ } else {
+ indicatorLine = append(indicatorLine, ' ')
+ }
+ i++
}
- _, _ = sb.WriteString(strings.Repeat("^", targetLen))
- _ = sb.WriteByte('\n')
}
-
- // Finally write the footer
- sb.WriteString(dashSeparator)
-
- return sb.String()
+ // if the indicatorLine only contains spaces, trim it together
+ return strings.TrimRight(string(line)+"\n"+string(indicatorLine), " \t\r\n")
}
diff --git a/modules/templates/htmlrenderer_test.go b/modules/templates/htmlrenderer_test.go
new file mode 100644
index 0000000000..2a74b74c23
--- /dev/null
+++ b/modules/templates/htmlrenderer_test.go
@@ -0,0 +1,106 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package templates
+
+import (
+ "errors"
+ "html/template"
+ "os"
+ "strings"
+ "testing"
+
+ "code.gitea.io/gitea/modules/assetfs"
+
+ "github.com/stretchr/testify/assert"
+)
+
+func TestExtractErrorLine(t *testing.T) {
+ cases := []struct {
+ code string
+ line int
+ pos int
+ target string
+ expect string
+ }{
+ {"hello world\nfoo bar foo bar\ntest", 2, -1, "bar", `
+foo bar foo bar
+ ^^^ ^^^
+`},
+
+ {"hello world\nfoo bar foo bar\ntest", 2, 4, "bar", `
+foo bar foo bar
+ ^
+`},
+
+ {
+ "hello world\nfoo bar foo bar\ntest", 2, 4, "",
+ `
+foo bar foo bar
+ ^
+`,
+ },
+
+ {
+ "hello world\nfoo bar foo bar\ntest", 5, 0, "",
+ `unable to find target line 5`,
+ },
+ }
+
+ for _, c := range cases {
+ actual := extractErrorLine([]byte(c.code), c.line, c.pos, c.target)
+ assert.Equal(t, strings.TrimSpace(c.expect), strings.TrimSpace(actual))
+ }
+}
+
+func TestHandleError(t *testing.T) {
+ dir := t.TempDir()
+
+ p := &templateErrorPrettier{assets: assetfs.Layered(assetfs.Local("tmp", dir))}
+
+ test := func(s string, h func(error) string, expect string) {
+ err := os.WriteFile(dir+"/test.tmpl", []byte(s), 0o644)
+ assert.NoError(t, err)
+ tmpl := template.New("test")
+ _, err = tmpl.Parse(s)
+ assert.Error(t, err)
+ msg := h(err)
+ assert.EqualValues(t, strings.TrimSpace(expect), strings.TrimSpace(msg))
+ }
+
+ test("{{", p.handleGenericTemplateError, `
+template error: tmp:test:1 : unclosed action
+----------------------------------------------------------------------
+{{
+----------------------------------------------------------------------
+`)
+
+ test("{{Func}}", p.handleFuncNotDefinedError, `
+template error: tmp:test:1 : function "Func" not defined
+----------------------------------------------------------------------
+{{Func}}
+ ^^^^
+----------------------------------------------------------------------
+`)
+
+ test("{{'x'3}}", p.handleUnexpectedOperandError, `
+template error: tmp:test:1 : unexpected "3" in operand
+----------------------------------------------------------------------
+{{'x'3}}
+ ^
+----------------------------------------------------------------------
+`)
+
+ // no idea about how to trigger such strange error, so mock an error to test it
+ err := os.WriteFile(dir+"/test.tmpl", []byte("god knows XXX"), 0o644)
+ assert.NoError(t, err)
+ expectedMsg := `
+template error: tmp:test:1 : expected end; found XXX
+----------------------------------------------------------------------
+god knows XXX
+ ^^^
+----------------------------------------------------------------------
+`
+ actualMsg := p.handleExpectedEndError(errors.New("template: test:1: expected end; found XXX"))
+ assert.EqualValues(t, strings.TrimSpace(expectedMsg), strings.TrimSpace(actualMsg))
+}