fix(csrf): possible infinite loop (#2607)

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
 

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)
+	cookies := req.Cookies()
-	if err != nil {
+	for _, cookie := range cookies {
-		return nil, err
+		if cookie.Name != cookieName {
+			continue
 		}
 
-	return decodeCSRFCookie(cookie, opts)
+		csrf, err := decodeCSRFCookie(cookie, opts)
+		if err != nil {
+			continue
+		}
+
+		return csrf, nil
+	}
+
+	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

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()