Repository navigation
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4815 +/- ##
==========================================
+ Coverage 96.06% 96.19% +0.13%
==========================================
Files 156 157 +1
Lines 20016 20856 +840
==========================================
+ Hits 19228 20063 +835
- Misses 477 478 +1
- Partials 311 315 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
Keeping draft until other PR's are merged. |
gaby
force-pushed
the
ccr-ac6228da-p45qep
branch
from
October 29, 2026 14:48
5646fdf to
f54397c
Compare
Contributor
|
✅ No significant benchmark change. c58244e vs main@0bc83ea · 2212/2284 results compared · 72 new, 0 gone · retest: 1 reported re-checked · noise-aware thresholds · full results |
gaby
force-pushed
the
ccr-ac6228da-p45qep
branch
4 times, most recently
from
November 2, 2026 15:28
e70e243 to
12ba319
Compare
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
gaby
force-pushed
the
ccr-ac6228da-p45qep
branch
2 times, most recently
from
November 5, 2026 01:05
519c02e to
a49c81c
Compare
gaby
force-pushed
the
ccr-ac6228da-p45qep
branch
5 times, most recently
from
November 6, 2026 15:02
24463bf to
9b6fa29
Compare
…hash, deadline cap, GetAs, pluggable codec
Six additive, non-breaking changes to middleware/session. Every new option is
off by default; with the default configuration the responses and the storage
calls are those of main, apart from the cases listed below.
- Several sessions per request. A store takes the IDs Store.Get issued only
from stores that read the same source (cookie, header, query parameter...),
so a second store no longer adopts another store's ID. Store.FromContext(c)
returns one store's middleware.
- Config.SkipEmptySessions keeps a new session without data out of the storage
and the response (saveUninitialized: false in express-session).
- Config.StorageKeyPrefix and Config.HashStorageKeys keep applications that
share a storage apart and store each session under the SHA-256 digest of its
ID.
- Hardening. A save keeps the record and the cookie no longer than the time
left before AbsoluteTimeout; warnings for __Host-/__Secure- cookies that
browsers reject and for store options set on the wrong Config; a typed-nil
Storage panics in NewStore.
- Ergonomics. session.GetAs[V], IdleTimeout() and AbsoluteExpiry().
- Pluggable codec. Config.Codec stores each value encoded on its own, so a
request decodes only what it reads (GetInto, GetAs) and a save encodes only
what was set. session.JSONCodec{} and session.NewCodec(name, marshal,
unmarshal). Existing sessions stay gob until they end. csrf keeps its token
under a key that is not a string, so csrf.New panics for a session store
with a codec.
Behavior that differs from main (details in docs/whats_new.md):
1. Stores over different cookies no longer adopt each other's cached ID.
2. A save keeps record and cookie for the time left before AbsoluteTimeout: the
first cookie says Max-Age=1799 where main says 1800 when it equals
IdleTimeout.
3. A typed-nil Storage panics in NewStore, not on the first request.
4. An abandoned request's middleware and session are not recycled; after a
timeout answer a handler's own Save writes the record and sets no cookie.
5. A store without a Codec that finds a codec record starts a new session and
logs a warning, where main answers 500.
6. A store without AbsoluteTimeout honours a stored deadline; once passed,
GetByID no longer finds the session.
7. After a panic unwinds the middleware, a Save from the recovering middleware
saves, and a goroutine can no longer write its cookie into another request.
8. store.Get after the middleware has finished reads the ID from the request
again, or finds the session an earlier store.Get saved.
9. Stores are told apart by every source they read, so a link ?sid=X no longer
makes the cookie stores behind a query store serve X.
10. Once the inner of two stacked session middleware has finished,
session.FromContext(c) returns the outer one, not nil.
11. With several session middleware in a request, csrf keeps its token in the
session of the store it is given as Session, not of the innermost
middleware; with one middleware, or when that store has none, nothing
changes.
Without a Codec, Session.Get and Middleware.Get cost what they do on main, and
the session benchmarks allocate the same or less. Docs: docs/middleware/session.md,
docs/middleware/csrf.md and docs/whats_new.md.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wv2sey8ZLZai81oADq52Re
gaby
force-pushed
the
ccr-ac6228da-p45qep
branch
from
November 6, 2026 15:23
9b6fa29 to
9e89f66
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Six additive, non-breaking improvements to
middleware/session; every new option is off by default. With the default configuration a differential run againstmaingives the same responses and storage calls, apart from the cases under "Behavior that differs frommain".Store.Getissued only from stores that read the same source (cookie, header, query parameter...), so a second store no longer adopts another store's ID.Store.FromContext(c)returns one store's middleware.Config.SkipEmptySessionskeeps a new session without data out of the storage and the response (saveUninitialized: falsein express-session).Config.StorageKeyPrefixandConfig.HashStorageKeyskeep applications that share a storage apart and store each session under the SHA-256 digest of its ID.AbsoluteTimeout; warnings for__Host-/__Secure-cookies that browsers reject and for store options set on the wrongConfig; a typed-nilStoragepanics inNewStore.session.GetAs[V],IdleTimeout()andAbsoluteExpiry().Config.Codecstores each value encoded on its own: a request decodes only what it reads (GetInto,GetAs) and a save encodes only what was set.session.JSONCodec{}andsession.NewCodec(name, marshal, unmarshal). Existing sessions stay gob until they end.Based on
mainat cf50ad3. Lifecycle fixes beyond the five changes asked for stay in this change (differences 4, 6 and 7); their tests, inmiddleware_test.goandstore_test.go, can be reviewed or split out separately. About 10,400 added lines: 2,800 production, 7,400 tests, 240 docs.Fixes: no open issue.
Changes introduced
store_test.go,middleware_test.go,session_test.go,codec_test.go.docs/middleware/session.md,docs/middleware/csrf.md.docs/whats_new.md.SkipEmptySessionsmirrorssaveUninitialized: false.Storeholds atomics now, sogo vetflags copying one by value.Behavior that differs from
mainDetails in
docs/whats_new.md.AbsoluteTimeout: the first cookie saysMax-Age=1799wheremainsays1800when it equalsIdleTimeout.Storagepanics inNewStore, not on the first request.timeoutanswer a handler's ownSavewrites the record and sets no cookie.Codecthat finds a codec record starts a new session and logs a warning, wheremainanswers500.AbsoluteTimeouthonours a stored deadline; once passed,GetByIDno longer finds the session.Savefrom the recovering middleware saves, and a goroutine can no longer write its cookie into another request.store.Getafter the middleware has finished reads the ID from the request again, or finds the session an earlierstore.Getsaved.?sid=Xno longer makes the cookie stores behind a query store serveX.session.FromContext(c)returns the outer one, notnil.csrfkeeps its token in the session of the store it is given asSession, not of the innermost middleware; with one middleware, or when that store has none, nothing changes.Performance
Against
main(cf50ad3), the same benchmark code in both binaries; allocations from-benchmem(5,000 iterations), instructions per iteration from valgrind callgrind (the cost of the benchmark body over 20,000 iterations, GC off, one P, mean of 3 to 6 runs).mainmain-> branchmain-> branchRuns of one binary differ by up to 1,000 instructions, so only the returning-visitor middleware differs from
mainbeyond that spread: it is lower because the branch drops the per-requestConfigcopy and a Go context wrapped twice. Bytes per operation are lower in every row, and allocations in every row but the new visitor. Without aCodec,Session.GetandMiddleware.Getcost what they do onmainto within 2% (344 instructions against 345, and 404 against 397) andSet4 and 7 more (callgrind, mean of three runs).HashStorageKeyscosts about 0.1 µs;SkipEmptySessionsmakes a visitor without a cookie cheaper (2 allocations instead of 19).With a
Codec(Benchmark_Codec_Request, a signed-in user with five values):JSONCodecA codec decodes on every read and encodes on every set, so it loses to gob when a request reads the same struct many times (2.5 times as long at 20 reads), sets one key 1000 times (10 times) or handles a 100 KiB string (5 times); the docs say so under "When gob is the better choice".
Verification
go test ./... -race -shuffle=onpasses (session and csrf also five times in a row, and as 32-bit binaries);go vetis clean for five platforms; lint (with gosec) and markdownlint report nothing.make auditfails onmaintoo, for the same 38 advisories in the go1.26.0 standard library.main(79 scenario groups, 677 requests) gives the same responses and storage calls, apart from difference 2. The four fuzz targets ran 30 s each without a finding.mainadded, that a request which only read does not store its snapshot over a change another request stored meanwhile, and that itsRegeneratecarries that change, holds for a store with aCodecas well (Test_Codec_UnchangedRequestKeepsConcurrentChange); 109 hand-written mutants of the new code are all caught by the tests.go test ./middleware/session ./middleware/csrf -race -count=1 -shuffle=on.Known limitations
All of them are in
docs/middleware/session.md; the ones to know before adopting:StorageKeyPrefixthat, followed bysession:, starts another store's prefix lets a client address that store's records through the shorter one, as onmain: useHashStorageKeys.Codec: a release without this feature answers a codec record with a500; deploy one that has it with the option off everywhere first.csrfcannot use a session store with a codec (its token key is not a string):csrf.Newpanics forConfig.Session.Type of change
Checklist
/docs/directory for Fiber's documentation.🤖 Maintainers:
/generatere-runs the code generators on this branch🤖 Generated with Claude Code
https://claude.ai/code/session_01Wv2sey8ZLZai81oADq52Re