🐛 [Bug]: Adaptor middleware duplicates cookies

[DreamwareDevelopment]

Bug Description

I proved that the middleware is duplicating cookies in the cookie header, I have a fix that worked for me.

How to Reproduce

Steps to reproduce the behavior:

1. Use cookies (I used supertokens.Middleware as the argument to adaptor.HTTPMiddleware() which sets cookies)

Expected Behavior

Cookies should not be duplicated within the header.

Fiber Version

v2.52.4

Code Snippet (optional)

Here is the fix:

func HTTPMiddleware(mw func(http.Handler) http.Handler) fiber.Handler {
	return func(c fiber.Ctx) error {
		var next bool
		nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
			// Convert again in case request may modify by middleware
			next = true
			c.Request().Header.SetMethod(r.Method)
			c.Request().SetRequestURI(r.RequestURI)
			c.Request().SetHost(r.Host)
			c.Request().Header.SetHost(r.Host)

			for key, val := range r.Header {
				if key == "Cookie" {
					// Handle Cookie header separately to avoid duplicates
					c.Request().Header.Del(key)
					c.Request().Header.Set(key, strings.Join(val, "; "))
				} else {
					for _, v := range val {
						c.Request().Header.Set(key, v)
					}
				}
			}
			CopyContextToFiberContext(r.Context(), c.Context())
		})

		if err := HTTPHandler(mw(nextHandler))(c); err != nil {
			return err
		}

		if next {
			return c.Next()
		}
		return nil
	}
}

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.

[welcome[bot]]

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

[gaby]

@DreamwareDevelopment The middleware is not duplicating cookies, you are sending the cookie as a header.

Please provide an example of how you are using Adaptor with SuperToken.

[DreamwareDevelopment]

Cookies are a type of header...

app.Use(adaptor.HTTPMiddleware(supertokens.Middleware))

[gaby]

@DreamwareDevelopment We need a full example to be able to replicate/debug

[DreamwareDevelopment]

Sorry, I'm a solo founder and I don't have time to do a base example. But here's the final code that fixed my issue for the middleware adaptor:

func HTTPMiddleware(mw func(http.Handler) http.Handler) fiber.Handler {
	return func(c *fiber.Ctx) error {
		var next bool
		nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			next = true
			c.Request().Header.Reset()
			c.Request().Header.SetMethod(r.Method)
			c.Request().SetRequestURI(r.RequestURI)
			c.Request().SetHost(r.Host)
			c.Request().Header.SetHost(r.Host)
			// Handle Cookie header separately
			if cookies := r.Header.Get("Cookie"); cookies != "" {
				c.Request().Header.Set("Cookie", cookies)
			}

			for key, val := range r.Header {
				if key != "Cookie" {
					for _, v := range val {
						c.Request().Header.Set(key, v)
					}
				}
			}
			adaptor.CopyContextToFiberContext(r.Context(), c.Context())
		})

		if err := adaptor.HTTPHandler(mw(nextHandler))(c); err != nil {
			return err
		}

		if next {
			return c.Next()
		}
		return nil
	}
}

To note here is that I'm clearing the fiber headers before writing to them, then cookies are being set with the value in the http request header.

[haikalSusanto]

Hello, sorry to jump in, after reading the code I managed to reproduce this particular issue through unit test and some printing with fmt package.

I modify this handler and test in function Test_HTTPMiddleware to investigate what happened

original: https://github.com/gofiber/fiber/blob/e43763311d894a2de777c4d116bfd97a1bf00a17/middleware/adaptor/adaptor_test.go#L163-L181

modified:

	app.Post("/", func(c fiber.Ctx) error {
		fmt.Println("Start of request handler: ", c.GetReqHeaders())
		value := c.Context().Value(TestContextKey)
		val, ok := value.(string)
		if !ok {
			t.Error("unexpected error on type-assertion")
		}
		if value != nil {
			c.Set("context_okay", val)
		}
		value = c.Context().Value(TestContextSecondKey)
		if value != nil {
			val, ok := value.(string)
			if !ok {
				t.Error("unexpected error on type-assertion")
			}
			c.Set("context_second_okay", val)
		}

		// RETURNING CURRENT COOKIES TO RESPONSE
		var cookies []string = strings.Split(c.Get("Cookie"), "; ")
		for _, cookie := range cookies {
			c.Set("Set-Cookie", cookie)
		}

		fmt.Println("End of request handler: ", c.GetReqHeaders())
		return c.SendStatus(fiber.StatusOK)
	})

modified test

	req, err := http.NewRequestWithContext(context.Background(), fiber.MethodPost, "/", nil)
	req.Host = expectedHost
	req.AddCookie(&http.Cookie{Name: "cookieOne", Value: "valueCookieOne"})
	req.AddCookie(&http.Cookie{Name: "cookieTwo", Value: "valueCookieTwo"})
	require.NoError(t, err)

	resp, err := app.Test(req)
	require.NoError(t, err)
	require.Len(t, resp.Cookies(), 2)
	require.Equal(t, "okay", resp.Header.Get("context_okay"))
	require.Equal(t, "okay", resp.Header.Get("context_second_okay"))

the result cookie is duplicating in a weird way like this below.

 Error:          "[cookieOne=valueCookieOne cookieTwo=valueCookieTwo 0ookieOne=valueCookieOne cookieTwo=valueCookieTwo]" should have 2 item(s), but has 4

i found out that, when it comes to value with type of array

https://github.com/gofiber/fiber/blob/e43763311d894a2de777c4d116bfd97a1bf00a17/middleware/adaptor/adaptor.go#L99-L103

the setter of Header is not replacing existing Header but instead appending it. this is the existing behavior in fasthttp.

i suggest that, for type array, we need to do some checking if there is any difference first before we use the setter. if this suggestion is okay then ill help to make the solution for this, or maybe there is a better way?

peace 😁

[gaby]

@haikalSusanto Thanks, I will investigate today.

[haikalSusanto]

Any update on this?

[gaby]

@haikalSusanto If you can submit a fix for this with unit-tests that would be great. Thanks!

[sigmundxia]

If my check was correct, the problem might be caused by upstream fasthttp, I've filed an issue there :)

[sigmundxia]

This issue actually deals with two problems: duplication of cookies, and the strange 0ookieOne that appeared in @haikalSusanto's test.

The latter problem was a bug in fasthttp that has been fixed by this pull request and merged into version v1.56.0, so the issue should be resolved 😊

Regarding the former problem, after the discussion of this pull request, header.Set keeping the original features (not deleting the original cookies) was considered a better solution. Therefore, I submitted a pull request to try to fix this issue in gofiber and add the corresponding tests, hopefully this will be useful in solving this problem 😊