From 95d0d844e974b77da7272a943deef6af0f553727 Mon Sep 17 00:00:00 2001 From: Dan Milne Date: Mon, 5 Jan 2026 13:01:32 +1100 Subject: [PATCH] Add a method to remove parameters from urls, so we can redirect without risk of infinite redirect. Fix a bunch of redirects to login afer being foced to log out. Add missing migrations --- app/controllers/concerns/authentication.rb | 1 - app/controllers/oidc_controller.rb | 35 ++++++++++++++----- app/controllers/sessions_controller.rb | 19 ---------- app/controllers/totp_controller.rb | 5 --- ...032220_add_skip_consent_to_applications.rb | 5 +++ ...d_claims_requests_to_oidc_user_consents.rb | 5 +++ ...ms_requests_to_oidc_authorization_codes.rb | 5 +++ 7 files changed, 42 insertions(+), 33 deletions(-) create mode 100644 db/migrate/20260104032220_add_skip_consent_to_applications.rb create mode 100644 db/migrate/20260105000745_add_claims_requests_to_oidc_user_consents.rb create mode 100644 db/migrate/20260105000809_add_claims_requests_to_oidc_authorization_codes.rb diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 7118f3b..c0ddd7a 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -40,7 +40,6 @@ module Authentication end def after_authentication_url - session[:return_to_after_authenticating] session.delete(:return_to_after_authenticating) || root_url end diff --git a/app/controllers/oidc_controller.rb b/app/controllers/oidc_controller.rb index b315f10..db9fff0 100644 --- a/app/controllers/oidc_controller.rb +++ b/app/controllers/oidc_controller.rb @@ -246,9 +246,7 @@ class OidcController < ApplicationController # Store the current URL (which contains all OAuth params) for redirect after login # Remove prompt=login to prevent infinite re-auth loop - return_url = request.url.sub(/&prompt=login(?=&|$)|\?prompt=login&?/, '\1') - # Fix any resulting URL issues (like ?& or & at end) - return_url = return_url.gsub("?&", "?").gsub(/[?&]$/, "") + return_url = remove_query_param(request.url, "prompt") session[:return_to_after_authenticating] = return_url redirect_to signin_path, alert: "Please sign in to continue" @@ -263,15 +261,19 @@ class OidcController < ApplicationController # Calculate session age session_age_seconds = Time.current.to_i - Current.session.created_at.to_i - if session_age_seconds > max_age_seconds + if session_age_seconds >= max_age_seconds # Session is too old - require re-authentication - # Store return URL in session (creates new session cookie) + # Store the return URL in Rails session, then destroy the Session record - # Destroy session and clear cookie to force fresh login + # Store return URL before destroying anything + # Remove max_age from return URL to prevent infinite re-auth loop + return_url = remove_query_param(request.url, "max_age") + session[:return_to_after_authenticating] = return_url + + # Destroy the Session record and clear its cookie Current.session&.destroy! cookies.delete(:session_id) - - session[:return_to_after_authenticating] = request.url + Current.session = nil redirect_to signin_path, alert: "Please sign in to continue" return @@ -1113,6 +1115,23 @@ class OidcController < ApplicationController end end + # Remove a query parameter from a URL using proper URI parsing + # More robust than regex - handles URL encoding, edge cases, etc. + def remove_query_param(url, param_name) + uri = URI.parse(url) + return url unless uri.query + + # Parse query string into hash + params = CGI.parse(uri.query) + params.delete(param_name) + + # Rebuild query string (empty string if no params left) + uri.query = params.any? ? URI.encode_www_form(params) : nil + uri.to_s + rescue URI::InvalidURIError + url + end + def send_backchannel_logout_notifications(user) # Find all active OIDC consents for this user consents = OidcUserConsent.where(user: user).includes(:application) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 20eadc3..44df291 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -86,17 +86,8 @@ class SessionsController < ApplicationController end # Sign in successful (password only) - # Preserve the return_to_after_authenticating value across session boundary - # (e.g., when max_age flow destroys the session and creates a temporary one) - preserved_return_url = session[:return_to_after_authenticating] - start_new_session_for user, acr: "1" - # Restore the return URL if it was lost during session recreation - if preserved_return_url.present? && session[:return_to_after_authenticating].blank? - session[:return_to_after_authenticating] = preserved_return_url - end - # Use status: :see_other to ensure browser makes a GET request # This prevents Turbo from converting it to a TURBO_STREAM request redirect_to after_authentication_url, notice: "Signed in successfully.", allow_other_host: true, status: :see_other @@ -134,12 +125,7 @@ class SessionsController < ApplicationController if session[:totp_redirect_url].present? session[:return_to_after_authenticating] = session.delete(:totp_redirect_url) end - # Preserve return URL across session boundary for max_age flow - preserved_return_url = session[:return_to_after_authenticating] start_new_session_for user, acr: "2" - if preserved_return_url.present? && session[:return_to_after_authenticating].blank? - session[:return_to_after_authenticating] = preserved_return_url - end redirect_to after_authentication_url, notice: "Signed in successfully.", allow_other_host: true return end @@ -151,12 +137,7 @@ class SessionsController < ApplicationController if session[:totp_redirect_url].present? session[:return_to_after_authenticating] = session.delete(:totp_redirect_url) end - # Preserve return URL across session boundary for max_age flow - preserved_return_url = session[:return_to_after_authenticating] start_new_session_for user, acr: "2" - if preserved_return_url.present? && session[:return_to_after_authenticating].blank? - session[:return_to_after_authenticating] = preserved_return_url - end redirect_to after_authentication_url, notice: "Signed in successfully using backup code.", allow_other_host: true return end diff --git a/app/controllers/totp_controller.rb b/app/controllers/totp_controller.rb index f6d3378..75d1513 100644 --- a/app/controllers/totp_controller.rb +++ b/app/controllers/totp_controller.rb @@ -106,12 +106,7 @@ class TotpController < ApplicationController session[:return_to_after_authenticating] = session.delete(:totp_redirect_url) end - # Preserve return URL across session boundary for max_age flow - preserved_return_url = session[:return_to_after_authenticating] start_new_session_for @user - if preserved_return_url.present? && session[:return_to_after_authenticating].blank? - session[:return_to_after_authenticating] = preserved_return_url - end redirect_to after_authentication_url, notice: "Two-factor authentication enabled. Signed in successfully.", allow_other_host: true end diff --git a/db/migrate/20260104032220_add_skip_consent_to_applications.rb b/db/migrate/20260104032220_add_skip_consent_to_applications.rb new file mode 100644 index 0000000..1b0a45b --- /dev/null +++ b/db/migrate/20260104032220_add_skip_consent_to_applications.rb @@ -0,0 +1,5 @@ +class AddSkipConsentToApplications < ActiveRecord::Migration[8.1] + def change + add_column :applications, :skip_consent, :boolean, default: false, null: false + end +end diff --git a/db/migrate/20260105000745_add_claims_requests_to_oidc_user_consents.rb b/db/migrate/20260105000745_add_claims_requests_to_oidc_user_consents.rb new file mode 100644 index 0000000..4865e33 --- /dev/null +++ b/db/migrate/20260105000745_add_claims_requests_to_oidc_user_consents.rb @@ -0,0 +1,5 @@ +class AddClaimsRequestsToOidcUserConsents < ActiveRecord::Migration[8.1] + def change + add_column :oidc_user_consents, :claims_requests, :json, default: {}, null: false + end +end diff --git a/db/migrate/20260105000809_add_claims_requests_to_oidc_authorization_codes.rb b/db/migrate/20260105000809_add_claims_requests_to_oidc_authorization_codes.rb new file mode 100644 index 0000000..ca90267 --- /dev/null +++ b/db/migrate/20260105000809_add_claims_requests_to_oidc_authorization_codes.rb @@ -0,0 +1,5 @@ +class AddClaimsRequestsToOidcAuthorizationCodes < ActiveRecord::Migration[8.1] + def change + add_column :oidc_authorization_codes, :claims_requests, :json, default: {}, null: false + end +end