Merge pull request #1762 from 18F/jmhooper-validate-redirect-uris

Validate redirect uris are parsable uris

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

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
 

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!

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]

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')

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

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

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)