From a8678e09483f99b85059eb31d931247048fab402 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 27 Jun 2024 09:44:23 -0700 Subject: [PATCH] fix: make require_state skip verification of state In https://github.com/omniauth/omniauth_openid_connect/pull/127, `require_state` was introduced because 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. ``` During review, the `require_state` parameter was modified to verify `state` as long as `stored_state` was present. However, `stored_state` always holds at least a random value, so when `require_state` were `false` and if an OpenID provider did not relay the `state` value, authentication would halt with a "Invalid 'state' parameter" error. This commit updates it so that if `require_state` is set to `false`, the `state` parameter is never checked at all. --- lib/omniauth/strategies/openid_connect.rb | 2 +- .../strategies/openid_connect_test.rb | 33 ++++++++++++++++++- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index ebfaaa17..e91b88a1 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -120,7 +120,7 @@ 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 = options.require_state && (params['state'].to_s.empty? || params['state'] != stored_state) 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 diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index 031e1e3c..110d1180 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -501,12 +501,43 @@ def test_callback_phase_with_invalid_state_without_state_verification state = SecureRandom.hex(16) strategy.options.require_state = false + strategy.options.discovery = false request.stubs(:params).returns('code' => code, 'state' => 'foobar') 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) + + 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.state' => state, 'omniauth.nonce' => nonce }) - strategy.expects(:fail!) strategy.callback_phase end