diff options
20 files changed, 58 insertions, 149 deletions
diff --git a/app/assets/javascripts/boards/components/board_sidebar.js b/app/assets/javascripts/boards/components/board_sidebar.js index 983429550f0..add24303e7b 100644 --- a/app/assets/javascripts/boards/components/board_sidebar.js +++ b/app/assets/javascripts/boards/components/board_sidebar.js @@ -2,6 +2,7 @@ import Vue from 'vue'; import Flash from '../../flash'; +import { __ } from '../../locale'; import Sidebar from '../../right_sidebar'; import eventHub from '../../sidebar/event_hub'; import assigneeTitle from '../../sidebar/components/assignees/assignee_title'; @@ -95,7 +96,7 @@ gl.issueBoards.BoardSidebar = Vue.extend({ }) .catch(() => { this.loadingAssignees = false; - return new Flash('An error occurred while saving assignees'); + Flash(__('An error occurred while saving assignees')); }); }, }, diff --git a/app/assets/javascripts/boards/components/modal/footer.js b/app/assets/javascripts/boards/components/modal/footer.js index 182957113a2..03cd7ef65cb 100644 --- a/app/assets/javascripts/boards/components/modal/footer.js +++ b/app/assets/javascripts/boards/components/modal/footer.js @@ -1,7 +1,6 @@ -/* eslint-disable no-new */ - import Vue from 'vue'; import Flash from '../../../flash'; +import { __ } from '../../../locale'; import './lists_dropdown'; import { pluralize } from '../../../lib/utils/text_utility'; @@ -36,7 +35,7 @@ gl.issueBoards.ModalFooter = Vue.extend({ gl.boardService.bulkUpdate(issueIds, { add_label_ids: [list.label.id], }).catch(() => { - new Flash('Failed to update issues, please try again.', 'alert'); + Flash(__('Failed to update issues, please try again.')); selectedIssues.forEach((issue) => { list.removeIssue(issue); diff --git a/app/assets/javascripts/boards/components/sidebar/remove_issue.js b/app/assets/javascripts/boards/components/sidebar/remove_issue.js index 1ad97211934..0ae32bb4d0a 100644 --- a/app/assets/javascripts/boards/components/sidebar/remove_issue.js +++ b/app/assets/javascripts/boards/components/sidebar/remove_issue.js @@ -1,7 +1,6 @@ -/* eslint-disable no-new */ - import Vue from 'vue'; import Flash from '../../../flash'; +import { __ } from '../../../locale'; const Store = gl.issueBoards.BoardsStore; @@ -45,7 +44,7 @@ gl.issueBoards.RemoveIssueBtn = Vue.extend({ }, }; Vue.http.patch(this.updateUrl, data).catch(() => { - new Flash('Failed to remove issue from board, please try again.', 'alert'); + Flash(__('Failed to remove issue from board, please try again.')); lists.forEach((list) => { list.addIssue(issue); diff --git a/app/assets/javascripts/groups_select.js b/app/assets/javascripts/groups_select.js index a69a0bde17b..65a2395fe29 100644 --- a/app/assets/javascripts/groups_select.js +++ b/app/assets/javascripts/groups_select.js @@ -1,5 +1,6 @@ +import axios from './lib/utils/axios_utils'; import Api from './api'; -import { normalizeCRLFHeaders } from './lib/utils/common_utils'; +import { normalizeHeaders } from './lib/utils/common_utils'; export default function groupsSelect() { // Needs to be accessible in rspec @@ -17,24 +18,23 @@ export default function groupsSelect() { dataType: 'json', quietMillis: 250, transport(params) { - return $.ajax(params) - .then((data, status, xhr) => { - const results = data || []; - - const headers = normalizeCRLFHeaders(xhr.getAllResponseHeaders()); + axios[params.type.toLowerCase()](params.url, { + params: params.data, + }) + .then((res) => { + const results = res.data || []; + const headers = normalizeHeaders(res.headers); const currentPage = parseInt(headers['X-PAGE'], 10) || 0; const totalPages = parseInt(headers['X-TOTAL-PAGES'], 10) || 0; const more = currentPage < totalPages; - return { + params.success({ results, pagination: { more, }, - }; - }) - .then(params.success) - .fail(params.error); + }); + }).catch(params.error); }, data(search, page) { return { diff --git a/app/assets/javascripts/pages/projects/tree/show/index.js b/app/assets/javascripts/pages/projects/tree/show/index.js index c4b3356e478..cba57058380 100644 --- a/app/assets/javascripts/pages/projects/tree/show/index.js +++ b/app/assets/javascripts/pages/projects/tree/show/index.js @@ -14,7 +14,7 @@ export default () => { $('#tree-slider').waitForImages(() => ajaxGet(document.querySelector('.js-tree-content').dataset.logsPath)); - const commitPipelineStatusEl = document.getElementById('commit-pipeline-status'); + const commitPipelineStatusEl = document.querySelector('.js-commit-pipeline-status'); const statusLink = document.querySelector('.commit-actions .ci-status-link'); if (statusLink != null) { statusLink.remove(); diff --git a/app/models/key.rb b/app/models/key.rb index ae5769c0627..7406c98c99e 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -33,8 +33,9 @@ class Key < ActiveRecord::Base after_destroy :refresh_user_cache def key=(value) - write_attribute(:key, value.present? ? Gitlab::SSHPublicKey.sanitize(value) : nil) - + value&.delete!("\n\r") + value.strip! unless value.blank? + write_attribute(:key, value) @public_key = nil end @@ -96,7 +97,7 @@ class Key < ActiveRecord::Base def generate_fingerprint self.fingerprint = nil - return unless public_key.valid? + return unless self.key.present? self.fingerprint = public_key.fingerprint end diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 64259669c19..6ff7bcae54f 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -51,7 +51,7 @@ - if commit.status(ref) = render_commit_status(commit, ref: ref) - #commit-pipeline-status{ data: { endpoint: pipelines_project_commit_path(project, commit.id) } } + .js-commit-pipeline-status{ data: { endpoint: pipelines_project_commit_path(project, commit.id) } } = link_to commit.short_id, link, class: "commit-sha btn btn-transparent btn-link" = clipboard_button(text: commit.id, title: _("Copy commit SHA to clipboard")) = link_to_browse_code(project, commit) diff --git a/app/views/projects/milestones/show.html.haml b/app/views/projects/milestones/show.html.haml index 623c42ba88e..de381d489c6 100644 --- a/app/views/projects/milestones/show.html.haml +++ b/app/views/projects/milestones/show.html.haml @@ -27,7 +27,7 @@ Edit - if @project.group - = link_to promote_project_milestone_path(@milestone.project, @milestone), title: "Promote to Group Milestone", class: 'btn btn-grouped', data: { confirm: "You are about to promote #{@milestone.title} to a group level. This will make this milestone available to all projects inside #{@project.group.name}. The existing project milestone will be merged into the group level. This action cannot be reversed.", toggle: "tooltip" }, method: :post do + = link_to promote_project_milestone_path(@milestone.project, @milestone), title: "Promote to Group Milestone", class: 'btn btn-grouped', data: { confirm: "Promoting #{@milestone.title} will make it available for all projects inside #{@project.group.name}. Existing project milestones with the same name will be merged. This action cannot be reversed.", toggle: "tooltip" }, method: :post do Promote - if @milestone.active? diff --git a/app/views/shared/_label.html.haml b/app/views/shared/_label.html.haml index c0eebdfaddd..8847d11f623 100644 --- a/app/views/shared/_label.html.haml +++ b/app/views/shared/_label.html.haml @@ -48,7 +48,7 @@ .pull-right.hidden-xs.hidden-sm - if label.is_a?(ProjectLabel) && label.project.group && can?(current_user, :admin_label, label.project.group) - = link_to promote_project_label_path(label.project, label), title: "Promote to Group Label", class: 'btn btn-transparent btn-action', data: {confirm: "You are about to promote #{label.title} to a group level. This will make this milestone available to all projects inside #{label.project.group.name}. The existing project label will be merged into the group level. This action cannot be reversed.", toggle: "tooltip"}, method: :post do + = link_to promote_project_label_path(label.project, label), title: "Promote to Group Label", class: 'btn btn-transparent btn-action', data: {confirm: "Promoting #{label.title} will make it available for all projects inside #{label.project.group.name}. Existing project labels with the same name will be merged. This action cannot be reversed.", toggle: "tooltip"}, method: :post do %span.sr-only Promote to Group = sprite_icon('level-up') - if can?(current_user, :admin_label, label) diff --git a/app/views/shared/milestones/_milestone.html.haml b/app/views/shared/milestones/_milestone.html.haml index e08a49b4e59..e3b2b53833e 100644 --- a/app/views/shared/milestones/_milestone.html.haml +++ b/app/views/shared/milestones/_milestone.html.haml @@ -51,7 +51,7 @@ \ - if @project.group - = link_to promote_project_milestone_path(milestone.project, milestone), title: "Promote to Group Milestone", class: 'btn btn-xs btn-grouped', data: { confirm: "You are about to promote #{milestone.title} to a group level. This will make this milestone available to all projects inside #{@project.group.name}. The existing project milestone will be merged into the group level. This action cannot be reversed.", toggle: "tooltip" }, method: :post do + = link_to promote_project_milestone_path(milestone.project, milestone), title: "Promote to Group Milestone", class: 'btn btn-xs btn-grouped', data: { confirm: "Promoting #{milestone.title} will make it available for all projects inside #{@project.group.name}. Existing project milestones with the same name will be merged. This action cannot be reversed.", toggle: "tooltip" }, method: :post do Promote = link_to 'Close Milestone', project_milestone_path(@project, milestone, milestone: {state_event: :close }), method: :put, remote: true, class: "btn btn-xs btn-close btn-grouped" diff --git a/changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml b/changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml deleted file mode 100644 index 9e4811ca308..00000000000 --- a/changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Sanitize extra blank spaces used when uploading a SSH key -merge_request: 40552 -author: -type: fixed diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb index cc1e92480be..d4c54049b74 100644 --- a/lib/gitlab/middleware/multipart.rb +++ b/lib/gitlab/middleware/multipart.rb @@ -42,7 +42,7 @@ module Gitlab key, value = parsed_field.first if value.nil? - value = open_file(tmp_path) + value = open_file(tmp_path, @request.params["#{key}.name"]) @open_files << value else value = decorate_params_value(value, @request.params[key], tmp_path) @@ -70,7 +70,7 @@ module Gitlab case path_value when nil - value_hash[path_key] = open_file(tmp_path) + value_hash[path_key] = open_file(tmp_path, value_hash.dig(path_key, '.name')) @open_files << value_hash[path_key] value_hash when Hash @@ -81,8 +81,8 @@ module Gitlab end end - def open_file(path) - ::UploadedFile.new(path, File.basename(path), 'application/octet-stream') + def open_file(path, name) + ::UploadedFile.new(path, name || File.basename(path), 'application/octet-stream') end end diff --git a/lib/gitlab/query_limiting.rb b/lib/gitlab/query_limiting.rb index f64f1757144..9f69a9e4a39 100644 --- a/lib/gitlab/query_limiting.rb +++ b/lib/gitlab/query_limiting.rb @@ -6,7 +6,7 @@ module Gitlab # This ensures we don't produce any errors that users can't do anything # about themselves. def self.enable? - Gitlab.com? || Rails.env.development? || Rails.env.test? + Rails.env.development? || Rails.env.test? end # Allows the current request to execute any number of SQL queries. diff --git a/lib/gitlab/ssh_public_key.rb b/lib/gitlab/ssh_public_key.rb index 545e7c74f7e..89ca1298120 100644 --- a/lib/gitlab/ssh_public_key.rb +++ b/lib/gitlab/ssh_public_key.rb @@ -21,22 +21,6 @@ module Gitlab technology(name)&.supported_sizes end - def self.sanitize(key_content) - ssh_type, *parts = key_content.strip.split - - return key_content if parts.empty? - - parts.each_with_object("#{ssh_type} ").with_index do |(part, content), index| - content << part - - if Gitlab::SSHPublicKey.new(content).valid? - break [content, parts[index + 1]].compact.join(' ') # Add the comment part if present - elsif parts.size == index + 1 # return original content if we've reached the last element - break key_content - end - end - end - attr_reader :key_text, :key # Unqualified MD5 fingerprint for compatibility @@ -53,23 +37,23 @@ module Gitlab end def valid? - key.present? && bits && technology.supported_sizes.include?(bits) + key.present? end def type - technology.name if key.present? + technology.name if valid? end def bits - return if key.blank? + return unless valid? case type when :rsa - key.n&.num_bits + key.n.num_bits when :dsa - key.p&.num_bits + key.p.num_bits when :ecdsa - key.group.order&.num_bits + key.group.order.num_bits when :ed25519 256 else diff --git a/spec/factories/keys.rb b/spec/factories/keys.rb index 23a98a899f1..f0c43f3d6f5 100644 --- a/spec/factories/keys.rb +++ b/spec/factories/keys.rb @@ -5,10 +5,6 @@ FactoryBot.define do title key { Spec::Support::Helpers::KeyGeneratorHelper.new(1024).generate + ' dummy@gitlab.com' } - factory :key_without_comment do - key { Spec::Support::Helpers::KeyGeneratorHelper.new(1024).generate } - end - factory :deploy_key, class: 'DeployKey' factory :personal_key do diff --git a/spec/lib/gitlab/middleware/multipart_spec.rb b/spec/lib/gitlab/middleware/multipart_spec.rb index 8d925460f01..a2ba91dae80 100644 --- a/spec/lib/gitlab/middleware/multipart_spec.rb +++ b/spec/lib/gitlab/middleware/multipart_spec.rb @@ -5,15 +5,17 @@ require 'tempfile' describe Gitlab::Middleware::Multipart do let(:app) { double(:app) } let(:middleware) { described_class.new(app) } + let(:original_filename) { 'filename' } it 'opens top-level files' do Tempfile.open('top-level') do |tempfile| - env = post_env({ 'file' => tempfile.path }, { 'file.name' => 'filename' }, Gitlab::Workhorse.secret, 'gitlab-workhorse') + env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename }, Gitlab::Workhorse.secret, 'gitlab-workhorse') expect(app).to receive(:call) do |env| file = Rack::Request.new(env).params['file'] expect(file).to be_a(::UploadedFile) expect(file.path).to eq(tempfile.path) + expect(file.original_filename).to eq(original_filename) end middleware.call(env) @@ -34,13 +36,14 @@ describe Gitlab::Middleware::Multipart do it 'opens files one level deep' do Tempfile.open('one-level') do |tempfile| - in_params = { 'user' => { 'avatar' => { '.name' => 'filename' } } } + in_params = { 'user' => { 'avatar' => { '.name' => original_filename } } } env = post_env({ 'user[avatar]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse') expect(app).to receive(:call) do |env| file = Rack::Request.new(env).params['user']['avatar'] expect(file).to be_a(::UploadedFile) expect(file.path).to eq(tempfile.path) + expect(file.original_filename).to eq(original_filename) end middleware.call(env) @@ -49,13 +52,14 @@ describe Gitlab::Middleware::Multipart do it 'opens files two levels deep' do Tempfile.open('two-levels') do |tempfile| - in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => 'filename' } } } } + in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => original_filename } } } } env = post_env({ 'project[milestone][themesong]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse') expect(app).to receive(:call) do |env| file = Rack::Request.new(env).params['project']['milestone']['themesong'] expect(file).to be_a(::UploadedFile) expect(file.path).to eq(tempfile.path) + expect(file.original_filename).to eq(original_filename) end middleware.call(env) diff --git a/spec/lib/gitlab/query_limiting_spec.rb b/spec/lib/gitlab/query_limiting_spec.rb index 2eddab0b8c3..42877b1e2dd 100644 --- a/spec/lib/gitlab/query_limiting_spec.rb +++ b/spec/lib/gitlab/query_limiting_spec.rb @@ -12,14 +12,16 @@ describe Gitlab::QueryLimiting do expect(described_class.enable?).to eq(true) end - it 'returns true on GitLab.com' do + it 'returns false on GitLab.com' do + expect(Rails.env).to receive(:development?).and_return(false) + expect(Rails.env).to receive(:test?).and_return(false) allow(Gitlab).to receive(:com?).and_return(true) - expect(described_class.enable?).to eq(true) + expect(described_class.enable?).to eq(false) end - it 'returns true in a non GitLab.com' do - expect(Gitlab).to receive(:com?).and_return(false) + it 'returns false in a non GitLab.com' do + allow(Gitlab).to receive(:com?).and_return(false) expect(Rails.env).to receive(:development?).and_return(false) expect(Rails.env).to receive(:test?).and_return(false) diff --git a/spec/lib/gitlab/ssh_public_key_spec.rb b/spec/lib/gitlab/ssh_public_key_spec.rb index c15e29774b6..93d538141ce 100644 --- a/spec/lib/gitlab/ssh_public_key_spec.rb +++ b/spec/lib/gitlab/ssh_public_key_spec.rb @@ -37,41 +37,6 @@ describe Gitlab::SSHPublicKey, lib: true do end end - describe '.sanitize(key_content)' do - let(:content) { build(:key).key } - - context 'when key has blank space characters' do - it 'removes the extra blank space characters' do - unsanitized = content.insert(100, "\n") - .insert(40, "\r\n") - .insert(30, ' ') - - sanitized = described_class.sanitize(unsanitized) - _, body = sanitized.split - - expect(sanitized).not_to eq(unsanitized) - expect(body).not_to match(/\s/) - end - end - - context "when key doesn't have blank space characters" do - it "doesn't modify the content" do - sanitized = described_class.sanitize(content) - - expect(sanitized).to eq(content) - end - end - - context "when key is invalid" do - it 'returns the original content' do - unsanitized = "ssh-foo any content==" - sanitized = described_class.sanitize(unsanitized) - - expect(sanitized).to eq(unsanitized) - end - end - end - describe '#valid?' do subject { public_key } diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 78fcbf6d47e..2b6b6a61182 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1413,7 +1413,7 @@ describe Ci::Build do [ { key: 'CI', value: 'true', public: true }, { key: 'GITLAB_CI', value: 'true', public: true }, - { key: 'GITLAB_FEATURES', value: '', public: true }, + { key: 'GITLAB_FEATURES', value: project.namespace.features.join(','), public: true }, { key: 'CI_SERVER_NAME', value: 'GitLab', public: true }, { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true }, { key: 'CI_SERVER_REVISION', value: Gitlab::REVISION, public: true }, diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index bf5703ac986..7398fd25aa8 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -72,52 +72,15 @@ describe Key, :mailer do expect(build(:key)).to be_valid end - it 'rejects the unfingerprintable key (not a key)' do - expect(build(:key, key: 'ssh-rsa an-invalid-key==')).not_to be_valid - end - - where(:factory, :chars, :expected_sections) do - [ - [:key, ["\n", "\r\n"], 3], - [:key, [' ', ' '], 3], - [:key_without_comment, [' ', ' '], 2] - ] - end - - with_them do - let!(:key) { create(factory) } - let!(:original_fingerprint) { key.fingerprint } - - it 'accepts a key with blank space characters after stripping them' do - modified_key = key.key.insert(100, chars.first).insert(40, chars.last) - _, content = modified_key.split - - key.update!(key: modified_key) - - expect(key).to be_valid - expect(key.key.split.size).to eq(expected_sections) - - expect(content).not_to match(/\s/) - expect(original_fingerprint).to eq(key.fingerprint) - end - end - end - - context 'validate size' do - where(:key_content, :result) do - [ - [Spec::Support::Helpers::KeyGeneratorHelper.new(512).generate, false], - [Spec::Support::Helpers::KeyGeneratorHelper.new(8192).generate, false], - [Spec::Support::Helpers::KeyGeneratorHelper.new(1024).generate, true] - ] + it 'accepts a key with newline charecters after stripping them' do + key = build(:key) + key.key = key.key.insert(100, "\n") + key.key = key.key.insert(40, "\r\n") + expect(key).to be_valid end - with_them do - it 'validates the size of the key' do - key = build(:key, key: key_content) - - expect(key.valid?).to eq(result) - end + it 'rejects the unfingerprintable key (not a key)' do + expect(build(:key, key: 'ssh-rsa an-invalid-key==')).not_to be_valid end end |