Skip to content

Make error rendering context-aware #21803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 71 additions & 19 deletions modules/context/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ package context
import (
"context"
"fmt"
"html/template"
"io"
"net/http"
"net/url"
"strings"
Expand All @@ -21,6 +23,8 @@ import (
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/web/middleware"
auth_service "code.gitea.io/gitea/services/auth"

"github.com/unrolled/render"
)

// APIContext is a specific context for API service
Expand Down Expand Up @@ -89,25 +93,19 @@ func (ctx *APIContext) ServerError(title string, err error) {
// Error responds with an error message to client with given obj as the message.
// If status is 500, also it prints error to log.
func (ctx *APIContext) Error(status int, title string, obj interface{}) {
var message string
var foundError error
message := title
if err, ok := obj.(error); ok {
message = err.Error()
foundError = err
} else {
message = fmt.Sprintf("%s", obj)
}

if status == http.StatusInternalServerError {
log.ErrorWithSkip(1, "%s: %s", title, message)

if setting.IsProd && !(ctx.Doer != nil && ctx.Doer.IsAdmin) {
message = ""
}
log.ErrorWithSkip(1, "%s: %v", title, foundError)
}

ctx.JSON(status, APIError{
Message: message,
URL: setting.API.SwaggerURL,
})
ctx.ErrorHandler.Error(ctx.Context, status, message, foundError)
}

// InternalServerError responds with an error message to the client with the error as a message
Expand Down Expand Up @@ -239,17 +237,31 @@ func APIAuth(authMethod auth_service.Method) func(*APIContext) {
}
}

// Implements a stub Render implementation to demonstrate that the API context lacks this feature.
// Instead of displaying a vague NPE panic when called, it now displays a clear panic message.
type panicRender struct{}

func (panicRender) TemplateLookup(string) *template.Template {
panic("Render.TemplateLookup is not implemented for API context.")
}

func (panicRender) HTML(w io.Writer, status int, name string, binding interface{}, htmlOpt ...render.HTMLOptions) error {
panic("Render.HTML is not implemented for API context.")
}

// APIContexter returns apicontext as middleware
func APIContexter() func(http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
locale := middleware.Locale(w, req)
ctx := APIContext{
Context: &Context{
Resp: NewResponse(w),
Data: map[string]interface{}{},
Locale: locale,
Cache: cache.GetCache(),
Resp: NewResponse(w),
Render: panicRender{},
ErrorHandler: apiErrorHandler{},
Data: map[string]interface{}{},
Locale: locale,
Cache: cache.GetCache(),
Repo: &Repository{
PullRequest: &PullRequest{},
},
Expand Down Expand Up @@ -294,24 +306,64 @@ func APIContexter() func(http.Handler) http.Handler {
// String will replace message, errors will be added to a slice
func (ctx *APIContext) NotFound(objs ...interface{}) {
message := ctx.Tr("error.not_found")
var errors []string
var foundError error
for _, obj := range objs {
// Ignore nil
if obj == nil {
continue
}

if err, ok := obj.(error); ok {
errors = append(errors, err.Error())
foundError = err
} else {
message = obj.(string)
}
}

ctx.ErrorHandler.NotFound(ctx.Context, message, foundError)
}

type apiErrorHandler struct{}

var _ ErrorHandler = apiErrorHandler{}

func (apiErrorHandler) NotFound(ctx *Context, logMessage string, logErr error) {
ctx.JSON(http.StatusNotFound, map[string]interface{}{
"message": logMessage,
"url": setting.API.SwaggerURL,
"error": logErr,
})
}

func (apiErrorHandler) ServerError(ctx *Context, logMessage string, logErr error) {
message := logMessage

if setting.IsProd && (ctx.Doer == nil || !ctx.Doer.IsAdmin) {
message = ""
logErr = nil
}

ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"message": message,
"url": setting.API.SwaggerURL,
"error": logErr,
})
}

func (apiErrorHandler) Error(ctx *Context, status int, logMessage string, logErr error) {
message := logMessage

// Errors are given as message and priority over logMessage.
if logErr != nil {
message = logErr.Error()
}

if status == http.StatusInternalServerError && setting.IsProd && (ctx.Doer == nil || !ctx.Doer.IsAdmin) {
message = ""
}

ctx.JSON(status, map[string]interface{}{
"message": message,
"url": setting.API.SwaggerURL,
"errors": errors,
})
}

Expand Down
145 changes: 82 additions & 63 deletions modules/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,23 @@ type Render interface {
HTML(w io.Writer, status int, name string, binding interface{}, htmlOpt ...render.HTMLOptions) error
}

type ErrorHandler interface {
// Logs and display an 404 error.
NotFound(ctx *Context, logMessage string, logErr error)
// Logs and display an 500 error.
ServerError(ctx *Context, logMessage string, logErr error)
// Logs and display the specified status, logErr can be nil.
Error(ctx *Context, status int, logMessage string, logErr error)
}

// Context represents context of a request.
type Context struct {
Resp ResponseWriter
Req *http.Request
Data map[string]interface{} // data used by MVC templates
PageData map[string]interface{} // data used by JavaScript modules in one page, it's `window.config.pageData`
Render Render
Resp ResponseWriter
Req *http.Request
Data map[string]interface{} // data used by MVC templates
PageData map[string]interface{} // data used by JavaScript modules in one page, it's `window.config.pageData`
Render Render
ErrorHandler ErrorHandler
translation.Locale
Cache cache.Cache
csrf CSRFProtector
Expand Down Expand Up @@ -253,68 +263,23 @@ func (ctx *Context) RenderWithErr(msg string, tpl base.TplName, form interface{}

// NotFound displays a 404 (Not Found) page and prints the given error, if any.
func (ctx *Context) NotFound(logMsg string, logErr error) {
ctx.notFoundInternal(logMsg, logErr)
}

func (ctx *Context) notFoundInternal(logMsg string, logErr error) {
if logErr != nil {
log.Log(2, log.DEBUG, "%s: %v", logMsg, logErr)
if !setting.IsProd {
ctx.Data["ErrorMsg"] = logErr
}
}

// response simple message if Accept isn't text/html
showHTML := false
for _, part := range ctx.Req.Header["Accept"] {
if strings.Contains(part, "text/html") {
showHTML = true
break
}
}

if !showHTML {
ctx.plainTextInternal(3, http.StatusNotFound, []byte("Not found.\n"))
return
}

ctx.Data["IsRepo"] = ctx.Repo.Repository != nil
ctx.Data["Title"] = "Page Not Found"
ctx.HTML(http.StatusNotFound, base.TplName("status/404"))
ctx.ErrorHandler.NotFound(ctx, logMsg, logErr)
}

// ServerError displays a 500 (Internal Server Error) page and prints the given error, if any.
func (ctx *Context) ServerError(logMsg string, logErr error) {
ctx.serverErrorInternal(logMsg, logErr)
}

func (ctx *Context) serverErrorInternal(logMsg string, logErr error) {
if logErr != nil {
log.ErrorWithSkip(2, "%s: %v", logMsg, logErr)
if _, ok := logErr.(*net.OpError); ok || errors.Is(logErr, &net.OpError{}) {
// This is an error within the underlying connection
// and further rendering will not work so just return
return
}

if !setting.IsProd {
ctx.Data["ErrorMsg"] = logErr
}
}

ctx.Data["Title"] = "Internal Server Error"
ctx.HTML(http.StatusInternalServerError, base.TplName("status/500"))
ctx.ErrorHandler.ServerError(ctx, logMsg, logErr)
}

// 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)
ctx.NotFound(logMsg, err)
return
}
ctx.serverErrorInternal(logMsg, err)
ctx.ServerError(logMsg, err)
}

// PlainTextBytes renders bytes as plain text
Expand Down Expand Up @@ -427,11 +392,11 @@ func (ctx *Context) UploadStream() (rd io.ReadCloser, needToClose bool, err erro

// Error returned an error to web browser
func (ctx *Context) Error(status int, contents ...string) {
v := http.StatusText(status)
message := http.StatusText(status)
if len(contents) > 0 {
v = contents[0]
message = contents[0]
}
http.Error(ctx.Resp, v, status)
ctx.ErrorHandler.Error(ctx, status, message, nil)
}

// JSON render content as JSON
Expand Down Expand Up @@ -618,6 +583,59 @@ func (ctx *Context) AppendAccessControlExposeHeaders(names ...string) {
}
}

type webErrorHandler struct{}

var _ ErrorHandler = webErrorHandler{}

func (webErrorHandler) NotFound(ctx *Context, logMsg string, logErr error) {
if logErr != nil {
log.Log(2, log.DEBUG, "%s: %v", logMsg, logErr)
if !setting.IsProd {
ctx.Data["ErrorMsg"] = logErr
}
}

// response simple message if Accept isn't text/html
showHTML := false
for _, part := range ctx.Req.Header["Accept"] {
if strings.Contains(part, "text/html") {
showHTML = true
break
}
}

if !showHTML {
ctx.plainTextInternal(3, http.StatusNotFound, []byte("Not found.\n"))
return
}

ctx.Data["IsRepo"] = ctx.Repo.Repository != nil
ctx.Data["Title"] = "Page Not Found"
ctx.HTML(http.StatusNotFound, base.TplName("status/404"))
}

func (webErrorHandler) ServerError(ctx *Context, logMsg string, logErr error) {
if logErr != nil {
log.ErrorWithSkip(2, "%s: %v", logMsg, logErr)
if _, ok := logErr.(*net.OpError); ok || errors.Is(logErr, &net.OpError{}) {
// This is an error within the underlying connection
// and further rendering will not work so just return
return
}

if !setting.IsProd {
ctx.Data["ErrorMsg"] = logErr
}
}

ctx.Data["Title"] = "Internal Server Error"
ctx.HTML(http.StatusInternalServerError, base.TplName("status/500"))
}

func (webErrorHandler) Error(ctx *Context, status int, logMsg string, _ error) {
http.Error(ctx.Resp, logMsg, status)
}

// Handler represents a custom handler
type Handler func(*Context)

Expand Down Expand Up @@ -699,12 +717,13 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler {
link := setting.AppSubURL + strings.TrimSuffix(req.URL.EscapedPath(), "/")

ctx := Context{
Resp: NewResponse(resp),
Cache: mc.GetCache(),
Locale: locale,
Link: link,
Render: rnd,
Session: session.GetSession(req),
Resp: NewResponse(resp),
Cache: mc.GetCache(),
Locale: locale,
Link: link,
Render: rnd,
ErrorHandler: webErrorHandler{},
Session: session.GetSession(req),
Repo: &Repository{
PullRequest: &PullRequest{},
},
Expand Down
7 changes: 4 additions & 3 deletions modules/context/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,10 @@ func PackageContexter(ctx gocontext.Context) func(next http.Handler) http.Handle
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
ctx := Context{
Resp: NewResponse(resp),
Data: map[string]interface{}{},
Render: rnd,
Resp: NewResponse(resp),
Data: map[string]interface{}{},
Render: rnd,
ErrorHandler: apiErrorHandler{},
}
defer ctx.Close()

Expand Down