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

Handling password on import by url page

See merge request gitlab/gitlabhq!3061

app/controllers/concerns/import_url_params.rb

+# frozen_string_literal: true
+
+module ImportUrlParams
+  def import_url_params
+    { import_url: import_params_to_full_url(params[:project]) }
+  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

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!
@@ -67,10 +68,12 @@ class Projects::ImportsController < Projects::ApplicationController
   end
 
   def import_params_attributes
-    [:import_url]
+    []
   end
 
   def import_params
-    params.require(:project).permit(import_params_attributes)
+    params.require(:project)
+      .permit(import_params_attributes)
+      .merge(import_url_params)
   end
 end

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

app/views/shared/_import_form.html.haml

 - 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

changelogs/unreleased/security-id-leaked-password-in-import-url-frontend.yml

+---
+title: Add extra fields for handling basic auth on import by url page
+merge_request:
+author:
+type: security

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

locale/gitlab.pot

@@ -5051,7 +5051,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"
@@ -6790,6 +6790,9 @@ msgstr ""
 msgid "Password"
 msgstr ""
 
+msgid "Password (optional)"
+msgstr ""
+
 msgid "Password authentication is unavailable."
 msgstr ""
 
@@ -10831,6 +10834,9 @@ msgstr ""
 msgid "UserProfile|Your projects can be available publicly, internally, or privately, at your choice."
 msgstr ""
 
+msgid "Username (optional)"
+msgstr ""
+
 msgid "Username is already taken."
 msgstr ""
 

spec/controllers/concerns/import_url_params_spec.rb

+# 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

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

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);
 

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