-
Notifications
You must be signed in to change notification settings - Fork 32
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.
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
base: master
Are you sure you want to change the base?
Conversation
| # - 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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 3 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:
- Restore CSRF protection for
createandcheck_passcode, which are clearly state‑changing login/2FA actions using regular form posts. These should not bypass CSRF. - Allow
consume_auth_tokento remain exempt (if it must handle GET redirects with query tokens), but explicitly document that itsauth_tokenis treated as a high‑entropy, single‑use secret. That narrows the exemption to the one action that plausibly needs it. - This is achieved purely by tightening the
skip_before_actionfilter 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 withskip_before_action :verify_authenticity_token, only: %i[create consume_auth_token check_passcode], change theonly: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
createandcheck_passcode.
-
Copy modified lines R8-R15
| @@ -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] | ||
|
|
Summary
Fixes intermittent CSRF token validation errors on the "Sync Password" flow by skipping CSRF verification for login endpoints, and adds consistent
safe_redirect_url?validation to prevent open redirect vulnerabilities.Problem
Users clicking "Sync Password" in their profile intermittently encountered a "Can't verify CSRF token authenticity" error (HTTP 422). The issue only affected the "Sync Password" flow (users with existing sessions), not fresh logins.
Root Cause
When a user with an existing session visits the login page,
AuthSession.logout()callsreset_session()plugins/monsoon-openstack-auth/app/controllers/monsoon_openstack_auth/sessions_controller.rb:The flow:
token_storereturnsnil→reset_sessionreturns early → CSRF works ✅reset_sessionruns fully → new session ID created → new CSRF token generated → but due to session management complexity (ActiveRecord store, domain-scoped cookie paths), the token can become invalid between form render and submit ❌Solution
Skip CSRF verification for login actions (
create,consume_auth_token,check_passcode)Add
safe_redirect_url?validation tocreate,check_passcode, anddestroyactionsconsume_auth_token, now applied consistentlyChanges Made
skip_before_action :verify_authenticity_tokenfor login-related actionssafe_redirect_url?validation toafter_loginandredirect_toparameters in all actionssafe_redirect_url?Flow Diagram
Related Issues
Checklist