fix: session create with expired timestamp #248
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors session handling to fix issues with expired timestamps by introducing a new ForceSetSession function. The change allows for two modes of session recovery: automatic recovery when detecting corrupted sessions, and forced recreation of sessions regardless of their state.
Key changes:
- Introduces
ForceSetSessionwith aforceparameter to control session creation behavior - Refactors
SetSessionto delegate toForceSetSessionwithforce=false - Reorganizes imports to follow Go conventions (standard library before third-party imports)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func SetSession(w http.ResponseWriter, r *http.Request, key string, value interface{}) bool { | ||
| return ForceSetSession(w, r, key, value, false) | ||
| } | ||
|
|
There was a problem hiding this comment.
The new public function ForceSetSession lacks documentation explaining its purpose, parameters, and when the force parameter should be used. Public API functions should have godoc comments describing their behavior.
| // ForceSetSession stores the given key/value pair in the current user's session and saves it. | |
| // When force is false, the function attempts to load the existing session identified by the | |
| // node-specific session name and will create a new one only if the stored value is corrupted. | |
| // When force is true, the function always creates a new session instead of loading an existing | |
| // one, which can be useful when the caller wants to reset the session state explicitly. | |
| // It returns true if the session was saved successfully, or false if any error occurred. |
| if strings.Contains(err.Error(), "the value is not valid") { | ||
| log.Warnf("Session corrupted in SetSession, creating new one: %v", err) | ||
| session, err = s.New(r, getSessionName()) | ||
| if err != nil { | ||
| log.Warnf("Failed to create new session in SetSession: %v", err) | ||
| return false | ||
| } |
There was a problem hiding this comment.
When force is false and a corrupted session is detected (lines 119-125), the old session cookie is not cleared before creating a new session. This is inconsistent with the force=true path (lines 133-140) and with GetSession (lines 77-84) which both clear the corrupted cookie. This could lead to issues where the old corrupted cookie persists.
| func SetSession(w http.ResponseWriter, r *http.Request, key string, value interface{}) bool { | ||
| return ForceSetSession(w, r, key, value, false) | ||
| } | ||
|
|
||
| func ForceSetSession(w http.ResponseWriter, r *http.Request, key string, value interface{}, force bool) bool { | ||
| s := getStore() | ||
| session, err := s.Get(r, getSessionName()) | ||
| var ( | ||
| session *sessions.Session | ||
| err error | ||
| ) | ||
| if !force { | ||
| session, err = s.Get(r, getSessionName()) | ||
|
|
||
| if err != nil { | ||
| if strings.Contains(err.Error(), "the value is not valid") { | ||
| log.Warnf("Session corrupted in SetSession, creating new one: %v", err) | ||
| session, err = s.New(r, getSessionName()) | ||
| if err != nil { | ||
| log.Warnf("Failed to create new session in SetSession: %v", err) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "the value is not valid") { | ||
| log.Warnf("Session corrupted in SetSession, creating new one: %v", err) | ||
| session, err = s.New(r, getSessionName()) | ||
| if err != nil { | ||
| log.Warnf("Failed to create new session in SetSession: %v", err) | ||
| return false | ||
| } | ||
| } else { | ||
| log.Warnf("Failed to get session in SetSession: %v", err) | ||
| return false | ||
| } | ||
| } else { | ||
| log.Warnf("Failed to get session in SetSession: %v", err) | ||
| } | ||
| } else { | ||
| // Destroy the corrupted session completely | ||
| if cookie, err := r.Cookie(getSessionName()); err == nil { | ||
| http.SetCookie(w, &http.Cookie{ | ||
| Name: cookie.Name, | ||
| Value: "", | ||
| Path: "/", | ||
| MaxAge: -1, | ||
| }) | ||
| } | ||
| session, err = s.New(r, getSessionName()) | ||
| if err != nil { | ||
| log.Warnf("Failed to create new session in ForceSetSession: %v", err) | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
The new ForceSetSession function and the refactored SetSession logic lack test coverage. Given that tests exist in core/api/api_test.go for other API functionality, this new session handling behavior (especially the force parameter and corrupted session recovery) should be tested to ensure it works correctly.
What does this PR do
Rationale for this change
Standards checklist