diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-06-03 00:59:19 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-06-03 00:59:19 +0300 |
commit | 1385478346704d03ab9d3a9bf8ae3812cea0b6b5 (patch) | |
tree | c2b68728119200c48fbfe09bb09397d4e31659b7 /spec | |
parent | 361d9dae8bafae8c830d68d16ea0f76482ba9343 (diff) |
Add latest changes from gitlab-org/security/gitlab@16-0-stable-ee
Diffstat (limited to 'spec')
27 files changed, 812 insertions, 203 deletions
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index b652aba1fff..577f10b961c 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -193,38 +193,79 @@ RSpec.describe ProjectsController, feature_category: :projects do end end - context 'when the default branch name can resolve to another ref' do - let!(:project_with_default_branch) do - create(:project, :public, :custom_repo, files: ['somefile']).tap do |p| - p.repository.create_branch("refs/heads/refs/heads/#{other_ref}", 'master') - p.change_head("refs/heads/#{other_ref}") - end.reload + context 'when the default branch name is ambiguous' do + let_it_be(:project_with_default_branch) do + create(:project, :public, :custom_repo, files: ['somefile']) end - let(:other_ref) { 'branch-name' } + shared_examples 'ambiguous ref redirects' do + let(:project) { project_with_default_branch } + let(:branch_ref) { "refs/heads/#{ref}" } + let(:repo) { project.repository } - context 'but there is no other ref' do - it 'responds with ok' do - get :show, params: { namespace_id: project_with_default_branch.namespace, id: project_with_default_branch } - expect(response).to be_ok + before do + repo.create_branch(branch_ref, 'master') + repo.change_head(ref) end - end - context 'and that other ref exists' do - let(:tree_with_default_branch) do - branch = project_with_default_branch.repository.find_branch(project_with_default_branch.default_branch) - project_tree_path(project_with_default_branch, branch.target) + after do + repo.change_head('master') + repo.delete_branch(branch_ref) end - before do - project_with_default_branch.repository.create_branch(other_ref, 'master') + subject do + get( + :show, + params: { + namespace_id: project.namespace, + id: project + } + ) + end + + context 'when there is no conflicting ref' do + let(:other_ref) { 'non-existent-ref' } + + it { is_expected.to have_gitlab_http_status(:ok) } end - it 'redirects to tree view for the default branch' do - get :show, params: { namespace_id: project_with_default_branch.namespace, id: project_with_default_branch } - expect(response).to redirect_to(tree_with_default_branch) + context 'and that other ref exists' do + let(:other_ref) { 'master' } + + let(:project_default_root_tree_path) do + sha = repo.find_branch(project.default_branch).target + project_tree_path(project, sha) + end + + it 'redirects to tree view for the default branch' do + is_expected.to redirect_to(project_default_root_tree_path) + end end end + + context 'when ref starts with ref/heads/' do + let(:ref) { "refs/heads/#{other_ref}" } + + include_examples 'ambiguous ref redirects' + end + + context 'when ref starts with ref/tags/' do + let(:ref) { "refs/tags/#{other_ref}" } + + include_examples 'ambiguous ref redirects' + end + + context 'when ref starts with heads/' do + let(:ref) { "heads/#{other_ref}" } + + include_examples 'ambiguous ref redirects' + end + + context 'when ref starts with tags/' do + let(:ref) { "tags/#{other_ref}" } + + include_examples 'ambiguous ref redirects' + end end end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 351583b7ef6..9cf755b2842 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -20,6 +20,10 @@ FactoryBot.define do public_email { email } end + trait :notification_email do + notification_email { email } + end + trait :private_profile do private_profile { true } end diff --git a/spec/frontend/single_file_diff_spec.js b/spec/frontend/single_file_diff_spec.js index ff2a4e31e0b..572fd3810ab 100644 --- a/spec/frontend/single_file_diff_spec.js +++ b/spec/frontend/single_file_diff_spec.js @@ -67,6 +67,21 @@ describe('SingleFileDiff', () => { expect(mock.history.get.length).toBe(1); }); + it('ignores user-defined diff path attributes', () => { + setHTMLFixture(` + <div class="diff-file"> + <div class="diff-content"> + <div class="diff-viewer" data-type="simple"> + <div class="note-text"><a data-diff-for-path="test/note/path">Test note</a></div> + <div data-diff-for-path="${blobDiffPath}">MOCK CONTENT</div> + </div> + </div> + </div> +`); + const { diffForPath } = new SingleFileDiff(document.querySelector('.diff-file')); + expect(diffForPath).toEqual(blobDiffPath); + }); + it('does not load diffs via axios for already expanded diffs', async () => { setHTMLFixture(` <div class="diff-file"> diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb index 93df9d5f94b..277869e7bd3 100644 --- a/spec/helpers/merge_requests_helper_spec.rb +++ b/spec/helpers/merge_requests_helper_spec.rb @@ -183,10 +183,18 @@ RSpec.describe MergeRequestsHelper, feature_category: :code_review_workflow do end describe '#merge_request_source_branch' do - let_it_be(:project) { create(:project) } - let(:forked_project) { fork_project(project) } - let(:merge_request_forked) { create(:merge_request, source_project: forked_project, target_project: project) } + branch_name = 'name<script>test</script>' + let(:project) { create(:project) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:forked_project) { fork_project(project) } + let(:merge_request_forked) do + create( + :merge_request, + source_project: forked_project, + source_branch: branch_name, + target_project: project + ) + end context 'when merge request is a fork' do subject { merge_request_source_branch(merge_request_forked) } @@ -194,6 +202,10 @@ RSpec.describe MergeRequestsHelper, feature_category: :code_review_workflow do it 'does show the fork icon' do expect(subject).to match(/fork/) end + + it 'escapes properly' do + expect(subject).to include(html_escape(branch_name)) + end end context 'when merge request is not a fork' do diff --git a/spec/lib/banzai/filter/front_matter_filter_spec.rb b/spec/lib/banzai/filter/front_matter_filter_spec.rb index b15f3ad2cd6..9d25dafb727 100644 --- a/spec/lib/banzai/filter/front_matter_filter_spec.rb +++ b/spec/lib/banzai/filter/front_matter_filter_spec.rb @@ -189,19 +189,29 @@ RSpec.describe Banzai::Filter::FrontMatterFilter, feature_category: :team_planni end end - it 'fails fast for strings with many spaces' do - content = "coding:" + " " * 50_000 + ";" + describe 'protects against malicious backtracking' do + it 'fails fast for strings with many spaces' do + content = "coding:" + " " * 50_000 + ";" - expect do - Timeout.timeout(3.seconds) { filter(content) } - end.not_to raise_error - end + expect do + Timeout.timeout(3.seconds) { filter(content) } + end.not_to raise_error + end + + it 'fails fast for strings with many newlines' do + content = "coding:\n" + ";;;" + "\n" * 10_000 + "x" - it 'fails fast for strings with many newlines' do - content = "coding:\n" + ";;;" + "\n" * 10_000 + "x" + expect do + Timeout.timeout(3.seconds) { filter(content) } + end.not_to raise_error + end + + it 'fails fast for strings with many `coding:`' do + content = "coding:" * 120_000 + "\n" * 80_000 + ";" - expect do - Timeout.timeout(3.seconds) { filter(content) } - end.not_to raise_error + expect do + Timeout.timeout(3.seconds) { filter(content) } + end.not_to raise_error + end end end diff --git a/spec/lib/banzai/filter/inline_diff_filter_spec.rb b/spec/lib/banzai/filter/inline_diff_filter_spec.rb index 1ef00139db2..1388a9053d9 100644 --- a/spec/lib/banzai/filter/inline_diff_filter_spec.rb +++ b/spec/lib/banzai/filter/inline_diff_filter_spec.rb @@ -67,4 +67,12 @@ RSpec.describe Banzai::Filter::InlineDiffFilter do doc = "<tt>START {+something added+} END</tt>" expect(filter(doc).to_html).to eq(doc) end + + it 'protects against malicious backtracking' do + doc = '[-{-' * 250_000 + + expect do + Timeout.timeout(3.seconds) { filter(doc) } + end.not_to raise_error + end end diff --git a/spec/lib/banzai/filter/math_filter_spec.rb b/spec/lib/banzai/filter/math_filter_spec.rb index ded94dd6ce5..e4ebebc0fde 100644 --- a/spec/lib/banzai/filter/math_filter_spec.rb +++ b/spec/lib/banzai/filter/math_filter_spec.rb @@ -215,6 +215,14 @@ RSpec.describe Banzai::Filter::MathFilter, feature_category: :team_planning do expect(doc.search('.js-render-math').count).to eq(2) end + it 'protects against malicious backtracking' do + doc = pipeline_filter("$$#{' ' * 1_000_000}$") + + expect do + Timeout.timeout(3.seconds) { filter(doc) } + end.not_to raise_error + end + def pipeline_filter(text) context = { project: nil, no_sourcepos: true } diff --git a/spec/lib/bitbucket_server/representation/pull_request_spec.rb b/spec/lib/bitbucket_server/representation/pull_request_spec.rb index c5d0ee8908d..5312bc1d71b 100644 --- a/spec/lib/bitbucket_server/representation/pull_request_spec.rb +++ b/spec/lib/bitbucket_server/representation/pull_request_spec.rb @@ -115,7 +115,6 @@ RSpec.describe BitbucketServer::Representation::PullRequest, feature_category: : author: "root", description: "Test", source_branch_name: "refs/heads/root/CODE_OF_CONDUCTmd-1530600625006", - source_branch_sha: "074e2b4dddc5b99df1bf9d4a3f66cfc15481fdc8", target_branch_name: "refs/heads/master", target_branch_sha: "839fa9a2d434eb697815b8fcafaecc51accfdbbc", title: "Added a new line" diff --git a/spec/lib/gitlab/background_migration/mailers/unconfirm_mailer_spec.rb b/spec/lib/gitlab/background_migration/mailers/unconfirm_mailer_spec.rb index f430009989b..05bd020d4e2 100644 --- a/spec/lib/gitlab/background_migration/mailers/unconfirm_mailer_spec.rb +++ b/spec/lib/gitlab/background_migration/mailers/unconfirm_mailer_spec.rb @@ -7,6 +7,6 @@ RSpec.describe Gitlab::BackgroundMigration::Mailers::UnconfirmMailer do let(:subject) { described_class.unconfirm_notification_email(user) } it 'contains abuse report url' do - expect(subject.body.encoded).to include(Rails.application.routes.url_helpers.new_abuse_report_url(user_id: user.id)) + expect(subject.body.encoded).to include(Gitlab::Routing.url_helpers.user_url(user.id)) end end diff --git a/spec/lib/gitlab/checks/tag_check_spec.rb b/spec/lib/gitlab/checks/tag_check_spec.rb index 50ffa5fad10..e75b0459337 100644 --- a/spec/lib/gitlab/checks/tag_check_spec.rb +++ b/spec/lib/gitlab/checks/tag_check_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Checks::TagCheck do +RSpec.describe Gitlab::Checks::TagCheck, feature_category: :source_code_management do include_context 'change access checks context' describe '#validate!' do @@ -14,6 +14,29 @@ RSpec.describe Gitlab::Checks::TagCheck do expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, 'You are not allowed to change existing tags on this project.') end + context "prohibited tags check" do + it "prohibits tag names that include refs/tags/ at the head" do + allow(subject).to receive(:tag_name).and_return("refs/tags/foo") + + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a tag with a prohibited pattern.") + end + + it "doesn't prohibit a nested refs/tags/ string in a tag name" do + allow(subject).to receive(:tag_name).and_return("fix-for-refs/tags/foo") + + expect { subject.validate! }.not_to raise_error + end + + context "deleting a refs/tags headed tag" do + let(:newrev) { "0000000000000000000000000000000000000000" } + let(:ref) { "refs/tags/refs/tags/267208abfe40e546f5e847444276f7d43a39503e" } + + it "doesn't prohibit the deletion of a refs/tags/ tag name" do + expect { subject.validate! }.not_to raise_error + end + end + end + context 'with protected tag' do let!(:protected_tag) { create(:protected_tag, project: project, name: 'v*') } diff --git a/spec/lib/gitlab/ci/artifacts/decompressed_artifact_size_validator_spec.rb b/spec/lib/gitlab/ci/artifacts/decompressed_artifact_size_validator_spec.rb new file mode 100644 index 00000000000..ef39a431d63 --- /dev/null +++ b/spec/lib/gitlab/ci/artifacts/decompressed_artifact_size_validator_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Ci::Artifacts::DecompressedArtifactSizeValidator, feature_category: :build_artifacts do + include WorkhorseHelpers + + let_it_be(:file_path) { File.join(Dir.tmpdir, 'decompressed_archive_size_validator_spec.gz') } + let(:file) { File.open(file_path) } + let(:file_format) { :gzip } + let(:max_bytes) { 20 } + let(:gzip_valid?) { true } + let(:validator) { instance_double(::Gitlab::Ci::DecompressedGzipSizeValidator, valid?: gzip_valid?) } + + before(:all) do + Zlib::GzipWriter.open(file_path) do |gz| + gz.write('Hello World!') + end + end + + after(:all) do + FileUtils.rm(file_path) + end + + before do + allow(::Gitlab::Ci::DecompressedGzipSizeValidator) + .to receive(:new) + .and_return(validator) + end + + subject { described_class.new(file: file, file_format: file_format, max_bytes: max_bytes) } + + shared_examples 'when file does not exceed allowed compressed size' do + let(:gzip_valid?) { true } + + it 'passes validation' do + expect { subject.validate! }.not_to raise_error + end + end + + shared_examples 'when file exceeds allowed decompressed size' do + let(:gzip_valid?) { false } + + it 'raises an exception' do + expect { subject.validate! } + .to raise_error(Gitlab::Ci::Artifacts::DecompressedArtifactSizeValidator::FileDecompressionError) + end + end + + describe '#validate!' do + it_behaves_like 'when file does not exceed allowed compressed size' + + it_behaves_like 'when file exceeds allowed decompressed size' + end + + context 'when file is not provided' do + let(:file) { nil } + + it 'passes validation' do + expect { subject.validate! }.not_to raise_error + end + end + + context 'when the file is located in the cloud' do + let(:remote_path) { File.join(remote_store_path, remote_id) } + + let(:file_url) { "http://s3.amazonaws.com/#{remote_path}" } + let(:file) do + instance_double(JobArtifactUploader, + path: file_path, + url: file_url, + object_store: ObjectStorage::Store::REMOTE) + end + + let(:remote_id) { 'generated-remote-id-12345' } + let(:remote_store_path) { ObjectStorage::TMP_UPLOAD_PATH } + + before do + stub_request(:get, %r{s3.amazonaws.com/#{remote_path}}) + .to_return(status: 200, body: File.read('spec/fixtures/build.env.gz')) + end + + it_behaves_like 'when file does not exceed allowed compressed size' + + it_behaves_like 'when file exceeds allowed decompressed size' + end + + context 'when file_format is not on the list' do + let_it_be(:file_format) { 'rar' } + + it 'passes validation' do + expect { subject.validate! }.not_to raise_error + end + end +end diff --git a/spec/lib/gitlab/ci/decompressed_gzip_size_validator_spec.rb b/spec/lib/gitlab/ci/decompressed_gzip_size_validator_spec.rb new file mode 100644 index 00000000000..6ca3f4d415e --- /dev/null +++ b/spec/lib/gitlab/ci/decompressed_gzip_size_validator_spec.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Ci::DecompressedGzipSizeValidator, feature_category: :importers do + let_it_be(:filepath) { File.join(Dir.tmpdir, 'decompressed_gzip_size_validator_spec.gz') } + + before(:all) do + create_compressed_file + end + + after(:all) do + FileUtils.rm(filepath) + end + + subject { described_class.new(archive_path: filepath, max_bytes: max_bytes) } + + describe '#valid?' do + let(:max_bytes) { 20 } + + context 'when file does not exceed allowed decompressed size' do + it 'returns true' do + expect(subject.valid?).to eq(true) + end + + context 'when the waiter thread no longer exists due to being terminated or crashing' do + it 'gracefully handles the absence of the waiter without raising exception' do + allow(Process).to receive(:getpgid).and_raise(Errno::ESRCH) + + expect(subject.valid?).to eq(true) + end + end + end + + context 'when file exceeds allowed decompressed size' do + let(:max_bytes) { 1 } + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + + context 'when exception occurs during header readings' do + shared_examples 'raises exception and terminates validator process group' do + let(:std) { instance_double(IO, close: nil) } + let(:wait_thr) { double } + let(:wait_threads) { [wait_thr, wait_thr] } + + before do + allow(Process).to receive(:getpgid).and_return(2) + allow(Open3).to receive(:pipeline_r).and_return([std, wait_threads]) + allow(wait_thr).to receive(:[]).with(:pid).and_return(1) + allow(wait_thr).to receive(:value).and_raise(exception) + end + + it 'terminates validator process group' do + expect(Process).to receive(:kill).with(-1, 2).twice + expect(subject.valid?).to eq(false) + end + end + + context 'when timeout occurs' do + let(:exception) { Timeout::Error } + + include_examples 'raises exception and terminates validator process group' + end + + context 'when exception occurs' do + let(:error_message) { 'Error!' } + let(:exception) { StandardError.new(error_message) } + + include_examples 'raises exception and terminates validator process group' + end + end + + describe 'archive path validation' do + let(:filesize) { nil } + + context 'when archive path is traversed' do + let(:filepath) { '/foo/../bar' } + + it 'does not pass validation' do + expect(subject.valid?).to eq(false) + end + end + end + + context 'when archive path is not a string' do + let(:filepath) { 123 } + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + + context 'when archive path is a symlink' do + let(:filepath) { File.join(Dir.tmpdir, 'symlink') } + + before do + FileUtils.ln_s(filepath, filepath, force: true) + end + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + + context 'when archive path is not a file' do + let(:filepath) { Dir.mktmpdir } + let(:filesize) { File.size(filepath) } + + after do + FileUtils.rm_rf(filepath) + end + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + end + + def create_compressed_file + Zlib::GzipWriter.open(filepath) do |gz| + gz.write('Hello World!') + end + end +end diff --git a/spec/lib/gitlab/import_export/project/tree_saver_spec.rb b/spec/lib/gitlab/import_export/project/tree_saver_spec.rb index 4166eba4e8e..d8b8903c8ca 100644 --- a/spec/lib/gitlab/import_export/project/tree_saver_spec.rb +++ b/spec/lib/gitlab/import_export/project/tree_saver_spec.rb @@ -7,6 +7,8 @@ RSpec.describe Gitlab::ImportExport::Project::TreeSaver, :with_license, feature_ let_it_be(:exportable_path) { 'project' } let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } + let_it_be(:private_project) { create(:project, :private, group: group) } + let_it_be(:private_mr) { create(:merge_request, source_project: private_project, project: private_project) } let_it_be(:project) { setup_project } shared_examples 'saves project tree successfully' do @@ -118,6 +120,13 @@ RSpec.describe Gitlab::ImportExport::Project::TreeSaver, :with_license, feature_ expect(reviewer).not_to be_nil expect(reviewer['user_id']).to eq(user.id) end + + it 'has merge requests system notes' do + system_notes = subject.first['notes'].select { |note| note['system'] } + + expect(system_notes.size).to eq(1) + expect(system_notes.first['note']).to eq('merged') + end end context 'with snippets' do @@ -492,6 +501,9 @@ RSpec.describe Gitlab::ImportExport::Project::TreeSaver, :with_license, feature_ create(:milestone, project: project) discussion_note = create(:discussion_note, noteable: issue, project: project) mr_note = create(:note, noteable: merge_request, project: project) + create(:system_note, noteable: merge_request, project: project, author: user, note: 'merged') + private_system_note = "mentioned in merge request #{private_mr.to_reference(project)}" + create(:system_note, noteable: merge_request, project: project, author: user, note: private_system_note) create(:note, noteable: snippet, project: project) create(:note_on_commit, author: user, diff --git a/spec/models/concerns/ci/artifactable_spec.rb b/spec/models/concerns/ci/artifactable_spec.rb index 6af244a5a0f..f61fb74fa4e 100644 --- a/spec/models/concerns/ci/artifactable_spec.rb +++ b/spec/models/concerns/ci/artifactable_spec.rb @@ -36,6 +36,21 @@ RSpec.describe Ci::Artifactable do expect { |b| artifact.each_blob(&b) }.to yield_control.exactly(3).times end end + + context 'when decompressed artifact size validator fails' do + let(:artifact) { build(:ci_job_artifact, :junit) } + + before do + allow_next_instance_of(Gitlab::Ci::DecompressedGzipSizeValidator) do |instance| + allow(instance).to receive(:valid?).and_return(false) + end + end + + it 'fails on blob' do + expect { |b| artifact.each_blob(&b) } + .to raise_error(::Gitlab::Ci::Artifacts::DecompressedArtifactSizeValidator::FileDecompressionError) + end + end end context 'when file format is raw' do diff --git a/spec/models/concerns/exportable_spec.rb b/spec/models/concerns/exportable_spec.rb index 74709b06403..4f36fa52c6b 100644 --- a/spec/models/concerns/exportable_spec.rb +++ b/spec/models/concerns/exportable_spec.rb @@ -9,6 +9,7 @@ RSpec.describe Exportable, feature_category: :importers do let_it_be(:issue) { create(:issue, project: project, milestone: milestone) } let_it_be(:note1) { create(:system_note, project: project, noteable: issue) } let_it_be(:note2) { create(:system_note, project: project, noteable: issue) } + let_it_be(:options) { { include: [{ notes: { only: [:note] }, milestone: { only: :title } }] } } let_it_be(:model_klass) do Class.new(ApplicationRecord) do @@ -28,19 +29,27 @@ RSpec.describe Exportable, feature_category: :importers do subject { model_klass.new } - describe '.readable_records' do + describe '.to_authorized_json' do let_it_be(:model_record) { model_klass.new } - context 'when model does not respond to association name' do - it 'returns nil' do - expect(subject.readable_records(:foo, current_user: user)).to be_nil + context 'when key to authorize is not an association name' do + it 'returns string without given key' do + expect(subject.to_authorized_json([:foo], user, options)).not_to include('foo') end end - context 'when model does respond to association name' do + context 'when key to authorize is an association name' do + let(:key_to_authorize) { :notes } + + subject(:record_json) { model_record.to_authorized_json([key_to_authorize], user, options) } + context 'when there are no records' do - it 'returns nil' do - expect(model_record.readable_records(:notes, current_user: user)).to be_nil + before do + allow(model_record).to receive(:notes).and_return(Note.none) + end + + it 'returns string including the empty association' do + expect(record_json).to include("\"notes\":[]") end end @@ -57,8 +66,9 @@ RSpec.describe Exportable, feature_category: :importers do end end - it 'returns collection of readable records' do - expect(model_record.readable_records(:notes, current_user: user)).to contain_exactly(note1, note2) + it 'returns string containing all records' do + expect(record_json) + .to include("\"notes\":[{\"note\":\"#{note1.note}\"},{\"note\":\"#{note2.note}\"}]") end end @@ -70,8 +80,19 @@ RSpec.describe Exportable, feature_category: :importers do end end - it 'returns collection of readable records' do - expect(model_record.readable_records(:notes, current_user: user)).to eq([]) + it 'returns string including the empty association' do + expect(record_json).to include("\"notes\":[]") + end + end + + context 'when user can read some records' do + before do + allow(model_record).to receive(:readable_records).with(:notes, current_user: user) + .and_return([note1]) + end + + it 'returns string containing readable records only' do + expect(record_json).to include("\"notes\":[{\"note\":\"#{note1.note}\"}]") end end end @@ -87,13 +108,15 @@ RSpec.describe Exportable, feature_category: :importers do it 'calls #readable_by?' do expect(note1).to receive(:readable_by?).with(user) - model_record.readable_records(:notes, current_user: user) + record_json end end context 'with single relation' do + let(:key_to_authorize) { :milestone } + before do - allow(model_record).to receive(:try).with(:milestone).and_return(issue.milestone) + allow(model_record).to receive(:milestone).and_return(issue.milestone) end context 'when user can read the record' do @@ -101,8 +124,8 @@ RSpec.describe Exportable, feature_category: :importers do allow(milestone).to receive(:readable_by?).with(user).and_return(true) end - it 'returns collection of readable records' do - expect(model_record.readable_records(:milestone, current_user: user)).to eq(milestone) + it 'returns string including association' do + expect(record_json).to include("\"milestone\":{\"title\":\"#{milestone.title}\"}") end end @@ -111,8 +134,8 @@ RSpec.describe Exportable, feature_category: :importers do allow(milestone).to receive(:readable_by?).with(user).and_return(false) end - it 'returns collection of readable records' do - expect(model_record.readable_records(:milestone, current_user: user)).to be_nil + it 'returns string with null association' do + expect(record_json).to include("\"milestone\":null") end end end @@ -211,26 +234,4 @@ RSpec.describe Exportable, feature_category: :importers do end end end - - describe '.has_many_association?' do - let(:model_associations) { [:notes, :labels] } - - context 'when association type is `has_many`' do - it 'returns true' do - expect(subject.has_many_association?(:notes)).to eq(true) - end - end - - context 'when association type is `has_one`' do - it 'returns true' do - expect(subject.has_many_association?(:milestone)).to eq(false) - end - end - - context 'when association type is `belongs_to`' do - it 'returns true' do - expect(subject.has_many_association?(:project)).to eq(false) - end - end - end end diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index e11c7e98287..42b70cbb858 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -1057,4 +1057,22 @@ RSpec.describe Issuable do end end end + + context 'with exportable associations' do + let_it_be(:project) { create(:project, group: create(:group, :private)) } + + context 'for issues' do + let_it_be_with_reload(:resource) { create(:issue, project: project) } + + it_behaves_like 'an exportable' + end + + context 'for merge requests' do + let_it_be_with_reload(:resource) do + create(:merge_request, source_project: project, project: project) + end + + it_behaves_like 'an exportable' + end + end end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index 26a1edcbcff..65e02da2b5d 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -44,6 +44,122 @@ RSpec.describe Label do is_expected.to allow_value("customer's request").for(:title) is_expected.to allow_value('s' * 255).for(:title) end + + describe 'description length' do + let(:invalid_description) { 'x' * (::Label::DESCRIPTION_LENGTH_MAX + 1) } + let(:valid_description) { 'short description' } + let(:label) { build(:label, project: project, description: description) } + + let(:error_message) do + format( + _('is too long (%{size}). The maximum size is %{max_size}.'), + size: ActiveSupport::NumberHelper.number_to_human_size(invalid_description.bytesize), + max_size: ActiveSupport::NumberHelper.number_to_human_size(::Label::DESCRIPTION_LENGTH_MAX) + ) + end + + subject(:validate) { label.validate } + + context 'when label is a new record' do + context 'when description exceeds the maximum size' do + let(:description) { invalid_description } + + it 'adds a description too long error' do + validate + + expect(label.errors[:description]).to contain_exactly(error_message) + end + end + + context 'when description is within the allowed limits' do + let(:description) { valid_description } + + it 'does not add a validation error' do + validate + + expect(label.errors).not_to have_key(:description) + end + end + end + + context 'when label is an existing record' do + before do + label.description = existing_description + label.save!(validate: false) + label.description = description + end + + context 'when record already had a valid description' do + let(:existing_description) { 'small difference so it triggers description_changed?' } + + context 'when new description exceeds the maximum size' do + let(:description) { invalid_description } + + it 'adds a description too long error' do + validate + + expect(label.errors[:description]).to contain_exactly(error_message) + end + end + + context 'when new description is within the allowed limits' do + let(:description) { valid_description } + + it 'does not add a validation error' do + validate + + expect(label.errors).not_to have_key(:description) + end + end + end + + context 'when record existed with an invalid description' do + let(:existing_description) { "#{invalid_description} small difference so it triggers description_changed?" } + + context 'when description is not changed' do + let(:description) { existing_description } + + it 'does not add a validation error' do + validate + + expect(label.errors).not_to have_key(:description) + end + end + + context 'when new description exceeds the maximum size' do + context 'when new description is shorter than existing description' do + let(:description) { invalid_description } + + it 'allows updating descriptions that already existed above the limit' do + validate + + expect(label.errors).not_to have_key(:description) + end + end + + context 'when new description is longer than existing description' do + let(:description) { "#{existing_description}1" } + + it 'adds a description too long error' do + validate + + expect(label.errors[:description]).to contain_exactly(error_message) + end + end + end + + context 'when new description is within the allowed limits' do + let(:description) { valid_description } + + it 'does not add a validation error' do + validate + + expect(label.errors).not_to have_key(:description) + end + end + end + end + end end describe 'scopes' do diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 20dae056646..67af7a049cb 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -106,36 +106,6 @@ RSpec.describe ProjectMember do end end - describe '.import_team' do - before do - @project_1 = create(:project) - @project_2 = create(:project) - - @user_1 = create :user - @user_2 = create :user - - @project_1.add_developer(@user_1) - @project_2.add_reporter(@user_2) - - @status = @project_2.team.import(@project_1) - end - - it { expect(@status).to be_truthy } - - describe 'project 2 should get user 1 as developer. user_2 should not be changed' do - it { expect(@project_2.users).to include(@user_1) } - it { expect(@project_2.users).to include(@user_2) } - - it { expect(Ability.allowed?(@user_1, :create_project, @project_2)).to be_truthy } - it { expect(Ability.allowed?(@user_2, :read_project, @project_2)).to be_truthy } - end - - describe 'project 1 should not be changed' do - it { expect(@project_1.users).to include(@user_1) } - it { expect(@project_1.users).not_to include(@user_2) } - end - end - describe '.truncate_teams' do before do @project_1 = create(:project) diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index f4cf3130aa9..aca6f7d053f 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -164,6 +164,57 @@ RSpec.describe ProjectTeam, feature_category: :subgroups do end end + describe '#import_team' do + let_it_be(:source_project) { create(:project) } + let_it_be(:target_project) { create(:project) } + let_it_be(:source_project_owner) { source_project.first_owner } + let_it_be(:source_project_developer) { create(:user) { |user| source_project.add_developer(user) } } + let_it_be(:current_user) { create(:user) { |user| target_project.add_maintainer(user) } } + + subject(:import) { target_project.team.import(source_project, current_user) } + + it { is_expected.to be_truthy } + + it 'target project includes source member with the same access' do + import + + imported_member_access = target_project.members.find_by!(user: source_project_developer).access_level + expect(imported_member_access).to eq(Gitlab::Access::DEVELOPER) + end + + it 'does not change the source project members' do + import + + expect(source_project.users).to include(source_project_developer) + expect(source_project.users).not_to include(current_user) + end + + shared_examples 'imports source owners with correct access' do + specify do + import + + source_owner_access_in_target = target_project.members.find_by!(user: source_project_owner).access_level + expect(source_owner_access_in_target).to eq(target_access_level) + end + end + + context 'when importer is a maintainer in target project' do + it_behaves_like 'imports source owners with correct access' do + let(:target_access_level) { Gitlab::Access::MAINTAINER } + end + end + + context 'when importer is an owner in target project' do + before do + target_project.add_owner(current_user) + end + + it_behaves_like 'imports source owners with correct access' do + let(:target_access_level) { Gitlab::Access::OWNER } + end + end + end + describe '#find_member' do context 'personal project' do let(:project) do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c73dac7251e..a643dd0b4e5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -684,34 +684,116 @@ RSpec.describe User, feature_category: :user_profile do end end - describe '#commit_email=' do - subject(:user) { create(:user) } + shared_examples 'for user notification, public, and commit emails' do + context 'when confirmed primary email' do + let(:user) { create(:user) } + let(:email) { user.email } - it 'can be set to a confirmed email' do - confirmed = create(:email, :confirmed, user: user) - user.commit_email = confirmed.email + it 'can be set' do + set_email - expect(user).to be_valid + expect(user).to be_valid + end + + context 'when primary email is changed' do + before do + user.email = generate(:email) + end + + it 'can not be set' do + set_email + + expect(user).not_to be_valid + end + end + + context 'when confirmed secondary email' do + let(:email) { create(:email, :confirmed, user: user).email } + + it 'can be set' do + set_email + + expect(user).to be_valid + end + end + + context 'when unconfirmed secondary email' do + let(:email) { create(:email, user: user).email } + + it 'can not be set' do + set_email + + expect(user).not_to be_valid + end + end + + context 'when invalid confirmed secondary email' do + let(:email) { create(:email, :confirmed, :skip_validate, user: user, email: 'invalid') } + + it 'can not be set' do + set_email + + expect(user).not_to be_valid + end + end end - it 'can not be set to an unconfirmed email' do - unconfirmed = create(:email, user: user) - user.commit_email = unconfirmed.email + context 'when unconfirmed primary email ' do + let(:user) { create(:user, :unconfirmed) } + let(:email) { user.email } - expect(user).not_to be_valid + it 'can not be set' do + set_email + + expect(user).not_to be_valid + end end - it 'can not be set to a non-existent email' do - user.commit_email = 'non-existent-email@nonexistent.nonexistent' + context 'when new record' do + let(:user) { build(:user, :unconfirmed) } + let(:email) { user.email } - expect(user).not_to be_valid + it 'can not be set' do + set_email + + expect(user).not_to be_valid + end + + context 'when skipping confirmation' do + before do + user.skip_confirmation = true + end + + it 'can be set' do + set_email + + expect(user).to be_valid + end + end end + end - it 'can not be set to an invalid email, even if confirmed' do - confirmed = create(:email, :confirmed, :skip_validate, user: user, email: 'invalid') - user.commit_email = confirmed.email + describe 'notification_email' do + include_examples 'for user notification, public, and commit emails' do + subject(:set_email) do + user.notification_email = email + end + end + end - expect(user).not_to be_valid + describe 'public_email' do + include_examples 'for user notification, public, and commit emails' do + subject(:set_email) do + user.public_email = email + end + end + end + + describe 'commit_email' do + include_examples 'for user notification, public, and commit emails' do + subject(:set_email) do + user.commit_email = email + end end end @@ -3532,15 +3614,40 @@ RSpec.describe User, feature_category: :user_profile do describe '#verified_emails' do let(:user) { create(:user) } + let!(:confirmed_email) { create(:email, :confirmed, user: user) } - it 'returns only confirmed emails' do - email_confirmed = create :email, user: user, confirmed_at: Time.current - create :email, user: user + before do + create(:email, user: user) + end + it 'returns only confirmed emails' do expect(user.verified_emails).to contain_exactly( user.email, user.private_commit_email, - email_confirmed.email + confirmed_email.email + ) + end + + it 'does not return primary email when primary email is changed' do + original_email = user.email + user.email = generate(:email) + + expect(user.verified_emails).to contain_exactly( + user.private_commit_email, + confirmed_email.email, + original_email + ) + end + + it 'does not return unsaved primary email even if skip_confirmation is enabled' do + original_email = user.email + user.skip_confirmation = true + user.email = generate(:email) + + expect(user.verified_emails).to contain_exactly( + user.private_commit_email, + confirmed_email.email, + original_email ) end end diff --git a/spec/requests/abuse_reports_controller_spec.rb b/spec/requests/abuse_reports_controller_spec.rb index 4b81394aea3..0b9cf24230d 100644 --- a/spec/requests/abuse_reports_controller_spec.rb +++ b/spec/requests/abuse_reports_controller_spec.rb @@ -19,43 +19,6 @@ RSpec.describe AbuseReportsController, feature_category: :insider_threat do sign_in(reporter) end - describe 'GET new' do - let(:ref_url) { 'http://example.com' } - - it 'sets the instance variables' do - get new_abuse_report_path(user_id: user.id, ref_url: ref_url) - - expect(assigns(:abuse_report)).to be_kind_of(AbuseReport) - expect(assigns(:abuse_report)).to have_attributes( - user_id: user.id, - reported_from_url: ref_url - ) - end - - context 'when the user has already been deleted' do - it 'redirects the reporter to root_path' do - user_id = user.id - user.destroy! - - get new_abuse_report_path(user_id: user_id) - - expect(response).to redirect_to root_path - expect(flash[:alert]).to eq(_('Cannot create the abuse report. The user has been deleted.')) - end - end - - context 'when the user has already been blocked' do - it 'redirects the reporter to the user\'s profile' do - user.block - - get new_abuse_report_path(user_id: user.id) - - expect(response).to redirect_to user - expect(flash[:alert]).to eq(_('Cannot create the abuse report. This user has been blocked.')) - end - end - end - describe 'POST add_category', :aggregate_failures do subject(:request) { post add_category_abuse_reports_path, params: request_params } diff --git a/spec/requests/api/npm_instance_packages_spec.rb b/spec/requests/api/npm_instance_packages_spec.rb index 591a8ee68dc..97de7fa9e52 100644 --- a/spec/requests/api/npm_instance_packages_spec.rb +++ b/spec/requests/api/npm_instance_packages_spec.rb @@ -13,11 +13,12 @@ RSpec.describe API::NpmInstancePackages, feature_category: :package_registry do describe 'GET /api/v4/packages/npm/*package_name' do let(:url) { api("/packages/npm/#{package_name}") } + subject { get(url) } + it_behaves_like 'handling get metadata requests', scope: :instance + it_behaves_like 'rejects invalid package names' context 'with a duplicate package name in another project' do - subject { get(url) } - let_it_be(:project2) { create(:project, :public, namespace: namespace) } let_it_be(:package2) do create(:npm_package, diff --git a/spec/requests/api/npm_project_packages_spec.rb b/spec/requests/api/npm_project_packages_spec.rb index 1f5ebc80824..d673645c51a 100644 --- a/spec/requests/api/npm_project_packages_spec.rb +++ b/spec/requests/api/npm_project_packages_spec.rb @@ -23,6 +23,9 @@ RSpec.describe API::NpmProjectPackages, feature_category: :package_registry do it_behaves_like 'handling get metadata requests', scope: :project it_behaves_like 'accept get request on private project with access to package registry for everyone' + it_behaves_like 'rejects invalid package names' do + subject { get(url) } + end end describe 'GET /api/v4/projects/:id/packages/npm/-/package/*package_name/dist-tags' do diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index 2bd23340a88..a59ca5211ed 100644 --- a/spec/routing/routing_spec.rb +++ b/spec/routing/routing_spec.rb @@ -351,14 +351,6 @@ RSpec.describe InvitesController, 'routing' do end end -RSpec.describe AbuseReportsController, 'routing' do - let_it_be(:user) { create(:user) } - - it 'to #new' do - expect(get("/-/abuse_reports/new?user_id=#{user.id}")).to route_to('abuse_reports#new', user_id: user.id.to_s) - end -end - RSpec.describe SentNotificationsController, 'routing' do it 'to #unsubscribe' do expect(get("/-/sent_notifications/4bee17d4a63ed60cf5db53417e9aeb4c/unsubscribe")) diff --git a/spec/support/shared_examples/features/reportable_note_shared_examples.rb b/spec/support/shared_examples/features/reportable_note_shared_examples.rb index 133da230bed..264cc9c798b 100644 --- a/spec/support/shared_examples/features/reportable_note_shared_examples.rb +++ b/spec/support/shared_examples/features/reportable_note_shared_examples.rb @@ -6,7 +6,6 @@ RSpec.shared_examples 'reportable note' do |type| let(:comment) { find("##{ActionView::RecordIdentifier.dom_id(note)}") } let(:more_actions_selector) { '.more-actions.dropdown' } - let(:abuse_report_path) { new_abuse_report_path(user_id: note.author.id, ref_url: noteable_note_url(note)) } it 'has an edit button' do expect(comment).to have_selector('.js-note-edit') diff --git a/spec/support/shared_examples/models/exportable_shared_examples.rb b/spec/support/shared_examples/models/exportable_shared_examples.rb index 37c3e68fd5f..57e231e4a6e 100644 --- a/spec/support/shared_examples/models/exportable_shared_examples.rb +++ b/spec/support/shared_examples/models/exportable_shared_examples.rb @@ -1,27 +1,28 @@ # frozen_string_literal: true -RSpec.shared_examples 'resource with exportable associations' do - before do - stub_licensed_features(stubbed_features) if stubbed_features.any? - end +RSpec.shared_examples 'an exportable' do |restricted_association: :project| + let_it_be(:user) { create(:user) } describe '#exportable_association?' do - let(:association) { single_association } + let(:association) { restricted_association } subject { resource.exportable_association?(association, current_user: user) } it { is_expected.to be_falsey } - context 'when user can read resource' do + context 'when user can only read resource' do before do - group.add_developer(user) + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?) + .with(user, :"read_#{resource.to_ability_name}", resource) + .and_return(true) end it { is_expected.to be_falsey } context "when user can read resource's association" do before do - other_group.add_developer(user) + allow(resource).to receive(:readable_record?).with(anything, user).and_return(true) end it { is_expected.to be_truthy } @@ -31,41 +32,48 @@ RSpec.shared_examples 'resource with exportable associations' do it { is_expected.to be_falsey } end - - context 'for an unauthenticated user' do - let(:user) { nil } - - it { is_expected.to be_falsey } - end end end end - describe '#readable_records' do - subject { resource.readable_records(association, current_user: user) } + describe '#to_authorized_json' do + let(:options) { { include: [{ notes: { only: [:id] } }] } } + + subject { resource.to_authorized_json(keys, user, options) } before do - group.add_developer(user) + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?) + .with(user, :"read_#{resource.to_ability_name}", resource) + .and_return(true) end context 'when association not supported' do - let(:association) { :foo } + let(:keys) { [:foo] } - it { is_expected.to be_nil } + it { is_expected.not_to include('foo') } end context 'when association is `:notes`' do - let(:association) { :notes } + let_it_be(:readable_note) { create(:system_note, noteable: resource, project: project, note: 'readable') } + let_it_be(:restricted_note) { create(:system_note, noteable: resource, project: project, note: 'restricted') } - it { is_expected.to match_array([readable_note]) } + let(:restricted_note_access) { false } + let(:keys) { [:notes] } - context 'when user have access' do - before do - other_group.add_developer(user) - end + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :read_note, readable_note).and_return(true) + allow(Ability).to receive(:allowed?).with(user, :read_note, restricted_note).and_return(restricted_note_access) + end + + it { is_expected.to include("\"notes\":[{\"id\":#{readable_note.id}}]") } + + context 'when user have access to all notes' do + let(:restricted_note_access) { true } - it 'returns all records' do - is_expected.to match_array([readable_note, restricted_note]) + it 'string includes all notes' do + is_expected.to include("\"notes\":[{\"id\":#{readable_note.id}},{\"id\":#{restricted_note.id}}]") end end end diff --git a/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb b/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb index f53532d00d7..f430db61976 100644 --- a/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb @@ -849,3 +849,14 @@ RSpec.shared_examples 'handling different package names, visibilities and user r it_behaves_like example_name, status: status end end + +RSpec.shared_examples 'rejects invalid package names' do + let(:package_name) { "%0d%0ahttp:/%2fexample.com" } + + it do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(Gitlab::Json.parse(response.body)).to eq({ 'error' => 'package_name should be a valid file path' }) + end +end |