From d8deaa124b776656f1075eb966357abe853a6a15 Mon Sep 17 00:00:00 2001
From: Joel  Speed <joel.speed@hotmail.co.uk>
Date: Wed, 13 Oct 2021 18:48:09 +0100
Subject: [PATCH] Improve error message when no cookie is found

---
 CHANGELOG.md                              |  1 +
 oauthproxy.go                             |  2 ++
 pkg/middleware/stored_session.go          |  2 +-
 pkg/sessions/cookie/session_store.go      |  4 +--
 pkg/sessions/tests/session_store_tests.go | 39 ++++++++++++++++-------
 5 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index fab445b5..d36a0dbc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,7 @@
 
 ## Changes since v7.1.3
 
+- [#1404](https://github.com/oauth2-proxy/oauth2-proxy/pull/1404) Improve error message when no cookie is found (@JoelSpeed)
 - [#1315](https://github.com/oauth2-proxy/oauth2-proxy/pull/1315) linkedin: Update provider to v2 (@wuurrd)
 - [#1348](https://github.com/oauth2-proxy/oauth2-proxy/pull/1348) Using the native httputil proxy code for websockets rather than yhat/wsutil to properly handle HTTP-level failures (@thetrime)
 - [#1379](https://github.com/oauth2-proxy/oauth2-proxy/pull/1379) Fix the manual sign in with --htpasswd-user-group switch (@janrotter)
diff --git a/oauthproxy.go b/oauthproxy.go
index 08e6246a..503985e8 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -853,11 +853,13 @@ func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) {
 	case ErrNeedsLogin:
 		// we need to send the user to a login screen
 		if p.forceJSONErrors || isAjax(req) {
+			logger.Printf("No valid authentication in request. Access Denied.")
 			// no point redirecting an AJAX request
 			p.errorJSON(rw, http.StatusUnauthorized)
 			return
 		}
 
+		logger.Printf("No valid authentication in request. Initiating login.")
 		if p.SkipProviderButton {
 			p.OAuthStart(rw, req)
 		} else {
diff --git a/pkg/middleware/stored_session.go b/pkg/middleware/stored_session.go
index 6748816f..4cbc47eb 100644
--- a/pkg/middleware/stored_session.go
+++ b/pkg/middleware/stored_session.go
@@ -71,7 +71,7 @@ func (s *storedSessionLoader) loadSession(next http.Handler) http.Handler {
 		}
 
 		session, err := s.getValidatedSession(rw, req)
-		if err != nil {
+		if err != nil && !errors.Is(err, http.ErrNoCookie) {
 			// In the case when there was an error loading the session,
 			// we should clear the session
 			logger.Errorf("Error loading cookied session: %v, removing session", err)
diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go
index 1b3c12de..035ae1f0 100644
--- a/pkg/sessions/cookie/session_store.go
+++ b/pkg/sessions/cookie/session_store.go
@@ -51,7 +51,7 @@ func (s *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) {
 	c, err := loadCookie(req, s.Cookie.Name)
 	if err != nil {
 		// always http.ErrNoCookie
-		return nil, fmt.Errorf("cookie %q not present", s.Cookie.Name)
+		return nil, err
 	}
 	val, _, ok := encryption.Validate(c, s.Cookie.Secret, s.Cookie.Expire)
 	if !ok {
@@ -216,7 +216,7 @@ func loadCookie(req *http.Request, cookieName string) (*http.Cookie, error) {
 		}
 	}
 	if len(cookies) == 0 {
-		return nil, fmt.Errorf("could not find cookie %s", cookieName)
+		return nil, http.ErrNoCookie
 	}
 	return joinCookies(cookies, cookieName)
 }
diff --git a/pkg/sessions/tests/session_store_tests.go b/pkg/sessions/tests/session_store_tests.go
index 416969a3..80debebb 100644
--- a/pkg/sessions/tests/session_store_tests.go
+++ b/pkg/sessions/tests/session_store_tests.go
@@ -452,21 +452,38 @@ func SessionStoreInterfaceTests(in *testInput) {
 	})
 
 	Context("when Load is called", func() {
-		BeforeEach(func() {
-			req := httptest.NewRequest("GET", "http://example.com/", nil)
-			resp := httptest.NewRecorder()
-			err := in.ss().Save(resp, req, in.session)
-			Expect(err).ToNot(HaveOccurred())
+		Context("with a valid session cookie in the request", func() {
+			BeforeEach(func() {
+				req := httptest.NewRequest("GET", "http://example.com/", nil)
+				resp := httptest.NewRecorder()
+				err := in.ss().Save(resp, req, in.session)
+				Expect(err).ToNot(HaveOccurred())
+				for _, cookie := range resp.Result().Cookies() {
+					in.request.AddCookie(cookie)
+				}
+			})
 
-			for _, cookie := range resp.Result().Cookies() {
-				in.request.AddCookie(cookie)
-			}
+			Context("before the refresh period", func() {
+				LoadSessionTests(in)
+			})
 		})
 
-		Context("before the refresh period", func() {
-			LoadSessionTests(in)
-		})
+		Context("with no cookies in the request", func() {
+			var loadedSession *sessionsapi.SessionState
+			var loadErr error
 
+			BeforeEach(func() {
+				loadedSession, loadErr = in.ss().Load(in.request)
+			})
+
+			It("returns an empty session", func() {
+				Expect(loadedSession).To(BeNil())
+			})
+
+			It("should return a no cookie error", func() {
+				Expect(loadErr).To(MatchError(http.ErrNoCookie))
+			})
+		})
 	})
 }