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

app/controllers/concerns/authentication.rb

@@ -27,19 +27,46 @@ def authenticate_user!
       end
     end
 
+    # Session TTL constants (F-04, CWE-613)
+    SESSION_ABSOLUTE_TTL = 30.days
+    SESSION_IDLE_TTL = 24.hours
+
     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)
+      return nil unless session
+
+      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
+
+      # Touch to refresh idle timer on each request
+      session.touch
+      session
     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,6 +33,12 @@ 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!
     redirect_to root_path, notice: t(".success")
   end

app/controllers/mfa_controller.rb

@@ -29,8 +29,30 @@ def verify
   def verify_code
     @user = User.find_by(id: session[:mfa_user_id])
 
+    # 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)
+    if session[:mfa_started_at].present? && Time.current - Time.parse(session[:mfa_started_at].to_s) > 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)
       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 # Prevent session fixation (FIX-01)
         @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 # Prevent session fixation (FIX-01)
         @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,37 @@ 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
+      post verify_mfa_path, params: { code: ROTP::TOTP.new(other_user.otp_secret).now }
+      assert_redirected_to new_session_path
+    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,40 @@ 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
+
+    sign_in @user
+
+    assert_not_equal old_session_id, session.id, "Session ID should change on login to prevent fixation"
+  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