From 41fe33bbb1862d8bffaecb028c6ed6573c05de2a Mon Sep 17 00:00:00 2001
From: Daniel Schalla <daniel@schalla.me>
Date: Thu, 4 Apr 2019 18:24:40 +0200
Subject: [PATCH] Avoid panic from reading CSRF of nil session pointer (#10554)

* Avoid panic from reading CSRF of nil session pointer

* Reorganize CSRF Handling

* Remove autoimport added by IDE

* Remove unnecessary nil check

* gofmt
---
 web/handlers.go | 46 +++++++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/web/handlers.go b/web/handlers.go
index 60aaef64f7..70ac08e8d2 100644
--- a/web/handlers.go
+++ b/web/handlers.go
@@ -102,10 +102,29 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	if len(token) != 0 {
 		session, err := c.App.GetSession(token)
+		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
+		}
+
 		csrfCheckPassed := false
 
 		// CSRF Check
-		if tokenLocation == app.TokenLocationCookie && h.RequireSession && !h.TrustRequester && r.Method != "GET" {
+		if c.Err == nil && tokenLocation == app.TokenLocationCookie && h.RequireSession && !h.TrustRequester && r.Method != "GET" {
 			csrfHeader := r.Header.Get(model.HEADER_CSRF_TOKEN)
 			if csrfHeader == session.GetCSRF() {
 				csrfCheckPassed = true
@@ -122,32 +141,9 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 			if !csrfCheckPassed {
 				token = ""
-				session = nil
+				c.App.Session = model.Session{}
 				c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token+" Appears to be a CSRF attempt", http.StatusUnauthorized)
 			}
-		} else {
-			csrfCheckPassed = true
-		}
-
-		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
-			}
 		}
 	}