Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add send_state parameter to disable sending of state #182

Merged
merged 2 commits into from
Jul 3, 2024
Merged

feat: Add send_state parameter to disable sending of state #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75f56ee
Revert "fix: make require_state skip verification of state (#181)"
stanhu Jun 29, 2024
1e70c98
feat: Add `send_state` parameter to disable sending of state
stanhu Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
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
View file
Open in desktop
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
30 changes: 22 additions & 8 deletions test/lib/omniauth/strategies/openid_connect_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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

Expand Down
Loading