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

Option to set a session cookie #289

Open
k0mmsussert0d opened this issue Apr 25, 2022 · 0 comments
Open

Option to set a session cookie #289

k0mmsussert0d opened this issue Apr 25, 2022 · 0 comments

Comments

@k0mmsussert0d
Copy link

Hi!
While using this code in my project I noticed there is no explicit option to omit both Max-Age and Expires parameters in a cookie handed-out when SendCookie parameter is set to true making it a session cookie. After looking through the code briefly I found CookieMaxAge parameter inside GinJWTMiddleware struct, unmentioned in the README. At the first glance, setting it to "0" might seem to be a good idea for this purpose. However, the way the flag is used during cookie generation concerns be a bit.

gin-jwt/auth_jwt.go

Lines 507 to 509 in 22d2e61

if mw.SendCookie {
expireCookie := mw.TimeFunc().Add(mw.CookieMaxAge)
maxage := int(expireCookie.Unix() - mw.TimeFunc().Unix())

Let's assume default mw.TimeFunc() implementation, which is time.Now. Max-Age attribute is calculated as duration difference between (time.Now() + CookieMaxAge) and (time.Now()) to the nearest second. Evaluating deductible and deductive is based on two separate calls to mw.TimeFunc() later converted to Unix timestamp. This might cause an inconsistency if system clock flips by one second between those two calls.

Step-by-step scenario:

  • Current time is Mon, 25 Apr 2022 15:14:16 GMT expressed as 1650899656 with epoch timestamp
mw.CookieMaxAge = "0"
expireCookie := mw.TimeFunc().Add(mw.CookieMaxAge)
  • mw.TimeFunc() returns 1650899656, adding 0 to it makes expireCookie := 1650899656

  • Time advances by one second now, it's Mon, 25 Apr 2022 15:14:17 GMT or 1650899657 expressed with epoch timestamp

maxage := int(expireCookie.Unix() - mw.TimeFunc().Unix()) 
  • mw.TimeFunc() returns 1650899657, maxage is a result of 1650899656 - 1650899657, which is -1.

According to net/http package documentation passing a negative value as MaxAge parameter for http.Cookie struct effectively results in a cookie with Max-Age: 0 attribute. This causes the cookie to be considered as expired [at] the earliest representable date and time, which is significantly different behavior compared to what's achieved with the lack of Expires and Max-Age attributes.

Excerpt from net/http documentation:

// MaxAge=0 means no 'Max-Age' attribute specified.
// MaxAge<0 means delete cookie now, equivalently 'Max-Age: 0'
// MaxAge>0 means Max-Age attribute present and given in seconds

Supposing time did not change between mw.TimeFunc calls, this would yield maxage := 0, which is correct MaxAge value to pass to http.Cookie to make the Max-Age attribute unspecified in the output cookie.

I propose to improve the cookie expiration timeout evaluation method by making sure mw.TimeFunc is called only once. On top of that, I'd see a separate config flag like SessionCookie boolean overriding CookieMaxAge and Timeout as a huge convenience for this kind of use case.

Could you please share your thoughts on my proposal? I'm wiling to implement it if you decide you'd like it in the project.

Thanks!

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

No branches or pull requests

1 participant