From d119d3d1b25aac661e6251addf87b280bd37f0c5 Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Thu, 11 Apr 2019 06:29:07 +0000 Subject: 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. --- app/models/application_setting.rb | 8 +- app/models/badge.rb | 2 +- app/models/ci/build_runner_session.rb | 2 +- app/models/environment.rb | 2 +- .../project_error_tracking_setting.rb | 2 +- app/models/generic_commit_status.rb | 2 +- app/models/project.rb | 2 +- app/models/releases/link.rb | 2 +- app/models/remote_mirror.rb | 2 +- app/validators/addressable_url_validator.rb | 112 ++++++++ app/validators/public_url_validator.rb | 19 +- app/validators/url_validator.rb | 104 ------- ...alidator-to-validate_url-gem-implementation.yml | 5 + .../after_export_strategies/web_upload_strategy.rb | 2 +- lib/gitlab/url_blocker.rb | 10 +- .../projects/mirrors_controller_spec.rb | 2 +- spec/lib/gitlab/url_blocker_spec.rb | 6 +- spec/requests/api/commit_statuses_spec.rb | 17 +- .../shared_examples/url_validator_examples.rb | 24 +- spec/validators/addressable_url_validator_spec.rb | 315 +++++++++++++++++++++ spec/validators/public_url_validator_spec.rb | 8 +- spec/validators/url_validator_spec.rb | 226 --------------- 22 files changed, 497 insertions(+), 377 deletions(-) create mode 100644 app/validators/addressable_url_validator.rb delete mode 100644 app/validators/url_validator.rb create mode 100644 changelogs/unreleased/24985-align-urlvalidator-to-validate_url-gem-implementation.yml create mode 100644 spec/validators/addressable_url_validator_spec.rb delete mode 100644 spec/validators/url_validator_spec.rb diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index d28a12413bf..21a97c8d773 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -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, diff --git a/app/models/badge.rb b/app/models/badge.rb index a244ed473de..50299cd6652 100644 --- a/app/models/badge.rb +++ b/app/models/badge.rb @@ -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) diff --git a/app/models/ci/build_runner_session.rb b/app/models/ci/build_runner_session.rb index 80dbb150085..997bf298025 100644 --- a/app/models/ci/build_runner_session.rb +++ b/app/models/ci/build_runner_session.rb @@ -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) diff --git a/app/models/environment.rb b/app/models/environment.rb index fa29a83e517..69224635e34 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -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 diff --git a/app/models/error_tracking/project_error_tracking_setting.rb b/app/models/error_tracking/project_error_tracking_setting.rb index 70954bf8b05..72270ee8b4f 100644 --- a/app/models/error_tracking/project_error_tracking_setting.rb +++ b/app/models/error_tracking/project_error_tracking_setting.rb @@ -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 diff --git a/app/models/generic_commit_status.rb b/app/models/generic_commit_status.rb index 3028bf21301..8a768b3a2c0 100644 --- a/app/models/generic_commit_status.rb +++ b/app/models/generic_commit_status.rb @@ -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 diff --git a/app/models/project.rb b/app/models/project.rb index 66fc83113ea..89ad90a964e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -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 } diff --git a/app/models/releases/link.rb b/app/models/releases/link.rb index 36ec33d3e3e..58c2b98e524 100644 --- a/app/models/releases/link.rb +++ b/app/models/releases/link.rb @@ -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) } diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb index 5610cfe0f24..b2fd5394a03 100644 --- a/app/models/remote_mirror.rb +++ b/app/models/remote_mirror.rb @@ -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? diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb new file mode 100644 index 00000000000..273e15ef925 --- /dev/null +++ b/app/validators/addressable_url_validator.rb @@ -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: +# * message - A custom error message (default is: "must be a valid URL"). +# * schemes - Array of URI schemes. Default: +['http', 'https']+ +# * allow_localhost - Allow urls pointing to +localhost+. Default: +true+ +# * allow_local_network - Allow urls pointing to private network addresses. Default: +true+ +# * allow_blank - Allow urls to be +blank+. Default: +false+ +# * allow_nil - Allow urls to be +nil+. Default: +false+ +# * 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, 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 diff --git a/app/validators/public_url_validator.rb b/app/validators/public_url_validator.rb index 3ff880deedd..91847c5d866 100644 --- a/app/validators/public_url_validator.rb +++ b/app/validators/public_url_validator.rb @@ -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 diff --git a/app/validators/url_validator.rb b/app/validators/url_validator.rb deleted file mode 100644 index 3fd015c3cf5..00000000000 --- a/app/validators/url_validator.rb +++ /dev/null @@ -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 diff --git a/changelogs/unreleased/24985-align-urlvalidator-to-validate_url-gem-implementation.yml b/changelogs/unreleased/24985-align-urlvalidator-to-validate_url-gem-implementation.yml new file mode 100644 index 00000000000..1143e4effea --- /dev/null +++ b/changelogs/unreleased/24985-align-urlvalidator-to-validate_url-gem-implementation.yml @@ -0,0 +1,5 @@ +--- +title: "Align UrlValidator to validate_url gem implementation" +merge_request: 27194 +author: Horatiu Eugen Vlad +type: fixed diff --git a/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb b/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb index b30900f7c61..fcf6a25ab00 100644 --- a/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb +++ b/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb @@ -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) diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 9b7b0db9525..641ba70ef83 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -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 diff --git a/spec/controllers/projects/mirrors_controller_spec.rb b/spec/controllers/projects/mirrors_controller_spec.rb index f2b73956e8d..3ababe18055 100644 --- a/spec/controllers/projects/mirrors_controller_spec.rb +++ b/spec/controllers/projects/mirrors_controller_spec.rb @@ -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 diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 62970bd8cb6..445a56ab0d8 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -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 diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index 9388343c392..b5e45f99109 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -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 diff --git a/spec/support/shared_examples/url_validator_examples.rb b/spec/support/shared_examples/url_validator_examples.rb index 1f7e2f7ff79..25277ccd9aa 100644 --- a/spec/support/shared_examples/url_validator_examples.rb +++ b/spec/support/shared_examples/url_validator_examples.rb @@ -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 diff --git a/spec/validators/addressable_url_validator_spec.rb b/spec/validators/addressable_url_validator_spec.rb new file mode 100644 index 00000000000..387e84b2d04 --- /dev/null +++ b/spec/validators/addressable_url_validator_spec.rb @@ -0,0 +1,315 @@ +# 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]) } + + it 'does not block urls pointing to localhost' do + badge.link_url = 'https://127.0.0.1' + + subject + + expect(badge.errors).to be_empty + end + + it 'does not block urls pointing to the local network' do + badge.link_url = 'https://192.168.1.1' + + subject + + 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 + badge.link_url = "\n\r\n\nhttps://127.0.0.1\r\n\r\n\n\n\n" + + # It's unusual for a validator to modify its arguments. Some extensions, + # such as attr_encrypted, freeze the string to signal that modifications + # will not be persisted, so freeze this string to ensure the scheme is + # compatible with them. + badge.link_url.freeze + + subject + + expect(badge.errors).to be_empty + expect(badge.link_url).to eq('https://127.0.0.1') + 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) } + + it 'blocks urls pointing to localhost' do + badge.link_url = 'https://127.0.0.1' + + subject + + 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 + + context 'when allow_local_network is set to false' do + let(:validator) { described_class.new(attributes: [:link_url], allow_local_network: false) } + + it 'blocks urls pointing to the local network' do + badge.link_url = 'https://192.168.1.1' + + subject + + 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 + + context 'when ports is' do + let(:validator) { described_class.new(attributes: [:link_url], ports: ports) } + + context 'empty' do + let(:ports) { [] } + + it 'does not block any port' do + subject + + expect(badge.errors).to be_empty + end + end + + context 'set' do + let(:ports) { [443] } + + it 'blocks urls with a different port' do + subject + + expect(badge.errors).to be_present + end + end + end + + context 'when enforce_user is' do + let(:url) { 'http://$user@example.com'} + let(:validator) { described_class.new(attributes: [:link_url], enforce_user: enforce_user) } + + context 'true' do + let(:enforce_user) { true } + + it 'checks user format' do + badge.link_url = url + + subject + + expect(badge.errors).to be_present + end + end + + context 'false (default)' do + let(:enforce_user) { false } + + it 'does not check user format' do + badge.link_url = url + + subject + + expect(badge.errors).to be_empty + end + end + end + + context 'when ascii_only is' do + let(:url) { 'https://𝕘itⅼαƄ.com/foo/foo.bar'} + let(:validator) { described_class.new(attributes: [:link_url], ascii_only: ascii_only) } + + context 'true' do + let(:ascii_only) { true } + + it 'prevents unicode characters' do + badge.link_url = url + + subject + + expect(badge.errors).to be_present + end + end + + context 'false (default)' do + let(:ascii_only) { false } + + it 'does not prevent unicode characters' do + badge.link_url = url + + subject + + expect(badge.errors).to be_empty + end + end + end + + context 'when enforce_sanitization is' do + let(:validator) { described_class.new(attributes: [:link_url], enforce_sanitization: enforce_sanitization) } + let(:unsafe_url) { "https://replaceme.com/'>" } + let(:safe_url) { 'https://replaceme.com/path/to/somewhere' } + + let(:unsafe_internal_url) do + Gitlab.config.gitlab.protocol + '://' + Gitlab.config.gitlab.host + + "/'>" + end + + context 'true' do + let(:enforce_sanitization) { true } + + it 'prevents unsafe urls' do + badge.link_url = unsafe_url + + subject + + expect(badge.errors).to be_present + end + + it 'prevents unsafe internal urls' do + badge.link_url = unsafe_internal_url + + subject + + expect(badge.errors).to be_present + end + + it 'allows safe urls' do + badge.link_url = safe_url + + subject + + expect(badge.errors).to be_empty + end + end + + context 'false' do + let(:enforce_sanitization) { false } + + it 'allows unsafe urls' do + badge.link_url = unsafe_url + + subject + + expect(badge.errors).to be_empty + end + end + end +end diff --git a/spec/validators/public_url_validator_spec.rb b/spec/validators/public_url_validator_spec.rb index 710dd3dc38e..f6364fb1dd5 100644 --- a/spec/validators/public_url_validator_spec.rb +++ b/spec/validators/public_url_validator_spec.rb @@ -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 diff --git a/spec/validators/url_validator_spec.rb b/spec/validators/url_validator_spec.rb deleted file mode 100644 index 1bb42382e8a..00000000000 --- a/spec/validators/url_validator_spec.rb +++ /dev/null @@ -1,226 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe UrlValidator do - let!(:badge) { build(:badge, link_url: 'http://www.example.com') } - subject { validator.validate_each(badge, :link_url, badge.link_url) } - - include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS - - 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_nil - expect(badge.errors.first[1]).to eq 'must be a valid URL' - 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' - 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 - end - - context 'by default' do - let(:validator) { described_class.new(attributes: [:link_url]) } - - it 'does not block urls pointing to localhost' do - badge.link_url = 'https://127.0.0.1' - - subject - - expect(badge.errors.empty?).to be true - end - - it 'does not block urls pointing to the local network' do - badge.link_url = 'https://192.168.1.1' - - subject - - expect(badge.errors.empty?).to be true - end - - it 'strips urls' do - badge.link_url = "\n\r\n\nhttps://127.0.0.1\r\n\r\n\n\n\n" - - # It's unusual for a validator to modify its arguments. Some extensions, - # such as attr_encrypted, freeze the string to signal that modifications - # will not be persisted, so freeze this string to ensure the scheme is - # compatible with them. - badge.link_url.freeze - - subject - - expect(badge.errors).to be_empty - expect(badge.link_url).to eq('https://127.0.0.1') - end - end - - context 'when allow_localhost is set to false' do - let(:validator) { described_class.new(attributes: [:link_url], allow_localhost: false) } - - it 'blocks urls pointing to localhost' do - badge.link_url = 'https://127.0.0.1' - - subject - - expect(badge.errors.empty?).to be false - end - end - - context 'when allow_local_network is set to false' do - let(:validator) { described_class.new(attributes: [:link_url], allow_local_network: false) } - - it 'blocks urls pointing to the local network' do - badge.link_url = 'https://192.168.1.1' - - subject - - expect(badge.errors.empty?).to be false - end - end - - context 'when ports is' do - let(:validator) { described_class.new(attributes: [:link_url], ports: ports) } - - context 'empty' do - let(:ports) { [] } - - it 'does not block any port' do - subject - - expect(badge.errors.empty?).to be true - end - end - - context 'set' do - let(:ports) { [443] } - - it 'blocks urls with a different port' do - subject - - expect(badge.errors.empty?).to be false - end - end - end - - context 'when enforce_user is' do - let(:url) { 'http://$user@example.com'} - let(:validator) { described_class.new(attributes: [:link_url], enforce_user: enforce_user) } - - context 'true' do - let(:enforce_user) { true } - - it 'checks user format' do - badge.link_url = url - - subject - - expect(badge.errors.empty?).to be false - end - end - - context 'false (default)' do - let(:enforce_user) { false } - - it 'does not check user format' do - badge.link_url = url - - subject - - expect(badge.errors.empty?).to be true - end - end - end - - context 'when ascii_only is' do - let(:url) { 'https://𝕘itⅼαƄ.com/foo/foo.bar'} - let(:validator) { described_class.new(attributes: [:link_url], ascii_only: ascii_only) } - - context 'true' do - let(:ascii_only) { true } - - it 'prevents unicode characters' do - badge.link_url = url - - subject - - expect(badge.errors.empty?).to be false - end - end - - context 'false (default)' do - let(:ascii_only) { false } - - it 'does not prevent unicode characters' do - badge.link_url = url - - subject - - expect(badge.errors.empty?).to be true - end - end - end - - context 'when enforce_sanitization is' do - let(:validator) { described_class.new(attributes: [:link_url], enforce_sanitization: enforce_sanitization) } - let(:unsafe_url) { "https://replaceme.com/'>" } - let(:safe_url) { 'https://replaceme.com/path/to/somewhere' } - - let(:unsafe_internal_url) do - Gitlab.config.gitlab.protocol + '://' + Gitlab.config.gitlab.host + - "/'>" - end - - context 'true' do - let(:enforce_sanitization) { true } - - it 'prevents unsafe urls' do - badge.link_url = unsafe_url - - subject - - expect(badge.errors.empty?).to be false - end - - it 'prevents unsafe internal urls' do - badge.link_url = unsafe_internal_url - - subject - - expect(badge.errors.empty?).to be false - end - - it 'allows safe urls' do - badge.link_url = safe_url - - subject - - expect(badge.errors.empty?).to be true - end - end - - context 'false' do - let(:enforce_sanitization) { false } - - it 'allows unsafe urls' do - badge.link_url = unsafe_url - - subject - - expect(badge.errors.empty?).to be true - end - end - end -end -- cgit v1.2.1