π fix(session): redesign read-only extractor handling β opt-in trust + cached source#4235
π fix(session): redesign read-only extractor handling β opt-in trust + cached source#4235ReneWerner87 with Copilot wants to merge 8 commits into
Conversation
β¦ no data found in store Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/eb400386-f910-4188-a7b8-33bb8fba5d4f Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
β¦curity note Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/eb400386-f910-4188-a7b8-33bb8fba5d4f Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
|
@ReneWerner87 The solution it implemented seems kind of weird |
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4235 +/- ##
==========================================
+ Coverage 91.25% 91.29% +0.03%
==========================================
Files 127 127
Lines 12556 12589 +33
==========================================
+ Hits 11458 11493 +35
+ Misses 687 686 -1
+ Partials 411 410 -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:
|
|
I will check it tomorrow |
β¦cached source Replaces the implicit "preserve query/form/param/custom session IDs" behaviour with an explicit, validated opt-in and centralizes source tracking so chained extractors stay consistent across multiple Store.Get calls in the same request. Changes: - extractors: add Source.IsWritable() so callers no longer hand-roll the cookie/header check. - session.Store: cache the resolved session ID together with its originating Source in request locals (sessionIDInfo). resolveSessionID iterates chain sub-extractors and reports the source that actually produced the value; subsequent Store.Get calls in the same request reuse that decision instead of re-deriving it from the chain wrapper. - session.Config: add TrustClientSessionID (default false) and ClientSessionIDValidator. Read-only sources (query/form/param/custom) only preserve a client-supplied unknown ID when both flags are set and the validator accepts it; otherwise the ID is discarded and a fresh server ID is generated, matching cookie/header fixation protection. Cookie/header sources are unaffected and always discard unknown IDs. - docs/middleware/session.md: document the new flags, the security trade-offs, and the recommended HMAC/allow-list validator pattern. - store_test.go: update Test_Store_resolveSessionID to the new signature, cover trust-on/off, validator rejects, opt-in roundtrip, chain source resolution, two-Get-same-request consistency for both query and cookie sources, and empty client ID. Test_Store_DeleteSession now models the cross-request delete + re-Get path instead of relying on the previous regenerate-loop bug. Closes #4234
ReneWerner87
changed the title
|
@gaby - agree, the original approach was off. Pushed a redesign in Why the first cut was weird
What the new commit does
PR title + body updated to match. Ready for another look. |
β¦and docs Pure prose cleanup. No behaviour change.
- Reorder Config fields so the bool TrustClientSessionID joins the trailing bool group; ClientSessionIDValidator moves into the function-pointer cluster. Sizeof(Config) drops from 208 to 200 bytes (fieldalignment). - Spelling: behaviour -> behavior in Config doc comment. - Tests: cover Source.IsWritable for every Source value, the resolveSessionID chain skip-nil-Extract path, the chain/single no-value wrapper-source fallback, and the Storage.GetWithContext error propagation in getSession.
|
/gemini review |
|
@coderabbitai review |
β Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the βοΈ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR resolves issue ChangesClient-Supplied Session ID Trust & Security
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as Session Middleware
participant Extractor as Extractors
participant Validator as ClientSessionIDValidator
participant Store as Session Store
participant Storage as Backend Storage
Client->>Middleware: Request with query SESSIONID=abc
Middleware->>Store: Get(ctx)
activate Store
Store->>Extractor: resolveSessionID(extractors, ctx)
Extractor-->>Store: {id: "abc", source: SourceQuery}
Store->>Store: acceptClientID(id, source, config)
alt TrustClientSessionID=true
Store->>Validator: ClientSessionIDValidator("abc")
alt Valid
Validator-->>Store: true
Store->>Storage: Load session "abc"
Storage-->>Store: session data
else Invalid
Validator-->>Store: false
Store->>Store: Generate new ID
Store->>Storage: Load session (new ID)
end
else TrustClientSessionID=false
Store->>Store: Generate new ID
Store->>Storage: Load session (new ID)
end
deactivate Store
Store-->>Middleware: session data
Middleware-->>Client: Response with Set-Cookie or persisted session
Estimated Code Review Effortπ― 3 (Moderate) | β±οΈ ~25 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
π₯ Pre-merge checks | β 5β Passed checks (5 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 |
|
@copilot review |
![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 the ability to trust client-supplied session IDs from read-only sources, such as query parameters or form data, by adding TrustClientSessionID and ClientSessionIDValidator to the session configuration. It implements logic to track the source of extracted IDs, ensuring that unknown IDs from read-only sources are discarded by default to prevent session fixation unless explicitly validated. The update also includes comprehensive tests and documentation regarding the security implications of these new settings. I have no feedback to provide.
|
@gaby @sixcolors can you check this |
Reviewed
All tests pass ( |
There was a problem hiding this comment.
Pull request overview
This PR redesigns session ID handling for read-only extractors (query/form/param/custom/auth-header) to address #4234 while making client-supplied ID acceptance explicit and opt-in, and ensuring consistent behavior across multiple Store.Get calls within the same request.
Changes:
- Adds
extractors.Source.IsWritable()to centralize βwritable vs read-onlyβ source classification. - Updates session store ID resolution to cache
{id, source}in request locals, adds opt-inTrustClientSessionID+ClientSessionIDValidator, and adjusts unknown-ID handling accordingly. - Extends docs and tests to cover the new configuration and expected behaviors (including chained extractor behavior and per-request caching).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
middleware/session/store.go |
Caches resolved session ID + originating source; adds opt-in trusted client ID acceptance for read-only sources. |
middleware/session/store_test.go |
Updates and expands tests for new ID resolution/caching behavior and opt-in trust/validator logic. |
middleware/session/config.go |
Adds TrustClientSessionID and ClientSessionIDValidator configuration fields and documentation. |
extractors/extractors.go |
Adds Source.IsWritable() helper for consistent source classification. |
extractors/extractors_test.go |
Adds unit tests for Source.IsWritable(). |
docs/middleware/session.md |
Documents new trust/validator options and security implications; updates config properties table. |
|
Thanks for working on this. Appreciate the redesign effort to make this safe-by-default, but I still don't think client-supplied session IDs belong in session middleware regardless of gating. Session IDs should be server-issued and opaque. OWASP is clear that they should not be user-controllable, since they effectively become credentials. Allowing client-supplied IDs, even behind a flag and callback, turns them into user-influenced lookup keys and weakens the trust boundary. The earlier fix (GHSA-98j2-3j3p-fw2v) treated unknown client IDs as untrusted. This weakens that protection. The callback pushes responsibility to user code with framework blessing. This would also be an outlier. Express.js, Django, Rails, ASP.NET Core, and Spring all keep session IDs server-generated via cookies or headers. None support client-generated IDs through extractors. Adding If someone needs to key off an external token, that's an app/auth concern, not session middleware. It can already be handled without framework changes: app.Use(func(c fiber.Ctx) error {
// Check for a session
handler := session.FromContext(c)
if handler == nil {
return c.SendStatus(fiber.StatusInternalServerError)
}
if v, ok := handler.Get("authorized").(bool); !ok || !v {
// Return a 401 Unauthorized with session info for debugging
return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{
"authorized": false,
"session_id": handler.ID(),
"session_timeout": sessionTimeout.Seconds(),
})
// Or a redirected to the /init route to initialize the session
}
// Continue to the next handler
return c.Next()
})
app.Get("/init", func(c fiber.Ctx) error {
handler := session.FromContext(c)
if handler == nil {
return c.SendStatus(fiber.StatusInternalServerError)
}
if !validateAPIToken(c.Query("api_token")) {
return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{
"error": "Invalid API token",
"session_id": handler.ID(),
"session_timeout": sessionTimeout.Seconds(),
})
}
// Regenerate session ID to prevent session fixation attacks
if err := handler.Regenerate(); err != nil {
return c.SendStatus(fiber.StatusInternalServerError)
}
handler.Set("authorized", true)
handler.Set("user_id", lookupUserID(c.Query("api_token")))
return c.JSON(fiber.Map{
"session_id": handler.ID(),
"session_timeout": sessionTimeout.Seconds(),
})
})I'd recommend keeping the current behavior. |
Description
Fixes a bug where session IDs supplied via read-only extractors (query, form, URL param, custom) were silently discarded when no session data existed in storage for that ID. A new random ID was generated instead β but never communicated back to the client (these sources are write-only; no
Set-Cookie/response header is written), so every request created an orphaned session.The initial fix preserved client-supplied IDs unconditionally, which introduced a session-fixation vector and a chain-aware writability inconsistency between the first and subsequent
Store.Getcalls within the same request. This revision keeps the bug fix while making the behaviour explicit, opt-in, and validated, and centralises source tracking so the chain decision is cached together with the resolved ID.Changes introduced
extractors/extractors.goSource.IsWritable()helper. Cookie and header sources are writable; query, form, param, custom, and auth-header sources are not. Removes open-codedsrc == SourceCookie || src == SourceHeaderchecks that the prior revision duplicated across callers.middleware/session/store.go(string, bool)return with asessionIDInfo{id, source}struct cached in request locals.resolveSessionIDwalks chained sub-extractors and reports the actual source that produced the value.Store.Getwithin the same request reuses the cachedsessionIDInfoinstead of re-deriving writability from the chain wrapper, fixing a regression where twoStore.Getcalls would yield different source decisions (and therefore different discard/preserve outcomes).acceptClientID(info)consolidates the read-only-trust decision: writable sources are never accepted here; read-only sources require bothTrustClientSessionIDand aClientSessionIDValidatorthat accepts the supplied ID.rawData == nilbranch becomes a clean three-way switch: already-resolved (keep), opt-in trusted client ID (keep + fresh), otherwise discard + regenerate.middleware/session/config.goTwo new optional fields:
TrustClientSessionID boolβ defaultfalse. Allows client-supplied IDs from read-only sources to be preserved.ClientSessionIDValidator func(id string) boolβ required whenTrustClientSessionIDis true; nil rejects all.Defaults are unchanged for cookie/header users. Read-only-source users keep safe-by-default behaviour (
KeyGenerator()regeneration) and must explicitly opt in; preserving client-controlled IDs is a security-relevant decision and should not be silent.docs/middleware/session.mdmiddleware/session/store_test.goandextractors/extractors_test.goTest_Store_resolveSessionIDupdated for the new return type; new sub-tests assert the chain reports the source of the actually-providing extractor, skips nil Extract funcs, returns wrapper source when no value is found.Test_Source_IsWritablecovers everySourcevalue.Test_Store_Getextended: default rejects client query ID; opt-in + permissive validator preserves it; opt-in + rejecting validator discards it; opt-in without validator fails closed; opt-in roundtrip persists data across requests; chain cookie-first still discards unknown cookie even with opt-in; chain query fallback preserved when cookie absent; twoStore.Getcalls in same request return the same ID for both query and cookie sources; empty client query ID falls through to server generation; storageGetWithContexterror is propagated.Test_Store_DeleteSessionnow models the realistic cross-request delete + re-Get path.Behaviour Matrix
TrustClientSessionID=true+ validator passesCompatibility
Cookie/header users: no behaviour change.
Read-only-source users must set
TrustClientSessionID: trueand provide aClientSessionIDValidatorto preserve client-supplied IDs.Benchmarks: Hot path: one extra interface boxing per request (already present for the existing locals key). No measurable impact.
Documentation Update:
docs/middleware/session.mdupdated with new config fields and security guidance.Changelog/What's New: Bug fix β session middleware now correctly handles read-only extractor sources with explicit, validated opt-in for client-supplied ID trust.
Migration Guide: No breaking changes for cookie/header users. Read-only-source users who relied on implicit ID preservation must add
TrustClientSessionID: trueand aClientSessionIDValidator.API Alignment with Express: N/A.
API Longevity: New config fields are additive and default to safe behaviour; no existing API is modified.
Examples: See
TrustClientSessionID/ClientSessionIDValidatorindocs/middleware/session.md.Type of change
Checklist
/docs/directory for Fiber's documentation.