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 3 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
5 changes: 4 additions & 1 deletion middleware/session/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,10 @@ func (s *Session) Regenerate() error {

// Create new ID
s.id = s.config.KeyGenerator()


// We assign a new ID to the session, so the session must be fresh
s.fresh = true

return nil
}

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
4 changes: 4 additions & 0 deletions middleware/session/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,11 @@ func (s *Store) Get(c *fiber.Ctx) (*Session, error) {
} else if err != nil {
return nil, err
} else {
// 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.fresh = true
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
sess.id = s.KeyGenerator()
}
}

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")
}
})
}