🐛 bug: harden session middleware lifecycle and error handling#4107
🐛 bug: harden session middleware lifecycle and error handling#4107gaby merged 2 commits intoupdate-session-middleware-to-fail-gracefullyfrom
Conversation
…anics Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## update-session-middleware-to-fail-gracefully #4107 +/- ##
================================================================================
- Coverage 91.15% 91.13% -0.03%
================================================================================
Files 119 119
Lines 11354 11367 +13
================================================================================
+ Hits 10350 10359 +9
- Misses 636 638 +2
- Partials 368 370 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: a7fdf2c | Previous: 92ada8a | Ratio |
|---|---|---|---|
BenchmarkDecoderedirectionMsgs - MB/s |
114.95 MB/s |
47.86 MB/s |
2.40 |
This comment was automatically generated by workflow using github-action-benchmark.
93fdf46
into
update-session-middleware-to-fail-gracefully
There was a problem hiding this comment.
Pull request overview
This pull request hardens the session middleware against three classes of potential defects: leaked pool objects, unsanitized session IDs, and panics from pool type assertion failures. The changes improve the robustness and reliability of the session middleware without introducing breaking changes to the public API.
Changes:
- Replaced
c.Locals(key, nil)withfiber.StoreInContext(c, key, nil)to properly clear middleware pointers from both Locals and context.Context when PassLocalsToContext is enabled - Added session ID validation (
isValidSessionID) that rejects empty, oversized (>4096 bytes), or non-visible-ASCII session IDs before they're used as storage keys - Converted panic-based error handling to safe type assertions with fallback allocation in
acquireMiddleware(),acquireSession(), andacquireData()
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| middleware/session/store.go | Added session ID validation function and integrated it into getSessionID; improved nolint comment clarity |
| middleware/session/session.go | Replaced panic with safe type assertion and fallback allocation in acquireSession |
| middleware/session/middleware.go | Changed cleanup to use fiber.StoreInContext for proper context clearing; removed panic from acquireMiddleware |
| middleware/session/data.go | Replaced panic with safe type assertion and fallback allocation in acquireData |
| middleware/session/store_test.go | Added comprehensive tests for session ID validation including boundary cases |
| middleware/session/middleware_test.go | Added test to verify proper cleanup when PassLocalsToContext is enabled |
Description
Hardens the session middleware against three classes of defects: leaked pool objects via incomplete context cleanup, unsanitized session IDs used directly as storage keys, and panics in the request handler path from pool type assertions.
Changes introduced
fiber.StoreInContextfor cleanup — Middleware cleanup now usesfiber.StoreInContext(c, key, nil)instead ofc.Locals(key, nil), so released*Middlewarepointers are cleared from both Locals andcontext.ContextwhenPassLocalsToContextis enabled. Previously, stale pointers remained accessible viac.Context()after the object was returned to the pool.Session ID validation — Extracted session IDs are now validated before reaching
Storage.GetWithContext().isValidSessionID()rejects empty, oversized (>4096 bytes), and non-visible-ASCII (outside 0x21–0x7E) values. Invalid IDs are silently discarded and a fresh session is generated.Panic removal —
acquireMiddleware(),acquireSession(), andacquireData()now use safe type assertions with fallback allocation instead ofpanic(). Pool corruption no longer crashes the server.Init-error handling —
initialize()returnserror;NewWithStoreroutes init failures through the configuredErrorHandler/DefaultErrorHandler, releases the middleware, and only returns the raw error when no response was written.finalizeSession()— Conditionally callssaveSession()(skips when destroyed) and always callsreleaseSession()exactly once.Store error paths —
getSessionreleases the session before returning onReset()failure;GetByIDreleases before returningErrSessionIDNotFoundInStoreafter expired-session destroy. Fixed the nolint contextcheck comment to reflect thatsess.gctxis now correctly set.ErrTypeAssertionFailedretained for backward compatibility despite panic removal.Type of change
Checklist
Commit formatting
🐛 Used throughout.
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.