From 925780caf1f669002af72d5a6be6a3a6551308cc Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 29 Nov 2023 16:24:13 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@16-6-stable-ee --- app/finders/packages/composer/packages_finder.rb | 8 ++--- app/finders/packages/group_packages_finder.rb | 19 +++++++----- app/models/concerns/protected_branch_access.rb | 2 ++ app/models/integrations/jira.rb | 4 +-- lib/gitlab/checks/branch_check.rb | 4 +-- lib/gitlab/regex.rb | 4 +-- .../packages/composer/packages_finder_spec.rb | 15 ++++++++-- .../finders/packages/group_packages_finder_spec.rb | 24 ++++++++++++++- spec/lib/gitlab/checks/branch_check_spec.rb | 34 +++++++++++----------- spec/models/integrations/jira_spec.rb | 2 +- 10 files changed, 75 insertions(+), 41 deletions(-) diff --git a/app/finders/packages/composer/packages_finder.rb b/app/finders/packages/composer/packages_finder.rb index b5a1b19216f..1581c48dd74 100644 --- a/app/finders/packages/composer/packages_finder.rb +++ b/app/finders/packages/composer/packages_finder.rb @@ -2,14 +2,12 @@ module Packages module Composer class PackagesFinder < Packages::GroupPackagesFinder - def initialize(current_user, group, params = {}) - @current_user = current_user - @group = group - @params = params + def initialize(current_user, group, params = { package_type: :composer, with_package_registry_enabled: true }) + super(current_user, group, params) end def execute - packages_for_group_projects(installable_only: true).composer.preload_composer + packages_for_group_projects(installable_only: true).preload_composer end end end diff --git a/app/finders/packages/group_packages_finder.rb b/app/finders/packages/group_packages_finder.rb index 3a068252d5c..3b211882fa0 100644 --- a/app/finders/packages/group_packages_finder.rb +++ b/app/finders/packages/group_packages_finder.rb @@ -40,14 +40,17 @@ module Packages # access to packages is ruled by: # - project is public or the current user has access to it with at least the reporter level # - the repository feature is available to the current_user - if current_user.is_a?(DeployToken) - current_user.accessible_projects - else - ::Project - .in_namespace(groups) - .public_or_visible_to_user(current_user, Gitlab::Access::REPORTER) - .with_feature_available_for_user(:repository, current_user) - end + projects = if current_user.is_a?(DeployToken) + current_user.accessible_projects + else + ::Project + .in_namespace(groups) + .public_or_visible_to_user(current_user, Gitlab::Access::REPORTER) + .with_feature_available_for_user(:repository, current_user) + end + + projects = projects.with_package_registry_enabled if params[:with_package_registry_enabled] + projects end def groups diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb index 8156090fd9c..6a7fdce62fb 100644 --- a/app/models/concerns/protected_branch_access.rb +++ b/app/models/concerns/protected_branch_access.rb @@ -10,3 +10,5 @@ module ProtectedBranchAccess delegate :project, to: :protected_branch end end + +ProtectedBranchAccess.prepend_mod_with('ProtectedBranchAccess') diff --git a/app/models/integrations/jira.rb b/app/models/integrations/jira.rb index 22367ee336d..bf49dbca294 100644 --- a/app/models/integrations/jira.rb +++ b/app/models/integrations/jira.rb @@ -401,9 +401,9 @@ module Integrations private def jira_issue_match_regex - return /\b#{jira_issue_prefix}(?#{Gitlab::Regex.jira_issue_key_regex})/ if jira_issue_regex.blank? + jira_regex = jira_issue_regex.presence || Gitlab::Regex.jira_issue_key_regex.source - Gitlab::UntrustedRegexp.new("\\b#{jira_issue_prefix}(?P#{jira_issue_regex})") + Gitlab::UntrustedRegexp.new("\\b#{jira_issue_prefix}(?P#{jira_regex})") end def parse_project_from_issue_key(issue_key) diff --git a/lib/gitlab/checks/branch_check.rb b/lib/gitlab/checks/branch_check.rb index b675eca826a..3bedc483e75 100644 --- a/lib/gitlab/checks/branch_check.rb +++ b/lib/gitlab/checks/branch_check.rb @@ -13,7 +13,7 @@ module Gitlab create_protected_branch: 'You are not allowed to create protected branches on this project.', invalid_commit_create_protected_branch: 'You can only use an existing protected branch ref as the basis of a new protected branch.', non_web_create_protected_branch: 'You can only create protected branches using the web interface and API.', - prohibited_hex_branch_name: 'You cannot create a branch with a 40-character hexadecimal branch name.', + prohibited_hex_branch_name: 'You cannot create a branch with a SHA-1 or SHA-256 branch name.', invalid_branch_name: 'You cannot create a branch with an invalid name.' }.freeze @@ -43,7 +43,7 @@ module Gitlab def prohibited_branch_checks return if deletion? - if %r{\A#{Gitlab::Git::Commit::RAW_FULL_SHA_PATTERN}(-/|/|\z)}o.match?(branch_name) + if %r{\A#{Gitlab::Git::Commit::RAW_FULL_SHA_PATTERN}}o.match?(branch_name) raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_hex_branch_name] end diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 6ac37986d5c..2de0a8d7b41 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -241,10 +241,8 @@ module Gitlab # Based on Jira's project key format # https://confluence.atlassian.com/adminjiraserver073/changing-the-project-key-format-861253229.html - # Avoids linking CVE IDs (https://cve.mitre.org/cve/identifiers/syntaxchange.html#new) as Jira issues. - # CVE IDs use the format of CVE-YYYY-NNNNNNN def jira_issue_key_regex(expression_escape: '\b') - /#{expression_escape}(?!CVE-\d+-\d+)[A-Z][A-Z_0-9]+-\d+/ + /#{expression_escape}([A-Z][A-Z_0-9]+-\d+)/ end def jira_issue_key_project_key_extraction_regex diff --git a/spec/finders/packages/composer/packages_finder_spec.rb b/spec/finders/packages/composer/packages_finder_spec.rb index d4328827de3..1701243063b 100644 --- a/spec/finders/packages/composer/packages_finder_spec.rb +++ b/spec/finders/packages/composer/packages_finder_spec.rb @@ -1,18 +1,19 @@ # frozen_string_literal: true require 'spec_helper' -RSpec.describe ::Packages::Composer::PackagesFinder do +RSpec.describe ::Packages::Composer::PackagesFinder, feature_category: :package_registry do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, group: group) } - let(:params) { {} } + let(:params) { { package_type: :composer } } describe '#execute' do let_it_be(:composer_package) { create(:composer_package, project: project) } let_it_be(:composer_package2) { create(:composer_package, project: project) } let_it_be(:error_package) { create(:composer_package, :error, project: project) } let_it_be(:composer_package3) { create(:composer_package) } + let_it_be(:nuget_package) { create(:nuget_package, project: project) } subject { described_class.new(user, group, params).execute } @@ -21,5 +22,15 @@ RSpec.describe ::Packages::Composer::PackagesFinder do end it { is_expected.to match_array([composer_package, composer_package2]) } + + context 'when disabling the package registry for the project' do + let(:params) { super().merge(with_package_registry_enabled: true) } + + before do + project.update!(package_registry_access_level: 'disabled', packages_enabled: false) + end + + it { is_expected.to be_empty } + end end end diff --git a/spec/finders/packages/group_packages_finder_spec.rb b/spec/finders/packages/group_packages_finder_spec.rb index a2698bc0153..d270d026da6 100644 --- a/spec/finders/packages/group_packages_finder_spec.rb +++ b/spec/finders/packages/group_packages_finder_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -RSpec.describe Packages::GroupPackagesFinder do +RSpec.describe Packages::GroupPackagesFinder, feature_category: :package_registry do using RSpec::Parameterized::TableSyntax let_it_be(:user) { create(:user) } @@ -25,6 +25,16 @@ RSpec.describe Packages::GroupPackagesFinder do it { is_expected.to match_array([send("package_#{package_type}")]) } end + shared_examples 'disabling package registry for project' do + let(:params) { super().merge(with_package_registry_enabled: true) } + + before do + project.update!(package_registry_access_level: 'disabled', packages_enabled: false) + end + + it { is_expected.to match_array(packages_returned) } + end + def self.package_types @package_types ||= Packages::Package.package_types.keys end @@ -117,6 +127,10 @@ RSpec.describe Packages::GroupPackagesFinder do let(:user) { deploy_token_for_group } it { is_expected.to match_array([package1, package2, package4]) } + + it_behaves_like 'disabling package registry for project' do + let(:packages_returned) { [package4] } + end end context 'project deploy token' do @@ -126,6 +140,11 @@ RSpec.describe Packages::GroupPackagesFinder do let(:user) { deploy_token_for_project } it { is_expected.to match_array([package4]) } + + it_behaves_like 'disabling package registry for project' do + let(:project) { subproject } + let(:packages_returned) { [] } + end end end @@ -200,6 +219,9 @@ RSpec.describe Packages::GroupPackagesFinder do it_behaves_like 'concerning versionless param' it_behaves_like 'concerning package statuses' + it_behaves_like 'disabling package registry for project' do + let(:packages_returned) { [] } + end end context 'group has package of all types' do diff --git a/spec/lib/gitlab/checks/branch_check_spec.rb b/spec/lib/gitlab/checks/branch_check_spec.rb index c3d6b9510e5..8772e8dd904 100644 --- a/spec/lib/gitlab/checks/branch_check_spec.rb +++ b/spec/lib/gitlab/checks/branch_check_spec.rb @@ -19,39 +19,39 @@ RSpec.describe Gitlab::Checks::BranchCheck, feature_category: :source_code_manag end end - context "prohibited branches check" do - it "prohibits 40-character hexadecimal branch names" do + describe "prohibited branches check" do + it "forbids SHA-1 values" do allow(subject).to receive(:branch_name).and_return("267208abfe40e546f5e847444276f7d43a39503e") - expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a 40-character hexadecimal branch name.") + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a SHA-1 or SHA-256 branch name.") end - it "prohibits 40-character hexadecimal branch names as the start of a path" do - allow(subject).to receive(:branch_name).and_return("267208abfe40e546f5e847444276f7d43a39503e/test") + it "forbids SHA-256 values" do + allow(subject).to receive(:branch_name).and_return("09b9fd3ea68e9b95a51b693a29568c898e27d1476bbd83c825664f18467fc175") - expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a 40-character hexadecimal branch name.") + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a SHA-1 or SHA-256 branch name.") end - it "prohibits 40-character hexadecimal branch names followed by a dash as the start of a path" do - allow(subject).to receive(:branch_name).and_return("267208abfe40e546f5e847444276f7d43a39503e-/test") + it "forbids '{SHA-1}{+anything}' values" do + allow(subject).to receive(:branch_name).and_return("267208abfe40e546f5e847444276f7d43a39503e-") - expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a 40-character hexadecimal branch name.") + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a SHA-1 or SHA-256 branch name.") end - it "prohibits 64-character hexadecimal branch names" do - allow(subject).to receive(:branch_name).and_return("09b9fd3ea68e9b95a51b693a29568c898e27d1476bbd83c825664f18467fc175") + it "forbids '{SHA-256}{+anything} values" do + allow(subject).to receive(:branch_name).and_return("09b9fd3ea68e9b95a51b693a29568c898e27d1476bbd83c825664f18467fc175-") - expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a 40-character hexadecimal branch name.") + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a SHA-1 or SHA-256 branch name.") end - it "prohibits 64-character hexadecimal branch names as the start of a path" do - allow(subject).to receive(:branch_name).and_return("09b9fd3ea68e9b95a51b693a29568c898e27d1476bbd83c825664f18467fc175/test") + it "allows SHA-1 values to be appended to the branch name" do + allow(subject).to receive(:branch_name).and_return("fix-267208abfe40e546f5e847444276f7d43a39503e") - expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a 40-character hexadecimal branch name.") + expect { subject.validate! }.not_to raise_error end - it "doesn't prohibit a nested hexadecimal in a branch name" do - allow(subject).to receive(:branch_name).and_return("267208abfe40e546f5e847444276f7d43a39503e-fix") + it "allows SHA-256 values to be appended to the branch name" do + allow(subject).to receive(:branch_name).and_return("fix-09b9fd3ea68e9b95a51b693a29568c898e27d1476bbd83c825664f18467fc175") expect { subject.validate! }.not_to raise_error end diff --git a/spec/models/integrations/jira_spec.rb b/spec/models/integrations/jira_spec.rb index af021c51035..2a3a3ec7f09 100644 --- a/spec/models/integrations/jira_spec.rb +++ b/spec/models/integrations/jira_spec.rb @@ -251,7 +251,7 @@ RSpec.describe Integrations::Jira, feature_category: :integrations do 'EXT_EXT-1234' | 'EXT_EXT-1234' 'EXT3_EXT-1234' | 'EXT3_EXT-1234' '3EXT_EXT-1234' | '' - 'CVE-2022-123' | '' + 'CVE-2022-123' | 'CVE-2022' 'CVE-123' | 'CVE-123' 'abc-JIRA-1234' | 'JIRA-1234' end -- cgit v1.2.3 From 706c0db2fcea38033a4b23e9434461127deb87e1 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 29 Nov 2023 16:28:43 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@16-6-stable-ee --- app/controllers/projects/tags_controller.rb | 2 +- app/models/pages_domain.rb | 5 ++-- app/policies/issue_policy.rb | 1 + package.json | 2 +- spec/controllers/projects/tags_controller_spec.rb | 12 +++++++++ .../projects/pages/user_adds_domain_spec.rb | 1 - spec/models/pages_domain_spec.rb | 23 ++++++++++++++--- spec/policies/issue_policy_spec.rb | 30 +++++++++++----------- yarn.lock | 8 +++--- 9 files changed, 56 insertions(+), 28 deletions(-) diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb index 3c1735c728c..d3e38774aaa 100644 --- a/app/controllers/projects/tags_controller.rb +++ b/app/controllers/projects/tags_controller.rb @@ -29,7 +29,7 @@ class Projects::TagsController < Projects::ApplicationController tag_names = @tags.map(&:name) @tags_pipelines = @project.ci_pipelines.latest_successful_for_refs(tag_names) - @releases = project.releases.where(tag: tag_names) + @releases = ReleasesFinder.new(project, current_user, tag: tag_names).execute @tag_pipeline_statuses = Ci::CommitStatusesFinder.new(@project, @repository, current_user, @tags).execute rescue Gitlab::Git::CommandError => e diff --git a/app/models/pages_domain.rb b/app/models/pages_domain.rb index cabd3924fd6..33de5aa21aa 100644 --- a/app/models/pages_domain.rb +++ b/app/models/pages_domain.rb @@ -35,10 +35,11 @@ class PagesDomain < ApplicationRecord validates :verification_code, presence: true, allow_blank: false validate :validate_pages_domain + validate :max_certificate_key_length, if: ->(domain) { domain.key.present? } validate :validate_matching_key, if: ->(domain) { domain.certificate.present? || domain.key.present? } - validate :validate_intermediates, if: ->(domain) { domain.certificate.present? && domain.certificate_changed? } + # validate_intermediates must run after key validations to skip expensive SSL validation when there is a key error + validate :validate_intermediates, if: ->(domain) { domain.certificate.present? && domain.certificate_changed? && errors[:key].blank? } validate :validate_custom_domain_count_per_project, on: :create - validate :max_certificate_key_length, if: ->(domain) { domain.key.present? } attribute :auto_ssl_enabled, default: -> { ::Gitlab::LetsEncrypt.enabled? } attribute :wildcard, default: false diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index 683c53d8d78..c95cde86e38 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -60,6 +60,7 @@ class IssuePolicy < IssuablePolicy rule { ~can?(:read_issue) }.policy do prevent :create_note prevent :read_note + prevent :award_emoji end rule { locked }.policy do diff --git a/package.json b/package.json index 773afdd1112..03317ca31b9 100644 --- a/package.json +++ b/package.json @@ -164,7 +164,7 @@ "marked-bidi": "^1.0.3", "mathjax": "3", "mdurl": "^1.0.1", - "mermaid": "10.6.0", + "mermaid": "10.6.1", "micromatch": "^4.0.5", "minimatch": "^3.0.4", "monaco-editor": "^0.30.1", diff --git a/spec/controllers/projects/tags_controller_spec.rb b/spec/controllers/projects/tags_controller_spec.rb index 3d1f8c12022..cab0778bd13 100644 --- a/spec/controllers/projects/tags_controller_spec.rb +++ b/spec/controllers/projects/tags_controller_spec.rb @@ -52,6 +52,18 @@ RSpec.describe Projects::TagsController do expect(assigns(:releases)).not_to include(invalid_release) end + context 'when releases are private' do + before do + project.project_feature.update!(releases_access_level: ProjectFeature::PRIVATE) + end + + it 'does not contain release data' do + subject + + expect(assigns(:releases)).to be_empty + end + end + context '@tag_pipeline_status' do context 'when no pipelines exist' do it 'is empty' do diff --git a/spec/features/projects/pages/user_adds_domain_spec.rb b/spec/features/projects/pages/user_adds_domain_spec.rb index 14b01cb63d2..04a9f450b52 100644 --- a/spec/features/projects/pages/user_adds_domain_spec.rb +++ b/spec/features/projects/pages/user_adds_domain_spec.rb @@ -155,7 +155,6 @@ RSpec.describe 'User adds pages domain', :js, feature_category: :pages do click_button 'Save Changes' expect(page).to have_content('Certificate must be a valid PEM certificate') - expect(page).to have_content('Certificate misses intermediates') expect(page).to have_content("Key doesn't match the certificate") end end diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index 7aa5cf993dc..a9d2552d7b7 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -165,7 +165,7 @@ RSpec.describe PagesDomain, feature_category: :pages do it "adds error to certificate" do domain.valid? - expect(domain.errors.attribute_names).to contain_exactly(:key, :certificate) + expect(domain.errors.attribute_names).to contain_exactly(:key) end end @@ -206,10 +206,25 @@ RSpec.describe PagesDomain, feature_category: :pages do it 'validates the certificate key length' do valid_domain = build(:pages_domain, :key_length_8192) expect(valid_domain).to be_valid + end + + context 'when the key has more than 8192 bytes' do + let(:domain) do + build(:pages_domain, :extra_long_key) + end - invalid_domain = build(:pages_domain, :extra_long_key) - expect(invalid_domain).to be_invalid - expect(invalid_domain.errors[:key]).to include('Certificate Key is too long. (Max 8192 bytes)') + it 'adds a human readable error' do + expect(domain).to be_invalid + expect(domain.errors[:key]).to include('Certificate Key is too long. (Max 8192 bytes)') + end + + it 'does not run SSL key verification' do + allow(domain).to receive(:validate_intermediates) + + domain.valid? + + expect(domain).not_to have_received(:validate_intermediates) + end end end end diff --git a/spec/policies/issue_policy_spec.rb b/spec/policies/issue_policy_spec.rb index c19b7bcf9ea..1d7748ee25a 100644 --- a/spec/policies/issue_policy_spec.rb +++ b/spec/policies/issue_policy_spec.rb @@ -146,50 +146,50 @@ RSpec.describe IssuePolicy, feature_category: :team_planning do let(:confidential_issue_no_assignee) { create(:issue, :confidential, project: project) } it 'does not allow non-members to read confidential issues' do - expect(permissions(non_member, confidential_issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :admin_issue_relation) - expect(permissions(non_member, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation) + expect(permissions(non_member, confidential_issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :admin_issue_relation, :award_emoji) + expect(permissions(non_member, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation, :award_emoji) end it 'does not allow guests to read confidential issues' do - expect(permissions(guest, confidential_issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :admin_issue_relation) - expect(permissions(guest, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation) + expect(permissions(guest, confidential_issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :admin_issue_relation, :award_emoji) + expect(permissions(guest, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation, :award_emoji) end it 'allows reporters to read, update, and admin confidential issues' do - expect(permissions(reporter, confidential_issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation) - expect(permissions(reporter, confidential_issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation) + expect(permissions(reporter, confidential_issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation, :award_emoji) + expect(permissions(reporter, confidential_issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation, :award_emoji) end it 'allows reporters from group links to read, update, and admin confidential issues' do - expect(permissions(reporter_from_group_link, confidential_issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation) - expect(permissions(reporter_from_group_link, confidential_issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation) + expect(permissions(reporter_from_group_link, confidential_issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation, :award_emoji) + expect(permissions(reporter_from_group_link, confidential_issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation, :award_emoji) end it 'allows issue authors to read and update their confidential issues' do - expect(permissions(author, confidential_issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue_relation) + expect(permissions(author, confidential_issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue_relation, :award_emoji) expect(permissions(author, confidential_issue)).to be_disallowed(:admin_issue, :set_issue_metadata, :set_confidentiality) - expect(permissions(author, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :admin_issue_relation) - expect(permissions(author, confidential_issue_no_assignee)).to be_disallowed(:admin_issue, :set_issue_metadata, :set_confidentiality) + expect(permissions(author, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :admin_issue_relation, :award_emoji) + expect(permissions(author, confidential_issue_no_assignee)).to be_disallowed(:admin_issue, :set_issue_metadata, :set_confidentiality, :award_emoji) end it 'does not allow issue author to read or update confidential issue moved to an private project' do confidential_issue.project = create(:project, :private) - expect(permissions(author, confidential_issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation) + expect(permissions(author, confidential_issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation, :award_emoji) end it 'allows issue assignees to read and update their confidential issues' do - expect(permissions(assignee, confidential_issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue) + expect(permissions(assignee, confidential_issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :award_emoji) expect(permissions(assignee, confidential_issue)).to be_disallowed(:admin_issue, :set_issue_metadata, :set_confidentiality) - expect(permissions(assignee, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation) + expect(permissions(assignee, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation, :award_emoji) end it 'does not allow issue assignees to read or update confidential issue moved to an private project' do confidential_issue.project = create(:project, :private) - expect(permissions(assignee, confidential_issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation) + expect(permissions(assignee, confidential_issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :set_issue_metadata, :set_confidentiality, :admin_issue_relation, :award_emoji) end end end diff --git a/yarn.lock b/yarn.lock index 689d0b669a6..9f41b43bd3c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9362,10 +9362,10 @@ merge2@^1.3.0, merge2@^1.4.1: resolved "https://registry.yarnpkg.com/merge2/-/merge2-1.4.1.tgz#4368892f885e907455a6fd7dc55c0c9d404990ae" integrity sha512-8q7VEgMJW4J8tcfVPy8g09NcQwZdbwFEqhe/WZkoIzjn/3TGDwtOCYtXGxA3O8tPzpczCCDgv+P2P5y00ZJOOg== -mermaid@10.6.0: - version "10.6.0" - resolved "https://registry.yarnpkg.com/mermaid/-/mermaid-10.6.0.tgz#151af64fb7c6cf1f8a5c403c53c6151832268b87" - integrity sha512-Hcti+Q2NiWnb2ZCijSX89Bn2i7TCUwosBdIn/d+u63Sz7y40XU6EKMctT4UX4qZuZGfKGZpfOeim2/KTrdR7aQ== +mermaid@10.6.1: + version "10.6.1" + resolved "https://registry.yarnpkg.com/mermaid/-/mermaid-10.6.1.tgz#701f4160484137a417770ce757ce1887a98c00fc" + integrity sha512-Hky0/RpOw/1il9X8AvzOEChfJtVvmXm+y7JML5C//ePYMy0/9jCEmW1E1g86x9oDfW9+iVEdTV/i+M6KWRNs4A== dependencies: "@braintree/sanitize-url" "^6.0.1" "@types/d3-scale" "^4.0.3" -- cgit v1.2.3 From bc77a6adefcd02a058e3d5a10718d74a5dad954e Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 29 Nov 2023 16:29:06 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@16-6-stable-ee --- .../types/permission_types/base_permission_type.rb | 2 +- app/helpers/groups_helper.rb | 11 + app/models/ability.rb | 14 +- app/models/user.rb | 6 +- app/policies/ci/pipeline_schedule_policy.rb | 3 +- .../ci/pipeline_schedules/base_save_service.rb | 6 +- .../ci/pipeline_schedules/update_service.rb | 6 + app/services/members/creator_service.rb | 24 +- app/views/groups/_invite_members_modal.html.haml | 2 +- locale/gitlab.pot | 3 + .../projects/pipeline_schedules_controller_spec.rb | 4 + spec/helpers/groups_helper_spec.rb | 63 ++++ spec/models/ability_spec.rb | 39 ++- spec/policies/ci/pipeline_schedule_policy_spec.rb | 338 +++++++++++++++------ .../ci/pipeline_schedules/update_service_spec.rb | 56 +++- 15 files changed, 454 insertions(+), 123 deletions(-) diff --git a/app/graphql/types/permission_types/base_permission_type.rb b/app/graphql/types/permission_types/base_permission_type.rb index 3c0e68bdaf2..3a8519e8196 100644 --- a/app/graphql/types/permission_types/base_permission_type.rb +++ b/app/graphql/types/permission_types/base_permission_type.rb @@ -30,7 +30,7 @@ module Types def self.define_field_resolver_method(ability) unless respond_to?(ability) define_method ability.to_sym do |*args| - Ability.allowed?(context[:current_user], ability, object, args.to_h) + Ability.allowed?(context[:current_user], ability, object, **args.to_h) end end end diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index f48157cb65a..6cabdf21483 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -207,6 +207,17 @@ module GroupsHelper new_group_custom_emoji_path(group) end + def access_level_roles_user_can_assign(group) + return {} unless current_user + return group.access_level_roles if current_user.can_admin_all_resources? + + max_access_level = group.highest_group_member(current_user)&.access_level + + return {} unless max_access_level + + GroupMember.access_level_roles.select { |_k, v| v <= max_access_level } + end + private def group_title_link(group, hidable: false, show_avatar: false, for_dropdown: false) diff --git a/app/models/ability.rb b/app/models/ability.rb index b8433191d84..9ae96c35d4f 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -70,13 +70,13 @@ class Ability end end - def allowed?(user, ability, subject = :global, opts = {}) + def allowed?(user, ability, subject = :global, **opts) if subject.is_a?(Hash) opts = subject subject = :global end - policy = policy_for(user, subject) + policy = policy_for(user, subject, **opts.slice(:cache)) before_check(policy, ability.to_sym, user, subject, opts) @@ -100,8 +100,14 @@ class Ability # See Support::AbilityCheck and Support::PermissionsCheck. end - def policy_for(user, subject = :global) - DeclarativePolicy.policy_for(user, subject, cache: ::Gitlab::SafeRequestStore.storage) + # We cache in the request store by default. This can lead to unexpected + # results if abilities are re-checked after objects are modified and the + # check depends on the modified attributes. In such cases, you should pass + # `cache: false` for the second check to ensure all rules get re-evaluated. + def policy_for(user, subject = :global, cache: true) + policy_cache = cache ? ::Gitlab::SafeRequestStore.storage : {} + + DeclarativePolicy.policy_for(user, subject, cache: policy_cache) end # This method is something of a band-aid over the problem. The problem is diff --git a/app/models/user.rb b/app/models/user.rb index 25f22563136..4a66192e9d8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1307,9 +1307,11 @@ class User < MainClusterwide::ApplicationRecord several_namespaces? || admin end - def can?(action, subject = :global) - Ability.allowed?(self, action, subject) + # rubocop: disable Style/ArgumentsForwarding + def can?(action, subject = :global, **opts) + Ability.allowed?(self, action, subject, **opts) end + # rubocop: enable Style/ArgumentsForwarding def confirm_deletion_with_password? !password_automatically_set? && allow_password_authentication? diff --git a/app/policies/ci/pipeline_schedule_policy.rb b/app/policies/ci/pipeline_schedule_policy.rb index cbc60c4a30a..9e558cd91c1 100644 --- a/app/policies/ci/pipeline_schedule_policy.rb +++ b/app/policies/ci/pipeline_schedule_policy.rb @@ -25,7 +25,7 @@ module Ci rule { can?(:create_pipeline) }.enable :play_pipeline_schedule - rule { can?(:admin_pipeline) | (can?(:update_build) & owner_of_schedule) }.policy do + rule { can?(:admin_pipeline) | (owner_of_schedule & can?(:update_build)) }.policy do enable :admin_pipeline_schedule enable :read_pipeline_schedule_variables end @@ -45,6 +45,7 @@ module Ci rule { protected_ref }.policy do prevent :play_pipeline_schedule prevent :create_pipeline_schedule + prevent :update_pipeline_schedule end private diff --git a/app/services/ci/pipeline_schedules/base_save_service.rb b/app/services/ci/pipeline_schedules/base_save_service.rb index 45d70e5a65d..e6f633498e9 100644 --- a/app/services/ci/pipeline_schedules/base_save_service.rb +++ b/app/services/ci/pipeline_schedules/base_save_service.rb @@ -23,7 +23,11 @@ module Ci attr_reader :project, :user, :params, :schedule def allowed_to_save? - user.can?(self.class::AUTHORIZE, schedule) + # Disable cache because the same ability may already have been checked + # for the same records with different attributes. For example, we do not + # want an unauthorized user to change an unprotected ref to a protected + # ref. + user.can?(self.class::AUTHORIZE, schedule, cache: false) end def forbidden_to_save diff --git a/app/services/ci/pipeline_schedules/update_service.rb b/app/services/ci/pipeline_schedules/update_service.rb index 2fd1173ecce..76b2121c4e1 100644 --- a/app/services/ci/pipeline_schedules/update_service.rb +++ b/app/services/ci/pipeline_schedules/update_service.rb @@ -12,6 +12,12 @@ module Ci @params = params end + def execute + return forbidden_to_save unless allowed_to_save? + + super + end + private def authorize_message diff --git a/app/services/members/creator_service.rb b/app/services/members/creator_service.rb index 22d8b30db18..d7bf073d8e9 100644 --- a/app/services/members/creator_service.rb +++ b/app/services/members/creator_service.rb @@ -156,12 +156,13 @@ module Members end def commit_member - if can_commit_member? - assign_member_attributes - commit_changes - else - add_commit_error - end + return add_commit_error unless can_commit_member? + + assign_member_attributes + + return add_member_role_error if member_role_too_high? + + commit_changes end def can_commit_member? @@ -175,6 +176,11 @@ module Members end end + # overridden in Members::Groups::CreatorService + def member_role_too_high? + false + end + def can_create_new_member? raise NotImplementedError end @@ -240,6 +246,12 @@ module Members member.errors.add(:base, msg) end + def add_member_role_error + msg = _("the member access level can't be higher than the current user's one") + + member.errors.add(:base, msg) + end + def find_or_build_member @member = builder.new(source, invitee, existing_members).execute end diff --git a/app/views/groups/_invite_members_modal.html.haml b/app/views/groups/_invite_members_modal.html.haml index d53190948fd..c60e7a78b1d 100644 --- a/app/views/groups/_invite_members_modal.html.haml +++ b/app/views/groups/_invite_members_modal.html.haml @@ -1,7 +1,7 @@ - return unless can_admin_group_member?(group) .js-invite-members-modal{ data: { is_project: 'false', - access_levels: group.access_level_roles.to_json, + access_levels: access_level_roles_user_can_assign(group).to_json, reload_page_on_submit: current_path?('group_members#index').to_s, help_link: help_page_url('user/permissions'), is_signup_enabled: signup_enabled?.to_s, diff --git a/locale/gitlab.pot b/locale/gitlab.pot index f95890d9ccf..234a9601a4c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -58301,6 +58301,9 @@ msgstr "" msgid "the following issues" msgstr "" +msgid "the member access level can't be higher than the current user's one" +msgstr "" + msgid "the wiki" msgstr "" diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index cd828c956a0..7cd4f43d4da 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -65,6 +65,10 @@ RSpec.describe Projects::PipelineSchedulesController, feature_category: :continu create(:protected_branch, *branch_access_levels, name: ref_name, project: project) end + after do + ProtectedBranches::CacheService.new(project).refresh + end + it { expect { go }.to try(maintainer_accessible, :maintainer).of(project) } it { expect { go }.to try(developer_accessible, :developer).of(project) } end diff --git a/spec/helpers/groups_helper_spec.rb b/spec/helpers/groups_helper_spec.rb index f5cadfc2761..93191cc956b 100644 --- a/spec/helpers/groups_helper_spec.rb +++ b/spec/helpers/groups_helper_spec.rb @@ -604,4 +604,67 @@ RSpec.describe GroupsHelper, feature_category: :groups_and_projects do end end end + + describe '#access_level_roles_user_can_assign' do + subject { helper.access_level_roles_user_can_assign(group) } + + let_it_be(:group) { create(:group) } + let_it_be_with_reload(:user) { create(:user) } + + context 'when user is provided' do + before do + allow(helper).to receive(:current_user).and_return(user) + end + + context 'when a user is a group member' do + before do + group.add_developer(user) + end + + it 'returns only the roles the provided user can assign' do + expect(subject).to eq( + { + 'Guest' => 10, + 'Reporter' => 20, + 'Developer' => 30 + } + ) + end + end + + context 'when a user is an admin', :enable_admin_mode do + before do + user.update!(admin: true) + end + + it 'returns all roles' do + expect(subject).to eq( + { + 'Guest' => 10, + 'Reporter' => 20, + 'Developer' => 30, + 'Maintainer' => 40, + 'Owner' => 50 + } + ) + end + end + + context 'when a user is not a group member' do + it 'returns the empty array' do + expect(subject).to be_empty + end + end + + context 'when user is not provided' do + before do + allow(helper).to receive(:current_user).and_return(nil) + end + + it 'returns the empty array' do + expect(subject).to be_empty + end + end + end + end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index a808cb1c823..1f0e074d90b 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -3,9 +3,42 @@ require 'spec_helper' RSpec.describe Ability do - context 'using a nil subject' do - it 'has no permissions' do - expect(described_class.policy_for(nil, nil)).to be_banned + describe '#policy_for' do + subject(:policy) { described_class.policy_for(user, subject, **options) } + + let(:user) { User.new } + let(:subject) { :global } + let(:options) { {} } + + context 'using a nil subject' do + let(:user) { nil } + let(:subject) { nil } + + it 'has no permissions' do + expect(policy).to be_banned + end + end + + context 'with request store', :request_store do + before do + ::Gitlab::SafeRequestStore.write(:example, :value) # make request store different from {} + end + + it 'caches in the request store' do + expect(DeclarativePolicy).to receive(:policy_for).with(user, subject, cache: ::Gitlab::SafeRequestStore.storage) + + policy + end + + context 'when cache: false' do + let(:options) { { cache: false } } + + it 'uses a fresh cache each time' do + expect(DeclarativePolicy).to receive(:policy_for).with(user, subject, cache: {}) + + policy + end + end end end diff --git a/spec/policies/ci/pipeline_schedule_policy_spec.rb b/spec/policies/ci/pipeline_schedule_policy_spec.rb index 8fc5c6ca296..1d353b9a35e 100644 --- a/spec/policies/ci/pipeline_schedule_policy_spec.rb +++ b/spec/policies/ci/pipeline_schedule_policy_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Ci::PipelineSchedulePolicy, :models, :clean_gitlab_redis_cache, f using RSpec::Parameterized::TableSyntax let_it_be(:user) { create(:user) } + let_it_be(:other_user) { create(:user) } let_it_be_with_reload(:project) { create(:project, :repository, create_tag: tag_ref_name) } let_it_be_with_reload(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly, project: project) } let_it_be(:tag_ref_name) { "v1.0.0" } @@ -17,89 +18,180 @@ RSpec.describe Ci::PipelineSchedulePolicy, :models, :clean_gitlab_redis_cache, f describe 'rules' do describe 'rules for protected ref' do context 'for branch' do + subject(:policy) { described_class.new(user, pipeline_schedule) } + %w[refs/heads/master master].each do |branch_ref| context "with #{branch_ref}" do let_it_be(:branch_ref_name) { "master" } - let_it_be(:branch_pipeline_schedule) do + let_it_be(:pipeline_schedule) do create(:ci_pipeline_schedule, :nightly, project: project, ref: branch_ref) end - where(:push_access_level, :merge_access_level, :project_role, :accessible) do - :no_one_can_push | :no_one_can_merge | :owner | :be_disallowed - :no_one_can_push | :no_one_can_merge | :maintainer | :be_disallowed - :no_one_can_push | :no_one_can_merge | :developer | :be_disallowed - :no_one_can_push | :no_one_can_merge | :reporter | :be_disallowed - :no_one_can_push | :no_one_can_merge | :guest | :be_disallowed - - :maintainers_can_push | :no_one_can_merge | :owner | :be_allowed - :maintainers_can_push | :no_one_can_merge | :maintainer | :be_allowed - :maintainers_can_push | :no_one_can_merge | :developer | :be_disallowed - :maintainers_can_push | :no_one_can_merge | :reporter | :be_disallowed - :maintainers_can_push | :no_one_can_merge | :guest | :be_disallowed - - :developers_can_push | :no_one_can_merge | :owner | :be_allowed - :developers_can_push | :no_one_can_merge | :maintainer | :be_allowed - :developers_can_push | :no_one_can_merge | :developer | :be_allowed - :developers_can_push | :no_one_can_merge | :reporter | :be_disallowed - :developers_can_push | :no_one_can_merge | :guest | :be_disallowed - - :no_one_can_push | :maintainers_can_merge | :owner | :be_allowed - :no_one_can_push | :maintainers_can_merge | :maintainer | :be_allowed - :no_one_can_push | :maintainers_can_merge | :developer | :be_disallowed - :no_one_can_push | :maintainers_can_merge | :reporter | :be_disallowed - :no_one_can_push | :maintainers_can_merge | :guest | :be_disallowed - - :maintainers_can_push | :maintainers_can_merge | :owner | :be_allowed - :maintainers_can_push | :maintainers_can_merge | :maintainer | :be_allowed - :maintainers_can_push | :maintainers_can_merge | :developer | :be_disallowed - :maintainers_can_push | :maintainers_can_merge | :reporter | :be_disallowed - :maintainers_can_push | :maintainers_can_merge | :guest | :be_disallowed - - :developers_can_push | :maintainers_can_merge | :owner | :be_allowed - :developers_can_push | :maintainers_can_merge | :maintainer | :be_allowed - :developers_can_push | :maintainers_can_merge | :developer | :be_allowed - :developers_can_push | :maintainers_can_merge | :reporter | :be_disallowed - :developers_can_push | :maintainers_can_merge | :guest | :be_disallowed - - :no_one_can_push | :developers_can_merge | :owner | :be_allowed - :no_one_can_push | :developers_can_merge | :maintainer | :be_allowed - :no_one_can_push | :developers_can_merge | :developer | :be_allowed - :no_one_can_push | :developers_can_merge | :reporter | :be_disallowed - :no_one_can_push | :developers_can_merge | :guest | :be_disallowed - - :maintainers_can_push | :developers_can_merge | :owner | :be_allowed - :maintainers_can_push | :developers_can_merge | :maintainer | :be_allowed - :maintainers_can_push | :developers_can_merge | :developer | :be_allowed - :maintainers_can_push | :developers_can_merge | :reporter | :be_disallowed - :maintainers_can_push | :developers_can_merge | :guest | :be_disallowed - - :developers_can_push | :developers_can_merge | :owner | :be_allowed - :developers_can_push | :developers_can_merge | :maintainer | :be_allowed - :developers_can_push | :developers_can_merge | :developer | :be_allowed - :developers_can_push | :developers_can_merge | :reporter | :be_disallowed - :developers_can_push | :developers_can_merge | :guest | :be_disallowed + shared_examples_for 'allowed by those who can update the branch' do + where(:push_access_level, :merge_access_level, :project_role, :accessible) do + :no_one_can_push | :no_one_can_merge | :owner | :be_disallowed + :no_one_can_push | :no_one_can_merge | :maintainer | :be_disallowed + :no_one_can_push | :no_one_can_merge | :developer | :be_disallowed + :no_one_can_push | :no_one_can_merge | :reporter | :be_disallowed + :no_one_can_push | :no_one_can_merge | :guest | :be_disallowed + + :maintainers_can_push | :no_one_can_merge | :owner | :be_allowed + :maintainers_can_push | :no_one_can_merge | :maintainer | :be_allowed + :maintainers_can_push | :no_one_can_merge | :developer | :be_disallowed + :maintainers_can_push | :no_one_can_merge | :reporter | :be_disallowed + :maintainers_can_push | :no_one_can_merge | :guest | :be_disallowed + + :developers_can_push | :no_one_can_merge | :owner | :be_allowed + :developers_can_push | :no_one_can_merge | :maintainer | :be_allowed + :developers_can_push | :no_one_can_merge | :developer | :be_allowed + :developers_can_push | :no_one_can_merge | :reporter | :be_disallowed + :developers_can_push | :no_one_can_merge | :guest | :be_disallowed + + :no_one_can_push | :maintainers_can_merge | :owner | :be_allowed + :no_one_can_push | :maintainers_can_merge | :maintainer | :be_allowed + :no_one_can_push | :maintainers_can_merge | :developer | :be_disallowed + :no_one_can_push | :maintainers_can_merge | :reporter | :be_disallowed + :no_one_can_push | :maintainers_can_merge | :guest | :be_disallowed + + :maintainers_can_push | :maintainers_can_merge | :owner | :be_allowed + :maintainers_can_push | :maintainers_can_merge | :maintainer | :be_allowed + :maintainers_can_push | :maintainers_can_merge | :developer | :be_disallowed + :maintainers_can_push | :maintainers_can_merge | :reporter | :be_disallowed + :maintainers_can_push | :maintainers_can_merge | :guest | :be_disallowed + + :developers_can_push | :maintainers_can_merge | :owner | :be_allowed + :developers_can_push | :maintainers_can_merge | :maintainer | :be_allowed + :developers_can_push | :maintainers_can_merge | :developer | :be_allowed + :developers_can_push | :maintainers_can_merge | :reporter | :be_disallowed + :developers_can_push | :maintainers_can_merge | :guest | :be_disallowed + + :no_one_can_push | :developers_can_merge | :owner | :be_allowed + :no_one_can_push | :developers_can_merge | :maintainer | :be_allowed + :no_one_can_push | :developers_can_merge | :developer | :be_allowed + :no_one_can_push | :developers_can_merge | :reporter | :be_disallowed + :no_one_can_push | :developers_can_merge | :guest | :be_disallowed + + :maintainers_can_push | :developers_can_merge | :owner | :be_allowed + :maintainers_can_push | :developers_can_merge | :maintainer | :be_allowed + :maintainers_can_push | :developers_can_merge | :developer | :be_allowed + :maintainers_can_push | :developers_can_merge | :reporter | :be_disallowed + :maintainers_can_push | :developers_can_merge | :guest | :be_disallowed + + :developers_can_push | :developers_can_merge | :owner | :be_allowed + :developers_can_push | :developers_can_merge | :maintainer | :be_allowed + :developers_can_push | :developers_can_merge | :developer | :be_allowed + :developers_can_push | :developers_can_merge | :reporter | :be_disallowed + :developers_can_push | :developers_can_merge | :guest | :be_disallowed + end + + with_them do + before do + create(:protected_branch, push_access_level, merge_access_level, name: branch_ref_name, + project: project) + project.add_role(user, project_role) + end + + it { expect(policy).to try(accessible, :create_pipeline_schedule) } + end end - with_them do - before do - create(:protected_branch, push_access_level, merge_access_level, name: branch_ref_name, - project: project) - project.add_role(user, project_role) + shared_examples_for 'only allowed by schedule owners who can update the branch' do + where(:push_access_level, :merge_access_level, :schedule_owner, :project_role, :accessible) do + :no_one_can_push | :no_one_can_merge | :other_user | :owner | :be_disallowed + :no_one_can_push | :no_one_can_merge | :user | :owner | :be_disallowed + :no_one_can_push | :no_one_can_merge | :user | :maintainer | :be_disallowed + :no_one_can_push | :no_one_can_merge | :user | :developer | :be_disallowed + :no_one_can_push | :no_one_can_merge | :user | :reporter | :be_disallowed + :no_one_can_push | :no_one_can_merge | :user | :guest | :be_disallowed + + :maintainers_can_push | :no_one_can_merge | :other_user | :owner | :be_disallowed + :maintainers_can_push | :no_one_can_merge | :user | :owner | :be_allowed + :maintainers_can_push | :no_one_can_merge | :user | :maintainer | :be_allowed + :maintainers_can_push | :no_one_can_merge | :user | :developer | :be_disallowed + :maintainers_can_push | :no_one_can_merge | :user | :reporter | :be_disallowed + :maintainers_can_push | :no_one_can_merge | :user | :guest | :be_disallowed + + :developers_can_push | :no_one_can_merge | :other_user | :owner | :be_disallowed + :developers_can_push | :no_one_can_merge | :user | :owner | :be_allowed + :developers_can_push | :no_one_can_merge | :user | :maintainer | :be_allowed + :developers_can_push | :no_one_can_merge | :user | :developer | :be_allowed + :developers_can_push | :no_one_can_merge | :user | :reporter | :be_disallowed + :developers_can_push | :no_one_can_merge | :user | :guest | :be_disallowed + + :no_one_can_push | :maintainers_can_merge | :other_user | :owner | :be_disallowed + :no_one_can_push | :maintainers_can_merge | :user | :owner | :be_allowed + :no_one_can_push | :maintainers_can_merge | :user | :maintainer | :be_allowed + :no_one_can_push | :maintainers_can_merge | :user | :developer | :be_disallowed + :no_one_can_push | :maintainers_can_merge | :user | :reporter | :be_disallowed + :no_one_can_push | :maintainers_can_merge | :user | :guest | :be_disallowed + + :maintainers_can_push | :maintainers_can_merge | :other_user | :owner | :be_disallowed + :maintainers_can_push | :maintainers_can_merge | :user | :owner | :be_allowed + :maintainers_can_push | :maintainers_can_merge | :user | :maintainer | :be_allowed + :maintainers_can_push | :maintainers_can_merge | :user | :developer | :be_disallowed + :maintainers_can_push | :maintainers_can_merge | :user | :reporter | :be_disallowed + :maintainers_can_push | :maintainers_can_merge | :user | :guest | :be_disallowed + + :developers_can_push | :maintainers_can_merge | :other_user | :owner | :be_disallowed + :developers_can_push | :maintainers_can_merge | :user | :owner | :be_allowed + :developers_can_push | :maintainers_can_merge | :user | :maintainer | :be_allowed + :developers_can_push | :maintainers_can_merge | :user | :developer | :be_allowed + :developers_can_push | :maintainers_can_merge | :user | :reporter | :be_disallowed + :developers_can_push | :maintainers_can_merge | :user | :guest | :be_disallowed + + :no_one_can_push | :developers_can_merge | :other_user | :owner | :be_disallowed + :no_one_can_push | :developers_can_merge | :user | :owner | :be_allowed + :no_one_can_push | :developers_can_merge | :user | :maintainer | :be_allowed + :no_one_can_push | :developers_can_merge | :user | :developer | :be_allowed + :no_one_can_push | :developers_can_merge | :user | :reporter | :be_disallowed + :no_one_can_push | :developers_can_merge | :user | :guest | :be_disallowed + + :maintainers_can_push | :developers_can_merge | :other_user | :owner | :be_disallowed + :maintainers_can_push | :developers_can_merge | :user | :owner | :be_allowed + :maintainers_can_push | :developers_can_merge | :user | :maintainer | :be_allowed + :maintainers_can_push | :developers_can_merge | :user | :developer | :be_allowed + :maintainers_can_push | :developers_can_merge | :user | :reporter | :be_disallowed + :maintainers_can_push | :developers_can_merge | :user | :guest | :be_disallowed + + :developers_can_push | :developers_can_merge | :other_user | :owner | :be_disallowed + :developers_can_push | :developers_can_merge | :user | :owner | :be_allowed + :developers_can_push | :developers_can_merge | :user | :maintainer | :be_allowed + :developers_can_push | :developers_can_merge | :user | :developer | :be_allowed + :developers_can_push | :developers_can_merge | :user | :reporter | :be_disallowed + :developers_can_push | :developers_can_merge | :user | :guest | :be_disallowed end - context 'for create_pipeline_schedule' do - subject(:policy) { described_class.new(user, new_branch_pipeline_schedule) } + with_them do + before do + create(:protected_branch, push_access_level, merge_access_level, name: branch_ref_name, + project: project) + project.add_role(user, project_role) + project.add_role(other_user, project_role) - let(:new_branch_pipeline_schedule) { project.pipeline_schedules.new(ref: branch_ref) } + pipeline_schedule.owner = schedule_owner == :user ? user : other_user + end - it { expect(policy).to try(accessible, :create_pipeline_schedule) } + it { expect(policy).to try(accessible, ability_name) } end + end - context 'for play_pipeline_schedule' do - subject(:policy) { described_class.new(user, branch_pipeline_schedule) } + describe 'create_pipeline_schedule' do + let(:ability_name) { :create_pipeline_schedule } + let(:pipeline_schedule) { project.pipeline_schedules.new(ref: branch_ref) } - it { expect(policy).to try(accessible, :play_pipeline_schedule) } - end + it_behaves_like 'allowed by those who can update the branch' + end + + describe 'play_pipeline_schedule' do + let(:ability_name) { :play_pipeline_schedule } + + it_behaves_like 'allowed by those who can update the branch' + end + + describe 'update_pipeline_schedule' do + let(:ability_name) { :update_pipeline_schedule } + + it_behaves_like 'only allowed by schedule owners who can update the branch' end end end @@ -108,49 +200,97 @@ RSpec.describe Ci::PipelineSchedulePolicy, :models, :clean_gitlab_redis_cache, f context 'for tag' do %w[refs/tags/v1.0.0 v1.0.0].each do |tag_ref| context "with #{tag_ref}" do - let_it_be(:tag_pipeline_schedule) do + let_it_be(:pipeline_schedule) do create(:ci_pipeline_schedule, :nightly, project: project, ref: tag_ref) end - where(:access_level, :project_role, :accessible) do - :no_one_can_create | :owner | :be_disallowed - :no_one_can_create | :maintainer | :be_disallowed - :no_one_can_create | :developer | :be_disallowed - :no_one_can_create | :reporter | :be_disallowed - :no_one_can_create | :guest | :be_disallowed - - :maintainers_can_create | :owner | :be_allowed - :maintainers_can_create | :maintainer | :be_allowed - :maintainers_can_create | :developer | :be_disallowed - :maintainers_can_create | :reporter | :be_disallowed - :maintainers_can_create | :guest | :be_disallowed - - :developers_can_create | :owner | :be_allowed - :developers_can_create | :maintainer | :be_allowed - :developers_can_create | :developer | :be_allowed - :developers_can_create | :reporter | :be_disallowed - :developers_can_create | :guest | :be_disallowed + subject(:policy) { described_class.new(user, pipeline_schedule) } + + shared_examples_for 'allowed by those who can update the tag' do + where(:access_level, :project_role, :accessible) do + :no_one_can_create | :owner | :be_disallowed + :no_one_can_create | :maintainer | :be_disallowed + :no_one_can_create | :developer | :be_disallowed + :no_one_can_create | :reporter | :be_disallowed + :no_one_can_create | :guest | :be_disallowed + + :maintainers_can_create | :owner | :be_allowed + :maintainers_can_create | :maintainer | :be_allowed + :maintainers_can_create | :developer | :be_disallowed + :maintainers_can_create | :reporter | :be_disallowed + :maintainers_can_create | :guest | :be_disallowed + + :developers_can_create | :owner | :be_allowed + :developers_can_create | :maintainer | :be_allowed + :developers_can_create | :developer | :be_allowed + :developers_can_create | :reporter | :be_disallowed + :developers_can_create | :guest | :be_disallowed + end + + with_them do + before do + create(:protected_tag, access_level, name: tag_ref_name, project: project) + project.add_role(user, project_role) + end + + it { expect(policy).to try(accessible, ability_name) } + end end - with_them do - before do - create(:protected_tag, access_level, name: tag_ref_name, project: project) - project.add_role(user, project_role) + shared_examples_for 'only allowed by schedule owners who can update the tag' do + where(:access_level, :schedule_owner, :project_role, :accessible) do + :no_one_can_create | :other_user | :owner | :be_disallowed + :no_one_can_create | :user | :owner | :be_disallowed + :no_one_can_create | :user | :maintainer | :be_disallowed + :no_one_can_create | :user | :developer | :be_disallowed + :no_one_can_create | :user | :reporter | :be_disallowed + :no_one_can_create | :user | :guest | :be_disallowed + + :maintainers_can_create | :other_user | :owner | :be_disallowed + :maintainers_can_create | :user | :owner | :be_allowed + :maintainers_can_create | :user | :maintainer | :be_allowed + :maintainers_can_create | :user | :developer | :be_disallowed + :maintainers_can_create | :user | :reporter | :be_disallowed + :maintainers_can_create | :user | :guest | :be_disallowed + + :developers_can_create | :other_user | :owner | :be_disallowed + :developers_can_create | :user | :owner | :be_allowed + :developers_can_create | :user | :maintainer | :be_allowed + :developers_can_create | :user | :developer | :be_allowed + :developers_can_create | :user | :reporter | :be_disallowed + :developers_can_create | :user | :guest | :be_disallowed end - context 'for create_pipeline_schedule' do - subject(:policy) { described_class.new(user, new_tag_pipeline_schedule) } + with_them do + before do + create(:protected_tag, access_level, name: tag_ref_name, project: project) + project.add_role(user, project_role) + project.add_role(other_user, project_role) - let(:new_tag_pipeline_schedule) { project.pipeline_schedules.new(ref: tag_ref) } + pipeline_schedule.owner = schedule_owner == :user ? user : other_user + end - it { expect(policy).to try(accessible, :create_pipeline_schedule) } + it { expect(policy).to try(accessible, ability_name) } end + end - context 'for play_pipeline_schedule' do - subject(:policy) { described_class.new(user, tag_pipeline_schedule) } + describe 'create_pipeline_schedule' do + let(:ability_name) { :create_pipeline_schedule } + let(:pipeline_schedule) { project.pipeline_schedules.new(ref: tag_ref) } - it { expect(policy).to try(accessible, :play_pipeline_schedule) } - end + it_behaves_like 'allowed by those who can update the tag' + end + + describe 'play_pipeline_schedule' do + let(:ability_name) { :play_pipeline_schedule } + + it_behaves_like 'allowed by those who can update the tag' + end + + describe 'update_pipeline_schedule' do + let(:ability_name) { :update_pipeline_schedule } + + it_behaves_like 'only allowed by schedule owners who can update the tag' end end end diff --git a/spec/services/ci/pipeline_schedules/update_service_spec.rb b/spec/services/ci/pipeline_schedules/update_service_spec.rb index 834bbcfcfeb..b84afacdcff 100644 --- a/spec/services/ci/pipeline_schedules/update_service_spec.rb +++ b/spec/services/ci/pipeline_schedules/update_service_spec.rb @@ -7,16 +7,16 @@ RSpec.describe Ci::PipelineSchedules::UpdateService, feature_category: :continuo let_it_be_with_reload(:project) { create(:project, :public, :repository) } let_it_be_with_reload(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: user) } let_it_be(:reporter) { create(:user) } + let_it_be(:project_owner) { create(:user) } let_it_be(:pipeline_schedule_variable) do create(:ci_pipeline_schedule_variable, key: 'foo', value: 'foovalue', pipeline_schedule: pipeline_schedule) end - subject(:service) { described_class.new(pipeline_schedule, user, params) } - before_all do project.add_maintainer(user) + project.add_owner(project_owner) project.add_reporter(reporter) pipeline_schedule.reload @@ -54,8 +54,10 @@ RSpec.describe Ci::PipelineSchedules::UpdateService, feature_category: :continuo subject(:service) { described_class.new(pipeline_schedule, user, params) } it 'updates database values with passed params' do - expect { service.execute } - .to change { pipeline_schedule.description }.from('pipeline schedule').to('updated_desc') + expect do + service.execute + pipeline_schedule.reload + end.to change { pipeline_schedule.description }.from('pipeline schedule').to('updated_desc') .and change { pipeline_schedule.ref }.from('master').to('patch-x') .and change { pipeline_schedule.active }.from(true).to(false) .and change { pipeline_schedule.cron }.from('0 1 * * *').to('*/1 * * * *') @@ -63,6 +65,48 @@ RSpec.describe Ci::PipelineSchedules::UpdateService, feature_category: :continuo .and change { pipeline_schedule.variables.last.value }.from('foovalue').to('barvalue') end + context 'when the new branch is protected', :request_store do + let(:maintainer_access) { :no_one_can_merge } + + before do + create(:protected_branch, :no_one_can_push, maintainer_access, name: 'patch-x', project: project) + end + + after do + ProtectedBranches::CacheService.new(project).refresh + end + + context 'when called by someone other than the schedule owner who can update the ref' do + let(:maintainer_access) { :maintainers_can_merge } + + subject(:service) { described_class.new(pipeline_schedule, project_owner, params) } + + it 'does not update the schedule' do + expect do + service.execute + pipeline_schedule.reload + end.not_to change { pipeline_schedule.description } + end + end + + context 'when called by the schedule owner' do + it 'does not update the schedule' do + expect do + service.execute + pipeline_schedule.reload + end.not_to change { pipeline_schedule.description } + end + + context 'when the owner can update the ref' do + let(:maintainer_access) { :maintainers_can_merge } + + it 'updates the schedule' do + expect { service.execute }.to change { pipeline_schedule.description } + end + end + end + end + context 'when creating a variable' do let(:params) do { @@ -126,6 +170,8 @@ RSpec.describe Ci::PipelineSchedules::UpdateService, feature_category: :continuo end end - it_behaves_like 'pipeline schedules checking variables permission' + it_behaves_like 'pipeline schedules checking variables permission' do + subject(:service) { described_class.new(pipeline_schedule, user, params) } + end end end -- cgit v1.2.3 From b432ab076cb183ac235b925491c97b0af7f61881 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Thu, 30 Nov 2023 08:34:08 +0000 Subject: Update VERSION files [ci skip] --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index fe194a26408..235d052b5f0 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -16.6.0 \ No newline at end of file +16.6.1 \ No newline at end of file -- cgit v1.2.3 From 21d8cc00343440dc18682ace76094a8958f701fa Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 30 Nov 2023 08:37:59 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@16-6-stable-ee --- CHANGELOG.md | 22 ++++++++++++++++++++++ GITALY_SERVER_VERSION | 2 +- GITLAB_PAGES_VERSION | 2 +- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3937abd655b..516e17ed7d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,28 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 16.6.1 (2023-11-30) + +### Fixed (3 changes) + +- [Revert "Merge branch 'sc1-release-goredis' into 'master'"](gitlab-org/security/gitlab@9589d80224cae8723bea3180418061363deeddd9) +- [Truncate verification failure message to 255](gitlab-org/security/gitlab@d3c363a1e644525c386e670abca295181a9ae450) **GitLab Enterprise Edition** +- [Prefer custom sort order with search in users API](gitlab-org/security/gitlab@3c9b46eb086ebfa595083452f82ddd19db586e5b) + +### Security (11 changes) + +- [Validate adding members with higher role](gitlab-org/security/gitlab@e55b3d8e5f3cf86fa5b124b0c85d3c70e94056b0) ([merge request](gitlab-org/security/gitlab!3713)) +- [Enforce ref protection on pipeline schedule updates](gitlab-org/security/gitlab@a4565e7ddc064035a622c0f645bdcf583f8d9945) ([merge request](gitlab-org/security/gitlab!3703)) +- [Update mermaid version for DOS security fixes](gitlab-org/security/gitlab@baec50f7af8077e77cf3124ac695ecb12d2d0028) ([merge request](gitlab-org/security/gitlab!3707)) +- [Prevent guest users from being able to add emojis in confidential issues](gitlab-org/security/gitlab@7700354a9e5bd11b8db8e6b116d6708c9ef15e72) ([merge request](gitlab-org/security/gitlab!3699)) +- [Do not run ssl cert validation if key has errors](gitlab-org/security/gitlab@a585a7ad29319b9cdaa6086287251ac34b0cd2be) ([merge request](gitlab-org/security/gitlab!3702)) +- [Ensure access is checked when loading releases associated with tags](gitlab-org/security/gitlab@68cb75d412db5e1fe97823f21cd848299cb1c969) ([merge request](gitlab-org/security/gitlab!3701)) +- [XSS and ReDoS in Markdown via Banzai pipeline of Jira](gitlab-org/security/gitlab@4ab2701284c928a392b5390977e4daed30b1b39f) ([merge request](gitlab-org/security/gitlab!3697)) +- [Prevent branch names starting with SHA-1 and SHA-256 values](gitlab-org/security/gitlab@cc65b6c8c94b1b647995fe5f2d6afd23cc621f12) ([merge request](gitlab-org/security/gitlab!3698)) +- [Filter out projects with disabled package registry in Composer finder](gitlab-org/security/gitlab@576f1ee9a3b612a579f987471e59dcd4820f5bd4) ([merge request](gitlab-org/security/gitlab!3684)) +- [Check max role for user for group access to protected ref](gitlab-org/security/gitlab@1e39ee42f24588675336da5b95a9863ee46b33c4) ([merge request](gitlab-org/security/gitlab!3700)) +- [Treat security policy bots as external](gitlab-org/security/gitlab@487e39c72883c71f5a4149191c9580017b0babd2) ([merge request](gitlab-org/security/gitlab!3678)) + ## 16.6.0 (2023-11-15) ### Added (117 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index fe194a26408..235d052b5f0 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -16.6.0 \ No newline at end of file +16.6.1 \ No newline at end of file diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index fe194a26408..235d052b5f0 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -16.6.0 \ No newline at end of file +16.6.1 \ No newline at end of file -- cgit v1.2.3