🐛 [Bug]: Ctx.GetReqHeaders doesn't respect multiple fields with same name

[makkes]

Bug Description

When calling Ctx.GetReqHeaders on a request with this header:

"Accept": "text/plain",
"Accept": "application/json",
[...]

Then the resulting map contains only the last field value.

See the relevant RFC section.

How to Reproduce

Steps to reproduce the behavior:

1. Run the code snippet from below
2. Run curl localhost:3000 -H 'Accept: text/plain' -H 'Accept: application/json'
3. Check the logs of the app

Expected Behavior

The logs should show the value of both Accept fields:

map[Accept:application/json,text/plain Host:localhost:3000 User-Agent:curl/8.2.1]

Fiber Version

v2.48.0

Code Snippet (optional)

package main

import (
	"fmt"
	"log"

	"github.com/gofiber/fiber/v2"
)

func main() {
	app := fiber.New()

	app.Get("", func(c *fiber.Ctx) error {
		fmt.Println(c.GetReqHeaders())
		return nil
	})

	log.Fatal(app.Listen(":3000"))
}

Checklist:

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

[welcome]

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

[efectn]

I think we need to change signature to fix the bug https://github.com/gofiber/fiber/blob/master/ctx.go#L654

Also getrespheaders needs to be fixed, too.

[makkes]

I don't think the signature needs to be changed. All the function would need to do is to join all the values using , according to the RFC I linked above:

When a field name is repeated within a section, its combined field value consists of the list of corresponding field line values within that section, concatenated in order, with each field line value separated by a comma.

So a header of

Accept: text/plain
Accept: application/json

would result in a map with key Accept and value text/plain, application/json.

[efectn]

I don't think the signature needs to be changed. All the function would need to do is to join all the values using , according to the RFC I linked above:

When a field name is repeated within a section, its combined field value consists of the list of corresponding field line values within that section, concatenated in order, with each field line value separated by a comma.

So a header of

Accept: text/plain
Accept: application/json

would result in a map with key Accept and value text/plain, application/json.

Yes it's also an option but making them slice seems more logical to me. Joining headers might be bad idea for several cases. i.e when i have a comma in header value, it will be difficult to parse it

[makkes]

Yes it's also an option but making them slice seems more logical to me. Joining headers might be bad idea for several cases. i.e when i have a comma in header value, it will be difficult to parse it

RFC 9110 is pretty specific about this in demanding that field values that can contain commas need to be quoted. But I agree it can get complicated quickly depending on the individual header field. Go also uses a map[string][]string so it might be the most reasonable way to follow that example.

[efectn]

Yes it's also an option but making them slice seems more logical to me. Joining headers might be bad idea for several cases. i.e when i have a comma in header value, it will be difficult to parse it

RFC 9110 is pretty specific about this in demanding that field values that can contain commas need to be quoted. But I agree it can get complicated quickly depending on the individual header field. Go also uses a map[string][]string so it might be the most reasonable way to follow that example.

What do you think @gaby @ReneWerner87

[ReneWerner87]

the objection is correct, just don't know if we should change the signature

That would be another breaking change
the question is how best the usage is for the comsumer in such a case

breaking change would still work if necessary, but then I want to be sure that we don't make the whole thing complicated, because currently it is a simple function for simple access
but this access becomes more complicated for all other values which were simple before
although the case that it is a list probably only occurs in 0-3% of the cases

i.e. we have to be clear whether we want to adjust all other simple cases with the signature change and thus adjust the ease of access based on these 3% cases and thus cause a breaking change or whether the simple solution with the comma separated values would also work

currently not sure, the signature change seems to be more correct, but also makes it more complicated

[ReneWerner87]

@gaby @efectn let's talk together about the possible solution

[ReneWerner87]

@gaby @efectn let's talk together about the possible solution

We have made an internal vote in the maintainer cycle and decided for the variant with the string list, so the signature change

So we accept this change as pr

[trco]

Should the initial bug be fixed with this? When I'm making request with "Content-Type: test1" and "Content-Type: test2" headers I'm getting only "Content-Type": ["test2"] in the map returned by GetReqHeaders method. I was expecting "Content-Type": ["test1", "test2"].

I'm using latest version 2.51.0.

[ReneWerner87]

is it because of this header, is this a valid example?
does it make sense to assign several values to the content type in the request?
with all other headers it works by the way

[trco]

Wow. My bad. While testing I simply duplicated the first header and I didn't pay attention to the context of the header itself. Thanks for your response.