From c38b31cea823eb970804e29d4fdc0b725ba77b43 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Tue, 31 Oct 2017 19:38:04 -0500 Subject: [PATCH 1/6] Validate redirect uris are parsable uris **Why**: We need to parse the URIs in the CORS middleware to determine which origins are allowed. With this PR we can be confident that the URIs are properly formed and not need to rescue from URI parsing errors. --- app/models/service_provider.rb | 17 +++++++++++++++++ config/application.rb | 7 +------ spec/models/service_provider_spec.rb | 19 +++++++++++++++++++ spec/requests/openid_connect_cors_spec.rb | 10 ---------- 4 files changed, 37 insertions(+), 16 deletions(-) diff --git a/app/models/service_provider.rb b/app/models/service_provider.rb index f065650df5d..506c43d5f3a 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,19 @@ def encryption_opts def live? active? && approved? end + + private + + def redirect_uris_are_parsable + return if redirect_uris.blank? + + redirect_uris.each do |uri| + begin + next if uri =~ URI::DEFAULT_PARSER.regexp[:ABS_URI] && URI.parse(uri) + errors.add(:redirect_uris, :invalid) + rescue URI::BadURIError, URI::InvalidURIError + errors.add(:redirect_uris, :invalid) + end + end + end end diff --git a/config/application.rb b/config/application.rb index 4ecc6cc3069..d83d393f0eb 100644 --- a/config/application.rb +++ b/config/application.rb @@ -41,12 +41,7 @@ class Application < Rails::Application 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::BadURIError => err - Rails.logger.warn({ warning: err.class.to_s, source: 'Rack::Cors', uri: uri }.to_json) - '' - end + URI.join(uri, '/').to_s[0..-2] end.include?(source) end resource '/.well-known/openid-configuration', headers: :any, methods: [:get] diff --git a/spec/models/service_provider_spec.rb b/spec/models/service_provider_spec.rb index f4d864e1c9a..9d038abf0b6 100644 --- a/spec/models/service_provider_spec.rb +++ b/spec/models/service_provider_spec.rb @@ -1,6 +1,25 @@ 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']) + 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(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 068344b4375..6214aaaa69e 100644 --- a/spec/requests/openid_connect_cors_spec.rb +++ b/spec/requests/openid_connect_cors_spec.rb @@ -23,16 +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 end describe 'certs endpoint' do From 28188fdb0c98dc75f6a6de72f61a5b921aa1f59f Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Tue, 31 Oct 2017 19:50:11 -0500 Subject: [PATCH 2/6] refactor to fix reek issue --- app/models/service_provider.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/service_provider.rb b/app/models/service_provider.rb index 506c43d5f3a..cca525b8b71 100644 --- a/app/models/service_provider.rb +++ b/app/models/service_provider.rb @@ -54,9 +54,10 @@ def redirect_uris_are_parsable redirect_uris.each do |uri| begin next if uri =~ URI::DEFAULT_PARSER.regexp[:ABS_URI] && URI.parse(uri) - errors.add(:redirect_uris, :invalid) + raise URI::InvalidURIError rescue URI::BadURIError, URI::InvalidURIError errors.add(:redirect_uris, :invalid) + break end end end From 888484481789624a254738b7585d780f1a6cfc89 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 2 Nov 2017 08:52:24 -0500 Subject: [PATCH 3/6] use regex instead of URI.parse --- config/application.rb | 11 ++++++++--- spec/requests/openid_connect_cors_spec.rb | 12 ++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/config/application.rb b/config/application.rb index d83d393f0eb..078817b31cf 100644 --- a/config/application.rb +++ b/config/application.rb @@ -40,9 +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| - URI.join(uri, '/').to_s[0..-2] - end.include?(source) + next if source == Figaro.env.domain_name + + ServiceProvider.pluck(:redirect_uris).flatten.compact.find do |uri| + match = URI::DEFAULT_PARSER.regexp[:ABS_URI].match(uri) + parsed_uri = "#{match[1]}://#{match[4]}" + parsed_uri += ":#{match[5]}" if match[5].present? + source == parsed_uri + 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/requests/openid_connect_cors_spec.rb b/spec/requests/openid_connect_cors_spec.rb index 6214aaaa69e..09f3e627ac2 100644 --- a/spec/requests/openid_connect_cors_spec.rb +++ b/spec/requests/openid_connect_cors_spec.rb @@ -146,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 From 60f7fb6e78878773d4580b8ad2ebc3a197328ec1 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 2 Nov 2017 13:49:04 -0500 Subject: [PATCH 4/6] clean up validation code and faster sp uri resolution --- app/models/service_provider.rb | 17 ++++++++++------- config/application.rb | 8 ++++---- spec/models/service_provider_spec.rb | 2 ++ 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/app/models/service_provider.rb b/app/models/service_provider.rb index cca525b8b71..222ab58776a 100644 --- a/app/models/service_provider.rb +++ b/app/models/service_provider.rb @@ -52,13 +52,16 @@ def redirect_uris_are_parsable return if redirect_uris.blank? redirect_uris.each do |uri| - begin - next if uri =~ URI::DEFAULT_PARSER.regexp[:ABS_URI] && URI.parse(uri) - raise URI::InvalidURIError - rescue URI::BadURIError, URI::InvalidURIError - errors.add(:redirect_uris, :invalid) - break - end + 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/config/application.rb b/config/application.rb index 078817b31cf..e4a54896b2d 100644 --- a/config/application.rb +++ b/config/application.rb @@ -43,10 +43,10 @@ class Application < Rails::Application next if source == Figaro.env.domain_name ServiceProvider.pluck(:redirect_uris).flatten.compact.find do |uri| - match = URI::DEFAULT_PARSER.regexp[:ABS_URI].match(uri) - parsed_uri = "#{match[1]}://#{match[4]}" - parsed_uri += ":#{match[5]}" if match[5].present? - source == parsed_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] diff --git a/spec/models/service_provider_spec.rb b/spec/models/service_provider_spec.rb index 9d038abf0b6..3d43f1d80e4 100644 --- a/spec/models/service_provider_spec.rb +++ b/spec/models/service_provider_spec.rb @@ -4,11 +4,13 @@ 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 From c52ef8770b4962c65fffd13788028aff5d69632c Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 2 Nov 2017 14:29:27 -0500 Subject: [PATCH 5/6] raise in service provider updater if service provider is invalid --- app/services/service_provider_updater.rb | 2 +- .../services/service_provider_updater_spec.rb | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) 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/spec/services/service_provider_updater_spec.rb b/spec/services/service_provider_updater_spec.rb index 8f62726fe72..49374295558 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 + ) + subject.run + 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) From ea47b3e83c7c4aff6d203bffee5972a348849585 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 2 Nov 2017 15:27:53 -0500 Subject: [PATCH 6/6] add a test to raise for validation errors in service provider seeder --- app/services/service_provider_seeder.rb | 2 +- spec/services/service_provider_seeder_spec.rb | 19 +++++++++++++++++++ .../services/service_provider_updater_spec.rb | 2 +- 3 files changed, 21 insertions(+), 2 deletions(-) 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/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 49374295558..d4214ebcdb8 100644 --- a/spec/services/service_provider_updater_spec.rb +++ b/spec/services/service_provider_updater_spec.rb @@ -176,7 +176,7 @@ status: 200, body: dashboard_service_providers.to_json ) - subject.run + expect { subject.run }.to raise_error(ActiveRecord::RecordInvalid) end end