Use cache store for Rack session to prevent OmniAuth callback overflow#1577
Use cache store for Rack session to prevent OmniAuth callback overflow#1577ttt-alexr wants to merge 2 commits intowe-promise:mainfrom
Conversation
Configure session store to use cache store with specific options. Issue 1571 Signed-off-by: ttt-alexr <alex@rubyville.net>
brin-security-scanner
Bot
added
the
contributor:flagged
|
| Dimension | Score | What it measures |
|---|---|---|
| Identity | 30 | Account age, contribution history, GPG keys, org memberships |
| Behavior | 80 | PR patterns, unsolicited contribution ratio, activity cadence |
| Content | 100 | PR body substance, issue linkage, contribution quality |
| Graph | 30 | Cross-repo trust, co-contributor relationships |
Analyzed by Brin · Full profile
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Rails initializer that configures the Rack session store: it derives cache_store type from Rails config, computes session TTL from ENV (default 1 hour), and selects Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6260276ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/initializers/session_store.rb`:
- Around line 10-11: Replace the hard-coded session configuration values by
reading environment variables: don't leave literal values for the session cookie
key and TTL in the initializer—use ENV (e.g. ENV['SESSION_KEY'] with a safe
default like "_sure_session") for the key and parse a TTL env var (e.g.
ENV['SESSION_TTL_HOURS'] or SESSION_TTL_SECONDS) to compute expire_after instead
of the hard-coded 1.hour; update the lines that set key and expire_after in the
session_store initializer (the entries named key and expire_after) to fetch and
coerce the env values with sensible defaults and types.
- Around line 9-13: The app switched sessions to cache_store via
Rails.application.config.session_store which makes volatile keys like
session[:pending_oidc_auth], session[:mfa_user_id], and session[:mobile_sso]
susceptible to Redis evictions/restarts; instead persist those critical flow
states in a durable store (e.g., create DB-backed models such as OidcPendingAuth
or MfaSession to hold the full flow state) and change session usage to only
store a short lookup token/ID; update code paths that set/read
session[:pending_oidc_auth], session[:mfa_user_id], and session[:mobile_sso] to
write/read the DB record and keep only the record ID/token in the Rack session,
or alternatively switch Rails.application.config.session_store to a DB-backed
store (ActiveRecord store) if you want all session data durable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab2b5f17-80c6-4683-925c-a28bfc1ebcf1
📒 Files selected for processing (1)
config/initializers/session_store.rb
| Rails.application.config.session_store :cache_store, | ||
| key: "_sure_session", | ||
| expire_after: 1.hour, | ||
| httponly: true, | ||
| secure: Rails.env.production? |
There was a problem hiding this comment.
Cache-backed session makes multi-step OIDC state volatile.
Line 9 shifts session data to Redis-backed cache; values like session[:pending_oidc_auth], session[:mfa_user_id], and session[:mobile_sso] can be lost on Redis restart/eviction, which can break linking/MFA after callback. Consider persisting these critical flow states in DB (or another durable store) and keeping only a lookup token in Rack session.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/initializers/session_store.rb` around lines 9 - 13, The app switched
sessions to cache_store via Rails.application.config.session_store which makes
volatile keys like session[:pending_oidc_auth], session[:mfa_user_id], and
session[:mobile_sso] susceptible to Redis evictions/restarts; instead persist
those critical flow states in a durable store (e.g., create DB-backed models
such as OidcPendingAuth or MfaSession to hold the full flow state) and change
session usage to only store a short lookup token/ID; update code paths that
set/read session[:pending_oidc_auth], session[:mfa_user_id], and
session[:mobile_sso] to write/read the DB record and keep only the record
ID/token in the Rack session, or alternatively switch
Rails.application.config.session_store to a DB-backed store (ActiveRecord store)
if you want all session data durable.
There was a problem hiding this comment.
Re volatile state under cache_store: this is a valid consideration but I'd argue it's outside the scope of this fix.
The current CookieStore already loses session state on SECRET_KEY_BASE rotation, browser cookie clearing, or any cookie size overflow (which is the bug being fixed). Persisting sensitive flow state in the DB would be an improvement over either store, but it's a refactor of the auth flow, not a session-storage change.
Redis with appendonly enabled (default in most production deployments, including Sure's own Sidekiq usage) survives restarts. The eviction concern only applies if maxmemory-policy is set to evict keys, which would already affect Sidekiq queue health.
There was a problem hiding this comment.
@ttt-alexr, thanks for the update — externalizing the TTL via RACK_SESSION_TTL_HOURS is a good improvement, and keeping _sure_session hard-coded is entirely reasonable (it's an internal implementation detail, not an operator knob).
That said, the core concern in the original comment hasn't been addressed by those changes: the null_store fallback guards dev/test environments, but in production the cache store is Redis, and a Redis restart or LRU eviction can silently drop an in-flight session during the OAuth round-trip or an MFA step. The TTL being 1 hour reduces the window but doesn't eliminate it.
The specific risks remain:
session[:mfa_user_id]evicted mid-flow → user bypasses or is stranded at MFAsession[:pending_oidc_auth]evicted → OIDC callback state lost,state/noncemismatchsession[:mobile_sso]evicted → mobile SSO redirect lost
If Redis is configured with maxmemory-policy noeviction in your production setup, eviction is not a concern (only restarts are, and a 1-hour TTL keeps exposure narrow). It would be worth either:
- Documenting the required Redis
maxmemory-policy(e.g.,noevictionorvolatile-lruwith the session key never expiring early) so future operators don't misconfigure it. - Or persisting only a lookup token in the Rack session and storing the actual flow state in a short-lived DB record, which is fully durable and sidesteps the Redis dependency for correctness.
If option 1 is already covered by your infrastructure/ops runbook, would you like me to open a follow-up issue to track the documentation requirement?
🧠 Learnings used
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-10T13:48:41.488Z
Learning: Applies to config/initializers/simplefin.rb : SimpleFIN runtime toggles are configured via `Rails.configuration.x.simplefin.*` in `config/initializers/simplefin.rb`
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T12:17:21.117Z
Learning: Applies to **/{app/javascript/controllers,app/views}/**/*.{js,erb} : Use query parameters for state management instead of localStorage or session storage
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-10T13:48:41.488Z
Learning: Applies to config/initializers/lunchflow.rb : Lunchflow runtime toggles are configured via `Rails.configuration.x.lunchflow.*` in `config/initializers/lunchflow.rb`
Learnt from: jjmata
Repo: we-promise/sure PR: 593
File: app/controllers/pages_controller.rb:200-203
Timestamp: 2026-01-09T12:25:59.839Z
Learning: Do not compare hard-coded strings to identify the 'Uncategorized Investments' category in Ruby/Rails code (e.g., ct.category.name != "Uncategorized Investments"). Localization/internationalization can change strings. Prefer: (1) a model-level predicate such as ct.category.uncategorized_investment? or (2) compare by identity with a canonical category, e.g., ct.category != Category.uncategorized_investments. This improves maintainability and correctness across translations. Apply this pattern to Ruby files that perform category checks (controllers, models, or elsewhere).
In development without caching enabled and in test, Rails configures the cache store as :null_store, which silently drops writes. Falling back to the cookie store in those environments preserves multi-step flow state (OmniAuth, MFA, mobile SSO) without reintroducing the CookieOverflow risk, since dev/test users are unlikely to have production-scale group claims. Also externalize TTL via RACK_SESSION_TTL_HOURS per coding guidelines. Signed-off-by: ttt-alexr <alex@rubyville.net>
1 participant
Closes #1571.
Summary by CodeRabbit