From 0bf690d44c251223d1caaa4aed6d8ba1be9044f0 Mon Sep 17 00:00:00 2001 From: Joost <439100+jvnoije@users.noreply.github.com> Date: Mon, 16 Mar 2026 10:56:04 +0100 Subject: [PATCH] All cookie variables in a struct Signed-off-by: Joost <439100+jvnoije@users.noreply.github.com> --- pkg/cookies/cookies.go | 28 +++++--- pkg/cookies/cookies_test.go | 82 +++++++++-------------- pkg/cookies/csrf.go | 40 ++++++----- pkg/cookies/csrf_per_request_test.go | 20 +++--- pkg/sessions/cookie/session_store.go | 37 ++++++---- pkg/sessions/persistence/ticket.go | 39 ++++++----- pkg/sessions/tests/session_store_tests.go | 12 +++- 7 files changed, 144 insertions(+), 114 deletions(-) diff --git a/pkg/cookies/cookies.go b/pkg/cookies/cookies.go index 178f77f4..be5ee451 100644 --- a/pkg/cookies/cookies.go +++ b/pkg/cookies/cookies.go @@ -7,14 +7,24 @@ import ( "strings" "time" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" requestutil "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests/util" ) +type CookieOptions struct { + Name string + Value string + Domains []string + Expiration time.Duration + SameSite string + Path string + HTTPOnly bool + Secure bool +} + // 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, sameSite string) *http.Cookie { +func MakeCookieFromOptions(req *http.Request, opts *CookieOptions) *http.Cookie { domain := GetCookieDomain(req, opts.Domains) // If nothing matches, create the cookie with the shortest domain if domain == "" && len(opts.Domains) > 0 { @@ -26,18 +36,18 @@ func MakeCookieFromOptions(req *http.Request, name string, value string, opts *o } c := &http.Cookie{ - Name: name, - Value: value, + Name: opts.Name, + Value: opts.Value, Path: opts.Path, Domain: domain, HttpOnly: opts.HTTPOnly, Secure: opts.Secure, - SameSite: ParseSameSite(sameSite), + SameSite: ParseSameSite(opts.SameSite), } - if expiration > time.Duration(0) { - c.MaxAge = int(expiration.Seconds()) - } else if expiration < time.Duration(0) { + if opts.Expiration > time.Duration(0) { + c.MaxAge = int(opts.Expiration.Seconds()) + } else if opts.Expiration < time.Duration(0) { c.MaxAge = -1 } @@ -58,7 +68,7 @@ func GetCookieDomain(req *http.Request, cookieDomains []string) string { return "" } -// Parse a valid http.SameSite value from a user supplied string for use of making cookies. +// ParseSameSite a valid http.SameSite value from a user supplied string for use of making cookies. func ParseSameSite(v string) http.SameSite { switch v { case "lax": diff --git a/pkg/cookies/cookies_test.go b/pkg/cookies/cookies_test.go index 5664dffe..b67f8a69 100644 --- a/pkg/cookies/cookies_test.go +++ b/pkg/cookies/cookies_test.go @@ -5,8 +5,6 @@ import ( "net/http" "time" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" - middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -82,16 +80,12 @@ var _ = Describe("Cookie Tests", func() { Context("MakeCookieFromOptions", func() { type makeCookieFromOptionsTableInput struct { host string - name string - value string - opts options.Cookie - expiration time.Duration + opts CookieOptions now time.Time expectedOutput int } validName := "_oauth2_proxy" - validSecret := "secretthirtytwobytes+abcdefghijk" domains := []string{"www.cookies.test"} now := time.Now() @@ -106,62 +100,50 @@ var _ = Describe("Cookie Tests", func() { ) Expect(err).ToNot(HaveOccurred()) - Expect(MakeCookieFromOptions(req, in.name, in.value, &in.opts, in.expiration, in.opts.SameSite).MaxAge).To(Equal(in.expectedOutput)) + Expect(MakeCookieFromOptions(req, &in.opts).MaxAge).To(Equal(in.expectedOutput)) }, Entry("persistent cookie", makeCookieFromOptionsTableInput{ - host: "www.cookies.test", - name: validName, - value: "1", - opts: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: domains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: true, - HTTPOnly: false, - SameSite: "", + host: "www.cookies.test", + opts: CookieOptions{ + Name: validName, + Value: "1", + Domains: domains, + Expiration: 15 * time.Minute, + SameSite: "", + Path: "", + HTTPOnly: false, + Secure: true, }, - expiration: 15 * time.Minute, now: now, 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: "", + host: "www.cookies.test", + opts: CookieOptions{ + Name: validName, + Value: "1", + Domains: domains, + Expiration: time.Hour * -1, + SameSite: "", + Path: "", + HTTPOnly: false, + Secure: true, }, - expiration: time.Hour * -1, now: now, expectedOutput: -1, }), Entry("session cookie", makeCookieFromOptionsTableInput{ - host: "www.cookies.test", - name: validName, - value: "1", - opts: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: domains, - Path: "", - Expire: 0, - Refresh: 15 * time.Minute, - Secure: true, - HTTPOnly: false, - SameSite: "", + host: "www.cookies.test", + opts: CookieOptions{ + Name: validName, + Value: "1", + Domains: domains, + Expiration: 0, + SameSite: "", + Path: "", + HTTPOnly: false, + Secure: true, }, - expiration: 0, now: now, expectedOutput: expectedMaxAge, }), diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go index cef7a029..ce17f164 100644 --- a/pkg/cookies/csrf.go +++ b/pkg/cookies/csrf.go @@ -150,14 +150,18 @@ func (c *csrf) SetCookie(rw http.ResponseWriter, req *http.Request) (*http.Cooki return nil, err } - cookie := MakeCookieFromOptions( - req, - c.cookieName(), - encoded, - c.cookieOpts, - c.cookieOpts.CSRFExpire, - getCSRFSameSite(c.cookieOpts), - ) + csrfCookieOptions := &CookieOptions{ + Name: c.cookieName(), + Value: encoded, + Domains: c.cookieOpts.Domains, + Expiration: c.cookieOpts.CSRFExpire, + SameSite: getCSRFSameSite(c.cookieOpts), + Path: c.cookieOpts.Path, + HTTPOnly: c.cookieOpts.HTTPOnly, + Secure: c.cookieOpts.Secure, + } + + cookie := MakeCookieFromOptions(req, csrfCookieOptions) http.SetCookie(rw, cookie) return cookie, nil @@ -207,14 +211,18 @@ func ClearExtraCsrfCookies(opts *options.Cookie, rw http.ResponseWriter, req *ht // ClearCookie removes the CSRF cookie func (c *csrf) ClearCookie(rw http.ResponseWriter, req *http.Request) { - http.SetCookie(rw, MakeCookieFromOptions( - req, - c.cookieName(), - "", - c.cookieOpts, - time.Hour*-1, - getCSRFSameSite(c.cookieOpts), - )) + csrfCookieOptions := &CookieOptions{ + Name: c.cookieName(), + Value: "", + Domains: c.cookieOpts.Domains, + Expiration: time.Hour * -1, + SameSite: getCSRFSameSite(c.cookieOpts), + Path: c.cookieOpts.Path, + HTTPOnly: c.cookieOpts.HTTPOnly, + Secure: c.cookieOpts.Secure, + } + + http.SetCookie(rw, MakeCookieFromOptions(req, csrfCookieOptions)) } // encodeCookie MessagePack encodes and encrypts the CSRF and then creates a diff --git a/pkg/cookies/csrf_per_request_test.go b/pkg/cookies/csrf_per_request_test.go index 6ec07d83..59ff0a8a 100644 --- a/pkg/cookies/csrf_per_request_test.go +++ b/pkg/cookies/csrf_per_request_test.go @@ -216,14 +216,18 @@ var _ = Describe("CSRF Cookie with non-fixed name Tests", func() { for _, csrf := range []*csrf{privateCSRF1, privateCSRF2, privateCSRF3} { encoded, err := csrf.encodeCookie() Expect(err).ToNot(HaveOccurred()) - cookie := MakeCookieFromOptions( - req, - csrf.cookieName(), - encoded, - csrf.cookieOpts, - csrf.cookieOpts.CSRFExpire, - csrf.cookieOpts.CSRFSameSite, - ) + csrfCookieOptions := &CookieOptions{ + Name: csrf.cookieName(), + Value: encoded, + Domains: csrf.cookieOpts.Domains, + Expiration: csrf.cookieOpts.CSRFExpire, + SameSite: getCSRFSameSite(csrf.cookieOpts), + Path: csrf.cookieOpts.Path, + HTTPOnly: csrf.cookieOpts.HTTPOnly, + Secure: csrf.cookieOpts.Secure, + } + + cookie := MakeCookieFromOptions(req, csrfCookieOptions) cookies = append(cookies, fmt.Sprintf("%v=%v", cookie.Name, cookie.Value)) } diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index c5fab5c0..a4da3734 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/pkg/sessions/cookie/session_store.go @@ -76,7 +76,17 @@ 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) + sessionCookieOptions := &pkgcookies.CookieOptions{ + Name: c.Name, + Value: "", + Domains: s.Cookie.Domains, + Expiration: time.Hour * -1, + SameSite: s.Cookie.SameSite, + Path: s.Cookie.Path, + HTTPOnly: s.Cookie.HTTPOnly, + Secure: s.Cookie.Secure, + } + clearCookie := pkgcookies.MakeCookieFromOptions(req, sessionCookieOptions) http.SetCookie(rw, clearCookie) } @@ -117,7 +127,7 @@ func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Reques return nil } -// makeSessionCookie creates an http.Cookie containing the authenticated user's +// makeSessionCookie creates a http.Cookie containing the authenticated user's // authentication details func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now time.Time) ([]*http.Cookie, error) { strValue := string(value) @@ -132,24 +142,23 @@ 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) + sessionCookieOptions := &pkgcookies.CookieOptions{ + Name: s.Cookie.Name, + Value: strValue, + Domains: s.Cookie.Domains, + Expiration: s.Cookie.Expire, + SameSite: s.Cookie.SameSite, + Path: s.Cookie.Path, + HTTPOnly: s.Cookie.HTTPOnly, + Secure: s.Cookie.Secure, + } + c := pkgcookies.MakeCookieFromOptions(req, sessionCookieOptions) 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) *http.Cookie { - return pkgcookies.MakeCookieFromOptions( - req, - name, - value, - s.Cookie, - expiration, - s.Cookie.SameSite, - ) -} - // NewCookieSessionStore initialises a new instance of the SessionStore from // the configuration given func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessions.SessionStore, error) { diff --git a/pkg/sessions/persistence/ticket.go b/pkg/sessions/persistence/ticket.go index 98ee32f7..c955143a 100644 --- a/pkg/sessions/persistence/ticket.go +++ b/pkg/sessions/persistence/ticket.go @@ -221,14 +221,17 @@ func (t *ticket) setCookie(rw http.ResponseWriter, req *http.Request, s *session // clearCookie removes any cookies that would be where this ticket // would set them func (t *ticket) clearCookie(rw http.ResponseWriter, req *http.Request) { - http.SetCookie(rw, cookies.MakeCookieFromOptions( - req, - t.options.Name, - "", - t.options, - time.Hour*-1, - t.options.SameSite, - )) + cookieOptions := &cookies.CookieOptions{ + Name: t.options.Name, + Value: "", + Domains: t.options.Domains, + Expiration: time.Hour * -1, + SameSite: t.options.SameSite, + Path: t.options.Path, + HTTPOnly: t.options.HTTPOnly, + Secure: t.options.Secure, + } + http.SetCookie(rw, cookies.MakeCookieFromOptions(req, cookieOptions)) } // makeCookie makes a cookie, signing the value if present @@ -245,14 +248,18 @@ func (t *ticket) makeCookie(req *http.Request, value string, expires time.Durati } } - return cookies.MakeCookieFromOptions( - req, - t.options.Name, - value, - t.options, - expires, - t.options.SameSite, - ), nil + cookieOptions := &cookies.CookieOptions{ + Name: t.options.Name, + Value: value, + Domains: t.options.Domains, + Expiration: expires, + SameSite: t.options.SameSite, + Path: t.options.Path, + HTTPOnly: t.options.HTTPOnly, + Secure: t.options.Secure, + } + + return cookies.MakeCookieFromOptions(req, cookieOptions), nil } // makeCipher makes a AES-GCM cipher out of the ticket's secret diff --git a/pkg/sessions/tests/session_store_tests.go b/pkg/sessions/tests/session_store_tests.go index fe3a324b..dd678042 100644 --- a/pkg/sessions/tests/session_store_tests.go +++ b/pkg/sessions/tests/session_store_tests.go @@ -422,7 +422,17 @@ 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, in.cookieOpts.SameSite) + cookieOptions := &cookiesapi.CookieOptions{ + Name: in.cookieOpts.Name, + Value: value, + Domains: in.cookieOpts.Domains, + Expiration: in.cookieOpts.Expire, + SameSite: in.cookieOpts.SameSite, + Path: in.cookieOpts.Path, + HTTPOnly: in.cookieOpts.HTTPOnly, + Secure: in.cookieOpts.Secure, + } + cookie := cookiesapi.MakeCookieFromOptions(in.request, cookieOptions) in.request.AddCookie(cookie) err = in.ss().Save(in.response, in.request, in.session)