Add latest changes from gitlab-org/security/gitlab@15-0-stable-ee

10 files changed, 63 insertions, 2 deletions

diff --git a/app/assets/javascripts/gfm_auto_complete.js b/app/assets/javascripts/gfm_auto_complete.js
index b1af1ad797b..146255df31f 100644
--- a/app/assets/javascripts/gfm_auto_complete.js
+++ b/app/assets/javascripts/gfm_auto_complete.js
@@ -955,7 +955,7 @@ GfmAutoComplete.Milestones = {
 };
 GfmAutoComplete.Contacts = {
   templateFunction({ email, firstName, lastName }) {
-    return `<li><small>${firstName} ${lastName}</small> ${escape(email)}</li>`;
+    return `<li><small>${escape(firstName)} ${escape(lastName)}</small> ${escape(email)}</li>`;
   },
 };
 
diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb
index bf72ade32d0..aec3247f4b2 100644
--- a/app/controllers/groups/application_controller.rb
+++ b/app/controllers/groups/application_controller.rb
@@ -67,6 +67,12 @@ class Groups::ApplicationController < ApplicationController
     end
   end
 
+  def authorize_read_group_member!
+    unless can?(current_user, :read_group_member, group)
+      render_403
+    end
+  end
+
   def build_canonical_path(group)
     params[:group_id] = group.to_param
 
diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb
index d325bb402e7..b95d8c87a4a 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/app/controllers/groups/group_members_controller.rb
@@ -14,6 +14,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
   # Authorize
   before_action :authorize_admin_group_member!, except: admin_not_required_endpoints
+  before_action :authorize_read_group_member!, only: :index
 
   skip_before_action :check_two_factor_requirement, only: :leave
   skip_cross_project_access_check :index, :update, :destroy, :request_access,
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index a4600c720a3..9aae295aea7 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -23,6 +23,7 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
   condition(:parent_share_with_group_locked, scope: :subject) { @subject.parent&.share_with_group_lock? }
   condition(:can_change_parent_share_with_group_lock) { can?(:change_share_with_group_lock, @subject.parent) }
   condition(:migration_bot, scope: :user) { @user.migration_bot? }
+  condition(:can_read_group_member) { can_read_group_member? }
 
   desc "User is a project bot"
   condition(:project_bot) { user.project_bot? && access_level >= GroupMember::GUEST }
@@ -128,6 +129,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
 
   rule { ~public_group & ~has_access }.prevent :read_counts
 
+  rule { ~can_read_group_member }.policy do
+    prevent :read_group_member
+  end
+
   rule { ~can?(:read_group) }.policy do
     prevent :read_design_activity
   end
@@ -316,6 +321,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     true
   end
 
+  def can_read_group_member?
+    !(@subject.private? && access_level == GroupMember::NO_ACCESS)
+  end
+
   def resource_access_token_creation_allowed?
     resource_access_token_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed?
   end
diff --git a/doc/user/group/subgroups/index.md b/doc/user/group/subgroups/index.md
index 2cddbaa9723..5f3c859d15a 100644
--- a/doc/user/group/subgroups/index.md
+++ b/doc/user/group/subgroups/index.md
@@ -93,6 +93,9 @@ For more information, view the [permissions table](../../permissions.md#group-me
 
 ## Subgroup membership
 
+NOTE:
+There is a bug that causes some pages in the parent group to be accessible by subgroup members. For more details, see [this issue](https://gitlab.com/gitlab-org/gitlab/-/issues/340421).
+
 When you add a member to a group, that member is also added to all subgroups. The user's permissions are inherited from
 the group's parent.
 
diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb
index c91e153c7b9..6a3cf5c87ae 100644
--- a/lib/api/helpers/members_helpers.rb
+++ b/lib/api/helpers/members_helpers.rb
@@ -15,6 +15,10 @@ module API
         public_send("find_#{source_type}!", id) # rubocop:disable GitlabSecurity/PublicSend
       end
 
+      def authorize_read_source_member!(source_type, source)
+        authorize! :"read_#{source_type}_member", source
+      end
+
       def authorize_admin_source!(source_type, source)
         authorize! :"admin_#{source_type}", source
       end
diff --git a/lib/api/members.rb b/lib/api/members.rb
index e2045c6def7..b94f68f60b5 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -32,6 +32,8 @@ module API
         get ":id/members", feature_category: feature_category do
           source = find_source(source_type, params[:id])
 
+          authorize_read_source_member!(source_type, source)
+
           members = paginate(retrieve_members(source, params: params))
 
           present_members members
@@ -51,6 +53,8 @@ module API
         get ":id/members/all", feature_category: feature_category do
           source = find_source(source_type, params[:id])
 
+          authorize_read_source_member!(source_type, source)
+
           members = paginate(retrieve_members(source, params: params, deep: true))
 
           present_members members
@@ -66,6 +70,8 @@ module API
         get ":id/members/:user_id", feature_category: feature_category do
           source = find_source(source_type, params[:id])
 
+          authorize_read_source_member!(source_type, source)
+
           members = source_members(source)
           member = members.find_by!(user_id: params[:user_id])
 
@@ -83,6 +89,8 @@ module API
         get ":id/members/all/:user_id", feature_category: feature_category do
           source = find_source(source_type, params[:id])
 
+          authorize_read_source_member!(source_type, source)
+
           members = find_all_members(source)
           member = members.find_by!(user_id: params[:user_id])
 
diff --git a/spec/features/security/group/private_access_spec.rb b/spec/features/security/group/private_access_spec.rb
index fc1fb3e3848..f733145b5e3 100644
--- a/spec/features/security/group/private_access_spec.rb
+++ b/spec/features/security/group/private_access_spec.rb
@@ -97,7 +97,7 @@ RSpec.describe 'Private Group access' do
     it { is_expected.to be_allowed_for(:developer).of(group) }
     it { is_expected.to be_allowed_for(:reporter).of(group) }
     it { is_expected.to be_allowed_for(:guest).of(group) }
-    it { is_expected.to be_allowed_for(project_guest) }
+    it { is_expected.to be_denied_for(project_guest) }
     it { is_expected.to be_denied_for(:user) }
     it { is_expected.to be_denied_for(:external) }
     it { is_expected.to be_denied_for(:visitor) }
diff --git a/spec/frontend/gfm_auto_complete_spec.js b/spec/frontend/gfm_auto_complete_spec.js
index aa98b2774ea..552377e3381 100644
--- a/spec/frontend/gfm_auto_complete_spec.js
+++ b/spec/frontend/gfm_auto_complete_spec.js
@@ -868,4 +868,19 @@ describe('GfmAutoComplete', () => {
       );
     });
   });
+
+  describe('Contacts', () => {
+    it('escapes name and email correct', () => {
+      const xssPayload = '<script>alert(1)</script>';
+      const escapedPayload = '&lt;script&gt;alert(1)&lt;/script&gt;';
+
+      expect(
+        GfmAutoComplete.Contacts.templateFunction({
+          email: xssPayload,
+          firstName: xssPayload,
+          lastName: xssPayload,
+        }),
+      ).toBe(`<li><small>${escapedPayload} ${escapedPayload}</small> ${escapedPayload}</li>`);
+    });
+  });
 });
diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb
index 0db42e7439c..63ef8643088 100644
--- a/spec/requests/api/members_spec.rb
+++ b/spec/requests/api/members_spec.rb
@@ -184,6 +184,21 @@ RSpec.describe API::Members do
       expect(json_response).to be_an Array
       expect(json_response.map { |u| u['id'] }).to match_array [maintainer.id, developer.id, nested_user.id]
     end
+
+    context 'with a subgroup' do
+      let(:group) { create(:group, :private)}
+      let(:subgroup) { create(:group, :private, parent: group)}
+      let(:project) { create(:project, group: subgroup) }
+
+      before do
+        subgroup.add_developer(developer)
+      end
+
+      it 'subgroup member cannot get parent group members list' do
+        get api("/groups/#{group.id}/members/all", developer)
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
   end
 
   shared_examples 'GET /:source_type/:id/members/(all/):user_id' do |source_type, all|