diff --git a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb index 033cbf37d18..9aa44bd3804 100644 --- a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb +++ b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb @@ -23,6 +23,10 @@ def create private + def presenter_for_two_factor_authentication_method + TwoFactorAuthCode::PersonalKeyPresenter.new + end + def handle_result(result) if result.success? generate_new_personal_key diff --git a/app/services/openid_connect_redirector.rb b/app/services/openid_connect_redirector.rb index c960eb68a7d..4e5d1b0ebbf 100644 --- a/app/services/openid_connect_redirector.rb +++ b/app/services/openid_connect_redirector.rb @@ -73,11 +73,25 @@ def validate_redirect_uri_matches_sp_redirect_uri errors.add(error_attr, t('openid_connect.authorization.errors.redirect_uri_no_match')) end + # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/MethodLength def redirect_uri_matches_sp_redirect_uri? - redirect_uri.present? && - service_provider.active? && - service_provider.redirect_uris.any? do |sp_redirect_uri| - redirect_uri.start_with?(sp_redirect_uri) - end + return unless redirect_uri.present? && service_provider.active? + parsed_redirect_uri = URI(redirect_uri) + service_provider.redirect_uris.any? do |sp_redirect_uri| + parsed_sp_redirect_uri = URI(sp_redirect_uri) + + parsed_redirect_uri.scheme == parsed_sp_redirect_uri.scheme && + parsed_redirect_uri.port == parsed_sp_redirect_uri.port && + parsed_redirect_uri.host == parsed_sp_redirect_uri.host && + parsed_redirect_uri.path.start_with?(parsed_sp_redirect_uri.path) + end + rescue URI::Error + false end + + # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/CyclomaticComplexity + # rubocop:enable Metrics/MethodLength end diff --git a/config/routes.rb b/config/routes.rb index e48f79a500e..016118cacb7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -77,7 +77,7 @@ get '/login/two_factor/piv_cac' => 'two_factor_authentication/piv_cac_verification#show' end get '/login/two_factor/:otp_delivery_preference' => 'two_factor_authentication/otp_verification#show', - as: :login_two_factor + as: :login_two_factor, constraints: { otp_delivery_preference: /sms|voice/ } post '/login/two_factor/:otp_delivery_preference' => 'two_factor_authentication/otp_verification#create', as: :login_otp diff --git a/spec/features/two_factor_authentication/remember_device_spec.rb b/spec/features/two_factor_authentication/remember_device_spec.rb index 69b0c22013a..2f31bb951a0 100644 --- a/spec/features/two_factor_authentication/remember_device_spec.rb +++ b/spec/features/two_factor_authentication/remember_device_spec.rb @@ -104,7 +104,7 @@ def remember_device_and_sign_out_user it 'does not offer the option to remember device' do sign_in_user(user) - expect(current_path).to eq(login_two_factor_path(otp_delivery_preference: :authenticator)) + expect(current_path).to eq(login_two_factor_authenticator_path) expect(page).to_not have_content( t('forms.messages.remember_device', duration: Figaro.env.remember_device_expiration_days!) ) diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index c48e27b06d6..2929a3927ed 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -653,7 +653,7 @@ def submit_prefilled_otp_code click_submit_default expect(page).to have_content(t('devise.two_factor_authentication.invalid_otp')) - expect(current_path).to eq login_two_factor_path(otp_delivery_preference: :authenticator) + expect(current_path).to eq login_two_factor_authenticator_path end end end @@ -677,6 +677,13 @@ def submit_prefilled_otp_code end end + it 'generates a 404 with bad otp_delivery_preference' do + sign_in_before_2fa + visit '/login/two_factor/bad' + + expect(page.status_code).to eq(404) + end + describe 'visiting OTP delivery and verification pages after fully authenticating' do it 'redirects to profile page' do sign_in_and_2fa_user diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 6826ff45371..20197801cf0 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -425,6 +425,19 @@ end end + context 'user attempts sign in with bad personal key' do + it 'remains on the login with personal key page' do + user = create(:user, :signed_up) + signin(user.email, user.password) + choose_another_security_option('personal_key') + enter_personal_key(personal_key: 'foo') + click_submit_default + + expect(page).to have_current_path(login_two_factor_personal_key_path) + expect(page).to have_content t('devise.two_factor_authentication.invalid_personal_key') + end + end + it 'does not whitelist style-src in CSP' do allow(FeatureManagement).to receive(:recaptcha_enabled?).and_return(true) visit root_path diff --git a/spec/services/openid_connect_redirector_spec.rb b/spec/services/openid_connect_redirector_spec.rb index a3751f3b857..0c1c621e956 100644 --- a/spec/services/openid_connect_redirector_spec.rb +++ b/spec/services/openid_connect_redirector_spec.rb @@ -35,6 +35,16 @@ end describe '#validate' do + context 'with a redirect_uri that spoofs a hostname' do + let(:redirect_uri) { 'https://example.com.evilish.com/' } + + it 'is invalid' do + redirector.validate + expect(errors[:redirect_uri]). + to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) + end + end + context 'with a valid redirect_uri' do let(:redirect_uri) { 'http://localhost:7654/result/more/extra' } it 'is valid' do