Skip to content

Commit c63ed77

Browse files
committed
Warn on inaccessible website in profile (instead of raising error)
But still block internal URLs
1 parent 8aa4495 commit c63ed77

7 files changed

Lines changed: 30 additions & 22 deletions

File tree

app/assets/javascripts/url_checker.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ var UrlChecker = {
1010

1111
$(this).addClass('loading');
1212

13+
if (!checkExistsUrl) {
14+
return UrlChecker.validUrl(input, url, testValidUrl);
15+
}
1316
$.ajax({
1417
method: 'POST',
1518
dataType: 'json',

app/models/profile.rb

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

77
before_validation :normalize_orcid
88
validates :firstname, :surname, :description, presence: true, if: :public?
9-
validates :website, url: true, http_url: true, allow_blank: true
9+
validates :website, url: true, http_url: { allow_inaccessible: true }, allow_blank: true
1010
validates :orcid, orcid: true, allow_blank: true
1111
after_validation :check_public
1212
after_commit :reindex_trainer

app/validators/http_url_validator.rb

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,34 @@
11
class HttpUrlValidator < ActiveModel::EachValidator
2-
def self.blocked?(value)
2+
def blocked?(value)
33
(TeSS::Config.blocked_domains || []).any? do |regex|
44
value =~ regex
55
end
66
end
77

8-
def self.accessible?(value)
8+
def accessible?(value)
9+
code = nil
910
begin
1011
uri = URI.parse(value) rescue nil
1112
if uri && (uri.scheme == 'http' || uri.scheme == 'https')
1213
PrivateAddressCheck.only_public_connections do
1314
res = HTTParty.get(value, { timeout: Rails.env.test? ? 1 : 5 })
14-
res.code == 200
15+
code = res.code
1516
end
1617
end
17-
rescue PrivateAddressCheck::PrivateConnectionAttemptedError, Net::OpenTimeout, Net::ReadTimeout, SocketError,
18+
rescue PrivateAddressCheck::PrivateConnectionAttemptedError
19+
return false
20+
rescue Net::OpenTimeout, Net::ReadTimeout, SocketError,
1821
Errno::ECONNREFUSED, Errno::EHOSTUNREACH, OpenSSL::SSL::SSLError, URI::InvalidURIError
19-
false
22+
code = 999
2023
end
24+
options[:allow_inaccessible] || code == 200
2125
end
2226

2327
def validate_each(record, attribute, value)
2428
if value.present?
25-
if self.class.blocked?(value)
29+
if blocked?(value)
2630
record.errors.add(attribute, 'is blocked')
27-
elsif !self.class.accessible?(value)
31+
elsif !accessible?(value)
2832
record.errors.add(attribute, 'is not accessible')
2933
end
3034
end

app/views/users/_form.html.erb

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,10 @@
1414

1515
<%= f.input :surname, as: :string %>
1616

17-
<%= f.input :website, as: :string, input_html: { title: t('profile.hints.website') } %>
17+
<%# Blank hint is to render a "help-block" in which errors will be displayed.
18+
Blank "data-url-check" property is just so the event gets bound %>
19+
<%= f.input :website, as: :string, hint: ' ', input_html: { title: t('profile.hints.website'),
20+
data: { 'url-check': '', 'url-valid': test_url_path } } %>
1821

1922
<%= f.input :orcid, as: :string, label: 'ORCID', input_html: { title: t('profile.hints.orcid') } %>
2023

test/controllers/application_controller_test.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ class ApplicationControllerTest < ActionController::TestCase
3737
assert_equal 'Could not access the given URL', JSON.parse(response.body)['message']
3838

3939
with_net_connection do
40-
get :test_url, params: { url: 'http://127.0.0.1', format: :json }
40+
get :test_url, params: { url: 'http://192.168.0.1', format: :json }
4141
assert_equal 'Could not access the given URL', JSON.parse(response.body)['message']
4242
end
4343
end

test/controllers/users_controller_test.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -305,8 +305,8 @@ class UsersControllerTest < ActionController::TestCase
305305

306306
# check errors
307307
profile = assigns(:user).profile
308-
assert_equal 3, profile.errors.size, 'invalid number of errors'
309-
assert_equal 2, profile.errors.full_messages_for(:website).size, 'invalid error count for: website'
308+
assert_equal 2, profile.errors.size, 'invalid number of errors'
309+
assert_equal 1, profile.errors.full_messages_for(:website).size, 'invalid error count for: website'
310310
assert_equal 1, profile.errors.full_messages_for(:orcid).size, 'invalid error count for: orcid'
311311
assert_equal "Website is not a valid URL", profile.errors.full_messages_for(:website).first
312312
assert_equal "ORCID isn't a valid ORCID identifier", profile.errors.full_messages_for(:orcid).first

test/models/profile_test.rb

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -73,24 +73,22 @@ class ProfileTest < ActiveSupport::TestCase
7373
# accessible domain
7474
assert profile.update(website: 'http://200host.com')
7575

76-
# blocked domain
76+
# blocked domain is not OK
7777
refute profile.update(website: 'https://bad-domain.example/')
7878
assert profile.errors.added?(:website, 'is blocked')
79-
refute profile.errors.added?(:website, 'is not accessible'), 'connection should not be attempted to blocked domains'
8079

81-
# inaccessible domain
82-
refute profile.update(website: 'http://404host.com')
83-
assert profile.errors.added?(:website, 'is not accessible')
80+
# inaccessible domain is OK
81+
assert profile.update(website: 'http://404host.com')
8482
refute profile.errors.added?(:website, 'is blocked')
8583

86-
# private address
84+
# private address is not OK
8785
with_net_connection do # Allow request through to be caught by private_address_check
88-
refute profile.update(website: 'http://127.0.0.1')
86+
refute profile.update(website: 'http://192.168.0.1')
8987
assert profile.errors.added?(:website, 'is not accessible')
9088
end
9189

92-
# address that times out
93-
refute profile.update(website: 'http://slowhost.com')
94-
assert profile.errors.added?(:website, 'is not accessible')
90+
# address that times out is OK
91+
assert profile.update(website: 'http://slowhost.com')
92+
refute profile.errors.added?(:website, 'is not accessible')
9593
end
9694
end

0 commit comments

Comments
 (0)