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

feat!(middleware/session): re-write session middleware with handler #3016

Merged
merged 93 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
93 commits
Select commit Hold shift + click to select a range
6e76847
feat!(middleware/session): re-write session middleware with handler
sixcolors May 28, 2024
ac9a028
test(middleware/session): refactor to IdleTimeout
sixcolors May 28, 2024
81f6789
fix: lint errors
sixcolors May 28, 2024
28790cb
test: Save session after setting or deleting raw data in CSRF middleware
sixcolors May 28, 2024
7ffae3d
Update middleware/session/middleware.go
sixcolors May 30, 2024
68f2739
fix: mutex and globals order
sixcolors May 30, 2024
92e6877
feat: Re-Add read lock to session Get method
sixcolors Jun 4, 2024
239db00
feat: Migrate New() to return middleware
sixcolors Jun 15, 2024
0b93c5c
chore: Refactor session middleware to improve session handling
sixcolors Jun 15, 2024
7cb4a6e
chore: Private get on store
sixcolors Jun 15, 2024
b4c8ea8
chore: Update session middleware to use saveSession instead of save
sixcolors Jun 15, 2024
aafee92
chore: Update session middleware to use getSession instead of get
sixcolors Jun 15, 2024
cd91db4
chore: Remove unused error handler in session middleware config
sixcolors Jun 15, 2024
c3b303f
chore: Update session middleware to use NewWithStore in CSRF tests
sixcolors Jun 15, 2024
75cffca
Merge branch 'main' into 2741-session-changes
sixcolors Jun 15, 2024
2731428
test: add test
sixcolors Jun 15, 2024
4f04291
Merge branch '2741-session-changes' of https://github.com/sixcolors/f…
sixcolors Jun 15, 2024
ee193dc
fix: destroyed session and GHSA-98j2-3j3p-fw2v
sixcolors Jun 22, 2024
01571cb
Merge remote-tracking branch 'origin/main' into 2741-session-changes
sixcolors Jul 29, 2024
5f032d4
Merge branch 'main' into 2741-session-changes
sixcolors Jul 29, 2024
1a5a3d7
chore: Refactor session_test.go to use newStore() instead of New()
sixcolors Jul 29, 2024
52e41a4
feat: Improve session middleware test coverage and error handling
sixcolors Jul 29, 2024
ed95d83
chore: fix lint issues
sixcolors Jul 29, 2024
c6e1c34
chore: Fix session middleware locking issue and improve error handling
sixcolors Jul 29, 2024
8a5663a
test: improve middleware test coverage and error handling
sixcolors Aug 3, 2024
46845e6
test: Add idle timeout test case to session middleware test
sixcolors Aug 3, 2024
ba0e491
feat: add GetSession(id string) (*Session, error)
sixcolors Aug 10, 2024
d08b686
chore: lint
sixcolors Aug 10, 2024
8df7c81
Merge branch 'main' into pr/3016
sixcolors Aug 14, 2024
508cf24
Merge branch 'main' into pr/3016
sixcolors Aug 27, 2024
6ee953b
Merge branch 'main' into pr/3016
sixcolors Aug 28, 2024
355b8f5
Merge branch 'main' into pr/3016
sixcolors Sep 2, 2024
2f3f724
Merge branch 'main' into 2741-session-changes
sixcolors Sep 3, 2024
c08ddc1
docs: Update session middleware docs
sixcolors Sep 8, 2024
56f6ce0
docs: Security Note to examples
sixcolors Sep 8, 2024
9e406f4
docs: Add recommendation for CSRF protection in session middleware
sixcolors Sep 8, 2024
12b219a
chore: markdown lint
sixcolors Sep 8, 2024
6812fc4
docs: Update session middleware docs
sixcolors Sep 8, 2024
28aad65
docs: makrdown lint
sixcolors Sep 8, 2024
d4e607e
Merge branch 'main' into 2741-session-changes
sixcolors Sep 13, 2024
14c7a6c
test(middleware/session): Add unit tests for session config.go
sixcolors Sep 13, 2024
a865ba5
test(middleware/session): Add unit tests for store.go
sixcolors Sep 13, 2024
eaedc6d
test(middleware/session): Add data.go unit tests
sixcolors Sep 13, 2024
d2cf5b8
refactor(middleware/session): session tests and add session release test
sixcolors Sep 13, 2024
b479895
refactor: session data locking in middleware/session/data.go
sixcolors Sep 13, 2024
afab580
refactor(middleware/session): Add unit test for session middleware store
sixcolors Sep 13, 2024
6c0bf25
test: fix session_test.go and store_test.go unit tests
sixcolors Sep 13, 2024
ad337f8
refactor(docs): Update session.md with v3 changes to Expiration
sixcolors Sep 13, 2024
280d539
refactor(middleware/session): Improve data pool handling and locking
sixcolors Sep 14, 2024
40da2c0
chore(middleware/session): TODO for Expiration field in session config
sixcolors Sep 14, 2024
3ad4bc9
refactor(middleware/session): Improve session data pool handling and …
sixcolors Sep 14, 2024
ffac824
refactor(middleware/session): Improve session data pool handling and …
sixcolors Sep 14, 2024
9f8c2d7
test(middleware/csrf): add session middleware coverage
sixcolors Sep 19, 2024
ecac9ce
chroe(middleware/session): TODO for unregistered session middleware
sixcolors Sep 19, 2024
e272082
refactor(middleware/session): Update session middleware for v3 changes
sixcolors Sep 19, 2024
b262a08
refactor(middleware/session): Update session middleware for v3 changes
sixcolors Sep 19, 2024
937a9b3
Merge branch 'main' into pr/3016
sixcolors Sep 19, 2024
9ec2b30
refactor(middleware/session): Update session middleware idle timeout
sixcolors Sep 20, 2024
684dc8a
docws(middleware/session): Add note about IdleTimeout requiring save …
sixcolors Sep 20, 2024
05d30a4
refactor(middleware/session): Update session middleware idle timeout
sixcolors Sep 20, 2024
ec5a698
docs(middleware/session): Update session middleware idle timeout and …
sixcolors Sep 20, 2024
13a1eb4
test(middleware/session): Fix tests for updated panics
sixcolors Sep 20, 2024
9d3b032
refactor(middleware/session): Update session middleware initializatio…
sixcolors Sep 20, 2024
9762767
refactor(middleware/session): Remove unnecessary comment about negati…
sixcolors Sep 20, 2024
e59905f
refactor(middleware/session): Update session middleware make NewStore…
sixcolors Sep 25, 2024
7765ee5
Merge branch 'main' into 2741-session-changes
sixcolors Sep 25, 2024
8716c95
refactor(middleware/session): Update session middleware Set, Get, and…
sixcolors Sep 25, 2024
0e302e9
Merge branch 'main' into 2741-session-changes
sixcolors Sep 26, 2024
951691d
feat(middleware/session): AbsoluteTimeout and key any
sixcolors Sep 26, 2024
3ac9b68
fix(middleware/session): locking issues and lint errors
sixcolors Sep 26, 2024
bc95c6a
chore(middleware/session): Regenerate code in data_msgp.go
sixcolors Sep 26, 2024
6bba849
refactor(middleware/session): rename GetSessionByID to GetByID
sixcolors Sep 26, 2024
281c0e1
docs(middleware/session): AbsoluteTimeout
sixcolors Sep 26, 2024
3d88ece
refactor(middleware/csrf): Rename Expiration to IdleTimeout
sixcolors Sep 26, 2024
3ddfeae
docs(whats-new): CSRF Rename Expiration to IdleTimeout and remove Ses…
sixcolors Sep 26, 2024
c3d3f0c
refactor(middleware/session): Rename expirationKeyType to absExpirati…
sixcolors Sep 26, 2024
0e9a73e
refactor(middleware/session): rename Test_Session_Save_Absolute to Te…
sixcolors Sep 26, 2024
a467236
chore(middleware/session): update as per PR comments
sixcolors Oct 1, 2024
6f35ff8
docs(middlware/session): fix indent lint
sixcolors Oct 1, 2024
f3c4e8e
fix(middleware/session): Address EfeCtn Comments
sixcolors Oct 1, 2024
e41ee74
refactor(middleware/session): Move bytesBuffer to it's own pool
sixcolors Oct 2, 2024
07092c8
test(middleware/session): add decodeSessionData error coverage
sixcolors Oct 2, 2024
84adbe1
refactor(middleware/session): Update absolute timeout handling
sixcolors Oct 2, 2024
f6440e2
refactor(session/middleware): fix *Session nil ctx when using Store.G…
sixcolors Oct 2, 2024
eac16b6
refactor(middleware/session): Remove unnecessary line in session_test.go
sixcolors Oct 2, 2024
7068a0e
fix(middleware/session): *Session lifecycle issues
sixcolors Oct 2, 2024
87a6cb9
docs(middleware/session): Update GetByID method documentation
sixcolors Oct 2, 2024
e5e5fd8
docs(middleware/session): Update GetByID method documentation
sixcolors Oct 2, 2024
00b9e07
docs(middleware/session): markdown lint
sixcolors Oct 2, 2024
23e823b
refactor(middleware/session): Simplify error handling in DefaultError…
sixcolors Oct 2, 2024
ba38786
fix( middleware/session/config.go
sixcolors Oct 3, 2024
f77fa8f
Merge branch 'main' into 2741-session-changes
gaby Oct 8, 2024
b54c954
add ctx releases for the test cases
ReneWerner87 Oct 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 44 additions & 19 deletions middleware/csrf/session_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,22 @@ func newSessionManager(s *session.Store, k string) *sessionManager {

// get token from session
func (m *sessionManager) getRaw(c fiber.Ctx, key string, raw []byte) []byte {
sixcolors marked this conversation as resolved.
Show resolved Hide resolved
sess, err := m.session.Get(c)
if err != nil {
return nil
sess := session.FromContext(c)
var token Token
var ok bool

if sess != nil {
token, ok = sess.Get(m.key).(Token)
} else {
// Try to get the session from the store
storeSess, err := m.session.Get(c)
if err != nil {
// Handle error
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve test coverage for error handling paths

The static analysis tool has identified several lines that are not covered by tests, primarily in error handling paths. This lack of coverage could lead to undetected issues in error scenarios. Consider adding test cases that exercise these error paths:

  1. Failure to get session from store (lines 48, 74, 93)
  2. Failure to save session (lines 78, 97)
  3. Session deletion (line 87)

Improving test coverage for these scenarios will enhance the reliability of the CSRF middleware and ensure that error handling behaves as expected in various situations.

Would you like assistance in generating test cases for these error handling paths?

Also applies to: 74-74, 78-78, 87-87, 93-93, 97-97

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 48-48: middleware/csrf/session_manager.go#L48
Added line #L48 was not covered by tests

}
sixcolors marked this conversation as resolved.
Show resolved Hide resolved
token, ok = storeSess.Get(m.key).(Token)
sixcolors marked this conversation as resolved.
Show resolved Hide resolved
}
token, ok := sess.Get(m.key).(Token)

if ok {
if token.Expiration.Before(time.Now()) || key != token.Key || !compareTokens(raw, token.Raw) {
return nil
Expand All @@ -44,25 +55,39 @@ func (m *sessionManager) getRaw(c fiber.Ctx, key string, raw []byte) []byte {

// set token in session
func (m *sessionManager) setRaw(c fiber.Ctx, key string, raw []byte, exp time.Duration) {
sess, err := m.session.Get(c)
if err != nil {
return
}
// the key is crucial in crsf and sometimes a reference to another value which can be reused later(pool/unsafe values concept), so a copy is made here
sess.Set(m.key, &Token{key, raw, time.Now().Add(exp)})
if err := sess.Save(); err != nil {
log.Warn("csrf: failed to save session: ", err)
sess := session.FromContext(c)
if sess != nil {
// the key is crucial in crsf and sometimes a reference to another value which can be reused later(pool/unsafe values concept), so a copy is made here
sess.Set(m.key, &Token{key, raw, time.Now().Add(exp)})
} else {
// Try to get the session from the store
storeSess, err := m.session.Get(c)
if err != nil {
// Handle error
return
}
sixcolors marked this conversation as resolved.
Show resolved Hide resolved
storeSess.Set(m.key, &Token{key, raw, time.Now().Add(exp)})
if err := storeSess.Save(); err != nil {
log.Warn("csrf: failed to save session: ", err)
}
}
}

// delete token from session
func (m *sessionManager) delRaw(c fiber.Ctx) {
sess, err := m.session.Get(c)
if err != nil {
return
}
sess.Delete(m.key)
if err := sess.Save(); err != nil {
log.Warn("csrf: failed to save session: ", err)
sess := session.FromContext(c)
if sess != nil {
sess.Delete(m.key)
} else {
// Try to get the session from the store
storeSess, err := m.session.Get(c)
if err != nil {
sixcolors marked this conversation as resolved.
Show resolved Hide resolved
// Handle error
return
}
storeSess.Delete(m.key)
if err := storeSess.Save(); err != nil {
log.Warn("csrf: failed to save session: ", err)
}
}
}
10 changes: 5 additions & 5 deletions middleware/session/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import (

// Config defines the config for middleware.
type Config struct {
// Allowed session duration
// Allowed session idle duration
// Optional. Default value 24 * time.Hour
Expiration time.Duration
IdleTimeout time.Duration

// Storage interface to store the session data
// Optional. Default value memory.New()
Expand Down Expand Up @@ -70,7 +70,7 @@ const (

// ConfigDefault is the default config
var ConfigDefault = Config{
Expiration: 24 * time.Hour,
IdleTimeout: 24 * time.Hour,
KeyLookup: "cookie:session_id",
KeyGenerator: utils.UUIDv4,
source: "cookie",
Expand All @@ -88,8 +88,8 @@ func configDefault(config ...Config) Config {
cfg := config[0]

// Set default values
if int(cfg.Expiration.Seconds()) <= 0 {
cfg.Expiration = ConfigDefault.Expiration
if int(cfg.IdleTimeout.Seconds()) <= 0 {
cfg.IdleTimeout = ConfigDefault.IdleTimeout
sixcolors marked this conversation as resolved.
Show resolved Hide resolved
sixcolors marked this conversation as resolved.
Show resolved Hide resolved
}
if cfg.KeyLookup == "" {
cfg.KeyLookup = ConfigDefault.KeyLookup
Expand Down
188 changes: 188 additions & 0 deletions middleware/session/middleware.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
package session

import (
"errors"
"fmt"
"sync"

"github.com/gofiber/fiber/v3"
"github.com/gofiber/fiber/v3/log"
)

// key for looking up session middleware in request context
const key = 0
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a unique context key to prevent collisions

The constant key is set to 0, which might collide with other context values in the Fiber context. It's safer to use a unique key to avoid potential conflicts with other middleware or handlers.

Consider defining the key as a unique type or string:

-const key = 0
+type contextKey string
+const key contextKey = "sessionMiddleware"
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const key = 0
type contextKey string
const key contextKey = "sessionMiddleware"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do this instead:

type contextKey int
const key contextKey = 0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sixcolors, that solution works well! Using an unexported contextKey type with an int helps avoid context key collisions. Thank you for addressing this.

(_/)
( ^_^ )
/ \


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


// Session defines the session middleware configuration
type MiddlewareConfig struct {
// Next defines a function to skip this middleware when returned true.
//
// Optional. Default: nil
Next func(c fiber.Ctx) bool

// Store defines the session store
//
// Required.
Store *Store

// ErrorHandler defines a function which is executed for errors
//
// Optional. Default: nil
ErrorHandler func(*fiber.Ctx, error)
}

type Middleware struct {
config MiddlewareConfig
Session *Session
ctx *fiber.Ctx
hasChanged bool
mu sync.RWMutex
}

// Middleware pool
var middlewarePool = &sync.Pool{
New: func() any {
return &Middleware{}
},
}

// Session is a middleware to manage session state
//
// Session middleware manages common session state between requests.
// This middleware is dependent on the session store, which is responsible for
// storing the session data.
func NewMiddleware(config MiddlewareConfig) fiber.Handler {
sixcolors marked this conversation as resolved.
Show resolved Hide resolved
return func(c fiber.Ctx) error {
// Don't execute middleware if Next returns true
if config.Next != nil && config.Next(c) {
return c.Next()
}

// Get the session
session, err := config.Store.Get(c)
if err != nil {
return err
}

// get a middleware from the pool
m := acquireMiddleware()
m.config = config
m.Session = session
m.ctx = &c

// Store the middleware in the context
c.Locals(key, m)

// Continue stack
stackErr := c.Next()

// Save the session
// This is done after the response is sent to the client
// It allows us to modify the session data during the request
// Without having to worry about calling Save()
//
// It will also extend the session idle timeout automatically.
if err := session.Save(); err != nil {
if config.ErrorHandler != nil {
config.ErrorHandler(&c, err)
} else {
log.Errorf("session: %v", err)
}
}

// release the middleware back to the pool
releaseMiddleware(m)

return stackErr
}
}

var ErrTypeAssertionFailed = errors.New("failed to type-assert to *Middleware")
sixcolors marked this conversation as resolved.
Show resolved Hide resolved

// acquireMiddleware returns a new Middleware from the pool
func acquireMiddleware() *Middleware {
middleware, ok := middlewarePool.Get().(*Middleware)
if !ok {
panic(fmt.Errorf("%w", ErrTypeAssertionFailed))
sixcolors marked this conversation as resolved.
Show resolved Hide resolved
}
return middleware
}

// releaseMiddleware returns a Middleware to the pool
func releaseMiddleware(m *Middleware) {
m.config = MiddlewareConfig{}
m.Session = nil
m.ctx = nil
middlewarePool.Put(m)
}

// FromContext returns the Middleware from the fiber context
func FromContext(c fiber.Ctx) *Middleware {
m, ok := c.Locals(key).(*Middleware)
if !ok {
log.Warn("session: Session middleware not registered. See https://docs.gofiber.io/middleware/session")
efectn marked this conversation as resolved.
Show resolved Hide resolved
return nil
}
return m
}

func (m *Middleware) Set(key string, value any) {
m.mu.Lock()
defer m.mu.Unlock()

m.Session.Set(key, value)
m.hasChanged = true
}

func (m *Middleware) Get(key string) any {
m.mu.RLock()
defer m.mu.RUnlock()

return m.Session.Get(key)
}

func (m *Middleware) Delete(key string) {
m.mu.Lock()
defer m.mu.Unlock()

m.Session.Delete(key)
m.hasChanged = true
}

func (m *Middleware) Destroy() error {
m.mu.Lock()
defer m.mu.Unlock()

err := m.Session.Destroy()
m.reaquireSession()
return err
}

func (m *Middleware) Fresh() bool {
return m.Session.Fresh()
}

func (m *Middleware) ID() string {
return m.Session.ID()
}

func (m *Middleware) Reset() error {
m.mu.Lock()
defer m.mu.Unlock()

err := m.Session.Reset()
m.hasChanged = true
return err
}

func (m *Middleware) reaquireSession() {
if m.ctx == nil {
return
}

session, err := m.config.Store.Get(*m.ctx)
if err != nil {
m.config.ErrorHandler(m.ctx, err)
}
m.Session = session
m.hasChanged = false
}
34 changes: 17 additions & 17 deletions middleware/session/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ import (
)

type Session struct {
id string // session id
fresh bool // if new session
ctx fiber.Ctx // fiber context
config *Store // store configuration
data *data // key value data
byteBuffer *bytes.Buffer // byte buffer for the en- and decode
exp time.Duration // expiration of this session
id string // session id
fresh bool // if new session
ctx fiber.Ctx // fiber context
config *Store // store configuration
data *data // key value data
byteBuffer *bytes.Buffer // byte buffer for the en- and decode
idleTimeout time.Duration // idleTimeout of this session
}

var sessionPool = sync.Pool{
Expand All @@ -42,7 +42,7 @@ func acquireSession() *Session {

func releaseSession(s *Session) {
s.id = ""
s.exp = 0
s.idleTimeout = 0
s.ctx = nil
s.config = nil
if s.data != nil {
Expand Down Expand Up @@ -135,7 +135,7 @@ func (s *Session) Reset() error {
s.byteBuffer.Reset()
}
// Reset expiration
s.exp = 0
s.idleTimeout = 0

// Delete old id from storage
if err := s.config.Storage.Delete(s.id); err != nil {
Expand Down Expand Up @@ -167,9 +167,9 @@ func (s *Session) Save() error {
return nil
}

// Check if session has your own expiration, otherwise use default value
if s.exp <= 0 {
s.exp = s.config.Expiration
// Check if session has your own idle timeout, otherwise use default value
if s.idleTimeout <= 0 {
s.idleTimeout = s.config.IdleTimeout
}

// Update client cookie
Expand All @@ -189,7 +189,7 @@ func (s *Session) Save() error {
copy(encodedBytes, s.byteBuffer.Bytes())

// pass copied bytes with session id to provider
if err := s.config.Storage.Set(s.id, encodedBytes, s.exp); err != nil {
if err := s.config.Storage.Set(s.id, encodedBytes, s.idleTimeout); err != nil {
return err
}

Expand All @@ -209,8 +209,8 @@ func (s *Session) Keys() []string {
}

// SetExpiry sets a specific expiration for this session
func (s *Session) SetExpiry(exp time.Duration) {
s.exp = exp
func (s *Session) SetIdleTimeout(idleTimeout time.Duration) {
s.idleTimeout = idleTimeout
}

func (s *Session) setSession() {
Expand All @@ -226,8 +226,8 @@ func (s *Session) setSession() {
// Cookies are also session cookies if they do not specify the Expires or Max-Age attribute.
// refer: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
if !s.config.CookieSessionOnly {
fcookie.SetMaxAge(int(s.exp.Seconds()))
fcookie.SetExpire(time.Now().Add(s.exp))
fcookie.SetMaxAge(int(s.idleTimeout.Seconds()))
fcookie.SetExpire(time.Now().Add(s.idleTimeout))
}
fcookie.SetSecure(s.config.CookieSecure)
fcookie.SetHTTPOnly(s.config.CookieHTTPOnly)
Expand Down
Loading
Loading