From 2fd92f2dc784ade9cb4e1c33dd60cbfad7b86818 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 4 Mar 2020 21:07:54 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- .../groups/group_links_controller_spec.rb | 26 ++++++- spec/controllers/snippets_controller_spec.rb | 10 ++- spec/features/issues/issue_detail_spec.rb | 2 +- .../user_creates_merge_request_spec.rb | 24 +++++- .../projects/wiki/user_updates_wiki_page_spec.rb | 7 +- .../lib/gitlab/import_export/complex/project.json | 6 +- .../components/error_details_spec.js | 22 ++++++ spec/graphql/types/diff_refs_type_spec.rb | 6 +- spec/lib/gitlab/asset_proxy_spec.rb | 50 +++++++++++++ ...ect_authorizations_with_min_max_user_id_spec.rb | 38 ++++++++++ .../gitlab/dependency_linker/base_linker_spec.rb | 53 +++++++++++++ .../import_export/project/tree_restorer_spec.rb | 22 +++++- spec/lib/gitlab/project_authorizations_spec.rb | 14 ++++ spec/lib/gitlab/user_access_spec.rb | 11 +++ spec/lib/gitlab/utils_spec.rb | 14 ++++ spec/migrations/clean_grafana_url_spec.rb | 37 +++++++++ ...ulate_project_authorizations_second_run_spec.rb | 28 +++++++ spec/models/application_setting_spec.rb | 48 ++++++++++++ spec/models/badge_spec.rb | 16 ++++ spec/models/group_spec.rb | 39 ++++++++++ spec/models/members/group_member_spec.rb | 21 +++++- spec/models/project_spec.rb | 32 ++++++++ spec/models/user_detail_spec.rb | 2 +- spec/presenters/ci/pipeline_presenter_spec.rb | 87 +++++++++++++++++++++- spec/requests/api/triggers_spec.rb | 12 +++ ...ntainer_registry_authentication_service_spec.rb | 44 +++++++++++ .../groups/group_links/destroy_service_spec.rb | 15 +--- .../groups/group_links/update_service_spec.rb | 59 +++++++++++++++ .../lfs_pointers/lfs_download_service_spec.rb | 27 ++++--- .../lfs_object_download_list_service_spec.rb | 33 -------- .../policies/group_policy_shared_context.rb | 1 + .../controllers/uploads_actions_shared_examples.rb | 30 +++++++- spec/uploaders/file_mover_spec.rb | 24 +++--- spec/uploaders/file_uploader_spec.rb | 44 +++++++++-- spec/uploaders/personal_file_uploader_spec.rb | 7 +- spec/validators/addressable_url_validator_spec.rb | 16 ++++ 36 files changed, 818 insertions(+), 109 deletions(-) create mode 100644 spec/lib/gitlab/asset_proxy_spec.rb create mode 100644 spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb create mode 100644 spec/lib/gitlab/dependency_linker/base_linker_spec.rb create mode 100644 spec/migrations/clean_grafana_url_spec.rb create mode 100644 spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb create mode 100644 spec/services/groups/group_links/update_service_spec.rb (limited to 'spec') diff --git a/spec/controllers/groups/group_links_controller_spec.rb b/spec/controllers/groups/group_links_controller_spec.rb index c062de468fc..21169188386 100644 --- a/spec/controllers/groups/group_links_controller_spec.rb +++ b/spec/controllers/groups/group_links_controller_spec.rb @@ -6,9 +6,13 @@ describe Groups::GroupLinksController do let(:shared_with_group) { create(:group, :private) } let(:shared_group) { create(:group, :private) } let(:user) { create(:user) } + let(:group_member) { create(:user) } + let!(:project) { create(:project, group: shared_group) } before do sign_in(user) + + shared_with_group.add_developer(group_member) end describe '#create' do @@ -40,13 +44,9 @@ describe Groups::GroupLinksController do end context 'when user has correct access to both groups' do - let(:group_member) { create(:user) } - before do shared_with_group.add_developer(user) shared_group.add_owner(user) - - shared_with_group.add_developer(group_member) end context 'when default access level is requested' do @@ -56,6 +56,10 @@ describe Groups::GroupLinksController do context 'when owner access is requested' do let(:shared_group_access) { Gitlab::Access::OWNER } + before do + shared_with_group.add_owner(group_member) + end + include_examples 'creates group group link' it 'allows admin access for group member' do @@ -64,6 +68,10 @@ describe Groups::GroupLinksController do end end + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:read_project, project) }.from(false).to(true) + end + context 'when shared with group id is not present' do let(:shared_with_group_id) { nil } @@ -149,6 +157,7 @@ describe Groups::GroupLinksController do context 'when user has admin access to the shared group' do before do shared_group.add_owner(user) + shared_with_group.refresh_members_authorized_projects end it 'updates existing link' do @@ -162,6 +171,10 @@ describe Groups::GroupLinksController do expect(link.group_access).to eq(Gitlab::Access::GUEST) expect(link.expires_at).to eq(expiry_date) end + + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false) + end end context 'when user does not have admin access to the shared group' do @@ -199,11 +212,16 @@ describe Groups::GroupLinksController do context 'when user has admin access to the shared group' do before do shared_group.add_owner(user) + shared_with_group.refresh_members_authorized_projects end it 'deletes existing link' do expect { subject }.to change(GroupGroupLink, :count).by(-1) end + + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false) + end end context 'when user does not have admin access to the shared group' do diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index 16947f4d3be..a675014a77b 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -260,8 +260,10 @@ describe SnippetsController do context 'when the snippet description contains a file' do include FileMoverHelpers - let(:picture_file) { "/-/system/user/#{user.id}/secret56/picture.jpg" } - let(:text_file) { "/-/system/user/#{user.id}/secret78/text.txt" } + let(:picture_secret) { SecureRandom.hex } + let(:text_secret) { SecureRandom.hex } + let(:picture_file) { "/-/system/user/#{user.id}/#{picture_secret}/picture.jpg" } + let(:text_file) { "/-/system/user/#{user.id}/#{text_secret}/text.txt" } let(:description) do "Description with picture: ![picture](/uploads#{picture_file}) and "\ "text: [text.txt](/uploads#{text_file})" @@ -284,8 +286,8 @@ describe SnippetsController do snippet = subject expected_description = "Description with picture: "\ - "![picture](/uploads/-/system/personal_snippet/#{snippet.id}/secret56/picture.jpg) and "\ - "text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/secret78/text.txt)" + "![picture](/uploads/-/system/personal_snippet/#{snippet.id}/#{picture_secret}/picture.jpg) and "\ + "text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/#{text_secret}/text.txt)" expect(snippet.description).to eq(expected_description) end diff --git a/spec/features/issues/issue_detail_spec.rb b/spec/features/issues/issue_detail_spec.rb index 0d24b02a64c..3bb70fdf376 100644 --- a/spec/features/issues/issue_detail_spec.rb +++ b/spec/features/issues/issue_detail_spec.rb @@ -28,7 +28,7 @@ describe 'Issue Detail', :js do visit project_issue_path(project, issue) end - it 'encodes the description to prevent xss issues' do + it 'encodes the description to prevent xss issues', quarantine: 'https://gitlab.com/gitlab-org/gitlab/issues/207951' do page.within('.issuable-details .detail-page-description') do image = find('img.js-lazy-loaded') diff --git a/spec/features/merge_request/user_creates_merge_request_spec.rb b/spec/features/merge_request/user_creates_merge_request_spec.rb index 67f6d8ebe32..86ee9fa5aa5 100644 --- a/spec/features/merge_request/user_creates_merge_request_spec.rb +++ b/spec/features/merge_request/user_creates_merge_request_spec.rb @@ -5,9 +5,9 @@ require "spec_helper" describe "User creates a merge request", :js do include ProjectForksHelper + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } let(:title) { "Some feature" } - let(:project) { create(:project, :repository) } - let(:user) { create(:user) } before do project.add_maintainer(user) @@ -38,6 +38,26 @@ describe "User creates a merge request", :js do end end + context "XSS branch name exists" do + before do + project.repository.create_branch("", "master") + end + + it "doesn't execute the dodgy branch name" do + visit(project_new_merge_request_path(project)) + + find(".js-source-branch").click + click_link("") + + find(".js-target-branch").click + click_link("feature") + + click_button("Compare branches") + + expect { page.driver.browser.switch_to.alert }.to raise_error(Selenium::WebDriver::Error::NoSuchAlertError) + end + end + context "to a forked project" do let(:forked_project) { fork_project(project, user, namespace: user.namespace, repository: true) } diff --git a/spec/features/projects/wiki/user_updates_wiki_page_spec.rb b/spec/features/projects/wiki/user_updates_wiki_page_spec.rb index d3a0c9b790b..9d9c83331fb 100644 --- a/spec/features/projects/wiki/user_updates_wiki_page_spec.rb +++ b/spec/features/projects/wiki/user_updates_wiki_page_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe 'User updates wiki page' do + include WikiHelpers + let(:user) { create(:user) } before do @@ -11,8 +13,11 @@ describe 'User updates wiki page' do end context 'when wiki is empty' do - before do + before do |example| visit(project_wikis_path(project)) + + wait_for_svg_to_be_loaded(example) + click_link "Create your first page" end diff --git a/spec/fixtures/lib/gitlab/import_export/complex/project.json b/spec/fixtures/lib/gitlab/import_export/complex/project.json index fae02540868..72ed8b818d8 100644 --- a/spec/fixtures/lib/gitlab/import_export/complex/project.json +++ b/spec/fixtures/lib/gitlab/import_export/complex/project.json @@ -6452,7 +6452,7 @@ ] }, { - "id": 37, + "id": 26, "project_id": 5, "ref": "master", "sha": "048721d90c449b244b7b4c53a9186b04330174ec", @@ -6744,7 +6744,7 @@ ] }, { - "id": 40, + "id": 19, "project_id": 5, "ref": "master", "sha": "2ea1f3dec713d940208fb5ce4a38765ecb5d3f73", @@ -6851,7 +6851,7 @@ ] }, { - "id": 42, + "id": 20, "project_id": 5, "ref": "master", "sha": "ce84140e8b878ce6e7c4d298c7202ff38170e3ac", diff --git a/spec/frontend/error_tracking/components/error_details_spec.js b/spec/frontend/error_tracking/components/error_details_spec.js index e43f9569ffc..2baec5f37fb 100644 --- a/spec/frontend/error_tracking/components/error_details_spec.js +++ b/spec/frontend/error_tracking/components/error_details_spec.js @@ -137,6 +137,28 @@ describe('ErrorDetails', () => { expect(wrapper.findAll(GlButton).length).toBe(3); }); + describe('unsafe chars for culprit field', () => { + const findReportedText = () => wrapper.find('[data-qa-selector="reported_text"]'); + const culprit = ''; + beforeEach(() => { + store.state.details.loadingStacktrace = false; + wrapper.setData({ + error: { + culprit, + }, + }); + }); + + it('should not convert interpolated text to html entities', () => { + expect(findReportedText().findAll('script').length).toEqual(0); + expect(findReportedText().findAll('strong').length).toEqual(1); + }); + + it('should render text instead of converting to html entities', () => { + expect(findReportedText().text()).toContain(culprit); + }); + }); + describe('Badges', () => { it('should show language and error level badges', () => { wrapper.setData({ diff --git a/spec/graphql/types/diff_refs_type_spec.rb b/spec/graphql/types/diff_refs_type_spec.rb index 91017c827ad..85225e5809c 100644 --- a/spec/graphql/types/diff_refs_type_spec.rb +++ b/spec/graphql/types/diff_refs_type_spec.rb @@ -5,5 +5,9 @@ require 'spec_helper' describe GitlabSchema.types['DiffRefs'] do it { expect(described_class.graphql_name).to eq('DiffRefs') } - it { expect(described_class).to have_graphql_fields(:base_sha, :head_sha, :start_sha) } + it { is_expected.to have_graphql_fields(:head_sha, :base_sha, :start_sha).only } + + it { expect(described_class.fields['headSha'].type).to be_non_null } + it { expect(described_class.fields['baseSha'].type).not_to be_non_null } + it { expect(described_class.fields['startSha'].type).to be_non_null } end diff --git a/spec/lib/gitlab/asset_proxy_spec.rb b/spec/lib/gitlab/asset_proxy_spec.rb new file mode 100644 index 00000000000..f5aa1819982 --- /dev/null +++ b/spec/lib/gitlab/asset_proxy_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::AssetProxy do + context 'when asset proxy is disabled' do + before do + stub_asset_proxy_setting(enabled: false) + end + + it 'returns the original URL' do + url = 'http://example.com/test.png' + + expect(described_class.proxy_url(url)).to eq(url) + end + end + + context 'when asset proxy is enabled' do + before do + stub_asset_proxy_setting(whitelist: %w(gitlab.com *.mydomain.com)) + stub_asset_proxy_setting( + enabled: true, + url: 'https://assets.example.com', + secret_key: 'shared-secret', + domain_regexp: Banzai::Filter::AssetProxyFilter.compile_whitelist(Gitlab.config.asset_proxy.whitelist) + ) + end + + it 'returns a proxied URL' do + url = 'http://example.com/test.png' + proxied_url = 'https://assets.example.com/08df250eeeef1a8cf2c761475ac74c5065105612/687474703a2f2f6578616d706c652e636f6d2f746573742e706e67' + + expect(described_class.proxy_url(url)).to eq(proxied_url) + end + + context 'whitelisted domain' do + it 'returns original URL for single domain whitelist' do + url = 'http://gitlab.com/test.png' + + expect(described_class.proxy_url(url)).to eq(url) + end + + it 'returns original URL for wildcard subdomain whitelist' do + url = 'http://test.mydomain.com/test.png' + + expect(described_class.proxy_url(url)).to eq(url) + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb b/spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb new file mode 100644 index 00000000000..14ba57eecbf --- /dev/null +++ b/spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::RecalculateProjectAuthorizationsWithMinMaxUserId, :migration, schema: 20200204113224 do + let(:users_table) { table(:users) } + let(:min) { 1 } + let(:max) { 5 } + + before do + min.upto(max) do |i| + users_table.create!(id: i, email: "user#{i}@example.com", projects_limit: 10) + end + end + + describe '#perform' do + it 'initializes Users::RefreshAuthorizedProjectsService with correct users' do + min.upto(max) do |i| + user = User.find(i) + expect(Users::RefreshAuthorizedProjectsService).to( + receive(:new).with(user, any_args).and_call_original) + end + + described_class.new.perform(min, max) + end + + it 'executes Users::RefreshAuthorizedProjectsService' do + expected_call_counts = max - min + 1 + + service = instance_double(Users::RefreshAuthorizedProjectsService) + expect(Users::RefreshAuthorizedProjectsService).to( + receive(:new).exactly(expected_call_counts).times.and_return(service)) + expect(service).to receive(:execute).exactly(expected_call_counts).times + + described_class.new.perform(min, max) + end + end +end diff --git a/spec/lib/gitlab/dependency_linker/base_linker_spec.rb b/spec/lib/gitlab/dependency_linker/base_linker_spec.rb new file mode 100644 index 00000000000..1466ce2dfcc --- /dev/null +++ b/spec/lib/gitlab/dependency_linker/base_linker_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::DependencyLinker::BaseLinker do + let(:linker_class) do + Class.new(described_class) do + def link_dependencies + link_regex(%r{^(?https?://[^ ]+)}, &:itself) + end + end + end + + let(:plain_content) do + <<~CONTENT + http://\\njavascript:alert(1) + https://gitlab.com/gitlab-org/gitlab + CONTENT + end + + let(:highlighted_content) do + <<~CONTENT + http://\\njavascript:alert(1) + https://gitlab.com/gitlab-org/gitlab + CONTENT + end + + let(:linker) { linker_class.new(plain_content, highlighted_content) } + + describe '#link' do + subject { linker.link } + + it 'only converts valid links' do + expect(subject).to eq( + <<~CONTENT + #{link('http://')}#{link('\n', url: '%5Cn')}#{link('javascript:alert(1)', url: nil)} + #{link('https://gitlab.com/gitlab-org/gitlab')} + CONTENT + ) + end + end + + def link(text, url: text) + attrs = [ + 'rel="nofollow noreferrer noopener"', + 'target="_blank"' + ] + + attrs.unshift(%{href="#{url}"}) if url + + %{#{text}} + end +end diff --git a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb index a46c6579670..7bc17b804df 100644 --- a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb @@ -104,6 +104,24 @@ describe Gitlab::ImportExport::Project::TreeRestorer do expect(pipeline.merge_request.source_branch).to eq('feature_conflict') end + it 'restores pipelines based on ascending id order' do + expected_ordered_shas = %w[ + 2ea1f3dec713d940208fb5ce4a38765ecb5d3f73 + ce84140e8b878ce6e7c4d298c7202ff38170e3ac + 048721d90c449b244b7b4c53a9186b04330174ec + sha-notes + 5f923865dde3436854e9ceb9cdb7815618d4e849 + d2d430676773caa88cdaf7c55944073b2fd5561a + 2ea1f3dec713d940208fb5ce4a38765ecb5d3f73 + ] + + project = Project.find_by_path('project') + + project.ci_pipelines.order(:id).each_with_index do |pipeline, i| + expect(pipeline['sha']).to eq expected_ordered_shas[i] + end + end + it 'preserves updated_at on issues' do issue = Issue.where(description: 'Aliquam enim illo et possimus.').first @@ -385,7 +403,7 @@ describe Gitlab::ImportExport::Project::TreeRestorer do it 'has the correct number of pipelines and statuses' do expect(@project.ci_pipelines.size).to eq(7) - @project.ci_pipelines.order(:id).zip([2, 2, 2, 2, 2, 0, 0]) + @project.ci_pipelines.order(:id).zip([2, 0, 2, 2, 2, 2, 0]) .each do |(pipeline, expected_status_size)| expect(pipeline.statuses.size).to eq(expected_status_size) end @@ -422,7 +440,7 @@ describe Gitlab::ImportExport::Project::TreeRestorer do end it 'restores external pull request for the restored pipeline' do - pipeline_with_external_pr = @project.ci_pipelines.order(:id).last + pipeline_with_external_pr = @project.ci_pipelines.where(source: 'external_pull_request_event').first expect(pipeline_with_external_pr.external_pull_request).to be_persisted end diff --git a/spec/lib/gitlab/project_authorizations_spec.rb b/spec/lib/gitlab/project_authorizations_spec.rb index 1c579128223..7b282433061 100644 --- a/spec/lib/gitlab/project_authorizations_spec.rb +++ b/spec/lib/gitlab/project_authorizations_spec.rb @@ -109,6 +109,20 @@ describe Gitlab::ProjectAuthorizations do end end + context 'with lower group access level than max access level for share' do + let(:user) { create(:user) } + + it 'creates proper authorizations' do + group.add_reporter(user) + + mapping = map_access_levels(authorizations) + + expect(mapping[project_parent.id]).to be_nil + expect(mapping[project.id]).to eq(Gitlab::Access::REPORTER) + expect(mapping[project_child.id]).to eq(Gitlab::Access::REPORTER) + end + end + context 'parent group user' do let(:user) { parent_group_user } diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index 8d13f377677..78370f0136c 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -30,6 +30,17 @@ describe Gitlab::UserAccess do end end + describe 'push to branch in an internal project' do + it 'will not infinitely loop when a project is internal' do + project.visibility_level = Gitlab::VisibilityLevel::INTERNAL + project.save! + + expect(project).not_to receive(:branch_allows_collaboration?) + + access.can_push_to_branch?('master') + end + end + describe 'push to empty project' do let(:empty_project) { create(:project_empty_repo) } let(:project_access) { described_class.new(user, project: empty_project) } diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index 48fc2d826bc..d3780d22241 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/spec/lib/gitlab/utils_spec.rb @@ -291,4 +291,18 @@ describe Gitlab::Utils do expect(described_class.string_to_ip_object('1:0:0:0:0:0:0:0/124')).to eq(IPAddr.new('1:0:0:0:0:0:0:0/124')) end end + + describe '.parse_url' do + it 'returns Addressable::URI object' do + expect(described_class.parse_url('http://gitlab.com')).to be_instance_of(Addressable::URI) + end + + it 'returns nil when URI cannot be parsed' do + expect(described_class.parse_url('://gitlab.com')).to be nil + end + + it 'returns nil with invalid parameter' do + expect(described_class.parse_url(1)).to be nil + end + end end diff --git a/spec/migrations/clean_grafana_url_spec.rb b/spec/migrations/clean_grafana_url_spec.rb new file mode 100644 index 00000000000..9f060fbaf7d --- /dev/null +++ b/spec/migrations/clean_grafana_url_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20200214085940_clean_grafana_url.rb') + +describe CleanGrafanaUrl, :migration do + let(:application_settings_table) { table(:application_settings) } + + [ + 'javascript:alert(window.opener.document.location)', + ' javascript:alert(window.opener.document.location)' + ].each do |grafana_url| + it "sets grafana_url back to its default value when grafana_url is '#{grafana_url}'" do + application_settings = application_settings_table.create!(grafana_url: grafana_url) + + migrate! + + expect(application_settings.reload.grafana_url).to eq('/-/grafana') + end + end + + ['/-/grafana', '/some/relative/url', 'http://localhost:9000'].each do |grafana_url| + it "does not modify grafana_url when grafana_url is '#{grafana_url}'" do + application_settings = application_settings_table.create!(grafana_url: grafana_url) + + migrate! + + expect(application_settings.reload.grafana_url).to eq(grafana_url) + end + end + + context 'when application_settings table has no rows' do + it 'does not fail' do + migrate! + end + end +end diff --git a/spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb b/spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb new file mode 100644 index 00000000000..04726f98c89 --- /dev/null +++ b/spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20200204113224_schedule_recalculate_project_authorizations_second_run.rb') + +describe ScheduleRecalculateProjectAuthorizationsSecondRun, :migration do + let(:users_table) { table(:users) } + + before do + stub_const("#{described_class}::BATCH_SIZE", 2) + + 1.upto(4) do |i| + users_table.create!(id: i, name: "user#{i}", email: "user#{i}@example.com", projects_limit: 1) + end + end + + it 'schedules background migration' do + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + expect(BackgroundMigrationWorker.jobs.size).to eq(2) + expect(described_class::MIGRATION).to be_scheduled_migration(1, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(3, 4) + end + end + end +end diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 82ece6b2b91..9dad6b1e766 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -19,6 +19,7 @@ describe ApplicationSetting do let(:http) { 'http://example.com' } let(:https) { 'https://example.com' } let(:ftp) { 'ftp://example.com' } + let(:javascript) { 'javascript:alert(window.opener.document.location)' } it { is_expected.to allow_value(nil).for(:home_page_url) } it { is_expected.to allow_value(http).for(:home_page_url) } @@ -81,6 +82,53 @@ describe ApplicationSetting do it { is_expected.not_to allow_value('abc').for(:minimum_password_length) } it { is_expected.to allow_value(10).for(:minimum_password_length) } + context 'grafana_url validations' do + before do + subject.instance_variable_set(:@parsed_grafana_url, nil) + end + + it { is_expected.to allow_value(http).for(:grafana_url) } + it { is_expected.to allow_value(https).for(:grafana_url) } + it { is_expected.not_to allow_value(ftp).for(:grafana_url) } + it { is_expected.not_to allow_value(javascript).for(:grafana_url) } + it { is_expected.to allow_value('/-/grafana').for(:grafana_url) } + it { is_expected.to allow_value('http://localhost:9000').for(:grafana_url) } + + context 'when local URLs are not allowed in system hooks' do + before do + stub_application_setting(allow_local_requests_from_system_hooks: false) + end + + it { is_expected.not_to allow_value('http://localhost:9000').for(:grafana_url) } + end + + context 'with invalid grafana URL' do + it 'adds an error' do + subject.grafana_url = ' ' + http + expect(subject.save).to be false + + expect(subject.errors[:grafana_url]).to eq([ + 'must be a valid relative or absolute URL. ' \ + 'Please check your Grafana URL setting in ' \ + 'Admin Area > Settings > Metrics and profiling > Metrics - Grafana' + ]) + end + end + + context 'with blocked grafana URL' do + it 'adds an error' do + subject.grafana_url = javascript + expect(subject.save).to be false + + expect(subject.errors[:grafana_url]).to eq([ + 'is blocked: Only allowed schemes are http, https. Please check your ' \ + 'Grafana URL setting in ' \ + 'Admin Area > Settings > Metrics and profiling > Metrics - Grafana' + ]) + end + end + end + context 'when snowplow is enabled' do before do setting.snowplow_enabled = true diff --git a/spec/models/badge_spec.rb b/spec/models/badge_spec.rb index 60ae579eb03..fba8f40e99b 100644 --- a/spec/models/badge_spec.rb +++ b/spec/models/badge_spec.rb @@ -91,6 +91,22 @@ describe Badge do let(:method) { :image_url } it_behaves_like 'rendered_links' + + context 'when asset proxy is enabled' do + let(:placeholder_url) { 'http://www.example.com/image' } + + before do + stub_asset_proxy_setting( + enabled: true, + url: 'https://assets.example.com', + secret_key: 'shared-secret' + ) + end + + it 'returns a proxied URL' do + expect(badge.rendered_image_url).to start_with('https://assets.example.com') + end + end end end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index b5ed29189fd..576ac4393ed 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -563,6 +563,18 @@ describe Group do expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER) expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER) end + + context 'with lower group access level than max access level for share' do + let(:user) { create(:user) } + + it 'returns correct access level' do + group.add_reporter(user) + + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER) + end + end end context 'with user in the parent group' do @@ -584,6 +596,33 @@ describe Group do expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) end end + + context 'unrelated project owner' do + let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 } + let!(:group) { create(:group, id: common_id) } + let!(:unrelated_project) { create(:project, id: common_id) } + let(:user) { unrelated_project.owner } + + it 'returns correct access level' do + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + end + end + + context 'user without accepted access request' do + let!(:user) { create(:user) } + + before do + create(:group_member, :developer, :access_request, user: user, group: group) + end + + it 'returns correct access level' do + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + end + end end context 'when feature flag share_group_with_group is disabled' do diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index ad7dfac87af..9b5cce1aebf 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -65,10 +65,10 @@ describe GroupMember do end describe '#update_two_factor_requirement' do - let(:user) { build :user } - let(:group_member) { build :group_member, user: user } - it 'is called after creation and deletion' do + user = build :user + group_member = build :group_member, user: user + expect(user).to receive(:update_two_factor_requirement) group_member.save @@ -79,6 +79,21 @@ describe GroupMember do end end + describe '#after_accept_invite' do + it 'calls #update_two_factor_requirement' do + email = 'foo@email.com' + user = build(:user, email: email) + group = create(:group, require_two_factor_authentication: true) + group_member = create(:group_member, group: group, invite_token: '1234', invite_email: email) + + expect(user).to receive(:require_two_factor_authentication_from_group).and_call_original + + group_member.accept_invite!(user) + + expect(user.require_two_factor_authentication_from_group).to be_truthy + end + end + context 'access levels' do context 'with parent group' do it_behaves_like 'inherited access level as a member of entity' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index cbd55077316..56f4c68a913 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -4866,6 +4866,38 @@ describe Project do end end + context 'with cross internal project merge requests' do + let(:project) { create(:project, :repository, :internal) } + let(:forked_project) { fork_project(project, nil, repository: true) } + let(:user) { double(:user) } + + it "does not endlessly loop for internal projects with MRs to each other", :sidekiq_inline do + allow(user).to receive(:can?).and_return(true, false, true) + allow(user).to receive(:id).and_return(1) + + create( + :merge_request, + target_project: project, + target_branch: 'merge-test', + source_project: forked_project, + source_branch: 'merge-test', + allow_collaboration: true + ) + + create( + :merge_request, + target_project: forked_project, + target_branch: 'merge-test', + source_project: project, + source_branch: 'merge-test', + allow_collaboration: true + ) + + expect(user).to receive(:can?).at_most(5).times + project.branch_allows_collaboration?(user, "merge-test") + end + end + context 'with cross project merge requests' do let(:user) { create(:user) } let(:target_project) { create(:project, :repository) } diff --git a/spec/models/user_detail_spec.rb b/spec/models/user_detail_spec.rb index 3b6de5bb390..2b2bfff7be2 100644 --- a/spec/models/user_detail_spec.rb +++ b/spec/models/user_detail_spec.rb @@ -7,7 +7,7 @@ describe UserDetail do describe 'validations' do describe 'job_title' do - it { is_expected.to validate_presence_of(:job_title) } + it { is_expected.not_to validate_presence_of(:job_title) } it { is_expected.to validate_length_of(:job_title).is_at_most(200) } end end diff --git a/spec/presenters/ci/pipeline_presenter_spec.rb b/spec/presenters/ci/pipeline_presenter_spec.rb index 700d1f5cbb6..c9c4f567549 100644 --- a/spec/presenters/ci/pipeline_presenter_spec.rb +++ b/spec/presenters/ci/pipeline_presenter_spec.rb @@ -6,6 +6,7 @@ describe Ci::PipelinePresenter do include Gitlab::Routing let(:user) { create(:user) } + let(:current_user) { user } let(:project) { create(:project) } let(:pipeline) { create(:ci_pipeline, project: project) } @@ -15,7 +16,7 @@ describe Ci::PipelinePresenter do before do project.add_developer(user) - allow(presenter).to receive(:current_user) { user } + allow(presenter).to receive(:current_user) { current_user } end it 'inherits from Gitlab::View::Presenter::Delegated' do @@ -224,10 +225,90 @@ describe Ci::PipelinePresenter do describe '#all_related_merge_requests' do it 'memoizes the returned relation' do query_count = ActiveRecord::QueryRecorder.new do - 2.times { presenter.send(:all_related_merge_requests).count } + 3.times { presenter.send(:all_related_merge_requests).count } end.count - expect(query_count).to eq(1) + expect(query_count).to eq(2) + end + + context 'permissions' do + let!(:merge_request) do + create(:merge_request, project: project, source_project: project) + end + + subject(:all_related_merge_requests) do + presenter.send(:all_related_merge_requests) + end + + shared_examples 'private merge requests' do + context 'when not logged in' do + let(:current_user) {} + + it { is_expected.to be_empty } + end + + context 'when logged in as a non_member' do + let(:current_user) { create(:user) } + + it { is_expected.to be_empty } + end + + context 'when logged in as a guest' do + let(:current_user) { create(:user) } + + before do + project.add_guest(current_user) + end + + it { is_expected.to be_empty } + end + + context 'when logged in as a developer' do + it { is_expected.to contain_exactly(merge_request) } + end + + context 'when logged in as a maintainer' do + let(:current_user) { create(:user) } + + before do + project.add_maintainer(current_user) + end + + it { is_expected.to contain_exactly(merge_request) } + end + end + + context 'with a private project' do + it_behaves_like 'private merge requests' + end + + context 'with a public project with private merge requests' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + + project + .project_feature + .update!(merge_requests_access_level: ProjectFeature::PRIVATE) + end + + it_behaves_like 'private merge requests' + end + + context 'with a public project with public merge requests' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + + project + .project_feature + .update!(merge_requests_access_level: ProjectFeature::ENABLED) + end + + context 'when not logged in' do + let(:current_user) {} + + it { is_expected.to contain_exactly(merge_request) } + end + end end end diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 0309618e243..bcc1c6bc4d4 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -116,6 +116,18 @@ describe API::Triggers do end end end + + context 'when is triggered by a pipeline hook' do + it 'does not create a new pipeline' do + expect do + post api("/projects/#{project.id}/ref/master/trigger/pipeline?token=#{trigger_token}"), + params: { ref: 'refs/heads/other-branch' }, + headers: { WebHookService::GITLAB_EVENT_HEADER => 'Pipeline Hook' } + end.not_to change(Ci::Pipeline, :count) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end end describe 'GET /projects/:id/triggers' do diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 5003dfcc951..84f4a7a4e7a 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -769,6 +769,15 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when deploy token has read_registry as a scope' do let(:current_user) { create(:deploy_token, projects: [project]) } + shared_examples 'able to login' do + context 'registry provides read_container_image authentication_abilities' do + let(:current_params) { {} } + let(:authentication_abilities) { [:read_container_image] } + + it_behaves_like 'an authenticated' + end + end + context 'for public project' do let(:project) { create(:project, :public) } @@ -783,6 +792,8 @@ describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'an inaccessible' end + + it_behaves_like 'able to login' end context 'for internal project' do @@ -799,6 +810,8 @@ describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'an inaccessible' end + + it_behaves_like 'able to login' end context 'for private project' do @@ -815,18 +828,38 @@ describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'an inaccessible' end + + it_behaves_like 'able to login' end end context 'when deploy token does not have read_registry scope' do let(:current_user) { create(:deploy_token, projects: [project], read_registry: false) } + shared_examples 'unable to login' do + context 'registry provides no container authentication_abilities' do + let(:current_params) { {} } + let(:authentication_abilities) { [] } + + it_behaves_like 'a forbidden' + end + + context 'registry provides inapplicable container authentication_abilities' do + let(:current_params) { {} } + let(:authentication_abilities) { [:download_code] } + + it_behaves_like 'a forbidden' + end + end + context 'for public project' do let(:project) { create(:project, :public) } context 'when pulling' do it_behaves_like 'a pullable' end + + it_behaves_like 'unable to login' end context 'for internal project' do @@ -835,6 +868,8 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pulling' do it_behaves_like 'an inaccessible' end + + it_behaves_like 'unable to login' end context 'for private project' do @@ -843,6 +878,15 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pulling' do it_behaves_like 'an inaccessible' end + + context 'when logging in' do + let(:current_params) { {} } + let(:authentication_abilities) { [] } + + it_behaves_like 'a forbidden' + end + + it_behaves_like 'unable to login' end end diff --git a/spec/services/groups/group_links/destroy_service_spec.rb b/spec/services/groups/group_links/destroy_service_spec.rb index 6f49b6eda94..284bcd0df2e 100644 --- a/spec/services/groups/group_links/destroy_service_spec.rb +++ b/spec/services/groups/group_links/destroy_service_spec.rb @@ -40,24 +40,11 @@ describe Groups::GroupLinks::DestroyService, '#execute' do end it 'updates project authorization once per group' do - expect(GroupGroupLink).to receive(:delete) + expect(GroupGroupLink).to receive(:delete).and_call_original expect(group).to receive(:refresh_members_authorized_projects).once expect(another_group).to receive(:refresh_members_authorized_projects).once subject.execute(links) end - - it 'rolls back changes when error happens' do - group.add_developer(user) - - expect(group).to receive(:refresh_members_authorized_projects).once.and_call_original - expect(another_group).to( - receive(:refresh_members_authorized_projects).and_raise('boom')) - - expect { subject.execute(links) }.to raise_error('boom') - - expect(GroupGroupLink.count).to eq(links.length) - expect(Ability.allowed?(user, :read_project, project)).to be_truthy - end end end diff --git a/spec/services/groups/group_links/update_service_spec.rb b/spec/services/groups/group_links/update_service_spec.rb new file mode 100644 index 00000000000..446364c9799 --- /dev/null +++ b/spec/services/groups/group_links/update_service_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Groups::GroupLinks::UpdateService, '#execute' do + let(:user) { create(:user) } + + let_it_be(:group) { create(:group, :private) } + let_it_be(:shared_group) { create(:group, :private) } + let_it_be(:project) { create(:project, group: shared_group) } + let(:group_member) { create(:user) } + let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) } + + let(:expiry_date) { 1.month.from_now.to_date } + let(:group_link_params) do + { group_access: Gitlab::Access::GUEST, + expires_at: expiry_date } + end + + subject { described_class.new(link).execute(group_link_params) } + + before do + group.add_developer(group_member) + end + + it 'updates existing link' do + expect(link.group_access).to eq(Gitlab::Access::DEVELOPER) + expect(link.expires_at).to be_nil + + subject + + link.reload + + expect(link.group_access).to eq(Gitlab::Access::GUEST) + expect(link.expires_at).to eq(expiry_date) + end + + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false) + end + + it 'executes UserProjectAccessChangedService' do + expect_next_instance_of(UserProjectAccessChangedService) do |service| + expect(service).to receive(:execute) + end + + subject + end + + context 'with only param not requiring authorization refresh' do + let(:group_link_params) { { expires_at: Date.tomorrow } } + + it 'does not execute UserProjectAccessChangedService' do + expect(UserProjectAccessChangedService).not_to receive(:new) + + subject + end + end +end diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 21a139cdf3c..496d1fe67f2 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -134,6 +134,21 @@ describe Projects::LfsPointers::LfsDownloadService do end end + context 'when an lfs object with the same oid already exists' do + let!(:existing_lfs_object) { create(:lfs_object, oid: oid) } + + before do + stub_full_request(download_link).to_return(body: lfs_content) + end + + it_behaves_like 'no lfs object is created' + + it 'does not update the file attached to the existing LfsObject' do + expect { subject.execute } + .not_to change { existing_lfs_object.reload.file.file.file } + end + end + context 'when credentials present' do let(:download_link_with_credentials) { "http://user:password@gitlab.com/#{oid}" } let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials) } @@ -211,17 +226,5 @@ describe Projects::LfsPointers::LfsDownloadService do subject.execute end end - - context 'when an lfs object with the same oid already exists' do - before do - create(:lfs_object, oid: oid) - end - - it 'does not download the file' do - expect(subject).not_to receive(:download_lfs_file!) - - subject.execute - end - end end end diff --git a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb index 9dac29765a2..e94d8a85987 100644 --- a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb @@ -24,7 +24,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do describe '#execute' do context 'when no lfs pointer is linked' do before do - allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return([]) allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links) expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original end @@ -35,12 +34,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do subject.execute end - it 'links existent lfs objects to the project' do - expect_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute) - - subject.execute - end - it 'retrieves the download links of non existent objects' do expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids) @@ -48,32 +41,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do end end - context 'when some lfs objects are linked' do - before do - allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(existing_lfs_objects.keys) - allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links) - end - - it 'retrieves the download links of non existent objects' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids) - - subject.execute - end - end - - context 'when all lfs objects are linked' do - before do - allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(all_oids.keys) - allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute) - end - - it 'retrieves no download links' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with({}).and_call_original - - expect(subject.execute).to be_empty - end - end - context 'when lfsconfig file exists' do before do allow(project.repository).to receive(:lfsconfig_for).and_return("[lfs]\n\turl = #{lfs_endpoint}\n") diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb index 63ebbcb93f9..3a306f80b3c 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -7,6 +7,7 @@ RSpec.shared_context 'GroupPolicy context' do let_it_be(:maintainer) { create(:user) } let_it_be(:owner) { create(:user) } let_it_be(:admin) { create(:admin) } + let_it_be(:non_group_member) { create(:user) } let_it_be(:group, refind: true) { create(:group, :private, :owner_subgroup_creation_only) } let(:guest_permissions) do diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 73087befad2..662c64647d6 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -69,13 +69,39 @@ RSpec.shared_examples 'handle uploads' do end describe "GET #show" do + let(:filename) { "rails_sample.jpg" } + + let(:upload_service) do + UploadService.new(model, jpg, uploader_class).execute + end + let(:show_upload) do - get :show, params: params.merge(secret: secret, filename: "rails_sample.jpg") + get :show, params: params.merge(secret: secret, filename: filename) end before do allow(FileUploader).to receive(:generate_secret).and_return(secret) - UploadService.new(model, jpg, uploader_class).execute + upload_service + end + + context 'when the secret is invalid' do + let(:secret) { "../../../../../../../../" } + let(:filename) { "Gemfile.lock" } + let(:upload_service) { nil } + + it 'responds with status 404' do + show_upload + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'is a working exploit without the validation' do + allow_any_instance_of(FileUploader).to receive(:secret) { secret } + + show_upload + + expect(response).to have_gitlab_http_status(:ok) + end end context 'when accessing a specific upload via different model' do diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index 9f053f03b0e..474515b537c 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -7,7 +7,7 @@ describe FileMover do let(:user) { create(:user) } let(:filename) { 'banana_sample.gif' } - let(:secret) { 'secret55' } + let(:secret) { SecureRandom.hex } let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", secret, filename) } let(:temp_description) do @@ -47,8 +47,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\ - "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ") + .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) "\ + "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) ") end it 'updates existing upload record' do @@ -75,8 +75,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\ - "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ") + .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) "\ + "same ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) ") end it 'does not change the upload record' do @@ -101,8 +101,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\ - "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ") + .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) "\ + "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) ") end it 'creates new target upload record an delete the old upload' do @@ -121,8 +121,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\ - "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ") + .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) "\ + "same ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) ") end it 'does not change the upload record' do @@ -138,12 +138,8 @@ describe FileMover do let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", '..', 'another_subdir_of_temp') } it 'does not trigger move if path is outside designated directory' do - stub_file_mover("uploads/-/system/user/#{user.id}/another_subdir_of_temp") expect(FileUtils).not_to receive(:move) - - subject - - expect(snippet.reload.description).to eq(temp_description) + expect { subject }.to raise_error(FileUploader::InvalidSecret) end end diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index beb7aa3cf2c..efdbea85d4a 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -6,7 +6,8 @@ describe FileUploader do let(:group) { create(:group, name: 'awesome') } let(:project) { create(:project, :legacy_storage, namespace: group, name: 'project') } let(:uploader) { described_class.new(project, :avatar) } - let(:upload) { double(model: project, path: 'secret/foo.jpg') } + let(:upload) { double(model: project, path: "#{secret}/foo.jpg") } + let(:secret) { "55dc16aa0edd05693fd98b5051e83321" } # this would be nicer as SecureRandom.hex, but the shared_examples breaks subject { uploader } @@ -14,7 +15,7 @@ describe FileUploader do include_examples 'builds correct paths', store_dir: %r{awesome/project/\h+}, upload_path: %r{\h+/}, - absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg} + absolute_path: %r{#{described_class.root}/awesome/project/55dc16aa0edd05693fd98b5051e83321/foo.jpg} end context 'legacy storage' do @@ -51,11 +52,11 @@ describe FileUploader do end describe 'initialize' do - let(:uploader) { described_class.new(double, secret: 'secret') } + let(:uploader) { described_class.new(double, secret: secret) } it 'accepts a secret parameter' do expect(described_class).not_to receive(:generate_secret) - expect(uploader.secret).to eq('secret') + expect(uploader.secret).to eq(secret) end end @@ -154,8 +155,39 @@ describe FileUploader do describe '#secret' do it 'generates a secret if none is provided' do - expect(described_class).to receive(:generate_secret).and_return('secret') - expect(uploader.secret).to eq('secret') + expect(described_class).to receive(:generate_secret).and_return(secret) + expect(uploader.secret).to eq(secret) + expect(uploader.secret.size).to eq(32) + end + + context "validation" do + before do + uploader.instance_variable_set(:@secret, secret) + end + + context "32-byte hexadecimal" do + let(:secret) { SecureRandom.hex } + + it "returns the secret" do + expect(uploader.secret).to eq(secret) + end + end + + context "10-byte hexadecimal" do + let(:secret) { SecureRandom.hex(10) } + + it "returns the secret" do + expect(uploader.secret).to eq(secret) + end + end + + context "invalid secret supplied" do + let(:secret) { "%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2Fgrafana%2Fconf%2F" } + + it "raises an exception" do + expect { uploader.secret }.to raise_error(described_class::InvalidSecret) + end + end end end diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb index ec652af222d..c211ec3607c 100644 --- a/spec/uploaders/personal_file_uploader_spec.rb +++ b/spec/uploaders/personal_file_uploader_spec.rb @@ -6,12 +6,13 @@ describe PersonalFileUploader do let(:model) { create(:personal_snippet) } let(:uploader) { described_class.new(model) } let(:upload) { create(:upload, :personal_snippet_upload) } + let(:secret) { SecureRandom.hex } subject { uploader } shared_examples '#base_dir' do before do - subject.instance_variable_set(:@secret, 'secret') + subject.instance_variable_set(:@secret, secret) end it 'is prefixed with uploads/-/system' do @@ -23,12 +24,12 @@ describe PersonalFileUploader do shared_examples '#to_h' do before do - subject.instance_variable_set(:@secret, 'secret') + subject.instance_variable_set(:@secret, secret) end it 'is correct' do allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name')) - expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name" + expected_url = "/uploads/-/system/personal_snippet/#{model.id}/#{secret}/file_name" expect(uploader.to_h).to eq( alt: 'file_name', diff --git a/spec/validators/addressable_url_validator_spec.rb b/spec/validators/addressable_url_validator_spec.rb index e8a44f7a12a..46b1bebb074 100644 --- a/spec/validators/addressable_url_validator_spec.rb +++ b/spec/validators/addressable_url_validator_spec.rb @@ -5,6 +5,9 @@ require 'spec_helper' describe AddressableUrlValidator do let!(:badge) { build(:badge, link_url: 'http://www.example.com') } + let(:validator) { described_class.new(validator_options.reverse_merge(attributes: [:link_url])) } + let(:validator_options) { {} } + subject { validator.validate(badge) } include_examples 'url validator examples', described_class::DEFAULT_OPTIONS[:schemes] @@ -114,6 +117,19 @@ describe AddressableUrlValidator do end end + context 'when blocked_message is set' do + let(:message) { 'is not allowed due to: %{exception_message}' } + let(:validator_options) { { blocked_message: message } } + + it 'blocks url with provided error message' do + badge.link_url = 'javascript:alert(window.opener.document.location)' + + subject + + expect(badge.errors.first[1]).to eq 'is not allowed due to: Only allowed schemes are http, https' + end + end + context 'when allow_nil is set to true' do let(:validator) { described_class.new(attributes: [:link_url], allow_nil: true) } -- cgit v1.2.3