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

x/net/http/httpguts: add ParseCookie and ParseSetCookie #25194

Closed
posener opened this issue May 1, 2018 · 26 comments
Closed

x/net/http/httpguts: add ParseCookie and ParseSetCookie #25194

posener opened this issue May 1, 2018 · 26 comments
Labels
Milestone

Comments

@posener
Copy link

posener commented May 1, 2018

What version of Go are you using (go version)?

go version go1.10 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

I wanted to extract cookies struct from a cookie header string.
I had to type this piece of code:

req := http.Request{Header: http.Header{"Cookie": []string{raw}}}
cookies := req.Cookies()

It felt kind of workaround.

What did you expect to see?

Have the appropriate method in the net/http package that is public and parses cookie headers to a []*Cookie struct.
For example:

func ParseCookies(raw string) []*Cookie {
    ...
}

What did you see instead?

Nothing.

@gopherbot gopherbot added this to the Proposal milestone May 1, 2018
@odeke-em
Copy link
Member

odeke-em commented May 2, 2018

Thank you for the proposal @posener!

How about a package in say golang.org/x/net/cookie for handling cookies? In there we can bundle functionality even more functionality like #17587 which requests for a way to access all the cookies in a CookieJar

/cc @dsnet @vdobler @bradfitz @mdlayher

@meirf
Copy link
Contributor

meirf commented May 2, 2018

There is https://golang.org/pkg/net/http/cookiejar, but I don't think cookie parsing should be put there since that package is specific to the jar. If there was a net/http/cookie or x/net/cookie, that would be a better place.

But note that there is a precedent for including ParseCookies in net/http. Namely, ParseHTTPVersion is a similar situation. The value that ParseHTTPVersion provides could just as well be computed from Response.ProtoMajor and Response.ProtoMinor. Similarly, the value that ParseCookies provides could just as well be computed from Request.Cookies. They are both utility methods which could also be handled by their natural parent objects and probably provide a bunch of value when not in the context of executing/serving http.

@vdobler
Copy link
Contributor

vdobler commented May 2, 2018

@posener I understand that it would be convenient to have this as a function available.

But I doubt that this function is used that often that it is worth an API change (which
basically can never be undone). Serialized cookie headers (your raw) are common
only as part of a request header and Request.Cookies() does exactly what you need.
I cannot imagine a common, requiring situation where I would have a raw cookie
header which is not already part of a http.Header or even a http.Request (except
during testing).

So I doubt that this is a common problem, especially as it has a very simple and
efficient two line solution.

@meirf Your precedent isn't one. ParseHTTPVersion cannot be computed from
ProtoMajor/Minor: It parses the version. And this function was exported before
Go 1.0 and the compatibility promise.

@odeke-em A new package like golang.org/x/net/cookie is a tempting idea.
But while these golang.org/x repose are formally not covered by the compatibility
guarantee users still do expect a A) well thought of API which is B) stable and
free from braking changes.
Both -- the ParseCookie function proposed here as well as a more open cookiejar
discussed in #17587 -- can (and at least the cookiejar does) live in external packages.

So I would reject this proposal.

@rsc
Copy link
Contributor

rsc commented May 7, 2018

It does seem awkward that we have this functionality in the standard library already, it's useful, and it's hidden from users except via an odd use of Request. Trying to make it more available seems OK in the abstract.

Looking at the net/http source it's a little awkward that Cookie: and Set-Cookie: headers need to be parsed differently, so "ParseCookies" is not completely well-specified. Set-Cookie has extra attributes on the line and accepts double-quoted values, where Cookie does not, apparently.

Probably ParseCookies should be able to return an error as well?

Which should ParseCookie do? Does someone want to propose a specific API?

@posener
Copy link
Author

posener commented May 8, 2018

I can also refer the httputil/header package in the gddo repository. It contains some header pasing utils, among them ParseValueAndParams.
I may also refer this issue with that function.

@posener
Copy link
Author

posener commented May 11, 2018

I took my time to look into the specification.
This is what I found.

 set-cookie-header = "Set-Cookie:" SP set-cookie-string
 set-cookie-string = cookie-pair *( ";" SP cookie-av )
 cookie-pair       = cookie-name "=" cookie-value
 cookie-name       = token
 cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
 cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash
 token             = <token, defined in [RFC2616], Section 2.2>

 cookie-av         = expires-av / max-age-av / domain-av /
                     path-av / secure-av / httponly-av /
                     extension-av
... (more options) ...

The Set-Cookie is sent from the server to the client. It contains information only about one cookie. If a server want to sets some cookies, it adds more Set-Cookie headers, one header for each cookie.
Each header contains the cookie name and value, and metadata, separated by semicolon and space.

For example: name=value; Path=/; HttpOnly; Secure; Domain=example.org

The value defined as "cookie-octet" double quoted "cookie-octet"

   cookie-header = "Cookie:" OWS cookie-string OWS
   cookie-string = cookie-pair *( ";" SP cookie-pair )

The Cookie header sent from the client to the server contains all the cookies in one header - serialized, and without the metadata information (expiry, path/ domain and so.)
They are separated by the same semicolon and space, and they have the same definition of cookie-pair as in the Set-Cookie header.

For example: name1=value; name2="value2"

Conclusions and Thoughts

So If there were functions that parse cookies values, there should be two of them:

// ParseSetCookie parses Set-Cookie header value and return a cookie
// It returns error on syntax error.
func ParseSetCookie(cookie string) (*Cookie, error) {}

// ParseCookie parses a Cookie header value and returns all the cookies
// which were set in it. Since the same cookie name can appear multiple times
// the returned Values can contain more than one value for a given key.
func ParseCookie(cookies string) (Values, error) {}

General comments:

  • Many more header types, each has it's own rules, but there might be a room also for a general header parsing function according to this RFC section
  • One one hand - the suggestion above by @odeke-em to create a specific, external package (maybe httpheader might be the right approach.
  • On the other hand, it seems like the standard library needs the package functionality to parse cookies - so it should not be an external one.

@vdobler
Copy link
Contributor

vdobler commented May 14, 2018

Three remarks:

ParseCookie must return []*Cookie: A Cookie header contains key=value pairs,
not only values. (There might be different keys).

Documentation should note which RFC governs parsing and which exceptions
are handled in which way. Which leads to my main point:

"[...] it seems like the standard library needs the package functionality to
parse cookies". The stdlib already does! One problem with cookies is
that it is a bit of a moving target with a lot of garbage being sent around.
Now net/http transparently handles RFC 6265 conforming cookies
and we always told people who needed to deal with non-RFC-6265-compliant
cookies to deal with such malformed cookies by themself by parsing whatever
Header["{Set}Cookie"] they have to deal with.
With dedicated and exported functions to manually parse these two
HTTP header values people might expect them to handle their 12kbyte long
unquoted UTF-8 cookie values including spaces because their
Firefox-Ruby-combo for their intranet uses them without problems.

I'm still not convinced there is a need for these functions: If the cookies
are RFC 6265 compliant than net/http does all the heavy lifting and you
do not need to manually parse {Set}Cookie headers. If you need to parse
them manually then typically because they are formally malformed headers
in which case Parse{Set}Cookie would be helpful only by providing
dedicated error handling.
Parsing RFC 6265 cookie with net/http is a minor inconvenience only.
If the proposed new API is about exposing the errors encountered by
parsing malformed {Set}Cookie headers then I do see the need, but not
for the parsing alone.

@rsc
Copy link
Contributor

rsc commented May 21, 2018

Thanks @vdobler for pointing out that the use for these functions is unclear. Most of the time you have an http.Request and have good getter/setter functions. @posener, what is the context in which these extra functions would be needed? @vdobler suggests maybe for getting at the error to find out why a particular Cookie line is malformed, but that's pretty special-case for new API.

@posener
Copy link
Author

posener commented May 22, 2018

The use case is parsing a authentication cookie that I get as string value from a rest framework (go-swagger). From the birds eye it might look like a very specific case.
However, I did meet the need to parse HTTP headers in gddo, go-server-timing (that uses gddo). Cookie parsing is a special case I guess.

@bradfitz
Copy link
Contributor

I'd be fine adding new API to https://godoc.org/golang.org/x/net/http/httpguts which is specifically a catch-all bin of misc & shared HTTP stuff.

@posener
Copy link
Author

posener commented May 29, 2018

@bradfitz , adding it in httpguts will cause a duplication of the implementation of parsing cookies, right? Once in http and once in httpguts.

@bradfitz
Copy link
Contributor

@posener, not if you make net/http use httpguts's implementation. net/http already depends on guts.

@rsc
Copy link
Contributor

rsc commented Jun 4, 2018

It sounds like this proposal has morphed into adding these to httpguts:

// ParseSetCookie parses Set-Cookie header value and return a cookie
// It returns error on syntax error.
func ParseSetCookie(cookie string) (*Cookie, error)

// ParseCookie parses a Cookie header value and returns all the cookies
// which were set in it. Since the same cookie name can appear multiple times
// the returned Values can contain more than one value for a given key.
func ParseCookie(cookies string) ([]*Cookie, error)

That's OK with proposal-review, including @bradfitz.

@rsc rsc closed this as completed Jun 4, 2018
@rsc rsc changed the title proposal: net/http: Add ParseCookie function proposal: x/net/http/httpguts: add ParseCookie and ParseSetCookie Jun 4, 2018
@rsc rsc changed the title proposal: x/net/http/httpguts: add ParseCookie and ParseSetCookie x/net/http/httpguts: add ParseCookie and ParseSetCookie Jun 4, 2018
@rsc rsc modified the milestones: Proposal, Unreleased Jun 4, 2018
@ianlancetaylor ianlancetaylor reopened this Jun 4, 2018
@smasher164
Copy link
Member

smasher164 commented Jul 5, 2018

I may be missing something here, but if net/http depends on httpguts and ParseSetCookie returns an *http.Cookie, wouldn't there be an import cycle?

Edit: We could move the cookie implementation to httpguts, and create an alias declaration for the cookie type in net/http. I'm not sure if this would violate the backwards compatibility promise.

type Cookie = httpguts.Cookie

@tv42
Copy link

tv42 commented Aug 14, 2018

type Cookie = httpguts.Cookie

That would make godoc unhelpful.

@smasher164
Copy link
Member

What if we bundle x/net/http/httpguts into net/http like we do for x/net/http2? That way, the implementation is maintained in a separate library, but the API is exposed through the standard library.

@bradfitz
Copy link
Contributor

I suppose that'd work, but it'd be a little unfortunate to have more copies of it in binaries. Because http2 depends on httpguts already. I actually don't think x/tools/cmd/bundle would work as-is with multiple levels of bundling. It'd need work.

And moving the definition of Cookie to httpguts is also pretty gross.

We could almost have an exact copy of the Cookie struct type in the httpguts package (and have users convert between the differently named identical structs), except in Go 1.11 we added the SameSite field with type http.SameSite, so there's another cycle.

Perhaps the net/http Cookie type could move to a child package and use type aliases (and we could fix godoc to conditionally promote the docs or something). But then this is all sounding complicated.

Perhaps bundling is the best option. Somebody could try it.

@jrozner
Copy link

jrozner commented Apr 14, 2020

Has there been any movement on this?

@smasher164
Copy link
Member

smasher164 commented May 6, 2020

It seems that the only viable option is to do bundling. The functions above should be implemented in httpguts as specified. x/tools/cmd/bundle would be updated to handle a package that is bundled multiple times. net/http internally would then use the bundled implementation of httpguts_ParseCookie and httpguts_ParseSetCookie.

The only other option I see would be to linkname in the functions from httpguts, but that would require net/http to import unsafe, which is not going to happen.

Given the constraints this imposes on implementation, perhaps this proposal should be reevaluated to be implemented in the standard library?

@codenoid
Copy link

how it's going?

It's nice to have built-in functionality to parse Cookie & Set-Cookie header content

@smasher164
Copy link
Member

Given the constraints this imposes on implementation, perhaps this proposal should be reevaluated to be implemented in the standard library?

/cc @golang/proposal-review

@gopherbot
Copy link

Change https://go.dev/cl/566795 mentions this issue: http/httpguts: implement ParseCookie and ParseSetCookie

@odeke-em
Copy link
Member

I've mailed out https://go-review.googlesource.com/c/net/+/566795 please take a look.

@neild
Copy link
Contributor

neild commented Feb 26, 2024

Thanks for https://go.dev/cl/566795.

I really dislike the duplication of the Cookie type between httpguts and net/http.

CL 566795 only duplicates the bare minimum required to implement ParseCookie and ParseSetCookie, which means httpguts.Cookie doesn't have any of the usual Cookie methods, notably String and Valid. Any user is probably going to want to immediately type convert the httpguts.Cookie into an http.Cookie, which is gross.

I know this is an accepted proposal, but I don't want to have to support this. If we want to add ParseCookie and ParseSetCookie, we should do it in the standard library.

@rsc
Copy link
Contributor

rsc commented Feb 28, 2024

Filed #66008 for net/http change. Let's close this as a duplicate of that.

@rsc rsc closed this as completed Feb 28, 2024
@rsc
Copy link
Contributor

rsc commented Mar 8, 2024

I've unaccepted this proposal in favor of #66008. The meaning of cookie attributes seems to have changed since 2018 too, so good not to do this API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests