diff --git a/CHANGELOG.md b/CHANGELOG.md index efd68a01..02728333 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ - [#2734](https://github.com/oauth2-proxy/oauth2-proxy/pull/2734) Added s390x architecture option support (@priby05) - [#2589](https://github.com/oauth2-proxy/oauth2-proxy/pull/2589) Added support for regex path matching and rewriting when using a static `file:` upstream (@ianroberts) - [#2790](https://github.com/oauth2-proxy/oauth2-proxy/pull/2790) chore(deps): update all golang dependencies (@tuunit) +- [#2607](https://github.com/oauth2-proxy/oauth2-proxy/pull/2607) fix(csrf): fix possible infinite loop (@Primexz) # V7.6.0 diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go index 9840b544..284717dd 100644 --- a/pkg/cookies/csrf.go +++ b/pkg/cookies/csrf.go @@ -73,15 +73,23 @@ func NewCSRF(opts *options.Cookie, codeVerifier string) (CSRF, error) { // LoadCSRFCookie loads a CSRF object from a request's CSRF cookie func LoadCSRFCookie(req *http.Request, opts *options.Cookie) (CSRF, error) { - cookieName := GenerateCookieName(req, opts) - cookie, err := req.Cookie(cookieName) - if err != nil { - return nil, err + cookies := req.Cookies() + for _, cookie := range cookies { + if cookie.Name != cookieName { + continue + } + + csrf, err := decodeCSRFCookie(cookie, opts) + if err != nil { + continue + } + + return csrf, nil } - return decodeCSRFCookie(cookie, opts) + return nil, errors.New("CSRF cookie not found") } // GenerateCookieName in case cookie options state that CSRF cookie has fixed name then set fixed name, otherwise diff --git a/pkg/cookies/csrf_test.go b/pkg/cookies/csrf_test.go index b8760d08..09d0d45a 100644 --- a/pkg/cookies/csrf_test.go +++ b/pkg/cookies/csrf_test.go @@ -140,7 +140,7 @@ var _ = Describe("CSRF Cookie Tests", func() { Host: cookieDomain, Path: cookiePath, }, - } + Header: make(http.Header)} }) AfterEach(func() { @@ -168,6 +168,67 @@ var _ = Describe("CSRF Cookie Tests", func() { }) }) + Context("LoadCSRFCookie", func() { + BeforeEach(func() { + // we need to reset the time to ensure the cookie is valid + privateCSRF.time.Reset() + }) + + It("should return error when no cookie is set", func() { + csrf, err := LoadCSRFCookie(req, cookieOpts) + Expect(err).To(HaveOccurred()) + Expect(csrf).To(BeNil()) + }) + + It("should find one valid cookie", func() { + privateCSRF.OAuthState = []byte(csrfState) + privateCSRF.OIDCNonce = []byte(csrfNonce) + encoded, err := privateCSRF.encodeCookie() + Expect(err).ToNot(HaveOccurred()) + + req.AddCookie(&http.Cookie{ + Name: privateCSRF.cookieName(), + Value: encoded, + }) + + csrf, err := LoadCSRFCookie(req, cookieOpts) + Expect(err).ToNot(HaveOccurred()) + Expect(csrf).ToNot(BeNil()) + }) + + It("should return error when one invalid cookie is set", func() { + req.AddCookie(&http.Cookie{ + Name: privateCSRF.cookieName(), + Value: "invalid", + }) + + csrf, err := LoadCSRFCookie(req, cookieOpts) + Expect(err).To(HaveOccurred()) + Expect(csrf).To(BeNil()) + }) + + It("should be able to handle two cookie with one invalid", func() { + privateCSRF.OAuthState = []byte(csrfState) + privateCSRF.OIDCNonce = []byte(csrfNonce) + encoded, err := privateCSRF.encodeCookie() + Expect(err).ToNot(HaveOccurred()) + + req.AddCookie(&http.Cookie{ + Name: privateCSRF.cookieName(), + Value: "invalid", + }) + + req.AddCookie(&http.Cookie{ + Name: privateCSRF.cookieName(), + Value: encoded, + }) + + csrf, err := LoadCSRFCookie(req, cookieOpts) + Expect(err).ToNot(HaveOccurred()) + Expect(csrf).ToNot(BeNil()) + }) + }) + Context("ClearCookie", func() { It("sets a cookie with an empty value in the past", func() { rw := httptest.NewRecorder()