diff options
author | Thong Kuah <tkuah@gitlab.com> | 2019-04-11 09:29:07 +0300 |
---|---|---|
committer | James Lopez <james@gitlab.com> | 2019-04-11 09:29:07 +0300 |
commit | d119d3d1b25aac661e6251addf87b280bd37f0c5 (patch) | |
tree | aeaf0d9503326ec7f51968e8d1de48d83ce90503 /spec | |
parent | 79bf4bdaad438dc0f82771b102f3c07225a428da (diff) |
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.
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/projects/mirrors_controller_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/url_blocker_spec.rb | 6 | ||||
-rw-r--r-- | spec/requests/api/commit_statuses_spec.rb | 17 | ||||
-rw-r--r-- | spec/support/shared_examples/url_validator_examples.rb | 24 | ||||
-rw-r--r-- | spec/validators/addressable_url_validator_spec.rb (renamed from spec/validators/url_validator_spec.rb) | 131 | ||||
-rw-r--r-- | spec/validators/public_url_validator_spec.rb | 8 |
6 files changed, 146 insertions, 42 deletions
diff --git a/spec/controllers/projects/mirrors_controller_spec.rb b/spec/controllers/projects/mirrors_controller_spec.rb index f2b73956e8d..3ababe18055 100644 --- a/spec/controllers/projects/mirrors_controller_spec.rb +++ b/spec/controllers/projects/mirrors_controller_spec.rb @@ -79,7 +79,7 @@ describe Projects::MirrorsController do do_put(project, remote_mirrors_attributes: remote_mirror_attributes) expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-push-remote-settings')) - expect(flash[:alert]).to match(/Only allowed protocols are/) + expect(flash[:alert]).to match(/Only allowed schemes are/) end it 'does not create a RemoteMirror object' do diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 62970bd8cb6..445a56ab0d8 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -23,10 +23,10 @@ describe Gitlab::UrlBlocker do expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git', ports: ports)).to be true end - it 'returns true for bad protocol' do - expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['https'])).to be false + it 'returns true for bad scheme' do + expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', schemes: ['https'])).to be false expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false - expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['http'])).to be true + expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', schemes: ['http'])).to be true end it 'returns true for bad protocol on configured web/SSH host and ports' do diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index 9388343c392..b5e45f99109 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -306,7 +306,22 @@ describe API::CommitStatuses do it 'responds with bad request status and validation errors' do expect(response).to have_gitlab_http_status(400) expect(json_response['message']['target_url']) - .to include 'is blocked: Only allowed protocols are http, https' + .to include 'is blocked: Only allowed schemes are http, https' + end + end + + context 'when target URL is an unsupported scheme' do + before do + post api(post_url, developer), params: { + state: 'pending', + target_url: 'git://example.com' + } + end + + it 'responds with bad request status and validation errors' do + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']['target_url']) + .to include 'is blocked: Only allowed schemes are http, https' end end end diff --git a/spec/support/shared_examples/url_validator_examples.rb b/spec/support/shared_examples/url_validator_examples.rb index 1f7e2f7ff79..25277ccd9aa 100644 --- a/spec/support/shared_examples/url_validator_examples.rb +++ b/spec/support/shared_examples/url_validator_examples.rb @@ -1,15 +1,15 @@ -RSpec.shared_examples 'url validator examples' do |protocols| +RSpec.shared_examples 'url validator examples' do |schemes| let(:validator) { described_class.new(attributes: [:link_url], **options) } let!(:badge) { build(:badge, link_url: 'http://www.example.com') } - subject { validator.validate_each(badge, :link_url, badge.link_url) } + subject { validator.validate(badge) } - describe '#validates_each' do + describe '#validate' do context 'with no options' do let(:options) { {} } - it "allows #{protocols.join(',')} protocols by default" do - expect(validator.send(:default_options)[:protocols]).to eq protocols + it "allows #{schemes.join(',')} schemes by default" do + expect(validator.options[:schemes]).to eq schemes end it 'checks that the url structure is valid' do @@ -17,25 +17,25 @@ RSpec.shared_examples 'url validator examples' do |protocols| subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present end end - context 'with protocols' do - let(:options) { { protocols: %w[http] } } + context 'with schemes' do + let(:options) { { schemes: %w(http) } } - it 'allows urls with the defined protocols' do + it 'allows urls with the defined schemes' do subject - expect(badge.errors.empty?).to be true + expect(badge.errors).to be_empty end - it 'add error if the url protocol does not match the selected ones' do + it 'add error if the url scheme does not match the selected ones' do badge.link_url = 'https://www.example.com' subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present end end end diff --git a/spec/validators/url_validator_spec.rb b/spec/validators/addressable_url_validator_spec.rb index 1bb42382e8a..387e84b2d04 100644 --- a/spec/validators/url_validator_spec.rb +++ b/spec/validators/addressable_url_validator_spec.rb @@ -2,11 +2,11 @@ require 'spec_helper' -describe UrlValidator do +describe AddressableUrlValidator do let!(:badge) { build(:badge, link_url: 'http://www.example.com') } - subject { validator.validate_each(badge, :link_url, badge.link_url) } + subject { validator.validate(badge) } - include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS + include_examples 'url validator examples', described_class::DEFAULT_OPTIONS[:schemes] describe 'validations' do include_context 'invalid urls' @@ -14,13 +14,13 @@ describe UrlValidator do let(:validator) { described_class.new(attributes: [:link_url]) } it 'returns error when url is nil' do - expect(validator.validate_each(badge, :link_url, nil)).to be_nil - expect(badge.errors.first[1]).to eq 'must be a valid URL' + expect(validator.validate_each(badge, :link_url, nil)).to be_falsey + expect(badge.errors.first[1]).to eq validator.options.fetch(:message) end it 'returns error when url is empty' do - expect(validator.validate_each(badge, :link_url, '')).to be_nil - expect(badge.errors.first[1]).to eq 'must be a valid URL' + expect(validator.validate_each(badge, :link_url, '')).to be_falsey + expect(badge.errors.first[1]).to eq validator.options.fetch(:message) end it 'does not allow urls with CR or LF characters' do @@ -30,6 +30,17 @@ describe UrlValidator do end end end + + it 'provides all arguments to UrlBlock validate' do + expect(Gitlab::UrlBlocker) + .to receive(:validate!) + .with(badge.link_url, described_class::BLOCKER_VALIDATE_OPTIONS) + .and_return(true) + + subject + + expect(badge.errors).to be_empty + end end context 'by default' do @@ -40,7 +51,7 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be true + expect(badge.errors).to be_empty end it 'does not block urls pointing to the local network' do @@ -48,7 +59,23 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be true + expect(badge.errors).to be_empty + end + + it 'does block nil urls' do + badge.link_url = nil + + subject + + expect(badge.errors).to be_present + end + + it 'does block blank urls' do + badge.link_url = '\n\r \n' + + subject + + expect(badge.errors).to be_present end it 'strips urls' do @@ -67,6 +94,40 @@ describe UrlValidator do end end + context 'when message is set' do + let(:message) { 'is blocked: test message' } + let(:validator) { described_class.new(attributes: [:link_url], allow_nil: false, message: message) } + + it 'does block nil url with provided error message' do + expect(validator.validate_each(badge, :link_url, nil)).to be_falsey + expect(badge.errors.first[1]).to eq message + end + end + + context 'when allow_nil is set to true' do + let(:validator) { described_class.new(attributes: [:link_url], allow_nil: true) } + + it 'does not block nil urls' do + badge.link_url = nil + + subject + + expect(badge.errors).to be_empty + end + end + + context 'when allow_blank is set to true' do + let(:validator) { described_class.new(attributes: [:link_url], allow_blank: true) } + + it 'does not block blank urls' do + badge.link_url = "\n\r \n" + + subject + + expect(badge.errors).to be_empty + end + end + context 'when allow_localhost is set to false' do let(:validator) { described_class.new(attributes: [:link_url], allow_localhost: false) } @@ -75,7 +136,21 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present + end + + context 'when allow_setting_local_requests is set to true' do + it 'does not block urls pointing to localhost' do + expect(described_class) + .to receive(:allow_setting_local_requests?) + .and_return(true) + + badge.link_url = 'https://127.0.0.1' + + subject + + expect(badge.errors).to be_empty + end end end @@ -87,7 +162,21 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present + end + + context 'when allow_setting_local_requests is set to true' do + it 'does not block urls pointing to local network' do + expect(described_class) + .to receive(:allow_setting_local_requests?) + .and_return(true) + + badge.link_url = 'https://192.168.1.1' + + subject + + expect(badge.errors).to be_empty + end end end @@ -100,7 +189,7 @@ describe UrlValidator do it 'does not block any port' do subject - expect(badge.errors.empty?).to be true + expect(badge.errors).to be_empty end end @@ -110,7 +199,7 @@ describe UrlValidator do it 'blocks urls with a different port' do subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present end end end @@ -127,7 +216,7 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present end end @@ -139,7 +228,7 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be true + expect(badge.errors).to be_empty end end end @@ -156,7 +245,7 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present end end @@ -168,7 +257,7 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be true + expect(badge.errors).to be_empty end end end @@ -191,7 +280,7 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present end it 'prevents unsafe internal urls' do @@ -199,7 +288,7 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present end it 'allows safe urls' do @@ -207,7 +296,7 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be true + expect(badge.errors).to be_empty end end @@ -219,7 +308,7 @@ describe UrlValidator do subject - expect(badge.errors.empty?).to be true + expect(badge.errors).to be_empty end end end diff --git a/spec/validators/public_url_validator_spec.rb b/spec/validators/public_url_validator_spec.rb index 710dd3dc38e..f6364fb1dd5 100644 --- a/spec/validators/public_url_validator_spec.rb +++ b/spec/validators/public_url_validator_spec.rb @@ -1,20 +1,20 @@ require 'spec_helper' describe PublicUrlValidator do - include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS + include_examples 'url validator examples', AddressableUrlValidator::DEFAULT_OPTIONS[:schemes] context 'by default' do let(:validator) { described_class.new(attributes: [:link_url]) } let!(:badge) { build(:badge, link_url: 'http://www.example.com') } - subject { validator.validate_each(badge, :link_url, badge.link_url) } + subject { validator.validate(badge) } it 'blocks urls pointing to localhost' do badge.link_url = 'https://127.0.0.1' subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present end it 'blocks urls pointing to the local network' do @@ -22,7 +22,7 @@ describe PublicUrlValidator do subject - expect(badge.errors.empty?).to be false + expect(badge.errors).to be_present end end end |