-
Notifications
You must be signed in to change notification settings - Fork 187
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
How not to send the state
parameter?
#174
Comments
@skycocker @syakovyn @bufferoverflow I think diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb
index ebfaaa1..6d80a66 100644
--- a/lib/omniauth/strategies/openid_connect.rb
+++ b/lib/omniauth/strategies/openid_connect.rb
@@ -120,7 +120,12 @@ module OmniAuth
def callback_phase
error = params['error_reason'] || params['error']
error_description = params['error_description'] || params['error_reason']
- invalid_state = (options.require_state && params['state'].to_s.empty?) || params['state'] != stored_state
+ invalid_state =
+ if options.require_state
+ params['state'].to_s.empty? || params['state'] != stored_state
+ else
+ false
+ end
raise CallbackError, error: params['error'], reason: error_description, uri: params['error_uri'] if error
raise CallbackError, error: :csrf_detected, reason: "Invalid 'state' parameter" if invalid_state I'm not sure if |
Oh, I see that was proposed in #127 (comment). The issue here is that if the |
Hi! Thanks for looking into this. In the mean time, I've gone ahead and made a working though naive implementation of a solution introducing an additional I went with this solution because I didn't want to alter the behavior of the Hope this helps! |
Yeah, I'm trying to avoid adding yet another config parameter here, plus clear up some confusion about how |
I submitted #181 to fix |
Thank you @stanhu for this! The fix you're proposing allows me to complete my scenario of a stateless user agent which is not able to forward the I think my initial intention was to prevent sending the I realize the "prevent sending de That being said I understand that this library might wish to very slightly deviate from the specs to enforce good practices. Please forgive me if I appear picky, I think this research and write up also serve me as a learning exercise on the subject. Have a great rest of your day and thanks again! |
@amessinger Thanks, we could consider using
A OIDC provider that drops the |
I think your interpretation of the specs is correct. Maybe Or maybe a more encompassing name would be more appropriate such as |
Let me know whenever y'all are ready for the PR to be merged. I'll have to double check to see if this is one of the Omniauth libraries that I actually have rubygems release privileges for. |
I am sorry, but I have an issue with the decision made. I have the 2 scenarios:
With the previous implementation, I could cover both cases with I would be happy with any suggestion that can cover both cases. Thanks, |
@syakovyn Thanks for raising your concerns here.
As discussed above, if we modified
I'm a little confused about this case. In this case, the |
I'd like to keep sending the
Exactly. The IdP keeps registered service providers and allows users to sign into a service provider directly from the IdP site. I am required to support that flow and, as the IdP doesn't have the I thought having The problem is that, in the first case, I want to send the |
@syakovyn How does the IdP gain authorization to make this callback, though? Does this mean that anyone could spoof the IdP? UPDATE: I suppose the IdP somehow has a valid ID token? I see https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin touches on this. |
@stanhu, the IdP (in my case it's Clever) uses the first redirect URI and sends the ID token of the user that initiated that request. UPDATE: It is similar to Send ID Token directly to app (Okta Simplified) described on https://support.okta.com/help/s/article/How-to-make-the-OIDCOAuth-App-visible-in-Okta-dashboard-and-what-are-the-login-flows-available?language=en_US My case is also described on https://stackoverflow.com/a/51050953/419735 |
Actually, I don't need Thus, having some option not to have/generate the state should cover my 2 scenarios and this one. E.g. allowing I would opt for generating the state (probably should be a default setting) and @amessinger will opt out of generating the state. Or even reusing the current state option with |
I think think an explicit
|
This reverts #181 and adds a `send_state` parameter instead to address #174. According to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1, `state` is recommended but not required: ``` state RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. ``` In #181 we attempted to make `require_state` skip the `state` verification if it were `true`, but this was reverted for two reasons: 1. If identity providers make direct requests to the callback phase with a valid token, no `state` is passed in the request, so `require_state` to fail the request would break the existing flow. 2. If `state` isn't used, it shouldn't be sent in the first place. `send_state` will now disable the sending of a `state` in the authorize phase.
This reverts #181 and adds a `send_state` parameter instead to address #174. According to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1, `state` is recommended but not required: ``` state RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. ``` In #181 we attempted to make `require_state` skip the `state` verification if it were `true`, but this was reverted for two reasons: 1. If identity providers make direct requests to the callback phase with a valid token, no `state` is passed in the request. If `require_state` were `true`, this change fails the request and breaks existing flows. 2. If `state` isn't sent in the first place, it should not be verified. `send_state` will now disable the sending of a `state` in the authorize phase.
Please take a look at #182. |
This reverts #181 and adds a `send_state` parameter instead to address #174. According to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1, `state` is recommended but not required: ``` state RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. ``` In #181 we attempted to make `require_state` skip the `state` verification if it were `true`, but this was reverted for two reasons: 1. If identity providers make direct requests to the callback phase with a valid token, no `state` is passed in the request. If `require_state` were `true`, this change fails the request and breaks existing flows. 2. If `state` isn't sent in the first place, it should not be verified. `send_state` will now disable the sending of a `state` in the authorize phase.
This reverts #181 and adds a `send_state` parameter instead to address #174. According to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1, `state` is recommended but not required: ``` state RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. ``` In #181 we attempted to make `require_state` skip the `state` verification if it were `true`, but this was reverted for two reasons: 1. If identity providers make direct requests to the callback phase with a valid token, no `state` is passed in the request. If `require_state` were `true`, this change fails the request and breaks existing flows. 2. If `state` isn't sent in the first place, it should not be verified. `send_state` will now disable the sending of a `state` in the authorize phase.
Just tested the changes shipped in #182. Works like a charm on my local environment (didn't have to change a thing appart from the Thanks a lot, I'd say this issue can be closed. |
The change works for me too. Thanks @stanhu |
Hi there! Thanks for creating & maintaining this gem :)
I'd like to prevent this client from sending the
state
parameter during the request phase because in some cases I can't rely on the session to store it.I thought the
require_state
config option set tofalse
would do the trick but it seems to be used only for validation during the callback phase :omniauth_openid_connect/lib/omniauth/strategies/openid_connect.rb
Line 123 in b56629d
From what I understand, the
state
parameter is still sent during the request phase, the identity provider forwards it back to the callback URL and thisstate
value in the URL's parameters, though valid, can't be matched to the session'sstate
value since the session is not available.Couldn't the creation of the
state
parameter be made conditionally to the value of therequire_state
config option?omniauth_openid_connect/lib/omniauth/strategies/openid_connect.rb
Line 178 in b56629d
Any help would be appreciated!
The text was updated successfully, but these errors were encountered: