Make error messages even more descriptive

3 files changed, 51 insertions, 37 deletions

diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
index af6c389b4a2..f2c97791b9d 100644
--- a/lib/gitlab/url_blocker.rb
+++ b/lib/gitlab/url_blocker.rb
@@ -10,28 +10,27 @@ module Gitlab
 
         begin
           uri = Addressable::URI.parse(url)
-          # Allow imports from the GitLab instance itself but only from the configured ports
-          return true if internal?(uri)
-
-          raise BlockedUrlError, "Port is blocked" if blocked_port?(uri.port, valid_ports)
-          raise BlockedUrlError, "User is blocked" if blocked_user_or_hostname?(uri.user)
-          raise BlockedUrlError, "Hostname is blocked" if blocked_user_or_hostname?(uri.hostname)
+        rescue Addressable::URI::InvalidURIError
+          raise BlockedUrlError, "URI is invalid"
+        end
 
-          addrs_info = Addrinfo.getaddrinfo(uri.hostname, 80, nil, :STREAM)
+        # Allow imports from the GitLab instance itself but only from the configured ports
+        return true if internal?(uri)
 
-          if !allow_localhost && localhost?(addrs_info)
-            raise BlockedUrlError, "Requests to localhost are blocked"
-          end
+        port = uri.port || uri.default_port
+        validate_port!(port, valid_ports) if valid_ports.any?
+        validate_user!(uri.user)
+        validate_hostname!(uri.hostname)
 
-          if !allow_private_networks && private_network?(addrs_info)
-            raise BlockedUrlError, "Requests to the private local network are blocked"
-          end
-        rescue Addressable::URI::InvalidURIError
-          raise BlockedUrlError, "URI is invalid"
+        begin
+          addrs_info = Addrinfo.getaddrinfo(uri.hostname, port, nil, :STREAM)
         rescue SocketError
-          return
+          return true
         end
 
+        validate_localhost!(addrs_info) unless allow_localhost
+        validate_local_network!(addrs_info) unless allow_private_networks
+
         true
       end
 
@@ -45,16 +44,42 @@ module Gitlab
 
       private
 
-      def blocked_port?(port, valid_ports)
-        return false if port.blank? || valid_ports.blank?
+      def validate_port!(port, valid_ports)
+        return if port.blank?
+        # Only ports under 1024 are restricted
+        return if port >= 1024
+        return if valid_ports.include?(port)
+
+        raise BlockedUrlError, "Only allowed ports are #{valid_ports.join(', ')}, and any over 1024"
+      end
+
+      def validate_user!(value)
+        return if value.blank?
+        return if value =~ /\A\p{Alnum}/
 
-        port < 1024 && !valid_ports.include?(port)
+        raise BlockedUrlError, "Username needs to start with an alphanumeric character"
       end
 
-      def blocked_user_or_hostname?(value)
-        return false if value.blank?
+      def validate_hostname!(value)
+        return if value.blank?
+        return if value =~ /\A\p{Alnum}/
 
-        value !~ /\A\p{Alnum}/
+        raise BlockedUrlError, "Hostname needs to start with an alphanumeric character"
+      end
+
+      def validate_localhost!(addrs_info)
+        local_ips = ["127.0.0.1", "::1", "0.0.0.0"]
+        local_ips.concat(Socket.ip_address_list.map(&:ip_address))
+
+        return if (local_ips & addrs_info.map(&:ip_address)).empty?
+
+        raise BlockedUrlError, "Requests to localhost are not allowed"
+      end
+
+      def validate_local_network!(addrs_info)
+        return unless addrs_info.any? { |addr| addr.ipv4_private? || addr.ipv6_sitelocal? }
+
+        raise BlockedUrlError, "Requests to the local network are not allowed"
       end
 
       def internal?(uri)
@@ -71,17 +96,6 @@ module Gitlab
           (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port)
       end
 
-      def localhost?(addrs_info)
-        blocked_ips = ["127.0.0.1", "::1", "0.0.0.0"]
-        blocked_ips.concat(Socket.ip_address_list.map(&:ip_address))
-
-        (blocked_ips & addrs_info.map(&:ip_address)).any?
-      end
-
-      def private_network?(addrs_info)
-        addrs_info.any? { |addr| addr.ipv4_private? || addr.ipv6_sitelocal? }
-      end
-
       def config
         Gitlab.config
       end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 96adf64bcec..fef868ac0f2 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -224,14 +224,14 @@ describe Project do
       project2 = build(:project, import_url: 'http://localhost:9000/t.git')
 
       expect(project2).to be_invalid
-      expect(project2.errors[:import_url]).to include('imports are not allowed from that URL')
+      expect(project2.errors[:import_url].first).to include('Requests to localhost are not allowed')
     end
 
     it "does not allow blocked import_url port" do
       project2 = build(:project, import_url: 'http://github.com:25/t.git')
 
       expect(project2).to be_invalid
-      expect(project2.errors[:import_url]).to include('imports are not allowed from that URL')
+      expect(project2.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443')
     end
 
     describe 'project pending deletion' do
diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb
index bf7facaec99..30c89ebd821 100644
--- a/spec/services/projects/import_service_spec.rb
+++ b/spec/services/projects/import_service_spec.rb
@@ -156,7 +156,7 @@ describe Projects::ImportService do
         result = described_class.new(project, user).execute
 
         expect(result[:status]).to eq :error
-        expect(result[:message]).to end_with 'Blocked import URL.'
+        expect(result[:message]).to include('Requests to localhost are not allowed')
       end
 
       it 'fails with port 25' do
@@ -165,7 +165,7 @@ describe Projects::ImportService do
         result = described_class.new(project, user).execute
 
         expect(result[:status]).to eq :error
-        expect(result[:message]).to end_with 'Blocked import URL.'
+        expect(result[:message]).to include('Only allowed ports are 22, 80, 443')
       end
     end