-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes address inconsistencies in session and CSRF middleware handling by aligning with NIST guidelines on timeout and expiration. Key modifications include refactoring session handling to use Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant FiberApp
participant Middleware
participant SessionStore
Client->>FiberApp: HTTP Request
FiberApp->>Middleware: Process Request
Middleware->>SessionStore: Retrieve Session (session.FromContext(c))
SessionStore-->>Middleware: Session Data
Middleware->>FiberApp: Continue Processing
FiberApp-->>Client: HTTP Response
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
middleware/csrf/session_manager.go (1)
74-84
: Consider adding logging for error scenarios in session deletion.if err != nil { log.Errorf("Failed to retrieve session for deletion: %v", err) }middleware/session/middleware.go (1)
51-94
: Add detailed documentation for the middleware lifecycle.// NewMiddleware creates and manages the lifecycle of session middleware. // It retrieves a middleware instance from the pool, attaches it to the context, // and ensures it is properly released after the request is processed.middleware/session/session.go (1)
Line range hint
170-192
: Improve error handling in theSave
method to handle potential encoding failures more gracefully.if err != nil { log.Errorf("Session encoding failed: %v", err) return err }
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- middleware/csrf/session_manager.go (3 hunks)
- middleware/session/config.go (3 hunks)
- middleware/session/middleware.go (1 hunks)
- middleware/session/session.go (7 hunks)
Additional comments not posted (4)
middleware/csrf/session_manager.go (1)
57-68
: Ensure thread safety when setting session tokens.Verification successful
The
Set
method implementations found in the codebase indicate that thread safety measures are in place. Specifically:
- In
middleware/session/middleware.go
, theSet
method uses a mutex (m.mu.Lock()
andm.mu.Unlock()
) to ensure thread safety.- In
middleware/session/data.go
, theSet
method also uses a lock (d.Lock()
andd.Unlock()
) to ensure thread safety.These implementations confirm that the session management operations are thread-safe.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential data races when setting session tokens. # Command: Run your Go data race detector tool on this section.Length of output: 0
Script:
#!/bin/bash # Description: Check if the session management library used in the codebase provides thread-safe operations. # Command: Search for the session management library and inspect its documentation or implementation for thread safety. # Find the session management library used in the codebase session_lib=$(rg --files-with-matches 'session.FromContext' | head -n 1) # Check if the session management library provides thread-safe operations if [ -n "$session_lib" ]; then rg 'thread-safe' "$session_lib" else echo "Session management library not found." fiLength of output: 208
Script:
#!/bin/bash # Description: Extract and inspect the implementation of the `Set` method in the session management library to verify thread safety. # Find the implementation of the `Set` method in the session management library ast-grep --lang go --pattern 'func ($_ $_) Set($_, $_) $_' middleware/csrf/session_manager.goLength of output: 95
Script:
#!/bin/bash # Description: Search the entire codebase for the implementation of the `Set` method to verify thread safety. # Find the implementation of the `Set` method in the entire codebase ast-grep --lang go --pattern 'func ($_ $_) Set($_, $_) $_'Length of output: 1417
middleware/session/config.go (1)
13-15
: RenamingExpiration
toIdleTimeout
clarifies the purpose and aligns with best practices.middleware/session/middleware.go (1)
31-37
: Well-structuredMiddleware
struct with appropriate use of synchronization for thread safety.middleware/session/session.go (1)
16-22
: The addition ofidleTimeout
in theSession
struct is a positive change for better session management.
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 | ||
} | ||
token, ok = storeSess.Get(m.key).(Token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor to improve error handling in session retrieval.
if err != nil {
// Log the error or handle it according to your error handling policy
return nil
}
if int(cfg.IdleTimeout.Seconds()) <= 0 { | ||
cfg.IdleTimeout = ConfigDefault.IdleTimeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhance error handling for configuration validation.
if len(selectors) != numSelectors {
log.Errorf("[session] KeyLookup format is incorrect: %s", cfg.KeyLookup)
return ConfigDefault // Return default config or handle error appropriately
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: There should probably be a way to allow for not having an idle timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sixcolors, to accommodate the possibility of not having an idle timeout, we can introduce a specific value for IdleTimeout
that, when set, indicates the session should not expire based on time. For example, setting IdleTimeout
to -1
could be used to represent no timeout.
Here's a proposed change to the configDefault
function in middleware/session/config.go
to support this:
func configDefault(config ...Config) Config {
if len(config) < 1 {
return ConfigDefault
}
cfg := config[0]
// Set default values
if cfg.IdleTimeout == -1 {
// -1 means no idle timeout
cfg.IdleTimeout = 0 // 0 could internally represent no timeout
} else if int(cfg.IdleTimeout.Seconds()) <= 0 {
cfg.IdleTimeout = ConfigDefault.IdleTimeout
}
// Existing code for setting other defaults
if cfg.KeyLookup == "" {
cfg.KeyLookup = ConfigDefault.KeyLookup
}
if cfg.KeyGenerator == nil {
cfg.KeyGenerator = ConfigDefault.KeyGenerator
}
// Existing code for validating KeyLookup
selectors := strings.Split(cfg.KeyLookup, ":")
const numSelectors = 2
if len(selectors) != numSelectors {
panic("[session] KeyLookup must in the form of <source>:<name>")
}
switch Source(selectors[0]) {
case SourceCookie:
cfg.source = SourceCookie
case SourceHeader:
cfg.source = SourceHeader
case SourceURLQuery:
cfg.source = SourceURLQuery
default:
panic("[session] source is not supported")
}
cfg.sessionName = selectors[1]
return cfg
}
This modification allows setting IdleTimeout
to -1
to disable the timeout, while maintaining the default behavior when a non-negative timeout is specified. If this approach aligns with your expectations, it could be implemented to enhance the flexibility of session management.
Known IssuesStorage Adapter UPSERT BehaviorThere is an issue with the way Fiber's storage adapters handle setting keys. The current operation for setting a key is an UPSERT, which means it will insert a new key if it does not exist or update it if it does. This can lead to issues in scenarios with simultaneous requests altering the same session. For example:
If the session destruction completes first, the inconsequential change could recreate the session, potentially preventing actions like logging out and posing security risks. Since this PR changes the session behavior to save and update on every request, it amplifies the problem if not addressed. |
See: https://github.com/gofiber/recipes/tree/v3-csrf-with-session-PR-3016-update/csrf-with-session for usage example of this PR in current state. |
Those new sequence diagrams are awesome |
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the Mutex Locks here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mu mutex in the Middleware struct is used to ensure thread-safety when modifying the Session and hasChanged fields of the Middleware instance. This is necessary because the Middleware instance could potentially be accessed by multiple goroutines concurrently.
However, since the Session struct itself already ensures thread-safety in its methods (i.e., it uses its own mutexes to protect its data), then the mu mutex in the Middleware struct might be redundant for operations on the Session.
But, the mu mutex is still necessary for the Session and hasChanged field. If this field is accessed concurrently without a mutex, it could lead to race conditions.
I will update.
middleware/session/middleware.go
Outdated
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better if we use New() here. Maybe we can rename current New to NewStore, NewStorage etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that uses only e New
or NewSession
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should we handle the interaction between Session and Middleware? Here's the potential issue: if the middleware is in use and the session for the same user is retrieved from the store, any changes made and saved in the handler would be overwritten by the middleware.
We could add a check in the store. If the middleware is in the context, it could pass that session when Store.Get(c)
was called. And Session.Save()
could noop
if it's the middleware session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sixcolors it makes sense to me. I believe that follows with your point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, you've been doing a good job. I commented on a few details. Thanks for contributing here.
if err != nil { | ||
// Handle error | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds not so good. Why do you ignore error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSRF middleware lacks a mechanism to handle that error beyond failing the CSRF check. A refactor of the CSRF middleware would be necessary to address this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's okay, can you open this issue for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai open an issue to improve error handling in the middleware/csrf/session_manager.go and middleware/csrf/storage_manager.go files as discussed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sixcolors, I've successfully opened an issue to improve error handling in the CSRF middleware as discussed. You can track the progress and further details on this issue at GitHub Issue #3020.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue for improving error handling in specified files
gh issue create --title "Improve Error Handling in CSRF Middleware" --body "There is a need to enhance error handling in the CSRF middleware, specifically within the files `middleware/csrf/session_manager.go` and `middleware/csrf/storage_manager.go`. This issue was identified during a discussion in PR https://github.com/gofiber/fiber/pull/3016. Improving this could involve refactoring the error handling logic to provide more robust and informative error responses.
For more context, see the discussion here: https://github.com/gofiber/fiber/pull/3016#discussion_r1619785319" --assignee sixcolors
Length of output: 855
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll take a look tomorrow
if err != nil { | ||
// Handle error | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here about ignore error
} else { | ||
// Try to get the session from the store | ||
storeSess, err := m.session.Get(c) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, middleware/csrf/storage_manager.go
also ignore errors in the same manner, for example, _ = m.storage.Set(key, raw, exp) //nolint:errcheck // TODO: Do not ignore error
. As a result, the CSRF check fails in the middleware, and the CSRF error handler is called with ErrTokenNotFound
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's go to solve together
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "safe" option appears to be failing the CSRF, so it's likely the correct result already. We could incorporate logging to improve things.
middleware/session/middleware.go
Outdated
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that uses only e New
or NewSession
Co-authored-by: Renan Bastos <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3016 +/- ##
==========================================
- Coverage 82.78% 82.12% -0.67%
==========================================
Files 115 116 +1
Lines 8227 8348 +121
==========================================
+ Hits 6811 6856 +45
- Misses 1086 1147 +61
- Partials 330 345 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Feature: Re-write Session Middleware with Handler
Summary
This pull request introduces significant changes to the session middleware, enhancing its functionality and improving its robustness. The primary focus is on the re-write of the session middleware with a new handler.
Status
session.New()
returns middleware handler)Changelog
Middleware Changes
middleware/session/middleware.go
to define the session middleware and its configuration.CSRF Session Manager Changes
getRaw
,setRaw
, anddelRaw
methods to use the session from the context first before falling back to the store.log
) from the session manager.Configuration Changes
Expiration
toIdleTimeout
to better reflect the session idle duration.Session Struct Changes
exp
is nowidleTimeout
to indicate the idle timeout duration for sessions.idleTimeout
instead ofexp
.Save
method to handle idle timeout correctly.Future Work On This PR
This is an initial commit for the new middleware approach. If the new middleware handler proves to be more effective, further changes will be made to:
session.New
to return theMiddleware
handler instead of the oldsession.Store
struct.Session
toRawSession
, preferring the new style of session management.A suggested standard use for the session might look something like this after these modifications:
Testing
Notes
Please review the changes and provide feedback or approval for merging.
Fixes #2741