From 98f6ca54bd689c2df59cedb41d724f6e7c19a83f Mon Sep 17 00:00:00 2001 From: Daniel Lobato Date: Fri, 4 Dec 2015 11:50:40 +0100 Subject: [PATCH] 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 | 4 +- app/models/smart_proxy.rb | 15 ++--- app/validators/url_schema_validator.rb | 14 ++++ test/unit/medium_test.rb | 30 +++++---- test/unit/smart_proxy_test.rb | 64 +++++++++---------- .../validators/url_schema_validator_test.rb | 29 +++++++++ 6 files changed, 97 insertions(+), 59 deletions(-) create mode 100644 app/validators/url_schema_validator.rb create mode 100644 test/unit/validators/url_schema_validator_test.rb diff --git a/app/models/medium.rb b/app/models/medium.rb index a27042cebc0..348c3129a01 100644 --- a/app/models/medium.rb +++ b/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 } diff --git a/app/models/smart_proxy.rb b/app/models/smart_proxy.rb index 7ebd1ad3310..3012457bbff 100644 --- a/app/models/smart_proxy.rb +++ b/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) diff --git a/app/validators/url_schema_validator.rb b/app/validators/url_schema_validator.rb new file mode 100644 index 00000000000..6d406ac038c --- /dev/null +++ b/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 diff --git a/test/unit/medium_test.rb b/test/unit/medium_test.rb index bacb901a17b..e47246bd0a0 100644 --- a/test/unit/medium_test.rb +++ b/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 diff --git a/test/unit/smart_proxy_test.rb b/test/unit/smart_proxy_test.rb index dfe97230c7c..55c97859305 100644 --- a/test/unit/smart_proxy_test.rb +++ b/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 diff --git a/test/unit/validators/url_schema_validator_test.rb b/test/unit/validators/url_schema_validator_test.rb new file mode 100644 index 00000000000..1c59800f284 --- /dev/null +++ b/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