diff --git a/app/models/service_provider.rb b/app/models/service_provider.rb index f065650df5d..222ab58776a 100644 --- a/app/models/service_provider.rb +++ b/app/models/service_provider.rb @@ -3,6 +3,8 @@ class ServiceProvider < ApplicationRecord scope(:active, -> { where(active: true) }) + validate :redirect_uris_are_parsable + def self.from_issuer(issuer) find_by(issuer: issuer) || NullServiceProvider.new(issuer: issuer) end @@ -43,4 +45,23 @@ def encryption_opts def live? active? && approved? end + + private + + def redirect_uris_are_parsable + return if redirect_uris.blank? + + redirect_uris.each do |uri| + next if redirect_uri_valid?(uri) + errors.add(:redirect_uris, :invalid) + break + end + end + + def redirect_uri_valid?(redirect_uri) + parsed_uri = URI.parse(redirect_uri) + parsed_uri.scheme.present? || parsed_uri.host.present? + rescue URI::BadURIError, URI::InvalidURIError + false + end end diff --git a/app/services/service_provider_seeder.rb b/app/services/service_provider_seeder.rb index cb5e3c06f80..8073304b28d 100644 --- a/app/services/service_provider_seeder.rb +++ b/app/services/service_provider_seeder.rb @@ -13,7 +13,7 @@ def run sp.approved = true sp.active = true sp.native = true - end.update(config.except('restrict_to_deploy_env')) + end.update!(config.except('restrict_to_deploy_env')) end end diff --git a/app/services/service_provider_updater.rb b/app/services/service_provider_updater.rb index a8b20fadff5..ecf08217b6a 100644 --- a/app/services/service_provider_updater.rb +++ b/app/services/service_provider_updater.rb @@ -36,7 +36,7 @@ def create_or_update_service_provider(issuer, service_provider) def sync_model(sp, cleaned_attributes) if sp.is_a?(NullServiceProvider) - ServiceProvider.create(cleaned_attributes) + ServiceProvider.create!(cleaned_attributes) else sp.attributes = cleaned_attributes sp.save! diff --git a/config/application.rb b/config/application.rb index 9106c7bbfd7..e4a54896b2d 100644 --- a/config/application.rb +++ b/config/application.rb @@ -40,14 +40,14 @@ class Application < Rails::Application config.middleware.insert_before 0, Rack::Cors do allow do origins do |source, _env| - ServiceProvider.pluck(:redirect_uris).flatten.compact.map do |uri| - begin - URI.join(uri, '/').to_s[0..-2] - rescue URI::Error => err - Rails.logger.warn({ warning: err.class.to_s, source: 'Rack::Cors', uri: uri }.to_json) - '' - end - end.include?(source) + next if source == Figaro.env.domain_name + + ServiceProvider.pluck(:redirect_uris).flatten.compact.find do |uri| + split_uri = uri.split('//') + protocol = split_uri[0] + domain = split_uri[1].split('/')[0] + source == "#{protocol}//#{domain}" + end.present? end resource '/.well-known/openid-configuration', headers: :any, methods: [:get] resource '/api/openid_connect/certs', headers: :any, methods: [:get] diff --git a/spec/models/service_provider_spec.rb b/spec/models/service_provider_spec.rb index 75e0f4a7339..3e50f869581 100644 --- a/spec/models/service_provider_spec.rb +++ b/spec/models/service_provider_spec.rb @@ -1,6 +1,27 @@ require 'rails_helper' describe ServiceProvider do + describe 'validations' do + it 'validates that all redirect_uris are absolute, parsable uris' do + valid_sp = build(:service_provider, redirect_uris: ['http://foo.com']) + missing_protocol_sp = build(:service_provider, redirect_uris: ['foo.com']) + empty_uri_sp = build(:service_provider, redirect_uris: ['']) + relative_uri_sp = build(:service_provider, redirect_uris: ['/asdf/hjkl']) + bad_uri_sp = build(:service_provider, redirect_uris: [' http://foo.com']) + + expect(valid_sp).to be_valid + expect(missing_protocol_sp).to_not be_valid + expect(empty_uri_sp).to_not be_valid + expect(relative_uri_sp).to_not be_valid + expect(bad_uri_sp).to_not be_valid + end + + it 'allows redirect_uris to be blank' do + sp = build(:service_provider, redirect_uris: nil) + expect(sp).to be_valid + end + end + describe '#issuer' do it 'returns the constructor value' do sp = ServiceProvider.from_issuer('http://localhost:3000') diff --git a/spec/requests/openid_connect_cors_spec.rb b/spec/requests/openid_connect_cors_spec.rb index d780d34f789..09f3e627ac2 100644 --- a/spec/requests/openid_connect_cors_spec.rb +++ b/spec/requests/openid_connect_cors_spec.rb @@ -23,26 +23,6 @@ end end end - - context 'with a bad URI in the database' do - before { create(:service_provider, redirect_uris: %w[/foobar]) } - - it 'does not blow up' do - get api_openid_connect_certs_path, headers: { 'HTTP_ORIGIN' => 'https://example.com' } - - expect(response).to be_ok - end - end - - context 'with an invalid URI in the database' do - before { create(:service_provider, redirect_uris: [' https://foo.com']) } - - it 'does not blow up' do - get api_openid_connect_certs_path, headers: { 'HTTP_ORIGIN' => 'https://example.com' } - - expect(response).to be_ok - end - end end describe 'certs endpoint' do @@ -166,4 +146,16 @@ end end end + + describe 'domain name as the origin' do + it 'does not load any ServiceProvider objects' do + stub_const 'ServiceProvider', double + get openid_connect_configuration_path, headers: { 'HTTP_ORIGIN' => Figaro.env.domain_name } + + aggregate_failures do + expect(response).to be_ok + expect(response['Access-Control-Allow-Origin']).to be_nil + end + end + end end diff --git a/spec/services/service_provider_seeder_spec.rb b/spec/services/service_provider_seeder_spec.rb index a536580a837..537a424c371 100644 --- a/spec/services/service_provider_seeder_spec.rb +++ b/spec/services/service_provider_seeder_spec.rb @@ -94,6 +94,25 @@ to be_present end end + + context 'when a service provider is invalid' do + let(:invalid_service_providers) do + { + 'https://rp2.serviceprovider.com/auth/saml/metadata' => { + acs_url: 'http://example.com/test/saml/decode_assertion', + assertion_consumer_logout_service_url: 'http://example.com/test/saml/decode_slo_request', + block_encryption: 'aes256-cbc', + cert: 'saml_test_sp', + redirect_uris: [''], + }, + } + end + + it 'raises an error' do + expect(instance).to receive(:service_providers).and_return(invalid_service_providers) + expect { run }.to raise_error(ActiveRecord::RecordInvalid) + end + end end end end diff --git a/spec/services/service_provider_updater_spec.rb b/spec/services/service_provider_updater_spec.rb index 8f62726fe72..d4214ebcdb8 100644 --- a/spec/services/service_provider_updater_spec.rb +++ b/spec/services/service_provider_updater_spec.rb @@ -148,6 +148,38 @@ end end + context 'a non-native servce provider is invalid' do + let(:dashboard_service_providers) do + [ + { + id: 'big number', + created_at: '2010-01-01 00:00:00'.to_datetime, + updated_at: '2010-01-01 00:00:00'.to_datetime, + issuer: dashboard_sp_issuer, + agency: 'a service provider', + friendly_name: 'a friendly service provider', + description: 'user friendly login.gov dashboard', + acs_url: 'http://sp.example.org/saml/login', + assertion_consumer_logout_service_url: 'http://sp.example.org/saml/logout', + block_encryption: 'aes256-cbc', + cert: saml_test_sp_cert, + active: true, + native: false, + approved: true, + redirect_uris: [''], + }, + ] + end + + it 'raises an error' do + stub_request(:get, fake_dashboard_url).to_return( + status: 200, + body: dashboard_service_providers.to_json + ) + expect { subject.run }.to raise_error(ActiveRecord::RecordInvalid) + end + end + context 'GET request to dashboard raises an error' do it 'logs error and does not affect registry' do allow(Rails.logger).to receive(:error)