Fixes #12698 - Insufficient URL validation Smart Proxy and Medium.

Problem: The regex that validates smart proxies URLs only matches
'beginning of text'. This allows us to add just \n after a valid URL and
put anything after it. For instance, javascript:alert('hacked'). I
haven't found any link to the Foreman proxy URL so the script would not
trigger, but if we were to put link_to @smart_proxy.url somewhere (or a
    plugin did this) it would be unsafe. Same problem occurrs on Medium
path.

Solution: Make the regex match the end of the URL with \Z. I substituted
the regex by an standard one, URI.regexp so we don't have to maintain it
anymore.

Extra: I added one test for this, but other tests have been rearranged
to use stubs rather than building actual SmartProxy objects &
associations.

app/models/medium.rb

@@ -22,9 +22,7 @@ class Medium < ActiveRecord::Base
   VALID_NFS_PATH=/\A([-\w\d\.]+):(\/[\w\d\/\$\.]+)\Z/
   validates :name, :uniqueness => true, :presence => true
   validates :path, :uniqueness => true, :presence => true,
-                   :format => { :with => /\A(http|https|ftp|nfs):\/\//,
-                                :message => N_("Only URLs with schema http://, https://, ftp:// or nfs:// are allowed (e.g. nfs://server/vol/dir)")
-                              }
+    :url_schema => ['http', 'https', 'ftp', 'nfs']
   validates :media_path, :config_path, :image_path, :allow_blank => true,
                 :format => { :with => VALID_NFS_PATH, :message => N_("does not appear to be a valid nfs mount path")},
                 :if => Proc.new { |m| m.respond_to? :media_path }

app/models/smart_proxy.rb

@@ -19,10 +19,9 @@ class SmartProxy < ActiveRecord::Base
   has_many :puppet_ca_hosts, :class_name => 'Host::Managed',  :foreign_key => 'puppet_ca_proxy_id'
   has_many :puppet_ca_hostgroups, :class_name => 'Hostgroup', :foreign_key => 'puppet_ca_proxy_id'
   has_many :realms,                                           :foreign_key => 'realm_proxy_id'
-  URL_HOSTNAME_MATCH = %r{\A(?:http|https):\/\/([^:\/]+)}
   validates :name, :uniqueness => true, :presence => true
-  validates :url, :presence => true, :format => { :with => URL_HOSTNAME_MATCH, :message => N_('is invalid - only  http://, https:// are allowed') },
-            :uniqueness     => { :message => N_('Only one declaration of a proxy is allowed') }
+  validates :url, :presence => true, :url_schema => ['http', 'https'],
+    :uniqueness => { :message => N_('Only one declaration of a proxy is allowed') }
 
   # There should be no problem with associating features before the proxy is saved as the whole operation is in a transaction
   before_save :sanitize_url, :associate_features
@@ -45,16 +44,12 @@ class SmartProxy < ActiveRecord::Base
                  Net::HTTPBadResponse, Net::HTTPHeaderSyntaxError, Net::ProtocolError]
 
   def hostname
-    # This will always match as it is validated
-    url.match(URL_HOSTNAME_MATCH)[1]
+    URI(url).host
   end
 
   def to_s
-    if Setting[:legacy_puppet_hostname]
-      hostname =~ /^puppet\./ ? 'puppet' : hostname
-    else
-      hostname
-    end
+    return hostname unless Setting[:legacy_puppet_hostname]
+    hostname.match(/^puppet\./) ? 'puppet' : hostname
   end
 
   def self.smart_proxy_ids_for(hosts)

app/validators/url_schema_validator.rb

@@ -0,0 +1,14 @@
+class UrlSchemaValidator < ActiveModel::EachValidator
+  def initialize(args)
+    @schemas = args[:in]
+    super
+  end
+
+  def validate_each(record, attribute, value)
+    unless value =~ /\A#{URI.regexp(@schemas)}\z/
+      error_message = _('URL must be valid and schema must be one of %s') %
+        @schemas.to_sentence
+      record.errors.add(attribute, error_message)
+    end
+  end
+end

test/unit/medium_test.rb

@@ -27,18 +27,24 @@ class MediumTest < ActiveSupport::TestCase
     assert !other_medium.save
   end
 
-  test "path can't be blank" do
-    medium = Medium.new :name => "Archlinux mirror", :path => "  "
-    assert medium.path.strip.empty?
-    assert !medium.save
-  end
-
-  test "path must be unique" do
-    medium = Medium.new :name => "Archlinux mirror", :path => "http://www.google.com"
-    assert medium.save!
-
-    other_medium = Medium.new :name => "Ubuntu mirror", :path => "http://www.google.com"
-    assert !other_medium.save
+  context 'path validations' do
+    setup do
+      @medium = FactoryGirl.build(:medium)
+    end
+
+    test "can't be blank" do
+      @medium.path = '  '
+      assert @medium.path.strip.empty?
+      refute_valid @medium
+    end
+
+    test 'must be unique' do
+      @medium.path = 'http://www.google.com'
+      assert @medium.save!
+
+      other_medium = FactoryGirl.build(:medium, :path => @medium.path)
+      refute_valid other_medium
+    end
   end
 
   test "should destroy and nullify host.medium_id if medium is in use but host.build? is false" do

test/unit/smart_proxy_test.rb

@@ -1,51 +1,47 @@
 require 'test_helper'
 
 class SmartProxyTest < ActiveSupport::TestCase
-  test "should be valid" do
-    proxy = SmartProxy.new
-    proxy.name = "test proxy"
-    proxy.url  = "https://secure.proxy:4568"
-    assert proxy.valid?
-  end
+  context 'url validations' do
+    setup do
+      @proxy = FactoryGirl.
+        build_stubbed(:smart_proxy, :url => 'https://secure.proxy:4568')
+    end
 
-  test "should not be modified if has no leading slashes" do
-    proxy = SmartProxy.new
-    proxy.name = "test proxy"
-    proxy.url  = "https://secure.proxy:4568"
-    assert proxy.valid?
-    assert_equal proxy.url, "https://secure.proxy:4568"
-  end
+    test "should be valid" do
+      assert_valid @proxy
+    end
 
-  test "should not include trailing slash" do
-    proxy = SmartProxy.new
-    proxy.name = "test a proxy"
-    proxy.url  = "http://some.proxy:4568/"
-    as_admin do
-      assert proxy.save
+    test "should not be modified if has no leading slashes" do
+      assert_equal @proxy.url, "https://secure.proxy:4568"
     end
-    assert_equal proxy.url, "http://some.proxy:4568"
   end
 
-  test "should honor legacy puppet hostname true setting" do
-    Setting[:legacy_puppet_hostname] = true
-    proxy = SmartProxy.new
-    proxy.name = "test proxy"
-    proxy.url = "http://puppet.example.com:4568"
-
-    assert_equal proxy.to_s, "puppet"
+  test "should not include trailing slash" do
+    @proxy = FactoryGirl.build(:smart_proxy)
+    @proxy.url = 'http://some.proxy:4568/'
+    as_admin { assert @proxy.save }
+    assert_equal @proxy.url, "http://some.proxy:4568"
   end
 
-  test "should honor legacy puppet hostname false setting" do
-    Setting[:legacy_puppet_hostname] = false
-    proxy = SmartProxy.new
-    proxy.name = "test proxy"
-    proxy.url = "http://puppet.example.com:4568"
+  context 'legacy puppet hostname' do
+    setup do
+      @proxy = FactoryGirl.build_stubbed(:smart_proxy)
+      @proxy.url = "http://puppet.example.com:4568"
+    end
 
-    assert_equal proxy.to_s, "puppet.example.com"
+    test "when true returns puppet part of hostname" do
+      Setting.expects(:[]).with(:legacy_puppet_hostname).returns(true)
+      assert_equal "puppet", @proxy.to_s
+    end
+
+    test "when false returns whole hostname" do
+      Setting.expects(:[]).with(:legacy_puppet_hostname).returns(false)
+      assert_equal "puppet.example.com", @proxy.to_s
+    end
   end
 
   test "proxy should respond correctly to has_feature? method" do
-    proxy = FactoryGirl.create(:template_smart_proxy)
+    proxy = FactoryGirl.build_stubbed(:template_smart_proxy)
     assert proxy.has_feature?('Templates')
     refute proxy.has_feature?('Puppet CA')
   end

test/unit/validators/url_schema_validator_test.rb

@@ -0,0 +1,29 @@
+require 'test_helper'
+
+class UrlSchemaValidatorTest < ActiveSupport::TestCase
+  class Validatable
+    include ActiveModel::Validations
+    validates :url, :url_schema => ['http', 'https', 'nfs', 'ftp']
+    attr_accessor :url
+  end
+
+  setup do
+    @validatable = Validatable.new
+  end
+
+  test 'url regexp does not match new lines' do
+    @validatable.url = "http://puppet.example.com:4568\njavascript('alert')"
+    refute_valid @validatable
+  end
+
+  test 'passes if url uses one of the specified schemas' do
+    @validatable.url = 'ftp://puppet.example.com:4568'
+    assert_valid @validatable
+  end
+
+  test 'fails if url contains the wrong schema' do
+    @validatable.url = 'unix://puppet.example.com:4568'
+    refute_valid @validatable
+    assert_match /URL must be valid/, @validatable.errors.messages.to_s
+  end
+end