security: session & cookie hardening (fixation, TTL, MFA rate limit, impersonation auth)

app/controllers/concerns/authentication.rb

@@ -27,19 +27,66 @@ def authenticate_user!
       end
     end
 
+    # Session TTL constants (F-04, CWE-613)
+    SESSION_ABSOLUTE_TTL = 30.days
+    SESSION_IDLE_TTL = 24.hours
+    # Throttle the per-request `updated_at` touch so every authenticated request
+    # doesn't issue a write against `sessions`. 1 minute is granular enough for
+    # the 24h idle TTL while keeping write amplification bounded.
+    SESSION_TOUCH_THROTTLE = 1.minute
+
     def find_session_by_cookie
       cookie_value = cookies.signed[:session_token]
+      return nil unless cookie_value.present?
 
-      if cookie_value.present?
-        Session.find_by(id: cookie_value)
-      else
-        nil
+      session = Session.find_by(id: cookie_value)
+      unless session
+        # Stale cookie (session row was deleted or recycled) — remove it so the
+        # browser doesn't keep resending a dead token.
+        cookies.delete(:session_token)
+        return nil
+      end
+
+      now = Time.current
+
+      # Absolute TTL: session older than 30 days is always expired
+      if session.created_at < now - SESSION_ABSOLUTE_TTL
+        session.destroy
+        cookies.delete(:session_token)
+        return nil
       end
+
+      # Idle TTL: session not used in 24h is expired
+      if session.updated_at < now - SESSION_IDLE_TTL
+        session.destroy
+        cookies.delete(:session_token)
+        return nil
+      end
+
+      # Refresh idle timer, throttled to avoid a write per request.
+      session.touch if session.updated_at < now - SESSION_TOUCH_THROTTLE
+      session
+    end
+
+    # Resets the Rails session to prevent session fixation on privilege change
+    # (login, MFA verify) while preserving the pending invitation token stored
+    # pre-login. `reset_session` clears everything by default, which would drop
+    # a legitimate pending invitation before `accept_pending_invitation_for`
+    # could use it.
+    def reset_session_preserving_pending_invitation
+      pending_invitation = session[:pending_invitation_token]
+      reset_session
+      session[:pending_invitation_token] = pending_invitation if pending_invitation
     end
 
     def create_session_for(user)
       session = user.sessions.create!
-      cookies.signed.permanent[:session_token] = { value: session.id, httponly: true }
+      cookies.signed.permanent[:session_token] = {
+        value: session.id,
+        httponly: true,
+        secure: Rails.env.production?,
+        same_site: :lax
+      }
       session
     end
 

app/controllers/impersonation_sessions_controller.rb

@@ -33,7 +33,20 @@ def reject
   end
 
   def complete
+    # PT-005: Only the impersonator or the impersonated user may end the session.
+    # set_impersonation_session already scopes by Current.true_user, so a missing
+    # record means the caller is not a participant; this check also defends
+    # against future changes to the scope.
+    raise_unauthorized! if @impersonation_session.nil?
+    raise_unauthorized! unless [ @impersonation_session.impersonator, @impersonation_session.impersonated ].include?(Current.true_user)
     @impersonation_session.complete!
+    # Clear the active_impersonator_session reference on the caller's browser
+    # session when it still points at this (now completed) record. Without
+    # this, Current.session keeps a dangling pointer and impersonation-aware UI
+    # (banner, Current.user resolution) continues as if the session were active.
+    if Current.session&.active_impersonator_session_id == @impersonation_session.id
+      Current.session.update!(active_impersonator_session: nil)
+    end
     redirect_to root_path, notice: t(".success")
   end
 

app/controllers/mfa_controller.rb

@@ -29,8 +29,36 @@ def verify
   def verify_code
     @user = User.find_by(id: session[:mfa_user_id])
 
-    if @user&.verify_otp?(params[:code])
+    # Rate limit: max 5 attempts, then force re-login (PT-003)
+    session[:mfa_attempts] = (session[:mfa_attempts] || 0) + 1
+    if session[:mfa_attempts] > 5
+      session.delete(:mfa_user_id)
+      session.delete(:mfa_attempts)
+      session.delete(:mfa_started_at)
+      redirect_to new_session_path, alert: t(".too_many_attempts", default: "Too many attempts. Please sign in again.")
+      return
+    end
+
+    # TTL: MFA flow expires after 5 minutes (PT-003)
+    # Use a non-raising parse so a tampered/legacy value redirects the user
+    # cleanly instead of producing a 500.
+    started_at = begin
+      Time.zone.parse(session[:mfa_started_at].to_s)
+    rescue ArgumentError, TypeError
+      nil
+    end
+    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])
+      # reset_session clears the mfa_* keys (and everything else) — keep the
+      # pending invitation token so post-MFA login can still honour the invite.
+      reset_session_preserving_pending_invitation # FIX-01 / PT-003
       @session = create_session_for(@user)
       flash[:notice] = t("invitations.accept_choice.joined_household") if accept_pending_invitation_for(@user)
       redirect_to root_path

app/controllers/password_resets_controller.rb

@@ -38,6 +38,8 @@ def update
     end
 
     if @user.update(password_params)
+      # Invalidate all existing sessions after password reset (FIX-13)
+      @user.sessions.destroy_all
       redirect_to new_session_path, notice: t(".success")
     else
       render :edit, status: :unprocessable_entity

app/controllers/sessions_controller.rb

@@ -63,9 +63,12 @@ def create
       if user.otp_required?
         log_super_admin_override_login(user)
         session[:mfa_user_id] = user.id
+        session[:mfa_started_at] = Time.current.iso8601
+        session[:mfa_attempts] = 0
         redirect_to verify_mfa_path
       else
         log_super_admin_override_login(user)
+        reset_session_preserving_pending_invitation # FIX-01 + preserve invitation token
         @session = create_session_for(user)
         flash[:notice] = t("invitations.accept_choice.joined_household") if accept_pending_invitation_for(user)
         redirect_to root_path
@@ -183,6 +186,7 @@ def openid_connect
         session[:mfa_user_id] = user.id
         redirect_to verify_mfa_path
       else
+        reset_session_preserving_pending_invitation # FIX-01 + preserve invitation token
         @session = create_session_for(user)
         flash[:notice] = t("invitations.accept_choice.joined_household") if accept_pending_invitation_for(user)
         redirect_to root_path

test/controllers/impersonation_sessions_controller_test.rb

@@ -109,4 +109,16 @@ class ImpersonationSessionsControllerTest < ActionDispatch::IntegrationTest
     assert_equal "rejected", impersonator_session.reload.status
     assert_redirected_to root_path
   end
+
+  # PT-005: Only the impersonator or the impersonated may complete.
+  test "unrelated user cannot complete another impersonation session" do
+    sign_in users(:family_admin)
+
+    impersonator_session = impersonation_sessions(:in_progress)
+
+    put complete_impersonation_session_path(impersonator_session)
+
+    assert_response :not_found
+    assert_equal "in_progress", impersonator_session.reload.status
+  end
 end

test/controllers/mfa_controller_test.rb

@@ -116,4 +116,49 @@ def sign_out
     assert_nil @user.otp_secret
     assert_empty @user.otp_backup_codes
   end
+
+  # ── Security tests added with pentest fixes (PT-003) ─────────────────────────
+
+  test "MFA verify_code rate-limits after 5 attempts" do
+    other_user = users(:family_admin)
+    other_user.setup_mfa!
+    other_user.enable_mfa!
+
+    sign_out
+    # Simulate partial login with MFA pending
+    post sessions_path, params: { email: other_user.email, password: user_password_test }
+
+    6.times do
+      post verify_mfa_path, params: { code: "000000" }
+    end
+
+    assert_redirected_to new_session_path
+  end
+
+  test "MFA session expires after TTL" do
+    other_user = users(:family_admin)
+    other_user.setup_mfa!
+    other_user.enable_mfa!
+
+    sign_out
+    post sessions_path, params: { email: other_user.email, password: user_password_test }
+
+    # Travel past the 5 minute TTL window
+    travel_to(6.minutes.from_now) do
+      # Capture how many sessions the user had before the MFA attempt so we
+      # can assert no new one was created. Any existing session from sign-in
+      # below is from the sign_out/post cycle above (which creates an MFA
+      # handoff but no auth session yet).
+      before = Session.where(user_id: other_user.id).count
+
+      post verify_mfa_path, params: { code: ROTP::TOTP.new(other_user.otp_secret).now }
+
+      assert_redirected_to new_session_path
+      # Negative assertion: the TTL branch cleared the MFA handoff and the OTP
+      # was NOT accepted as a legitimate login.
+      assert_nil session[:mfa_user_id], "MFA handoff should be cleared on TTL expiry"
+      assert_equal before, Session.where(user_id: other_user.id).count,
+        "No auth session should be created when MFA expires"
+    end
+  end
 end

test/controllers/password_resets_controller_test.rb

@@ -87,4 +87,17 @@ class PasswordResetsControllerTest < ActionDispatch::IntegrationTest
     sso_user.reload
     assert_nil sso_user.password_digest, "SSO-only user should still have nil password_digest"
   end
+
+  # Security: Password reset should invalidate all active sessions (FIX-13)
+  test "update invalidates all existing sessions" do
+    sign_in @user
+    session_count_before = @user.reload.sessions.count
+    assert session_count_before >= 1
+
+    token = @user.generate_token_for(:password_reset)
+    patch password_reset_path(token: token),
+      params: { user: { password: "NewPassword123!", password_confirmation: "NewPassword123!" } }
+
+    assert_equal 0, @user.reload.sessions.count, "All sessions should be destroyed after password reset"
+  end
 end

test/controllers/sessions_controller_test.rb

@@ -678,4 +678,61 @@ def setup_omniauth_mock(provider:, uid:, email:, name:, first_name: nil, last_na
     follow_redirect!
     assert_response :success
   end
+
+  # ── Security tests added with pentest fixes ──────────────────────────────────
+
+  test "session fixation: new session created on login (reset_session)" do
+    get new_session_url # establish session
+    old_session_id = session.id
+    # Seed a sentinel to prove the old session was actually cleared, not just
+    # that session.id happened to differ.
+    session[:pre_auth_sentinel] = "seed"
+
+    sign_in @user
+
+    assert_nil session[:pre_auth_sentinel], "Pre-auth session data should be cleared by reset_session"
+    assert_not_equal old_session_id, session.id, "Session ID should change on login to prevent fixation"
+  end
+
+  test "session fixation: pending invitation token survives reset_session" do
+    invitation = invitations(:one)
+
+    # GET /sessions/new?invitation=<token> triggers store_pending_invitation_if_valid,
+    # which puts the token into the session BEFORE the POST that calls reset_session.
+    get new_session_url(invitation: invitation.token)
+
+    # Signing in as a user whose email doesn't match the invitation means
+    # accept_pending_invitation_for short-circuits without consuming the token,
+    # so the post-reset session should still carry it — which can only happen
+    # if reset_session_preserving_pending_invitation restored it.
+    sign_in @user
+
+    assert_equal invitation.token, session[:pending_invitation_token],
+      "Token must be preserved across reset_session so accept_pending_invitation_for can honor it"
+  end
+
+  # F-04: Session TTL — absolute (30d) and idle (24h) expiry enforced server-side.
+  test "session is expired after 30d absolute TTL" do
+    sign_in @user
+    db_session = Session.order(created_at: :desc).find_by!(user_id: @user.id)
+
+    # Backdate the session past the absolute TTL.
+    db_session.update_columns(created_at: 31.days.ago, updated_at: 1.hour.ago)
+
+    get root_url
+    assert_redirected_to new_session_url
+    assert_not Session.exists?(db_session.id), "Expired session should be destroyed"
+  end
+
+  test "session is expired after 24h idle TTL" do
+    sign_in @user
+    db_session = Session.order(created_at: :desc).find_by!(user_id: @user.id)
+
+    # Session is young in absolute terms but idle beyond the idle TTL.
+    db_session.update_columns(created_at: 2.days.ago, updated_at: 25.hours.ago)
+
+    get root_url
+    assert_redirected_to new_session_url
+    assert_not Session.exists?(db_session.id), "Idle-expired session should be destroyed"
+  end
 end