fix(bff): declare max_cookie_size in schema, sweep expired sessions - #167
Conversation
The /oidc/session node sets additionalProperties: false but never declared max_cookie_size, so every shipped cookie-mode exhibit that declares it was refused at boot -- the documentation described a configuration the gateway rejected. Declare the property with only "type": "integer" and a description, exactly mirroring the sibling max_sessions shape. The valid range (40..8192) and the default (4096) are stated in prose only: SealedSessionCookieCodec owns the numerals and ConfigValidator remains the sole enforcing authority, so no minimum/maximum/default keyword is added and no third copy of the bounds is created. Bind the shipped exhibits to the schema so this cannot regress: extend DocumentedSetsContractTest to extract the cookie-mode exhibit from doc/user/bff-cookie.adoc and the annotated gateway.yaml skeleton from doc/configuration.adoc, and validate both through the same com.networknt code path ConfigLoader boots with. A negative control drives an undeclared oidc.session key through that identical path, so the zero-error assertions cannot pass vacuously. Co-Authored-By: Claude <noreply@anthropic.com>
sweepExpired had no trigger. Nothing called it, so an expired session kept its slot indefinitely and a server-mode store could sit permanently full of dead sessions, refusing every login while holding no live one. max_sessions capped accumulated sessions rather than concurrent ones. Give it the one trigger it was missing. SessionStore.create(SessionRecord) becomes create(SessionRecord, Instant now), threading the reference instant that ServerSessionBinding.bind and persist already hold. At the bound, create sweeps once and re-tests: a reclaimed slot admits the session, and a store genuinely full of live sessions still refuses fail-closed with the existing IllegalStateException. An upsert of an already-stored id is admitted without consulting the bound at all, since replacing a record consumes no new capacity — that is the path a rotated session takes. No scheduler, executor, timer thread, or configuration knob is introduced. The javadoc in SessionStore, InMemorySessionStore and package-info claimed a periodic sweep that never existed; it now states the real at-capacity opportunistic trigger. This is a clean break: create(SessionRecord) has no remaining call site. The 32 call sites across 8 test files are migrated mechanically, each passing its own existing reference instant, with assertions unchanged. Three regression cases cover the bound from both sides. The reclaim case drives reclamation through create alone — never calling sweepExpired by hand, which would merely re-test the pre-existing swept-capacity case. The live-session control asserts the bound still refuses when nothing is reclaimable, so reclamation cannot silently become a way around the DoS guard. The upsert case pins the no-new-capacity path at a full store. Co-Authored-By: Claude <noreply@anthropic.com>
The two preceding fixes made shipped documentation false in five places. README.adoc carried a Known Limitations entry stating that cookie mode cannot be configured as documented because max_cookie_size is absent from the bundled schema. The schema now declares it, so the entry is deleted outright rather than softened, and the server-mode entry loses its trailing "subject to the schema limitation above" qualifier. Everything else follows the reclamation change. The old prose said capacity is never reclaimed on its own and told operators to size max_sessions against total logins per process lifetime — advice that was correct only while sweepExpired had no caller. The bound now caps live sessions: a new id arriving at a full store sweeps the expired entries and is admitted if that freed a slot, and a store still full afterwards refuses fail-closed. The sizing advice therefore becomes expected peak concurrency, in README.adoc, doc/user/bff-session.adoc and doc/configuration.adoc's field reference and annotated skeleton. doc/variants/02-bff-session.adoc keeps its load-testing caution but states the real shape of it: successive runs no longer accumulate indefinitely, while a ceiling genuinely full of live sessions still measures the guard rather than the gateway. doc/plan/07-bff-server-session.adoc claimed "a periodic sweep" that never existed. It now names the opportunistic at-capacity trigger and widens the "(no per-session timer threads)" guarantee to deny a scheduler and a periodic sweep as well. The doc/configuration.adoc edit deliberately leaves every literal DocumentedSetsContractTest anchors on untouched — the max_cookie_size skeleton comment and field entry, the extension map, the profile: strict line and the version: 1 opening. A whitespace-tolerant census of "periodic sweep" leaves three hits, each of them a denial adjacent to the real trigger. Co-Authored-By: Claude <noreply@anthropic.com>
…nfig-and-reclaim Delete shouldEnforceMaxBound() from InMemorySessionStoreTest — it became a near-identical duplicate of shouldRefuseWhenBoundIsFullOfLiveSessions() added by this plan. Both fill a 2-session store with live sessions and assert the third create throws; they differ only in the 'now' constant, and since both stored sessions are live at either instant the at-capacity sweep reclaims nothing in both cases. The surviving case is strictly stronger: it advances the clock so the sweep genuinely runs, and additionally asserts size() is untouched by the refusal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RY6KQkkise7WhZKpKZtosh
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesSession capacity and configuration contracts
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
PR Reviewer Guide 🔍(Review updated until commit 12668bb)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/InMemorySessionStore.java (1)
73-93: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRepair secondary indexes on same-ID upserts.
Line [87] replaces the record in
byId, but it does not remove the previous record frombySuborbySid. If an upsert changessuborsid, the old index keeps the same session ID. A laterdestroyBySub(oldSub)ordestroyBySid(oldSid)can remove the replacement record and leave a stale index entry.Deindex the previous record before indexing the replacement. Add a regression test with changed
subandsid; the current upsert test reuses both index keys.ServerSessionBinding.persistrelies on this upsert path.Proposed fix
- byId.put(session.sessionId(), session); + SessionRecord previous = byId.put(session.sessionId(), session); + if (previous != null) { + deindex(bySub, previous.sub(), session.sessionId()); + if (previous.sid() != null) { + deindex(bySid, previous.sid(), session.sessionId()); + } + } index(bySub, session.sub(), session.sessionId());
🧹 Nitpick comments (1)
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/DocumentedSetsContractTest.java (1)
332-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind the negative control to the key it injects.
The assertion accepts any non-empty error list. The control proves rejection today, because
max_cookie_sizesis the only defect in the document. A later schema change that adds an unrelated error — for example a new required key at the root or underoidc— keeps this control green after it stops proving that an undeclaredoidc.sessionkey is refused.Assert that one message points at the injected key. This keeps the control aligned with the "no vacuous pass" discipline the class javadoc states.
♻️ Proposed tightening of the negative control
// Act List<String> errors = validationErrors(document, "the oidc.session negative control"); // Assert - assertFalse(errors.isEmpty(), "the bundled gateway schema accepted 'max_cookie_sizes' under" + assertTrue(errors.stream().anyMatch(error -> error.contains("/oidc/session")), + "the bundled gateway schema accepted 'max_cookie_sizes' under" + " oidc.session, which it does not declare. The two exhibit guards above assert zero" + " errors through this same code path, so without a demonstrated rejection they would" + " pass just as happily against a schema that validates nothing at all. The" - + " oidc.session node sets additionalProperties: false and must refuse an undeclared key"); + + " oidc.session node sets additionalProperties: false and must refuse an undeclared key." + + " The reported errors were: " + errors);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 24e1225f-fcca-428c-a3b4-557233fe7239
📒 Files selected for processing (19)
README.adocapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/InMemorySessionStore.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/ServerSessionBinding.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionStore.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/package-info.javaapi-sheriff/src/main/resources/schema/gateway.schema.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/refresh/TokenRefreshCoordinatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/LoginInitiationEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/LogoutEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/UserInfoEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/runtime/SessionAuthenticationStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/session/InMemorySessionStoreTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/DocumentedSetsContractTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javadoc/configuration.adocdoc/plan/07-bff-server-session.adocdoc/user/bff-session.adocdoc/variants/02-bff-session.adoc
Triage dispositionsIn reply to comment_id:
|
create() replaced the byId entry for an already-stored session id without removing the previous record's bySub/bySid memberships. An upsert that changed sub or sid left the old index pointing at the id, so a later destroyBySub/ destroyBySid on the stale key destroyed the replacement record and reported a phantom deletion count. Capture the previous record from byId.put and deindex its own sub/sid before indexing the replacement. Also restate the sweep trigger as a capacity-consuming create in both the class javadoc and the package-info bullet (a same-id upsert bypasses the sweep), and hoist the instant out of the assertThrows lambda to clear Sonar java:S5778. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RY6KQkkise7WhZKpKZtosh
…jects The control asserted only that the error list was non-empty, so it proved rejection only because max_cookie_sizes was the injected document's single defect. Any later unrelated schema error would have kept it green after it had stopped proving that an undeclared oidc.session key is refused. Assert instead that some reported error points at the injected key. The error renders as "<instanceLocation>: <message>" and that message is emitted in the JVM default locale, so the predicate uses only the two locale-stable parts -- the /oidc/session pointer and the interpolated key name -- and the observed errors are included in the failure message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RY6KQkkise7WhZKpKZtosh
…seam The secondary-index repair hand-rolled a deindex block that was a near-verbatim copy of removeInternal's body, and contradicted the class's own documented invariant that every removal path keeps the indexes consistent through a single removeInternal seam. Collapse it to a removeInternal(sessionId) call ahead of the put — semantically identical (the capacity guard has already run, and the branch it guards only fires when the id is absent, so the call is a no-op there) and one fewer copy of the index-maintenance rule. Also collapse the doubled statement of the sweep-trigger nuance in package-info into a single clause, preserving both asserted facts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RY6KQkkise7WhZKpKZtosh
1 participant
Summary
Closes two independent, currently-shipped defects on the
oidc.sessionconfiguration surface before the 0.1.0 cut.gateway.schema.json's/oidc/sessionnode setsadditionalProperties: falseand omitsmax_cookie_size, so everygateway.yamlthat declares the documented key is hard-rejected before binding — even thoughConfigValidatoralready implements and bounds it.max_sessionsis not the memory guard it is presented as.InMemorySessionStore.createrefused admission onbyId.size() >= maxSessionscounting expired entries, andsweepExpiredhad no production caller — degrading the bound from a concurrency guard into a lifetime login ceiling.Changes
api-sheriff/src/main/resources/schema/gateway.schema.json— declaremax_cookie_sizeon/oidc/sessionas a type-onlyintegerproperty, in the exact shape of its siblingmax_sessions. The40..8192range and4096default live in the description prose, never as machine-enforcedminimum/maximumkeywords, soConfigValidatorstays the single bounds authority and the codec-derived numerals are not copied a third time.api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/DocumentedSetsContractTest.java— extend the existing documentation-vs-code contract test so the shipped cookie-mode andoidcexhibits round-trip through the bundled schema, with a negative control proving the guard is non-vacuous.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionStore.java,InMemorySessionStore.java,ServerSessionBinding.java— wiresweepExpiredto exactly one trigger: an opportunistic sweep at the capacity bound, reached by threading thenowinstant thatServerSessionBindingalready holds throughSessionStore.create.api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/session/InMemorySessionStoreTest.java— regression coverage for the capacity/expiry interaction (fill to the bound, advance past expiry, assert a subsequentcreatesucceeds), plus a live-session control that still fails if the guard were deleted rather than reclaimed.createsignature acrossTokenRefreshCoordinatorTest.java,GatewayEdgeRouteBffWiringTest.java,AuthenticationStageTest.java,SessionAuthenticationStageTest.java,LogoutEndpointTest.java,LoginInitiationEndpointTest.java,UserInfoEndpointTest.java.README.adoc(bothKnown Limitationsentries and the "subject to the schema limitation above" qualifier removed),doc/configuration.adoc,doc/user/bff-session.adoc,doc/variants/02-bff-session.adoc,doc/plan/07-bff-server-session.adoc, and the javadoc inapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/package-info.java. The two documents that contradicted each other onmax_sessionssizing now agree: size for expected concurrency.Test Plan
python3 .plan/execute-script.py plan-marshall:build-maven:maven run --command-args "verify -Ppre-commit")python3 .plan/execute-script.py plan-marshall:build-maven:maven run --command-args "verify")Related Issues
None.
Generated by plan-finalize skill
Intent
Problem. Two shipped defects on
oidc.session. The schema's/oidc/sessionnode isadditionalProperties: falseand omitsmax_cookie_size, so the documented cookie-mode key is hard-rejected before binding despiteConfigValidatoralready implementing it. Separately, themax_sessionscapacity check counted expired entries andsweepExpiredhad no production caller, so the bound behaved as a lifetime login ceiling rather than the memory guard it is documented as.Approach. Minimal and symmetric. The schema gains one type-only property mirroring its sibling
max_sessions: the 40..8192 range and 4096 default stay in description prose, never asminimum/maximumkeywords, soConfigValidatorremains the single bounds authority and the codec-derived numerals are not copied a third time. A contract test round-trips the shipped doc exhibits through the bundled schema so the gap cannot silently reopen.sweepExpiredgets exactly one trigger — an opportunistic sweep at the capacity bound, using thenowinstantServerSessionBindingalready holds, threaded throughSessionStore.create. A scheduled executor was rejected: it leaves a burst able to hit the bound between sweeps, and adds a native-image constraint.Non-goals.
ConfigValidatoris unchanged and stays the bounds authority.SealedSessionCookieCodecis read for constants only. No[Intent truncated — 1386 of 1594 characters shown; full outline in the plan workspace]
Summary by CodeRabbit
New Features
oidc.session.max_cookie_size.Bug Fixes
Documentation