diff --git a/app/controllers/concerns/import_url_params.rb b/app/controllers/concerns/import_url_params.rb new file mode 100644 index 0000000000000000000000000000000000000000..d9070e51573a55a0138a91425a876fd6ffadb806 --- /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 4640be015dec6a71d6355ff0955a0f0ff3b47539..25a137eeb38a17ac9bd2cfa71f17d88bfff246d3 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 e88c46144ef29121d2615418426c5439e1cd5757..12db493978b00810142ce5fb2d72c87e6455c846 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 7b593ca4f76eaca09614d893a5419ea82d428443..8a7bc78b5691875c09b73960217b72c8a94755a1 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 http://, https:// or git://.').html_safe %li - = _('If your HTTP repository is not publicly accessible, add authentication information to the URL: https://username:password@gitlab.company.com/group/project.git.').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 0000000000000000000000000000000000000000..df636ec37fb8208a726991d87e02380a56394c05 --- /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 880712de5fe7620480d471714405d23d4f44d75b..215454fe63cd1974722c599185d72cbfbad73ec7 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 b67818768e39209a8bc0ae5bffd55aa46a91e690..49ac8e5c63880b4b12b82165d91af034371a056e 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: https://username:password@gitlab.company.com/group/project.git." +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 0000000000000000000000000000000000000000..fc5dfb5263f7ad84c44e2bf901261cc70bfade24 --- /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 8d88ee7dfd6440f5ee68d7400ec0d343d7c4b2dc..bdc81efe3bc7cf4db6c0b9d66512e9c2d605ed45 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 b61e0ac872fbf054ec413f7d7b30dd731ff86af3..106a3ba94e47bff7bbcaf2d4245e40c9e508b8a6 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(`
- +
+ +
+
+
+ +
+
+ +
+
@@ -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 5861e6955a6140245ae794d0c57a1f60a03c29a3..7242255d5351f2ecb02a1d167811445deda2f711 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