diff --git a/CHANGELOG.md b/CHANGELOG.md index cb53e319..f1398152 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/pkg/cookies/cookies.go b/pkg/cookies/cookies.go index 1e017366..24ae2841 100644 --- a/pkg/cookies/cookies.go +++ b/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) { - c.Expires = now.Add(expiration) + if expiration > time.Duration(0) { + c.MaxAge = int(expiration.Seconds()) + } else if expiration < time.Duration(0) { + c.MaxAge = -1 } warnInvalidDomain(c, req) diff --git a/pkg/cookies/cookies_suite_test.go b/pkg/cookies/cookies_suite_test.go index 1e2e6f9c..f4893cbd 100644 --- a/pkg/cookies/cookies_suite_test.go +++ b/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)) -} diff --git a/pkg/cookies/cookies_test.go b/pkg/cookies/cookies_test.go index 31b155a9..ef0fbd4f 100644 --- a/pkg/cookies/cookies_test.go +++ b/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, }), ) }) diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go index b1f95763..698a7c8d 100644 --- a/pkg/cookies/csrf.go +++ b/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(), )) } diff --git a/pkg/cookies/csrf_per_request_test.go b/pkg/cookies/csrf_per_request_test.go index 79d633bb..28e2e296 100644 --- a/pkg/cookies/csrf_per_request_test.go +++ b/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)), ), )) }) diff --git a/pkg/cookies/csrf_test.go b/pkg/cookies/csrf_test.go index 8b22e408..37527bd0 100644 --- a/pkg/cookies/csrf_test.go +++ b/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)), ), )) }) diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index f2f4045f..3947177f 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/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, ) } diff --git a/pkg/sessions/persistence/ticket.go b/pkg/sessions/persistence/ticket.go index 5020ada9..581a7f45 100644 --- a/pkg/sessions/persistence/ticket.go +++ b/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 } diff --git a/pkg/sessions/tests/session_store_tests.go b/pkg/sessions/tests/session_store_tests.go index 8e2d02f9..a4818ef2 100644 --- a/pkg/sessions/tests/session_store_tests.go +++ b/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)