diff --git a/config/environments/test.rb b/config/environments/test.rb index 78c393d22..d86893643 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -56,6 +56,11 @@ # Fake omniauth for testing OmniAuth.config.test_mode = true + # Deterministic failure handling: omniauth raises from its FailureEndpoint + # when ENV['RACK_ENV'] is 'development', and Puma's Capybara test server sets + # ENV['RACK_ENV'] ||= 'development' mid-process — so specs running after a + # feature spec would otherwise see raise-out instead of the redirect. + OmniAuth.config.failure_raise_out_environments = [] config.after_initialize do Bullet.enable = true diff --git a/lib/omniauth/strategies/codebar.rb b/lib/omniauth/strategies/codebar.rb index 57aa69653..f8af0f171 100644 --- a/lib/omniauth/strategies/codebar.rb +++ b/lib/omniauth/strategies/codebar.rb @@ -45,8 +45,7 @@ def request_phase def callback_phase error = request.params['error'] if error - fail!(:auth_error, StandardError.new(error)) - return + return fail!(:auth_error, StandardError.new(error)) end # Verify state/nonce @@ -105,7 +104,11 @@ def callback_phase call_app! rescue StandardError => e - return if env['omniauth.error'] + raise if @env['omniauth.auth'] # app error after successful auth: not an auth failure + + # A failure must return the Rack response from on_failure — a bare return + # gives Rack a nil response and TempfileReaper raises `[]=' for nil`. + return OmniAuth.config.on_failure.call(env) if env['omniauth.error'] fail!(:unknown_error, e) end diff --git a/spec/lib/omniauth/strategies/codebar_spec.rb b/spec/lib/omniauth/strategies/codebar_spec.rb index c42820ad4..c0e614409 100644 --- a/spec/lib/omniauth/strategies/codebar_spec.rb +++ b/spec/lib/omniauth/strategies/codebar_spec.rb @@ -77,6 +77,17 @@ def build_env(path, query: '', session: {}) end describe 'callback error paths' do + it 'returns the failure response when the provider redirects back with error params' do + env = build_env('/auth/codebar/callback', + query: 'error=invalid_request&error_description=pkce+is+required+for+public+clients') + response = strategy.call!(env) + + # A nil return here crashes Rack's TempfileReaper with `[]=' for nil`. + expect(response).to be_a(Array) + expect(response[0]).to eq(302) + expect(env['omniauth.error.type']).to eq(:auth_error) + end + it 'fails with csrf_detected when state is missing from session' do env = build_env('/auth/codebar/callback', query: 'code=abc&state=some-state') strategy.call!(env) diff --git a/spec/requests/omniauth_error_callback_spec.rb b/spec/requests/omniauth_error_callback_spec.rb new file mode 100644 index 000000000..e6eb0e746 --- /dev/null +++ b/spec/requests/omniauth_error_callback_spec.rb @@ -0,0 +1,30 @@ +require 'rails_helper' + +RSpec.describe 'OmniAuth provider error callback' do + # Regression: a provider error redirect (error=invalid_scope, pkce required, + # ...) used to crash the middleware stack with NoMethodError '[]=' for nil + # instead of redirecting to the failure page. + around do |example| + OmniAuth.config.test_mode = false + example.run + OmniAuth.config.test_mode = true + end + + it 'redirects to the failure page instead of raising' do + get '/auth/codebar/callback', params: { + error: 'invalid_request', + error_description: 'pkce is required for public clients', + state: 'some-state', + iss: 'https://auth.codebar.io' + } + + expect(response).to have_http_status(:redirect) + expect(response.location).to start_with('/auth/failure?') + end + + it 'shows the authentication error flash after the failure redirect' do + get '/auth/failure', params: { message: 'auth_error', strategy: 'codebar' } + + expect(response).to redirect_to(root_url) + end +end