diff options
19 files changed, 309 insertions, 60 deletions
diff --git a/app/controllers/concerns/wiki_actions.rb b/app/controllers/concerns/wiki_actions.rb index b4b4fd84c37..7eef12fadfe 100644 --- a/app/controllers/concerns/wiki_actions.rb +++ b/app/controllers/concerns/wiki_actions.rb @@ -58,7 +58,7 @@ module WikiActions render 'shared/wikis/show' elsif file_blob - send_blob(wiki.repository, file_blob, allow_caching: container.public?) + send_blob(wiki.repository, file_blob) elsif show_create_form? # Assign a title to the WikiPage unless `id` is a randomly generated slug from #new title = params[:id] unless params[:random_title].present? diff --git a/app/finders/events_finder.rb b/app/finders/events_finder.rb index 52612f1f8aa..004fbc4cd22 100644 --- a/app/finders/events_finder.rb +++ b/app/finders/events_finder.rb @@ -33,6 +33,8 @@ class EventsFinder end def execute + return Event.none if cannot_access_private_profile? + events = get_events events = by_current_user_access(events) @@ -103,6 +105,10 @@ class EventsFinder end # rubocop: enable CodeReuse/ActiveRecord + def cannot_access_private_profile? + source.is_a?(User) && !Ability.allowed?(current_user, :read_user_profile, source) + end + def sort(events) return events unless params[:sort] diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index caf7b554427..a7e0907eb5f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -518,7 +518,7 @@ class MergeRequest < ApplicationRecord participants << merge_user end - participants + participants.select { |participant| Ability.allowed?(participant, :read_merge_request, self) } end def first_commit diff --git a/app/services/snippets/repository_validation_service.rb b/app/services/snippets/repository_validation_service.rb new file mode 100644 index 00000000000..c8197795383 --- /dev/null +++ b/app/services/snippets/repository_validation_service.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +module Snippets + class RepositoryValidationService + attr_reader :current_user, :snippet, :repository + + RepositoryValidationError = Class.new(StandardError) + + def initialize(user, snippet) + @current_user = user + @snippet = snippet + @repository = snippet.repository + end + + def execute + if snippet.nil? + return service_response_error('No snippet found.', 404) + end + + check_branch_count! + check_branch_name_default! + check_tag_count! + check_file_count! + check_size! + + ServiceResponse.success(message: 'Valid snippet repository.') + rescue RepositoryValidationError => e + ServiceResponse.error(message: "Error: #{e.message}", http_status: 400) + end + + private + + def check_branch_count! + return if repository.branch_count == 1 + + raise RepositoryValidationError, _('Repository has more than one branch.') + end + + def check_branch_name_default! + branches = repository.branch_names + + return if branches.first == Gitlab::Checks::SnippetCheck::DEFAULT_BRANCH + + raise RepositoryValidationError, _('Repository has an invalid default branch name.') + end + + def check_tag_count! + return if repository.tag_count == 0 + + raise RepositoryValidationError, _('Repository has tags.') + end + + def check_file_count! + file_count = repository.ls_files(nil).size + limit = Snippet.max_file_limit(current_user) + + if file_count > limit + raise RepositoryValidationError, _('Repository files count over the limit') + end + + if file_count == 0 + raise RepositoryValidationError, _('Repository must contain at least 1 file.') + end + end + + def check_size! + return unless snippet.repository_size_checker.above_size_limit? + + raise RepositoryValidationError, _('Repository size is above the limit.') + end + end +end diff --git a/changelogs/unreleased/private-profile-api.yml b/changelogs/unreleased/private-profile-api.yml new file mode 100644 index 00000000000..ce077882860 --- /dev/null +++ b/changelogs/unreleased/private-profile-api.yml @@ -0,0 +1,5 @@ +--- +title: Do not show activity for users with private profiles +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-215175-filter-merge-participants.yml b/changelogs/unreleased/security-215175-filter-merge-participants.yml new file mode 100644 index 00000000000..1baaa17399f --- /dev/null +++ b/changelogs/unreleased/security-215175-filter-merge-participants.yml @@ -0,0 +1,5 @@ +--- +title: Check access when sending TODOs related to merge requests +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-disable-caching-for-wiki-attachments.yml b/changelogs/unreleased/security-disable-caching-for-wiki-attachments.yml new file mode 100644 index 00000000000..8e1bececa8d --- /dev/null +++ b/changelogs/unreleased/security-disable-caching-for-wiki-attachments.yml @@ -0,0 +1,5 @@ +--- +title: Disable caching for wiki attachments +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fj-add-snippet-repository-validation-bundle-import.yml b/changelogs/unreleased/security-fj-add-snippet-repository-validation-bundle-import.yml new file mode 100644 index 00000000000..ff2ba0950fd --- /dev/null +++ b/changelogs/unreleased/security-fj-add-snippet-repository-validation-bundle-import.yml @@ -0,0 +1,5 @@ +--- +title: Add snippet repository validation after bundle import +merge_request: +author: +type: security diff --git a/lib/gitlab/import_export/snippet_repo_restorer.rb b/lib/gitlab/import_export/snippet_repo_restorer.rb index 334d13a13ae..31b1a37bbe1 100644 --- a/lib/gitlab/import_export/snippet_repo_restorer.rb +++ b/lib/gitlab/import_export/snippet_repo_restorer.rb @@ -3,7 +3,7 @@ module Gitlab module ImportExport class SnippetRepoRestorer < RepoRestorer - attr_reader :snippet + attr_reader :snippet, :user SnippetRepositoryError = Class.new(StandardError) @@ -33,6 +33,16 @@ module Gitlab def create_repository_from_bundle repository.create_from_bundle(path_to_bundle) snippet.track_snippet_repository(repository.storage) + + response = Snippets::RepositoryValidationService.new(user, snippet).execute + + if response.error? + repository.remove + snippet.snippet_repository.delete + snippet.repository.expire_exists_cache + + raise SnippetRepositoryError, _("Invalid repository bundle for snippet with id %{snippet_id}") % { snippet_id: snippet.id } + end end def create_repository_from_db diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8bc8e3426fc..77eb977633d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -12299,6 +12299,9 @@ msgstr "" msgid "Invalid query" msgstr "" +msgid "Invalid repository bundle for snippet with id %{snippet_id}" +msgstr "" + msgid "Invalid repository path" msgstr "" @@ -18943,15 +18946,33 @@ msgstr "" msgid "Repository cleanup has started. You will receive an email once the cleanup operation is complete." msgstr "" +msgid "Repository files count over the limit" +msgstr "" + +msgid "Repository has an invalid default branch name." +msgstr "" + +msgid "Repository has more than one branch." +msgstr "" + msgid "Repository has no locks." msgstr "" +msgid "Repository has tags." +msgstr "" + msgid "Repository maintenance" msgstr "" msgid "Repository mirroring" msgstr "" +msgid "Repository must contain at least 1 file." +msgstr "" + +msgid "Repository size is above the limit." +msgstr "" + msgid "Repository static objects" msgstr "" diff --git a/spec/finders/events_finder_spec.rb b/spec/finders/events_finder_spec.rb index 45a049f9442..b13ef7e94e7 100644 --- a/spec/finders/events_finder_spec.rb +++ b/spec/finders/events_finder_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe EventsFinder do let_it_be(:user) { create(:user) } + let(:private_user) { create(:user, private_profile: true) } let(:other_user) { create(:user) } let(:project1) { create(:project, :private, creator_id: user.id, namespace: user.namespace) } @@ -57,6 +58,12 @@ RSpec.describe EventsFinder do expect(events).to be_empty end + + it 'returns nothing when the target profile is private' do + events = described_class.new(source: private_user, current_user: other_user).execute + + expect(events).to be_empty + end end describe 'wiki events feature flag' do diff --git a/spec/lib/gitlab/import_export/snippet_repo_restorer_spec.rb b/spec/lib/gitlab/import_export/snippet_repo_restorer_spec.rb index 779b65e33d8..43c4b164b2d 100644 --- a/spec/lib/gitlab/import_export/snippet_repo_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/snippet_repo_restorer_spec.rb @@ -4,9 +4,9 @@ require 'spec_helper' describe Gitlab::ImportExport::SnippetRepoRestorer do let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, namespace: user.namespace) } - let(:snippet) { create(:project_snippet, project: project, author: user) } + let(:project) { create(:project, namespace: user.namespace) } + let(:snippet) { create(:project_snippet, project: project, author: user) } let(:shared) { project.import_export_shared } let(:exporter) { Gitlab::ImportExport::SnippetsRepoSaver.new(project: project, shared: shared, current_user: user) } let(:restorer) do @@ -57,33 +57,63 @@ describe Gitlab::ImportExport::SnippetRepoRestorer do it_behaves_like 'no bundle file present' end - context 'when the snippet bundle exists' do - let!(:snippet_with_repo) { create(:project_snippet, :repository, project: project) } + context 'when the snippet repository bundle exists' do + let!(:snippet_with_repo) { create(:project_snippet, :repository, project: project, author: user) } let(:bundle_path) { ::Gitlab::ImportExport.snippets_repo_bundle_path(shared.export_path) } let(:snippet_bundle_path) { File.join(bundle_path, "#{snippet_with_repo.hexdigest}.bundle") } let(:result) { exporter.save } + let(:repository) { snippet.repository } before do expect(exporter.save).to be_truthy end - it 'creates the repository from the bundle' do - expect(snippet.repository_exists?).to be_falsey - expect(snippet.snippet_repository).to be_nil - expect(snippet.repository).to receive(:create_from_bundle).and_call_original + context 'when it is valid' do + before do + allow(repository).to receive(:branch_count).and_return(1) + allow(repository).to receive(:tag_count).and_return(0) + allow(repository).to receive(:branch_names).and_return(['master']) + allow(repository).to receive(:ls_files).and_return(['foo']) + end - expect(restorer.restore).to be_truthy - expect(snippet.repository_exists?).to be_truthy - expect(snippet.snippet_repository).not_to be_nil - end + it 'creates the repository from the bundle' do + expect(snippet.repository_exists?).to be_falsey + expect(snippet.snippet_repository).to be_nil + expect(repository).to receive(:create_from_bundle).and_call_original - it 'sets same shard in snippet repository as in the repository storage' do - expect(snippet).to receive(:repository_storage).and_return('picked') - expect(snippet.repository).to receive(:create_from_bundle) + expect(restorer.restore).to be_truthy + expect(snippet.repository_exists?).to be_truthy + expect(snippet.snippet_repository).not_to be_nil + end - restorer.restore + it 'sets same shard in snippet repository as in the repository storage' do + expect(repository).to receive(:storage).and_return('picked') + expect(repository).to receive(:create_from_bundle) - expect(snippet.snippet_repository.shard_name).to eq 'picked' + expect(restorer.restore).to be_truthy + expect(snippet.snippet_repository.shard_name).to eq 'picked' + end + end + + context 'when it is invalid' do + it 'returns false and deletes the repository from disk and the database' do + gitlab_shell = Gitlab::Shell.new + shard_name = snippet.repository.shard + path = snippet.disk_path + '.git' + error_response = ServiceResponse.error(message: 'Foo', http_status: 400) + + allow_next_instance_of(Snippets::RepositoryValidationService) do |instance| + allow(instance).to receive(:execute).and_return(error_response) + end + + aggregate_failures do + expect(restorer.restore).to be false + expect(shared.errors.first).to match(/Invalid repository bundle/) + expect(snippet.repository_exists?).to eq false + expect(snippet.reload.snippet_repository).to be_nil + expect(gitlab_shell.repository_exists?(shard_name, path)).to eq false + end + end end end end diff --git a/spec/lib/gitlab/import_export/snippets_repo_restorer_spec.rb b/spec/lib/gitlab/import_export/snippets_repo_restorer_spec.rb index fdae259c2f1..ac73462073e 100644 --- a/spec/lib/gitlab/import_export/snippets_repo_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/snippets_repo_restorer_spec.rb @@ -38,6 +38,7 @@ describe Gitlab::ImportExport::SnippetsRepoRestorer do expect(snippet1.repository_exists?).to be false expect(snippet2.repository_exists?).to be false + allow_any_instance_of(Snippets::RepositoryValidationService).to receive(:execute).and_return(ServiceResponse.success) expect(Gitlab::ImportExport::SnippetRepoRestorer).to receive(:new).with(hash_including(snippet: snippet1, path_to_bundle: bundle_path(snippet1))).and_call_original expect(Gitlab::ImportExport::SnippetRepoRestorer).to receive(:new).with(hash_including(snippet: snippet2, path_to_bundle: bundle_path(snippet2))).and_call_original expect(restorer.restore).to be_truthy diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c70ddac5da6..582cdc7b419 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -3655,7 +3655,7 @@ describe MergeRequest do describe '#merge_participants' do it 'contains author' do - expect(subject.merge_participants).to eq([subject.author]) + expect(subject.merge_participants).to contain_exactly(subject.author) end describe 'when merge_when_pipeline_succeeds? is true' do @@ -3669,8 +3669,20 @@ describe MergeRequest do author: user) end - it 'contains author only' do - expect(subject.merge_participants).to eq([subject.author]) + context 'author is not a project member' do + it 'is empty' do + expect(subject.merge_participants).to be_empty + end + end + + context 'author is a project member' do + before do + subject.project.team.add_reporter(user) + end + + it 'contains author only' do + expect(subject.merge_participants).to contain_exactly(subject.author) + end end end @@ -3683,8 +3695,24 @@ describe MergeRequest do merge_user: merge_user) end - it 'contains author and merge user' do - expect(subject.merge_participants).to eq([subject.author, merge_user]) + before do + subject.project.team.add_reporter(subject.author) + end + + context 'merge user is not a member' do + it 'contains author only' do + expect(subject.merge_participants).to contain_exactly(subject.author) + end + end + + context 'both author and merge users are project members' do + before do + subject.project.team.add_reporter(merge_user) + end + + it 'contains author and merge user' do + expect(subject.merge_participants).to contain_exactly(subject.author, merge_user) + end end end end diff --git a/spec/requests/api/events_spec.rb b/spec/requests/api/events_spec.rb index 0425e0791eb..58a55c2e6d0 100644 --- a/spec/requests/api/events_spec.rb +++ b/spec/requests/api/events_spec.rb @@ -192,6 +192,19 @@ describe API::Events do end end + context 'when target users profile is private' do + it 'returns no events' do + user.update!(private_profile: true) + private_project.add_developer(non_member) + + get api("/users/#{user.username}/events", non_member) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to eq([]) + end + end + context 'when scope is passed' do context 'when unauthenticated' do it 'returns no user events' do diff --git a/spec/services/snippets/repository_validation_service_spec.rb b/spec/services/snippets/repository_validation_service_spec.rb new file mode 100644 index 00000000000..1c139d8c223 --- /dev/null +++ b/spec/services/snippets/repository_validation_service_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Snippets::RepositoryValidationService do + describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be(:snippet) { create(:personal_snippet, :empty_repo, author: user) } + + let(:repository) { snippet.repository } + let(:service) { described_class.new(user, snippet) } + + subject { service.execute } + + before do + allow(repository).to receive(:branch_count).and_return(1) + allow(repository).to receive(:ls_files).and_return(['foo']) + allow(repository).to receive(:branch_names).and_return(['master']) + end + + it 'returns error when the repository has more than one branch' do + allow(repository).to receive(:branch_count).and_return(2) + + expect(subject).to be_error + expect(subject.message).to match /Repository has more than one branch/ + end + + it 'returns error when existing branch name is not the default one' do + allow(repository).to receive(:branch_names).and_return(['foo']) + + expect(subject).to be_error + expect(subject.message).to match /Repository has an invalid default branch name/ + end + + it 'returns error when the repository has tags' do + allow(repository).to receive(:tag_count).and_return(1) + + expect(subject).to be_error + expect(subject.message).to match /Repository has tags/ + end + + it 'returns error when the repository has more file than the limit' do + limit = Snippet.max_file_limit(user) + 1 + files = Array.new(limit) { FFaker::Filesystem.file_name } + allow(repository).to receive(:ls_files).and_return(files) + + expect(subject).to be_error + expect(subject.message).to match /Repository files count over the limit/ + end + + it 'returns error when the repository has no files' do + allow(repository).to receive(:ls_files).and_return([]) + + expect(subject).to be_error + expect(subject.message).to match /Repository must contain at least 1 file/ + end + + it 'returns error when the repository size is over the limit' do + expect_any_instance_of(Gitlab::RepositorySizeChecker).to receive(:above_size_limit?).and_return(true) + + expect(subject).to be_error + expect(subject.message).to match /Repository size is above the limit/ + end + + it 'returns success when no validation errors are raised' do + expect(subject).to be_success + end + end +end diff --git a/spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb b/spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb index c128bbe5e02..b5f2c0d07bf 100644 --- a/spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb @@ -158,46 +158,18 @@ RSpec.shared_examples 'wiki controller actions' do context 'when page is a file' do include WikiHelpers - let(:id) { upload_file_to_wiki(container, user, file_name) } + where(:file_name) { ['dk.png', 'unsanitized.svg', 'git-cheat-sheet.pdf'] } - context 'when file is an image' do - let(:file_name) { 'dk.png' } + with_them do + let(:id) { upload_file_to_wiki(container, user, file_name) } - it 'delivers the image' do + it 'delivers the file with the correct headers' do subject expect(response.headers['Content-Disposition']).to match(/^inline/) - expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" - end - - context 'when file is a svg' do - let(:file_name) { 'unsanitized.svg' } - - it 'delivers the image' do - subject - - expect(response.headers['Content-Disposition']).to match(/^inline/) - expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" - end - end - - it_behaves_like 'project cache control headers' do - let(:project) { container } - end - end - - context 'when file is a pdf' do - let(:file_name) { 'git-cheat-sheet.pdf' } - - it 'sets the content type to sets the content response headers' do - subject - - expect(response.headers['Content-Disposition']).to match(/^inline/) - expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" - end - - it_behaves_like 'project cache control headers' do - let(:project) { container } + expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq('true') + expect(response.cache_control[:public]).to be(false) + expect(response.cache_control[:extras]).to include('no-store') end end end diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore index 259148fa18f..259148fa18f 100644..100755 --- a/vendor/gitignore/C++.gitignore +++ b/vendor/gitignore/C++.gitignore diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore index a1c2a238a96..a1c2a238a96 100644..100755 --- a/vendor/gitignore/Java.gitignore +++ b/vendor/gitignore/Java.gitignore |