security: session & cookie hardening (fixation, TTL, MFA rate limit, impersonation auth)#1518
security: session & cookie hardening (fixation, TTL, MFA rate limit, impersonation auth)#1518dgilperez wants to merge 9 commits into
Conversation
Call reset_session before create_session_for on successful non-MFA login (both password and OIDC paths). This rotates the session ID and prevents session fixation attacks where an attacker pre-seeds a victim's session cookie before they authenticate. Source: pentest-2026-03-02 commit 0b04682.
After a user updates their password via the reset flow, destroy all their existing Session records so an attacker who has stolen a session cookie cannot continue to access the account once the legitimate user has regained control by resetting the password. Source: pentest-2026-03-02 commit 0b04682.
Set secure: Rails.env.production? and same_site: :lax on the signed session_token cookie in addition to the existing httponly: true. - secure: only transmit the cookie over HTTPS in production, preventing interception on the wire. - same_site: :lax: mitigates CSRF attacks by not sending the session cookie on cross-site subrequests while still allowing top-level nav. Matches finding PT-002 (pentest 2026-02-24) and C1 from issue #1087. Source: pentest-2026-03-02 commits d06290a + 3e6ab5b.
Harden the MFA verification flow after the first factor succeeds: - Track mfa_attempts in the session and redirect back to new_session_path after 5 wrong codes, forcing re-authentication of the first factor. - Track mfa_started_at when entering MFA flow; expire the pending MFA session after 5 minutes to bound the attack window. - Call reset_session on successful verify_code before create_session_for to prevent session fixation on the MFA login path (parallel to FIX-01 on the non-MFA path). Initialize mfa_started_at and mfa_attempts in the sessions#create MFA branch so the TTL and counter are always well-defined. Source: pentest-2026-03-02 commit d06290a.
Before PT-005, any authenticated user could end any in-progress impersonation session by calling the complete action with an arbitrary session id, because set_impersonation_session scoped lookup was the only authorization. Require Current.true_user to be either the impersonator or the impersonated party of the target session; otherwise raise a 404 via raise_unauthorized! in line with the other actions. Source: pentest-2026-03-02 commit d06290a + 43a04d8 (adjusted to allow both participants, matching real UX).
Sessions previously never expired server-side (CWE-613). Add server-side enforcement in find_session_by_cookie: - SESSION_ABSOLUTE_TTL (30 days): sessions older than this by created_at are destroyed and the cookie is cleared on the next request regardless of activity. - SESSION_IDLE_TTL (24 hours): sessions whose updated_at is older than this are destroyed and the cookie is cleared. - On every authenticated request we Session#touch to refresh the idle timer so active users stay logged in. Source: pentest-2026-03-02 commit 14927f7.
set_impersonation_session already scopes by Current.true_user's own impersonator/impersonated associations, so the record is nil for unrelated callers. Without this guard, the participant check crashed with NoMethodError for nil:NilClass instead of returning 404. Raise not-found first when the record is missing, then apply the participant check as defense-in-depth.
superagent-security
Bot
added
the
contributor:verified
|
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)
📝 WalkthroughWalkthroughEnforces session TTLs (30-day absolute, 24-hour idle), validates/deletes missing session cookies, prevents session fixation by resetting sessions while preserving pending-invitation tokens, adds MFA attempt/TTL safeguards, invalidates user sessions on password reset, and tightens impersonation completion authorization. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SessionsController
participant AuthenticationConcern
participant Session
participant MfaController
User->>SessionsController: POST /sessions (credentials)
SessionsController->>AuthenticationConcern: reset_session_preserving_pending_invitation
alt User requires MFA
SessionsController->>SessionsController: set session[:mfa_started_at], session[:mfa_attempts]=0
SessionsController-->>User: Redirect to /verify_mfa
User->>MfaController: POST /verify_mfa (OTP)
MfaController->>MfaController: increment session[:mfa_attempts]
alt Attempts > 5
MfaController->>MfaController: clear mfa keys
MfaController-->>User: Redirect to /sessions (too many attempts)
else TTL expired (>5m)
MfaController->>MfaController: clear mfa keys
MfaController-->>User: Redirect to /sessions (session expired)
else Valid OTP
MfaController->>AuthenticationConcern: reset_session_preserving_pending_invitation
AuthenticationConcern->>Session: create Session, set signed cookie (secure?, SameSite=Lax)
MfaController-->>User: Redirect to home (authenticated)
end
else No MFA required
SessionsController->>AuthenticationConcern: create_session_for
AuthenticationConcern->>Session: create Session, set signed cookie
SessionsController-->>User: Redirect to home (authenticated)
end
sequenceDiagram
actor Client
participant App
participant AuthenticationConcern
participant Session
Client->>App: GET /protected
App->>AuthenticationConcern: find_session_by_cookie
AuthenticationConcern->>Session: query by token
alt no cookie or no DB row
AuthenticationConcern-->>App: nil (delete cookie)
App-->>Client: Redirect to /sessions
else created_at > 30 days
AuthenticationConcern->>Session: destroy
AuthenticationConcern-->>App: nil (delete cookie)
App-->>Client: Redirect to /sessions
else updated_at > 24 hours
AuthenticationConcern->>Session: destroy
AuthenticationConcern-->>App: nil (delete cookie)
App-->>Client: Redirect to /sessions
else active
AuthenticationConcern->>Session: touch (if updated_at > 1 minute)
AuthenticationConcern-->>App: return session
App-->>Client: serve request (authenticated)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: d8c6520134
ℹ️ 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".
| session[:mfa_user_id] = user.id | ||
| redirect_to verify_mfa_path | ||
| else | ||
| reset_session # Prevent session fixation (FIX-01) |
There was a problem hiding this comment.
openid_connect stores session[:id_token_hint] and session[:sso_login_provider] immediately before this branch, but reset_session clears those values. SessionsController#destroy depends on those keys to build the IdP end-session URL, so OIDC users who log in through this path lose federated logout and only get local logout. Stash and restore the logout keys across session rotation.
Useful? React with 👍 / 👎.
| reset_session # Prevent session fixation (FIX-01) | ||
| @session = create_session_for(user) | ||
| flash[:notice] = t("invitations.accept_choice.joined_household") if accept_pending_invitation_for(user) |
There was a problem hiding this comment.
accept_pending_invitation_for reads session[:pending_invitation_token], which is set during SessionsController#new. Rotating the Rails session here clears that token before accept_pending_invitation_for(user) runs, so invited users can authenticate but fail to auto-join the household. Preserve this token across reset_session (same pattern applies to other login branches that reset before acceptance).
Useful? React with 👍 / 👎.
| end | ||
|
|
||
| # TTL: MFA flow expires after 5 minutes (PT-003) | ||
| if session[:mfa_started_at].present? && Time.current - Time.parse(session[:mfa_started_at].to_s) > 5.minutes |
There was a problem hiding this comment.
The timeout check only executes when session[:mfa_started_at] is present. In this commit, only the local password login path initializes mfa_started_at; OIDC MFA entry points still set only mfa_user_id, so this condition is skipped and the 5-minute MFA expiry is not enforced for those users. Initialize mfa_started_at in every MFA-start flow or treat a missing timestamp as expired.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/sessions_controller.rb (1)
70-75:⚠️ Potential issue | 🟠 Major
reset_sessionclears pending invitation data before it can be accepted.
accept_pending_invitation_forreadssession[:pending_invitation_token], which is populated bystore_pending_invitation_if_validin the login flow. However,reset_session(lines 71, 189 in sessions_controller; line 55 in mfa_controller) clears the entire session beforeaccept_pending_invitation_foris called, so the invitation token is lost and invitations never auto-link to users.The OIDC path in
oidc_accounts_controlleravoids this because it does not callreset_session, allowing invitations to work there.Fix: Capture the token before
reset_session:token = session[:pending_invitation_token] reset_session session[:pending_invitation_token] = tokenOr call
accept_pending_invitation_forbeforereset_session, then delete the token after.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/sessions_controller.rb` around lines 70 - 75, The session reset is clearing session[:pending_invitation_token] before accept_pending_invitation_for can read it; update the login flow in sessions_controller (and the analogous place in mfa_controller) to preserve the pending invitation token across reset_session — e.g. read session[:pending_invitation_token] into a local variable, call reset_session, then restore it to session[:pending_invitation_token] before calling accept_pending_invitation_for (alternatively call accept_pending_invitation_for(user) before reset_session and then remove the token), ensuring store_pending_invitation_if_valid remains the source of the token.
🧹 Nitpick comments (3)
app/controllers/concerns/authentication.rb (1)
41-59: Write amplification:session.touchon every authenticated request.
session.touchissues an UPDATE against thesessionstable on every request from every authenticated user. On a busy deployment this becomes a hot write path and also produces high replica lag / WAL churn. Consider only refreshing the idle timer when it is actually close to expiring (e.g., once per minute).♻️ Proposed throttled touch
- # Touch to refresh idle timer on each request - session.touch - session + # Refresh idle timer at most once per minute to avoid per-request writes + if session.updated_at < now - 1.minute + session.touch + end + session🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/concerns/authentication.rb` around lines 41 - 59, The hot-write comes from always calling session.touch; instead introduce a throttle so we only touch when the session's updated_at is sufficiently old (e.g., define SESSION_TOUCH_THROTTLE = 1.minute) and replace the unconditional session.touch with a conditional that calls session.touch only if session.updated_at < now - SESSION_TOUCH_THROTTLE (keep the existing absolute/idle TTL checks using SESSION_ABSOLUTE_TTL and SESSION_IDLE_TTL), ensuring the symbol session.touch is preserved and tests/behaviour around idle expiry are updated to account for the throttled refresh.app/controllers/mfa_controller.rb (1)
43-55: Two small refinements in the verify_code flow.
Time.parse(session[:mfa_started_at].to_s)will raiseArgumentErrorif the stored value is ever malformed (tampered cookie, legacy format, partial write). Sincereset_sessionat login sets it to a knowniso8601value, prefer a non-raising parse and treat any failure as "expired" so the user is safely redirected instead of hitting a 500.Lines 52–54 delete the three MFA session keys individually and then line 55 calls
reset_session, which wipes the entire Rails session. The three explicit deletes are dead work — keepreset_sessionand drop the deletes (or dropreset_sessionand keep only the targeted deletes; the former is safer against fixation and is what the comment promises).♻️ Proposed fix
- # TTL: MFA flow expires after 5 minutes (PT-003) - if session[:mfa_started_at].present? && Time.current - Time.parse(session[:mfa_started_at].to_s) > 5.minutes + # TTL: MFA flow expires after 5 minutes (PT-003) + started_at = Time.zone.parse(session[:mfa_started_at].to_s) rescue nil + if session[:mfa_started_at].present? && (started_at.nil? || Time.current - started_at > 5.minutes) session.delete(:mfa_user_id) session.delete(:mfa_attempts) session.delete(:mfa_started_at) redirect_to new_session_path, alert: t(".session_expired", default: "MFA session expired. Please sign in again.") return end if `@user`&.verify_otp?(params[:code]) - session.delete(:mfa_user_id) - session.delete(:mfa_attempts) - session.delete(:mfa_started_at) reset_session # Prevent session fixation (PT-003) `@session` = create_session_for(`@user`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/mfa_controller.rb` around lines 43 - 55, Replace the brittle Time.parse(session[:mfa_started_at].to_s) call with a non-raising parse (e.g. Time.iso8601(session[:mfa_started_at].to_s) or Time.zone.parse wrapped in rescue -> nil) and treat a failed/nil parse as expired so the flow redirects instead of raising; also remove the three explicit session.delete(:mfa_user_id)/(:mfa_attempts)/(:mfa_started_at) lines when you call reset_session (keep reset_session for fixation protection) so you don't perform dead work after reset_session in the verify flow that uses session[:mfa_started_at] and `@user.verify_otp`?(params[:code]).test/controllers/mfa_controller_test.rb (1)
138-151: Strengthen TTL assertion to rule out a false-positive pass.A valid TOTP is submitted here, so the only reason this redirect fires is that the TTL branch clears
mfa_user_idbefore verification. If that branch regressed (say, was placed afterverify_otp?), a successful authentication could also end onnew_session_pathvia other flows and still satisfyassert_redirected_to. Add a negative assertion to pin the intent.♻️ Proposed stronger assertion
travel_to(6.minutes.from_now) do post verify_mfa_path, params: { code: ROTP::TOTP.new(other_user.otp_secret).now } assert_redirected_to new_session_path + assert_not Session.exists?(user_id: other_user.id), "No auth session should be created after TTL expiry" end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/mfa_controller_test.rb` around lines 138 - 151, The test "MFA session expires after TTL" currently only asserts the redirect which can be satisfied by multiple flows; after the post to verify_mfa_path add a negative assertion that the MFA session was cleared (e.g. assert_nil session[:mfa_user_id] or assert_not session.key?(:mfa_user_id)) to ensure the TTL branch actually removed mfa_user_id for other_user rather than another successful auth path satisfying assert_redirected_to new_session_path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/concerns/authentication.rb`:
- Around line 38-39: When a signed :session_token cookie exists but
Session.find_by(id: cookie_value) returns nil, delete the stale cookie instead
of just returning nil; in the method where cookie_value is assigned and you call
Session.find_by(id: cookie_value), call the cookies.delete(:session_token) (or
equivalent delete for signed cookies) before returning nil so the browser no
longer resends the stale token on subsequent requests.
In `@app/controllers/impersonation_sessions_controller.rb`:
- Around line 35-43: The complete action marks the ImpersonationSession complete
but doesn't clear the browser's active impersonation reference; update the
complete method (in impersonation_sessions_controller#complete) to also clear
Current.session.active_impersonator_session (or call the same helper used by
leave) after `@impersonation_session.complete`! so the Current.session no longer
references the finished `@impersonation_session`; keep the existing authorization
checks and redirect/notice behavior.
In `@test/controllers/sessions_controller_test.rb`:
- Around line 684-691: The test currently can pass spuriously because the
pre-auth session may be empty; seed a known sentinel into the pre-auth session
(e.g. set session[:pre_auth_sentinel] = "seed" before calling sign_in in the
"session fixation: new session created on login (reset_session)" test), then
perform sign_in `@user` and assert that the sentinel is gone (assert_nil
session[:pre_auth_sentinel]) and that session.id changed if desired; update the
test body around get new_session_url, old_session_id = session.id, sign_in `@user`
to set the sentinel before sign_in and replace the weak assert_not_equal check
with the stronger sentinel disappearance assertion (and keep the session.id
inequality check if you still want to verify id rotation).
---
Outside diff comments:
In `@app/controllers/sessions_controller.rb`:
- Around line 70-75: The session reset is clearing
session[:pending_invitation_token] before accept_pending_invitation_for can read
it; update the login flow in sessions_controller (and the analogous place in
mfa_controller) to preserve the pending invitation token across reset_session —
e.g. read session[:pending_invitation_token] into a local variable, call
reset_session, then restore it to session[:pending_invitation_token] before
calling accept_pending_invitation_for (alternatively call
accept_pending_invitation_for(user) before reset_session and then remove the
token), ensuring store_pending_invitation_if_valid remains the source of the
token.
---
Nitpick comments:
In `@app/controllers/concerns/authentication.rb`:
- Around line 41-59: The hot-write comes from always calling session.touch;
instead introduce a throttle so we only touch when the session's updated_at is
sufficiently old (e.g., define SESSION_TOUCH_THROTTLE = 1.minute) and replace
the unconditional session.touch with a conditional that calls session.touch only
if session.updated_at < now - SESSION_TOUCH_THROTTLE (keep the existing
absolute/idle TTL checks using SESSION_ABSOLUTE_TTL and SESSION_IDLE_TTL),
ensuring the symbol session.touch is preserved and tests/behaviour around idle
expiry are updated to account for the throttled refresh.
In `@app/controllers/mfa_controller.rb`:
- Around line 43-55: Replace the brittle
Time.parse(session[:mfa_started_at].to_s) call with a non-raising parse (e.g.
Time.iso8601(session[:mfa_started_at].to_s) or Time.zone.parse wrapped in rescue
-> nil) and treat a failed/nil parse as expired so the flow redirects instead of
raising; also remove the three explicit
session.delete(:mfa_user_id)/(:mfa_attempts)/(:mfa_started_at) lines when you
call reset_session (keep reset_session for fixation protection) so you don't
perform dead work after reset_session in the verify flow that uses
session[:mfa_started_at] and `@user.verify_otp`?(params[:code]).
In `@test/controllers/mfa_controller_test.rb`:
- Around line 138-151: The test "MFA session expires after TTL" currently only
asserts the redirect which can be satisfied by multiple flows; after the post to
verify_mfa_path add a negative assertion that the MFA session was cleared (e.g.
assert_nil session[:mfa_user_id] or assert_not session.key?(:mfa_user_id)) to
ensure the TTL branch actually removed mfa_user_id for other_user rather than
another successful auth path satisfying assert_redirected_to new_session_path.
🪄 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: 4eb289a7-920a-4e70-a549-815ef70daaf2
📒 Files selected for processing (9)
app/controllers/concerns/authentication.rbapp/controllers/impersonation_sessions_controller.rbapp/controllers/mfa_controller.rbapp/controllers/password_resets_controller.rbapp/controllers/sessions_controller.rbtest/controllers/impersonation_sessions_controller_test.rbtest/controllers/mfa_controller_test.rbtest/controllers/password_resets_controller_test.rbtest/controllers/sessions_controller_test.rb
Major: - reset_session clearing pending_invitation_token: added reset_session_preserving_pending_invitation helper in Authentication concern and use it at all 3 privilege-change sites (two in sessions_controller, one in mfa_controller). Previously the token was wiped before accept_pending_invitation_for could read it, breaking invitation auto-link for any flow that went through local/super-admin login or MFA verify. - Stale session cookie: when cookies.signed[:session_token] references a deleted Session row, delete the cookie so the browser stops resending it. - Impersonation complete: clear Current.session.active_impersonator_session when it still points at the now-completed ImpersonationSession (previously the UI kept the banner/true_user mapping until page reload). Refinements: - session.touch throttled to max once per minute (SESSION_TOUCH_THROTTLE) to avoid a per-request write against the sessions table. Idle-TTL granularity is unaffected in practice. - mfa_controller verify_code: non-raising parse of :mfa_started_at (malformed values now redirect instead of 500), and drop the three redundant session.delete calls in the success path — reset_session already wipes them. Tests: - session fixation test seeds a pre-auth sentinel and asserts it was wiped, replacing the tautological "session.id changed" check. - new test: pending_invitation_token preserved across reset_session (goes through the real store_pending_invitation_if_valid path with a fixture). - MFA TTL test: negative assertion that no auth Session row is created when the OTP arrives past the TTL window.
|
Thanks — addressed all seven points. Major — Stale session cookie ( Impersonation complete keeps dangling pointer (
MFA verify brittleness + dead deletes (
Session fixation test tautological ( MFA TTL test spurious pass ( All changes in commit — wolfgang 🤖 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/controllers/concerns/authentication.rb (2)
30-36: TTL constants: consider making them environment-configurable.Hard-coded 30-day / 24-hour TTLs are reasonable defaults, but operators running self-hosted or compliance-sensitive deployments typically want to tune session lifetime without patching. Exposing these via ENV (with the current values as fallbacks) would align with the guideline to prefer environment variables over hard-coded configuration values, and makes incident response (forcing shorter TTLs) easier.
♻️ Optional refactor
- SESSION_ABSOLUTE_TTL = 30.days - SESSION_IDLE_TTL = 24.hours - SESSION_TOUCH_THROTTLE = 1.minute + SESSION_ABSOLUTE_TTL = (ENV["SESSION_ABSOLUTE_TTL_DAYS"]&.to_i&.days) || 30.days + SESSION_IDLE_TTL = (ENV["SESSION_IDLE_TTL_HOURS"]&.to_i&.hours) || 24.hours + SESSION_TOUCH_THROTTLE = 1.minuteAs per coding guidelines: "Use environment variables instead of hard-coded values in configuration."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/concerns/authentication.rb` around lines 30 - 36, Replace the hard-coded TTL constants (SESSION_ABSOLUTE_TTL, SESSION_IDLE_TTL, SESSION_TOUCH_THROTTLE) with values read from environment variables (e.g. ENV.fetch('SESSION_ABSOLUTE_TTL_DAYS', '30').to_i.days, ENV.fetch('SESSION_IDLE_TTL_HOURS', '24').to_i.hours, ENV.fetch('SESSION_TOUCH_THROTTLE_SECONDS', '60').to_i.seconds) so operators can override defaults; ensure parsing/conversion to ActiveSupport::Duration is applied and fallbacks preserve current behavior if the ENV vars are absent or invalid, and keep the same constant names so existing code (e.g. methods referencing SESSION_ABSOLUTE_TTL) continues to work.
82-91: Userequest.ssl?to enable secure flag for HTTPS environments beyond production.The
secure: Rails.env.production?check fails to set theSecureflag on thesession_tokencookie in staging, Codespaces, review apps, and self-hosted deployments running over HTTPS with a non-production Rails environment. This is inconsistent withconfig/initializers/codespaces.rb, which setssecure: trueon the Rails session store for Codespaces (even though it's a development environment) precisely because Codespaces is HTTPS-only.The
request.ssl?check reliably detects the actual TLS status of the current request and will properly secure the cookie wherever HTTPS is in use, regardless of environment or deployment type.♻️ Suggested change
cookies.signed.permanent[:session_token] = { value: session.id, httponly: true, - secure: Rails.env.production?, + secure: request.ssl?, same_site: :lax }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/concerns/authentication.rb` around lines 82 - 91, The secure flag on the session cookie is currently tied to Rails.env.production? which misses HTTPS deployments outside production; in create_session_for change the cookie options so secure is determined by the actual request TLS status (use request.ssl? or equivalent) instead of environment; update the cookies.signed.permanent[:session_token] options (the hash with value, httponly, secure, same_site) to set secure: request.ssl? so staging/Codespaces/self-hosted HTTPS requests get Secure set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/controllers/concerns/authentication.rb`:
- Around line 30-36: Replace the hard-coded TTL constants (SESSION_ABSOLUTE_TTL,
SESSION_IDLE_TTL, SESSION_TOUCH_THROTTLE) with values read from environment
variables (e.g. ENV.fetch('SESSION_ABSOLUTE_TTL_DAYS', '30').to_i.days,
ENV.fetch('SESSION_IDLE_TTL_HOURS', '24').to_i.hours,
ENV.fetch('SESSION_TOUCH_THROTTLE_SECONDS', '60').to_i.seconds) so operators can
override defaults; ensure parsing/conversion to ActiveSupport::Duration is
applied and fallbacks preserve current behavior if the ENV vars are absent or
invalid, and keep the same constant names so existing code (e.g. methods
referencing SESSION_ABSOLUTE_TTL) continues to work.
- Around line 82-91: The secure flag on the session cookie is currently tied to
Rails.env.production? which misses HTTPS deployments outside production; in
create_session_for change the cookie options so secure is determined by the
actual request TLS status (use request.ssl? or equivalent) instead of
environment; update the cookies.signed.permanent[:session_token] options (the
hash with value, httponly, secure, same_site) to set secure: request.ssl? so
staging/Codespaces/self-hosted HTTPS requests get Secure set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbfc664f-ce7e-4395-a90d-c5dfd2b8aadd
📒 Files selected for processing (6)
app/controllers/concerns/authentication.rbapp/controllers/impersonation_sessions_controller.rbapp/controllers/mfa_controller.rbapp/controllers/sessions_controller.rbtest/controllers/mfa_controller_test.rbtest/controllers/sessions_controller_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/controllers/sessions_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- test/controllers/mfa_controller_test.rb
- app/controllers/mfa_controller.rb
- app/controllers/sessions_controller.rb
- app/controllers/impersonation_sessions_controller.rb
2 participants
Summary
Session lifecycle, cookie security attributes, MFA flow rate limiting, and impersonation authorization.
Part of the security hardening originally bundled in #1104 (closed), now split by functional area per @jjmata's feedback. Companion PRs: #1516 #1517 #1519 #1520 #1521.
Findings addressed (7)
reset_sessionadded insessions_controller.rb(all login paths incl. OIDC)password_resets_controller.rbSESSION_ABSOLUTE_TTL = 30.days+SESSION_IDLE_TTL = 24.hoursenforced inconcerns/authentication.rbauthentication.rb:secure: Rails.env.production?,same_site: :laxreset_sessionon verify inmfa_controller.rbMigration notes
Existing long-lived sessions will be invalidated on first request after deploy (expected — was the goal).
Tests
sessions_controller_test.rb,mfa_controller_test.rb,password_resets_controller_test.rb,impersonation_sessions_controller_test.rb.bin/rubocopclean.Out of scope
Related
Summary by CodeRabbit
Bug Fixes
Improvements
Tests