🐛 bug: avoid panic when reading released Fiber context values#4271
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReorders Value[T] lookup precedence to check ChangesContext value lookup precedence and released-ctx safety
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4271 +/- ##
==========================================
+ Coverage 91.21% 91.26% +0.05%
==========================================
Files 130 130
Lines 12760 12767 +7
==========================================
+ Hits 11639 11652 +13
+ Misses 709 704 -5
+ Partials 412 411 -1
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.
Pull request overview
This PR restores nil-safe context value lookups by ensuring ValueFromContext won’t panic when called on a released fiber.Ctx (e.g., after ReleaseCtx), which is important for middleware/helpers that may retain a context reference beyond its lifetime.
Changes:
- Updated
internal/contextvalue.Valueto prefer a Fiber context’sValue(key)method overLocals(key)when both are available, avoiding nil dereferences after context release. - Added a regression test to ensure
ValueFromContextdoes not panic and returns(zero, false)when invoked on a releasedfiber.Ctx.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/contextvalue/contextvalue.go | Adds a Fiber-specific interface and dispatches to Value() first to avoid panics on released contexts. |
| helpers_test.go | Adds a regression subtest covering ValueFromContext behavior after ReleaseCtx. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new fiberValueContext interface and updates the Value function to prioritize it, ensuring nil-safe access to context values after a Fiber context has been released. A new test case was added to verify that accessing values from a released context does not cause a panic. The reviewer suggested a maintainability improvement to use the existing valueContext interface at the top of the type switch instead of introducing a new interface to achieve the same result.
|
@copilot Prioritizing fiberValueContext here correctly ensures that the nil-safe Value method is used for Fiber contexts, preventing panics after context release. As a minor improvement for maintainability, consider if the existing valueContext interface could be moved to the top of the switch instead of introducing the new fiberValueContext interface. This would achieve the same result for DefaultCtx while also simplifying the handling of context.Context and other types that implement Value. If this apply, make the changes. |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/6444a23b-cba5-4c7a-863a-4352f3361d1e Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/contextvalue/contextvalue.go (1)
26-31: 💤 Low valueThe
*fasthttp.RequestCtxconcrete case is redundant but intentional.Both the concrete
*fasthttp.RequestCtxcase (lines 26-28) and theuserValueContextinterface case (lines 29-31) callUserValue(key)identically. Since*fasthttp.RequestCtximplements theuserValueContextinterface, the interface case would handle it; however, the concrete type check is a performance optimization to avoid the overhead of interface assertion. This appears to be intentional design (documented inhelpers.goas an explicitly supported context type) rather than legacy code. If clarity is desired, consider adding a comment explaining the performance rationale for keeping the concrete case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/contextvalue/contextvalue.go` around lines 26 - 31, The concrete type switch case for *fasthttp.RequestCtx duplicates the userValueContext handling but was left intentionally as a performance optimization; add a short comment above the *fasthttp.RequestCtx case explaining that *fasthttp.RequestCtx implements userValueContext and that the concrete type check is kept to avoid the extra interface assertion overhead (referencing UserValue and the userValueContext interface), and optionally link or mention helpers.go where this explicit support is documented so future readers understand it’s deliberate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/contextvalue/contextvalue.go`:
- Around line 26-31: The concrete type switch case for *fasthttp.RequestCtx
duplicates the userValueContext handling but was left intentionally as a
performance optimization; add a short comment above the *fasthttp.RequestCtx
case explaining that *fasthttp.RequestCtx implements userValueContext and that
the concrete type check is kept to avoid the extra interface assertion overhead
(referencing UserValue and the userValueContext interface), and optionally link
or mention helpers.go where this explicit support is documented so future
readers understand it’s deliberate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca1c95f5-88e6-4dd0-a752-7d1ae4db42fa
📒 Files selected for processing (1)
internal/contextvalue/contextvalue.go
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/6444a23b-cba5-4c7a-863a-4352f3361d1e Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Applied in e0d04b6. I removed the extra |
|
@copilot do not remove the case: We need to support fiber ctx, fasthttp request ctx, and context.Context |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/a450c68f-35c5-4d62-b8ee-5b6963f77ae6 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/contextvalue/contextvalue.go (2)
21-21: ⚡ Quick winClarify documentation to reflect support for standard
context.Context.The comment states "supported Fiber context types" but the function also handles standard
context.Context(line 27-29), which is not Fiber-specific.📝 Proposed documentation fix
-// Value retrieves a value stored under key from supported Fiber context types. +// Value retrieves a value stored under key from supported context types (Fiber contexts, context.Context, and fasthttp.RequestCtx).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/contextvalue/contextvalue.go` at line 21, Update the doc comment for the Value function to reflect that it supports both Fiber context types and the standard context.Context: locate the Value function in contextvalue.go and change the sentence "Value retrieves a value stored under key from supported Fiber context types." to mention support for standard context.Context as well (e.g., "Value retrieves a value stored under key from supported Fiber context types and the standard context.Context"), ensuring the comment references the Value function name so readers know which function the doc covers.
24-38: ⚡ Quick winConsider the author's suggestion to reorder
valueContextbeforecontext.Contextfor clarity.The current order places
context.Context(line 27-29) beforevalueContext(line 30-32). Both cases call.Value(key), so this is functionally equivalent. However, as noted in the PR comments, movingvalueContextto be checked first would make the intent more explicit: prioritize types with aValuemethod (including Fiber'sDefaultCtxand other contexts) before falling back toLocals.The current placement of
fiberLocalContext(line 33-35) after bothcontext.ContextandvalueContextis correct and prevents panics on released contexts.Trade-offs:
- Current order (
context.Contextfirst): Standard library interface is checked first, which may be more intuitive for readers familiar with Go's context package.- Suggested order (
valueContextfirst): Makes the "prefer Value over Locals" design more explicit and aligns with the PR's intent to avoidLocalson released contexts.Since both approaches are safe and the author explicitly suggested this reordering, moving
valueContextbeforecontext.Contextwould improve maintainability and align with the PR's stated goal.♻️ Proposed reordering to prioritize valueContext
func Value[T any](ctx, key any) (T, bool) { switch typed := ctx.(type) { case *fasthttp.RequestCtx: val, ok := typed.UserValue(key).(T) return val, ok + case valueContext: + val, ok := typed.Value(key).(T) + return val, ok case context.Context: val, ok := typed.Value(key).(T) return val, ok - case valueContext: - val, ok := typed.Value(key).(T) - return val, ok case fiberLocalContext: val, ok := typed.Locals(key).(T) return val, ok case userValueContext: val, ok := typed.UserValue(key).(T) return val, ok default: var zero T return zero, false } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/contextvalue/contextvalue.go` around lines 24 - 38, Reorder the type switch cases so valueContext is checked before context.Context: in the switch handling *fasthttp.RequestCtx, context.Context, valueContext, fiberLocalContext, userValueContext, move the case valueContext block to appear immediately after the *fasthttp.RequestCtx case and before the context.Context case (leave fiberLocalContext and the other cases in place) so the code still uses .Value(key) for valueContext prior to falling back to the generic context.Context case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/contextvalue/contextvalue.go`:
- Line 21: Update the doc comment for the Value function to reflect that it
supports both Fiber context types and the standard context.Context: locate the
Value function in contextvalue.go and change the sentence "Value retrieves a
value stored under key from supported Fiber context types." to mention support
for standard context.Context as well (e.g., "Value retrieves a value stored
under key from supported Fiber context types and the standard context.Context"),
ensuring the comment references the Value function name so readers know which
function the doc covers.
- Around line 24-38: Reorder the type switch cases so valueContext is checked
before context.Context: in the switch handling *fasthttp.RequestCtx,
context.Context, valueContext, fiberLocalContext, userValueContext, move the
case valueContext block to appear immediately after the *fasthttp.RequestCtx
case and before the context.Context case (leave fiberLocalContext and the other
cases in place) so the code still uses .Value(key) for valueContext prior to
falling back to the generic context.Context case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4e887eae-f438-49fe-8a26-10c9a2d73d05
📒 Files selected for processing (1)
internal/contextvalue/contextvalue.go
ReneWerner87
left a comment
ReneWerner87
left a comment
There was a problem hiding this comment.
The diagnosis is right and the patch works (CI is green, regression test covers the panic path). Two suggestions, the first one is the load-bearing one.
1. Consider fixing the root cause in DefaultReq.Locals instead
The asymmetry between DefaultCtx.Value (ctx.go:625, nil-safe via if c.fasthttp == nil { return nil }) and DefaultReq.Locals (req.go:703, no guard) is the actual bug. Hardening only the helper means:
- every other caller of
c.Locals(key)afterReleaseCtxstill panics: middleware (session,requestid, ...), user code, tests; the helper reorder only protectsValueFromContext. - the type-switch reorder becomes load-bearing in a non-obvious way; a future contributor sorting cases by specificity reintroduces the bug.
A minimal alternative, applied at the symptom source:
func (r *DefaultReq) Locals(key any, value ...any) any {
if r.c.fasthttp == nil {
if len(value) > 0 {
return value[0]
}
return nil
}
if len(value) == 0 {
return r.c.fasthttp.UserValue(key)
}
r.c.fasthttp.SetUserValue(key, value[0])
return value[0]
}This mirrors Value's pattern, fixes every caller, and makes the helper reorder optional (you can keep it for clarity or revert to the previous order without functional impact).
2. If you keep the reorder, document why
The order in the type switch is now load-bearing for nil-safety. Without a comment the next contributor who thinks "most specific case first" can revert it. See inline.
3. Test could carry a tiny bit more weight
The regression test only covers *DefaultCtx after release with a string value. Adding a fiber.CustomCtx-released case and a pre-release sanity check (that key is retrievable before ReleaseCtx) would document the contract more fully and protect against accidental CustomCtx behavior changes from the reorder. See inline.
Nothing here is blocking; CI is green and Codecov is at 100% for modified lines.
|
@copilot Please address all the review comments. |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/7ea45adc-45b7-4fe5-96ca-b3688937a170 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/7ea45adc-45b7-4fe5-96ca-b3688937a170 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/7ea45adc-45b7-4fe5-96ca-b3688937a170 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/7ea45adc-45b7-4fe5-96ca-b3688937a170 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/7ea45adc-45b7-4fe5-96ca-b3688937a170 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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: 22d3321 | Previous: 33b7fd4 | Ratio |
|---|---|---|---|
Benchmark_NewError (github.com/gofiber/fiber/v3) |
61.77 ns/op 24 B/op 1 allocs/op |
37.8 ns/op 24 B/op 1 allocs/op |
1.63 |
Benchmark_NewError (github.com/gofiber/fiber/v3) - ns/op |
61.77 ns/op |
37.8 ns/op |
1.63 |
This comment was automatically generated by workflow using github-action-benchmark.
Motivation
fiber.Ctxinstances cannot cause a panic when helpers callValueFromContextafterReleaseCtx.Description
Value(key)overLocals(key)when a Fiber-like context exposes both by adding afiberValueContextinterface and dispatching toValuefirst ininternal/contextvalue/contextvalue.go.helpers_test.gothat assertsValueFromContextdoes not panic and returns(zero, false)when called with a releasedfiber.Ctx.