From 62e0c3c7d73f028e4c6c8c179d6f04f811a0859f Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 27 Jul 2022 19:06:07 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-2-stable-ee --- GITLAB_PAGES_VERSION | 2 +- app/controllers/autocomplete_controller.rb | 5 ++ app/finders/issuable_finder.rb | 20 ++++++ app/models/hooks/web_hook_log.rb | 7 ++ app/serializers/build_details_entity.rb | 2 +- config/gitlab_loose_foreign_keys.yml | 3 + ...0330_add_author_to_ci_subscriptions_projects.rb | 20 ++++++ db/schema_migrations/20220329130330 | 1 + db/structure.sql | 5 +- spec/controllers/autocomplete_controller_spec.rb | 77 ++++++++++++---------- spec/graphql/resolvers/issues_resolver_spec.rb | 11 +++- spec/lib/gitlab/import_export/all_models.yml | 1 - spec/models/hooks/web_hook_log_spec.rb | 35 ++++++++-- spec/serializers/build_details_entity_spec.rb | 18 +++++ .../finders/issues_finder_shared_examples.rb | 71 +++++++++++++------- 15 files changed, 207 insertions(+), 71 deletions(-) create mode 100644 db/migrate/20220329130330_add_author_to_ci_subscriptions_projects.rb create mode 100644 db/schema_migrations/20220329130330 diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index 91951fd8ad7..30e298c7494 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -1.61.0 +1.61.1 diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index 32d1ddf920e..6d1ffc1f2e8 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -5,6 +5,7 @@ class AutocompleteController < ApplicationController skip_before_action :authenticate_user!, only: [:users, :award_emojis, :merge_request_target_branches] before_action :check_search_rate_limit!, only: [:users, :projects] + before_action :authorize_admin_project, only: :deploy_keys_with_owners feature_category :users, [:users, :user] feature_category :projects, [:projects] @@ -69,6 +70,10 @@ class AutocompleteController < ApplicationController private + def authorize_admin_project + render_403 unless Ability.allowed?(current_user, :admin_project, project) + end + def project @project ||= Autocomplete::ProjectFinder .new(current_user, params) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 8ecf0c158e0..47b2a460e6f 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -483,10 +483,14 @@ class IssuableFinder end def by_crm_contact(items) + return items unless can_filter_by_crm_contact? + Issuables::CrmContactFilter.new(params: original_params).filter(items) end def by_crm_organization(items) + return items unless can_filter_by_crm_organization? + Issuables::CrmOrganizationFilter.new(params: original_params).filter(items) end @@ -499,4 +503,20 @@ class IssuableFinder def feature_flag_scope params.group || params.project end + + def can_filter_by_crm_contact? + current_user&.can?(:read_crm_contact, root_group) + end + + def can_filter_by_crm_organization? + current_user&.can?(:read_crm_organization, root_group) + end + + def root_group + strong_memoize(:root_group) do + base_group = params.group || params.project&.group + + base_group&.root_ancestor + end + end end diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb index 2f03b3591cf..24e5f193a32 100644 --- a/app/models/hooks/web_hook_log.rb +++ b/app/models/hooks/web_hook_log.rb @@ -22,6 +22,7 @@ class WebHookLog < ApplicationRecord validates :web_hook, presence: true before_save :obfuscate_basic_auth + before_save :redact_author_email def self.recent where('created_at >= ?', 2.days.ago.beginning_of_day) @@ -52,4 +53,10 @@ class WebHookLog < ApplicationRecord def obfuscate_basic_auth self.url = safe_url end + + def redact_author_email + return unless self.request_data.dig('commit', 'author', 'email').present? + + self.request_data['commit']['author']['email'] = _('[REDACTED]') + end end diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb index 5f72259f34a..dc7b5e95361 100644 --- a/app/serializers/build_details_entity.rb +++ b/app/serializers/build_details_entity.rb @@ -151,7 +151,7 @@ class BuildDetailsEntity < Ci::JobEntity # We do not return the invalid_dependencies for all scenarios see https://gitlab.com/gitlab-org/gitlab/-/issues/287772#note_914406387 punctuation = invalid_dependencies.empty? ? '.' : ': ' _("This job could not start because it could not retrieve the needed artifacts%{punctuation}%{invalid_dependencies}") % - { invalid_dependencies: invalid_dependencies, punctuation: punctuation } + { invalid_dependencies: html_escape(invalid_dependencies), punctuation: punctuation } end def help_message(docs_url) diff --git a/config/gitlab_loose_foreign_keys.yml b/config/gitlab_loose_foreign_keys.yml index 8a9f3f0da43..c5c2d0a61b9 100644 --- a/config/gitlab_loose_foreign_keys.yml +++ b/config/gitlab_loose_foreign_keys.yml @@ -144,6 +144,9 @@ ci_subscriptions_projects: - table: projects column: upstream_project_id on_delete: async_delete + - table: users + column: author_id + on_delete: async_delete ci_triggers: - table: users column: owner_id diff --git a/db/migrate/20220329130330_add_author_to_ci_subscriptions_projects.rb b/db/migrate/20220329130330_add_author_to_ci_subscriptions_projects.rb new file mode 100644 index 00000000000..b1d0ac64d42 --- /dev/null +++ b/db/migrate/20220329130330_add_author_to_ci_subscriptions_projects.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AddAuthorToCiSubscriptionsProjects < Gitlab::Database::Migration[2.0] + disable_ddl_transaction! + + INDEX_NAME = 'index_ci_subscriptions_projects_author_id' + + def up + unless column_exists?(:ci_subscriptions_projects, :author_id) + add_column :ci_subscriptions_projects, :author_id, :bigint + end + + add_concurrent_index :ci_subscriptions_projects, :author_id, name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :ci_subscriptions_projects, INDEX_NAME + remove_column :ci_subscriptions_projects, :author_id + end +end diff --git a/db/schema_migrations/20220329130330 b/db/schema_migrations/20220329130330 new file mode 100644 index 00000000000..367d43a89a2 --- /dev/null +++ b/db/schema_migrations/20220329130330 @@ -0,0 +1 @@ +8726707f9f4bb8d256886c592b6a11ba8487de24f5340c837800f5ce0c32df9d \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 28082885574..cc1e6dfb288 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13289,7 +13289,8 @@ ALTER SEQUENCE ci_stages_id_seq OWNED BY ci_stages.id; CREATE TABLE ci_subscriptions_projects ( id bigint NOT NULL, downstream_project_id bigint NOT NULL, - upstream_project_id bigint NOT NULL + upstream_project_id bigint NOT NULL, + author_id bigint ); CREATE SEQUENCE ci_subscriptions_projects_id_seq @@ -27805,6 +27806,8 @@ CREATE INDEX index_ci_stages_on_pipeline_id_and_position ON ci_stages USING btre CREATE INDEX index_ci_stages_on_project_id ON ci_stages USING btree (project_id); +CREATE INDEX index_ci_subscriptions_projects_author_id ON ci_subscriptions_projects USING btree (author_id); + CREATE INDEX index_ci_subscriptions_projects_on_upstream_project_id ON ci_subscriptions_projects USING btree (upstream_project_id); CREATE UNIQUE INDEX index_ci_subscriptions_projects_unique_subscription ON ci_subscriptions_projects USING btree (downstream_project_id, upstream_project_id); diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb index e874df62cd7..70e58124d21 100644 --- a/spec/controllers/autocomplete_controller_spec.rb +++ b/spec/controllers/autocomplete_controller_spec.rb @@ -378,63 +378,74 @@ RSpec.describe AutocompleteController do end context 'GET deploy_keys_with_owners' do - let!(:deploy_key) { create(:deploy_key, user: user) } - let!(:deploy_keys_project) { create(:deploy_keys_project, :write_access, project: project, deploy_key: deploy_key) } + let_it_be(:public_project) { create(:project, :public) } + let_it_be(:user) { create(:user) } + let_it_be(:deploy_key) { create(:deploy_key, user: user) } + let_it_be(:deploy_keys_project) do + create(:deploy_keys_project, :write_access, project: public_project, deploy_key: deploy_key) + end context 'unauthorized user' do it 'returns a not found response' do - get(:deploy_keys_with_owners, params: { project_id: project.id }) + get(:deploy_keys_with_owners, params: { project_id: public_project.id }) expect(response).to have_gitlab_http_status(:redirect) end end - context 'when the user who can read the project is logged in' do + context 'when the user is logged in' do before do sign_in(user) end - context 'and they cannot read the project' do + context 'with a non-existing project' do it 'returns a not found response' do - allow(Ability).to receive(:allowed?).and_call_original - allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(false) - - get(:deploy_keys_with_owners, params: { project_id: project.id }) + get(:deploy_keys_with_owners, params: { project_id: 9999 }) expect(response).to have_gitlab_http_status(:not_found) end end - it 'renders the deploy key in a json payload, with its owner' do - get(:deploy_keys_with_owners, params: { project_id: project.id }) + context 'with an existing project' do + context 'when user cannot admin project' do + it 'returns a forbidden response' do + get(:deploy_keys_with_owners, params: { project_id: public_project.id }) - expect(json_response.count).to eq(1) - expect(json_response.first['title']).to eq(deploy_key.title) - expect(json_response.first['owner']['id']).to eq(deploy_key.user.id) - expect(json_response.first['deploy_keys_projects']).to be_nil - end + expect(response).to have_gitlab_http_status(:forbidden) + end + end - context 'with an unknown project' do - it 'returns a not found response' do - get(:deploy_keys_with_owners, params: { project_id: 9999 }) + context 'when user can admin project' do + before do + public_project.add_maintainer(user) + end - expect(response).to have_gitlab_http_status(:not_found) - end - end + context 'and user can read owner of key' do + it 'renders the deploy keys in a json payload, with owner' do + get(:deploy_keys_with_owners, params: { project_id: public_project.id }) - context 'and the user cannot read the owner of the key' do - before do - allow(Ability).to receive(:allowed?).and_call_original - allow(Ability).to receive(:allowed?).with(user, :read_user, deploy_key.user).and_return(false) - end + expect(json_response.count).to eq(1) + expect(json_response.first['title']).to eq(deploy_key.title) + expect(json_response.first['owner']['id']).to eq(deploy_key.user.id) + expect(json_response.first['deploy_keys_projects']).to be_nil + end + end + + context 'and user cannot read owner of key' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :read_user, deploy_key.user).and_return(false) + end - it 'returns a payload without owner' do - get(:deploy_keys_with_owners, params: { project_id: project.id }) + it 'returns a payload without owner' do + get(:deploy_keys_with_owners, params: { project_id: public_project.id }) - expect(json_response.count).to eq(1) - expect(json_response.first['title']).to eq(deploy_key.title) - expect(json_response.first['owner']).to be_nil - expect(json_response.first['deploy_keys_projects']).to be_nil + expect(json_response.count).to eq(1) + expect(json_response.first['title']).to eq(deploy_key.title) + expect(json_response.first['owner']).to be_nil + expect(json_response.first['deploy_keys_projects']).to be_nil + end + end end end end diff --git a/spec/graphql/resolvers/issues_resolver_spec.rb b/spec/graphql/resolvers/issues_resolver_spec.rb index a5b5a8e4f72..89e45810033 100644 --- a/spec/graphql/resolvers/issues_resolver_spec.rb +++ b/spec/graphql/resolvers/issues_resolver_spec.rb @@ -30,6 +30,9 @@ RSpec.describe Resolvers::IssuesResolver do before_all do project.add_developer(current_user) project.add_reporter(reporter) + + create(:crm_settings, group: group, enabled: true) + create(:label_link, label: label1, target: issue1) create(:label_link, label: label1, target: issue2) create(:label_link, label: label2, target: issue2) @@ -399,6 +402,8 @@ RSpec.describe Resolvers::IssuesResolver do let_it_be(:crm_issue3) { create(:issue, project: project) } before_all do + group.add_developer(current_user) + create(:issue_customer_relations_contact, issue: crm_issue1, contact: contact1) create(:issue_customer_relations_contact, issue: crm_issue2, contact: contact2) create(:issue_customer_relations_contact, issue: crm_issue3, contact: contact3) @@ -631,13 +636,13 @@ RSpec.describe Resolvers::IssuesResolver do end it 'finds a specific issue with iid', :request_store do - result = batch_sync(max_queries: 7) { resolve_issues(iid: issue1.iid).to_a } + result = batch_sync(max_queries: 8) { resolve_issues(iid: issue1.iid).to_a } expect(result).to contain_exactly(issue1) end it 'batches queries that only include IIDs', :request_store do - result = batch_sync(max_queries: 7) do + result = batch_sync(max_queries: 8) do [issue1, issue2] .map { |issue| resolve_issues(iid: issue.iid.to_s) } .flat_map(&:to_a) @@ -647,7 +652,7 @@ RSpec.describe Resolvers::IssuesResolver do end it 'finds a specific issue with iids', :request_store do - result = batch_sync(max_queries: 7) do + result = batch_sync(max_queries: 8) do resolve_issues(iids: [issue1.iid]).to_a end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index af910b08fae..8c1e60e78b0 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -572,7 +572,6 @@ project: - remove_source_branch_after_merge - deleting_user - upstream_projects -- downstream_projects - upstream_project_subscriptions - downstream_project_subscriptions - service_desk_setting diff --git a/spec/models/hooks/web_hook_log_spec.rb b/spec/models/hooks/web_hook_log_spec.rb index e1fea3318f6..8ff8a1c3865 100644 --- a/spec/models/hooks/web_hook_log_spec.rb +++ b/spec/models/hooks/web_hook_log_spec.rb @@ -30,15 +30,12 @@ RSpec.describe WebHookLog do end describe '#save' do - let(:web_hook_log) { build(:web_hook_log, url: url) } - let(:url) { 'http://example.com' } - - subject { web_hook_log.save! } + context 'with basic auth credentials' do + let(:web_hook_log) { build(:web_hook_log, url: 'http://test:123@example.com') } - it { is_expected.to eq(true) } + subject { web_hook_log.save! } - context 'with basic auth credentials' do - let(:url) { 'http://test:123@example.com'} + it { is_expected.to eq(true) } it 'obfuscates the basic auth credentials' do subject @@ -46,6 +43,30 @@ RSpec.describe WebHookLog do expect(web_hook_log.url).to eq('http://*****:*****@example.com') end end + + context 'with author email' do + let(:author) { create(:user) } + let(:web_hook_log) { create(:web_hook_log, request_data: data) } + let(:data) do + { + commit: { + author: { + name: author.name, + email: author.email + } + } + }.deep_stringify_keys + end + + it "redacts author's email" do + expect(web_hook_log.request_data['commit']).to match a_hash_including( + 'author' => { + 'name' => author.name, + 'email' => _('[REDACTED]') + } + ) + end + end end describe '.delete_batch_for' do diff --git a/spec/serializers/build_details_entity_spec.rb b/spec/serializers/build_details_entity_spec.rb index dd8238456aa..916798c669c 100644 --- a/spec/serializers/build_details_entity_spec.rb +++ b/spec/serializers/build_details_entity_spec.rb @@ -170,6 +170,24 @@ RSpec.describe BuildDetailsEntity do expect(message).to include('could not retrieve the needed artifacts.') end end + + context 'when dependency contains invalid dependency names' do + invalid_name = 'XSS' + let!(:test1) { create(:ci_build, :success, :expired, pipeline: pipeline, name: invalid_name, stage_idx: 0) } + let!(:build) { create(:ci_build, :pending, pipeline: pipeline, stage_idx: 1, options: { dependencies: [invalid_name] }) } + + before do + build.pipeline.unlocked! + build.drop!(:missing_dependency_failure) + end + + it { is_expected.to include(failure_reason: 'missing_dependency_failure') } + + it 'escapes the invalid dependency names' do + escaped_name = html_escape(invalid_name) + expect(message).to include(escaped_name) + end + end end context 'when a build has environment with latest deployment' do diff --git a/spec/support/shared_examples/finders/issues_finder_shared_examples.rb b/spec/support/shared_examples/finders/issues_finder_shared_examples.rb index 9d8f37a3e64..049ead9fb89 100644 --- a/spec/support/shared_examples/finders/issues_finder_shared_examples.rb +++ b/spec/support/shared_examples/finders/issues_finder_shared_examples.rb @@ -914,42 +914,65 @@ RSpec.shared_examples 'issues or work items finder' do |factory, execute_context end end - context 'filtering by crm contact' do - let_it_be(:contact1) { create(:contact, group: group) } - let_it_be(:contact2) { create(:contact, group: group) } + context 'crm filtering' do + let_it_be(:root_group) { create(:group) } + let_it_be(:group) { create(:group, parent: root_group) } + let_it_be(:project_crm) { create(:project, :public, group: group) } + let_it_be(:organization) { create(:organization, group: root_group) } + let_it_be(:contact1) { create(:contact, group: root_group, organization: organization) } + let_it_be(:contact2) { create(:contact, group: root_group, organization: organization) } - let_it_be(:contact1_item1) { create(factory, project: project1) } - let_it_be(:contact1_item2) { create(factory, project: project1) } - let_it_be(:contact2_item1) { create(factory, project: project1) } + let_it_be(:contact1_item1) { create(factory, project: project_crm) } + let_it_be(:contact1_item2) { create(factory, project: project_crm) } + let_it_be(:contact2_item1) { create(factory, project: project_crm) } + let_it_be(:item_no_contact) { create(factory, project: project_crm) } - let(:params) { { crm_contact_id: contact1.id } } + let_it_be(:all_project_issues) do + [contact1_item1, contact1_item2, contact2_item1, item_no_contact] + end + + before do + create(:crm_settings, group: root_group, enabled: true) - it 'returns for that contact' do create(:issue_customer_relations_contact, issue: contact1_item1, contact: contact1) create(:issue_customer_relations_contact, issue: contact1_item2, contact: contact1) create(:issue_customer_relations_contact, issue: contact2_item1, contact: contact2) - - expect(items).to contain_exactly(contact1_item1, contact1_item2) end - end - context 'filtering by crm organization' do - let_it_be(:organization) { create(:organization, group: group) } - let_it_be(:contact1) { create(:contact, group: group, organization: organization) } - let_it_be(:contact2) { create(:contact, group: group, organization: organization) } + context 'filtering by crm contact' do + let(:params) { { project_id: project_crm.id, crm_contact_id: contact1.id } } - let_it_be(:contact1_item1) { create(factory, project: project1) } - let_it_be(:contact1_item2) { create(factory, project: project1) } - let_it_be(:contact2_item1) { create(factory, project: project1) } + context 'when the user can read crm contacts' do + it 'returns for that contact' do + root_group.add_reporter(user) - let(:params) { { crm_organization_id: organization.id } } + expect(items).to contain_exactly(contact1_item1, contact1_item2) + end + end - it 'returns for that contact' do - create(:issue_customer_relations_contact, issue: contact1_item1, contact: contact1) - create(:issue_customer_relations_contact, issue: contact1_item2, contact: contact1) - create(:issue_customer_relations_contact, issue: contact2_item1, contact: contact2) + context 'when the user can not read crm contacts' do + it 'does not filter by contact' do + expect(items).to match_array(all_project_issues) + end + end + end + + context 'filtering by crm organization' do + let(:params) { { project_id: project_crm.id, crm_organization_id: organization.id } } + + context 'when the user can read crm organization' do + it 'returns for that organization' do + root_group.add_reporter(user) - expect(items).to contain_exactly(contact1_item1, contact1_item2, contact2_item1) + expect(items).to contain_exactly(contact1_item1, contact1_item2, contact2_item1) + end + end + + context 'when the user can not read crm organization' do + it 'does not filter by organization' do + expect(items).to match_array(all_project_issues) + end + end end end -- cgit v1.2.3