From a7a9bf1dfa81cb553689f43c0f67ab2f9234934f Mon Sep 17 00:00:00 2001 From: Jonathan Perichon Date: Wed, 11 Oct 2017 11:55:58 -0700 Subject: [PATCH 1/2] Improve validation against local hosts --- lib/validate_url.rb | 44 ++++++++++++++++++++++++++++++++++++++- spec/validate_url_spec.rb | 17 ++++++++++++--- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/lib/validate_url.rb b/lib/validate_url.rb index 003185a..f3b24c8 100644 --- a/lib/validate_url.rb +++ b/lib/validate_url.rb @@ -1,5 +1,7 @@ require 'active_model' require 'active_support/i18n' +require 'ipaddr' +require 'resolv' I18n.load_path += Dir[File.dirname(__FILE__) + "/locale/*.yml"] module ActiveModel @@ -7,6 +9,20 @@ module Validations class UrlValidator < ActiveModel::EachValidator RESERVED_OPTIONS = [:schemes, :no_local] + BLACKLISTED_INTERNAL_IPS = [ + IPAddr.new('10.0.0.0/8'), + IPAddr.new('127.0.0.0/8'), + IPAddr.new('192.168.0.0/16'), + IPAddr.new('169.254.0.0/16'), + IPAddr.new('224.0.0.0/4'), + IPAddr.new('0.0.0.0/8'), + IPAddr.new('255.255.255.255'), + IPAddr.new('::1'), + IPAddr.new('fc00::/7'), + IPAddr.new('fd00::/8'), + IPAddr.new('fe80::/10') + ].freeze + def initialize(options) options.reverse_merge!(:schemes => %w(http https)) options.reverse_merge!(:message => :url) @@ -19,14 +35,40 @@ def validate_each(record, attribute, value) schemes = [*options.fetch(:schemes)].map(&:to_s) begin uri = URI.parse(value) - unless uri && uri.host && schemes.include?(uri.scheme) && (!options.fetch(:no_local) || uri.host.include?('.')) + hostname = uri.host || uri.to_s + + unless uri && uri.host && schemes.include?(uri.scheme) + record.errors.add(attribute, :url, filtered_options(value)) + return + end + + if options.fetch(:no_local) && self.class.local?(hostname) record.errors.add(attribute, :url, filtered_options(value)) + return end rescue URI::InvalidURIError record.errors.add(attribute, :url, filtered_options(value)) end end + def self.local?(hostname) + ip = begin + Resolv.getaddress(hostname) + rescue Resolv::ResolvError + nil + end + + return false unless ip + + ip_addr = IPAddr.new(ip) + + BLACKLISTED_INTERNAL_IPS.any? do |blacklisted_ip| + # note 1: explicit usage of triple equals operator + # note 2: a === b is not equal to b === a when dealing with IPAddr + blacklisted_ip === ip_addr + end + end + protected def filtered_options(value) diff --git a/spec/validate_url_spec.rb b/spec/validate_url_spec.rb index cdfb278..c21bcae 100644 --- a/spec/validate_url_spec.rb +++ b/spec/validate_url_spec.rb @@ -158,9 +158,20 @@ expect(@user).not_to be_valid end - it "should not allow weird urls that get interpreted as local hostnames" do - @user.homepage = "http://http://example.com" - expect(@user).not_to be_valid + [ + 'https://127.0.0.1', + 'https://192.168.1.13', + 'https://127.0.254.254', + 'https://10.100.103.243', + 'https://127.0.0.1:5555', + 'https://127.0.0.1.xip.io', + 'https://169.254.1.1', + 'https://0:0:0:0:0:ffff:7f00:1' + ].each do |url| + it "should not allow #{url}" do + @user.homepage = url + expect(@user).not_to be_valid + end end end From 1ce9586cf9ddd7f6c34fa376ff4197c461510e07 Mon Sep 17 00:00:00 2001 From: Jonathan Perichon Date: Wed, 11 Oct 2017 11:57:09 -0700 Subject: [PATCH 2/2] Support validation against blacklisted domains --- lib/validate_url.rb | 19 ++++++++++++++++- .../user_with_blacklisted_domains.rb | 9 ++++++++ spec/spec_helper.rb | 19 +++++++++-------- spec/validate_url_spec.rb | 21 +++++++++++++++++++ 4 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 spec/resources/user_with_blacklisted_domains.rb diff --git a/lib/validate_url.rb b/lib/validate_url.rb index f3b24c8..47940a9 100644 --- a/lib/validate_url.rb +++ b/lib/validate_url.rb @@ -7,7 +7,7 @@ module ActiveModel module Validations class UrlValidator < ActiveModel::EachValidator - RESERVED_OPTIONS = [:schemes, :no_local] + RESERVED_OPTIONS = [:schemes, :no_local, :blacklisted_domains] BLACKLISTED_INTERNAL_IPS = [ IPAddr.new('10.0.0.0/8'), @@ -27,6 +27,7 @@ def initialize(options) options.reverse_merge!(:schemes => %w(http https)) options.reverse_merge!(:message => :url) options.reverse_merge!(:no_local => false) + options.reverse_merge!(:blacklisted_domains => []) super(options) end @@ -46,11 +47,27 @@ def validate_each(record, attribute, value) record.errors.add(attribute, :url, filtered_options(value)) return end + + if options.fetch(:blacklisted_domains).any? && blacklisted_domain?(hostname) + record.errors.add(attribute, :url, filtered_options(value)) + return + end rescue URI::InvalidURIError record.errors.add(attribute, :url, filtered_options(value)) end end + def blacklisted_domain?(hostname) + blacklisted_domains_regex = Regexp.new( + options[:blacklisted_domains].map do |blacklisted_domain| + %r{^([\w-]+\.)?#{Regexp.escape(blacklisted_domain)}} + end.join('|'), + Regexp::IGNORECASE + ) + + (hostname =~ blacklisted_domains_regex).present? + end + def self.local?(hostname) ip = begin Resolv.getaddress(hostname) diff --git a/spec/resources/user_with_blacklisted_domains.rb b/spec/resources/user_with_blacklisted_domains.rb new file mode 100644 index 0000000..2e8cf3d --- /dev/null +++ b/spec/resources/user_with_blacklisted_domains.rb @@ -0,0 +1,9 @@ +require 'active_model/validations' + +class UserWithBlacklistedDomains + include ActiveModel::Validations + + attr_accessor :homepage + + validates :homepage, :url => {:blacklisted_domains => ['example.net']} +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index d5b1a00..6989592 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -16,12 +16,13 @@ require File.join(File.dirname(__FILE__), '..', 'init') -autoload :User, 'resources/user' -autoload :UserWithNil, 'resources/user_with_nil' -autoload :UserWithBlank, 'resources/user_with_blank' -autoload :UserWithLegacySyntax, 'resources/user_with_legacy_syntax' -autoload :UserWithAr, 'resources/user_with_ar' -autoload :UserWithArLegacy, 'resources/user_with_ar_legacy' -autoload :UserWithCustomScheme, 'resources/user_with_custom_scheme' -autoload :UserWithCustomMessage, 'resources/user_with_custom_message' -autoload :UserWithNoLocal, 'resources/user_with_no_local' +autoload :User, 'resources/user' +autoload :UserWithNil, 'resources/user_with_nil' +autoload :UserWithBlank, 'resources/user_with_blank' +autoload :UserWithLegacySyntax, 'resources/user_with_legacy_syntax' +autoload :UserWithAr, 'resources/user_with_ar' +autoload :UserWithArLegacy, 'resources/user_with_ar_legacy' +autoload :UserWithCustomScheme, 'resources/user_with_custom_scheme' +autoload :UserWithCustomMessage, 'resources/user_with_custom_message' +autoload :UserWithNoLocal, 'resources/user_with_no_local' +autoload :UserWithBlacklistedDomains, 'resources/user_with_blacklisted_domains' diff --git a/spec/validate_url_spec.rb b/spec/validate_url_spec.rb index c21bcae..2dfef3f 100644 --- a/spec/validate_url_spec.rb +++ b/spec/validate_url_spec.rb @@ -175,6 +175,27 @@ end end + context "with blacklisted_domains" do + before do + @user = UserWithBlacklistedDomains.new + end + + it "should allow a valid internet url" do + @user.homepage = "http://www.example.com" + expect(@user).to be_valid + end + + it "should not allow a blacklisted domain" do + @user.homepage = "http://example.net" + expect(@user).not_to be_valid + end + + it "should not allow a blacklisted subdomain" do + @user.homepage = "http://sub.example.net" + expect(@user).not_to be_valid + end + end + context "with legacy syntax" do before do @user = UserWithLegacySyntax.new