Commit 6643b1c9 authored by James Lopez's avatar James Lopez

Merge branch 'fix/url_validator_' into 'master'

Align UrlValidator to validate_url gem implementation

See merge request gitlab-org/gitlab-ce!27194
parents 79bf4bda d119d3d1
......@@ -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,
......
......@@ -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)
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -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 @@ 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 }
......
......@@ -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) }
......
......@@ -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?
......
# frozen_string_literal: true
# UrlValidator
# AddressableUrlValidator
#
# Custom validator for URLs.
# 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) protocols will be considered valid.
# Provide a `:protocols` option to configure accepted protocols.
# 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, url: true
# validates :personal_url, addressable_url: true
#
# validates :ftp_url, url: { protocols: %w(ftp) }
# validates :ftp_url, addressable_url: { schemes: %w(ftp) }
#
# validates :git_url, url: { protocols: %w(http https ssh git) }
# 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.
#
# 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
# 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, url: { allow_localhost: false, allow_local_network: false}
# validates :personal_url, addressable_url: { allow_localhost: false, allow_local_network: false}
#
# validates :web_url, url: { ports: [80, 443] }
# validates :web_url, addressable_url: { ports: [80, 443] }
# end
class UrlValidator < ActiveModel::EachValidator
DEFAULT_PROTOCOLS = %w(http https).freeze
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, 'must be a valid URL')
record.errors.add(attribute, options.fetch(:message))
return
end
......@@ -63,36 +86,21 @@ class UrlValidator < ActiveModel::EachValidator
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|
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?
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 allow_setting_local_requests?
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
......
......@@ -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
---
title: "Align UrlValidator to validate_url gem implementation"
merge_request: 27194
author: Horatiu Eugen Vlad
type: fixed
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
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
......
......@@ -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
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment