Skip to content

Commit

Permalink
feat: Add send_state parameter to disable sending of state
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
stanhu committed Jul 3, 2024
1 parent bd14191 commit 02839d7
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 4 deletions.
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ end
| scope | Which OpenID scopes to include (:openid is always required) | no | Array<sym> [:openid] | [:openid, :profile, :email] |
| response_type | Which OAuth2 response type to use with the authorization request | no | String: code | one of: 'code', 'id_token' |
| state | A value to be used for the OAuth2 state parameter on the authorization request. Can be a proc that generates a string. | no | Random 16 character string | Proc.new { SecureRandom.hex(32) } |
| require_state | Should state param be verified - this is recommended, not required by the OIDC specification | no | true | false |
| require_state | Should the callback phase require that a state is present. If `send_state` is true, then the callback state must match the authorize state. This is recommended, not required by the OIDC specification. | no | true | false |
| send_state | Should the authorize phase send a `state` parameter - this is recommended, not required by the OIDC specification | no | true | false |
| response_mode | The response mode per [spec](https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html) | no | nil | one of: :query, :fragment, :form_post, :web_message |
| display | An optional parameter to the authorization request to determine how the authorization and consent page | no | nil | one of: :page, :popup, :touch, :wap |
| prompt | An optional parameter to the authrization request to determine what pages the user will be shown | no | nil | one of: :none, :login, :consent, :select_account |
Expand Down
12 changes: 9 additions & 3 deletions lib/omniauth/strategies/openid_connect.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class OpenIDConnect # rubocop:disable Metrics/ClassLength
option :client_x509_signing_key
option :scope, [:openid]
option :response_type, 'code' # ['code', 'id_token']
option :send_state, true
option :require_state, true
option :state
option :response_mode # [:query, :fragment, :form_post, :web_message]
Expand Down Expand Up @@ -120,7 +121,12 @@ def request_phase
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.send_state
(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
Expand Down Expand Up @@ -169,13 +175,12 @@ def end_session_uri
end_session_uri.to_s
end

def authorize_uri
def authorize_uri # rubocop:disable Metrics/AbcSize
client.redirect_uri = redirect_uri
opts = {
response_type: options.response_type,
response_mode: options.response_mode,
scope: options.scope,
state: new_state,
login_hint: params['login_hint'],
ui_locales: params['ui_locales'],
claims_locales: params['claims_locales'],
Expand All @@ -185,6 +190,7 @@ def authorize_uri
acr_values: options.acr_values,
}

opts[:state] = new_state if options.send_state
opts.merge!(options.extra_authorize_params) unless options.extra_authorize_params.empty?

options.allow_authorize_params.each do |key|
Expand Down
44 changes: 44 additions & 0 deletions test/lib/omniauth/strategies/openid_connect_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,50 @@ def test_callback_phase_with_discovery # rubocop:disable Metrics/AbcSize
strategy.callback_phase
end

def test_callback_phase_with_send_state_disabled # rubocop:disable Metrics/AbcSize
code = SecureRandom.hex(16)

strategy.options.client_options.host = 'example.com'
strategy.options.require_state = true
strategy.options.send_state = false
strategy.options.discovery = true
refute_match(/state/, strategy.authorize_uri, 'URI must not contain state')

request.stubs(:params).returns('code' => code)
request.stubs(:path).returns('')

issuer = stub('OpenIDConnect::Discovery::Issuer')
issuer.stubs(:issuer).returns('https://example.com/')
::OpenIDConnect::Discovery::Provider.stubs(:discover!).returns(issuer)

config = stub('OpenIDConnect::Discovery::Provder::Config')
config.stubs(:authorization_endpoint).returns('https://example.com/authorization')
config.stubs(:token_endpoint).returns('https://example.com/token')
config.stubs(:userinfo_endpoint).returns('https://example.com/userinfo')
config.stubs(:jwks_uri).returns('https://example.com/jwks')
config.stubs(:jwks).returns(JSON::JWK::Set.new(jwks['keys']))

::OpenIDConnect::Discovery::Provider::Config.stubs(:discover!).with('https://example.com/').returns(config)

id_token = stub('OpenIDConnect::ResponseObject::IdToken')
id_token.stubs(:raw_attributes).returns('sub' => 'sub', 'name' => 'name', 'email' => 'email')
id_token.stubs(:verify!).with(issuer: 'https://example.com/', client_id: @identifier, nonce: nonce).returns(true)
::OpenIDConnect::ResponseObject::IdToken.stubs(:decode).returns(id_token)

strategy.unstub(:user_info)
access_token = stub('OpenIDConnect::AccessToken')
access_token.stubs(:access_token)
access_token.stubs(:refresh_token)
access_token.stubs(:expires_in)
access_token.stubs(:scope)
access_token.stubs(:id_token).returns(jwt.to_s)
client.expects(:access_token!).at_least_once.returns(access_token)
access_token.expects(:userinfo!).returns(user_info)

strategy.call!('rack.session' => { 'omniauth.nonce' => nonce })
strategy.callback_phase
end

def test_callback_phase_with_no_state_without_state_verification # rubocop:disable Metrics/AbcSize
code = SecureRandom.hex(16)

Expand Down

0 comments on commit 02839d7

Please sign in to comment.