Option to set a session cookie
- Dominant language
- Go
- Stars
- 3k
- Forks
- 387
- Avg merge
- 4h 16m
- Merged PRs (30d)
- 1
Description
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](https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#define_the_lifetime_of_a_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.
https://github.com/appleboy/gin-jwt/blob/22d2e6198bd396d324936a5d990b327a792bffe8/auth_jwt.go#L507-L509
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:1**6** 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:1**7** 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](https://pkg.go.dev/net/http#Cookie) 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](https://datatracker.ietf.org/doc/html/rfc6265#section-5.2.2), 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!
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in auth_jwt.go around the cookie-generation code at lines 507-509 and trace how CookieMaxAge, Timeout, and TimeFunc affect the emitted cookie. Check the README for existing middleware configuration documentation. Done means the requested session-cookie behavior and time calculation are explicitly defined, implemented, and covered by regression tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- authentication
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100