From 3dd03a1a19e6b788ec1296044e28f7727e5149a6 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 29 Mar 2023 23:48:05 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-10-stable-ee --- .../environments/environment_names_finder.rb | 11 +-------- app/models/project_feature.rb | 3 ++- .../explore/projects/page_out_of_bounds.html.haml | 2 +- .../environments/environment_names_finder_spec.rb | 26 +++++++++++++++++----- spec/policies/project_policy_spec.rb | 4 ++-- .../projects/page_out_of_bounds.html.haml_spec.rb | 26 ++++++++++++++++++++++ 6 files changed, 53 insertions(+), 19 deletions(-) create mode 100644 spec/views/explore/projects/page_out_of_bounds.html.haml_spec.rb diff --git a/app/finders/environments/environment_names_finder.rb b/app/finders/environments/environment_names_finder.rb index d4928f0fc84..ffb689f45e2 100644 --- a/app/finders/environments/environment_names_finder.rb +++ b/app/finders/environments/environment_names_finder.rb @@ -32,18 +32,9 @@ module Environments end def namespace_environments - # We assume reporter access is needed for the :read_environment permission - # here. This expection is also present in - # IssuableFinder::Params#min_access_level, which is used for filtering out - # merge requests that don't have the right permissions. - # - # We use this approach so we don't need to load every project into memory - # just to verify if we can see their environments. Doing so would not be - # efficient, and possibly mess up pagination if certain projects are not - # meant to be visible. projects = project_or_group .all_projects - .public_or_visible_to_user(current_user, Gitlab::Access::REPORTER) + .filter_by_feature_visibility(:environments, current_user) Environment.for_project(projects) end diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index 053ccfac050..52e623db7b0 100644 --- a/app/models/project_feature.rb +++ b/app/models/project_feature.rb @@ -36,7 +36,8 @@ class ProjectFeature < ApplicationRecord merge_requests: Gitlab::Access::REPORTER, metrics_dashboard: Gitlab::Access::REPORTER, container_registry: Gitlab::Access::REPORTER, - package_registry: Gitlab::Access::REPORTER + package_registry: Gitlab::Access::REPORTER, + environments: Gitlab::Access::REPORTER }.freeze PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT = { repository: Gitlab::Access::REPORTER }.freeze diff --git a/app/views/explore/projects/page_out_of_bounds.html.haml b/app/views/explore/projects/page_out_of_bounds.html.haml index ef5ee2c679e..e13768a3ccb 100644 --- a/app/views/explore/projects/page_out_of_bounds.html.haml +++ b/app/views/explore/projects/page_out_of_bounds.html.haml @@ -18,5 +18,5 @@ %h5= _("Maximum page reached") %p= _("Sorry, you have exceeded the maximum browsable page number. Please use the API to explore further.") - = render Pajamas::ButtonComponent.new(href: request.params.merge(page: @max_page_number)) do + = render Pajamas::ButtonComponent.new(href: safe_params.merge(page: @max_page_number)) do = _("Back to page %{number}") % { number: @max_page_number } diff --git a/spec/finders/environments/environment_names_finder_spec.rb b/spec/finders/environments/environment_names_finder_spec.rb index 438f9e9ea7c..c2336c59119 100644 --- a/spec/finders/environments/environment_names_finder_spec.rb +++ b/spec/finders/environments/environment_names_finder_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Environments::EnvironmentNamesFinder do describe '#execute' do let!(:group) { create(:group) } let!(:public_project) { create(:project, :public, namespace: group) } + let_it_be_with_reload(:public_project_with_private_environments) { create(:project, :public) } let!(:private_project) { create(:project, :private, namespace: group) } let!(:user) { create(:user) } @@ -14,6 +15,11 @@ RSpec.describe Environments::EnvironmentNamesFinder do create(:environment, name: 'gprd', project: public_project) create(:environment, name: 'gprd', project: private_project) create(:environment, name: 'gcny', project: private_project) + create(:environment, name: 'gprivprd', project: public_project_with_private_environments) + create(:environment, name: 'gprivstg', project: public_project_with_private_environments) + + public_project_with_private_environments.update!(namespace: group) + public_project_with_private_environments.project_feature.update!(environments_access_level: Featurable::PRIVATE) end context 'using a group' do @@ -23,7 +29,7 @@ RSpec.describe Environments::EnvironmentNamesFinder do names = described_class.new(group, user).execute - expect(names).to eq(%w[gcny gprd gstg]) + expect(names).to eq(%w[gcny gprd gprivprd gprivstg gstg]) end end @@ -33,7 +39,7 @@ RSpec.describe Environments::EnvironmentNamesFinder do names = described_class.new(group, user).execute - expect(names).to eq(%w[gcny gprd gstg]) + expect(names).to eq(%w[gcny gprd gprivprd gprivstg gstg]) end end @@ -57,8 +63,18 @@ RSpec.describe Environments::EnvironmentNamesFinder do end end + context 'with a public project reporter which has private environments' do + it 'returns environment names for public projects' do + public_project_with_private_environments.add_reporter(user) + + names = described_class.new(group, user).execute + + expect(names).to eq(%w[gprd gprivprd gprivstg gstg]) + end + end + context 'with a group guest' do - it 'returns environment names for all public projects' do + it 'returns environment names for public projects' do group.add_guest(user) names = described_class.new(group, user).execute @@ -68,7 +84,7 @@ RSpec.describe Environments::EnvironmentNamesFinder do end context 'with a non-member' do - it 'returns environment names for all public projects' do + it 'returns environment names for only public projects with public environments' do names = described_class.new(group, user).execute expect(names).to eq(%w[gprd gstg]) @@ -76,7 +92,7 @@ RSpec.describe Environments::EnvironmentNamesFinder do end context 'without a user' do - it 'returns environment names for all public projects' do + it 'returns environment names for only public projects with public environments' do names = described_class.new(group).execute expect(names).to eq(%w[gprd gstg]) diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 3759539677a..383f56b6ef5 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -2068,7 +2068,7 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do :public | ProjectFeature::ENABLED | :anonymous | true :public | ProjectFeature::PRIVATE | :maintainer | true :public | ProjectFeature::PRIVATE | :developer | true - :public | ProjectFeature::PRIVATE | :guest | true + :public | ProjectFeature::PRIVATE | :guest | false :public | ProjectFeature::PRIVATE | :anonymous | false :public | ProjectFeature::DISABLED | :maintainer | false :public | ProjectFeature::DISABLED | :developer | false @@ -2080,7 +2080,7 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do :internal | ProjectFeature::ENABLED | :anonymous | false :internal | ProjectFeature::PRIVATE | :maintainer | true :internal | ProjectFeature::PRIVATE | :developer | true - :internal | ProjectFeature::PRIVATE | :guest | true + :internal | ProjectFeature::PRIVATE | :guest | false :internal | ProjectFeature::PRIVATE | :anonymous | false :internal | ProjectFeature::DISABLED | :maintainer | false :internal | ProjectFeature::DISABLED | :developer | false diff --git a/spec/views/explore/projects/page_out_of_bounds.html.haml_spec.rb b/spec/views/explore/projects/page_out_of_bounds.html.haml_spec.rb new file mode 100644 index 00000000000..1ace28be5b4 --- /dev/null +++ b/spec/views/explore/projects/page_out_of_bounds.html.haml_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'explore/projects/page_out_of_bounds.html.haml', feature_category: :projects do + let(:page_limit) { 10 } + let(:unsafe_param) { 'hacked_using_unsafe_param!' } + + before do + assign(:max_page_number, page_limit) + + controller.params[:action] = 'index' + controller.params[:host] = unsafe_param + controller.params[:protocol] = unsafe_param + controller.params[:sort] = 'name_asc' + end + + it 'removes unsafe params from the link' do + render + + href = "/explore/projects?page=#{page_limit}&sort=name_asc" + button_text = format(_("Back to page %{number}"), number: page_limit) + expect(rendered).to have_link(button_text, href: href) + expect(rendered).not_to include(unsafe_param) + end +end -- cgit v1.2.1