Merge branch 'issue_17560' into 'master'
Mask credentials from URL when the import of project has failed. REF: #17560 See merge request !4185
This commit is contained in:
commit
4607323e13
7 changed files with 107 additions and 33 deletions
|
@ -369,14 +369,14 @@ class Project < ActiveRecord::Base
|
||||||
end
|
end
|
||||||
|
|
||||||
def import_url=(value)
|
def import_url=(value)
|
||||||
import_url = Gitlab::ImportUrl.new(value)
|
import_url = Gitlab::UrlSanitizer.new(value)
|
||||||
create_or_update_import_data(credentials: import_url.credentials)
|
create_or_update_import_data(credentials: import_url.credentials)
|
||||||
super(import_url.sanitized_url)
|
super(import_url.sanitized_url)
|
||||||
end
|
end
|
||||||
|
|
||||||
def import_url
|
def import_url
|
||||||
if import_data && super
|
if import_data && super
|
||||||
import_url = Gitlab::ImportUrl.new(super, credentials: import_data.credentials)
|
import_url = Gitlab::UrlSanitizer.new(super, credentials: import_data.credentials)
|
||||||
import_url.full_url
|
import_url.full_url
|
||||||
else
|
else
|
||||||
super
|
super
|
||||||
|
|
|
@ -13,7 +13,7 @@ class RepositoryImportWorker
|
||||||
result = Projects::ImportService.new(project, current_user).execute
|
result = Projects::ImportService.new(project, current_user).execute
|
||||||
|
|
||||||
if result[:status] == :error
|
if result[:status] == :error
|
||||||
project.update(import_error: result[:message])
|
project.update(import_error: Gitlab::UrlSanitizer.sanitize(result[:message]))
|
||||||
project.import_fail
|
project.import_fail
|
||||||
return
|
return
|
||||||
end
|
end
|
||||||
|
|
|
@ -24,7 +24,7 @@ class RemoveWrongImportUrlFromProjects < ActiveRecord::Migration
|
||||||
def process_projects_with_wrong_url
|
def process_projects_with_wrong_url
|
||||||
projects_with_wrong_import_url.each do |project|
|
projects_with_wrong_import_url.each do |project|
|
||||||
begin
|
begin
|
||||||
import_url = Gitlab::ImportUrl.new(project["import_url"])
|
import_url = Gitlab::UrlSanitizer.new(project["import_url"])
|
||||||
|
|
||||||
update_import_url(import_url, project)
|
update_import_url(import_url, project)
|
||||||
update_import_data(import_url, project)
|
update_import_data(import_url, project)
|
||||||
|
|
|
@ -1,7 +1,13 @@
|
||||||
module Gitlab
|
module Gitlab
|
||||||
class ImportUrl
|
class UrlSanitizer
|
||||||
|
def self.sanitize(content)
|
||||||
|
regexp = URI::Parser.new.make_regexp(['http', 'https', 'ssh', 'git'])
|
||||||
|
|
||||||
|
content.gsub(regexp) { |url| new(url).masked_url }
|
||||||
|
end
|
||||||
|
|
||||||
def initialize(url, credentials: nil)
|
def initialize(url, credentials: nil)
|
||||||
@url = URI.parse(URI.encode(url))
|
@url = Addressable::URI.parse(URI.encode(url))
|
||||||
@credentials = credentials
|
@credentials = credentials
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -9,6 +15,13 @@ module Gitlab
|
||||||
@sanitized_url ||= safe_url.to_s
|
@sanitized_url ||= safe_url.to_s
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def masked_url
|
||||||
|
url = @url.dup
|
||||||
|
url.password = "*****" unless url.password.nil?
|
||||||
|
url.user = "*****" unless url.user.nil?
|
||||||
|
url.to_s
|
||||||
|
end
|
||||||
|
|
||||||
def credentials
|
def credentials
|
||||||
@credentials ||= { user: @url.user, password: @url.password }
|
@credentials ||= { user: @url.user, password: @url.password }
|
||||||
end
|
end
|
|
@ -1,21 +0,0 @@
|
||||||
require 'spec_helper'
|
|
||||||
|
|
||||||
describe Gitlab::ImportUrl do
|
|
||||||
|
|
||||||
let(:credentials) { { user: 'blah', password: 'password' } }
|
|
||||||
let(:import_url) do
|
|
||||||
Gitlab::ImportUrl.new("https://github.com/me/project.git", credentials: credentials)
|
|
||||||
end
|
|
||||||
|
|
||||||
describe :full_url do
|
|
||||||
it { expect(import_url.full_url).to eq("https://blah:password@github.com/me/project.git") }
|
|
||||||
end
|
|
||||||
|
|
||||||
describe :sanitized_url do
|
|
||||||
it { expect(import_url.sanitized_url).to eq("https://github.com/me/project.git") }
|
|
||||||
end
|
|
||||||
|
|
||||||
describe :credentials do
|
|
||||||
it { expect(import_url.credentials).to eq(credentials) }
|
|
||||||
end
|
|
||||||
end
|
|
68
spec/lib/gitlab/url_sanitizer_spec.rb
Normal file
68
spec/lib/gitlab/url_sanitizer_spec.rb
Normal file
|
@ -0,0 +1,68 @@
|
||||||
|
require 'spec_helper'
|
||||||
|
|
||||||
|
describe Gitlab::UrlSanitizer, lib: true do
|
||||||
|
let(:credentials) { { user: 'blah', password: 'password' } }
|
||||||
|
let(:url_sanitizer) do
|
||||||
|
described_class.new("https://github.com/me/project.git", credentials: credentials)
|
||||||
|
end
|
||||||
|
|
||||||
|
describe '.sanitize' do
|
||||||
|
def sanitize_url(url)
|
||||||
|
# We want to try with multi-line content because is how error messages are formatted
|
||||||
|
described_class.sanitize(%Q{
|
||||||
|
remote: Not Found
|
||||||
|
fatal: repository '#{url}' not found
|
||||||
|
})
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'mask the credentials from HTTP URLs' do
|
||||||
|
filtered_content = sanitize_url('http://user:pass@test.com/root/repoC.git/')
|
||||||
|
|
||||||
|
expect(filtered_content).to include("http://*****:*****@test.com/root/repoC.git/")
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'mask the credentials from HTTPS URLs' do
|
||||||
|
filtered_content = sanitize_url('https://user:pass@test.com/root/repoA.git/')
|
||||||
|
|
||||||
|
expect(filtered_content).to include("https://*****:*****@test.com/root/repoA.git/")
|
||||||
|
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")
|
||||||
|
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')
|
||||||
|
|
||||||
|
expect(filtered_content).to include("git://host.test/path/to/repo.git")
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'does not modify scp-like URLs' do
|
||||||
|
filtered_content = sanitize_url('user@server:project.git')
|
||||||
|
|
||||||
|
expect(filtered_content).to include("user@server:project.git")
|
||||||
|
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) }
|
||||||
|
end
|
||||||
|
|
||||||
|
describe '#full_url' do
|
||||||
|
it { expect(url_sanitizer.full_url).to eq("https://blah:password@github.com/me/project.git") }
|
||||||
|
|
||||||
|
it 'supports scp-like URLs' do
|
||||||
|
sanitizer = described_class.new('user@server:project.git')
|
||||||
|
|
||||||
|
expect(sanitizer.full_url).to eq('user@server:project.git')
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
end
|
|
@ -6,6 +6,7 @@ describe RepositoryImportWorker do
|
||||||
subject { described_class.new }
|
subject { described_class.new }
|
||||||
|
|
||||||
describe '#perform' do
|
describe '#perform' do
|
||||||
|
context 'when the import was successful' do
|
||||||
it 'imports a project' do
|
it 'imports a project' do
|
||||||
expect_any_instance_of(Projects::ImportService).to receive(:execute).
|
expect_any_instance_of(Projects::ImportService).to receive(:execute).
|
||||||
and_return({ status: :ok })
|
and_return({ status: :ok })
|
||||||
|
@ -16,4 +17,17 @@ describe RepositoryImportWorker do
|
||||||
subject.perform(project.id)
|
subject.perform(project.id)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context 'when the import has failed' do
|
||||||
|
it 'hide the credentials that were used in the import URL' do
|
||||||
|
error = %Q{remote: Not Found fatal: repository 'https://user:pass@test.com/root/repoC.git/' not found }
|
||||||
|
expect_any_instance_of(Projects::ImportService).to receive(:execute).
|
||||||
|
and_return({ status: :error, message: error })
|
||||||
|
|
||||||
|
subject.perform(project.id)
|
||||||
|
|
||||||
|
expect(project.reload.import_error).to include("https://*****:*****@test.com/root/repoC.git/")
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in a new issue