From 1fb0bae24e6686b3571fc1c44cbf239d8563e0d7 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 30 Aug 2023 19:42:57 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@16-3-stable-ee --- app/controllers/projects/refs_controller.rb | 4 + app/graphql/resolvers/group_issues_resolver.rb | 5 ++ app/graphql/resolvers/issues_resolver.rb | 1 + lib/api/projects.rb | 7 +- lib/gitlab/pagination/gitaly_keyset_pager.rb | 6 +- spec/controllers/projects/refs_controller_spec.rb | 90 ++++++++++++++-------- spec/helpers/nav/new_dropdown_helper_spec.rb | 1 + spec/helpers/projects_helper_spec.rb | 1 + .../gitlab/pagination/gitaly_keyset_pager_spec.rb | 24 ++++-- spec/requests/api/projects_spec.rb | 41 ++++++++-- .../redacted_search_results_shared_examples.rb | 2 +- 11 files changed, 132 insertions(+), 50 deletions(-) diff --git a/app/controllers/projects/refs_controller.rb b/app/controllers/projects/refs_controller.rb index 4c2bd2a9d42..278d306301a 100644 --- a/app/controllers/projects/refs_controller.rb +++ b/app/controllers/projects/refs_controller.rb @@ -15,6 +15,8 @@ class Projects::RefsController < Projects::ApplicationController urgency :low, [:switch, :logs_tree] def switch + Gitlab::PathTraversal.check_path_traversal!(@id) + respond_to do |format| format.html do new_path = @@ -40,6 +42,8 @@ class Projects::RefsController < Projects::ApplicationController redirect_to new_path end end + rescue Gitlab::PathTraversal::PathTraversalAttackError + head :bad_request end def logs_tree diff --git a/app/graphql/resolvers/group_issues_resolver.rb b/app/graphql/resolvers/group_issues_resolver.rb index 43f01395896..7bbc662c6c8 100644 --- a/app/graphql/resolvers/group_issues_resolver.rb +++ b/app/graphql/resolvers/group_issues_resolver.rb @@ -9,6 +9,11 @@ module Resolvers include GroupIssuableResolver + before_connection_authorization do |nodes, _| + projects = nodes.map(&:project) + ActiveRecord::Associations::Preloader.new(records: projects, associations: :namespace).call + end + def ready?(**args) if args.dig(:not, :release_tag).present? raise ::Gitlab::Graphql::Errors::ArgumentError, 'releaseTag filter is not allowed when parent is a group.' diff --git a/app/graphql/resolvers/issues_resolver.rb b/app/graphql/resolvers/issues_resolver.rb index 17e3e159a5b..589366ba26d 100644 --- a/app/graphql/resolvers/issues_resolver.rb +++ b/app/graphql/resolvers/issues_resolver.rb @@ -23,6 +23,7 @@ module Resolvers projects = nodes.map(&:project) ::Preloaders::UserMaxAccessLevelInProjectsPreloader.new(projects, current_user).execute ::Preloaders::GroupPolicyPreloader.new(projects.filter_map(&:group), current_user).execute + ActiveRecord::Associations::Preloader.new(records: projects, associations: :namespace).call end def ready?(**args) diff --git a/lib/api/projects.rb b/lib/api/projects.rb index f6a2ce0f829..6d13512aad6 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -691,6 +691,7 @@ module API desc 'Mark this project as forked from another' do success code: 201, model: Entities::Project failure [ + { code: 401, message: 'Unauthorized' }, { code: 403, message: 'Unauthenticated' }, { code: 404, message: 'Not found' } ] @@ -708,7 +709,11 @@ module API authorize! :fork_project, fork_from_project - result = ::Projects::ForkService.new(fork_from_project, current_user).execute(user_project) + service = ::Projects::ForkService.new(fork_from_project, current_user) + + unauthorized!('Target Namespace') unless service.valid_fork_target?(user_project.namespace) + + result = service.execute(user_project) if result present_project user_project.reset, with: Entities::Project, current_user: current_user diff --git a/lib/gitlab/pagination/gitaly_keyset_pager.rb b/lib/gitlab/pagination/gitaly_keyset_pager.rb index 6235874132f..82d6fc64d89 100644 --- a/lib/gitlab/pagination/gitaly_keyset_pager.rb +++ b/lib/gitlab/pagination/gitaly_keyset_pager.rb @@ -15,7 +15,7 @@ module Gitlab # It is expected that the given finder will respond to `execute` method with `gitaly_pagination:` option # and supports pagination via gitaly. def paginate(finder) - return finder.execute(gitaly_pagination: false) if no_pagination? + return finder.execute(gitaly_pagination: false) if no_pagination?(finder) return paginate_via_gitaly(finder) if keyset_pagination_enabled?(finder) return paginate_first_page_via_gitaly(finder) if paginate_first_page?(finder) @@ -28,8 +28,8 @@ module Gitlab private - def no_pagination? - params[:pagination] == 'none' + def no_pagination?(finder) + params[:pagination] == 'none' && finder.is_a?(::Repositories::TreeFinder) end def keyset_pagination_enabled?(finder) diff --git a/spec/controllers/projects/refs_controller_spec.rb b/spec/controllers/projects/refs_controller_spec.rb index 0b1d0b75de7..7ea0e678a41 100644 --- a/spec/controllers/projects/refs_controller_spec.rb +++ b/spec/controllers/projects/refs_controller_spec.rb @@ -12,40 +12,70 @@ RSpec.describe Projects::RefsController, feature_category: :source_code_manageme end describe 'GET #switch' do - using RSpec::Parameterized::TableSyntax + context 'with normal parameters' do + using RSpec::Parameterized::TableSyntax - let(:id) { 'master' } - let(:params) do - { destination: destination, namespace_id: project.namespace.to_param, project_id: project, id: id, - ref_type: ref_type } - end + let(:id) { 'master' } + let(:id_and_path) { "#{id}/#{path}" } + + let(:params) do + { destination: destination, namespace_id: project.namespace.to_param, project_id: project, id: id, + ref_type: ref_type, path: path } + end + + subject { get :switch, params: params } + + where(:destination, :ref_type, :path, :redirected_to) do + 'tree' | nil | nil | lazy { project_tree_path(project, id) } + 'tree' | 'heads' | nil | lazy { project_tree_path(project, id) } + 'tree' | nil | 'foo/bar' | lazy { project_tree_path(project, id_and_path) } + 'blob' | nil | nil | lazy { project_blob_path(project, id) } + 'blob' | 'heads' | nil | lazy { project_blob_path(project, id) } + 'blob' | nil | 'foo/bar' | lazy { project_blob_path(project, id_and_path) } + 'graph' | nil | nil | lazy { project_network_path(project, id) } + 'graph' | 'heads' | nil | lazy { project_network_path(project, id, ref_type: 'heads') } + 'graph' | nil | 'foo/bar' | lazy { project_network_path(project, id_and_path) } + 'graphs' | nil | nil | lazy { project_graph_path(project, id) } + 'graphs' | 'heads' | nil | lazy { project_graph_path(project, id, ref_type: 'heads') } + 'graphs' | nil | 'foo/bar' | lazy { project_graph_path(project, id_and_path) } + 'find_file' | nil | nil | lazy { project_find_file_path(project, id) } + 'find_file' | 'heads' | nil | lazy { project_find_file_path(project, id) } + 'find_file' | nil | 'foo/bar' | lazy { project_find_file_path(project, id_and_path) } + 'graphs_commits' | nil | nil | lazy { commits_project_graph_path(project, id) } + 'graphs_commits' | 'heads' | nil | lazy { commits_project_graph_path(project, id) } + 'graphs_commits' | nil | 'foo/bar' | lazy { commits_project_graph_path(project, id_and_path) } + 'badges' | nil | nil | lazy { project_settings_ci_cd_path(project, ref: id) } + 'badges' | 'heads' | nil | lazy { project_settings_ci_cd_path(project, ref: id) } + 'badges' | nil | 'foo/bar' | lazy { project_settings_ci_cd_path(project, ref: id_and_path) } + 'commits' | nil | nil | lazy { project_commits_path(project, id) } + 'commits' | 'heads' | nil | lazy { project_commits_path(project, id, ref_type: 'heads') } + 'commits' | nil | 'foo/bar' | lazy { project_commits_path(project, id_and_path) } + nil | nil | nil | lazy { project_commits_path(project, id) } + nil | 'heads' | nil | lazy { project_commits_path(project, id, ref_type: 'heads') } + nil | nil | 'foo/bar' | lazy { project_commits_path(project, id_and_path) } + end - subject { get :switch, params: params } - - where(:destination, :ref_type, :redirected_to) do - 'tree' | nil | lazy { project_tree_path(project, id) } - 'tree' | 'heads' | lazy { project_tree_path(project, id) } - 'blob' | nil | lazy { project_blob_path(project, id) } - 'blob' | 'heads' | lazy { project_blob_path(project, id) } - 'graph' | nil | lazy { project_network_path(project, id) } - 'graph' | 'heads' | lazy { project_network_path(project, id, ref_type: 'heads') } - 'graphs' | nil | lazy { project_graph_path(project, id) } - 'graphs' | 'heads' | lazy { project_graph_path(project, id, ref_type: 'heads') } - 'find_file' | nil | lazy { project_find_file_path(project, id) } - 'find_file' | 'heads' | lazy { project_find_file_path(project, id) } - 'graphs_commits' | nil | lazy { commits_project_graph_path(project, id) } - 'graphs_commits' | 'heads' | lazy { commits_project_graph_path(project, id) } - 'badges' | nil | lazy { project_settings_ci_cd_path(project, ref: id) } - 'badges' | 'heads' | lazy { project_settings_ci_cd_path(project, ref: id) } - 'commits' | nil | lazy { project_commits_path(project, id) } - 'commits' | 'heads' | lazy { project_commits_path(project, id, ref_type: 'heads') } - nil | nil | lazy { project_commits_path(project, id) } - nil | 'heads' | lazy { project_commits_path(project, id, ref_type: 'heads') } + with_them do + it 'redirects to destination' do + expect(subject).to redirect_to(redirected_to) + end + end end - with_them do - it 'redirects to destination' do - expect(subject).to redirect_to(redirected_to) + context 'with bad path parameter' do + it 'returns 400 bad request' do + params = { + destination: 'tree', + namespace_id: project.namespace.to_param, + project_id: project, + id: 'master', + ref_type: nil, + path: '../bad_path_redirect' + } + + get :switch, params: params + + expect(response).to have_gitlab_http_status(:bad_request) end end end diff --git a/spec/helpers/nav/new_dropdown_helper_spec.rb b/spec/helpers/nav/new_dropdown_helper_spec.rb index 26dadd3b4f1..4ec120d152b 100644 --- a/spec/helpers/nav/new_dropdown_helper_spec.rb +++ b/spec/helpers/nav/new_dropdown_helper_spec.rb @@ -22,6 +22,7 @@ RSpec.describe Nav::NewDropdownHelper, feature_category: :navigation do allow(helper).to receive(:can?).and_return(false) allow(user).to receive(:can_create_project?) { with_can_create_project } allow(user).to receive(:can_create_group?) { with_can_create_group } + allow(user).to receive(:can?).and_call_original allow(user).to receive(:can?).with(:create_snippet) { with_can_create_snippet } end diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index aa064a26ec4..e1537c7b287 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -1170,6 +1170,7 @@ RSpec.describe ProjectsHelper, feature_category: :source_code_management do current_user = user if has_user allow(helper).to receive(:current_user).and_return(current_user) + allow(user).to receive(:can?).and_call_original allow(user).to receive(:can?).with(:fork_project, project).and_return(true) allow(user).to receive(:can?).with(:create_fork).and_return(true) allow(user).to receive(:can?).with(:create_projects, anything).and_return(true) diff --git a/spec/lib/gitlab/pagination/gitaly_keyset_pager_spec.rb b/spec/lib/gitlab/pagination/gitaly_keyset_pager_spec.rb index 2e87c582040..cb3f1fe86dc 100644 --- a/spec/lib/gitlab/pagination/gitaly_keyset_pager_spec.rb +++ b/spec/lib/gitlab/pagination/gitaly_keyset_pager_spec.rb @@ -127,17 +127,27 @@ RSpec.describe Gitlab::Pagination::GitalyKeysetPager do end end - context 'with "none" pagination option' do + context "with 'none' pagination option" do let(:expected_result) { double(:result) } let(:query) { { pagination: 'none' } } - it 'uses offset pagination' do - expect(finder).to receive(:execute).with(gitaly_pagination: false).and_return(expected_result) - expect(Kaminari).not_to receive(:paginate_array) - expect(Gitlab::Pagination::OffsetPagination).not_to receive(:new) + context "with a finder that is not a TreeFinder" do + it_behaves_like 'offset pagination' + end + + context "with a finder that is a TreeFinder" do + before do + allow(finder).to receive(:is_a?).with(::Repositories::TreeFinder).and_return(true) + end - actual_result = pager.paginate(finder) - expect(actual_result).to eq(expected_result) + it "doesn't uses offset pagination" do + expect(finder).to receive(:execute).with(gitaly_pagination: false).and_return(expected_result) + expect(Kaminari).not_to receive(:paginate_array) + expect(Gitlab::Pagination::OffsetPagination).not_to receive(:new) + + actual_result = pager.paginate(finder) + expect(actual_result).to eq(expected_result) + end end end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 26132215404..601e8cb3081 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -3311,16 +3311,41 @@ RSpec.describe API::Projects, :aggregate_failures, feature_category: :groups_and project_fork_target.add_maintainer(user) end - it 'allows project to be forked from an existing project' do - expect(project_fork_target).not_to be_forked + context 'and user is a reporter of target group' do + let_it_be_with_reload(:target_group) { create(:group, project_creation_level: ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS) } + let_it_be_with_reload(:project_fork_target) { create(:project, namespace: target_group) } - post api(path, user) - project_fork_target.reload + before do + target_group.add_reporter(user) + end - expect(response).to have_gitlab_http_status(:created) - expect(project_fork_target.forked_from_project.id).to eq(project_fork_source.id) - expect(project_fork_target.fork_network_member).to be_present - expect(project_fork_target).to be_forked + it 'fails as target namespace is unauthorized' do + post api(path, user) + + expect(response).to have_gitlab_http_status(:unauthorized) + expect(json_response['message']).to eq "401 Unauthorized - Target Namespace" + end + end + + context 'and user is a developer of target group' do + let_it_be_with_reload(:target_group) { create(:group, project_creation_level: ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS) } + let_it_be_with_reload(:project_fork_target) { create(:project, namespace: target_group) } + + before do + target_group.add_developer(user) + end + + it 'allows project to be forked from an existing project' do + expect(project_fork_target).not_to be_forked + + post api(path, user) + project_fork_target.reload + + expect(response).to have_gitlab_http_status(:created) + expect(project_fork_target.forked_from_project.id).to eq(project_fork_source.id) + expect(project_fork_target.fork_network_member).to be_present + expect(project_fork_target).to be_forked + end end it 'fails without permission from forked_from project' do diff --git a/spec/support/shared_examples/features/search/redacted_search_results_shared_examples.rb b/spec/support/shared_examples/features/search/redacted_search_results_shared_examples.rb index cbd0ffbab21..f2052f4202d 100644 --- a/spec/support/shared_examples/features/search/redacted_search_results_shared_examples.rb +++ b/spec/support/shared_examples/features/search/redacted_search_results_shared_examples.rb @@ -254,7 +254,7 @@ RSpec.shared_examples 'a redacted search results' do end context 'with :with_api_entity_associations' do - it_behaves_like "redaction limits N+1 queries", limit: 14 + it_behaves_like "redaction limits N+1 queries", limit: 15 end end -- cgit v1.2.3