diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 28758d9d9..132a2af4d 100644 --- a/app/controllers/concerns/authentication.rb +++ b/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 diff --git a/app/controllers/impersonation_sessions_controller.rb b/app/controllers/impersonation_sessions_controller.rb index 1a8c5db7e..183c52cf6 100644 --- a/app/controllers/impersonation_sessions_controller.rb +++ b/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 diff --git a/app/controllers/mfa_controller.rb b/app/controllers/mfa_controller.rb index 987170a9c..456b694e1 100644 --- a/app/controllers/mfa_controller.rb +++ b/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 diff --git a/app/controllers/password_resets_controller.rb b/app/controllers/password_resets_controller.rb index 482654a3a..adc6bb871 100644 --- a/app/controllers/password_resets_controller.rb +++ b/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 diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index ea2f37c08..abe4e1904 100644 --- a/app/controllers/sessions_controller.rb +++ b/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 diff --git a/test/controllers/impersonation_sessions_controller_test.rb b/test/controllers/impersonation_sessions_controller_test.rb index 7dce7f5ac..887533cb5 100644 --- a/test/controllers/impersonation_sessions_controller_test.rb +++ b/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 diff --git a/test/controllers/mfa_controller_test.rb b/test/controllers/mfa_controller_test.rb index b653df41a..50d9b19fd 100644 --- a/test/controllers/mfa_controller_test.rb +++ b/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 diff --git a/test/controllers/password_resets_controller_test.rb b/test/controllers/password_resets_controller_test.rb index 288c8a57e..b99b97669 100644 --- a/test/controllers/password_resets_controller_test.rb +++ b/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) # pipelock:ignore + 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 diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index d3ec232ef..74cb239bb 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/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= 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