From 637d9e0ea6a2dffc2f904d89aac2daafd2dbad51 Mon Sep 17 00:00:00 2001
From: Claudio Costa <cstcld91@gmail.com>
Date: Wed, 11 Dec 2019 16:56:53 +0100
Subject: [PATCH] Add CSRF check for handlers that do not require a session but
 have one (#13354)

---
 web/handlers.go      |   2 +-
 web/handlers_test.go | 105 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/web/handlers.go b/web/handlers.go
index 0d024eacf5..041349a9b5 100644
--- a/web/handlers.go
+++ b/web/handlers.go
@@ -222,7 +222,7 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 // checkCSRFToken performs a CSRF check on the provided request with the given CSRF token. Returns whether or not
 // a CSRF check occurred and whether or not it succeeded.
 func (h *Handler) checkCSRFToken(c *Context, r *http.Request, token string, tokenLocation app.TokenLocation, session *model.Session) (checked bool, passed bool) {
-	csrfCheckNeeded := c.Err == nil && tokenLocation == app.TokenLocationCookie && h.RequireSession && !h.TrustRequester && r.Method != "GET"
+	csrfCheckNeeded := session != nil && c.Err == nil && tokenLocation == app.TokenLocationCookie && !h.TrustRequester && r.Method != "GET"
 	csrfCheckPassed := false
 
 	if csrfCheckNeeded {
diff --git a/web/handlers_test.go b/web/handlers_test.go
index a64a7c6ade..36200bccdf 100644
--- a/web/handlers_test.go
+++ b/web/handlers_test.go
@@ -198,6 +198,44 @@ func TestHandlerServeCSRFToken(t *testing.T) {
 	if response.Code != 401 {
 		t.Errorf("Expected status 200, got %d", response.Code)
 	}
+
+	// Handler with RequireSession set to false
+
+	handlerNoSession := Handler{
+		GetGlobalAppOptions: web.GetGlobalAppOptions,
+		HandleFunc:          handlerForCSRFToken,
+		RequireSession:      false,
+		TrustRequester:      false,
+		RequireMfa:          false,
+		IsStatic:            false,
+	}
+
+	// CSRF Token Used - Success Expected
+
+	request = httptest.NewRequest("POST", "/api/v4/test", nil)
+	request.AddCookie(cookie)
+	request.AddCookie(cookie2)
+	request.AddCookie(cookie3)
+	request.Header.Add(model.HEADER_CSRF_TOKEN, session.GetCSRF())
+	response = httptest.NewRecorder()
+	handlerNoSession.ServeHTTP(response, request)
+
+	if response.Code != 200 {
+		t.Errorf("Expected status 200, got %d", response.Code)
+	}
+
+	// No CSRF Token Used - Failure Expected
+
+	request = httptest.NewRequest("POST", "/api/v4/test", nil)
+	request.AddCookie(cookie)
+	request.AddCookie(cookie2)
+	request.AddCookie(cookie3)
+	response = httptest.NewRecorder()
+	handlerNoSession.ServeHTTP(response, request)
+
+	if response.Code != 401 {
+		t.Errorf("Expected status 401, got %d", response.Code)
+	}
 }
 
 func handlerForCSPHeader(c *Context, w http.ResponseWriter, r *http.Request) {
@@ -481,8 +519,13 @@ func TestCheckCSRFToken(t *testing.T) {
 			App: th.App,
 		}
 		r, _ := http.NewRequest(http.MethodGet, "", nil)
+		session := &model.Session{
+			Props: map[string]string{
+				"csrf": token,
+			},
+		}
 
-		checked, passed := h.checkCSRFToken(c, r, token, tokenLocation, nil)
+		checked, passed := h.checkCSRFToken(c, r, token, tokenLocation, session)
 
 		assert.False(t, checked)
 		assert.False(t, passed)
@@ -505,6 +548,36 @@ func TestCheckCSRFToken(t *testing.T) {
 			App: th.App,
 		}
 		r, _ := http.NewRequest(http.MethodPost, "", nil)
+		session := &model.Session{
+			Props: map[string]string{
+				"csrf": token,
+			},
+		}
+
+		checked, passed := h.checkCSRFToken(c, r, token, tokenLocation, session)
+
+		assert.False(t, checked)
+		assert.False(t, passed)
+		assert.Nil(t, c.Err)
+	})
+
+	t.Run("should not check a request passing a nil session", func(t *testing.T) {
+		th := Setup()
+		defer th.TearDown()
+
+		h := &Handler{
+			RequireSession: false,
+			TrustRequester: false,
+		}
+
+		token := "token"
+		tokenLocation := app.TokenLocationCookie
+
+		c := &Context{
+			App: th.App,
+		}
+		r, _ := http.NewRequest(http.MethodPost, "", nil)
+		r.Header.Set(model.HEADER_CSRF_TOKEN, token)
 
 		checked, passed := h.checkCSRFToken(c, r, token, tokenLocation, nil)
 
@@ -512,4 +585,34 @@ func TestCheckCSRFToken(t *testing.T) {
 		assert.False(t, passed)
 		assert.Nil(t, c.Err)
 	})
+
+	t.Run("should check requests for handlers that don't require a session but have one", func(t *testing.T) {
+		th := Setup()
+		defer th.TearDown()
+
+		h := &Handler{
+			RequireSession: false,
+			TrustRequester: false,
+		}
+
+		token := "token"
+		tokenLocation := app.TokenLocationCookie
+
+		c := &Context{
+			App: th.App,
+		}
+		r, _ := http.NewRequest(http.MethodPost, "", nil)
+		r.Header.Set(model.HEADER_CSRF_TOKEN, token)
+		session := &model.Session{
+			Props: map[string]string{
+				"csrf": token,
+			},
+		}
+
+		checked, passed := h.checkCSRFToken(c, r, token, tokenLocation, session)
+
+		assert.True(t, checked)
+		assert.True(t, passed)
+		assert.Nil(t, c.Err)
+	})
 }