Hide password on import by url form

11 files changed, 160 insertions, 14 deletions

diff --git a/app/controllers/concerns/import_url_params.rb b/app/controllers/concerns/import_url_params.rb
new file mode 100644
index 00000000000..d9070e51573
--- /dev/null
+++ b/app/controllers/concerns/import_url_params.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+module ImportUrlParams
+  def import_url_params
+    import_params =
+      params
+        .require(:project)
+        .permit(:import_url, :import_url_user, :import_url_password)
+
+    { import_url: import_params_to_full_url(import_params) }
+  end
+
+  def import_params_to_full_url(params)
+    Gitlab::UrlSanitizer.new(
+      params[:import_url],
+      credentials: {
+        user: params[:import_url_user],
+        password: params[:import_url_password]
+      }
+    ).full_url
+  end
+end
diff --git a/app/controllers/projects/imports_controller.rb b/app/controllers/projects/imports_controller.rb
index 4640be015de..25a137eeb38 100644
--- a/app/controllers/projects/imports_controller.rb
+++ b/app/controllers/projects/imports_controller.rb
@@ -2,6 +2,7 @@
 
 class Projects::ImportsController < Projects::ApplicationController
   include ContinueParams
+  include ImportUrlParams
 
   # Authorize
   before_action :authorize_admin_project!
@@ -13,7 +14,7 @@ class Projects::ImportsController < Projects::ApplicationController
   end
 
   def create
-    if @project.update(import_params)
+    if @project.update(import_url_params)
       @project.import_state.reset.schedule
     end
 
@@ -65,12 +66,4 @@ class Projects::ImportsController < Projects::ApplicationController
       redirect_to project_path(@project)
     end
   end
-
-  def import_params_attributes
-    [:import_url]
-  end
-
-  def import_params
-    params.require(:project).permit(import_params_attributes)
-  end
 end
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index e88c46144ef..12db493978b 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -7,6 +7,7 @@ class ProjectsController < Projects::ApplicationController
   include PreviewMarkdown
   include SendFileUpload
   include RecordUserLastActivity
+  include ImportUrlParams
 
   prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) }
 
@@ -333,6 +334,7 @@ class ProjectsController < Projects::ApplicationController
   def project_params(attributes: [])
     params.require(:project)
       .permit(project_params_attributes + attributes)
+      .merge(import_url_params)
   end
 
   def project_params_attributes
diff --git a/app/views/shared/_import_form.html.haml b/app/views/shared/_import_form.html.haml
index 7b593ca4f76..98a8520084a 100644
--- a/app/views/shared/_import_form.html.haml
+++ b/app/views/shared/_import_form.html.haml
@@ -1,11 +1,32 @@
 - ci_cd_only = local_assigns.fetch(:ci_cd_only, false)
+- import_url = Gitlab::UrlSanitizer.new(f.object.import_url)
 
 .form-group.import-url-data
-  = f.label :import_url, class: 'label-bold' do
-    %span
-      = _('Git repository URL')
+  .form-group
+    = f.label :import_url, class: 'label-bold' do
+      %span
+        = _('Git repository URL')
+    = f.text_field :import_url, value: import_url.sanitized_url,
+        autocomplete: 'off', class: 'form-control', placeholder: 'https://gitlab.company.com/group/project.git', required: true
 
-  = f.text_field :import_url, autocomplete: 'off', class: 'form-control', placeholder: 'https://username:password@gitlab.company.com/group/project.git', required: true
+  .form-group#import_url_auth_method
+    = label :import_url_auth_method, class: 'label-bold' do
+      %span
+        = _('Authentication method')
+    = select_tag :import_url_auth_method, options_for_select([[_('None'), 'none'], [_('Username and Password'), 'username-and-password']]), class: 'form-control'
+
+  .div#import_url_auth_group{ style: 'display: none' }
+    .form-group
+      = f.label :import_url_user, class: 'label-bold' do
+        %span
+          = _('Username (optional)')
+      = f.text_field :import_url_user, value: import_url.user, class: 'form-control', required: false, autocomplete: 'new-password'
+
+    .form-group
+      = f.label :import_url_password, class: 'label-bold' do
+        %span
+          = _('Git repository password')
+      = f.password_field :import_url_password, class: 'form-control', required: false, autocomplete: 'new-password', placeholder: 'Basic Auth Password'
 
   .info-well.prepend-top-20
     .well-segment
diff --git a/app/workers/concerns/project_import_options.rb b/app/workers/concerns/project_import_options.rb
index 2baf768bfd1..53b0459c48c 100644
--- a/app/workers/concerns/project_import_options.rb
+++ b/app/workers/concerns/project_import_options.rb
@@ -3,7 +3,7 @@
 module ProjectImportOptions
   extend ActiveSupport::Concern
 
-  IMPORT_RETRY_COUNT = 5
+  IMPORT_RETRY_COUNT = 0
 
   included do
     sidekiq_options retry: IMPORT_RETRY_COUNT, status_expiration: StuckImportJobsWorker::IMPORT_JOBS_EXPIRATION
diff --git a/changelogs/unreleased/security-id-leaked-password-in-import-url-frontend.yml b/changelogs/unreleased/security-id-leaked-password-in-import-url-frontend.yml
new file mode 100644
index 00000000000..df636ec37fb
--- /dev/null
+++ b/changelogs/unreleased/security-id-leaked-password-in-import-url-frontend.yml
@@ -0,0 +1,5 @@
+---
+title: Add extra fields for handling basic auth on import by url page
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb
index 880712de5fe..215454fe63c 100644
--- a/lib/gitlab/url_sanitizer.rb
+++ b/lib/gitlab/url_sanitizer.rb
@@ -47,6 +47,10 @@ module Gitlab
       @credentials ||= { user: @url.user.presence, password: @url.password.presence }
     end
 
+    def user
+      credentials[:user]
+    end
+
     def full_url
       @full_url ||= generate_full_url.to_s
     end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index b67818768e3..952e0566f26 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -6641,6 +6641,9 @@ msgstr ""
 msgid "Password"
 msgstr ""
 
+msgid "Password (optional)"
+msgstr ""
+
 msgid "Password authentication is unavailable."
 msgstr ""
 
@@ -10574,6 +10577,9 @@ msgstr ""
 msgid "UserProfile|Your projects can be available publicly, internally, or privately, at your choice."
 msgstr ""
 
+msgid "Username and Password"
+msgstr ""
+
 msgid "Users"
 msgstr ""
 
diff --git a/spec/controllers/concerns/import_url_params_spec.rb b/spec/controllers/concerns/import_url_params_spec.rb
new file mode 100644
index 00000000000..fc5dfb5263f
--- /dev/null
+++ b/spec/controllers/concerns/import_url_params_spec.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ImportUrlParams do
+  let(:import_url_params) do
+    controller = OpenStruct.new(params: params).extend(described_class)
+    controller.import_url_params
+  end
+
+  context 'url and password separately provided' do
+    let(:params) do
+      ActionController::Parameters.new(project: {
+        import_url: 'https://url.com',
+        import_url_user: 'user', import_url_password: 'password'
+      })
+    end
+
+    describe '#import_url_params' do
+      it 'returns hash with import_url' do
+        expect(import_url_params).to eq(
+          import_url: "https://user:password@url.com"
+        )
+      end
+    end
+  end
+
+  context 'url with provided empty credentials' do
+    let(:params) do
+      ActionController::Parameters.new(project: {
+        import_url: 'https://user:password@url.com',
+        import_url_user: '', import_url_password: ''
+      })
+    end
+
+    describe '#import_url_params' do
+      it 'does not change the url' do
+        expect(import_url_params).to eq(
+          import_url: "https://user:password@url.com"
+        )
+      end
+    end
+  end
+end
diff --git a/spec/controllers/projects/imports_controller_spec.rb b/spec/controllers/projects/imports_controller_spec.rb
index 8d88ee7dfd6..bdc81efe3bc 100644
--- a/spec/controllers/projects/imports_controller_spec.rb
+++ b/spec/controllers/projects/imports_controller_spec.rb
@@ -122,4 +122,19 @@ describe Projects::ImportsController do
       end
     end
   end
+
+  describe 'POST #create' do
+    let(:params) { { import_url: 'https://github.com/vim/vim.git', import_url_user: 'user', import_url_password: 'password' } }
+    let(:project) { create(:project) }
+
+    before do
+      allow(RepositoryImportWorker).to receive(:perform_async)
+
+      post :create, params: { project: params, namespace_id: project.namespace.to_param, project_id: project }
+    end
+
+    it 'sets import_url to the project' do
+      expect(project.reload.import_url).to eq('https://user:password@github.com/vim/vim.git')
+    end
+  end
 end
diff --git a/spec/lib/gitlab/url_sanitizer_spec.rb b/spec/lib/gitlab/url_sanitizer_spec.rb
index 5861e6955a6..7242255d535 100644
--- a/spec/lib/gitlab/url_sanitizer_spec.rb
+++ b/spec/lib/gitlab/url_sanitizer_spec.rb
@@ -115,6 +115,40 @@ describe Gitlab::UrlSanitizer do
     end
   end
 
+  describe '#user' do
+    context 'credentials in hash' do
+      it 'overrides URL-provided user' do
+        sanitizer = described_class.new('http://a:b@example.com', credentials: { user: 'c', password: 'd' })
+
+        expect(sanitizer.user).to eq('c')
+      end
+    end
+
+    context 'credentials in URL' do
+      where(:url, :user) do
+        'http://foo:bar@example.com' | 'foo'
+        'http://foo:bar:baz@example.com' | 'foo'
+        'http://:bar@example.com'    | nil
+        'http://foo:@example.com'    | 'foo'
+        'http://foo@example.com'     | 'foo'
+        'http://:@example.com'       | nil
+        'http://@example.com'        | nil
+        'http://example.com'         | nil
+
+        # Other invalid URLs
+        nil  | nil
+        ''   | nil
+        'no' | nil
+      end
+
+      with_them do
+        subject { described_class.new(url).user }
+
+        it { is_expected.to eq(user) }
+      end
+    end
+  end
+
   describe '#full_url' do
     context 'credentials in hash' do
       where(:credentials, :userinfo) do