From f132561154314b6fc9050fc7f3d0baf34c13c44b Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Mon, 16 Sep 2019 17:50:41 +0800 Subject: EE port: Fix private feature Elasticsearch leak Add spec to test different combinations. Accept string for required_minimum_access_level Allow more flexible project membership query --- app/models/project.rb | 7 +- app/models/project_feature.rb | 3 +- changelogs/unreleased/security-29491.yml | 5 + spec/models/project_feature_spec.rb | 24 +++++ spec/models/project_spec.rb | 30 ++++++ spec/support/helpers/project_helpers.rb | 25 +++++ .../project_policy_table_shared_context.rb | 118 +++++++++++++++++++++ 7 files changed, 210 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-29491.yml create mode 100644 spec/support/helpers/project_helpers.rb create mode 100644 spec/support/shared_contexts/policies/project_policy_table_shared_context.rb diff --git a/app/models/project.rb b/app/models/project.rb index 7c065db9829..5c3bf4a3b5d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -468,7 +468,7 @@ class Project < ApplicationRecord # the feature is either public, enabled, or internal with permission for the user. # Note: this scope doesn't enforce that the user has access to the projects, it just checks # that the user has access to the feature. It's important to use this scope with others - # that checks project authorizations first. + # that checks project authorizations first (e.g. `filter_by_feature_visibility`). # # This method uses an optimised version of `with_feature_access_level` for # logged in users to more efficiently get private projects with the given @@ -496,6 +496,11 @@ class Project < ApplicationRecord end end + # This scope returns projects where user has access to both the project and the feature. + def self.filter_by_feature_visibility(feature, user) + with_feature_available_for_user(feature, user).public_or_visible_to_user(user) + end + scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') } scope :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) } diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index 13b20b1fead..2013f620b5b 100644 --- a/app/models/project_feature.rb +++ b/app/models/project_feature.rb @@ -62,7 +62,8 @@ class ProjectFeature < ApplicationRecord private def ensure_feature!(feature) - feature = feature.model_name.plural.to_sym if feature.respond_to?(:model_name) + feature = feature.model_name.plural if feature.respond_to?(:model_name) + feature = feature.to_sym raise ArgumentError, "invalid project feature: #{feature}" unless FEATURES.include?(feature) feature diff --git a/changelogs/unreleased/security-29491.yml b/changelogs/unreleased/security-29491.yml new file mode 100644 index 00000000000..ec4ada47c62 --- /dev/null +++ b/changelogs/unreleased/security-29491.yml @@ -0,0 +1,5 @@ +--- +title: Fix private feature Elasticsearch leak +merge_request: +author: +type: security diff --git a/spec/models/project_feature_spec.rb b/spec/models/project_feature_spec.rb index e6505bb4a51..5b448c343a0 100644 --- a/spec/models/project_feature_spec.rb +++ b/spec/models/project_feature_spec.rb @@ -228,4 +228,28 @@ describe ProjectFeature do end end end + + describe '.required_minimum_access_level' do + it 'handles reporter level' do + expect(described_class.required_minimum_access_level(:merge_requests)).to eq(Gitlab::Access::REPORTER) + end + + it 'handles guest level' do + expect(described_class.required_minimum_access_level(:issues)).to eq(Gitlab::Access::GUEST) + end + + it 'accepts ActiveModel' do + expect(described_class.required_minimum_access_level(MergeRequest)).to eq(Gitlab::Access::REPORTER) + end + + it 'accepts string' do + expect(described_class.required_minimum_access_level('merge_requests')).to eq(Gitlab::Access::REPORTER) + end + + it 'raises error if feature is invalid' do + expect do + described_class.required_minimum_access_level(:foos) + end.to raise_error + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 67f64822184..7fe48e66def 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3462,6 +3462,36 @@ describe Project do end end + describe '.filter_by_feature_visibility' do + include_context 'ProjectPolicyTable context' + include ProjectHelpers + using RSpec::Parameterized::TableSyntax + + set(:group) { create(:group) } + let!(:project) { create(:project, project_level, namespace: group ) } + let(:user) { create_user_from_membership(project, membership) } + + context 'reporter level access' do + let(:feature) { MergeRequest } + + where(:project_level, :feature_access_level, :membership, :expected_count) do + permission_table_for_reporter_feature_access + end + + with_them do + it "respects visibility" do + update_feature_access_level(project, feature_access_level) + + expected_objects = expected_count == 1 ? [project] : [] + + expect( + described_class.filter_by_feature_visibility(feature, user) + ).to eq(expected_objects) + end + end + end + end + describe '#pages_available?' do let(:project) { create(:project, group: group) } diff --git a/spec/support/helpers/project_helpers.rb b/spec/support/helpers/project_helpers.rb new file mode 100644 index 00000000000..61056b47aed --- /dev/null +++ b/spec/support/helpers/project_helpers.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module ProjectHelpers + # @params target [Project] membership target + # @params membership [Symbol] accepts the membership levels :guest, :reporter... + # and phony levels :non_member and :anonymous + def create_user_from_membership(target, membership) + case membership + when :anonymous + nil + when :non_member + create(:user, name: membership) + else + create(:user, name: membership).tap { |u| target.add_user(u, membership) } + end + end + + def update_feature_access_level(project, access_level) + project.update!( + repository_access_level: access_level, + merge_requests_access_level: access_level, + builds_access_level: access_level + ) + end +end diff --git a/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb b/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb new file mode 100644 index 00000000000..e666b346b8b --- /dev/null +++ b/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +RSpec.shared_context 'ProjectPolicyTable context' do + using RSpec::Parameterized::TableSyntax + + # rubocop:disable Metrics/AbcSize + def permission_table_for_reporter_feature_access + :public | :enabled | :reporter | 1 + :public | :enabled | :guest | 1 + :public | :enabled | :non_member | 1 + :public | :enabled | :anonymous | 1 + + :public | :private | :reporter | 1 + :public | :private | :guest | 0 + :public | :private | :non_member | 0 + :public | :private | :anonymous | 0 + + :public | :disabled | :reporter | 0 + :public | :disabled | :guest | 0 + :public | :disabled | :non_member | 0 + :public | :disabled | :anonymous | 0 + + :internal | :enabled | :reporter | 1 + :internal | :enabled | :guest | 1 + :internal | :enabled | :non_member | 1 + :internal | :enabled | :anonymous | 0 + + :internal | :private | :reporter | 1 + :internal | :private | :guest | 0 + :internal | :private | :non_member | 0 + :internal | :private | :anonymous | 0 + + :internal | :disabled | :reporter | 0 + :internal | :disabled | :guest | 0 + :internal | :disabled | :non_member | 0 + :internal | :disabled | :anonymous | 0 + + :private | :enabled | :reporter | 1 + :private | :enabled | :guest | 1 + :private | :enabled | :non_member | 0 + :private | :enabled | :anonymous | 0 + + :private | :private | :reporter | 1 + :private | :private | :guest | 0 + :private | :private | :non_member | 0 + :private | :private | :anonymous | 0 + + :private | :disabled | :reporter | 0 + :private | :disabled | :guest | 0 + :private | :disabled | :non_member | 0 + :private | :disabled | :anonymous | 0 + end + + def permission_table_for_guest_feature_access + :public | :enabled | :reporter | 1 + :public | :enabled | :guest | 1 + :public | :enabled | :non_member | 1 + :public | :enabled | :anonymous | 1 + + :public | :private | :reporter | 1 + :public | :private | :guest | 1 + :public | :private | :non_member | 0 + :public | :private | :anonymous | 0 + + :public | :disabled | :reporter | 0 + :public | :disabled | :guest | 0 + :public | :disabled | :non_member | 0 + :public | :disabled | :anonymous | 0 + + :internal | :enabled | :reporter | 1 + :internal | :enabled | :guest | 1 + :internal | :enabled | :non_member | 1 + :internal | :enabled | :anonymous | 0 + + :internal | :private | :reporter | 1 + :internal | :private | :guest | 1 + :internal | :private | :non_member | 0 + :internal | :private | :anonymous | 0 + + :internal | :disabled | :reporter | 0 + :internal | :disabled | :guest | 0 + :internal | :disabled | :non_member | 0 + :internal | :disabled | :anonymous | 0 + + :private | :enabled | :reporter | 1 + :private | :enabled | :guest | 1 + :private | :enabled | :non_member | 0 + :private | :enabled | :anonymous | 0 + + :private | :private | :reporter | 1 + :private | :private | :guest | 1 + :private | :private | :non_member | 0 + :private | :private | :anonymous | 0 + + :private | :disabled | :reporter | 0 + :private | :disabled | :guest | 0 + :private | :disabled | :non_member | 0 + :private | :disabled | :anonymous | 0 + end + + def permission_table_for_project_access + :public | :reporter | 1 + :public | :guest | 1 + :public | :non_member | 1 + :public | :anonymous | 1 + + :internal | :reporter | 1 + :internal | :guest | 1 + :internal | :non_member | 1 + :internal | :anonymous | 0 + + :private | :reporter | 1 + :private | :guest | 1 + :private | :non_member | 0 + :private | :anonymous | 0 + end + # rubocop:enable Metrics/AbcSize +end -- cgit v1.2.1 From e0eebfc0ea5bd69c0aa9f2c8b29f363dd5019b49 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Tue, 1 Oct 2019 17:03:58 +0000 Subject: Update CHANGELOG.md for 12.3.3 [ci skip] --- CHANGELOG.md | 7 +++++++ changelogs/unreleased/security-29491.yml | 5 ----- 2 files changed, 7 insertions(+), 5 deletions(-) delete mode 100644 changelogs/unreleased/security-29491.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index ae76d31dd24..1f5eabd58f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 12.3.3 + +### Security (1 change) + +- Fix private feature Elasticsearch leak. + + ## 12.3.2 ### Security (10 changes) diff --git a/changelogs/unreleased/security-29491.yml b/changelogs/unreleased/security-29491.yml deleted file mode 100644 index ec4ada47c62..00000000000 --- a/changelogs/unreleased/security-29491.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix private feature Elasticsearch leak -merge_request: -author: -type: security -- cgit v1.2.1 From ab531f31825d8d5aedce12d9975ecaa3e2720417 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Tue, 1 Oct 2019 17:05:23 +0000 Subject: Update VERSION to 12.3.3 --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 1701b30e164..021d5996174 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -12.3.2 +12.3.3 -- cgit v1.2.1