[MM-10346] CSRF Token Implementation + Tests (#10067)

* CSRF Token Implementation + Tests Remove debug statements Implement requested changes * Fix non-cookie authentication methods stripping auth data from requests * Fail when CSRF cookie is not returned as part of login
2025-02-25 18:55:24 -06:00 · 2019-01-31 20:39:02 +01:00
parent 86aa01cf36
commit 7cc66ee1d4
10 changed files with 202 additions and 36 deletions
--- a/web/handlers.go
+++ b/web/handlers.go
@@ -62,16 +62,6 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	c.App.Path = r.URL.Path
 	c.Log = c.App.Log

-	token, tokenLocation := app.ParseAuthTokenFromRequest(r)
-
-	// CSRF Check
-	if tokenLocation == app.TokenLocationCookie && h.RequireSession && !h.TrustRequester {
-		if r.Header.Get(model.HEADER_REQUESTED_WITH) != model.HEADER_REQUESTED_WITH_XML {
-			c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token+" Appears to be a CSRF attempt", http.StatusUnauthorized)
-			token = ""
-		}
-	}
-
 	subpath, _ := utils.GetSubpathFromConfig(c.App.Config())
 	siteURLHeader := app.GetProtocol(r) + "://" + r.Host + subpath
 	c.SetSiteURLHeader(siteURLHeader)
@@ -97,26 +87,51 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 	}

+	token, tokenLocation := app.ParseAuthTokenFromRequest(r)
+
 	if len(token) != 0 {
 		session, err := c.App.GetSession(token)
+		csrfCheckPassed := false

-		if err != nil {
-			c.Log.Info("Invalid session", mlog.Err(err))
-			if err.StatusCode == http.StatusInternalServerError {
-				c.Err = err
-			} else if h.RequireSession {
-				c.RemoveSessionCookie(w, r)
-				c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token, http.StatusUnauthorized)
+		// CSRF Check
+		if tokenLocation == app.TokenLocationCookie && h.RequireSession && !h.TrustRequester && r.Method != "GET" {
+			csrfHeader := r.Header.Get(model.HEADER_CSRF_TOKEN)
+			if csrfHeader == session.GetCSRF() {
+				csrfCheckPassed = true
+			} else if !*c.App.Config().ServiceSettings.ExperimentalStrictCSRFEnforcement && r.Header.Get(model.HEADER_REQUESTED_WITH) == model.HEADER_REQUESTED_WITH_XML {
+				// ToDo(DSchalla) 2019/01/04: Remove after deprecation period and only allow CSRF Header (MM-13657)
+				c.Log.Warn("CSRF Header check failed for request - Please upgrade your web application or custom app to set a CSRF Header")
+				csrfCheckPassed = true
+			}
+
+			if !csrfCheckPassed {
+				token = ""
+				session = nil
+				c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token+" Appears to be a CSRF attempt", http.StatusUnauthorized)
 			}
-		} else if !session.IsOAuth && tokenLocation == app.TokenLocationQueryString {
-			c.Err = model.NewAppError("ServeHTTP", "api.context.token_provided.app_error", nil, "token="+token, http.StatusUnauthorized)
 		} else {
-			c.App.Session = *session
+			csrfCheckPassed = true
 		}

-		// Rate limit by UserID
-		if c.App.Srv.RateLimiter != nil && c.App.Srv.RateLimiter.UserIdRateLimit(c.App.Session.UserId, w) {
-			return
+		if csrfCheckPassed {
+			if err != nil {
+				c.Log.Info("Invalid session", mlog.Err(err))
+				if err.StatusCode == http.StatusInternalServerError {
+					c.Err = err
+				} else if h.RequireSession {
+					c.RemoveSessionCookie(w, r)
+					c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token, http.StatusUnauthorized)
+				}
+			} else if !session.IsOAuth && tokenLocation == app.TokenLocationQueryString {
+				c.Err = model.NewAppError("ServeHTTP", "api.context.token_provided.app_error", nil, "token="+token, http.StatusUnauthorized)
+			} else {
+				c.App.Session = *session
+			}
+
+			// Rate limit by UserID
+			if c.App.Srv.RateLimiter != nil && c.App.Srv.RateLimiter.UserIdRateLimit(c.App.Session.UserId, w) {
+				return
+			}
 		}
 	}

--- a/web/handlers_test.go
+++ b/web/handlers_test.go
@@ -108,3 +108,105 @@ func TestHandlerServeHTTPSecureTransport(t *testing.T) {
 		t.Errorf("Strict-Transport-Security header is not expected, but returned")
 	}
 }
+
+
+func handlerForCSRFToken(c *Context, w http.ResponseWriter, r *http.Request) {
+}
+
+func TestHandlerServeCSRFToken(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	session :=&model.Session{
+		UserId: th.BasicUser.Id,
+		CreateAt: model.GetMillis(),
+		Roles: model.SYSTEM_USER_ROLE_ID,
+		IsOAuth: false,
+	}
+	session.GenerateCSRF()
+	session.SetExpireInDays(1)
+	session, err := th.App.CreateSession(session)
+	if err != nil {
+		t.Errorf("Expected nil, got %s", err)
+	}
+
+	web := New(th.Server, th.Server.AppOptions, th.Server.Router)
+
+	handler := Handler{
+		GetGlobalAppOptions: web.GetGlobalAppOptions,
+		HandleFunc:          handlerForCSRFToken,
+		RequireSession:      true,
+		TrustRequester:      false,
+		RequireMfa:          false,
+		IsStatic:            false,
+	}
+
+	cookie := &http.Cookie{
+		Name: model.SESSION_COOKIE_USER,
+		Value: th.BasicUser.Username,
+	}
+	cookie2 := &http.Cookie{
+		Name: model.SESSION_COOKIE_TOKEN,
+		Value: session.Token,
+	}
+	cookie3 := &http.Cookie{
+		Name: model.SESSION_COOKIE_CSRF,
+		Value: session.GetCSRF(),
+	}
+
+	// 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()
+	handler.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()
+	handler.ServeHTTP(response, request)
+
+	if response.Code != 401 {
+		t.Errorf("Expected status 401, got %d", response.Code)
+	}
+
+	// Fallback Behavior Used - Success expected
+	// ToDo (DSchalla) 2019/01/04: Remove once legacy CSRF Handling is removed
+	th.App.UpdateConfig(func(config *model.Config){
+		*config.ServiceSettings.ExperimentalStrictCSRFEnforcement = false
+	})
+	request = httptest.NewRequest("POST", "/api/v4/test", nil)
+	request.AddCookie(cookie)
+	request.AddCookie(cookie2)
+	request.AddCookie(cookie3)
+	request.Header.Add(model.HEADER_REQUESTED_WITH, model.HEADER_REQUESTED_WITH_XML)
+	response = httptest.NewRecorder()
+	handler.ServeHTTP(response, request)
+
+	if response.Code != 200 {
+		t.Errorf("Expected status 200, got %d", response.Code)
+	}
+
+	// Fallback Behavior Used with Strict Enforcement - Failure Expected
+	// ToDo (DSchalla) 2019/01/04: Remove once legacy CSRF Handling is removed
+	th.App.UpdateConfig(func(config *model.Config){
+		*config.ServiceSettings.ExperimentalStrictCSRFEnforcement = true
+	})
+	response = httptest.NewRecorder()
+	handler.ServeHTTP(response, request)
+
+	if response.Code != 401 {
+		t.Errorf("Expected status 200, got %d", response.Code)
+	}
+}