Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: add ParseCookie, ParseSetCookie #66008

Closed
rsc opened this issue Feb 28, 2024 · 12 comments
Closed

net/http: add ParseCookie, ParseSetCookie #66008

rsc opened this issue Feb 28, 2024 · 12 comments

Comments

@rsc
Copy link
Contributor

rsc commented Feb 28, 2024

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
Copy link
Contributor

vdobler commented Feb 29, 2024

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
Copy link
Contributor Author

rsc commented Mar 1, 2024

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

@rsc
Copy link
Contributor Author

rsc commented Mar 1, 2024

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
Copy link
Contributor Author

rsc commented Mar 8, 2024

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
Copy link
Contributor

neild commented Mar 9, 2024

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
Copy link
Contributor Author

rsc commented Mar 13, 2024

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
Copy link
Contributor Author

rsc commented Mar 13, 2024

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
Copy link
Contributor Author

rsc commented Mar 27, 2024

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
Copy link
Contributor Author

rsc commented Apr 4, 2024

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
Copy link
Contributor Author

rsc commented Apr 10, 2024

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)

@rsc rsc changed the title proposal: net/http: add ParseCookie, ParseSetCookie net/http: add ParseCookie, ParseSetCookie Apr 10, 2024
@rsc rsc modified the milestones: Proposal, Backlog Apr 10, 2024
@gopherbot
Copy link
Contributor

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

@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Apr 17, 2024
@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit that referenced this issue Apr 18, 2024
Updates #66008

Change-Id: Idd36a7f0b4128adfd0a3d7fe76eb6c2cea4306a3
Reviewed-on: https://go-review.googlesource.com/c/go/+/579795
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

5 participants