Commit e6a5d3cc authored by Douwe Maan's avatar Douwe Maan

Merge branch 'fj-relax-url-validator-rules-for-user' into 'master'

Avoid checking the user format in every url validation

Closes #47526

See merge request gitlab-org/gitlab-ce!19575
parents 180dc237 1418afc2
...@@ -292,6 +292,7 @@ class Project < ActiveRecord::Base ...@@ -292,6 +292,7 @@ class Project < ActiveRecord::Base
validates :name, uniqueness: { scope: :namespace_id } validates :name, uniqueness: { scope: :namespace_id }
validates :import_url, url: { protocols: %w(http https ssh git), validates :import_url, url: { protocols: %w(http https ssh git),
allow_localhost: false, allow_localhost: false,
enforce_user: true,
ports: VALID_IMPORT_PORTS }, if: [:external_import?, :import_url_changed?] ports: VALID_IMPORT_PORTS }, if: [:external_import?, :import_url_changed?]
validates :star_count, numericality: { greater_than_or_equal_to: 0 } validates :star_count, numericality: { greater_than_or_equal_to: 0 }
validate :check_limit, on: :create validate :check_limit, on: :create
......
...@@ -16,7 +16,7 @@ class RemoteMirror < ActiveRecord::Base ...@@ -16,7 +16,7 @@ class RemoteMirror < ActiveRecord::Base
belongs_to :project, inverse_of: :remote_mirrors 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? before_save :set_new_remote_name, if: :mirror_url_changed?
......
...@@ -18,6 +18,13 @@ ...@@ -18,6 +18,13 @@
# This validator can also block urls pointing to localhost or the local network to # 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. # 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: # Example:
# class User < ActiveRecord::Base # class User < ActiveRecord::Base
# validates :personal_url, url: { allow_localhost: false, allow_local_network: false} # validates :personal_url, url: { allow_localhost: false, allow_local_network: false}
...@@ -35,7 +42,7 @@ class UrlValidator < ActiveModel::EachValidator ...@@ -35,7 +42,7 @@ class UrlValidator < ActiveModel::EachValidator
if value.present? if value.present?
value.strip! value.strip!
else else
record.errors.add(attribute, "must be a valid URL") record.errors.add(attribute, 'must be a valid URL')
end end
Gitlab::UrlBlocker.validate!(value, blocker_args) Gitlab::UrlBlocker.validate!(value, blocker_args)
...@@ -51,7 +58,8 @@ class UrlValidator < ActiveModel::EachValidator ...@@ -51,7 +58,8 @@ class UrlValidator < ActiveModel::EachValidator
protocols: DEFAULT_PROTOCOLS, protocols: DEFAULT_PROTOCOLS,
ports: [], ports: [],
allow_localhost: true, allow_localhost: true,
allow_local_network: true allow_local_network: true,
enforce_user: false
} }
end end
...@@ -64,7 +72,7 @@ class UrlValidator < ActiveModel::EachValidator ...@@ -64,7 +72,7 @@ class UrlValidator < ActiveModel::EachValidator
end end
def blocker_args 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? if allow_setting_local_requests?
args[:allow_localhost] = args[:allow_local_network] = true args[:allow_localhost] = args[:allow_local_network] = true
end end
......
---
title: Avoid checking the user format in every url validation
merge_request: 19575
author:
type: changed
...@@ -5,7 +5,7 @@ module Gitlab ...@@ -5,7 +5,7 @@ module Gitlab
BlockedUrlError = Class.new(StandardError) BlockedUrlError = Class.new(StandardError)
class << self 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? return true if url.nil?
begin begin
...@@ -20,7 +20,7 @@ module Gitlab ...@@ -20,7 +20,7 @@ module Gitlab
port = uri.port || uri.default_port port = uri.port || uri.default_port
validate_protocol!(uri.scheme, protocols) validate_protocol!(uri.scheme, protocols)
validate_port!(port, ports) if ports.any? validate_port!(port, ports) if ports.any?
validate_user!(uri.user) validate_user!(uri.user) if enforce_user
validate_hostname!(uri.hostname) validate_hostname!(uri.hostname)
begin begin
......
...@@ -58,20 +58,6 @@ describe Gitlab::UrlBlocker do ...@@ -58,20 +58,6 @@ describe Gitlab::UrlBlocker do
end end
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 it 'returns true for invalid URL' do
expect(described_class.blocked_url?('http://:8080')).to be true expect(described_class.blocked_url?('http://:8080')).to be true
end end
...@@ -120,6 +106,38 @@ describe Gitlab::UrlBlocker do ...@@ -120,6 +106,38 @@ describe Gitlab::UrlBlocker do
allow(Addrinfo).to receive(:getaddrinfo).and_call_original allow(Addrinfo).to receive(:getaddrinfo).and_call_original
end end
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 end
# Resolv does not support resolving UTF-8 domain names # Resolv does not support resolving UTF-8 domain names
......
...@@ -238,20 +238,27 @@ describe Project do ...@@ -238,20 +238,27 @@ describe Project do
expect(project2.import_data).to be_nil expect(project2.import_data).to be_nil
end 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') project2 = build(:project, import_url: 'http://localhost:9000/t.git')
expect(project2).to be_invalid expect(project2).to be_invalid
expect(project2.errors[:import_url].first).to include('Requests to localhost are not allowed') expect(project2.errors[:import_url].first).to include('Requests to localhost are not allowed')
end 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') project2 = build(:project, import_url: 'http://github.com:25/t.git')
expect(project2).to be_invalid expect(project2).to be_invalid
expect(project2.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443') expect(project2.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443')
end 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 describe 'project pending deletion' do
let!(:project_pending_deletion) do let!(:project_pending_deletion) do
create(:project, create(:project,
......
...@@ -15,6 +15,13 @@ describe RemoteMirror do ...@@ -15,6 +15,13 @@ describe RemoteMirror do
expect(remote_mirror).not_to be_valid expect(remote_mirror).not_to be_valid
end 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
end end
......
...@@ -50,8 +50,21 @@ describe UrlValidator do ...@@ -50,8 +50,21 @@ describe UrlValidator do
end end
end end
context 'when ports is set' do context 'when ports is' do
let(:validator) { described_class.new(attributes: [:link_url], ports: [443]) } 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 it 'blocks urls with a different port' do
subject subject
...@@ -59,4 +72,34 @@ describe UrlValidator do ...@@ -59,4 +72,34 @@ describe UrlValidator do
expect(badge.errors.empty?).to be false expect(badge.errors.empty?).to be false
end end
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 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