Skip to content

Use cache store for Rack session to prevent OmniAuth callback overflow #1577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ttt-alexr wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Use cache store for Rack session to prevent OmniAuth callback overflow #1577

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a626027
Set session store to use cache store
ttt-alexr Apr 28, 2026
9d59732
Guard cache-backed session against NullStore environments
ttt-alexr Apr 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions config/initializers/session_store.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Use the Rails cache store (Redis in production) for the framework-level
# Rack session. The application's own user authentication is handled by
# the database-backed Session model (`_session_token`); this session is
# only used by middleware such as OmniAuth during the OIDC callback flow.
#
# Storing it in the cookie (Rails' default) overflows the 4KB limit when
# OmniAuth writes the full auth hash for IdPs that return many group
# claims. See: https://github.com/we-promise/sure/issues/1571
Rails.application.config.session_store :cache_store,
Comment thread
ttt-alexr marked this conversation as resolved.
Outdated
key: "_sure_session",
expire_after: 1.hour,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
httponly: true,
secure: Rails.env.production?
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Copy link
Copy Markdown
Author

@ttt-alexr ttt-alexr Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 MFA
  • session[:pending_oidc_auth] evicted → OIDC callback state lost, state/nonce mismatch
  • session[: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:

  1. Documenting the required Redis maxmemory-policy (e.g., noeviction or volatile-lru with the session key never expiring early) so future operators don't misconfigure it.
  2. 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).