Commit 77da24aa authored by Thong Kuah's avatar Thong Kuah
Browse files

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.
parent d6ac3b3ff966
......@@ -48,8 +48,8 @@
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,
......@@ -52,9 +52,9 @@
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,
......@@ -57,8 +57,8 @@
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 @@
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,
......
......@@ -22,7 +22,7 @@
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)
......
......@@ -13,7 +13,7 @@
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)
......
......@@ -35,7 +35,7 @@
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
......
......@@ -22,7 +22,7 @@
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
......
......@@ -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
......
......@@ -327,7 +327,7 @@
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 }
......
......@@ -6,7 +6,7 @@
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) }
......
......@@ -17,7 +17,7 @@
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?
......
# 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
......@@ -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,8 +12,8 @@
# 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
#
......@@ -16,7 +16,10 @@
#
# 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
......@@ -22,7 +25,7 @@
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
# 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
---
title: "Align UrlValidator to validate_url gem implementation"
merge_request: 27194
author: Horatiu Eugen Vlad
type: fixed
......@@ -8,7 +8,7 @@
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)
......
......@@ -8,7 +8,7 @@
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 @@
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 @@
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
......
......@@ -79,7 +79,7 @@
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
......
......@@ -23,6 +23,6 @@
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
......@@ -28,5 +28,5 @@
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
......
......@@ -306,7 +306,22 @@
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
......
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') }
......@@ -2,5 +2,5 @@
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) }
......@@ -6,5 +6,5 @@
describe '#validates_each' do
describe '#validate' do
context 'with no options' do
let(:options) { {} }
......@@ -8,8 +8,8 @@
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,7 +17,7 @@
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
end
......@@ -21,6 +21,6 @@
end
end
context 'with protocols' do
let(:options) { { protocols: %w[http] } }
context 'with schemes' do
let(:options) { { schemes: %w(http) } }
......@@ -26,4 +26,4 @@
it 'allows urls with the defined protocols' do
it 'allows urls with the defined schemes' do
subject
......@@ -28,5 +28,5 @@
subject
expect(badge.errors.empty?).to be true
expect(badge.errors).to be_empty
end
......@@ -31,7 +31,7 @@
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
......@@ -34,8 +34,8 @@
badge.link_url = 'https://www.example.com'
subject
expect(badge.errors.empty?).to be false
expect(badge.errors).to be_present
end
end
end
......
# frozen_string_literal: true
require 'spec_helper'
describe AddressableUrlValidator do
let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
subject { validator.validate(badge) }
include_examples 'url validator examples', described_class::DEFAULT_OPTIONS[:schemes]
describe 'validations' do
include_context 'invalid urls'
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_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_falsey
expect(badge.errors.first[1]).to eq validator.options.fetch(:message)
end
it 'does not allow urls with CR or LF characters' do
aggregate_failures do
urls_with_CRLF.each do |url|
expect(validator.validate_each(badge, :link_url, url)[0]).to eq 'is blocked: URI is invalid'
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
let(:validator) { described_class.new(attributes: [:link_url]) }