Merge remote-tracking branch 'dev/16-1-stable' into 16-1-stable

.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'

.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'

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)

GITALY_SERVER_VERSION

@@ -1 +1 @@
-16.1.0
+16.1.1

GITLAB_PAGES_VERSION

@@ -1 +1 @@
-16.1.0
+16.1.1

VERSION

@@ -1 +1 @@
-16.1.0
+16.1.1

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

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

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)

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

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
 

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

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
+            [filter.object_class.link_reference_pattern, filter.object_class.reference_pattern].each do |pattern|
+              next unless pattern
 
-              ident = filter.identifier($~)
-              refs[parent_path] << ident if ident
+              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

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

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

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

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' }

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

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

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' }