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

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