net/http: add ParseCookie, ParseSetCookie

[rsc]

Following up on #25194, which was approved in 2018 but only recently implemented, it looks like it makes more sense to add ParseCookie and ParseSetCookie directly to net/http, to avoid significant duplication (specifically of the Cookie type and the parsers) between httpguts and http.

[vdobler]

There is a longstanding issue #46443 (net/http: implementation of cookies does not conform to RFC 6265 for double-quoted values) on what exactly is the "value" of a cookie sent like name="foo". In short it seems as if this changed over the last 20 years from "the value is foo" to "the value is "foo" (i.e. the double quotes are part of the value).

net/http always stripped the double quotes from the value, so name="foo" and name=foo in the cookie header both get parsed into a net/http.Cookie with Value == "foo". Issue #46443 is about on how to distinguish name="foo" and name=foo and how to roundtrip a cookie received as name="foo" via net/http/cookiejar.

By adding ParseCookie (and probably ParseSetCookie) using just the current implementation we would fix that behaviour even more. Maybe this proposal should address on how to distinguish name="foo" and name=foo ?

It seems plausible to me that ParseCookie and ParseSetCookie might be useful to "manually" distinguish name="foo" and name=foo in the few cases where the double quotes matter and we do not want to change inner working of cookie parsing.

That said I still think adding a new boolean field net/http.Cookie.DoubleQuoted might solve #46443 and might be worth adding to this proposal.

[rsc]

@vdobler, nice to hear from you again. I've converted #46443 into a proposal.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

It's good we didn't actually do #25194, since that API looks wrong anyway. What is the API we want here? I assume we want net/http to be able to use these routines itself, or some common implementation at least. The implementations in net/http are:

// readCookies parses all "Cookie" values from the header h and
// returns the successfully parsed Cookies.
//
// if filter isn't empty, only cookies of that name are returned.
func readCookies(h Header, filter string) []*Cookie

// readSetCookies parses all "Set-Cookie" values from
// the header h and returns the successfully parsed Cookies.
func readSetCookies(h Header) []*Cookie

If we provided

func ParseCookie(line string) ([]*Cookie, error)
func ParseSetCookie(line string) (*Cookie, error)

I see at least the following implications:

1. readCookies would call ParseCookie in a loop, which would create small slices that would be thrown away immediately, appended to the big slice readCookies is accumulating. A bit of garbage. We could make ParseCookies be AppendCookies but that seems annoying. Perhaps internally it could be.
2. readCookies with a filter would have to duplicate a tiny bit of parsing, just a textproto.TrimSpace to look for 'filter=' at the start of the line. Or perhaps the internal function could accept a filter and error out early. (Edit: Since there can be multiple cookies per line, only the second approach is correct.)
3. Both readCookies and readSetCookies would discard the error results. Do other callers need them? The internal function could take a bool that says whether to bother allocating an error.

We can work around all of these, as noted, but it does mean that you can't use ParseCookie/ParseSetCookie to get all the speed that net/http does. Is that fine? Probably it is?

[neild]

Should ParseCookie and ParseSetCookie return an iter.Seq[*Cookie]? (Is there a way for them to return an error if they do? iter.Seq2[*Cookie, error]?)

Without iterators, if we want something that net/http can use itself then perhaps:

// ParseCookiesNamed is the existing readCookies.
// It parses all cookies when name is "".
ParseCookiesNamed(lines []string, name string) ([]*Cookie, error)

// ParseSetCookiesNamed is the existing readSetCookies, with added filter support.
// It parses all cookies when name is "".
ParseSetCookiesNamed(lines []string, name string) ([]*Cookie, error)

[rsc]

Iterators seems like a little bit of overkill here. For name filtering, internally net/http can get at anything. I'm not sure we need to expose the name filtering for user code.

[rsc]

Are there real-world use cases that are not well enough handled by:

func ParseCookie(line string) ([]*Cookie, error)
func ParseSetCookie(line string) (*Cookie, error)

?

[rsc]

Have all remaining concerns about this proposal been addressed?

The proposal is to add

func ParseCookie(line string) ([]*Cookie, error)
func ParseSetCookie(line string) (*Cookie, error)

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add

func ParseCookie(line string) ([]*Cookie, error)
func ParseSetCookie(line string) (*Cookie, error)

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add

func ParseCookie(line string) ([]*Cookie, error)
func ParseSetCookie(line string) (*Cookie, error)

[gopherbot]

Change https://go.dev/cl/578275 mentions this issue: net/http: add ParseCookie, ParseSetCookie

[gopherbot]

Change https://go.dev/cl/579795 mentions this issue: net/http: add case for TestParseSetCookie