🚀 [Feature]: middleware/recover: ignore error from panics

[tnako]

Description

Once panic occur there is possibility to recover and continue serving request. Current recover middleware also transform panic message into error and pass into response body with status code 500. This can confuse users or disclose secure information.

With skipping error message response become good (code 200) and body is empty. Extended with custom StackTraceHandler allow to write any response and status code.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist:

• For new functionalities I follow the inspiration of the express js framework and built them similar in usage
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation - /docs/ directory for https://docs.gofiber.io/
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• If new dependencies exist, I have checked that they are really necessary and agreed with the maintainers/community (we want to have as few dependencies as possible)
• I tried to make my code as fast as possible with as few allocations as possible
• For new code I have written benchmarks so that they can be analyzed and improved

[welcome]

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

[ReneWerner87]

I understand the security concerns
but to set the response to "OK" in case of an error, i.e. successful, i don't think is right either

then rather get the 500 and keep the body general "Internal server error"

[tnako]

hen rather get the 500 and keep the body general "Internal server error"

500 is not an option as proxy servers will close connection, increasing communication costs

The same for default text. Raw English text confuse way more especially when browser/service expect json for example.

With backward compatibility such custom code and text can be done with

...
	app.Use(recover.New(recover.Config{EnableStackTrace: true, SkipResponseError: true, StackTraceHandler: processPanics}))
...
func processPanics(c *fiber.Ctx, _ interface{}) {
	_ = c.Status(http.StatusInternalServerError).SendString("Internal server error")
}

[li-jin-gou]

It seems better to add a panic handler to customize the user behavior, the default behavior is to return err.

• refer to cloudwego/hertz@f687dac

[li-jin-gou]

sry, there is already a StackTraceHandler and a handler can be used to do this, but there will be break changes.😞

[ReneWerner87]

would be ok for me, but with fiber error handler we already have this possibility, because the error which is caught in the middleware is passed on to the error handler and this is responsible for the output or the conversion of the response.

https://docs.gofiber.io/guide/error-handling#custom-error-handler

[tnako]

Yeap though to add recoveryHandler(c *fiber.Ctx, p any) error but this mean everything else not needed :D

If community agree I can add logic to be the same as https://github.com/grpc-ecosystem/go-grpc-middleware/blob/main/interceptors/recovery/interceptors.go#L29 example

...
			if r := recover(); r != nil {
				err = recoverFrom(c, cfg, r)
			}
...
func recoverFrom(c *fiber.Ctx, cfg Config, p any) error {
	if cfg.EnableStackTrace {
		cfg.StackTraceHandler(c, r)
	}

	if cfg.RecoveryHandlerFunc != nil {
		return cfg.RecoveryHandlerFunc(c, p)
	}

	var err error
	var ok bool
	if err, ok = r.(error); !ok {
		// Set error that will call the global error handler
		err = fmt.Errorf("%v", r)
	}
	return err
}

[ReneWerner87]

why should this not be an option
if a hard error is thrown within and not passed on as is common best practice in golang
https://gobyexample.com/errors
https://go.dev/doc/effective_go#errors

in my opinion it is an internal server error
no 400 would fit there and i wouldn't spend a 200 either since a bug just happened
https://gobyexample.com/panic

whether the proxy server closes the connection or not
errors should hopefully only affect a few connections and those apparently have really fatal errors within them

I would also advise against the incorrect use of the stackTraceHandler, because this is designed to write away or reprocess the stack trace and should not be used incorrectly

[tnako]

Closing as custom response on panic can be done as part ErrorHandler in fiber.Config{}

[ReneWerner87]

anyway thanks that you wanted to improve something 🚀