fiber icon indicating copy to clipboard operation
fiber copied to clipboard

🐛 [Bug]: route (and Route()) property from fiber context returning nil when called after body limit error

Open gabrielmarinhoindico opened this issue 1 year ago • 6 comments

Bug Description

In my application, I create an instance of a fiber server and fill it with some configurations. After init it I pass the routes and middlewares created. I have a function for the Error Handler and with any other error when I use context.AllParams() inside it I get the proper return but with this specific error (body limit) the context.route (private field used internally by AllParams) or the context.Route() doesn't return the values, the first one returns nil and gets a panic when called dot params inside the AllParams method. The second one returns a struct with empty values. All the other methods are getting the proper return (ctx.Path, ctx.Queries, etc).

Captura de tela de 2024-02-05 10-00-31

image

I guess that the problem is the body limit error is thrown before getting to the routes that fill that variable internally.

How to Reproduce

Steps to reproduce the behavior:

  1. Create a struct to the application:
type Application struct {
	Server *fiber.App
}

var Data *Application

  1. Create a fiber server with the following config:
	appinstance.Data = &appinstance.Application{
		Server: fiber.New(fiber.Config{
			ServerHeader: "Some Header name Here",
			ErrorHandler: customErrorHandler,
			Prefork:      false,
		}),
	}

function to errorHandler:

func customErrorHandler(ctx *fiber.Ctx, err error) error {
   
     allParams := ctx.AllParams()
     // panic will happen here getting the params because the internal property route is nil


	logging.Log(
		&entity.LogDetails{
			Message:  message,
			Method:   ctx.Method(),
			Reason:   err.Error(),
			RemoteIP: ctx.IP(),
			Request: map[string]interface{}{
				"body":       string(ctx.BodyRaw()),
				"query":      ctx.Queries(),
				"url_params": ,
			},
			StatusCode: code,
			URLpath:    ctx.Path(),
		},
		"critical",
		nil,
	)

	return nil
}

  1. create a patch route that receives any path param;
  2. Listen the server and run it;
  3. make the request sending a file that it's higher than the fiber body limit (the default is 4MB +/- (4 * 1024 * 1024)

Expected Behavior

I expect that the AllParams return the proper value, or return an empty struct at least, because the AllParams don't expect to return an error, but if it can happen would be nice to return it in a v3 of fiber maybe?

Fiber Version

v2.49.2

Code Snippet (optional)

No response

Checklist:

  • [X] I agree to follow Fiber's Code of Conduct.
  • [X] I have checked for existing issues that describe my problem prior to opening this one.
  • [X] I understand that improperly formatted bug reports may be closed without explanation.

gabrielmarinhoindico avatar Feb 05 '24 13:02 gabrielmarinhoindico

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

welcome[bot] avatar Feb 05 '24 13:02 welcome[bot]

interesting i guess because body limit is an error which is handled on the fasthttp level and the parameters and routing comes after that

can you share the panic error

in this case, you probably have to check your own error handler for this error first and handle it without any further information

ReneWerner87 avatar Feb 05 '24 14:02 ReneWerner87

simliar to our check https://github.com/gofiber/fiber/blob/9b0a99ba27fcae1b841ecd28139785b6e230c59b/app.go#L1094-L1095

ReneWerner87 avatar Feb 05 '24 14:02 ReneWerner87

interesting i guess because body limit is an error which is handled on the fasthttp level and the parameters and routing comes after that

can you share the panic error

in this case, you probably have to check your own error handler for this error first and handle it without any further information

If the allParams expect always have a value in route property instead of a nil value, this doesn't mean that the function maybe need a change to check if the route is nil or not? maybe in a v3 of fiber?

The panic:


"runtime error: invalid memory address or nil pointer dereference"
Stack:
	 4  0x0000000000af3319 in github.com/gofiber/fiber/v2.(*Ctx).AllParams
	     at /home/gabrielclaudinomarinho/go/pkg/mod/github.com/gofiber/fiber/[email protected]/ctx.go:1017
	 5  0x0000000001426625 in api.example/pkg/app.customErrorHandler
	     at /home/gabrielclaudinomarinho/projects/api.example/pkg/app/app.go:89
	 6  0x0000000000aea62d in github.com/gofiber/fiber/v2.(*App).ErrorHandler
	     at /home/gabrielclaudinomarinho/go/pkg/mod/github.com/gofiber/fiber/[email protected]/app.go:1061
	 7  0x0000000000aeab71 in github.com/gofiber/fiber/v2.(*App).serverErrorHandler
	     at /home/gabrielclaudinomarinho/go/pkg/mod/github.com/gofiber/fiber/[email protected]/app.go:1093
	 8  0x0000000000b1b7a5 in github.com/gofiber/fiber/v2.(*App).serverErrorHandler-fm
	     at <autogenerated>:1
	 9  0x0000000000a50f87 in github.com/valyala/fasthttp.(*Server).writeErrorResponse
	     at /home/gabrielclaudinomarinho/go/pkg/mod/github.com/valyala/[email protected]/server.go:2850
	10  0x0000000000a4d5c5 in github.com/valyala/fasthttp.(*Server).serveConn
	     at /home/gabrielclaudinomarinho/go/pkg/mod/github.com/valyala/[email protected]/server.go:2287
	11  0x0000000000a63c05 in github.com/valyala/fasthttp.(*Server).serveConn-fm
	     at <autogenerated>:1
	12  0x0000000000a5f077 in github.com/valyala/fasthttp.(*workerPool).workerFunc
	     at /home/gabrielclaudinomarinho/go/pkg/mod/github.com/valyala/[email protected]/workerpool.go:224
	13  0x0000000000a5eda5 in github.com/valyala/fasthttp.(*workerPool).getCh.func1
	     at /home/gabrielclaudinomarinho/go/pkg/mod/github.com/valyala/[email protected]/workerpool.go:196

gabrielmarinhoindico avatar Feb 05 '24 14:02 gabrielmarinhoindico

ok, we will consider it for v3 and handle it better

ReneWerner87 avatar Feb 05 '24 14:02 ReneWerner87

simliar to our check

https://github.com/gofiber/fiber/blob/9b0a99ba27fcae1b841ecd28139785b6e230c59b/app.go#L1094-L1095

One thing that i noticed is that Params from ctx.Route().Params return an empty slice when this case happens, so to solve this internally before a possible update from fiber is to create a helper function, something like this:

func AllParams(c *fiber.Ctx) map[string]string {
	route := c.Route()

	if len(route.Params) == 0 {
		return nil
	}

	return c.AllParams()
}

Maybe help someone with the same problem.

gabrielmarinhoindico avatar Feb 05 '24 14:02 gabrielmarinhoindico