Merge pull request #2697 from matpen-wi/feat/max-age-instead-of-expires

pkg/cookies: use 'Max-Age' instead of 'Expires' for cookie expiration

CHANGELOG.md

@@ -9,6 +9,7 @@
 ## Changes since v7.8.1
 
 - [#2927](https://github.com/oauth2-proxy/oauth2-proxy/pull/2927) chore(deps/build): bump golang to 1.23 and use go.mod as single point of truth for all build files (@tuunit)
+- [#2697](https://github.com/oauth2-proxy/oauth2-proxy/pull/2697) Use `Max-Age` instead of `Expires` for cookie expiration (@matpen-wi)
 
 # V7.8.1
 

pkg/cookies/cookies.go

@@ -14,7 +14,7 @@ import (
 
 // MakeCookieFromOptions constructs a cookie based on the given *options.CookieOptions,
 // value and creation time
-func MakeCookieFromOptions(req *http.Request, name string, value string, opts *options.Cookie, expiration time.Duration, now time.Time) *http.Cookie {
+func MakeCookieFromOptions(req *http.Request, name string, value string, opts *options.Cookie, expiration time.Duration) *http.Cookie {
 	domain := GetCookieDomain(req, opts.Domains)
 	// If nothing matches, create the cookie with the shortest domain
 	if domain == "" && len(opts.Domains) > 0 {
@@ -35,8 +35,10 @@ func MakeCookieFromOptions(req *http.Request, name string, value string, opts *o
 		SameSite: ParseSameSite(opts.SameSite),
 	}
 
-	if expiration != time.Duration(0) {
+	if expiration > time.Duration(0) {
-		c.Expires = now.Add(expiration)
+		c.MaxAge = int(expiration.Seconds())
+	} else if expiration < time.Duration(0) {
+		c.MaxAge = -1
 	}
 
 	warnInvalidDomain(c, req)

pkg/cookies/cookies_suite_test.go

@@ -1,9 +1,7 @@
 package cookies
 
 import (
-	"net/http"
 	"testing"
-	"time"
 
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"
 	. "github.com/onsi/ginkgo/v2"
@@ -28,8 +26,3 @@ func TestProviderSuite(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Cookies")
 }
-
-func testCookieExpires(exp time.Time) string {
-	var buf [len(http.TimeFormat)]byte
-	return string(exp.UTC().AppendFormat(buf[:0], http.TimeFormat))
-}

pkg/cookies/cookies_test.go

@@ -87,7 +87,7 @@ var _ = Describe("Cookie Tests", func() {
 			opts           options.Cookie
 			expiration     time.Duration
 			now            time.Time
-			expectedOutput time.Time
+			expectedOutput int
 		}
 
 		validName := "_oauth2_proxy"
@@ -95,7 +95,7 @@ var _ = Describe("Cookie Tests", func() {
 		domains := []string{"www.cookies.test"}
 
 		now := time.Now()
-		var expectedExpires time.Time
+		var expectedMaxAge int
 
 		DescribeTable("should return cookies with or without expiration",
 			func(in makeCookieFromOptionsTableInput) {
@@ -106,7 +106,7 @@ var _ = Describe("Cookie Tests", func() {
 				)
 				Expect(err).ToNot(HaveOccurred())
 
-				Expect(MakeCookieFromOptions(req, in.name, in.value, &in.opts, in.expiration, in.now).Expires).To(Equal(in.expectedOutput))
+				Expect(MakeCookieFromOptions(req, in.name, in.value, &in.opts, in.expiration).MaxAge).To(Equal(in.expectedOutput))
 			},
 			Entry("persistent cookie", makeCookieFromOptionsTableInput{
 				host:  "www.cookies.test",
@@ -125,7 +125,26 @@ var _ = Describe("Cookie Tests", func() {
 				},
 				expiration:     15 * time.Minute,
 				now:            now,
-				expectedOutput: now.Add(15 * time.Minute),
+				expectedOutput: int((15 * time.Minute).Seconds()),
+			}),
+			Entry("persistent cookie to be cleared", makeCookieFromOptionsTableInput{
+				host:  "www.cookies.test",
+				name:  validName,
+				value: "1",
+				opts: options.Cookie{
+					Name:     validName,
+					Secret:   validSecret,
+					Domains:  domains,
+					Path:     "",
+					Expire:   time.Hour * -1,
+					Refresh:  15 * time.Minute,
+					Secure:   true,
+					HTTPOnly: false,
+					SameSite: "",
+				},
+				expiration:     time.Hour * -1,
+				now:            now,
+				expectedOutput: -1,
 			}),
 			Entry("session cookie", makeCookieFromOptionsTableInput{
 				host:  "www.cookies.test",
@@ -144,7 +163,7 @@ var _ = Describe("Cookie Tests", func() {
 				},
 				expiration:     0,
 				now:            now,
-				expectedOutput: expectedExpires,
+				expectedOutput: expectedMaxAge,
 			}),
 		)
 	})

pkg/cookies/csrf.go

@@ -145,7 +145,6 @@ func (c *csrf) SetCookie(rw http.ResponseWriter, req *http.Request) (*http.Cooki
 		encoded,
 		c.cookieOpts,
 		c.cookieOpts.CSRFExpire,
-		c.time.Now(),
 	)
 	http.SetCookie(rw, cookie)
 
@@ -160,7 +159,6 @@ func (c *csrf) ClearCookie(rw http.ResponseWriter, req *http.Request) {
 		"",
 		c.cookieOpts,
 		time.Hour*-1,
-		c.time.Now(),
 	))
 }
 

pkg/cookies/csrf_per_request_test.go

@@ -159,10 +159,10 @@ var _ = Describe("CSRF Cookie with non-fixed name Tests", func() {
 				))
 				Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring(
 					fmt.Sprintf(
-						"; Path=%s; Domain=%s; Expires=%s; HttpOnly; Secure",
+						"; Path=%s; Domain=%s; Max-Age=%d; HttpOnly; Secure",
 						cookiePath,
 						cookieDomain,
-						testCookieExpires(testNow.Add(cookieOpts.CSRFExpire)),
+						int(cookieOpts.CSRFExpire.Seconds()),
 					),
 				))
 			})
@@ -176,11 +176,10 @@ var _ = Describe("CSRF Cookie with non-fixed name Tests", func() {
 
 				Expect(rw.Header().Get("Set-Cookie")).To(Equal(
 					fmt.Sprintf(
-						"%s=; Path=%s; Domain=%s; Expires=%s; HttpOnly; Secure",
+						"%s=; Path=%s; Domain=%s; Max-Age=0; HttpOnly; Secure",
 						privateCSRF.cookieName(),
 						cookiePath,
 						cookieDomain,
-						testCookieExpires(testNow.Add(time.Hour*-1)),
 					),
 				))
 			})

pkg/cookies/csrf_test.go

@@ -161,10 +161,10 @@ var _ = Describe("CSRF Cookie Tests", func() {
 				))
 				Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring(
 					fmt.Sprintf(
-						"; Path=%s; Domain=%s; Expires=%s; HttpOnly; Secure",
+						"; Path=%s; Domain=%s; Max-Age=%d; HttpOnly; Secure",
 						cookiePath,
 						cookieDomain,
-						testCookieExpires(testNow.Add(cookieOpts.CSRFExpire)),
+						int(cookieOpts.CSRFExpire.Seconds()),
 					),
 				))
 			})
@@ -239,11 +239,10 @@ var _ = Describe("CSRF Cookie Tests", func() {
 
 				Expect(rw.Header().Get("Set-Cookie")).To(Equal(
 					fmt.Sprintf(
-						"%s=; Path=%s; Domain=%s; Expires=%s; HttpOnly; Secure",
+						"%s=; Path=%s; Domain=%s; Max-Age=0; HttpOnly; Secure",
 						privateCSRF.cookieName(),
 						cookiePath,
 						cookieDomain,
-						testCookieExpires(testNow.Add(time.Hour*-1)),
 					),
 				))
 			})

pkg/sessions/cookie/session_store.go

@@ -74,7 +74,7 @@ func (s *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) error {
 
 	for _, c := range req.Cookies() {
 		if cookieNameRegex.MatchString(c.Name) {
-			clearCookie := s.makeCookie(req, c.Name, "", time.Hour*-1, time.Now())
+			clearCookie := s.makeCookie(req, c.Name, "", time.Hour*-1)
 
 			http.SetCookie(rw, clearCookie)
 		}
@@ -126,21 +126,20 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now ti
 			return nil, err
 		}
 	}
-	c := s.makeCookie(req, s.Cookie.Name, strValue, s.Cookie.Expire, now)
+	c := s.makeCookie(req, s.Cookie.Name, strValue, s.Cookie.Expire)
 	if len(c.String()) > maxCookieLength {
 		return splitCookie(c), nil
 	}
 	return []*http.Cookie{c}, nil
 }
 
-func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie {
+func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration) *http.Cookie {
 	return pkgcookies.MakeCookieFromOptions(
 		req,
 		name,
 		value,
 		s.Cookie,
 		expiration,
-		now,
 	)
 }
 

pkg/sessions/persistence/ticket.go

@@ -223,7 +223,6 @@ func (t *ticket) clearCookie(rw http.ResponseWriter, req *http.Request) {
 		"",
 		t.options,
 		time.Hour*-1,
-		time.Now(),
 	))
 }
 
@@ -242,7 +241,6 @@ func (t *ticket) makeCookie(req *http.Request, value string, expires time.Durati
 		value,
 		t.options,
 		expires,
-		now,
 	), nil
 }
 

pkg/sessions/tests/session_store_tests.go

@@ -385,7 +385,7 @@ func SessionStoreInterfaceTests(in *testInput) {
 				broken := "BrokenSessionFromADifferentSessionImplementation"
 				value, err := encryption.SignedValue(in.cookieOpts.Secret, in.cookieOpts.Name, []byte(broken), time.Now())
 				Expect(err).ToNot(HaveOccurred())
-				cookie := cookiesapi.MakeCookieFromOptions(in.request, in.cookieOpts.Name, value, in.cookieOpts, in.cookieOpts.Expire, time.Now())
+				cookie := cookiesapi.MakeCookieFromOptions(in.request, in.cookieOpts.Name, value, in.cookieOpts, in.cookieOpts.Expire)
 				in.request.AddCookie(cookie)
 
 				err = in.ss().Save(in.response, in.request, in.session)