Return json on 500 error from API

[6543]

if ctx.InternalServerError(err) is used it will return the default 500 html page

on API a json Error is expected

[6543]

just noticed this behaviour - it was not documented at all - of coures if you do a code lookup you can see it too :)

[6543]

@techknowlogick can we backport this?

[techknowlogick]

whoops. label didn't apply at first. It's there now:)

[6543]

better error logging

[silverwind]

Bit of a shame that this needs to be done in every route. The way I'd handle this in JS routing is to let errors throw in the routes and then process them in a "catch-all" route but I guess with missing exceptions, this is not possible in Golang.

[zeripath]

The other option is for the API context to provide its own internalservererror function

[6543]

we should keep it simple ...

[zeripath]

It would be simpler...

in modules/context/api.go add the following function:

// InternalServerError responds with an error message to the client with the error as a message and the file and line of the caller.
func (ctx *APIContext) InternalServerError(err error) {
	log.ErrorWithSkip(1, "InternalServerError: %v", err)

	ctx.JSON(http.StatusInternalServerError, APIError{
		Message: err.Error(),
		URL:     setting.API.SwaggerURL,
	})
}

And whilst you're at it change the function above it to:

// 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
	if err, ok := obj.(error); ok {
		message = err.Error()
	} else {
		message = obj.(string)
	}

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

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

[zeripath]

Much simpler

[lafriks]

But won't this reveal internal error messages in api? That would be bad from security point of view

[silverwind]

What secrets would they reveal? Does it matter in an open-source application?

[6543]

But won't this reveal internal error messages in api? That would be bad from security point of view

@lafriks
they already do it now!!
just in html output and now in json

[lafriks]

I don't think actual error message is returned currently, if it is it should be fixed not to be. Even returning things like paths where Gitea is installed can be dangerous as it could give out information about where files are located. Also knowing what exact unexpected error is happening is so much easier to use it.
This does not matter if app is open source or not.
More info: https://cwe.mitre.org/data/definitions/209.html

[zeripath]

@lafriks the actual error message is returned currently - but yeah we shouldn't do that.
@6543 add a check like:

gitea/modules/context/context.go

Line 180 in 02a52d6

if macaron.Env != macaron.PROD {

Before setting the message.

[6543]

@lafriks I think to hide 500 errors on API will need its own pull to correct 500 errors witch should have other http status codes ...

[6543]

@zeripath #11641 (comment) and 497f226

[zeripath]

Shame about the api.Error endpoint - but if you don't have time to fix those right now - at least we can get a fix in for the ostensible reason for this PR.

These changes now at least don't make the situation worse.

[6543]

should we merge 🚀?

or wait for #11643

EDIT: #11644 is kind/breaking in my opinion and so #11641 should not be backported

[lafriks]

I don't see where InternalServerError function is used anymore :)

[6543]

?

[zeripath]

This PR provides a modules/context.APIContext.InternalServerError(...) method override for a method in modules/context.Context.

An example of a piece of code affected is in routers/api/v1/misc/markdown.go:79:ctx.InternalServerError(err)

Where ctx is actually a modules/context.APIContext and modules/context.APIContext is:

// APIContext is a specific macaron context for API service
type APIContext struct {
	*Context
	Org *APIOrganization
}

As there was previously no InternalServerError method defined for APIContext then the one used for Context was used. This PR stops that by providing a method specific for these to APIContext.

[lafriks]

sorry my bad :)

[6543]

I would say ready to merge

[guillep2k]

Please send backport. Can you check whether the "Allow maintainers to edit this post" checkbox shows and is checked up before posting? It's an ongoing mistery.... 🕵️

[6543]

I agree It's an ongoing mistery!

[6543]

have created backport to v1.11 too - old untuched code allowed it easely ;)