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 middleware/session update cookie. #1960

Merged
merged 1 commit into from Jul 2, 2022
Merged

Conversation

ly020044
Copy link
Contributor

@ly020044 ly020044 commented Jul 1, 2022

Session middleware should always update client cookie so that cookie expires at the same time as session.

@welcome
Copy link

welcome bot commented Jul 1, 2022

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87
Copy link
Member

ReneWerner87 commented Jul 1, 2022

should we refresh the expire time every time ? if you remove the fresh switch in the condition why is s.fresh still needed(think we are destroying the reason for introducing the property) ? would have expected some unittests outlining the problem or showing that your fix works -> otherwise it is not guaranteed that it is fixed forever

@iredmail
Copy link
Contributor

iredmail commented Jul 2, 2022

should we refresh the expire time every time ?

Yes.

Let's say session created at 2:00PM and expire time is set to 2 hours, each time you request a web page, the session expiration time is updated to stay 2 hours. If you request a page at 2:30PM, session expire time will be (correctly) updated to 4:30PM, but cookie still has expiration time at 4PM. In this case when request at any time after 4PM, the web session is invalid because session ID stored in client cookie expired and server regenerates one.

I checked other session implementations like Gin's session middleware, they all update cookie expiration time. Sample code (Please click the Set Count link and check cookie in web browser):

package main

import (
	"fmt"
	"html/template"
	"net/http"

	"github.com/gin-contrib/sessions"
	"github.com/gin-contrib/sessions/memstore"
	"github.com/gin-gonic/gin"
)

func main() {
	router := gin.Default()
	store := memstore.NewStore([]byte("niuniuniu"))
	router.Use(sessions.Sessions("gin_session", store))

	tmpl := "<h1>Hello World!</h1> <h3>count: {{.Count}}</h3> <h3><a href=\"/s\">Set Count</a></h3>"
	t, err := template.New("test").Parse(tmpl)
	if err != nil {
		panic(err)
	}
	router.SetHTMLTemplate(t)

	router.GET("/", func(ctx *gin.Context) {
		session := sessions.Default(ctx)
		ctx.HTML(http.StatusOK, "test", gin.H{
			"Count": session.Get("count"),
		})
	})

	router.GET("/s", func(ctx *gin.Context) {
		session := sessions.Default(ctx)
		var count int
		v := session.Get("count")
		if v == nil {
			count = 0
		} else {
			count = v.(int)
			count++
		}
		session.Set("count", count)
		if err := session.Save(); err != nil {
			fmt.Println(err)
		}

		ctx.Redirect(http.StatusFound, "/")
	})

	router.Run(":8080")
}

@ReneWerner87
Copy link
Member

ReneWerner87 commented Jul 2, 2022

Ok, thanks

@iredmail
Copy link
Contributor

iredmail commented Jul 2, 2022

Is it ok to merge this PR now? @ReneWerner87

@ReneWerner87 ReneWerner87 merged commit 41d31a0 into gofiber:master Jul 2, 2022
21 checks passed
@welcome
Copy link

welcome bot commented Jul 2, 2022

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants