diff --git a/README.md b/README.md index 169d908f..805ee8f5 100644 --- a/README.md +++ b/README.md @@ -79,7 +79,8 @@ end | scope | Which OpenID scopes to include (:openid is always required) | no | Array [: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 | diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index e91b88a1..e6143a93 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -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] @@ -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 @@ -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'], @@ -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| diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index f54bb624..6059df27 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -453,17 +453,18 @@ def test_callback_phase_with_discovery # rubocop:disable Metrics/AbcSize strategy.callback_phase end - def test_callback_phase_with_no_state_without_state_verification # rubocop:disable Metrics/AbcSize + def test_callback_phase_with_send_state_disabled # rubocop:disable Metrics/AbcSize code = SecureRandom.hex(16) - strategy.options.require_state = false + 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('') - strategy.options.client_options.host = 'example.com' - strategy.options.discovery = true - issuer = stub('OpenIDConnect::Discovery::Issuer') issuer.stubs(:issuer).returns('https://example.com/') ::OpenIDConnect::Discovery::Provider.stubs(:discover!).returns(issuer) @@ -496,13 +497,12 @@ def test_callback_phase_with_no_state_without_state_verification # rubocop:disab strategy.callback_phase end - def test_callback_phase_with_invalid_state_without_state_verification # rubocop:disable Metrics/AbcSize + def test_callback_phase_with_no_state_without_state_verification # rubocop:disable Metrics/AbcSize code = SecureRandom.hex(16) - state = SecureRandom.hex(16) strategy.options.require_state = false - request.stubs(:params).returns('code' => code, 'state' => 'foobar') + request.stubs(:params).returns('code' => code) request.stubs(:path).returns('') strategy.options.client_options.host = 'example.com' @@ -536,7 +536,21 @@ def test_callback_phase_with_invalid_state_without_state_verification # rubocop: 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_invalid_state_without_state_verification + code = SecureRandom.hex(16) + state = SecureRandom.hex(16) + + strategy.options.require_state = false + + request.stubs(:params).returns('code' => code, 'state' => 'foobar') + request.stubs(:path).returns('') + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + strategy.expects(:fail!) strategy.callback_phase end