Avoid checking the user format in every url validation

4 files changed, 96 insertions, 21 deletions

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