From 733aa55637adf1c9829fccf2de0d663b4f3edc25 Mon Sep 17 00:00:00 2001 From: Marc Ole Bulling Date: Wed, 5 May 2021 14:55:03 +0200 Subject: [PATCH] Use config mutex for sessions to prevent data race, added note about bug in Go <1.17 regarding data race in html/template --- internal/configuration/Configuration.go | 15 +++---- internal/configuration/Configuration_test.go | 2 +- internal/storage/FileServing.go | 5 ++- internal/webserver/Webserver.go | 2 +- internal/webserver/Webserver_test.go | 3 ++ .../sessionmanager/SessionManager.go | 39 +++++++------------ .../sessionmanager/SessionManager_test.go | 2 +- 7 files changed, 28 insertions(+), 40 deletions(-) diff --git a/internal/configuration/Configuration.go b/internal/configuration/Configuration.go index d62678e..e9044b7 100644 --- a/internal/configuration/Configuration.go +++ b/internal/configuration/Configuration.go @@ -85,7 +85,7 @@ func Lock() { // ReleaseAndSave unlocks and saves the configuration func ReleaseAndSave() { - Save() + save() mutex.Unlock() } @@ -131,7 +131,7 @@ func updateConfig() { if serverSettings.ConfigVersion < currentConfigVersion { fmt.Println("Successfully upgraded database") serverSettings.ConfigVersion = currentConfigVersion - Save() + save() } } @@ -179,11 +179,11 @@ func generateDefaultConfig() { DataDir: Environment.DataDir, LengthId: Environment.LengthId, } - Save() + save() } // Save the configuration as a json file -func Save() { +func save() { file, err := os.OpenFile(Environment.ConfigPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644) if err != nil { fmt.Println("Error reading configuration:", err) @@ -362,7 +362,7 @@ func addTrailingSlash(url string) string { // DisplayPasswordReset shows a password prompt in the CLI and saves the new password func DisplayPasswordReset() { serverSettings.AdminPassword = HashPassword(askForPassword(), false) - Save() + save() } // HashPassword hashes a string with SHA256 and a salt @@ -384,8 +384,3 @@ func HashPassword(password string, useFileSalt bool) string { func GetLengthId() int { return serverSettings.LengthId } - -// GetSessions returns a pointer to the session storage -func GetSessions() *map[string]models.Session { - return &serverSettings.Sessions -} diff --git a/internal/configuration/Configuration_test.go b/internal/configuration/Configuration_test.go index e34e3c8..cf51865 100644 --- a/internal/configuration/Configuration_test.go +++ b/internal/configuration/Configuration_test.go @@ -89,7 +89,7 @@ func TestUpgradeDb(t *testing.T) { test.IsEqualString(t, serverSettings.DataDir, Environment.DataDir) test.IsEqualInt(t, serverSettings.LengthId, 15) test.IsEqualBool(t, serverSettings.Hotlinks == nil, false) - test.IsEqualBool(t, GetSessions() == nil, false) + test.IsEqualBool(t, serverSettings.Sessions == nil, false) test.IsEqualBool(t, serverSettings.DownloadStatus == nil, false) test.IsEqualString(t, serverSettings.Files["MgXJLe4XLfpXcL12ec4i"].ContentType, "application/octet-stream") test.IsEqualInt(t, serverSettings.ConfigVersion, currentConfigVersion) diff --git a/internal/storage/FileServing.go b/internal/storage/FileServing.go index b8c1719..0189d5a 100644 --- a/internal/storage/FileServing.go +++ b/internal/storage/FileServing.go @@ -162,10 +162,11 @@ func CleanUp(periodic bool) { wasItemDeleted = true } } - configuration.Release() if wasItemDeleted { - configuration.Save() + configuration.ReleaseAndSave() CleanUp(false) + } else { + configuration.Release() } if periodic { time.Sleep(time.Hour) diff --git a/internal/webserver/Webserver.go b/internal/webserver/Webserver.go index 7fe6716..9eecd54 100644 --- a/internal/webserver/Webserver.go +++ b/internal/webserver/Webserver.go @@ -154,7 +154,7 @@ func showLogin(w http.ResponseWriter, r *http.Request) { failedLogin := false if pw != "" && user != "" { if strings.ToLower(user) == strings.ToLower(webserverAdminName) && configuration.HashPassword(pw, false) == webserverAdminPassword { - sessionmanager.CreateSession(w, false) + sessionmanager.CreateSession(w, nil) redirect(w, "admin") return } diff --git a/internal/webserver/Webserver_test.go b/internal/webserver/Webserver_test.go index 9219300..fca349c 100644 --- a/internal/webserver/Webserver_test.go +++ b/internal/webserver/Webserver_test.go @@ -12,6 +12,9 @@ import ( "time" ) +// Please note that if this test is run with go test -race, it will fail as there is a bug in html/template that +// causes data race. It will be fixed in Go 1.17, see https://github.com/golang/go/issues/39807 + func TestMain(m *testing.M) { testconfiguration2.Create(true) configuration.Load() diff --git a/internal/webserver/sessionmanager/SessionManager.go b/internal/webserver/sessionmanager/SessionManager.go index 961a2ac..370718e 100644 --- a/internal/webserver/sessionmanager/SessionManager.go +++ b/internal/webserver/sessionmanager/SessionManager.go @@ -9,15 +9,12 @@ import ( "Gokapi/internal/helper" "Gokapi/internal/models" "net/http" - "sync" "time" ) // If no login occurred during this time, the admin session will be deleted. Default 30 days const cookieLifeAdmin = 30 * 24 * time.Hour -var mutex sync.Mutex - // IsValidSession checks if the user is submitting a valid session token // If valid session is found, useSession will be called // Returns true if authenticated, otherwise false @@ -26,49 +23,42 @@ func IsValidSession(w http.ResponseWriter, r *http.Request) bool { if err == nil { sessionString := cookie.Value if sessionString != "" { - mutex.Lock() - sessions := configuration.GetSessions() - defer func() { unlockAndSave() }() - _, ok := (*sessions)[sessionString] + settings := configuration.GetServerSettings() + defer func() { configuration.ReleaseAndSave() }() + _, ok := (settings.Sessions)[sessionString] if ok { - return useSession(w, sessionString) + return useSession(w, sessionString, &settings.Sessions) } } } return false } -func unlockAndSave() { - configuration.Save() - mutex.Unlock() -} - // useSession checks if a session is still valid. It Changes the session string // if it has // been used for more than an hour to limit session hijacking // Returns true if session is still valid // Returns false if session is invalid (and deletes it) -func useSession(w http.ResponseWriter, sessionString string) bool { - sessions := configuration.GetSessions() +func useSession(w http.ResponseWriter, sessionString string, sessions *map[string]models.Session) bool { session := (*sessions)[sessionString] if session.ValidUntil < time.Now().Unix() { delete(*sessions, sessionString) return false } if session.RenewAt < time.Now().Unix() { - CreateSession(w, true) + CreateSession(w, sessions) delete(*sessions, sessionString) } return true } // CreateSession creates a new session - called after login with correct username / password -func CreateSession(w http.ResponseWriter, isLocked bool) { - if !isLocked { - mutex.Lock() - defer func() { unlockAndSave() }() +func CreateSession(w http.ResponseWriter, sessions *map[string]models.Session) { + if sessions == nil { + settings := configuration.GetServerSettings() + sessions = &settings.Sessions + defer func() { configuration.ReleaseAndSave() }() } sessionString := helper.GenerateRandomString(60) - sessions := configuration.GetSessions() (*sessions)[sessionString] = models.Session{ RenewAt: time.Now().Add(time.Hour).Unix(), ValidUntil: time.Now().Add(cookieLifeAdmin).Unix(), @@ -80,10 +70,9 @@ func CreateSession(w http.ResponseWriter, isLocked bool) { func LogoutSession(w http.ResponseWriter, r *http.Request) { cookie, err := r.Cookie("session_token") if err == nil { - mutex.Lock() - sessions := configuration.GetSessions() - delete(*sessions, cookie.Value) - unlockAndSave() + settings := configuration.GetServerSettings() + delete(settings.Sessions, cookie.Value) + configuration.ReleaseAndSave() } writeSessionCookie(w, "", time.Now()) } diff --git a/internal/webserver/sessionmanager/SessionManager_test.go b/internal/webserver/sessionmanager/SessionManager_test.go index 7f5a12e..e81163e 100644 --- a/internal/webserver/sessionmanager/SessionManager_test.go +++ b/internal/webserver/sessionmanager/SessionManager_test.go @@ -64,7 +64,7 @@ func TestIsValidSession(t *testing.T) { func TestCreateSession(t *testing.T) { w, _ := getRecorder(nil) - CreateSession(w, false) + CreateSession(w, nil) cookies := w.Result().Cookies() test.IsEqualInt(t, len(cookies), 1) test.IsEqualString(t, cookies[0].Name, "session_token")