diff options
| -rw-r--r-- | changelogs/unreleased/url-sanitizer-fixes.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/url_sanitizer.rb | 19 | ||||
| -rw-r--r-- | spec/lib/gitlab/url_sanitizer_spec.rb | 179 |
3 files changed, 147 insertions, 56 deletions
diff --git a/changelogs/unreleased/url-sanitizer-fixes.yml b/changelogs/unreleased/url-sanitizer-fixes.yml new file mode 100644 index 00000000000..769036c829c --- /dev/null +++ b/changelogs/unreleased/url-sanitizer-fixes.yml @@ -0,0 +1,5 @@ +--- +title: Fix problems sanitizing URLs with empty passwords +merge_request: 14083 +author: +type: fixed diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb index c81dc7e30d0..703adae12cb 100644 --- a/lib/gitlab/url_sanitizer.rb +++ b/lib/gitlab/url_sanitizer.rb @@ -9,7 +9,7 @@ module Gitlab end def self.valid?(url) - return false unless url + return false unless url.present? Addressable::URI.parse(url.strip) @@ -19,7 +19,12 @@ module Gitlab end def initialize(url, credentials: nil) - @url = Addressable::URI.parse(url.strip) + @url = Addressable::URI.parse(url.to_s.strip) + + %i[user password].each do |symbol| + credentials[symbol] = credentials[symbol].presence if credentials&.key?(symbol) + end + @credentials = credentials end @@ -29,13 +34,13 @@ module Gitlab def masked_url url = @url.dup - url.password = "*****" unless url.password.nil? - url.user = "*****" unless url.user.nil? + url.password = "*****" if url.password.present? + url.user = "*****" if url.user.present? url.to_s end def credentials - @credentials ||= { user: @url.user, password: @url.password } + @credentials ||= { user: @url.user.presence, password: @url.password.presence } end def full_url @@ -47,8 +52,10 @@ module Gitlab def generate_full_url return @url unless valid_credentials? @full_url = @url.dup - @full_url.user = credentials[:user] + @full_url.password = credentials[:password] + @full_url.user = credentials[:user] + @full_url end diff --git a/spec/lib/gitlab/url_sanitizer_spec.rb b/spec/lib/gitlab/url_sanitizer_spec.rb index 308b1a128be..fdc3990132a 100644 --- a/spec/lib/gitlab/url_sanitizer_spec.rb +++ b/spec/lib/gitlab/url_sanitizer_spec.rb @@ -1,11 +1,7 @@ require 'spec_helper' describe Gitlab::UrlSanitizer do - let(:credentials) { { user: 'blah', password: 'password' } } - let(:url_sanitizer) do - described_class.new("https://github.com/me/project.git", credentials: credentials) - end - let(:user) { double(:user, username: 'john.doe') } + using RSpec::Parameterized::TableSyntax describe '.sanitize' do def sanitize_url(url) @@ -16,83 +12,166 @@ describe Gitlab::UrlSanitizer do }) end - it 'mask the credentials from HTTP URLs' do - filtered_content = sanitize_url('http://user:pass@test.com/root/repoC.git/') + where(:input, :output) do + 'http://user:pass@test.com/root/repoC.git/' | 'http://*****:*****@test.com/root/repoC.git/' + 'https://user:pass@test.com/root/repoA.git/' | 'https://*****:*****@test.com/root/repoA.git/' + 'ssh://user@host.test/path/to/repo.git' | 'ssh://*****@host.test/path/to/repo.git' - expect(filtered_content).to include("http://*****:*****@test.com/root/repoC.git/") - end + # git protocol does not support authentication but clean any details anyway + 'git://user:pass@host.test/path/to/repo.git' | 'git://*****:*****@host.test/path/to/repo.git' + 'git://host.test/path/to/repo.git' | 'git://host.test/path/to/repo.git' - it 'mask the credentials from HTTPS URLs' do - filtered_content = sanitize_url('https://user:pass@test.com/root/repoA.git/') + # SCP-style URLs are left unmodified + 'user@server:project.git' | 'user@server:project.git' + 'user:pass@server:project.git' | 'user:pass@server:project.git' - expect(filtered_content).to include("https://*****:*****@test.com/root/repoA.git/") + # return an empty string for invalid URLs + 'ssh://' | '' end - it 'mask credentials from SSH URLs' do - filtered_content = sanitize_url('ssh://user@host.test/path/to/repo.git') - - expect(filtered_content).to include("ssh://*****@host.test/path/to/repo.git") + with_them do + it { expect(sanitize_url(input)).to include("repository '#{output}' not found") } end + end - it 'does not modify Git URLs' do - # git protocol does not support authentication - filtered_content = sanitize_url('git://host.test/path/to/repo.git') + describe '.valid?' do + where(:value, :url) do + false | nil + false | '' + false | '123://invalid:url' + true | 'valid@project:url.git' + true | 'ssh://example.com' + true | 'ssh://:@example.com' + true | 'ssh://foo@example.com' + true | 'ssh://foo:bar@example.com' + true | 'ssh://foo:bar@example.com/group/group/project.git' + true | 'git://example.com/group/group/project.git' + true | 'git://foo:bar@example.com/group/group/project.git' + true | 'http://foo:bar@example.com/group/group/project.git' + true | 'https://foo:bar@example.com/group/group/project.git' + end - expect(filtered_content).to include("git://host.test/path/to/repo.git") + with_them do + it { expect(described_class.valid?(url)).to eq(value) } end + end + + describe '#sanitized_url' do + context 'credentials in hash' do + where(username: ['foo', '', nil], password: ['bar', '', nil]) - it 'does not modify scp-like URLs' do - filtered_content = sanitize_url('user@server:project.git') + with_them do + let(:credentials) { { user: username, password: password } } + subject { described_class.new('http://example.com', credentials: credentials).sanitized_url } - expect(filtered_content).to include("user@server:project.git") + it { is_expected.to eq('http://example.com') } + end end - it 'returns an empty string for invalid URLs' do - filtered_content = sanitize_url('ssh://') + context 'credentials in URL' do + where(userinfo: %w[foo:bar@ foo@ :bar@ :@ @] + [nil]) - expect(filtered_content).to include("repository '' not found") - end - end + with_them do + subject { described_class.new("http://#{userinfo}example.com").sanitized_url } - describe '.valid?' do - it 'validates url strings' do - expect(described_class.valid?(nil)).to be(false) - expect(described_class.valid?('valid@project:url.git')).to be(true) - expect(described_class.valid?('123://invalid:url')).to be(false) + it { is_expected.to eq('http://example.com') } + end end end - describe '#sanitized_url' do - it { expect(url_sanitizer.sanitized_url).to eq("https://github.com/me/project.git") } - end - describe '#credentials' do - it { expect(url_sanitizer.credentials).to eq(credentials) } + context 'credentials in hash' do + where(:input, :output) do + { user: 'foo', password: 'bar' } | { user: 'foo', password: 'bar' } + { user: 'foo', password: '' } | { user: 'foo', password: nil } + { user: 'foo', password: nil } | { user: 'foo', password: nil } + { user: '', password: 'bar' } | { user: nil, password: 'bar' } + { user: '', password: '' } | { user: nil, password: nil } + { user: '', password: nil } | { user: nil, password: nil } + { user: nil, password: 'bar' } | { user: nil, password: 'bar' } + { user: nil, password: '' } | { user: nil, password: nil } + { user: nil, password: nil } | { user: nil, password: nil } + end - context 'when user is given to #initialize' do - let(:url_sanitizer) do - described_class.new("https://github.com/me/project.git", credentials: { user: user.username }) + with_them do + subject { described_class.new('user@example.com:path.git', credentials: input).credentials } + + it { is_expected.to eq(output) } end - it { expect(url_sanitizer.credentials).to eq({ user: 'john.doe' }) } + it 'overrides URL-provided credentials' do + sanitizer = described_class.new('http://a:b@example.com', credentials: { user: 'c', password: 'd' }) + + expect(sanitizer.credentials).to eq(user: 'c', password: 'd') + end + end + + context 'credentials in URL' do + where(:url, :credentials) do + 'http://foo:bar@example.com' | { user: 'foo', password: 'bar' } + 'http://:bar@example.com' | { user: nil, password: 'bar' } + 'http://foo:@example.com' | { user: 'foo', password: nil } + 'http://foo@example.com' | { user: 'foo', password: nil } + 'http://:@example.com' | { user: nil, password: nil } + 'http://@example.com' | { user: nil, password: nil } + 'http://example.com' | { user: nil, password: nil } + + # Credentials from SCP-style URLs are not supported at present + 'foo@example.com:path' | { user: nil, password: nil } + 'foo:bar@example.com:path' | { user: nil, password: nil } + + # Other invalid URLs + nil | { user: nil, password: nil } + '' | { user: nil, password: nil } + 'no' | { user: nil, password: nil } + end + + with_them do + subject { described_class.new(url).credentials } + + it { is_expected.to eq(credentials) } + end end end describe '#full_url' do - it { expect(url_sanitizer.full_url).to eq("https://blah:password@github.com/me/project.git") } + context 'credentials in hash' do + where(:credentials, :userinfo) do + { user: 'foo', password: 'bar' } | 'foo:bar@' + { user: 'foo', password: '' } | 'foo@' + { user: 'foo', password: nil } | 'foo@' + { user: '', password: 'bar' } | ':bar@' + { user: '', password: '' } | nil + { user: '', password: nil } | nil + { user: nil, password: 'bar' } | ':bar@' + { user: nil, password: '' } | nil + { user: nil, password: nil } | nil + end - it 'supports scp-like URLs' do - sanitizer = described_class.new('user@server:project.git') + with_them do + subject { described_class.new('http://example.com', credentials: credentials).full_url } - expect(sanitizer.full_url).to eq('user@server:project.git') + it { is_expected.to eq("http://#{userinfo}example.com") } + end end - context 'when user is given to #initialize' do - let(:url_sanitizer) do - described_class.new("https://github.com/me/project.git", credentials: { user: user.username }) + context 'credentials in URL' do + where(:input, :output) do + nil | '' + '' | :same + 'git@example.com' | :same + 'http://example.com' | :same + 'http://foo@example.com' | :same + 'http://foo:@example.com' | 'http://foo@example.com' + 'http://:bar@example.com' | :same + 'http://foo:bar@example.com' | :same end - it { expect(url_sanitizer.full_url).to eq("https://john.doe@github.com/me/project.git") } + with_them do + let(:expected) { output == :same ? input : output } + + it { expect(described_class.new(input).full_url).to eq(expected) } + end end end end |