Commit 1418afc2 authored by Francisco Javier López's avatar Francisco Javier López 🌴 Committed by Douwe Maan

Avoid checking the user format in every url validation

parent 180dc237
......@@ -292,6 +292,7 @@ class Project < ActiveRecord::Base
validates :name, uniqueness: { scope: :namespace_id }
validates :import_url, url: { protocols: %w(http https ssh git),
allow_localhost: false,
enforce_user: true,
ports: VALID_IMPORT_PORTS }, if: [:external_import?, :import_url_changed?]
validates :star_count, numericality: { greater_than_or_equal_to: 0 }
validate :check_limit, on: :create
......
......@@ -16,7 +16,7 @@ class RemoteMirror < ActiveRecord::Base
belongs_to :project, inverse_of: :remote_mirrors
validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true }
validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true }
before_save :set_new_remote_name, if: :mirror_url_changed?
......
......@@ -18,6 +18,13 @@
# 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
#
# Example:
# class User < ActiveRecord::Base
# validates :personal_url, url: { allow_localhost: false, allow_local_network: false}
......@@ -35,7 +42,7 @@ class UrlValidator < ActiveModel::EachValidator
if value.present?
value.strip!
else
record.errors.add(attribute, "must be a valid URL")
record.errors.add(attribute, 'must be a valid URL')
end
Gitlab::UrlBlocker.validate!(value, blocker_args)
......@@ -51,7 +58,8 @@ class UrlValidator < ActiveModel::EachValidator
protocols: DEFAULT_PROTOCOLS,
ports: [],
allow_localhost: true,
allow_local_network: true
allow_local_network: true,
enforce_user: false
}
end
......@@ -64,7 +72,7 @@ class UrlValidator < ActiveModel::EachValidator
end
def blocker_args
current_options.slice(:allow_localhost, :allow_local_network, :protocols, :ports).tap do |args|
current_options.slice(*default_options.keys).tap do |args|
if allow_setting_local_requests?
args[:allow_localhost] = args[:allow_local_network] = true
end
......
---
title: Avoid checking the user format in every url validation
merge_request: 19575
author:
type: changed
......@@ -5,7 +5,7 @@ module Gitlab
BlockedUrlError = Class.new(StandardError)
class << self
def validate!(url, allow_localhost: false, allow_local_network: true, ports: [], protocols: [])
def validate!(url, allow_localhost: false, allow_local_network: true, enforce_user: false, ports: [], protocols: [])
return true if url.nil?
begin
......@@ -20,7 +20,7 @@ module Gitlab
port = uri.port || uri.default_port
validate_protocol!(uri.scheme, protocols)
validate_port!(port, ports) if ports.any?
validate_user!(uri.user)
validate_user!(uri.user) if enforce_user
validate_hostname!(uri.hostname)
begin
......
......@@ -58,20 +58,6 @@ describe Gitlab::UrlBlocker do
end
end
it 'returns true for a non-alphanumeric username' do
stub_resolv
aggregate_failures do
expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami@example.com/a')
# The leading character here is a Unicode "soft hyphen"
expect(described_class).to be_blocked_url('ssh://­oProxyCommand=whoami@example.com/a')
# Unicode alphanumerics are allowed
expect(described_class).not_to be_blocked_url('ssh://ğitlab@example.com/a')
end
end
it 'returns true for invalid URL' do
expect(described_class.blocked_url?('http://:8080')).to be true
end
......@@ -120,6 +106,38 @@ describe Gitlab::UrlBlocker do
allow(Addrinfo).to receive(:getaddrinfo).and_call_original
end
end
context 'when enforce_user is' do
before do
stub_resolv
end
context 'false (default)' do
it 'does not block urls with a non-alphanumeric username' do
expect(described_class).not_to be_blocked_url('ssh://-oProxyCommand=whoami@example.com/a')
# The leading character here is a Unicode "soft hyphen"
expect(described_class).not_to be_blocked_url('ssh://­oProxyCommand=whoami@example.com/a')
# Unicode alphanumerics are allowed
expect(described_class).not_to be_blocked_url('ssh://ğitlab@example.com/a')
end
end
context 'true' do
it 'blocks urls with a non-alphanumeric username' do
aggregate_failures do
expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami@example.com/a', enforce_user: true)
# The leading character here is a Unicode "soft hyphen"
expect(described_class).to be_blocked_url('ssh://­oProxyCommand=whoami@example.com/a', enforce_user: true)
# Unicode alphanumerics are allowed
expect(described_class).not_to be_blocked_url('ssh://ğitlab@example.com/a', enforce_user: true)
end
end
end
end
end
# Resolv does not support resolving UTF-8 domain names
......
......@@ -238,20 +238,27 @@ describe Project do
expect(project2.import_data).to be_nil
end
it "does not allow blocked import_url localhost" do
it "does not allow import_url pointing to localhost" do
project2 = build(:project, import_url: 'http://localhost:9000/t.git')
expect(project2).to be_invalid
expect(project2.errors[:import_url].first).to include('Requests to localhost are not allowed')
end
it "does not allow blocked import_url port" do
it "does not allow import_url with invalid ports" do
project2 = build(:project, import_url: 'http://github.com:25/t.git')
expect(project2).to be_invalid
expect(project2.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443')
end
it "does not allow import_url with invalid user" do
project2 = build(:project, import_url: 'http://$user:password@github.com/t.git')
expect(project2).to be_invalid
expect(project2.errors[:import_url].first).to include('Username needs to start with an alphanumeric character')
end
describe 'project pending deletion' do
let!(:project_pending_deletion) do
create(:project,
......
......@@ -15,6 +15,13 @@ describe RemoteMirror do
expect(remote_mirror).not_to be_valid
end
it 'does not allow url with an invalid user' do
remote_mirror = build(:remote_mirror, url: 'http://$user:password@invalid.invalid')
expect(remote_mirror).to be_invalid
expect(remote_mirror.errors[:url].first).to include('Username needs to start with an alphanumeric character')
end
end
end
......
......@@ -50,13 +50,56 @@ describe UrlValidator do
end
end
context 'when ports is set' do
let(:validator) { described_class.new(attributes: [:link_url], ports: [443]) }
context 'when ports is' do
let(:validator) { described_class.new(attributes: [:link_url], ports: ports) }
it 'blocks urls with a different port' do
subject
context 'empty' do
let(:ports) { [] }
expect(badge.errors.empty?).to be false
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
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