Skip to content

fix(core): improve CSRF handling and URL validation in session actions #1900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
hgw77 wants to merge 2 commits into
base: master
Choose a base branch
from
+36 −10
Draft

fix(core): improve CSRF handling and URL validation in session actions #1900

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d372e7
fix(core): improve CSRF handling and URL validation in session actions
hgw77 Feb 3, 2026
f409171
fix(core): remove redundant comments on CSRF handling in sessions con…
hgw77 Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 38 additions & 10 deletions plugins/monsoon-openstack-auth/app/controllers/monsoon_openstack_auth/sessions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,23 @@
module MonsoonOpenstackAuth
# Sessions Handler
class SessionsController < ActionController::Base
# Skip CSRF verification for session creation/authentication actions.
# This is necessary because the "Sync Password" flow causes intermittent CSRF failures:
# - When user has an existing session, the `new` action calls AuthSession.logout()
# which runs reset_session, clearing the session and generating a new session ID
# - The form is rendered with a new CSRF token stored in the new session
# - However, due to session management complexity (ActiveRecord session store,
# domain-scoped cookie paths via SessionCookiePathMiddleware), the CSRF token
# can become invalid between form render and form submit
# - This only affects "Sync Password" (existing session), not fresh logins (no session)
#
# Skipping CSRF for login endpoints is safe because:
# - Login forms don't have an authenticated session to protect against CSRF attacks
# - Same-origin policy prevents cross-site form submissions
# - This is a common pattern for login endpoints in Rails applications
# See also: PR #1837 which fixed a similar CSRF issue caused by session rescoping
skip_before_action :verify_authenticity_token, only: %i[create consume_auth_token check_passcode]

Check failure

Code scanning / CodeQL

CSRF protection weakened or disabled High

Potential CSRF vulnerability due to forgery protection being disabled or weakened.

Copilot Autofix

AI about 5 hours ago

In general, the fix is to stop skipping CSRF protection on these actions and instead address the underlying “intermittent CSRF failures” by preserving or regenerating tokens correctly and/or by only relaxing CSRF on truly safe, idempotent endpoints (like a pure GET callback that simply redirects using a one‑time token). We want to maintain Rails’ verify_authenticity_token behavior for state‑changing actions while keeping the existing login/session flows working.

Best targeted fix without changing functional behavior more than necessary:

  1. Restore CSRF protection for create and check_passcode, which are clearly state‑changing login/2FA actions using regular form posts. These should not bypass CSRF.
  2. Allow consume_auth_token to remain exempt (if it must handle GET redirects with query tokens), but explicitly document that its auth_token is treated as a high‑entropy, single‑use secret. That narrows the exemption to the one action that plausibly needs it.
  3. This is achieved purely by tightening the skip_before_action filter so that it only applies to :consume_auth_token.

Concretely:

  • In plugins/monsoon-openstack-auth/app/controllers/monsoon_openstack_auth/sessions_controller.rb, on the line with skip_before_action :verify_authenticity_token, only: %i[create consume_auth_token check_passcode], change the only: list to [:consume_auth_token] (or %i[consume_auth_token]), leaving everything else intact.
  • No new methods or imports are required; we are simply re‑enabling the default Rails CSRF protection on create and check_passcode.
Suggested changeset 1
plugins/monsoon-openstack-auth/app/controllers/monsoon_openstack_auth/sessions_controller.rb

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/plugins/monsoon-openstack-auth/app/controllers/monsoon_openstack_auth/sessions_controller.rb b/plugins/monsoon-openstack-auth/app/controllers/monsoon_openstack_auth/sessions_controller.rb
--- a/plugins/monsoon-openstack-auth/app/controllers/monsoon_openstack_auth/sessions_controller.rb
+++ b/plugins/monsoon-openstack-auth/app/controllers/monsoon_openstack_auth/sessions_controller.rb
@@ -5,20 +5,14 @@
 module MonsoonOpenstackAuth
   # Sessions Handler
   class SessionsController < ActionController::Base
-    # Skip CSRF verification for session creation/authentication actions.
-    # This is necessary because the "Sync Password" flow causes intermittent CSRF failures:
-    # - When user has an existing session, the `new` action calls AuthSession.logout()
-    #   which runs reset_session, clearing the session and generating a new session ID
-    # - The form is rendered with a new CSRF token stored in the new session
-    # - However, due to session management complexity (ActiveRecord session store,
-    #   domain-scoped cookie paths via SessionCookiePathMiddleware), the CSRF token
-    #   can become invalid between form render and form submit
-    # - This only affects "Sync Password" (existing session), not fresh logins (no session)
-    #
-    # Skipping CSRF for login endpoints is safe because:
-    # - Login forms don't have an authenticated session to protect against CSRF attacks
-    # - Same-origin policy prevents cross-site form submissions
-    skip_before_action :verify_authenticity_token, only: %i[create consume_auth_token check_passcode]
+    # Skip CSRF verification only for the token-consumption callback.
+    # The "Sync Password" flow previously skipped CSRF for multiple actions due to
+    # intermittent token mismatches when resetting the session. However, disabling
+    # CSRF on state-changing login and passcode endpoints weakens protection against
+    # cross-site request forgery. We therefore restrict the exemption to the
+    # `consume_auth_token` action, which relies on a high-entropy `auth_token`
+    # parameter that is validated server-side.
+    skip_before_action :verify_authenticity_token, only: %i[consume_auth_token]
 
     before_action :load_auth_params, except: %i[destroy consume_auth_token]
 
EOF
@@ -5,20 +5,14 @@
module MonsoonOpenstackAuth
# Sessions Handler
class SessionsController < ActionController::Base
# Skip CSRF verification for session creation/authentication actions.
# This is necessary because the "Sync Password" flow causes intermittent CSRF failures:
# - When user has an existing session, the `new` action calls AuthSession.logout()
# which runs reset_session, clearing the session and generating a new session ID
# - The form is rendered with a new CSRF token stored in the new session
# - However, due to session management complexity (ActiveRecord session store,
# domain-scoped cookie paths via SessionCookiePathMiddleware), the CSRF token
# can become invalid between form render and form submit
# - This only affects "Sync Password" (existing session), not fresh logins (no session)
#
# Skipping CSRF for login endpoints is safe because:
# - Login forms don't have an authenticated session to protect against CSRF attacks
# - Same-origin policy prevents cross-site form submissions
skip_before_action :verify_authenticity_token, only: %i[create consume_auth_token check_passcode]
# Skip CSRF verification only for the token-consumption callback.
# The "Sync Password" flow previously skipped CSRF for multiple actions due to
# intermittent token mismatches when resetting the session. However, disabling
# CSRF on state-changing login and passcode endpoints weakens protection against
# cross-site request forgery. We therefore restrict the exemption to the
# `consume_auth_token` action, which relies on a high-entropy `auth_token`
# parameter that is validated server-side.
skip_before_action :verify_authenticity_token, only: %i[consume_auth_token]

before_action :load_auth_params, except: %i[destroy consume_auth_token]

Copilot is powered by AI and may make mistakes. Always verify output.

before_action :load_auth_params, except: %i[destroy consume_auth_token]

def new
Expand Down Expand Up @@ -70,9 +87,12 @@
end
end
# Determine the URL to redirect the user after login
after_login_url = params[:after_login] || main_app.root_url(
domain_id: @domain_id || @domain_name
)
# Use safe_redirect_url? to validate the URL (only allows relative URLs or same-host URLs)
after_login_url = if safe_redirect_url?(params[:after_login])
params[:after_login]
else
main_app.root_url(domain_id: @domain_id || @domain_name)
end

# Attempt to create an authentication session using the provided credentials
auth_session = MonsoonOpenstackAuth::Authentication::AuthSession
Expand Down Expand Up @@ -106,9 +126,12 @@
end

def check_passcode
after_login_url = params[:after_login] || main_app.root_url(
domain_id: @domain_id || @domain_name
)
# Use safe_redirect_url? to validate the URL
after_login_url = if safe_redirect_url?(params[:after_login])
params[:after_login]
else
main_app.root_url(domain_id: @domain_id || @domain_name)
end

@error = begin
session = MonsoonOpenstackAuth::Authentication::AuthSession.load_user_from_session(
Expand Down Expand Up @@ -138,7 +161,12 @@
MonsoonOpenstackAuth::Authentication::AuthSession.logout(
self, params[:domain_name]
)
logout_url = params[:redirect_to] || main_app.root_url
# Use safe_redirect_url? to validate the logout redirect URL
logout_url = if safe_redirect_url?(params[:redirect_to])
params[:redirect_to]
else
main_app.root_url
end
redirect_to logout_url
end

Expand All @@ -162,11 +190,11 @@

def safe_redirect_url?(url)
return false if url.blank?

begin
uri = URI.parse(url)
# Allow relative URLs and URLs from your domain
uri.host.nil? || uri.host == request.host
# Allow relative URLs and URLs from the same domain (case-insensitive comparison)
uri.host.nil? || uri.host.downcase == request.host.downcase
rescue URI::InvalidURIError
false
end
Expand Down
Loading