Merge branch 'url-sanitizer-fixes' into 'master'

Fix problems sanitizing URLs with empty passwords

Closes gitlab-ee#3352

See merge request !14083

changelogs/unreleased/url-sanitizer-fixes.yml

+---
+title: Fix problems sanitizing URLs with empty passwords
+merge_request: 14083
+author:
+type: fixed

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
 

spec/lib/gitlab/url_sanitizer_spec.rb

 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
 
-    it 'does not modify scp-like URLs' do
-      filtered_content = sanitize_url('user@server:project.git')
+  describe '#sanitized_url' do
+    context 'credentials in hash' do
+      where(username: ['foo', '', nil], password: ['bar', '', nil])
+
+      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
+
+    context 'credentials in URL' do
+      where(userinfo: %w[foo:bar@ foo@ :bar@ :@ @] + [nil])
 
-    it 'returns an empty string for invalid URLs' do
-      filtered_content = sanitize_url('ssh://')
+      with_them do
+        subject { described_class.new("http://#{userinfo}example.com").sanitized_url }
 
-      expect(filtered_content).to include("repository '' not found")
+        it { is_expected.to eq('http://example.com') }
+      end
     end
   end
 
-  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)
+  describe '#credentials' do
+    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
+
+      with_them do
+        subject { described_class.new('user@example.com:path.git', credentials: input).credentials }
+
+        it { is_expected.to eq(output) }
       end
 
-  describe '#sanitized_url' do
-    it { expect(url_sanitizer.sanitized_url).to eq("https://github.com/me/project.git") }
+      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
 
-  describe '#credentials' do
-    it { expect(url_sanitizer.credentials).to eq(credentials) }
+    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 }
 
-    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 })
+        # Other invalid URLs
+        nil  | { user: nil, password: nil }
+        ''   | { user: nil, password: nil }
+        'no' | { user: nil, password: nil }
       end
 
-      it { expect(url_sanitizer.credentials).to eq({ user: 'john.doe' }) }
+      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