Skip to content

Commit d119d3d

Browse files
Thong Kuahbluegod
Thong Kuah
authored andcommitted
Align UrlValidator to validate_url gem implementation.
Renamed UrlValidator to AddressableUrlValidator to avoid 'url:' naming collision with ActiveModel::Validations::UrlValidator in 'validates' statement. Make use of the options attribute of the parent class ActiveModel::EachValidator. Add more options: allow_nil, allow_blank, message. Renamed 'protocols' option to 'schemes' to match the option naming from UrlValidator.
1 parent 79bf4bd commit d119d3d

21 files changed

+292
-172
lines changed

app/models/application_setting.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,17 +48,17 @@ class ApplicationSetting < ApplicationRecord
4848

4949
validates :home_page_url,
5050
allow_blank: true,
51-
url: true,
51+
addressable_url: true,
5252
if: :home_page_url_column_exists?
5353

5454
validates :help_page_support_url,
5555
allow_blank: true,
56-
url: true,
56+
addressable_url: true,
5757
if: :help_page_support_url_column_exists?
5858

5959
validates :after_sign_out_path,
6060
allow_blank: true,
61-
url: true
61+
addressable_url: true
6262

6363
validates :admin_notification_email,
6464
devise_email: true,
@@ -218,7 +218,7 @@ class ApplicationSetting < ApplicationRecord
218218
if: :external_authorization_service_enabled
219219

220220
validates :external_authorization_service_url,
221-
url: true, allow_blank: true,
221+
addressable_url: true, allow_blank: true,
222222
if: :external_authorization_service_enabled
223223

224224
validates :external_authorization_service_timeout,

app/models/badge.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ class Badge < ApplicationRecord
2222

2323
scope :order_created_at_asc, -> { reorder(created_at: :asc) }
2424

25-
validates :link_url, :image_url, url: { protocols: %w(http https) }
25+
validates :link_url, :image_url, addressable_url: true
2626
validates :type, presence: true
2727

2828
def rendered_link_url(project = nil)

app/models/ci/build_runner_session.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ class BuildRunnerSession < ApplicationRecord
1313
belongs_to :build, class_name: 'Ci::Build', inverse_of: :runner_session
1414

1515
validates :build, presence: true
16-
validates :url, url: { protocols: %w(https) }
16+
validates :url, addressable_url: { schemes: %w(https) }
1717

1818
def terminal_specification
1919
wss_url = Gitlab::UrlHelpers.as_wss(self.url)

app/models/environment.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class Environment < ApplicationRecord
3535
validates :external_url,
3636
length: { maximum: 255 },
3737
allow_nil: true,
38-
url: true
38+
addressable_url: true
3939

4040
delegate :stop_action, :manual_actions, to: :last_deployment, allow_nil: true
4141

app/models/error_tracking/project_error_tracking_setting.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ class ProjectErrorTrackingSetting < ApplicationRecord
2222

2323
belongs_to :project
2424

25-
validates :api_url, length: { maximum: 255 }, public_url: true, url: { enforce_sanitization: true, ascii_only: true }, allow_nil: true
25+
validates :api_url, length: { maximum: 255 }, public_url: { enforce_sanitization: true, ascii_only: true }, allow_nil: true
2626

2727
validates :api_url, presence: { message: 'is a required field' }, if: :enabled
2828

app/models/generic_commit_status.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
class GenericCommitStatus < CommitStatus
44
before_validation :set_default_values
55

6-
validates :target_url, url: true,
6+
validates :target_url, addressable_url: true,
77
length: { maximum: 255 },
88
allow_nil: true
99

app/models/project.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,7 @@ class Project < ApplicationRecord
327327

328328
validates :namespace, presence: true
329329
validates :name, uniqueness: { scope: :namespace_id }
330-
validates :import_url, public_url: { protocols: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS },
330+
validates :import_url, public_url: { schemes: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS },
331331
ports: ->(project) { project.persisted? ? VALID_MIRROR_PORTS : VALID_IMPORT_PORTS },
332332
enforce_user: true }, if: [:external_import?, :import_url_changed?]
333333
validates :star_count, numericality: { greater_than_or_equal_to: 0 }

app/models/releases/link.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ class Link < ApplicationRecord
66

77
belongs_to :release
88

9-
validates :url, presence: true, url: { protocols: %w(http https ftp) }, uniqueness: { scope: :release }
9+
validates :url, presence: true, addressable_url: { schemes: %w(http https ftp) }, uniqueness: { scope: :release }
1010
validates :name, presence: true, uniqueness: { scope: :release }
1111

1212
scope :sorted, -> { order(created_at: :desc) }

app/models/remote_mirror.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ class RemoteMirror < ApplicationRecord
1717

1818
belongs_to :project, inverse_of: :remote_mirrors
1919

20-
validates :url, presence: true, public_url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true }
20+
validates :url, presence: true, public_url: { schemes: %w(ssh git http https), allow_blank: true, enforce_user: true }
2121

2222
before_save :set_new_remote_name, if: :mirror_url_changed?
2323

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
# frozen_string_literal: true
2+
3+
# AddressableUrlValidator
4+
#
5+
# Custom validator for URLs. This is a stricter version of UrlValidator - it also checks
6+
# for using the right protocol, but it actually parses the URL checking for any syntax errors.
7+
# The regex is also different from `URI` as we use `Addressable::URI` here.
8+
#
9+
# By default, only URLs for the HTTP(S) schemes will be considered valid.
10+
# Provide a `:schemes` option to configure accepted schemes.
11+
#
12+
# Example:
13+
#
14+
# class User < ActiveRecord::Base
15+
# validates :personal_url, addressable_url: true
16+
#
17+
# validates :ftp_url, addressable_url: { schemes: %w(ftp) }
18+
#
19+
# validates :git_url, addressable_url: { schemes: %w(http https ssh git) }
20+
# end
21+
#
22+
# This validator can also block urls pointing to localhost or the local network to
23+
# protect against Server-side Request Forgery (SSRF), or check for the right port.
24+
#
25+
# Configuration options:
26+
# * <tt>message</tt> - A custom error message (default is: "must be a valid URL").
27+
# * <tt>schemes</tt> - Array of URI schemes. Default: +['http', 'https']+
28+
# * <tt>allow_localhost</tt> - Allow urls pointing to +localhost+. Default: +true+
29+
# * <tt>allow_local_network</tt> - Allow urls pointing to private network addresses. Default: +true+
30+
# * <tt>allow_blank</tt> - Allow urls to be +blank+. Default: +false+
31+
# * <tt>allow_nil</tt> - Allow urls to be +nil+. Default: +false+
32+
# * <tt>ports</tt> - Allowed ports. Default: +all+.
33+
# * <tt>enforce_user</tt> - Validate user format. Default: +false+
34+
# * <tt>enforce_sanitization</tt> - Validate that there are no html/css/js tags. Default: +false+
35+
#
36+
# Example:
37+
# class User < ActiveRecord::Base
38+
# validates :personal_url, addressable_url: { allow_localhost: false, allow_local_network: false}
39+
#
40+
# validates :web_url, addressable_url: { ports: [80, 443] }
41+
# end
42+
class AddressableUrlValidator < ActiveModel::EachValidator
43+
attr_reader :record
44+
45+
BLOCKER_VALIDATE_OPTIONS = {
46+
schemes: %w(http https),
47+
ports: [],
48+
allow_localhost: true,
49+
allow_local_network: true,
50+
ascii_only: false,
51+
enforce_user: false,
52+
enforce_sanitization: false
53+
}.freeze
54+
55+
DEFAULT_OPTIONS = BLOCKER_VALIDATE_OPTIONS.merge({
56+
message: 'must be a valid URL'
57+
}).freeze
58+
59+
def initialize(options)
60+
options.reverse_merge!(DEFAULT_OPTIONS)
61+
62+
super(options)
63+
end
64+
65+
def validate_each(record, attribute, value)
66+
@record = record
67+
68+
unless value.present?
69+
record.errors.add(attribute, options.fetch(:message))
70+
return
71+
end
72+
73+
value = strip_value!(record, attribute, value)
74+
75+
Gitlab::UrlBlocker.validate!(value, blocker_args)
76+
rescue Gitlab::UrlBlocker::BlockedUrlError => e
77+
record.errors.add(attribute, "is blocked: #{e.message}")
78+
end
79+
80+
private
81+
82+
def strip_value!(record, attribute, value)
83+
new_value = value.strip
84+
return value if new_value == value
85+
86+
record.public_send("#{attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend
87+
end
88+
89+
def current_options
90+
options.map do |option, value|
91+
[option, value.is_a?(Proc) ? value.call(record) : value]
92+
end.to_h
93+
end
94+
95+
def blocker_args
96+
current_options.slice(*BLOCKER_VALIDATE_OPTIONS.keys).tap do |args|
97+
if self.class.allow_setting_local_requests?
98+
args[:allow_localhost] = args[:allow_local_network] = true
99+
end
100+
end
101+
end
102+
103+
def self.allow_setting_local_requests?
104+
# We cannot use Gitlab::CurrentSettings as ApplicationSetting itself
105+
# uses UrlValidator to validate urls. This ends up in a cycle
106+
# when Gitlab::CurrentSettings creates an ApplicationSetting which then
107+
# calls this validator.
108+
#
109+
# See https://gitlab.com/gitlab-org/gitlab-ee/issues/9833
110+
ApplicationSetting.current&.allow_local_requests_from_hooks_and_services?
111+
end
112+
end

app/validators/public_url_validator.rb

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
# PublicUrlValidator
44
#
5-
# Custom validator for URLs. This validator works like UrlValidator but
5+
# Custom validator for URLs. This validator works like AddressableUrlValidator but
66
# it blocks by default urls pointing to localhost or the local network.
77
#
88
# This validator accepts the same params UrlValidator does.
@@ -12,17 +12,20 @@
1212
# class User < ActiveRecord::Base
1313
# validates :personal_url, public_url: true
1414
#
15-
# validates :ftp_url, public_url: { protocols: %w(ftp) }
15+
# validates :ftp_url, public_url: { schemes: %w(ftp) }
1616
#
1717
# validates :git_url, public_url: { allow_localhost: true, allow_local_network: true}
1818
# end
1919
#
20-
class PublicUrlValidator < UrlValidator
21-
private
20+
class PublicUrlValidator < AddressableUrlValidator
21+
DEFAULT_OPTIONS = {
22+
allow_localhost: false,
23+
allow_local_network: false
24+
}.freeze
2225

23-
def default_options
24-
# By default block all urls pointing to localhost or the local network
25-
super.merge(allow_localhost: false,
26-
allow_local_network: false)
26+
def initialize(options)
27+
options.reverse_merge!(DEFAULT_OPTIONS)
28+
29+
super(options)
2730
end
2831
end

app/validators/url_validator.rb

Lines changed: 0 additions & 104 deletions
This file was deleted.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
title: "Align UrlValidator to validate_url gem implementation"
3+
merge_request: 27194
4+
author: Horatiu Eugen Vlad
5+
type: fixed

lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ class WebUploadStrategy < BaseAfterExportStrategy
88
POST_METHOD = 'POST'.freeze
99
INVALID_HTTP_METHOD = 'invalid. Only PUT and POST methods allowed.'.freeze
1010

11-
validates :url, url: true
11+
validates :url, addressable_url: true
1212

1313
validate do
1414
unless [PUT_METHOD, POST_METHOD].include?(http_method.upcase)

0 commit comments

Comments
 (0)