net/http: add Cookie.Valid method

[nulltrope]

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

$ go version
go version go1.16 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GOARCH="amd64"
GOOS="darwin"

What did you do?

I am attempting to construct an http.Cookie that will be set on an http.Server's response

http.HandleFunc("/foo", func(w http.ResponseWriter, r *http.Request) {
	cookie := &http.Cookie{
		Name:   "my-cookie",
		Value:  "bar",
		Domain: "example.com:8080",
	}
	http.SetCookie(w, cookie)
	fmt.Fprintf(w, "cookie: %s", cookie.String())
})

What did you expect to see?

Being that the http.SetCookie (or more specifically, the cookie.String method) function validates various fields of the cookie struct and returns no error, I would expect there to be a way to validate the cookie myself before attempting to set it.

Unfortunately, these validator functions are all unexported and the only way I can see to achieve this today would be to re-write the validation logic in my own code, which comes with its own set of concerns.

I can see why generally there isn't always a need to validate a cookie yourself before writing, but when dealing with cookies containing lots of dynamic attribute data (e.g. an app implementing oauth/oidc flows) it becomes paramount that the cookies being returned are written as expected.

What did you see instead?

The http.SetCookie function will log to stderr (or not, depending on your logging setup) and silently discard the portion of the cookie that failed validation from the final header.

For myself, this caused the Domain attribute to be dropped from the final cookie in the response. That in turn caused the cookie to not be sent in subsequent requests to a subdomain on the same host, which resulted in an error.

Proposal

Cookies in net/http have a concept of validity, but what constitutes a "Go valid cookie" is deliberately left undocumented.

Additionally, it is quite easy to construct an "invalid" cookie (by Go standards) and have it go unnoticed, due to the (*http.Cookie).String() method silently discarding any invalid fields.

I propose adding a new utility function that allows one to programmatically check a cookie's validity under the same rules used by net/http.

This can be either a validity check method, a serialization method, or both:

// Valid reports whether the cookie is valid.
func (c *Cookie) Valid() error

// Marshal returns the serialization of the cookie for use in a Cookie
// header (if only Name and Value are set) or a Set-Cookie response
// header (if other fields are set).
// If c is nil, the empty string is returned.
// If the cookie is invalid, an error is retuned.
func (c *Cookie) Marshal() (string, error)

Use Cases

1. When a cookie is populated with dynamic data from an http request or some other external source. Rather than having the (*http.Cookie).String() method just log to stderr and discard fields (which may be entirely swallowed depending on your logging setup) you'd be able check cookie validity beforehand, and handle invalid cookies gracefully by logging your own structured error, incrementing a failure metric, returning a different http response, etc.

http.HandleFunc("/foo", func(w http.ResponseWriter, r *http.Request) {
	cookie := &http.Cookie{
		Name:   "my-cookie",
		Value:  "bar",
		Domain: r.Host,
	}
  
  err := cookie.Valid()
  if err != nil {
    log.Printf("invalid cookie: %v", err)
    http.Error(w, http.StatusInternalServerError, 500)
    return
  }
  
	http.SetCookie(w, cookie)
	fmt.Fprintf(w, "cookie: %s", cookie.String())
})

2. When using statically defined cookie data, a validation check can still be valuable to both assert that the value as you define it is correct, and that the meaning of a "go valid cookie" has not changed.

[mknyszek]

CC @neild

[neild]

This seems reasonable, especially since (http.Cookie).String logs to stderr(!) for some (but not all!) invalid cookies. Ideally, String would return "" for cookies with an invalid domain, but that's probably difficult to change at this time.

Perhaps something like:

// IsValid reports whether c.Name and c.Domain are valid.
func (c *Cookie) IsValid() bool

[gazerro]

See related comment #37055 (comment).

[nulltrope]

This seems reasonable, especially since (http.Cookie).String logs to stderr(!) for some (but not all!) invalid cookies. Ideally, String would return "" for cookies with an invalid domain, but that's probably difficult to change at this time.

Perhaps something like:

// IsValid reports whether c.Name and c.Domain are valid.
func (c *Cookie) IsValid() bool

Yeah I can't say i'm a fan of the current behavior of (http.Cookie).String but changing it at this point is probably out of the question. Adding a new function like (http.Cookie).IsValid seems the best course of action.

I know we're in a development freeze right now but if you think this is a worthwhile change I can draft up a PR to be considered for the next release cycle.

See related comment #37055 (comment).

Thanks for the context! I don't expect the change i'm proposing to expose what exact cookie values are valid/invalid. This would only be providing a method to tell whether or not the cookie as a whole is "valid" by Go standards, the specifics of which may or may not change in the future (as @vdobler pointed out).

If anything, I think having an (http.Cookie).IsValid method would make these kinds of cookie validation changes safer, or at least less surprising to users.

[gazerro]

What is the exact meaning of "valid" for a cookie? Is it valid if it is not discarded by the SetCookie method? Or if all the fields of the cookie appear in the Set-Cookie header without changes? Or what else.

[nulltrope]

My thought was to consider a cookie "valid" if no values are discarded by the (http.Cookie).String method.

More specifically, (http.Cookie).IsValid would return true so long as isCookieNameValid, validCookieDomain, and validCookieExpires return true.

Edit: Clarifying as there was some confusion here: The above is just an example and additional validation cases may be considered. As for what constitutes a "valid" cookie, I think the definition "a cookie is valid if no values are discarded by the (http.Cookie).String method." sufficiently describes what the method will do without giving away too many implementation details.

Any additional formatting done to the values like adding quotes etc. shouldn't make a cookie invalid, as the resulting cookie header has all the same data. On the other hand, I would consider a cookie with a malformed domain or expiration invalid, because after calling SetCookie the final cookie sent in the response would be missing those field(s), and will interpreted differently by browsers vs. if they were included.

[gazerro]

Could cookies with a value or path that is changed or completely discarded by the String method be valid? With the previous definition they can be valid. For example this cookie would be valid

&http.Cookie{Name: "c", Domain: "domain.com", Expires: time.Now(), Value: ";"}

but the value would be discarded by the String method and an error would be logged

2009/11/10 23:00:00 net/http: invalid byte ';' in Cookie.Value; dropping invalid bytes
c=; Domain=domain.com; Expires=Tue, 10 Nov 2009 23:00:00 GMT

https://play.golang.org/p/Lm-6VjMFPV7

I'm not saying the above definition is wrong, but an IsValid function could create expectations that may not be met.

[neild]

Under the definition "a cookie is valid if no values are discarded by (http.Cookie).String", then a cookie with a Value of ";" would clearly be invalid.

[nulltrope]

Yes if the value is discarded and an error logged, I would consider that cookie invalid

[gazerro]

You are right, I had considered this as a definition

More specifically, (http.Cookie).IsValid would return true so long as isCookieNameValid, validCookieDomain, and validCookieExpires return true.

[nulltrope]

Yeah sorry, let me edit that comment to be more clear. I think a simple definition of "a cookie is valid if no values are discarded by (http.Cookie).String" sufficiently describes what we're trying to achieve here.

[gazerro]

As an alternative to the IsValid method, I propose to add a Serialize method

// Serialize works like String, but returns an empty string and a not nil
// error instead of discarding some fields and logging the error as String
// does.
func (c *Cookie) Serialize() (string, error)

It can be used like this

v, err := cookie.Serialize()
if err != nil {
   // do somenthing with the error
}
w.Header().Add("Set-Cookie", v)

[vdobler]

I would expect there to be a way to validate the cookie myself before attempting to set it.

I am not sure I understand the reason for this, especially if you control the parameters of the cookie yourself: Just do not set invalid names or values.

Being able to validate something is important in all the cases you take data you do not control and have to process it. But there is no real need to validate data you generate. Cookies you receive (at least via the builtin cookie stuff from net/http) are valid and can be sent in Cookie and Set-Cookie headers.

The rules for proper cookie names and values are pretty trivial and well understood and documented in RFC6265. Using something else is a simple programming (or design) error and not some actionable code path.

How would somebody use a Cookie.IsValid method?

cookie := &http.Cookie{
	Name:   "my-cookie",
	Value:  "bar",
	Domain: "example.com:8080",
}
if !cookie.IsValid() {
        // What action could you take here except?
        panic("cookie not valid")
        log.Println("cookie not valid")
        cookie = &http.Cookie{Name:"N", "Value": "V"} // use safe, working cookie?
}

In my opinion there is no sensible way to react in code to a cookie whose IsValid method returns false. So there is no need for a IsValid method.

Pointers can be nil and you can check if they are "valid" by a simple p != nil. This is reasonable because using a nil pointer has a well-defined meaning and is useful (zero value of pointers, indicating absence of data, all that). A "invalid" net/http.Cookie has neither a defined meaning nor is it useful, it is just a programming/design error to generate one. Therefore one doesn't need the ability to check if it is valid or not.

If your code generates name, values, domains, expire-times, etc. dynamically from data: Design these functions so that they do not generate "invalid" cookies. E.g. encode arbitrary values in base64/92/whatever. Names of cookies are part of their identity and need to be agreed on upfront in the design phase of your application, so just use valid names.

Conclusion: There is no need for a IsValid method on net/http.Cookie.

[gazerro]

I am not sure I understand the reason for this, especially if you control the parameters of the cookie yourself: Just do not set invalid names or values.

I totally agree that invalid cookies should not be created a priori, but the documentation of the (*Cookie).String method does not specify what exactly is intended for "invalid". There are slight differences between the cookie specification and the implementation of String (with good reason).

I agree that String should not document its implementation but in my opinion there should be a way to know if a cookie is considered valid or not by the String method.

We could make sure that the cookies comply with the specifications but we may want to do a second security check. For example:

v, err := cookie.Serialize()
if err != nil {
        log.Printf("attempt to send an invalid cookie: %s", err)
        http.Error(w, http.StatusInternalServerError, 500)
        return
}
w.Header().Add("Set-Cookie", v)

We can also use the Serialize (or IsValid) method in tests to verify that the cookies created comply with the implementation of String. This way we can verify that a change in the String implementation does not go unnoticed.

[vdobler]

but the documentation of the (*Cookie).String method does not specify what exactly is intended for "invalid".

Any valid RFC 6265 cookie is not invalid and that's all to know.

"Use valid cookies" instead of "do not use invalid cookies". It is that simple.

there should be a way to know if a cookie is considered valid or not by the String method.

Again, there is: Any valid cookie is valid and you know that. What counts as valid is defined by RFC 6265 not by the String method. The opposite case i.e. which cookies String considers invalid is a) not interesting, b) must not happen anyway, c) is un-actionable and d) is guaranteed to be an invalid cookie according to RFC 6265.

The reason why the String method doesn't consider all "RFC-6265-invalid" cookies as "Go-invalid" is to allow compatibility with broken legacy systems, frameworks and browsers. The reason why this isn't documented is because this compatibility with real-word browsers is a) important and b) a moving target.

[networkimprov]

cc @ianlancetaylor as likely proposal

[gazerro]

Any valid RFC 6265 cookie is not invalid and that0s all to know.

"value" is a valid cookie value for RFC 6265 but is invalid for the String method. Consequently I must know that the value in the Cookie.Value field should not be in the form ( DQUOTE *cookie-octet DQUOTE ) even though it is allowed for RFC 6265 because the String method handles double quotes.

[gazerro]

The reason why the String method doesn't consider all "RFC-6265-invalid" cookies as "Go-invalid" is to allow compatibility with broken legacy systems, frameworks and browsers.

It allows compatibility but not documenting the level of compatibility and not providing a function to check this compatibility, you can't rely on it to write Go code that must interact with a legacy system, framework or browser.

[vdobler]

@gazerro

I'm not sure what you want to express with

"value" is a valid cookie value for RFC 6265 but is invalid for the String method

because as stated it is wrong: Any RFC-6265-valid cookie-value is considered valid by net/http.Cookie.String.

Maybe you are misinterpreting ( DQUOTE *cookie-octet DQUOTE ) in RFC 6265: The double quotes " are not part of the cookie-value. A cookie-value in RFC 6265 must not contain double quotes. RFC 6265 allows valid cookie-values to be enclosed in quotes during transport in the HTTP header but these quotes are not part of the value: The value is the part inside the double quotes.

It allows compatibility but not documenting the level of compatibility and not providing a function to check this compatibility, you can't rely on it to write Go code that must interact with a legacy system, framework or browser.

The compatibility guaranteed is "If you follow RFC 6265 your cookies will work.". That is guaranteed and you can rely on this. If you have to interact with a legacy system which need non-RFC-6265-compliant cookies you have to test your code or work with raw headers.

[gazerro]

@vdobler

Maybe you are misinterpreting ( DQUOTE *cookie-octet DQUOTE ) in RFC 6265: The double quotes " are not part of the cookie-value.

I don't think so, the definition of cookie-value is

cookie-value = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )

so v (octet %x76) is a cookie-value but also "v" (octets %x22 %x76 %x22) is a cookie-value and cookie-value is the value of the cookie in the spec.

A cookie-value in RFC 6265 must not contain double quotes

Apart from the first and last octet.

RFC 6265 allows valid cookie-values to be enclosed in quotes during transport in the HTTP header

There is no reference to this statement in the RFC 6265.

The compatibility guaranteed is "If you follow RFC 6265 your cookies will work.". That is guaranteed and you can rely on this. If you have to interact with a legacy system which need non-RFC-6265-compliant cookies you have to test your code or work with raw headers.

In this case I don't understand why String should deviate from the standard if the purpose is not to allow a program in Go to do so using the String method.

[gazerro]

@vdobler i think the http.sanitizeCookieValue function has been implemented as if the syntax in the RFC 6265 is

cookie-pair   = cookie-name "=" ( cookie-value / ( DQUOTE cookie-value DQUOTE ) )
...
cookie-value  = *cookie-octet

but it is

cookie-pair   = cookie-name "=" cookie-value
...
cookie-value  = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )

[vdobler]

@gazerro Again. The optional DQUOTEs around the cookie value in RFC 6265 are optional and are *not part" of the value. http.sanitizeCookieValue is correct. Your reading of the RFC is wrong. Sorry.

I do not have time to look it up in the RFC and prove to you that the cookie value doesn't include the optional quotes. See e.g. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#attributes which is pretty clear that " do not belong into a cookie value and the pair of " around a value are optional and not part of the value.
Just try it out yourself in a browser. Current Chrome, Safari, Opera and Edge all strip the DQUOTEs and use just the value inside.
The code is correct and works as documented, and intended by the relevant RFCs and all modern browsers.

[gazerro]

@vdobler thank you for your reply, I've opened a the new issue #46443 with all the details.

[vdobler]

I doubt that both proposed godoc comments for (*Cookie).Valid are suitable.

In

// Valid reports whether the cookie is valid as per the String method,
// that is, String does not discard or change any fields.
func (c *Cookie) Valid() error

the "change any field" is at least misleading (if not wrong). The (*Cookie).String method "changes":

• cookie.MaxAge=-1 to Max-Age=0.
• the timezone of Expires to UTC/GMT.
• and quotes the "value" if it it contains spaces: cookie.Value="a b" is "changed" to name="a b". This technically is not a change but just encoding but e.g. @gazerro got that wrong by falsely attributing RFC 6265's cookie-value with net/http.Cookie.Value. Others might be tripped by that encoding to be a "change" too.)

Maybe "String does not discard or sanitize any fields" would be better.

And

// Valid reports whether the cookie is valid.
func (c *Cookie) Valid() error

is almost a circular definition. "A cookie is valid iff Valid". A Go Cookie whose (*Cookie).Valid method returns nil is a cookie which will be serialised by net/http "unaltered", "unsanitized" ("unchanged") in a HTTP header but it still might be "invalid" for a lot of different reasons:

• It might be (*Cookie).Valid but invalid according to RFC626 (e.g. if the Value contains a space).
• It's value might be too long and discarded by the client or server.
• Cookie.Valid just does syntactical checks on Domain and Path and a (*Cookie).Valid cookie can still be "invalid" for a certain request or response.

Maybe "Valid reports whether the cookie is syntactically valid." would be better.

[vdobler]

It sounds like we converged on adding (*Cookie).Valid and nothing else. Do I have that right?

I do not know which arguments where discussed in the proposal review meeting but I'm still not really convinced by the arguments in this issue that (*Cookie).Valid brings that much benefit. There have been two use cases stated here in this proposal:

1. "When a cookie is populated with dynamic data from an http request or some other external source. [...] check cookie validity beforehand, and handle invalid cookies gracefully by [...]"

For this use case a (*Cookie).Valid method helps but this method just catches most syntactically invalid cookies. A Cookie.Value might not contain illegal bytes but be too long for the client's taste, it's Domain may be formally a valid domain name but still be unsuitable as the Domain attribute, etc. So if you are dealing with third-party systems which generate cookie data for you but do so in a manner that is not guaranteed to be correct you have to write complicated validation logic anyway.

Or is there really a realistic case where the semantics of dynamically generated Domain, Path, Value, Expires, Secure/https are all okay, it just happens that sometimes Path contains an emoji or value contains newlines?

2. "When using statically defined cookie data, a validation check can still be valuable to both assert that the value as you define it is correct, and that the meaning of a "go valid cookie" has not changed."

This can be achieved by faking a cookie roundtrip: Create a request, add your cookies, serialize the request, pars it again and check your cookies are there "unaltered". This is far less convenient than calling (*Cookie).Valid but it can be done, it's not that complicated and it even is clearer on what the test actually want's to verify.

[gazerro]

@gazerro got that wrong by falsely attributing RFC 6265's cookie-value with net/http.Cookie.Value

@vdobler please, don't attribute to me statements I have not made.

[vdobler]

@gazerro Please excuse if I misread your comments #46370 (comment) and #46370 (comment) .

I was under the impression that you think "abc" is a valid cookie value (Cookie.Value) according to RFC 6265.

[gazerro]

@vdobler I think we have two different and irreconcilable interpretations of what a "cookie value" is and what a "valid value" is for the RFC 6265 and the http.Cookie implementation. This is fine but, this leads to misunderstandings.

The new method should be used to check whether String serializes the cookie as expected or not. So, in my opinion, what we have to decide is what is "expected" or perhaps a different name for the method might be more appropriate?

[vdobler]

@gazerro Yes our views are different.

I do share the view of bradfitz expressed in #18627 (comment), the view of the Mozilla developers expressed in https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#attributes and the view of RFC 2965 where these double quotes originate.

You share the intended reading of RFC 6265 as clarified in RFC 6265bis-02 draft and what browser engines do.

It seems that the semantic meaning of "value of a cookie" has changed in the past 15 years and that treating a=foo and a="foo" as semantically equal is now the sole business of the application interpreting that cookie.

[nulltrope]

It seems like we keep coming back to the Go valid cookie vs. RFC 6265 valid cookie debate which, while I think is probably worth talking about, is not the issue this proposal is trying to solve.

Whether or not it was the original intention, the (*http.Cookie).String method has a handful of explicit "error" conditions, which will just log to stderr and discard a field.

All im proposing is a way to catch these "syntactically" invalid (according to (*http.Cookie).String) Cookies and implement proper error handling in your own code.

1. "When a cookie is populated with dynamic data from an http request or some other external source. [...] check cookie validity beforehand, and handle invalid cookies gracefully by [...]"

For this use case a (*Cookie).Valid method helps but this method just catches most syntactically invalid cookies. A Cookie.Value might not contain illegal bytes but be too long for the client's taste, it's Domain may be formally a valid domain name but still be unsuitable as the Domain attribute, etc. So if you are dealing with third-party systems which generate cookie data for you but do so in a manner that is not guaranteed to be correct you have to write complicated validation logic anyway.

Or is there really a realistic case where the semantics of dynamically generated Domain, Path, Value, Expires, Secure/https are all okay, it just happens that sometimes Path contains an emoji or value contains newlines?

This wouldn't attempt to solve the problem of clients not accepting a cookie, as that is a whole separate issue. It should be the job of the client to report to the user when/if it rejects a cookie and why (for example, browser dev tools typically show you when a cookie was rejected).

Really what it comes down to is that (*http.Cookie).String tries to be "smart" and preemptively exclude fields it thinks clients may reject. But unlike letting the client reject the cookie (as would be expected), it silently discards fields.

I think it's good to handle what cookie validation we can on the server side, as server side will naturally be easier to debug, but when there's no way to tell when the validation fails it doesn't exactly add as much value.

1. "When using statically defined cookie data, a validation check can still be valuable to both assert that the value as you define it is correct, and that the meaning of a "go valid cookie" has not changed."

This can be achieved by faking a cookie roundtrip: Create a request, add your cookies, serialize the request, pars it again and check your cookies are there "unaltered". This is far less convenient than calling (*Cookie).Valid but it can be done, it's not that complicated and it even is clearer on what the test actually want's to verify.

Sure, there are other ways to achieve this but like you said, they are more complex. If these cookie validation functions already exist in the stdlib, it seems an easy win to me to simply expose them under a new public method.

[vdobler]

@nulltrope

Really what it comes down to is that (*http.Cookie).String tries to be "smart" and preemptively exclude fields it thinks clients may reject. But unlike letting the client reject the cookie (as would be expected), it silently discards fields.

This is the first very good argument (in the sense of convincing me ;-) in this thread.

There are characters that are impossible to send (e.g. 0x0a or 0x0d) and stuff that might work (e.g. chars in the 0x80-0xff range and or UTF-8 encoded stuff). With (*http.Cookie).String being too "patronizing" this leads naturally to a new question:

Should there be a new field like RawValue in Cookie like there are in net.URL? Such a RawValue could distinguish between a unset value Cookie: name= yielding RawValue==""and a deliberately quoted empty value Cookie: name="" yielding RawValue=="\"\"" and allow to send RFC 6265 invalid cookies and let the client reject them? This would probably also help solving #46443.

(*http.Cookie).String would use Cookie.RawValue if set and not clearly impossible to send (i.e. containing newlines or null bytes) and Cookie.Value otherwise.

(*http.Cookie).Valid would report whether (*http.Cookie).String would have to sanitize its input, Cookie.Value or Cookie.RawValue. With Cookie.Value being sanitized heavier than Cookie.RawValue.

[nulltrope]

@vdobler I don't have strong feelings either way about adding a new RawValue field, but it does seem like it increases the complexity a bit. It sounds like what you're proposing wouldn't change the need for a (*http.Cookie).Valid method, so it may make sense to treat RawValue as a separate feature proposal?

[rsc]

The problem being solved here is that http.SetCookie silently throws away cookies it deems invalid.
Adding Cookie.Valid provides a way to know whether the cookie is actually going to be sent.
The conversation above aside, most people seem in favor of adding Valid to solve this problem.
It is true that one can infer this from "faking a cookie roundtrip" but that seems fairly heavy-weight for a simple question,
and one that we can easily answer (the code exists already).

[rsc]

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

[rsc]

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

[nulltrope]

I'd be happy to take a stab at implementing this, i'm pretty familiar with the cookie validation logic at this point.

[gopherbot]

Change https://golang.org/cl/338590 mentions this issue: net/http: add Cookie.Valid method