golang / gddo

Go Doc Dot Org
https://godoc.org
BSD 3-Clause "New" or "Revised" License
1.1k stars 265 forks source link

httputil/header: ParseValueAndParams behavior for headers value containing special character #539

Open posener opened 6 years ago

posener commented 6 years ago

header.ParseValueAndParams can change it's behavior when parsing headers. It looks for the header value and stops on any special character (\t\"(),:;<=>?@[]\{}, characters that are lower than or equal to 31, and the 127th character) but then to enter the param parsing loop it looks only for ";" as the next character.

This leads to header with value containing special not parse the header correctly. (Or this is the right behavior?)

For example:

func main()  {
    h := make(http.Header)
    h.Add("key", "a;c=d")
    val, params := header.ParseValueAndParams(h, "key")
    fmt.Println(val, params)
}

Results in a map[c:d]

But

func main()  {
    h := make(http.Header)
-   h.Add("key", "a;c=d")
+   h.Add("key", "a a;c=d")
    val, params := header.ParseValueAndParams(h, "key")
    fmt.Println(val, params)
}

Results in a.

Maybe the value parsing should only stop at ";" character?

andybons commented 5 years ago

Seems OK as long at it abides by the RFC. Any thoughts, @bradfitz @dmitshur?

dmitshur commented 5 years ago

I am not familiar with the semantics of ParseValueAndParams to have a strong opinion. If it's within spec and doesn't break gddo, I don't have objections.

That said, I would like to see this package move in the direction of being made internal. It should be serving the needs of gddo only.

Packages in x/net subrepo (e.g., golang.org/x/net/http/httpguts) or elsewhere (e.g., https://godoc.org/?q=http) can serve general-purpose HTTP header parsing needs better. They would be better equipped to incorporate such changes, and more people would be able to benefit from them.