Add validation to webhook and service URLs to ensure they are not blocked because of SSRF

29 files changed, 122 insertions, 128 deletions

diff --git a/app/assets/javascripts/integrations/integration_settings_form.js b/app/assets/javascripts/integrations/integration_settings_form.js
index 741894b5e6c..cdb75752b4e 100644
--- a/app/assets/javascripts/integrations/integration_settings_form.js
+++ b/app/assets/javascripts/integrations/integration_settings_form.js
@@ -101,13 +101,19 @@ export default class IntegrationSettingsForm {
     return axios.put(this.testEndPoint, formData)
       .then(({ data }) => {
         if (data.error) {
-          flash(`${data.message} ${data.service_response}`, 'alert', document, {
-            title: 'Save anyway',
-            clickHandler: (e) => {
-              e.preventDefault();
-              this.$form.submit();
-            },
-          });
+          let flashActions;
+
+          if (data.test_failed) {
+            flashActions = {
+              title: 'Save anyway',
+              clickHandler: (e) => {
+                e.preventDefault();
+                this.$form.submit();
+              },
+            };
+          }
+
+          flash(`${data.message} ${data.service_response}`, 'alert', document, flashActions);
         } else {
           this.$form.submit();
         }
diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index a5ea9ff7ed7..690596b12db 100644
--- a/app/controllers/projects/services_controller.rb
+++ b/app/controllers/projects/services_controller.rb
@@ -41,13 +41,13 @@ class Projects::ServicesController < Projects::ApplicationController
       if outcome[:success]
         {}
       else
-        { error: true, message: 'Test failed.', service_response: outcome[:result].to_s }
+        { error: true, message: 'Test failed.', service_response: outcome[:result].to_s, test_failed: true }
       end
     else
-      { error: true, message: 'Validations failed.', service_response: @service.errors.full_messages.join(',') }
+      { error: true, message: 'Validations failed.', service_response: @service.errors.full_messages.join(','), test_failed: false }
     end
   rescue Gitlab::HTTP::BlockedUrlError => e
-    { error: true, message: 'Test failed.', service_response: e.message }
+    { error: true, message: 'Test failed.', service_response: e.message, test_failed: true }
   end
 
   def success_message
diff --git a/app/models/badge.rb b/app/models/badge.rb
index f7e10c2ebfc..265c5d872d4 100644
--- a/app/models/badge.rb
+++ b/app/models/badge.rb
@@ -18,7 +18,7 @@ class Badge < ActiveRecord::Base
 
   scope :order_created_at_asc, -> { reorder(created_at: :asc) }
 
-  validates :link_url, :image_url, url_placeholder: { protocols: %w(http https), placeholder_regex: PLACEHOLDERS_REGEX }
+  validates :link_url, :image_url, url: { protocols: %w(http https) }
   validates :type, presence: true
 
   def rendered_link_url(project = nil)
diff --git a/app/models/environment.rb b/app/models/environment.rb
index fddb269af4b..8d523dae324 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -32,7 +32,7 @@ class Environment < ActiveRecord::Base
   validates :external_url,
             length: { maximum: 255 },
             allow_nil: true,
-            addressable_url: true
+            url: true
 
   delegate :stop_action, :manual_actions, to: :last_deployment, allow_nil: true
 
diff --git a/app/models/generic_commit_status.rb b/app/models/generic_commit_status.rb
index 532b8f4ad69..5ac8bde44cd 100644
--- a/app/models/generic_commit_status.rb
+++ b/app/models/generic_commit_status.rb
@@ -1,7 +1,7 @@
 class GenericCommitStatus < CommitStatus
   before_validation :set_default_values
 
-  validates :target_url, addressable_url: true,
+  validates :target_url, url: true,
                          length: { maximum: 255 },
                          allow_nil: true
 
diff --git a/app/models/hooks/system_hook.rb b/app/models/hooks/system_hook.rb
index 0528266e5b3..6bef00f26ea 100644
--- a/app/models/hooks/system_hook.rb
+++ b/app/models/hooks/system_hook.rb
@@ -11,4 +11,9 @@ class SystemHook < WebHook
   default_value_for :push_events, false
   default_value_for :repository_update_events, true
   default_value_for :merge_requests_events, false
+
+  # Allow urls pointing localhost and the local network
+  def allow_local_requests?
+    true
+  end
 end
diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index 27729deeac9..e353abdda9c 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -3,7 +3,9 @@ class WebHook < ActiveRecord::Base
 
   has_many :web_hook_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
-  validates :url, presence: true, url: true
+  validates :url, presence: true, public_url: { allow_localhost: lambda(&:allow_local_requests?),
+                                                allow_local_network: lambda(&:allow_local_requests?) }
+
   validates :token, format: { without: /\n/ }
 
   def execute(data, hook_name)
@@ -13,4 +15,9 @@ class WebHook < ActiveRecord::Base
   def async_execute(data, hook_name)
     WebHookService.new(self, data, hook_name).async_execute
   end
+
+  # Allow urls pointing localhost and the local network
+  def allow_local_requests?
+    false
+  end
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index e275ac4dc6f..af9fca62dc3 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -289,8 +289,9 @@ class Project < ActiveRecord::Base
 
   validates :namespace, presence: true
   validates :name, uniqueness: { scope: :namespace_id }
-  validates :import_url, addressable_url: true, if: :external_import?
-  validates :import_url, importable_url: true, if: [:external_import?, :import_url_changed?]
+  validates :import_url, url: { protocols: %w(http https ssh git),
+                                allow_localhost: false,
+                                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
   validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? }
diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb
index 54e4b3278db..7f4c47a6d14 100644
--- a/app/models/project_services/bamboo_service.rb
+++ b/app/models/project_services/bamboo_service.rb
@@ -3,7 +3,7 @@ class BambooService < CiService
 
   prop_accessor :bamboo_url, :build_key, :username, :password
 
-  validates :bamboo_url, presence: true, url: true, if: :activated?
+  validates :bamboo_url, presence: true, public_url: true, if: :activated?
   validates :build_key, presence: true, if: :activated?
   validates :username,
     presence: true,
diff --git a/app/models/project_services/bugzilla_service.rb b/app/models/project_services/bugzilla_service.rb
index 046e2809f45..e4e3a80976b 100644
--- a/app/models/project_services/bugzilla_service.rb
+++ b/app/models/project_services/bugzilla_service.rb
@@ -1,5 +1,5 @@
 class BugzillaService < IssueTrackerService
-  validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated?
+  validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
   prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
 
diff --git a/app/models/project_services/buildkite_service.rb b/app/models/project_services/buildkite_service.rb
index d2aaff8817a..35884c4560c 100644
--- a/app/models/project_services/buildkite_service.rb
+++ b/app/models/project_services/buildkite_service.rb
@@ -8,7 +8,7 @@ class BuildkiteService < CiService
   prop_accessor :project_url, :token
   boolean_accessor :enable_ssl_verification
 
-  validates :project_url, presence: true, url: true, if: :activated?
+  validates :project_url, presence: true, public_url: true, if: :activated?
   validates :token, presence: true, if: :activated?
 
   after_save :compose_service_hook, if: :activated?
diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb
index 7591ab4f478..ae0debbd3ac 100644
--- a/app/models/project_services/chat_notification_service.rb
+++ b/app/models/project_services/chat_notification_service.rb
@@ -8,7 +8,7 @@ class ChatNotificationService < Service
   prop_accessor :webhook, :username, :channel
   boolean_accessor :notify_only_broken_pipelines, :notify_only_default_branch
 
-  validates :webhook, presence: true, url: true, if: :activated?
+  validates :webhook, presence: true, public_url: true, if: :activated?
 
   def initialize_properties
     # Custom serialized properties initialization
diff --git a/app/models/project_services/custom_issue_tracker_service.rb b/app/models/project_services/custom_issue_tracker_service.rb
index b9e3e982b64..456c7f5cee2 100644
--- a/app/models/project_services/custom_issue_tracker_service.rb
+++ b/app/models/project_services/custom_issue_tracker_service.rb
@@ -1,5 +1,5 @@
 class CustomIssueTrackerService < IssueTrackerService
-  validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated?
+  validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
   prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
 
diff --git a/app/models/project_services/drone_ci_service.rb b/app/models/project_services/drone_ci_service.rb
index a4bf427ac0b..ab4e46da89f 100644
--- a/app/models/project_services/drone_ci_service.rb
+++ b/app/models/project_services/drone_ci_service.rb
@@ -4,7 +4,7 @@ class DroneCiService < CiService
   prop_accessor :drone_url, :token
   boolean_accessor :enable_ssl_verification
 
-  validates :drone_url, presence: true, url: true, if: :activated?
+  validates :drone_url, presence: true, public_url: true, if: :activated?
   validates :token, presence: true, if: :activated?
 
   after_save :compose_service_hook, if: :activated?
diff --git a/app/models/project_services/external_wiki_service.rb b/app/models/project_services/external_wiki_service.rb
index 1553f169827..a4b1ef09e93 100644
--- a/app/models/project_services/external_wiki_service.rb
+++ b/app/models/project_services/external_wiki_service.rb
@@ -1,7 +1,7 @@
 class ExternalWikiService < Service
   prop_accessor :external_wiki_url
 
-  validates :external_wiki_url, presence: true, url: true, if: :activated?
+  validates :external_wiki_url, presence: true, public_url: true, if: :activated?
 
   def title
     'External Wiki'
diff --git a/app/models/project_services/gitlab_issue_tracker_service.rb b/app/models/project_services/gitlab_issue_tracker_service.rb
index 88c428b4aae..16e32a4139e 100644
--- a/app/models/project_services/gitlab_issue_tracker_service.rb
+++ b/app/models/project_services/gitlab_issue_tracker_service.rb
@@ -1,7 +1,7 @@
 class GitlabIssueTrackerService < IssueTrackerService
   include Gitlab::Routing
 
-  validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated?
+  validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
   prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
 
diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index ed4bbfb6cfc..eb3261c902f 100644
--- a/app/models/project_services/jira_service.rb
+++ b/app/models/project_services/jira_service.rb
@@ -3,8 +3,8 @@ class JiraService < IssueTrackerService
   include ApplicationHelper
   include ActionView::Helpers::AssetUrlHelper
 
-  validates :url, url: true, presence: true, if: :activated?
-  validates :api_url, url: true, allow_blank: true
+  validates :url, public_url: true, presence: true, if: :activated?
+  validates :api_url, public_url: true, allow_blank: true
   validates :username, presence: true, if: :activated?
   validates :password, presence: true, if: :activated?
 
diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb
index 20fed432e55..ddd4026019b 100644
--- a/app/models/project_services/kubernetes_service.rb
+++ b/app/models/project_services/kubernetes_service.rb
@@ -24,7 +24,7 @@ class KubernetesService < DeploymentService
   prop_accessor :ca_pem
 
   with_options presence: true, if: :activated? do
-    validates :api_url, url: true
+    validates :api_url, public_url: true
     validates :token
   end
 
diff --git a/app/models/project_services/mock_ci_service.rb b/app/models/project_services/mock_ci_service.rb
index 2221459c90b..b89dc07a73e 100644
--- a/app/models/project_services/mock_ci_service.rb
+++ b/app/models/project_services/mock_ci_service.rb
@@ -3,7 +3,7 @@ class MockCiService < CiService
   ALLOWED_STATES = %w[failed canceled running pending success success_with_warnings skipped not_found].freeze
 
   prop_accessor :mock_service_url
-  validates :mock_service_url, presence: true, url: true, if: :activated?
+  validates :mock_service_url, presence: true, public_url: true, if: :activated?
 
   def title
     'MockCI'
diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb
index dcaeb65dc32..df4254e0523 100644
--- a/app/models/project_services/prometheus_service.rb
+++ b/app/models/project_services/prometheus_service.rb
@@ -6,7 +6,7 @@ class PrometheusService < MonitoringService
   boolean_accessor :manual_configuration
 
   with_options presence: true, if: :manual_configuration? do
-    validates :api_url, url: true
+    validates :api_url, public_url: true
   end
 
   before_save :synchronize_service_state
diff --git a/app/models/project_services/redmine_service.rb b/app/models/project_services/redmine_service.rb
index 6acf611eba5..3721093a6d1 100644
--- a/app/models/project_services/redmine_service.rb
+++ b/app/models/project_services/redmine_service.rb
@@ -1,5 +1,5 @@
 class RedmineService < IssueTrackerService
-  validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated?
+  validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
   prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
 
diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb
index 145313b8e71..802678147cf 100644
--- a/app/models/project_services/teamcity_service.rb
+++ b/app/models/project_services/teamcity_service.rb
@@ -3,7 +3,7 @@ class TeamcityService < CiService
 
   prop_accessor :teamcity_url, :build_type, :username, :password
 
-  validates :teamcity_url, presence: true, url: true, if: :activated?
+  validates :teamcity_url, presence: true, public_url: true, if: :activated?
   validates :build_type, presence: true, if: :activated?
   validates :username,
     presence: true,
diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb
index bbf8fd9c6a7..9722cbb2b7c 100644
--- a/app/models/remote_mirror.rb
+++ b/app/models/remote_mirror.rb
@@ -17,7 +17,6 @@ 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, addressable_url: true, if: :url_changed?
 
   before_save :set_new_remote_name, if: :mirror_url_changed?
 
diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb
index bdd9598f85a..00080717600 100644
--- a/app/services/projects/import_service.rb
+++ b/app/services/projects/import_service.rb
@@ -29,7 +29,7 @@ module Projects
     def add_repository_to_project
       if project.external_import? && !unknown_url?
         begin
-          Gitlab::UrlBlocker.validate!(project.import_url, valid_ports: Project::VALID_IMPORT_PORTS)
+          Gitlab::UrlBlocker.validate!(project.import_url, ports: Project::VALID_IMPORT_PORTS)
         rescue Gitlab::UrlBlocker::BlockedUrlError => e
           raise Error, "Blocked import URL: #{e.message}"
         end
diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
deleted file mode 100644
index 94542125d43..00000000000
--- a/app/validators/addressable_url_validator.rb
+++ /dev/null
@@ -1,45 +0,0 @@
-# AddressableUrlValidator
-#
-# Custom validator for URLs. This is a stricter version of UrlValidator - it also checks
-# for using the right protocol, but it actually parses the URL checking for any syntax errors.
-# The regex is also different from `URI` as we use `Addressable::URI` here.
-#
-# By default, only URLs for http, https, ssh, and git protocols will be considered valid.
-# Provide a `:protocols` option to configure accepted protocols.
-#
-# Example:
-#
-#   class User < ActiveRecord::Base
-#     validates :personal_url, addressable_url: true
-#
-#     validates :ftp_url, addressable_url: { protocols: %w(ftp) }
-#
-#     validates :git_url, addressable_url: { protocols: %w(http https ssh git) }
-#   end
-#
-class AddressableUrlValidator < ActiveModel::EachValidator
-  DEFAULT_OPTIONS = { protocols: %w(http https ssh git) }.freeze
-
-  def validate_each(record, attribute, value)
-    unless valid_url?(value)
-      record.errors.add(attribute, "must be a valid URL")
-    end
-  end
-
-  private
-
-  def valid_url?(value)
-    return false unless value
-
-    valid_protocol?(value) && valid_uri?(value)
-  end
-
-  def valid_uri?(value)
-    Gitlab::UrlSanitizer.valid?(value)
-  end
-
-  def valid_protocol?(value)
-    options = DEFAULT_OPTIONS.merge(self.options)
-    value =~ /\A#{URI.regexp(options[:protocols])}\z/
-  end
-end
diff --git a/app/validators/importable_url_validator.rb b/app/validators/importable_url_validator.rb
deleted file mode 100644
index 612d3c71913..00000000000
--- a/app/validators/importable_url_validator.rb
+++ /dev/null
@@ -1,11 +0,0 @@
-# ImportableUrlValidator
-#
-# This validator blocks projects from using dangerous import_urls to help
-# protect against Server-side Request Forgery (SSRF).
-class ImportableUrlValidator < ActiveModel::EachValidator
-  def validate_each(record, attribute, value)
-    Gitlab::UrlBlocker.validate!(value, valid_ports: Project::VALID_IMPORT_PORTS)
-  rescue Gitlab::UrlBlocker::BlockedUrlError => e
-    record.errors.add(attribute, "is blocked: #{e.message}")
-  end
-end
diff --git a/app/validators/public_url_validator.rb b/app/validators/public_url_validator.rb
new file mode 100644
index 00000000000..1e8118fccbb
--- /dev/null
+++ b/app/validators/public_url_validator.rb
@@ -0,0 +1,26 @@
+# PublicUrlValidator
+#
+# Custom validator for URLs. This validator works like UrlValidator but
+# it blocks by default urls pointing to localhost or the local network.
+#
+# This validator accepts the same params UrlValidator does.
+#
+# Example:
+#
+#   class User < ActiveRecord::Base
+#     validates :personal_url, public_url: true
+#
+#     validates :ftp_url, public_url: { protocols: %w(ftp) }
+#
+#     validates :git_url, public_url: { allow_localhost: true, allow_local_network: true}
+#   end
+#
+class PublicUrlValidator < UrlValidator
+  private
+
+  def default_options
+    # By default block all urls pointing to localhost or the local network
+    super.merge(allow_localhost: false,
+                allow_local_network: false)
+  end
+end
diff --git a/app/validators/url_placeholder_validator.rb b/app/validators/url_placeholder_validator.rb
deleted file mode 100644
index dd681218b6b..00000000000
--- a/app/validators/url_placeholder_validator.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-# UrlValidator
-#
-# Custom validator for URLs.
-#
-# By default, only URLs for the HTTP(S) protocols will be considered valid.
-# Provide a `:protocols` option to configure accepted protocols.
-#
-# Also, this validator can help you validate urls with placeholders inside.
-# Usually, if you have a url like 'http://www.example.com/%{project_path}' the
-# URI parser will reject that URL format. Provide a `:placeholder_regex` option
-# to configure accepted placeholders.
-#
-# Example:
-#
-#   class User < ActiveRecord::Base
-#     validates :personal_url, url: true
-#
-#     validates :ftp_url, url: { protocols: %w(ftp) }
-#
-#     validates :git_url, url: { protocols: %w(http https ssh git) }
-#
-#     validates :placeholder_url, url: { placeholder_regex: /(project_path|project_id|default_branch)/ }
-#   end
-#
-class UrlPlaceholderValidator < UrlValidator
-  def validate_each(record, attribute, value)
-    placeholder_regex = self.options[:placeholder_regex]
-    value = value.gsub(/%{#{placeholder_regex}}/, 'foo') if placeholder_regex && value
-
-    super(record, attribute, value)
-  end
-end
diff --git a/app/validators/url_validator.rb b/app/validators/url_validator.rb
index a77beb2683d..8648c4c75e3 100644
--- a/app/validators/url_validator.rb
+++ b/app/validators/url_validator.rb
@@ -15,25 +15,63 @@
 #     validates :git_url, url: { protocols: %w(http https ssh git) }
 #   end
 #
+# 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.
+#
+# Example:
+#   class User < ActiveRecord::Base
+#     validates :personal_url, url: { allow_localhost: false, allow_local_network: false}
+#
+#     validates :web_url, url: { ports: [80, 443] }
+#   end
 class UrlValidator < ActiveModel::EachValidator
+  DEFAULT_PROTOCOLS = %w(http https).freeze
+
+  attr_reader :record
+
   def validate_each(record, attribute, value)
-    unless valid_url?(value)
+    @record = record
+
+    if value.present?
+      value.strip!
+    else
       record.errors.add(attribute, "must be a valid URL")
     end
+
+    Gitlab::UrlBlocker.validate!(value, blocker_args)
+  rescue Gitlab::UrlBlocker::BlockedUrlError => e
+    record.errors.add(attribute, "is blocked: #{e.message}")
   end
 
   private
 
   def default_options
-    @default_options ||= { protocols: %w(http https) }
+    # By default the validator doesn't block any url based on the ip address
+    {
+      protocols: DEFAULT_PROTOCOLS,
+      ports: [],
+      allow_localhost: true,
+      allow_local_network: true
+    }
   end
 
-  def valid_url?(value)
-    return false if value.nil?
+  def current_options
+    options = self.options.map do |option, value|
+      [option, value.is_a?(Proc) ? value.call(record) : value]
+    end.to_h
+
+    default_options.merge(options)
+  end
 
-    options = default_options.merge(self.options)
+  def blocker_args
+    current_options.slice(:allow_localhost, :allow_local_network, :protocols, :ports).tap do |args|
+      if allow_setting_local_requests?
+        args[:allow_localhost] = args[:allow_local_network] = true
+      end
+    end
+  end
 
-    value.strip!
-    value =~ /\A#{URI.regexp(options[:protocols])}\z/
+  def allow_setting_local_requests?
+    ApplicationSetting.current&.allow_local_requests_from_hooks_and_services?
   end
 end