Fix: Session middleware issues (#1407)

* Update session.go

Fix: Session.Regenerate does not set Session.fresh to be true.

* Fix: Session should be regenerated if the session can not be found in the storage
#1408

* Add test for session and store in session middleware.

* Clean up code

* Update middleware/session/session.go

Co-authored-by: hi019 <65871571+hi019@users.noreply.github.com>

middleware/session/session.go

@@ -118,12 +118,21 @@ func (s *Session) Regenerate() error {
 		return err
 	}
 
-	// Create new ID
-	s.id = s.config.KeyGenerator()
+	// Generate a new session, and set session.fresh to true
+	s.refresh()
 
 	return nil
 }
 
+// refresh generates a new session, and set session.fresh to be true
+func (s *Session) refresh() {
+	// Create a new id
+	s.id = s.config.KeyGenerator()
+
+	// We assign a new id to the session, so the session must be fresh
+	s.fresh = true
+}
+
 // Save will update the storage and client cookie
 func (s *Session) Save() error {
 

middleware/session/session_test.go

@@ -61,9 +61,11 @@ func Test_Session(t *testing.T) {
 	keys = sess.Keys()
 	utils.AssertEqual(t, []string{}, keys)
 
-	// get id
-	id := sess.ID()
-	utils.AssertEqual(t, "123", id)
+	// we do not get id here
+	// since the original id is not in the db
+	// sess.id must be a new-generated uuid, which is not equivalent to "123"
+	// id := sess.ID()
+	// utils.AssertEqual(t, "123", id)
 
 	// delete cookie
 	ctx.Request().Header.Del(fiber.HeaderCookie)
@@ -74,8 +76,22 @@ func Test_Session(t *testing.T) {
 	utils.AssertEqual(t, true, sess.Fresh())
 
 	// get id
-	id = sess.ID()
+	id := sess.ID()
 	utils.AssertEqual(t, 36, len(id))
+
+	// when we use the session for the second time
+	// the session be should be same if the session is not expired
+
+	// save the old session first
+	err = sess.Save()
+	utils.AssertEqual(t, nil, err)
+
+	// request the server with the old session
+	ctx.Request().Header.SetCookie(store.sessionName, id)
+	sess, err = store.Get(ctx)
+	utils.AssertEqual(t, nil, err)
+	utils.AssertEqual(t, false, sess.Fresh())
+	utils.AssertEqual(t, sess.id, id)
 }
 
 // go test -run Test_Session_Types
@@ -100,6 +116,10 @@ func Test_Session_Types(t *testing.T) {
 	utils.AssertEqual(t, nil, err)
 	utils.AssertEqual(t, true, sess.Fresh())
 
+	// the session string is no longer be 123
+	newSessionIDString := sess.ID()
+	ctx.Request().Header.SetCookie(store.sessionName, newSessionIDString)
+
 	type User struct {
 		Name string
 	}
@@ -420,6 +440,48 @@ func Test_Session_Deletes_Single_Key(t *testing.T) {
 	utils.AssertEqual(t, nil, sess.Get("id"))
 }
 
+// go test -run Test_Session_Regenerate
+// Regression: https://github.com/gofiber/fiber/issues/1395
+func Test_Session_Regenerate(t *testing.T) {
+	// fiber instance
+	app := fiber.New()
+	t.Run("set fresh to be true when regenerating a session", func(t *testing.T) {
+		// session store
+		store := New()
+		// a random session uuid
+		originalSessionUUIDString := ""
+		// fiber context
+		ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
+		defer app.ReleaseCtx(ctx)
+
+		// now the session is in the storage
+		freshSession, err := store.Get(ctx)
+		utils.AssertEqual(t, nil, err)
+
+		originalSessionUUIDString = freshSession.ID()
+
+		err = freshSession.Save()
+		utils.AssertEqual(t, nil, err)
+
+		// set cookie
+		ctx.Request().Header.SetCookie(store.sessionName, originalSessionUUIDString)
+
+		// as the session is in the storage, session.fresh should be false
+		acquiredSession, err := store.Get(ctx)
+		utils.AssertEqual(t, nil, err)
+		utils.AssertEqual(t, false, acquiredSession.Fresh())
+
+		err = acquiredSession.Regenerate()
+		utils.AssertEqual(t, nil, err)
+
+		if acquiredSession.ID() == originalSessionUUIDString {
+			t.Fatal("regenerate should generate another different id")
+		}
+		// acquiredSession.fresh should be true after regenerating
+		utils.AssertEqual(t, true, acquiredSession.Fresh())
+	})
+}
+
 // go test -v -run=^$ -bench=Benchmark_Session -benchmem -count=4
 func Benchmark_Session(b *testing.B) {
 	app, store := fiber.New(), New()

middleware/session/store.go

@@ -79,7 +79,10 @@ func (s *Store) Get(c *fiber.Ctx) (*Session, error) {
 		} else if err != nil {
 			return nil, err
 		} else {
-			sess.fresh = true
+			// raw is nil, which means id is not in the storage
+			// so it means that id is not valid (mainly because of id is expired or user provides an invalid id)
+			// therefore, we regenerate a id
+			sess.refresh()
 		}
 	}
 

middleware/session/store_test.go

@@ -56,3 +56,27 @@ func TestStore_getSessionID(t *testing.T) {
 		utils.AssertEqual(t, expectedID, store.getSessionID(ctx))
 	})
 }
+
+// go test -run TestStore_Get
+// Regression: https://github.com/gofiber/fiber/issues/1408
+func TestStore_Get(t *testing.T) {
+	unexpectedID := "test-session-id"
+	// fiber instance
+	app := fiber.New()
+	t.Run("regenerate a session when session is invalid", func(t *testing.T) {
+		// session store
+		store := New()
+		// fiber context
+		ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
+		defer app.ReleaseCtx(ctx)
+		// set cookie
+		ctx.Request().Header.SetCookie(store.sessionName, unexpectedID)
+
+		acquiredSession, err := store.Get(ctx)
+		utils.AssertEqual(t, err, nil)
+
+		if acquiredSession.ID() == unexpectedID {
+			t.Fatal("server should not accept the unexpectedID which is not in the store")
+		}
+	})
+}