Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Session middleware issues #1407

Merged
merged 5 commits into from
Jun 30, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions middleware/session/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 be true
hi019 marked this conversation as resolved.
Show resolved Hide resolved
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 {

Expand Down
70 changes: 66 additions & 4 deletions middleware/session/session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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()
Expand Down
5 changes: 4 additions & 1 deletion middleware/session/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}

Expand Down
24 changes: 24 additions & 0 deletions middleware/session/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
})
}