-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(auth): Detect SSO provider mismatch and fix 2FA redirect #106041
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
Conversation
src/sentry/utils/auth.py
Outdated
| PERSISTENT_NEXT_MAX_AGE = 60 * 60 # 1 hour | ||
|
|
||
|
|
||
| def set_persistent_next(request: HttpRequest, next_url: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont love this, gonna explore if we can just improve the session clearing
…vider mismatch Two issues in the authentication flow are addressed: 1. Redirect state loss: initiate_login() unconditionally cleared _next even when no new URL was provided. Now _next and _referrer are only updated if new values are given, preserving redirects across SSO retries. 2. Provider mismatch: When users authenticated with the wrong SSO provider (e.g., Google when Okta required), build_identity() would fail silently. Now we detect the mismatch before calling build_identity() and redirect users to the correct SSO flow with a clear warning message. Also fixes the 2FA flow setting after_2fa to the SSO callback URL instead of the user's original destination. URLs are validated with is_valid_redirect() to prevent open redirect attacks.
5cc2b4e to
5439199
Compare
Simplify naming by using `provider_key` instead of `actual_provider_key` for the pipeline state field that stores which provider handled the OAuth2/SAML2 callback. This field is used to detect when a user authenticates with a different provider than the org requires. Also adds better comments explaining the purpose of this field.
5d387db to
f5e9c7a
Compare
The provider mismatch detection was comparing provider_key (from callback) against self.provider.key (pipeline's provider), which are always the same. Fixed to compare against self.provider_model.provider (the org's configured SSO provider from the database). Also only check during FLOW_LOGIN since mismatch doesn't apply when setting up a new provider. Simplified the 2FA redirect URL logic for clarity.
| after_2fa_url = self.request.session.get("_next") | ||
| if not after_2fa_url or not auth.is_valid_redirect( | ||
| after_2fa_url, allowed_hosts=(self.request.get_host(),) | ||
| ): | ||
| after_2fa_url = self.request.build_absolute_uri() | ||
|
|
||
| user_was_logged_in = auth.login( | ||
| self.request, | ||
| user, | ||
| after_2fa=self.request.build_absolute_uri(), | ||
| after_2fa=after_2fa_url, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add back `from time import time` that was accidentally removed - Remove unused `response` variable assignments in tests
src/sentry/auth/providers/oauth2.py
Outdated
|
|
||
| # Store which provider handled this callback, used to detect when a user | ||
| # authenticates with a different provider than the org requires | ||
| pipeline.bind_state("provider_key", pipeline.provider.key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provider mismatch detection will never trigger
Medium Severity
The provider mismatch detection stores pipeline.provider.key, but pipeline.provider is always the expected provider from the organization's configuration (via self.provider_model.get_provider()), not the actual provider that processed the authentication. In finish_pipeline, the check compares this stored value against self.provider_model.provider, which are guaranteed to be equal since both derive from the same source. The mismatch detection will never trigger in production because the stored provider_key always matches the expected provider, making this feature ineffective for its intended purpose of detecting when users authenticate with the wrong SSO provider.
Additional Locations (2)
Revert the initiate_login() changes that preserved _next and _referrer session keys across calls. Analysis confirmed this preservation was unnecessary for the provider mismatch bug since _handle_provider_mismatch() doesn't call initiate_login() - the session _next value survives naturally. The provider mismatch detection (comparing against provider_model.provider) is sufficient to fix the original bug.
| initiate_login simply clears session cache | ||
| if provided a `next_url` will append to the session after clearing previous keys | ||
| Clears existing login state and initializes a new login flow. | ||
| Optionally sets the post-login redirect destination and referrer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redirect state fix not implemented despite PR claim
High Severity
The PR description claims that initiate_login() now preserves _next and _referrer when no new values are provided, but the implementation only changed the docstring. The for-loop still unconditionally deletes all keys including _next and _referrer, then only sets them back if new values are provided. This means redirect state will still be lost when initiate_login() is called without explicit next_url/referrer arguments. To match the stated behavior, _next and _referrer need to be removed from the deletion loop so they're only overwritten when new values are explicitly provided.
…tion
The previous implementation stored provider_key from the current session
state, which would always match the expected provider. This meant the
provider mismatch detection could never trigger in production.
Now the provider key is encoded in the OAuth state parameter and SAML
RelayState, which survive the IdP redirect. This allows detecting when
a user completes an SSO flow started for a different provider (e.g.,
multiple tabs with different orgs).
Changes:
- OAuth2Login: Encode provider key in state as `{nonce}:{provider_key}`
- OAuth2Callback: Extract provider key from returned state parameter
- SAML2LoginView: Encode provider key in RelayState
- SAML2ACSView: Extract provider key from returned RelayState
- Add tests for state/RelayState encoding and backward compatibility
JoshFerge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes make sense to me.
Note on Multi-Tab SSO BehaviorWhile reviewing the bot feedback, I investigated the provider mismatch detection and wanted to document the current state: Pre-existing Issue: Single Session KeyThe auth pipeline uses a single session key ( # src/sentry/auth/store.py
@property
def session_key(self) -> str:
return "auth_key"This means multiple concurrent SSO flows (e.g., different tabs for different orgs) will overwrite each other's session state. This is a pre-existing issue, not introduced by this PR. Before This PRWhen Tab A starts SSO, then Tab B starts SSO (overwriting the session pointer), and Tab A completes:
After This PR
So this PR does improve the SAML multi-tab scenario by providing better error messaging, even though the fundamental single-session-key limitation remains. The bot's claim that "provider mismatch detection will never trigger" is incorrect for this cross-tab SAML case. The proper fix for multi-tab support would require scoping the session key (e.g., by org slug), but that's a larger change outside the scope of this PR. |
Fixes two issues in the SSO authentication flow:
Provider mismatch detection: When users authenticated with the wrong SSO provider (e.g., Google when the org requires Okta),
build_identity()would fail with a confusing error. Now we detect the mismatch before callingbuild_identity()by comparing the callback's provider against the org's configured provider, and redirect users to the correct SSO flow with a clear warning message.2FA redirect fix: The 2FA flow was setting
after_2fato the SSO callback URL instead of the user's original destination. Now it uses_nextfrom the session (validated withis_valid_redirect()to prevent open redirects).Changes
src/sentry/auth/helper.py: Add provider mismatch detection infinish_pipeline(), fixafter_2faURL in_login()src/sentry/auth/providers/oauth2.py,saml2/provider.py: Storeprovider_keyin pipeline state for mismatch detection