From c8bdb20228b34130c7f0525ad92140702dce1e20 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Wed, 6 Sep 2017 15:20:25 +0100 Subject: Remove blank passwords from sanitized URLs --- changelogs/unreleased/url-sanitizer-fixes.yml | 5 + lib/gitlab/url_sanitizer.rb | 13 +- spec/lib/gitlab/url_sanitizer_spec.rb | 180 ++++++++++++++++++-------- 3 files changed, 144 insertions(+), 54 deletions(-) create mode 100644 changelogs/unreleased/url-sanitizer-fixes.yml 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 9c26490f40f..703adae12cb 100644 --- a/lib/gitlab/url_sanitizer.rb +++ b/lib/gitlab/url_sanitizer.rb @@ -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 @@ -47,8 +52,10 @@ module Gitlab def generate_full_url return @url unless valid_credentials? @full_url = @url.dup - @full_url.user = credentials[:user].presence - @full_url.password = credentials[:password].presence + + @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 c1dc39b38de..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,84 +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?('')).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 -- cgit v1.2.1