Merge branch 'security-id-leaked-password-in-import-url-frontend-11-11' into '11-11-stable'

Handling password on import by url page

See merge request gitlab/gitlabhq!3109

11 files changed, 168 insertions, 18 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..8a7bc78b569 100644
--- a/app/views/shared/_import_form.html.haml
+++ b/app/views/shared/_import_form.html.haml
@@ -1,11 +1,26 @@
 - 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')
+.import-url-data
+  .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
+  .row
+    .form-group.col-md-6
+      = 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.col-md-6
+      = f.label :import_url_password, class: 'label-bold' do
+        %span
+          = _('Password (optional)')
+      = f.password_field :import_url_password, class: 'form-control', required: false, autocomplete: 'new-password'
 
   .info-well.prepend-top-20
     .well-segment
@@ -13,7 +28,7 @@
         %li
           = _('The repository must be accessible over <code>http://</code>, <code>https://</code> or <code>git://</code>.').html_safe
         %li
-          = _('If your HTTP repository is not publicly accessible, add authentication information to the URL: <code>https://username:password@gitlab.company.com/group/project.git</code>.').html_safe
+          = _('If your HTTP repository is not publicly accessible, add your credentials.')
         %li
           = import_will_timeout_message(ci_cd_only)
         %li
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..49ac8e5c638 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -4941,7 +4941,7 @@ msgstr ""
 msgid "If this was a mistake you can leave the %{source_type}."
 msgstr ""
 
-msgid "If your HTTP repository is not publicly accessible, add authentication information to the URL: <code>https://username:password@gitlab.company.com/group/project.git</code>."
+msgid "If your HTTP repository is not publicly accessible, add your credentials."
 msgstr ""
 
 msgid "ImageDiffViewer|2-up"
@@ -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 (optional)"
+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/javascripts/projects/project_new_spec.js b/spec/javascripts/projects/project_new_spec.js
index b61e0ac872f..106a3ba94e4 100644
--- a/spec/javascripts/projects/project_new_spec.js
+++ b/spec/javascripts/projects/project_new_spec.js
@@ -10,7 +10,17 @@ describe('New Project', () => {
     setFixtures(`
       <div class='toggle-import-form'>
         <div class='import-url-data'>
-          <input id="project_import_url" />
+          <div class="form-group">
+            <input id="project_import_url" />
+          </div>
+          <div id="import-url-auth-method">
+            <div class="form-group">
+              <input id="project-import-url-user" />
+            </div>
+            <div class="form-group">
+              <input id="project_import_url_password" />
+            </div>
+          </div>
           <input id="project_name" />
           <input id="project_path" />
         </div>
@@ -119,7 +129,7 @@ describe('New Project', () => {
       });
 
       it('changes project path for HTTPS URL in $projectImportUrl', () => {
-        $projectImportUrl.val('https://username:password@gitlab.company.com/group/project.git');
+        $projectImportUrl.val('https://gitlab.company.com/group/project.git');
 
         projectNew.deriveProjectPathFromUrl($projectImportUrl);
 
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