From 72954e75623e4c924fb6589b99589e4f29b0e15d Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 28 Jun 2023 12:12:13 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@16-0-stable-ee --- .../cycle_analytics/value_stream_actions.rb | 7 ++++ app/controllers/import/github_controller.rb | 13 ++++-- app/controllers/projects/compare_controller.rb | 12 ++++-- app/helpers/users_helper.rb | 2 +- app/policies/project_policy.rb | 2 +- doc/ci/jobs/ci_job_token.md | 13 ++---- doc/update/index.md | 10 +++++ lib/api/entities/issue.rb | 9 ++++- lib/banzai/filter/references/reference_cache.rb | 31 ++++++-------- lib/gitlab/sidekiq_middleware/arguments_logger.rb | 3 +- spec/controllers/import/github_controller_spec.rb | 32 +++++++++++++++ .../projects/compare_controller_spec.rb | 32 ++++++++++++++- spec/features/admin/users/user_spec.rb | 8 +++- spec/lib/api/entities/issue_spec.rb | 47 ++++++++++++++++++++++ spec/policies/project_policy_spec.rb | 46 +++++++-------------- spec/requests/api/npm_project_packages_spec.rb | 4 +- spec/requests/lfs_http_spec.rb | 6 +-- 17 files changed, 198 insertions(+), 79 deletions(-) create mode 100644 spec/lib/api/entities/issue_spec.rb 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 60230d58e30..6571ec4caf8 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/policies/project_policy.rb b/app/policies/project_policy.rb index 47d8d0eef3e..e6bafbafd37 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -661,7 +661,7 @@ class ProjectPolicy < BasePolicy enable :read_project_for_iids end - rule { ~public_project & ~internal_access & ~project_allowed_for_job_token }.prevent_all + rule { ~project_allowed_for_job_token }.prevent_all rule { can?(:public_access) }.policy do enable :read_package diff --git a/doc/ci/jobs/ci_job_token.md b/doc/ci/jobs/ci_job_token.md index 9cbf45a16e7..b1958a27636 100644 --- a/doc/ci/jobs/ci_job_token.md +++ b/doc/ci/jobs/ci_job_token.md @@ -65,12 +65,10 @@ tries to steal tokens from other jobs. You can control what projects a CI/CD job token can access to increase the job token's security. A job token might give extra permissions that aren't necessary -to access specific private resources. The job token scope only controls access -to private projects. If an accessed project is public or internal, token scoping does -not apply. +to access specific resources. If a job token is leaked, it could potentially be used to access private data -to the job token's user. By limiting the job token access scope, private data cannot +to the job token's user. By limiting the job token access scope, project data cannot be accessed unless projects are explicitly authorized. There is a proposal to add more strategic control of the access permissions, @@ -90,8 +88,7 @@ their `CI_JOB_TOKEN`. For example, project `A` can add project `B` to the allowlist. CI/CD jobs in project `B` (the "allowed project") can now use their CI/CD job token to -authenticate API calls to access project `A`. If project `A` is public or internal, -the project can be accessed by project `B` without adding it to the allowlist. +authenticate API calls to access project `A`. By default, the allowlist of any project only includes itself. @@ -166,9 +163,7 @@ limited only by the user's access permissions. For example, when the setting is enabled, jobs in a pipeline in project `A` have a `CI_JOB_TOKEN` scope limited to project `A`. If the job needs to use the token -to make an API request to a private project `B`, then `B` must be added to the allowlist for `A`. -If project `B` is public or internal, you do not need to add -`B` to the allowlist to grant access. +to make an API request to project `B`, then `B` must be added to the allowlist for `A`. ### Configure the job token scope diff --git a/doc/update/index.md b/doc/update/index.md index 00c55f1e4b4..4ca85db546d 100644 --- a/doc/update/index.md +++ b/doc/update/index.md @@ -265,12 +265,22 @@ NOTE: Specific information that follow related to Ruby and Git versions do not apply to [Omnibus installations](https://docs.gitlab.com/omnibus/) and [Helm Chart deployments](https://docs.gitlab.com/charts/). They come with appropriate Ruby and Git versions and are not using system binaries for Ruby and Git. There is no need to install Ruby or Git when utilizing these two approaches. +### 16.0.6 + +- Accessing a public or internal project with a [CI/CD job token](../ci/jobs/ci_job_token.md) + now needs explicit authorization in the target project's allowlist. + ### 16.0.0 - Sidekiq jobs are only routed to `default` and `mailers` queues by default, and as a result, every Sidekiq process also listens to those queues to ensure all jobs are processed across all queues. This behavior does not apply if you have configured the [routing rules](../administration/sidekiq/processing_specific_job_classes.md#routing-rules). +### 15.11.10 + +- Accessing a public or internal project with a [CI/CD job token](../ci/jobs/ci_job_token.md) + now needs explicit authorization in the target project's allowlist. + ### 15.11.1 - Many [project importers](../user/project/import/index.md) and [group importers](../user/group/import/index.md) now 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/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)}

testing" } + 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/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index ae2a11bdbf0..72ee8e0d59e 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -2552,42 +2552,24 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do describe 'when user is authenticated via CI_JOB_TOKEN', :request_store do using RSpec::Parameterized::TableSyntax - where(:project_visibility, :user_role, :external_user, :scope_project_type, :token_scope_enabled, :result) do - :private | :reporter | false | :same | true | true - :private | :reporter | false | :same | false | true - :private | :reporter | false | :different | true | false - :private | :reporter | false | :different | false | true - :private | :guest | false | :same | true | true - :private | :guest | false | :same | false | true - :private | :guest | false | :different | true | false - :private | :guest | false | :different | false | true - - :internal | :reporter | false | :same | true | true - :internal | :reporter | true | :same | true | true - :internal | :reporter | false | :same | false | true - :internal | :reporter | false | :different | true | true - :internal | :reporter | true | :different | true | false - :internal | :reporter | false | :different | false | true - :internal | :guest | false | :same | true | true - :internal | :guest | true | :same | true | true - :internal | :guest | false | :same | false | true - :internal | :guest | false | :different | true | true - :internal | :guest | true | :different | true | false - :internal | :guest | false | :different | false | true - - :public | :reporter | false | :same | true | true - :public | :reporter | false | :same | false | true - :public | :reporter | false | :different | true | true - :public | :reporter | false | :different | false | true - :public | :guest | false | :same | true | true - :public | :guest | false | :same | false | true - :public | :guest | false | :different | true | true - :public | :guest | false | :different | false | true + where(:user_role, :external_user, :scope_project_type, :token_scope_enabled, :result) do + :reporter | false | :same | true | true + :reporter | true | :same | true | true + :reporter | false | :same | false | true + :reporter | false | :different | true | false + :reporter | true | :different | true | false + :reporter | false | :different | false | true + :guest | false | :same | true | true + :guest | true | :same | true | true + :guest | false | :same | false | true + :guest | false | :different | true | false + :guest | true | :different | true | false + :guest | false | :different | false | true end with_them do let(:current_user) { public_send(user_role) } - let(:project) { public_send("#{project_visibility}_project") } + let(:project) { public_project } let(:job) { build_stubbed(:ci_build, project: scope_project, user: current_user) } let(:scope_project) do diff --git a/spec/requests/api/npm_project_packages_spec.rb b/spec/requests/api/npm_project_packages_spec.rb index d673645c51a..61b8ab9a8f8 100644 --- a/spec/requests/api/npm_project_packages_spec.rb +++ b/spec/requests/api/npm_project_packages_spec.rb @@ -111,7 +111,7 @@ RSpec.describe API::NpmProjectPackages, feature_category: :package_registry do context 'with a job token for a different user' do let_it_be(:other_user) { create(:user) } - let_it_be_with_reload(:other_job) { create(:ci_build, :running, user: other_user) } + let_it_be_with_reload(:other_job) { create(:ci_build, :running, user: other_user, project: project) } let(:headers) { build_token_auth_header(other_job.token) } @@ -160,7 +160,7 @@ RSpec.describe API::NpmProjectPackages, feature_category: :package_registry do context 'with a job token for a different user' do let_it_be(:other_user) { create(:user) } - let_it_be_with_reload(:other_job) { create(:ci_build, :running, user: other_user) } + let_it_be_with_reload(:other_job) { create(:ci_build, :running, user: other_user, project: project) } let(:headers) { build_token_auth_header(other_job.token) } diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index b07296a0df2..81d6b5465e3 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -677,8 +677,7 @@ RSpec.describe 'Git LFS API and storage', feature_category: :source_code_managem context 'tries to push to other project' do let(:pipeline) { create(:ci_empty_pipeline, project: other_project) } - # I'm not sure what this tests that is different from the previous test - it_behaves_like 'LFS http 403 response' + it_behaves_like 'LFS http 404 response' end end @@ -1198,8 +1197,7 @@ RSpec.describe 'Git LFS API and storage', feature_category: :source_code_managem context 'tries to push to other project' do let(:pipeline) { create(:ci_empty_pipeline, project: other_project) } - # I'm not sure what this tests that is different from the previous test - it_behaves_like 'LFS http 403 response' + it_behaves_like 'LFS http 404 response' end end -- cgit v1.2.3 From 8f11a04bf6ab36fe0fbd5ba753058eb7776b5dd6 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 28 Jun 2023 12:12:21 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@16-0-stable-ee --- .rubocop_todo/gitlab/strong_memoize_attr.yml | 1 - .rubocop_todo/rspec/missing_feature_category.yml | 1 - app/models/hooks/web_hook.rb | 1 + spec/controllers/admin/hooks_controller_spec.rb | 9 +++++---- spec/models/hooks/web_hook_spec.rb | 7 +++++++ 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/.rubocop_todo/gitlab/strong_memoize_attr.yml b/.rubocop_todo/gitlab/strong_memoize_attr.yml index 4274e59a2a9..1ab7930a2d2 100644 --- a/.rubocop_todo/gitlab/strong_memoize_attr.yml +++ b/.rubocop_todo/gitlab/strong_memoize_attr.yml @@ -362,7 +362,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 0382b0ff43e..58d02eb3641 100644 --- a/.rubocop_todo/rspec/missing_feature_category.yml +++ b/.rubocop_todo/rspec/missing_feature_category.yml @@ -1013,7 +1013,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/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 5ccbc926a71..1ee47bc63cc 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/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/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 254b8c2520b..4c7317de903 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: :integrations 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' } -- cgit v1.2.3 From 12a0ac2a81afd9c40e7f97a0471dafab80e09c1e Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 28 Jun 2023 18:36:41 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@16-0-stable-ee --- app/policies/project_policy.rb | 2 +- doc/ci/jobs/ci_job_token.md | 13 +++++--- doc/update/index.md | 10 ------ spec/policies/project_policy_spec.rb | 46 ++++++++++++++++++-------- spec/requests/api/npm_project_packages_spec.rb | 4 +-- spec/requests/lfs_http_spec.rb | 6 ++-- 6 files changed, 48 insertions(+), 33 deletions(-) diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index e6bafbafd37..47d8d0eef3e 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -661,7 +661,7 @@ class ProjectPolicy < BasePolicy enable :read_project_for_iids end - rule { ~project_allowed_for_job_token }.prevent_all + rule { ~public_project & ~internal_access & ~project_allowed_for_job_token }.prevent_all rule { can?(:public_access) }.policy do enable :read_package diff --git a/doc/ci/jobs/ci_job_token.md b/doc/ci/jobs/ci_job_token.md index b1958a27636..9cbf45a16e7 100644 --- a/doc/ci/jobs/ci_job_token.md +++ b/doc/ci/jobs/ci_job_token.md @@ -65,10 +65,12 @@ tries to steal tokens from other jobs. You can control what projects a CI/CD job token can access to increase the job token's security. A job token might give extra permissions that aren't necessary -to access specific resources. +to access specific private resources. The job token scope only controls access +to private projects. If an accessed project is public or internal, token scoping does +not apply. If a job token is leaked, it could potentially be used to access private data -to the job token's user. By limiting the job token access scope, project data cannot +to the job token's user. By limiting the job token access scope, private data cannot be accessed unless projects are explicitly authorized. There is a proposal to add more strategic control of the access permissions, @@ -88,7 +90,8 @@ their `CI_JOB_TOKEN`. For example, project `A` can add project `B` to the allowlist. CI/CD jobs in project `B` (the "allowed project") can now use their CI/CD job token to -authenticate API calls to access project `A`. +authenticate API calls to access project `A`. If project `A` is public or internal, +the project can be accessed by project `B` without adding it to the allowlist. By default, the allowlist of any project only includes itself. @@ -163,7 +166,9 @@ limited only by the user's access permissions. For example, when the setting is enabled, jobs in a pipeline in project `A` have a `CI_JOB_TOKEN` scope limited to project `A`. If the job needs to use the token -to make an API request to project `B`, then `B` must be added to the allowlist for `A`. +to make an API request to a private project `B`, then `B` must be added to the allowlist for `A`. +If project `B` is public or internal, you do not need to add +`B` to the allowlist to grant access. ### Configure the job token scope diff --git a/doc/update/index.md b/doc/update/index.md index 4ca85db546d..00c55f1e4b4 100644 --- a/doc/update/index.md +++ b/doc/update/index.md @@ -265,22 +265,12 @@ NOTE: Specific information that follow related to Ruby and Git versions do not apply to [Omnibus installations](https://docs.gitlab.com/omnibus/) and [Helm Chart deployments](https://docs.gitlab.com/charts/). They come with appropriate Ruby and Git versions and are not using system binaries for Ruby and Git. There is no need to install Ruby or Git when utilizing these two approaches. -### 16.0.6 - -- Accessing a public or internal project with a [CI/CD job token](../ci/jobs/ci_job_token.md) - now needs explicit authorization in the target project's allowlist. - ### 16.0.0 - Sidekiq jobs are only routed to `default` and `mailers` queues by default, and as a result, every Sidekiq process also listens to those queues to ensure all jobs are processed across all queues. This behavior does not apply if you have configured the [routing rules](../administration/sidekiq/processing_specific_job_classes.md#routing-rules). -### 15.11.10 - -- Accessing a public or internal project with a [CI/CD job token](../ci/jobs/ci_job_token.md) - now needs explicit authorization in the target project's allowlist. - ### 15.11.1 - Many [project importers](../user/project/import/index.md) and [group importers](../user/group/import/index.md) now diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 72ee8e0d59e..ae2a11bdbf0 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -2552,24 +2552,42 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do describe 'when user is authenticated via CI_JOB_TOKEN', :request_store do using RSpec::Parameterized::TableSyntax - where(:user_role, :external_user, :scope_project_type, :token_scope_enabled, :result) do - :reporter | false | :same | true | true - :reporter | true | :same | true | true - :reporter | false | :same | false | true - :reporter | false | :different | true | false - :reporter | true | :different | true | false - :reporter | false | :different | false | true - :guest | false | :same | true | true - :guest | true | :same | true | true - :guest | false | :same | false | true - :guest | false | :different | true | false - :guest | true | :different | true | false - :guest | false | :different | false | true + where(:project_visibility, :user_role, :external_user, :scope_project_type, :token_scope_enabled, :result) do + :private | :reporter | false | :same | true | true + :private | :reporter | false | :same | false | true + :private | :reporter | false | :different | true | false + :private | :reporter | false | :different | false | true + :private | :guest | false | :same | true | true + :private | :guest | false | :same | false | true + :private | :guest | false | :different | true | false + :private | :guest | false | :different | false | true + + :internal | :reporter | false | :same | true | true + :internal | :reporter | true | :same | true | true + :internal | :reporter | false | :same | false | true + :internal | :reporter | false | :different | true | true + :internal | :reporter | true | :different | true | false + :internal | :reporter | false | :different | false | true + :internal | :guest | false | :same | true | true + :internal | :guest | true | :same | true | true + :internal | :guest | false | :same | false | true + :internal | :guest | false | :different | true | true + :internal | :guest | true | :different | true | false + :internal | :guest | false | :different | false | true + + :public | :reporter | false | :same | true | true + :public | :reporter | false | :same | false | true + :public | :reporter | false | :different | true | true + :public | :reporter | false | :different | false | true + :public | :guest | false | :same | true | true + :public | :guest | false | :same | false | true + :public | :guest | false | :different | true | true + :public | :guest | false | :different | false | true end with_them do let(:current_user) { public_send(user_role) } - let(:project) { public_project } + let(:project) { public_send("#{project_visibility}_project") } let(:job) { build_stubbed(:ci_build, project: scope_project, user: current_user) } let(:scope_project) do diff --git a/spec/requests/api/npm_project_packages_spec.rb b/spec/requests/api/npm_project_packages_spec.rb index 61b8ab9a8f8..d673645c51a 100644 --- a/spec/requests/api/npm_project_packages_spec.rb +++ b/spec/requests/api/npm_project_packages_spec.rb @@ -111,7 +111,7 @@ RSpec.describe API::NpmProjectPackages, feature_category: :package_registry do context 'with a job token for a different user' do let_it_be(:other_user) { create(:user) } - let_it_be_with_reload(:other_job) { create(:ci_build, :running, user: other_user, project: project) } + let_it_be_with_reload(:other_job) { create(:ci_build, :running, user: other_user) } let(:headers) { build_token_auth_header(other_job.token) } @@ -160,7 +160,7 @@ RSpec.describe API::NpmProjectPackages, feature_category: :package_registry do context 'with a job token for a different user' do let_it_be(:other_user) { create(:user) } - let_it_be_with_reload(:other_job) { create(:ci_build, :running, user: other_user, project: project) } + let_it_be_with_reload(:other_job) { create(:ci_build, :running, user: other_user) } let(:headers) { build_token_auth_header(other_job.token) } diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 81d6b5465e3..b07296a0df2 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -677,7 +677,8 @@ RSpec.describe 'Git LFS API and storage', feature_category: :source_code_managem context 'tries to push to other project' do let(:pipeline) { create(:ci_empty_pipeline, project: other_project) } - it_behaves_like 'LFS http 404 response' + # I'm not sure what this tests that is different from the previous test + it_behaves_like 'LFS http 403 response' end end @@ -1197,7 +1198,8 @@ RSpec.describe 'Git LFS API and storage', feature_category: :source_code_managem context 'tries to push to other project' do let(:pipeline) { create(:ci_empty_pipeline, project: other_project) } - it_behaves_like 'LFS http 404 response' + # I'm not sure what this tests that is different from the previous test + it_behaves_like 'LFS http 403 response' end end -- cgit v1.2.3 From b2b92cf1bbf1db209ea7a1b890a0c6fba535d7f9 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Wed, 28 Jun 2023 21:05:20 +0000 Subject: Update VERSION files [ci skip] --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 89cf45573e9..c299cb4a627 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -16.0.5 \ No newline at end of file +16.0.6 \ No newline at end of file -- cgit v1.2.3 From d118b4fe1cb0f0c771ec3f69384fb3ae9977c69f Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 28 Jun 2023 21:09:15 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@16-0-stable-ee --- CHANGELOG.md | 17 +++++++++++++++++ GITALY_SERVER_VERSION | 2 +- GITLAB_PAGES_VERSION | 2 +- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9edd323d8a0..2e16e655d06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,23 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 16.0.6 (2023-06-28) + +### Security (12 changes) + +- [Revert 'security-leaked-ci-job-token-permission-16-0' from '16-0'"](gitlab-org/security/gitlab@3c4fdbad26a123c581253fb501b5bace953a5e85) ([merge request](gitlab-org/security/gitlab!3373)) +- [Use fully qualified ref when loading code owner file](gitlab-org/security/gitlab@69c61fcbdc88873b60a217cfd3810364718417e9) ([merge request](gitlab-org/security/gitlab!3355)) +- [Maintainer can leak masked webhook secrets by manipulating URL masking](gitlab-org/security/gitlab@a3e055010523db5a1c346464e2589cc75f73629d) ([merge request](gitlab-org/security/gitlab!3360)) +- [Remove approvals when the only commit gets amended](gitlab-org/security/gitlab@01e59413e2570744dc34dd50efd2601dc91c8d2d) ([merge request](gitlab-org/security/gitlab!3367)) +- [Add authorization validation to GithubController#failures action](gitlab-org/security/gitlab@9eab0689991debab8c8a1afb9e32a3bac9978325) ([merge request](gitlab-org/security/gitlab!3334)) +- [Fix for fork permissions check in compare controller](gitlab-org/security/gitlab@da9bb4c761dfe7e8efdd910ed3fc89f348e47e90) ([merge request](gitlab-org/security/gitlab!3343)) +- [Webhook token leaked in Sidekiq logs if log format is 'default'](gitlab-org/security/gitlab@a9835cb72eddfae1748c66314618b3157a6bcb57) ([merge request](gitlab-org/security/gitlab!3346)) +- [Mitigate epic reference filter ReDOS](gitlab-org/security/gitlab@c8046028a30fe9dca7e141eec2acf3d4b49d93ee) ([merge request](gitlab-org/security/gitlab!3340)) +- [Increasing security for CI_JOB_TOKEN on public and internal projects](gitlab-org/security/gitlab@b67db0cdd9324633f4abb59bc27bca43e94e3362) ([merge request](gitlab-org/security/gitlab!3318)) +- [Adjust access to value stream create, edit and destroy actions](gitlab-org/security/gitlab@ee20f3f3a84a75c7e07e1aa6fde95761636a669f) ([merge request](gitlab-org/security/gitlab!3321)) +- [Sanitize user email addresses in admin confirm user dialog](gitlab-org/security/gitlab@545e0913336e823eb905a8bd86fe2905b321a284) ([merge request](gitlab-org/security/gitlab!3331)) +- [Obfuscate email of service desk issue creator in issue REST API](gitlab-org/security/gitlab@b921f10b565bafbd6d50d93d84d34b5f103839ea) ([merge request](gitlab-org/security/gitlab!3315)) + ## 16.0.5 (2023-06-16) ### Fixed (1 change) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 89cf45573e9..c299cb4a627 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -16.0.5 \ No newline at end of file +16.0.6 \ No newline at end of file diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index 89cf45573e9..c299cb4a627 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -16.0.5 \ No newline at end of file +16.0.6 \ No newline at end of file -- cgit v1.2.3