summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/project.rb1
-rw-r--r--app/models/remote_mirror.rb2
-rw-r--r--app/validators/url_validator.rb14
-rw-r--r--changelogs/unreleased/fj-relax-url-validator-rules-for-user.yml5
-rw-r--r--lib/gitlab/url_blocker.rb4
-rw-r--r--spec/lib/gitlab/url_blocker_spec.rb46
-rw-r--r--spec/models/project_spec.rb11
-rw-r--r--spec/models/remote_mirror_spec.rb7
-rw-r--r--spec/validators/url_validator_spec.rb53
9 files changed, 116 insertions, 27 deletions
diff --git a/app/models/project.rb b/app/models/project.rb
index 60cd13b371f..9ca733ecd98 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -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
diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb
index 5cd222e18a4..c4b5dd2dc96 100644
--- a/app/models/remote_mirror.rb
+++ b/app/models/remote_mirror.rb
@@ -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?
diff --git a/app/validators/url_validator.rb b/app/validators/url_validator.rb
index 8648c4c75e3..6854fec582e 100644
--- a/app/validators/url_validator.rb
+++ b/app/validators/url_validator.rb
@@ -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
diff --git a/changelogs/unreleased/fj-relax-url-validator-rules-for-user.yml b/changelogs/unreleased/fj-relax-url-validator-rules-for-user.yml
new file mode 100644
index 00000000000..4c72e4c5c1c
--- /dev/null
+++ b/changelogs/unreleased/fj-relax-url-validator-rules-for-user.yml
@@ -0,0 +1,5 @@
+---
+title: Avoid checking the user format in every url validation
+merge_request: 19575
+author:
+type: changed
diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
index 20be193ea0c..38be75b7482 100644
--- a/lib/gitlab/url_blocker.rb
+++ b/lib/gitlab/url_blocker.rb
@@ -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
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
index 81dbbb962dd..6f5f9938eca 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -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
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 1a6ad3edd78..b9a9c4ebf42 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -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,
diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb
index 1d94abe4195..4c086eeadfc 100644
--- a/spec/models/remote_mirror_spec.rb
+++ b/spec/models/remote_mirror_spec.rb
@@ -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
diff --git a/spec/validators/url_validator_spec.rb b/spec/validators/url_validator_spec.rb
index 2d719263fc8..93fe013d11c 100644
--- a/spec/validators/url_validator_spec.rb
+++ b/spec/validators/url_validator_spec.rb
@@ -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
View Lorry Controller Status