diff options
author | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2023-06-29 11:22:26 +0300 |
---|---|---|
committer | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2023-06-29 11:22:26 +0300 |
commit | 751cb432aab9837d3174bcdb309fae765925c869 (patch) | |
tree | 96b513d6f2fb66af92cf69de69c40487a980e2d4 | |
parent | 7b848eda5589ff5fa1bc3c6f782fc907c59a4417 (diff) | |
parent | 9ce736bb2cdbb3e28c522af172d595826f03d516 (diff) |
Merge remote-tracking branch 'dev/16-1-stable' into 16-1-stable
-rw-r--r-- | .rubocop_todo/gitlab/strong_memoize_attr.yml | 1 | ||||
-rw-r--r-- | .rubocop_todo/rspec/missing_feature_category.yml | 1 | ||||
-rw-r--r-- | CHANGELOG.md | 17 | ||||
-rw-r--r-- | GITALY_SERVER_VERSION | 2 | ||||
-rw-r--r-- | GITLAB_PAGES_VERSION | 2 | ||||
-rw-r--r-- | VERSION | 2 | ||||
-rw-r--r-- | app/controllers/concerns/analytics/cycle_analytics/value_stream_actions.rb | 7 | ||||
-rw-r--r-- | app/controllers/import/github_controller.rb | 13 | ||||
-rw-r--r-- | app/controllers/projects/compare_controller.rb | 12 | ||||
-rw-r--r-- | app/helpers/users_helper.rb | 2 | ||||
-rw-r--r-- | app/models/hooks/web_hook.rb | 1 | ||||
-rw-r--r-- | lib/api/entities/issue.rb | 9 | ||||
-rw-r--r-- | lib/banzai/filter/references/reference_cache.rb | 31 | ||||
-rw-r--r-- | lib/gitlab/sidekiq_middleware/arguments_logger.rb | 3 | ||||
-rw-r--r-- | spec/controllers/admin/hooks_controller_spec.rb | 9 | ||||
-rw-r--r-- | spec/controllers/import/github_controller_spec.rb | 32 | ||||
-rw-r--r-- | spec/controllers/projects/compare_controller_spec.rb | 32 | ||||
-rw-r--r-- | spec/features/admin/users/user_spec.rb | 8 | ||||
-rw-r--r-- | spec/lib/api/entities/issue_spec.rb | 47 | ||||
-rw-r--r-- | spec/models/hooks/web_hook_spec.rb | 7 |
20 files changed, 198 insertions, 40 deletions
diff --git a/.rubocop_todo/gitlab/strong_memoize_attr.yml b/.rubocop_todo/gitlab/strong_memoize_attr.yml index a6c273cc85b..62457a0108e 100644 --- a/.rubocop_todo/gitlab/strong_memoize_attr.yml +++ b/.rubocop_todo/gitlab/strong_memoize_attr.yml @@ -329,7 +329,6 @@ Gitlab/StrongMemoizeAttr: - 'ee/app/models/vulnerabilities/finding.rb' - 'ee/app/presenters/approval_rule_presenter.rb' - 'ee/app/presenters/ci/minutes/usage_presenter.rb' - - 'ee/app/presenters/merge_request_approver_presenter.rb' - 'ee/app/serializers/dashboard_operations_project_entity.rb' - 'ee/app/serializers/ee/member_user_entity.rb' - 'ee/app/services/app_sec/dast/pipelines/find_latest_service.rb' diff --git a/.rubocop_todo/rspec/missing_feature_category.yml b/.rubocop_todo/rspec/missing_feature_category.yml index 0835191b8f4..1f90a4c3ba5 100644 --- a/.rubocop_todo/rspec/missing_feature_category.yml +++ b/.rubocop_todo/rspec/missing_feature_category.yml @@ -1011,7 +1011,6 @@ RSpec/MissingFeatureCategory: - 'ee/spec/models/approval_merge_request_rule_spec.rb' - 'ee/spec/models/approval_state_spec.rb' - 'ee/spec/models/approval_wrapped_any_approver_rule_spec.rb' - - 'ee/spec/models/approval_wrapped_code_owner_rule_spec.rb' - 'ee/spec/models/approval_wrapped_rule_spec.rb' - 'ee/spec/models/approvals/scan_finding_wrapped_rule_set_spec.rb' - 'ee/spec/models/approvals/wrapped_rule_set_spec.rb' diff --git a/CHANGELOG.md b/CHANGELOG.md index 406f3286c33..5e9ba59743a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,23 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 16.1.1 (2023-06-28) + +### Security (12 changes) + +- [Revert 'security-leaked-ci-job-token-permission-16-1' from '16-1'](gitlab-org/security/gitlab@d2599119b120eab983a1446fc9ed3ca801c88368) ([merge request](gitlab-org/security/gitlab!3374)) +- [Use fully qualified ref when loading code owner file](gitlab-org/security/gitlab@e8ba90bb85de376bb020350c027bb369671c83d6) ([merge request](gitlab-org/security/gitlab!3356)) +- [Maintainer can leak masked webhook secrets by manipulating URL masking](gitlab-org/security/gitlab@2cf91108544e8c30aae6d9b207385c90c299869c) ([merge request](gitlab-org/security/gitlab!3359)) +- [Remove approvals when the only commit gets amended](gitlab-org/security/gitlab@3f81f7bc4236bcc2ed887f40b7a14702d756ca9e) ([merge request](gitlab-org/security/gitlab!3366)) +- [Add authorization validation to GithubController#failures action](gitlab-org/security/gitlab@3c8c305deef9c9bd1194788b40e0d7ae1de45f3b) ([merge request](gitlab-org/security/gitlab!3335)) +- [Fix for fork permissions check in compare controller](gitlab-org/security/gitlab@5b14436f3874de7be62e0f46a25e93a1d8c99975) ([merge request](gitlab-org/security/gitlab!3342)) +- [Webhook token leaked in Sidekiq logs if log format is 'default'](gitlab-org/security/gitlab@d2d76399c880c62d7449cdae6014ee3236bffc0b) ([merge request](gitlab-org/security/gitlab!3345)) +- [Mitigate epic reference filter ReDOS](gitlab-org/security/gitlab@874d5bc2d55e2e1092bf7cc4ebb0e53fc716d850) ([merge request](gitlab-org/security/gitlab!3341)) +- [Increasing security for CI_JOB_TOKEN on public and internal projects](gitlab-org/security/gitlab@c2aa392b932af04e395d67eb06a20b5c768ec683) ([merge request](gitlab-org/security/gitlab!3337)) +- [Adjust access to value stream create, edit and destroy actions](gitlab-org/security/gitlab@8a3645e265c71886951bdc03857837aacb57e558) ([merge request](gitlab-org/security/gitlab!3349)) +- [Sanitize user email addresses in admin confirm user dialog](gitlab-org/security/gitlab@70553e6ca6b3f244df37e306466e2d3b5d54f76b) ([merge request](gitlab-org/security/gitlab!3338)) +- [Obfuscate email of service desk issue creator in issue REST API](gitlab-org/security/gitlab@d0f27b8241ab53bee11f8ce6efb20811690a2d0d) ([merge request](gitlab-org/security/gitlab!3317)) + ## 16.1.0 (2023-06-21) ### Added (224 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 11155dbeea0..c9942a08a60 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -16.1.0
\ No newline at end of file +16.1.1
\ No newline at end of file diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index 11155dbeea0..c9942a08a60 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -16.1.0
\ No newline at end of file +16.1.1
\ No newline at end of file @@ -1 +1 @@ -16.1.0
\ No newline at end of file +16.1.1
\ No newline at end of file diff --git a/app/controllers/concerns/analytics/cycle_analytics/value_stream_actions.rb b/app/controllers/concerns/analytics/cycle_analytics/value_stream_actions.rb index f10b23d1664..cf0430307a3 100644 --- a/app/controllers/concerns/analytics/cycle_analytics/value_stream_actions.rb +++ b/app/controllers/concerns/analytics/cycle_analytics/value_stream_actions.rb @@ -7,6 +7,9 @@ module Analytics included do before_action :authorize + # Defining the before action here, because in the EE module we cannot define a before_action. + # Reason: this is a module which is being included into a controller. This module is extended in EE. + before_action :authorize_modification, only: %i[create destroy update] # rubocop:disable Rails/LexicallyScopedActionFilter end def index @@ -25,6 +28,10 @@ module Analytics def authorize authorize_read_cycle_analytics! end + + def authorize_modification + # no-op, overridden in EE + end end end end diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index 41477519ba5..12210afd44a 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -7,6 +7,8 @@ class Import::GithubController < Import::BaseController include ActionView::Helpers::SanitizeHelper include Import::GithubOauth + before_action :authorize_owner_access!, except: [:new, :callback, :personal_access_token, :status, :details, :create, + :realtime_changes, :cancel_all, :counts] before_action :verify_import_enabled before_action :provider_auth, only: [:status, :realtime_changes, :create] before_action :expire_etag_cache, only: [:status, :create] @@ -92,8 +94,6 @@ class Import::GithubController < Import::BaseController end def failures - project = Project.imported_from(provider_name).find(params[:project_id]) - unless project.import_finished? return render status: :bad_request, json: { message: _('The import is not complete.') @@ -107,7 +107,6 @@ class Import::GithubController < Import::BaseController end def cancel - project = Project.imported_from(provider_name).find(params[:project_id]) result = Import::Github::CancelProjectImportService.new(project, current_user).execute if result[:status] == :success @@ -168,6 +167,14 @@ class Import::GithubController < Import::BaseController private + def project + @project ||= Project.imported_from(provider_name).find(params[:project_id]) + end + + def authorize_owner_access! + return render_404 unless current_user.can?(:owner_access, project) + end + def import_params params.permit(permitted_import_params) end diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 266edd506d5..599bfd75e14 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -89,10 +89,14 @@ class Projects::CompareController < Projects::ApplicationController # target == start_ref == from def target_project strong_memoize(:target_project) do - next source_project.default_merge_request_target unless compare_params.key?(:from_project_id) - next source_project if compare_params[:from_project_id].to_i == source_project.id - - target_project = target_projects(source_project).find_by_id(compare_params[:from_project_id]) + target_project = + if !compare_params.key?(:from_project_id) + source_project.default_merge_request_target + elsif compare_params[:from_project_id].to_i == source_project.id + source_project + else + target_projects(source_project).find_by_id(compare_params[:from_project_id]) + end # Just ignore the field if it points at a non-existent or hidden project next source_project unless target_project && can?(current_user, :read_code, target_project) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index c8002c437a9..acc7d8a5a10 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -136,7 +136,7 @@ module UsersHelper def confirm_user_data(user) message = if user.unconfirmed_email.present? - _('This user has an unconfirmed email address (%{email}). You may force a confirmation.') % { email: user.unconfirmed_email } + safe_format(_('This user has an unconfirmed email address (%{email}). You may force a confirmation.'), email: user.unconfirmed_email) else _('This user has an unconfirmed email address. You may force a confirmation.') end diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 6dc1c9f290a..d7a95363337 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -135,6 +135,7 @@ class WebHook < ApplicationRecord return if url_variables_were.blank? || interpolated_url_was == interpolated_url + self.url_variables = {} if url_variables_were.keys.intersection(url_variables.keys).any? self.url_variables = {} if url_changed? && url_variables_were.to_a.intersection(url_variables.to_a).any? end diff --git a/lib/api/entities/issue.rb b/lib/api/entities/issue.rb index 56e942a0383..79adad92d6f 100644 --- a/lib/api/entities/issue.rb +++ b/lib/api/entities/issue.rb @@ -55,7 +55,14 @@ module API end expose :moved_to_id - expose :service_desk_reply_to + expose :service_desk_reply_to do |issue| + issue.present( + current_user: options[:current_user], + # We need to pass it explicitly to account for the case where `issue` + # is a `WorkItem` which doesn't have a presenter yet. + presenter_class: IssuePresenter + ).service_desk_reply_to + end end end end diff --git a/lib/banzai/filter/references/reference_cache.rb b/lib/banzai/filter/references/reference_cache.rb index c8370d5f9c1..759c34ab7e6 100644 --- a/lib/banzai/filter/references/reference_cache.rb +++ b/lib/banzai/filter/references/reference_cache.rb @@ -29,15 +29,19 @@ module Banzai @references_per_parent[parent_type] ||= begin refs = Hash.new { |hash, key| hash[key] = Set.new } - prepare_doc_for_scan.to_enum(:scan, regex).each do - parent_path = if parent_type == :project - full_project_path($~[:namespace], $~[:project]) - else - full_group_path($~[:group]) - end - - ident = filter.identifier($~) - refs[parent_path] << ident if ident + [filter.object_class.link_reference_pattern, filter.object_class.reference_pattern].each do |pattern| + next unless pattern + + prepare_doc_for_scan.to_enum(:scan, pattern).each do + parent_path = if parent_type == :project + full_project_path($~[:namespace], $~[:project]) + else + full_group_path($~[:group]) + end + + ident = filter.identifier($~) + refs[parent_path] << ident if ident + end end refs @@ -171,15 +175,6 @@ module Banzai delegate :project, :group, :parent, :parent_type, to: :filter - def regex - strong_memoize(:regex) do - [ - filter.object_class.link_reference_pattern, - filter.object_class.reference_pattern - ].compact.reduce { |a, b| Regexp.union(a, b) } - end - end - def refs_cache Gitlab::SafeRequestStore["banzai_#{parent_type}_refs".to_sym] ||= {} end diff --git a/lib/gitlab/sidekiq_middleware/arguments_logger.rb b/lib/gitlab/sidekiq_middleware/arguments_logger.rb index 2c506786d83..a743663d66a 100644 --- a/lib/gitlab/sidekiq_middleware/arguments_logger.rb +++ b/lib/gitlab/sidekiq_middleware/arguments_logger.rb @@ -6,7 +6,8 @@ module Gitlab include Sidekiq::ServerMiddleware def call(worker, job, queue) - logger.info "arguments: #{Gitlab::Json.dump(job['args'])}" + loggable_args = Gitlab::ErrorTracking::Processor::SidekiqProcessor.loggable_arguments(job['args'], job['class']) + logger.info "arguments: #{Gitlab::Json.dump(loggable_args)}" yield end end diff --git a/spec/controllers/admin/hooks_controller_spec.rb b/spec/controllers/admin/hooks_controller_spec.rb index 4e68ffdda2a..86c3405863a 100644 --- a/spec/controllers/admin/hooks_controller_spec.rb +++ b/spec/controllers/admin/hooks_controller_spec.rb @@ -55,12 +55,13 @@ RSpec.describe Admin::HooksController do hook.update!(url_variables: { 'foo' => 'bar', 'baz' => 'woo' }) hook_params = { - url: 'http://example.com/{baz}?token={token}', + url: 'http://example.com/{bar}?token={token}', enable_ssl_verification: false, url_variables: [ { key: 'token', value: 'some secret value' }, - { key: 'baz', value: 'qux' }, - { key: 'foo', value: nil } + { key: 'baz', value: nil }, + { key: 'foo', value: nil }, + { key: 'bar', value: 'qux' } ] } @@ -72,7 +73,7 @@ RSpec.describe Admin::HooksController do expect(flash[:notice]).to include('was updated') expect(hook).to have_attributes(hook_params.except(:url_variables)) expect(hook).to have_attributes( - url_variables: { 'token' => 'some secret value', 'baz' => 'qux' } + url_variables: { 'token' => 'some secret value', 'bar' => 'qux' } ) end end diff --git a/spec/controllers/import/github_controller_spec.rb b/spec/controllers/import/github_controller_spec.rb index fdc0ddda9f4..bf56043a496 100644 --- a/spec/controllers/import/github_controller_spec.rb +++ b/spec/controllers/import/github_controller_spec.rb @@ -395,6 +395,12 @@ RSpec.describe Import::GithubController, feature_category: :importers do ) end + let(:user) { project.owner } + + before do + sign_in(user) + end + context 'when import is not finished' do it 'return bad_request' do get :failures, params: { project_id: project.id } @@ -434,6 +440,16 @@ RSpec.describe Import::GithubController, feature_category: :importers do expect(json_response.first['title']).to eq(issue_title) end end + + context 'when signed user is not the owner' do + let(:user) { create(:user) } + + it 'renders 404' do + get :failures, params: { project_id: project.id } + + expect(response).to have_gitlab_http_status(:not_found) + end + end end describe "POST cancel" do @@ -444,6 +460,12 @@ RSpec.describe Import::GithubController, feature_category: :importers do ) end + let(:user) { project.owner } + + before do + sign_in(user) + end + context 'when project import was canceled' do before do allow(Import::Github::CancelProjectImportService) @@ -476,6 +498,16 @@ RSpec.describe Import::GithubController, feature_category: :importers do expect(json_response['errors']).to eq('The import cannot be canceled because it is finished') end end + + context 'when signed user is not the owner' do + let(:user) { create(:user) } + + it 'renders 404' do + post :cancel, params: { project_id: project.id } + + expect(response).to have_gitlab_http_status(:not_found) + end + end end describe 'POST cancel_all' do diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb index a49f8b51c12..7dc9bcd9677 100644 --- a/spec/controllers/projects/compare_controller_spec.rb +++ b/spec/controllers/projects/compare_controller_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::CompareController do +RSpec.describe Projects::CompareController, feature_category: :source_code_management do include ProjectForksHelper using RSpec::Parameterized::TableSyntax @@ -211,6 +211,36 @@ RSpec.describe Projects::CompareController do end end + context 'when the target project is the default source but hidden to the user' do + let(:project) { create(:project, :repository, :private) } + let(:from_ref) { 'improve%2Fmore-awesome' } + let(:to_ref) { 'feature' } + let(:whitespace) { nil } + + let(:request_params) do + { + namespace_id: project.namespace, + project_id: project, + from: from_ref, + to: to_ref, + w: whitespace, + page: page, + straight: straight + } + end + + it 'does not show the diff' do + allow(controller).to receive(:source_project).and_return(project) + expect(project).to receive(:default_merge_request_target).and_return(private_fork) + + show_request + + expect(response).to be_successful + expect(assigns(:diffs)).to be_empty + expect(assigns(:commits)).to be_empty + end + end + context 'when the source ref does not exist' do let(:from_project_id) { nil } let(:from_ref) { 'non-existent-source-ref' } diff --git a/spec/features/admin/users/user_spec.rb b/spec/features/admin/users/user_spec.rb index 18d62cb585f..f8f1fdaabb4 100644 --- a/spec/features/admin/users/user_spec.rb +++ b/spec/features/admin/users/user_spec.rb @@ -504,7 +504,9 @@ RSpec.describe 'Admin::Users::User', feature_category: :user_management do end context 'when user has an unconfirmed email', :js do - let(:unconfirmed_user) { create(:user, :unconfirmed) } + # Email address contains HTML to ensure email address is displayed in an HTML safe way. + let_it_be(:unconfirmed_email) { "#{generate(:email)}<h2>testing<img/src=http://localhost:8000/test.png>" } + let_it_be(:unconfirmed_user) { create(:user, :unconfirmed, unconfirmed_email: unconfirmed_email) } where(:path_helper) do [ @@ -524,7 +526,9 @@ RSpec.describe 'Admin::Users::User', feature_category: :user_management do within_modal do expect(page).to have_content("Confirm user #{unconfirmed_user.name}?") - expect(page).to have_content('This user has an unconfirmed email address. You may force a confirmation.') + expect(page).to have_content( + "This user has an unconfirmed email address (#{unconfirmed_email}). You may force a confirmation." + ) click_button 'Confirm user' end diff --git a/spec/lib/api/entities/issue_spec.rb b/spec/lib/api/entities/issue_spec.rb new file mode 100644 index 00000000000..15886542571 --- /dev/null +++ b/spec/lib/api/entities/issue_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::API::Entities::Issue, feature_category: :team_planning do + let_it_be(:project) { create(:project) } + let(:issue) { build_stubbed(:issue, project: project) } + let(:current_user) { build_stubbed(:user) } + let(:options) { { current_user: current_user }.merge(option_addons) } + let(:option_addons) { {} } + let(:entity) { described_class.new(issue, options) } + + subject(:json) { entity.as_json } + + describe '#service_desk_reply_to', feature_category: :service_desk do + # Setting to true (default) doesn't play nice with stubs + let(:option_addons) { { include_subscribed: false } } + let(:issue) { build_stubbed(:issue, project: project, service_desk_reply_to: email) } + let(:email) { 'creator@example.com' } + let(:role) { :developer } + + subject { json[:service_desk_reply_to] } + + context 'as developer' do + before do + stub_member_access_level(issue.project, developer: current_user) + end + + it { is_expected.to eq(email) } + end + + context 'as guest' do + before do + stub_member_access_level(issue.project, guest: current_user) + end + + it { is_expected.to eq('cr*****@e*****.c**') } + end + + context 'without email' do + let(:email) { nil } + + specify { expect(json).to have_key(:service_desk_reply_to) } + it { is_expected.to eq(nil) } + end + end +end diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 82cfb3983f8..308e16328d7 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -258,6 +258,13 @@ RSpec.describe WebHook, feature_category: :webhooks do expect(hook.url_variables).to eq({}) end + it 'resets url variables if url variables are overwritten' do + hook.url_variables = hook.url_variables.merge('abc' => 'baz') + + expect(hook).not_to be_valid + expect(hook.url_variables).to eq({}) + end + it 'does not reset url variables if both url and url variables are changed' do hook.url = 'http://example.com/{one}/{two}' hook.url_variables = { 'one' => 'foo', 'two' => 'bar' } |