Align UrlValidator to validate_url gem implementation.

Renamed UrlValidator to AddressableUrlValidator to avoid 'url:' naming collision with ActiveModel::Validations::UrlValidator in 'validates' statement.
Make use of the options attribute of the parent class ActiveModel::EachValidator.
Add more options: allow_nil, allow_blank, message.
Renamed 'protocols' option to 'schemes' to match the option naming from UrlValidator.
This commit is contained in:
Thong Kuah 2019-04-11 06:29:07 +00:00 committed by James Lopez
parent 79bf4bdaad
commit d119d3d1b2
21 changed files with 292 additions and 172 deletions

View File

@ -48,17 +48,17 @@ class ApplicationSetting < ApplicationRecord
validates :home_page_url,
allow_blank: true,
url: true,
addressable_url: true,
if: :home_page_url_column_exists?
validates :help_page_support_url,
allow_blank: true,
url: true,
addressable_url: true,
if: :help_page_support_url_column_exists?
validates :after_sign_out_path,
allow_blank: true,
url: true
addressable_url: true
validates :admin_notification_email,
devise_email: true,
@ -218,7 +218,7 @@ class ApplicationSetting < ApplicationRecord
if: :external_authorization_service_enabled
validates :external_authorization_service_url,
url: true, allow_blank: true,
addressable_url: true, allow_blank: true,
if: :external_authorization_service_enabled
validates :external_authorization_service_timeout,

View File

@ -22,7 +22,7 @@ class Badge < ApplicationRecord
scope :order_created_at_asc, -> { reorder(created_at: :asc) }
validates :link_url, :image_url, url: { protocols: %w(http https) }
validates :link_url, :image_url, addressable_url: true
validates :type, presence: true
def rendered_link_url(project = nil)

View File

@ -13,7 +13,7 @@ module Ci
belongs_to :build, class_name: 'Ci::Build', inverse_of: :runner_session
validates :build, presence: true
validates :url, url: { protocols: %w(https) }
validates :url, addressable_url: { schemes: %w(https) }
def terminal_specification
wss_url = Gitlab::UrlHelpers.as_wss(self.url)

View File

@ -35,7 +35,7 @@ class Environment < ApplicationRecord
validates :external_url,
length: { maximum: 255 },
allow_nil: true,
url: true
addressable_url: true
delegate :stop_action, :manual_actions, to: :last_deployment, allow_nil: true

View File

@ -22,7 +22,7 @@ module ErrorTracking
belongs_to :project
validates :api_url, length: { maximum: 255 }, public_url: true, url: { enforce_sanitization: true, ascii_only: true }, allow_nil: true
validates :api_url, length: { maximum: 255 }, public_url: { enforce_sanitization: true, ascii_only: true }, allow_nil: true
validates :api_url, presence: { message: 'is a required field' }, if: :enabled

View File

@ -3,7 +3,7 @@
class GenericCommitStatus < CommitStatus
before_validation :set_default_values
validates :target_url, url: true,
validates :target_url, addressable_url: true,
length: { maximum: 255 },
allow_nil: true

View File

@ -327,7 +327,7 @@ class Project < ApplicationRecord
validates :namespace, presence: true
validates :name, uniqueness: { scope: :namespace_id }
validates :import_url, public_url: { protocols: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS },
validates :import_url, public_url: { schemes: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS },
ports: ->(project) { project.persisted? ? VALID_MIRROR_PORTS : VALID_IMPORT_PORTS },
enforce_user: true }, if: [:external_import?, :import_url_changed?]
validates :star_count, numericality: { greater_than_or_equal_to: 0 }

View File

@ -6,7 +6,7 @@ module Releases
belongs_to :release
validates :url, presence: true, url: { protocols: %w(http https ftp) }, uniqueness: { scope: :release }
validates :url, presence: true, addressable_url: { schemes: %w(http https ftp) }, uniqueness: { scope: :release }
validates :name, presence: true, uniqueness: { scope: :release }
scope :sorted, -> { order(created_at: :desc) }

View File

@ -17,7 +17,7 @@ class RemoteMirror < ApplicationRecord
belongs_to :project, inverse_of: :remote_mirrors
validates :url, presence: true, public_url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true }
validates :url, presence: true, public_url: { schemes: %w(ssh git http https), allow_blank: true, enforce_user: true }
before_save :set_new_remote_name, if: :mirror_url_changed?

View File

@ -0,0 +1,112 @@
# frozen_string_literal: true
# AddressableUrlValidator
#
# Custom validator for URLs. This is a stricter version of UrlValidator - it also checks
# for using the right protocol, but it actually parses the URL checking for any syntax errors.
# The regex is also different from `URI` as we use `Addressable::URI` here.
#
# By default, only URLs for the HTTP(S) schemes will be considered valid.
# Provide a `:schemes` option to configure accepted schemes.
#
# Example:
#
# class User < ActiveRecord::Base
# validates :personal_url, addressable_url: true
#
# validates :ftp_url, addressable_url: { schemes: %w(ftp) }
#
# validates :git_url, addressable_url: { schemes: %w(http https ssh git) }
# end
#
# This validator can also block urls pointing to localhost or the local network to
# protect against Server-side Request Forgery (SSRF), or check for the right port.
#
# Configuration options:
# * <tt>message</tt> - A custom error message (default is: "must be a valid URL").
# * <tt>schemes</tt> - Array of URI schemes. Default: +['http', 'https']+
# * <tt>allow_localhost</tt> - Allow urls pointing to +localhost+. Default: +true+
# * <tt>allow_local_network</tt> - Allow urls pointing to private network addresses. Default: +true+
# * <tt>allow_blank</tt> - Allow urls to be +blank+. Default: +false+
# * <tt>allow_nil</tt> - Allow urls to be +nil+. Default: +false+
# * <tt>ports</tt> - Allowed ports. Default: +all+.
# * <tt>enforce_user</tt> - Validate user format. Default: +false+
# * <tt>enforce_sanitization</tt> - Validate that there are no html/css/js tags. Default: +false+
#
# Example:
# class User < ActiveRecord::Base
# validates :personal_url, addressable_url: { allow_localhost: false, allow_local_network: false}
#
# validates :web_url, addressable_url: { ports: [80, 443] }
# end
class AddressableUrlValidator < ActiveModel::EachValidator
attr_reader :record
BLOCKER_VALIDATE_OPTIONS = {
schemes: %w(http https),
ports: [],
allow_localhost: true,
allow_local_network: true,
ascii_only: false,
enforce_user: false,
enforce_sanitization: false
}.freeze
DEFAULT_OPTIONS = BLOCKER_VALIDATE_OPTIONS.merge({
message: 'must be a valid URL'
}).freeze
def initialize(options)
options.reverse_merge!(DEFAULT_OPTIONS)
super(options)
end
def validate_each(record, attribute, value)
@record = record
unless value.present?
record.errors.add(attribute, options.fetch(:message))
return
end
value = strip_value!(record, attribute, value)
Gitlab::UrlBlocker.validate!(value, blocker_args)
rescue Gitlab::UrlBlocker::BlockedUrlError => e
record.errors.add(attribute, "is blocked: #{e.message}")
end
private
def strip_value!(record, attribute, value)
new_value = value.strip
return value if new_value == value
record.public_send("#{attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend
end
def current_options
options.map do |option, value|
[option, value.is_a?(Proc) ? value.call(record) : value]
end.to_h
end
def blocker_args
current_options.slice(*BLOCKER_VALIDATE_OPTIONS.keys).tap do |args|
if self.class.allow_setting_local_requests?
args[:allow_localhost] = args[:allow_local_network] = true
end
end
end
def self.allow_setting_local_requests?
# We cannot use Gitlab::CurrentSettings as ApplicationSetting itself
# uses UrlValidator to validate urls. This ends up in a cycle
# when Gitlab::CurrentSettings creates an ApplicationSetting which then
# calls this validator.
#
# See https://gitlab.com/gitlab-org/gitlab-ee/issues/9833
ApplicationSetting.current&.allow_local_requests_from_hooks_and_services?
end
end

View File

@ -2,7 +2,7 @@
# PublicUrlValidator
#
# Custom validator for URLs. This validator works like UrlValidator but
# Custom validator for URLs. This validator works like AddressableUrlValidator but
# it blocks by default urls pointing to localhost or the local network.
#
# This validator accepts the same params UrlValidator does.
@ -12,17 +12,20 @@
# class User < ActiveRecord::Base
# validates :personal_url, public_url: true
#
# validates :ftp_url, public_url: { protocols: %w(ftp) }
# validates :ftp_url, public_url: { schemes: %w(ftp) }
#
# validates :git_url, public_url: { allow_localhost: true, allow_local_network: true}
# end
#
class PublicUrlValidator < UrlValidator
private
class PublicUrlValidator < AddressableUrlValidator
DEFAULT_OPTIONS = {
allow_localhost: false,
allow_local_network: false
}.freeze
def default_options
# By default block all urls pointing to localhost or the local network
super.merge(allow_localhost: false,
allow_local_network: false)
def initialize(options)
options.reverse_merge!(DEFAULT_OPTIONS)
super(options)
end
end

View File

@ -1,104 +0,0 @@
# frozen_string_literal: true
# UrlValidator
#
# Custom validator for URLs.
#
# By default, only URLs for the HTTP(S) protocols will be considered valid.
# Provide a `:protocols` option to configure accepted protocols.
#
# Example:
#
# class User < ActiveRecord::Base
# validates :personal_url, url: true
#
# validates :ftp_url, url: { protocols: %w(ftp) }
#
# validates :git_url, url: { protocols: %w(http https ssh git) }
# end
#
# This validator can also block urls pointing to localhost or the local network to
# protect against Server-side Request Forgery (SSRF), or check for the right port.
#
# The available options are:
# - protocols: Allowed protocols. Default: http and https
# - allow_localhost: Allow urls pointing to localhost. Default: true
# - allow_local_network: Allow urls pointing to private network addresses. Default: true
# - ports: Allowed ports. Default: all.
# - enforce_user: Validate user format. Default: false
# - enforce_sanitization: Validate that there are no html/css/js tags. Default: false
#
# Example:
# class User < ActiveRecord::Base
# validates :personal_url, url: { allow_localhost: false, allow_local_network: false}
#
# validates :web_url, url: { ports: [80, 443] }
# end
class UrlValidator < ActiveModel::EachValidator
DEFAULT_PROTOCOLS = %w(http https).freeze
attr_reader :record
def validate_each(record, attribute, value)
@record = record
unless value.present?
record.errors.add(attribute, 'must be a valid URL')
return
end
value = strip_value!(record, attribute, value)
Gitlab::UrlBlocker.validate!(value, blocker_args)
rescue Gitlab::UrlBlocker::BlockedUrlError => e
record.errors.add(attribute, "is blocked: #{e.message}")
end
private
def strip_value!(record, attribute, value)
new_value = value.strip
return value if new_value == value
record.public_send("#{attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend
end
def default_options
# By default the validator doesn't block any url based on the ip address
{
protocols: DEFAULT_PROTOCOLS,
ports: [],
allow_localhost: true,
allow_local_network: true,
ascii_only: false,
enforce_user: false,
enforce_sanitization: false
}
end
def current_options
options = self.options.map do |option, value|
[option, value.is_a?(Proc) ? value.call(record) : value]
end.to_h
default_options.merge(options)
end
def blocker_args
current_options.slice(*default_options.keys).tap do |args|
if allow_setting_local_requests?
args[:allow_localhost] = args[:allow_local_network] = true
end
end
end
def allow_setting_local_requests?
# We cannot use Gitlab::CurrentSettings as ApplicationSetting itself
# uses UrlValidator to validate urls. This ends up in a cycle
# when Gitlab::CurrentSettings creates an ApplicationSetting which then
# calls this validator.
#
# See https://gitlab.com/gitlab-org/gitlab-ee/issues/9833
ApplicationSetting.current&.allow_local_requests_from_hooks_and_services?
end
end

View File

@ -0,0 +1,5 @@
---
title: "Align UrlValidator to validate_url gem implementation"
merge_request: 27194
author: Horatiu Eugen Vlad
type: fixed

View File

@ -8,7 +8,7 @@ module Gitlab
POST_METHOD = 'POST'.freeze
INVALID_HTTP_METHOD = 'invalid. Only PUT and POST methods allowed.'.freeze
validates :url, url: true
validates :url, addressable_url: true
validate do
unless [PUT_METHOD, POST_METHOD].include?(http_method.upcase)

View File

@ -8,7 +8,7 @@ module Gitlab
BlockedUrlError = Class.new(StandardError)
class << self
def validate!(url, ports: [], protocols: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false)
def validate!(url, ports: [], schemes: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false)
return true if url.nil?
# Param url can be a string, URI or Addressable::URI
@ -20,7 +20,7 @@ module Gitlab
return true if internal?(uri)
port = get_port(uri)
validate_protocol!(uri.scheme, protocols)
validate_scheme!(uri.scheme, schemes)
validate_port!(port, ports) if ports.any?
validate_user!(uri.user) if enforce_user
validate_hostname!(uri.hostname)
@ -85,9 +85,9 @@ module Gitlab
raise BlockedUrlError, "Only allowed ports are #{ports.join(', ')}, and any over 1024"
end
def validate_protocol!(protocol, protocols)
if protocol.blank? || (protocols.any? && !protocols.include?(protocol))
raise BlockedUrlError, "Only allowed protocols are #{protocols.join(', ')}"
def validate_scheme!(scheme, schemes)
if scheme.blank? || (schemes.any? && !schemes.include?(scheme))
raise BlockedUrlError, "Only allowed schemes are #{schemes.join(', ')}"
end
end

View File

@ -79,7 +79,7 @@ describe Projects::MirrorsController do
do_put(project, remote_mirrors_attributes: remote_mirror_attributes)
expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-push-remote-settings'))
expect(flash[:alert]).to match(/Only allowed protocols are/)
expect(flash[:alert]).to match(/Only allowed schemes are/)
end
it 'does not create a RemoteMirror object' do

View File

@ -23,10 +23,10 @@ describe Gitlab::UrlBlocker do
expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git', ports: ports)).to be true
end
it 'returns true for bad protocol' do
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['https'])).to be false
it 'returns true for bad scheme' do
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', schemes: ['https'])).to be false
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['http'])).to be true
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', schemes: ['http'])).to be true
end
it 'returns true for bad protocol on configured web/SSH host and ports' do

View File

@ -306,7 +306,22 @@ describe API::CommitStatuses do
it 'responds with bad request status and validation errors' do
expect(response).to have_gitlab_http_status(400)
expect(json_response['message']['target_url'])
.to include 'is blocked: Only allowed protocols are http, https'
.to include 'is blocked: Only allowed schemes are http, https'
end
end
context 'when target URL is an unsupported scheme' do
before do
post api(post_url, developer), params: {
state: 'pending',
target_url: 'git://example.com'
}
end
it 'responds with bad request status and validation errors' do
expect(response).to have_gitlab_http_status(400)
expect(json_response['message']['target_url'])
.to include 'is blocked: Only allowed schemes are http, https'
end
end
end

View File

@ -1,15 +1,15 @@
RSpec.shared_examples 'url validator examples' do |protocols|
RSpec.shared_examples 'url validator examples' do |schemes|
let(:validator) { described_class.new(attributes: [:link_url], **options) }
let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
subject { validator.validate_each(badge, :link_url, badge.link_url) }
subject { validator.validate(badge) }
describe '#validates_each' do
describe '#validate' do
context 'with no options' do
let(:options) { {} }
it "allows #{protocols.join(',')} protocols by default" do
expect(validator.send(:default_options)[:protocols]).to eq protocols
it "allows #{schemes.join(',')} schemes by default" do
expect(validator.options[:schemes]).to eq schemes
end
it 'checks that the url structure is valid' do
@ -17,25 +17,25 @@ RSpec.shared_examples 'url validator examples' do |protocols|
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
end
context 'with protocols' do
let(:options) { { protocols: %w[http] } }
context 'with schemes' do
let(:options) { { schemes: %w(http) } }
it 'allows urls with the defined protocols' do
it 'allows urls with the defined schemes' do
subject
expect(badge.errors.empty?).to be true
expect(badge.errors).to be_empty
end
it 'add error if the url protocol does not match the selected ones' do
it 'add error if the url scheme does not match the selected ones' do
badge.link_url = 'https://www.example.com'
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
end
end

View File

@ -2,11 +2,11 @@
require 'spec_helper'
describe UrlValidator do
describe AddressableUrlValidator do
let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
subject { validator.validate_each(badge, :link_url, badge.link_url) }
subject { validator.validate(badge) }
include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS
include_examples 'url validator examples', described_class::DEFAULT_OPTIONS[:schemes]
describe 'validations' do
include_context 'invalid urls'
@ -14,13 +14,13 @@ describe UrlValidator do
let(:validator) { described_class.new(attributes: [:link_url]) }
it 'returns error when url is nil' do
expect(validator.validate_each(badge, :link_url, nil)).to be_nil
expect(badge.errors.first[1]).to eq 'must be a valid URL'
expect(validator.validate_each(badge, :link_url, nil)).to be_falsey
expect(badge.errors.first[1]).to eq validator.options.fetch(:message)
end
it 'returns error when url is empty' do
expect(validator.validate_each(badge, :link_url, '')).to be_nil
expect(badge.errors.first[1]).to eq 'must be a valid URL'
expect(validator.validate_each(badge, :link_url, '')).to be_falsey
expect(badge.errors.first[1]).to eq validator.options.fetch(:message)
end
it 'does not allow urls with CR or LF characters' do
@ -30,6 +30,17 @@ describe UrlValidator do
end
end
end
it 'provides all arguments to UrlBlock validate' do
expect(Gitlab::UrlBlocker)
.to receive(:validate!)
.with(badge.link_url, described_class::BLOCKER_VALIDATE_OPTIONS)
.and_return(true)
subject
expect(badge.errors).to be_empty
end
end
context 'by default' do
@ -40,7 +51,7 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be true
expect(badge.errors).to be_empty
end
it 'does not block urls pointing to the local network' do
@ -48,7 +59,23 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be true
expect(badge.errors).to be_empty
end
it 'does block nil urls' do
badge.link_url = nil
subject
expect(badge.errors).to be_present
end
it 'does block blank urls' do
badge.link_url = '\n\r \n'
subject
expect(badge.errors).to be_present
end
it 'strips urls' do
@ -67,6 +94,40 @@ describe UrlValidator do
end
end
context 'when message is set' do
let(:message) { 'is blocked: test message' }
let(:validator) { described_class.new(attributes: [:link_url], allow_nil: false, message: message) }
it 'does block nil url with provided error message' do
expect(validator.validate_each(badge, :link_url, nil)).to be_falsey
expect(badge.errors.first[1]).to eq message
end
end
context 'when allow_nil is set to true' do
let(:validator) { described_class.new(attributes: [:link_url], allow_nil: true) }
it 'does not block nil urls' do
badge.link_url = nil
subject
expect(badge.errors).to be_empty
end
end
context 'when allow_blank is set to true' do
let(:validator) { described_class.new(attributes: [:link_url], allow_blank: true) }
it 'does not block blank urls' do
badge.link_url = "\n\r \n"
subject
expect(badge.errors).to be_empty
end
end
context 'when allow_localhost is set to false' do
let(:validator) { described_class.new(attributes: [:link_url], allow_localhost: false) }
@ -75,7 +136,21 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
context 'when allow_setting_local_requests is set to true' do
it 'does not block urls pointing to localhost' do
expect(described_class)
.to receive(:allow_setting_local_requests?)
.and_return(true)
badge.link_url = 'https://127.0.0.1'
subject
expect(badge.errors).to be_empty
end
end
end
@ -87,7 +162,21 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
context 'when allow_setting_local_requests is set to true' do
it 'does not block urls pointing to local network' do
expect(described_class)
.to receive(:allow_setting_local_requests?)
.and_return(true)
badge.link_url = 'https://192.168.1.1'
subject
expect(badge.errors).to be_empty
end
end
end
@ -100,7 +189,7 @@ describe UrlValidator do
it 'does not block any port' do
subject
expect(badge.errors.empty?).to be true
expect(badge.errors).to be_empty
end
end
@ -110,7 +199,7 @@ describe UrlValidator do
it 'blocks urls with a different port' do
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
end
end
@ -127,7 +216,7 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
end
@ -139,7 +228,7 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be true
expect(badge.errors).to be_empty
end
end
end
@ -156,7 +245,7 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
end
@ -168,7 +257,7 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be true
expect(badge.errors).to be_empty
end
end
end
@ -191,7 +280,7 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
it 'prevents unsafe internal urls' do
@ -199,7 +288,7 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
it 'allows safe urls' do
@ -207,7 +296,7 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be true
expect(badge.errors).to be_empty
end
end
@ -219,7 +308,7 @@ describe UrlValidator do
subject
expect(badge.errors.empty?).to be true
expect(badge.errors).to be_empty
end
end
end

View File

@ -1,20 +1,20 @@
require 'spec_helper'
describe PublicUrlValidator do
include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS
include_examples 'url validator examples', AddressableUrlValidator::DEFAULT_OPTIONS[:schemes]
context 'by default' do
let(:validator) { described_class.new(attributes: [:link_url]) }
let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
subject { validator.validate_each(badge, :link_url, badge.link_url) }
subject { validator.validate(badge) }
it 'blocks urls pointing to localhost' do
badge.link_url = 'https://127.0.0.1'
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
it 'blocks urls pointing to the local network' do
@ -22,7 +22,7 @@ describe PublicUrlValidator do
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
end
end