net/http: cookie without Expires should be considered valid

[amitsaha]

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

$ go version
go version go1.18.1

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
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/echorand/Library/Caches/go-build"
GOENV="/Users/echorand/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/echorand/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/echorand/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.18.1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/zd/drdnv8qn1v3_0sl2v48kdrqc0000gn/T/go-build3714008858=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

For a cookie, it seems like both, Expires and MaxAge must be specified? The expectation is that, only MaxAge should be sufficient.

// valid

c1 := http.Cookie{
        Expires: time.Now().Add(3600 * 24 * time.Second),
        MaxAge:  24 * 3600, // 24 hours
}

// invalid

c2 := http.Cookie{  
        MaxAge:  24 * 3600, // 24 hours
}

Example: https://go.dev/play/p/3PTUM6WKwWS

What did you expect to see?

I expected Expires field to be optional and hence Valid() should return nil for the second case too.

What did you see instead?

Non-nil error:

cookie c2 valid?  http: invalid Cookie.Expires

Based on my understanding, this should be the fix:

diff --git a/src/net/http/cookie.go b/src/net/http/cookie.go
index 9cb0804f8f..8fe36adfcd 100644
--- a/src/net/http/cookie.go
+++ b/src/net/http/cookie.go
@@ -246,8 +246,10 @@ func (c *Cookie) Valid() error {
 	if !isCookieNameValid(c.Name) {
 		return errors.New("http: invalid Cookie.Name")
 	}
-	if !validCookieExpires(c.Expires) {
-		return errors.New("http: invalid Cookie.Expires")
+	if !c.Expires.IsZero() {
+		if !validCookieExpires(c.Expires) {
+			return errors.New("http: invalid Cookie.Expires")
+		}
 	}
 	for i := 0; i < len(c.Value); i++ {
 		if !validCookieValueByte(c.Value[i]) {

I am happy to create a PR if this seems like a valid issue. Thanks.

[krader1961]

The current behavior is probably explained, at least partially, by https://mrcoles.com/blog/cookies-max-age-vs-expires/. That is, by a desire to be compatible with old Microsoft Internet Explorer browsers and more modern browsers. I have no idea what the preferred trade-off is today vis-a-vis supporting ancient browsers, but any change of this nature requires careful consideration with regard to potentially breaking support for old, but not ancient, HTTP clients.

[shuLhan]

@amitsaha Based on my understanding, as per RFC 6265 Section 4.1.2.2 and Section 5.3,

• the check for Expires should be done if MaxAge is 0,
• cookie is valid if both MaxAge is 0 and Expires.IsZero() is true.

If a cookie has both the Max-Age and the Expires attribute, the Max-
Age attribute has precedence and controls the expiration date of the
cookie.  If a cookie has neither the Max-Age nor the Expires
attribute, the user agent will retain the cookie until "the current
session is over" (as defined by the user agent).

So, the patch should be at least like these, yes?

-	if !validCookieExpires(c.Expires) {
-		return errors.New("http: invalid Cookie.Expires")
+	if c.MaxAge == 0 && !c.Expires.IsZero() {
+		if !validCookieExpires(c.Expires) {
+			return errors.New("http: invalid Cookie.Expires")
+		}
 	}

@krader1961 I believe the context here is cookie from client side (before it send), not server/browser side.

[ZekeLu]

See the proposal that adds this method: #46370.

It's unfortunate that the minimal documentation is chosen. The commit message adds more context:

net/http: add Cookie.Valid method

The (*http.Cookie).String method used by SetCookie will silently discard
or sanitize any fields it deems invalid, making it difficult to tell
whether a cookie will be sent as expected.

This change introduces a new (*http.Cookie).Valid method which may be
used to check if any cookie fields will be discarded or sanitized prior
to calling (*http.Cookie).String.

[amitsaha]

The current behavior is probably explained, at least partially, by https://mrcoles.com/blog/cookies-max-age-vs-expires/. That is, by a desire to be compatible with old Microsoft Internet Explorer browsers and more modern browsers. I have no idea what the preferred trade-off is today vis-a-vis supporting ancient browsers, but any change of this nature requires careful consideration with regard to potentially breaking support for old, but not ancient, HTTP clients.

My concern is that the Expires field is currently optional as per the code, and hence, the behavior of Valid() should reflect that. Of course now having learned about what the spec states, I agree with the proposed fix in #52989 (comment)

Also, my current context is when setting it from the server-side.

[amitsaha]

See the proposal that adds this method: #46370.

It's unfortunate that the minimal documentation is chosen. The commit message adds more context:

net/http: add Cookie.Valid method

The (*http.Cookie).String method used by SetCookie will silently discard
or sanitize any fields it deems invalid, making it difficult to tell
whether a cookie will be sent as expected.

This change introduces a new (*http.Cookie).Valid method which may be
used to check if any cookie fields will be discarded or sanitized prior
to calling (*http.Cookie).String.

That's unfortunate indeed. The method name, Valid() seems to be only useful for figuring out whether any of the fields will be discarded or not, rather than the validity of the cookie. From where I am, still, I can see a good reason for the proposed fix as per: #52989 (comment)

[mknyszek]

CC @neild

[neild]

I agree that Cookie.Valid shouldn't complain about a cookie with a zero-valued Expires.

[amitsaha]

Some automation will likely kick in, but i have created a PR: https://go-review.googlesource.com/c/go/+/338590/ - thanks all for the discussions and feedback.

[gopherbot]

Change https://go.dev/cl/407654 mentions this issue: net/http: Cookie.Valid() method should allow zero-valued Expires