🐛 Session middleware does not use KeyLookup for session storage

[natebwangsut]

Fiber version
2.14 and above

Issue description
Session middleware is unable to lookup via given key

Code snippet

package main

import (
	"net/http"

	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/middleware/session"
)

func main() {
	store := session.New(session.Config{
		KeyLookup: "header:session",
	})
	app := fiber.New()

	app.Get("/token", func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			panic(err)
		}
		original := sess.Get("token")
		token := c.Query("token")
		if original == nil {
			sess.Set("token", token)
			if err := sess.Save(); err != nil {
				return c.SendStatus(http.StatusInternalServerError)
			}
			return c.SendStatus(http.StatusAccepted) // <- however this is instead return every time
		}
		return c.JSON(map[string]string{"original": original.(string), "new": token}) 	// <- this should return if session exists
	})

	app.Listen(":8080")
}

Reproducing

curl --request GET --header 'session: session' --url 'http://localhost:8080/token?token=next'
curl --request GET --header 'session: session' --url 'http://localhost:8080/token'

---

Responses on 2.13

HTTP 202
Accept

HTTP 200
{
    "new": "",
    "original": "next"
}

---

Responses on 2.14++

HTTP 202
Accept

HTTP 202
Accept

[welcome]

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

[natebwangsut]

The problem occurs from this line:

fiber/middleware/session/store.go

Line 85 in ff57431

sess.refresh()

When the we regenerating, we're giving session a new ID and thus subsequence session.Save() does not save to the same key that originally it meant to looked up to.

---

For example...

HTTP flow with key = "token"

sess := store.Get(c)

eventho we wanted to lookup from key

fiber/middleware/session/store.go

Line 43 in ff57431

id := s.getSessionID(c)

fiber/middleware/session/store.go

Line 68 in ff57431

raw, err := s.Storage.Get(id)

Storage will always return nil since the prior request with sess.Save() of the same key was put into randomly generated UUID

this potentially causes leakage as well due to unique UUID being generated and store all the time

[natebwangsut]

I can see that there's a PR merged to fix the issue

#1407
#1408

---

However the issue defined on #1408 was more of an application error rather than framework error.

I think #1408 highlighten the fact that how do I differentiate when

• there's mismatch on lookup or
• when the lookup is okay but raw data is nil

Since s.Storage.Get(id) returned nil, nil on both cases. (I've checked implementation on memory and redis storage abstraction, didn't checked the other)

[Spedoske]

I am wondering which is a more proper solution for the circumstance when we can not find a session in the store.

• regenerating the session_id, ignoring the session_id that is not found in the storage
  • pros: The session_id will follow the format of KeyGenerator.
  • cons: we will need to set the session in the user's browser, especially when we use KeyLookup since we should manually tell the user-side script to set the header with the new session_id.
• keep using the session_id that is not found in the storage
  • pros: we will no longer need to reset the session in the user's browser
  • cons: safety concerns. Users can manipulate the format of session_id. For example, users can use random strings instead of UUID, which is set in the config(KeyGenerator).

How does https://github.com/expressjs/session do?

• They use some way to check whether the session_id is signed by the server. If the session_id is invalid, they just ignore the session_id. (https://github.com/expressjs/session/blob/master/index.js#L217)
• If session_id is valid but there is no data corresponding to the session_id. They regenerate the session.

(https://github.com/expressjs/session/blob/master/index.js#L495)
However, expressjs-session can only set sessions in the cookie, which is a big limitation.
Maybe we can add a field in the Config of the middleware to specify one of these two strategies.