summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/controllers/concerns/wiki_actions.rb2
-rw-r--r--app/finders/events_finder.rb6
-rw-r--r--app/models/merge_request.rb2
-rw-r--r--app/services/snippets/repository_validation_service.rb72
-rw-r--r--changelogs/unreleased/private-profile-api.yml5
-rw-r--r--changelogs/unreleased/security-215175-filter-merge-participants.yml5
-rw-r--r--changelogs/unreleased/security-disable-caching-for-wiki-attachments.yml5
-rw-r--r--changelogs/unreleased/security-fj-add-snippet-repository-validation-bundle-import.yml5
-rw-r--r--lib/gitlab/import_export/snippet_repo_restorer.rb12
-rw-r--r--locale/gitlab.pot21
-rw-r--r--spec/finders/events_finder_spec.rb7
-rw-r--r--spec/lib/gitlab/import_export/snippet_repo_restorer_spec.rb64
-rw-r--r--spec/lib/gitlab/import_export/snippets_repo_restorer_spec.rb1
-rw-r--r--spec/models/merge_request_spec.rb38
-rw-r--r--spec/requests/api/events_spec.rb13
-rw-r--r--spec/services/snippets/repository_validation_service_spec.rb69
-rw-r--r--spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb42
-rwxr-xr-x[-rw-r--r--]vendor/gitignore/C++.gitignore0
-rwxr-xr-x[-rw-r--r--]vendor/gitignore/Java.gitignore0
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