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

Open
posener opened this Issue May 1, 2018 · 17 comments

Comments

Projects
None yet
10 participants
@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

@gopherbot gopherbot added the Proposal label May 1, 2018

@odeke-em

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

bradfitz commented May 22, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

bradfitz commented May 29, 2018

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

@rsc

This comment has been minimized.

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 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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

tv42 commented Aug 14, 2018

type Cookie = httpguts.Cookie

That would make godoc unhelpful.

@smasher164

This comment has been minimized.

Copy link
Contributor

smasher164 commented Oct 16, 2018

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

This comment has been minimized.

Copy link
Member

bradfitz commented Oct 16, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.