summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@selenight.nl>2018-04-02 17:13:23 +0200
committerDouwe Maan <douwe@selenight.nl>2018-04-02 17:20:18 +0200
commitb95918dda8c6cd5328d028492d36c3ee07e35943 (patch)
tree6756d41e609eb46a29b037742a4c34bf3d943504
parent7143bb88522db4784be6b593083061a104d4b630 (diff)
downloadgitlab-ce-b95918dda8c6cd5328d028492d36c3ee07e35943.tar.gz
Make error messages even more descriptive
-rw-r--r--lib/gitlab/url_blocker.rb80
-rw-r--r--spec/models/project_spec.rb4
-rw-r--r--spec/services/projects/import_service_spec.rb4
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