From 1bde803c0dc057fefff36be2104c0ae6b5cb1336 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 2 Mar 2023 00:18:50 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-9-stable-ee --- app/models/ci/build.rb | 10 +- .../projects/artifacts_controller_spec.rb | 4 +- spec/controllers/projects/jobs_controller_spec.rb | 46 ++------ spec/features/projects/jobs/permissions_spec.rb | 125 +++++---------------- spec/models/ci/build_spec.rb | 116 +++++++++---------- spec/requests/api/ci/jobs_spec.rb | 40 +++---- 6 files changed, 120 insertions(+), 221 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index f8b3777841d..1e70dd171ed 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -74,6 +74,7 @@ module Ci delegate :trigger_short_token, to: :trigger_request, allow_nil: true delegate :ensure_persistent_ref, to: :pipeline delegate :enable_debug_trace!, to: :metadata + delegate :debug_trace_enabled?, to: :metadata serialize :options # rubocop:disable Cop/ActiveRecordSerialize serialize :yaml_variables, Gitlab::Serializer::Ci::Variables # rubocop:disable Cop/ActiveRecordSerialize @@ -1069,11 +1070,10 @@ module Ci end def debug_mode? - # TODO: Have `debug_mode?` check against data on sent back from runner - # to capture all the ways that variables can be set. - # See (https://gitlab.com/gitlab-org/gitlab/-/issues/290955) - variables['CI_DEBUG_TRACE']&.value&.casecmp('true') == 0 || - variables['CI_DEBUG_SERVICES']&.value&.casecmp('true') == 0 + # perform the check on both sides in case the runner version is old + debug_trace_enabled? || + Gitlab::Utils.to_boolean(variables['CI_DEBUG_SERVICES']&.value, default: false) || + Gitlab::Utils.to_boolean(variables['CI_DEBUG_TRACE']&.value, default: false) end def drop_with_exit_code!(failure_reason, exit_code) diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb index 32cd10d9805..c707b5dc39d 100644 --- a/spec/controllers/projects/artifacts_controller_spec.rb +++ b/spec/controllers/projects/artifacts_controller_spec.rb @@ -244,7 +244,9 @@ RSpec.describe Projects::ArtifactsController, feature_category: :build_artifacts let(:job) { create(:ci_build, :success, :trace_artifact, pipeline: pipeline) } before do - create(:ci_job_variable, key: 'CI_DEBUG_TRACE', value: 'true', job: job) + allow_next_found_instance_of(Ci::Build) do |build| + allow(build).to receive(:debug_mode?).and_return(true) + end end context 'when the user does not have update_build permissions' do diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index 8fb9623c21a..2d047957430 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -629,40 +629,12 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state, featu expect(json_response['lines'].count).to be_positive end - context 'when CI_DEBUG_TRACE enabled' do - let!(:variable) { create(:ci_instance_variable, key: 'CI_DEBUG_TRACE', value: 'true') } - - context 'with proper permissions on a project' do - let(:user) { developer } - - before do - sign_in(user) - end - - it 'returns response ok' do - get_trace - - expect(response).to have_gitlab_http_status(:ok) - end - end - - context 'without proper permissions for debug logging' do - let(:user) { guest } - - before do - sign_in(user) - end - - it 'returns response forbidden' do - get_trace - - expect(response).to have_gitlab_http_status(:forbidden) + context 'when debug_mode? is enabled' do + before do + allow_next_found_instance_of(Ci::Build) do |build| + allow(build).to receive(:debug_mode?).and_return(true) end end - end - - context 'when CI_DEBUG_SERVICES enabled' do - let!(:variable) { create(:ci_instance_variable, key: 'CI_DEBUG_SERVICES', value: 'true') } context 'with proper permissions on a project' do let(:user) { developer } @@ -1243,10 +1215,10 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state, featu context 'when CI_DEBUG_TRACE and/or CI_DEBUG_SERVICES are enabled' do using RSpec::Parameterized::TableSyntax where(:ci_debug_trace, :ci_debug_services) do - 'true' | 'true' - 'true' | 'false' - 'false' | 'true' - 'false' | 'false' + true | true + true | false + false | true + false | false end with_them do @@ -1279,7 +1251,7 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state, featu it 'returns response forbidden if dev mode enabled' do response = subject - if ci_debug_trace == 'true' || ci_debug_services == 'true' + if ci_debug_trace || ci_debug_services expect(response).to have_gitlab_http_status(:forbidden) else expect(response).to have_gitlab_http_status(:ok) diff --git a/spec/features/projects/jobs/permissions_spec.rb b/spec/features/projects/jobs/permissions_spec.rb index c3c0043a6ef..dce86c9f0a4 100644 --- a/spec/features/projects/jobs/permissions_spec.rb +++ b/spec/features/projects/jobs/permissions_spec.rb @@ -149,109 +149,44 @@ RSpec.describe 'Project Jobs Permissions', feature_category: :projects do end end - context 'with CI_DEBUG_TRACE' do - let_it_be(:ci_instance_variable) { create(:ci_instance_variable, key: 'CI_DEBUG_TRACE') } - - describe 'trace endpoint' do - let_it_be(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) } - - where(:public_builds, :user_project_role, :ci_debug_trace, :expected_status_code) do - true | 'developer' | true | 200 - true | 'guest' | true | 403 - true | 'developer' | false | 200 - true | 'guest' | false | 200 - false | 'developer' | true | 200 - false | 'guest' | true | 403 - false | 'developer' | false | 200 - false | 'guest' | false | 403 - end - - with_them do - before do - ci_instance_variable.update!(value: ci_debug_trace) - project.update!(public_builds: public_builds) - project.add_role(user, user_project_role) - end - - it 'renders trace to authorized users' do - visit trace_project_job_path(project, job) - - expect(status_code).to eq(expected_status_code) - end - end + describe 'debug_mode' do + let(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) } + + where(:public_builds, :user_project_role, :debug_mode, :expected_status_code, :expected_msg) do + true | 'developer' | true | 200 | '' + true | 'guest' | true | 403 | 'You must have developer or higher permissions' + true | nil | true | 404 | 'Page Not Found Make sure the address is correct' + true | 'developer' | false | 200 | '' + true | 'guest' | false | 200 | '' + true | nil | false | 404 | 'Page Not Found Make sure the address is correct' + false | 'developer' | true | 200 | '' + false | 'guest' | true | 403 | 'You must have developer or higher permissions' + false | nil | true | 404 | 'Page Not Found Make sure the address is correct' + false | 'developer' | false | 200 | '' + false | 'guest' | false | 403 | 'The current user is not authorized to access the job log' + false | nil | false | 404 | 'Page Not Found Make sure the address is correct' end - describe 'raw page' do - let_it_be(:job) { create(:ci_build, :running, :coverage, :trace_artifact, pipeline: pipeline) } - - where(:public_builds, :user_project_role, :ci_debug_trace, :expected_status_code, :expected_msg) do - true | 'developer' | true | 200 | nil - true | 'guest' | true | 403 | 'You must have developer or higher permissions' - true | 'developer' | false | 200 | nil - true | 'guest' | false | 200 | nil - false | 'developer' | true | 200 | nil - false | 'guest' | true | 403 | 'You must have developer or higher permissions' - false | 'developer' | false | 200 | nil - false | 'guest' | false | 403 | 'The current user is not authorized to access the job log' - end - - with_them do - before do - ci_instance_variable.update!(value: ci_debug_trace) - project.update!(public_builds: public_builds) - project.add_role(user, user_project_role) - end - - it 'renders raw trace to authorized users' do - visit raw_project_job_path(project, job) - - expect(status_code).to eq(expected_status_code) - expect(page).to have_content(expected_msg) + with_them do + before do + project.update!(public_builds: public_builds) + user_project_role && project.add_role(user, user_project_role) + allow_next_found_instance_of(Ci::Build) do |build| + allow(build).to receive(:debug_mode?).and_return(debug_mode) end end - end - end - - context 'with CI_DEBUG_SERVICES' do - let_it_be(:ci_instance_variable) { create(:ci_instance_variable, key: 'CI_DEBUG_SERVICES') } - - describe 'trace endpoint and raw page' do - let_it_be(:job) { create(:ci_build, :running, :coverage, :trace_artifact, pipeline: pipeline) } - - where(:public_builds, :user_project_role, :ci_debug_services, :expected_status_code, :expected_msg) do - true | 'developer' | true | 200 | nil - true | 'guest' | true | 403 | 'You must have developer or higher permissions' - true | nil | true | 404 | 'Page Not Found Make sure the address is correct' - true | 'developer' | false | 200 | nil - true | 'guest' | false | 200 | nil - true | nil | false | 404 | 'Page Not Found Make sure the address is correct' - false | 'developer' | true | 200 | nil - false | 'guest' | true | 403 | 'You must have developer or higher permissions' - false | nil | true | 404 | 'Page Not Found Make sure the address is correct' - false | 'developer' | false | 200 | nil - false | 'guest' | false | 403 | 'The current user is not authorized to access the job log' - false | nil | false | 404 | 'Page Not Found Make sure the address is correct' - end - with_them do - before do - ci_instance_variable.update!(value: ci_debug_services) - project.update!(public_builds: public_builds) - user_project_role && project.add_role(user, user_project_role) - end - - it 'renders trace to authorized users' do - visit trace_project_job_path(project, job) + it 'renders trace to authorized users' do + visit trace_project_job_path(project, job) - expect(status_code).to eq(expected_status_code) - end + expect(status_code).to eq(expected_status_code) + end - it 'renders raw trace to authorized users' do - visit raw_project_job_path(project, job) + it 'renders raw trace to authorized users' do + visit raw_project_job_path(project, job) - expect(status_code).to eq(expected_status_code) - expect(page).to have_content(expected_msg) - end + expect(status_code).to eq(expected_status_code) + expect(page).to have_content(expected_msg) end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 2b3dc97e06d..80d6693e08e 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -5162,52 +5162,42 @@ RSpec.describe Ci::Build, feature_category: :continuous_integration, factory_def subject { build.debug_mode? } context 'when CI_DEBUG_TRACE=true is in variables' do - context 'when in instance variables' do - before do - create(:ci_instance_variable, key: 'CI_DEBUG_TRACE', value: 'true') + ['true', 1, 'y'].each do |value| + it 'reflects instance variables' do + create(:ci_instance_variable, key: 'CI_DEBUG_TRACE', value: value) + + is_expected.to eq true end - it { is_expected.to eq true } - end + it 'reflects group variables' do + create(:ci_group_variable, key: 'CI_DEBUG_TRACE', value: value, group: project.group) - context 'when in group variables' do - before do - create(:ci_group_variable, key: 'CI_DEBUG_TRACE', value: 'true', group: project.group) + is_expected.to eq true end - it { is_expected.to eq true } - end + it 'reflects pipeline variables' do + create(:ci_pipeline_variable, key: 'CI_DEBUG_TRACE', value: value, pipeline: pipeline) - context 'when in pipeline variables' do - before do - create(:ci_pipeline_variable, key: 'CI_DEBUG_TRACE', value: 'true', pipeline: pipeline) + is_expected.to eq true end - it { is_expected.to eq true } - end + it 'reflects project variables' do + create(:ci_variable, key: 'CI_DEBUG_TRACE', value: value, project: project) - context 'when in project variables' do - before do - create(:ci_variable, key: 'CI_DEBUG_TRACE', value: 'true', project: project) + is_expected.to eq true end - it { is_expected.to eq true } - end + it 'reflects job variables' do + create(:ci_job_variable, key: 'CI_DEBUG_TRACE', value: value, job: build) - context 'when in job variables' do - before do - create(:ci_job_variable, key: 'CI_DEBUG_TRACE', value: 'true', job: build) + is_expected.to eq true end - it { is_expected.to eq true } - end + it 'when in yaml variables' do + build.update!(yaml_variables: [{ key: :CI_DEBUG_TRACE, value: value.to_s }]) - context 'when in yaml variables' do - before do - build.update!(yaml_variables: [{ key: :CI_DEBUG_TRACE, value: 'true' }]) + is_expected.to eq true end - - it { is_expected.to eq true } end end @@ -5216,58 +5206,64 @@ RSpec.describe Ci::Build, feature_category: :continuous_integration, factory_def end context 'when CI_DEBUG_SERVICES=true is in variables' do - context 'when in instance variables' do - before do - create(:ci_instance_variable, key: 'CI_DEBUG_SERVICES', value: 'true') + ['true', 1, 'y'].each do |value| + it 'reflects instance variables' do + create(:ci_instance_variable, key: 'CI_DEBUG_SERVICES', value: value) + + is_expected.to eq true end - it { is_expected.to eq true } - end + it 'reflects group variables' do + create(:ci_group_variable, key: 'CI_DEBUG_SERVICES', value: value, group: project.group) - context 'when in group variables' do - before do - create(:ci_group_variable, key: 'CI_DEBUG_SERVICES', value: 'true', group: project.group) + is_expected.to eq true end - it { is_expected.to eq true } - end + it 'reflects pipeline variables' do + create(:ci_pipeline_variable, key: 'CI_DEBUG_SERVICES', value: value, pipeline: pipeline) - context 'when in pipeline variables' do - before do - create(:ci_pipeline_variable, key: 'CI_DEBUG_SERVICES', value: 'true', pipeline: pipeline) + is_expected.to eq true end - it { is_expected.to eq true } - end + it 'reflects project variables' do + create(:ci_variable, key: 'CI_DEBUG_SERVICES', value: value, project: project) - context 'when in project variables' do - before do - create(:ci_variable, key: 'CI_DEBUG_SERVICES', value: 'true', project: project) + is_expected.to eq true end - it { is_expected.to eq true } - end + it 'reflects job variables' do + create(:ci_job_variable, key: 'CI_DEBUG_SERVICES', value: value, job: build) - context 'when in job variables' do - before do - create(:ci_job_variable, key: 'CI_DEBUG_SERVICES', value: 'true', job: build) + is_expected.to eq true end - it { is_expected.to eq true } - end + it 'when in yaml variables' do + build.update!(yaml_variables: [{ key: :CI_DEBUG_SERVICES, value: value.to_s }]) - context 'when in yaml variables' do - before do - build.update!(yaml_variables: [{ key: :CI_DEBUG_SERVICES, value: 'true' }]) + is_expected.to eq true end - - it { is_expected.to eq true } end end context 'when CI_DEBUG_SERVICES is not in variables' do it { is_expected.to eq false } end + + context 'when metadata has debug_trace_enabled true' do + before do + build.metadata.update!(debug_trace_enabled: true) + end + + it { is_expected.to eq true } + end + + context 'when metadata has debug_trace_enabled false' do + before do + build.metadata.update!(debug_trace_enabled: false) + end + + it { is_expected.to eq false } + end end describe '#drop_with_exit_code!' do diff --git a/spec/requests/api/ci/jobs_spec.rb b/spec/requests/api/ci/jobs_spec.rb index 10dd9c3b556..8b3ec59b785 100644 --- a/spec/requests/api/ci/jobs_spec.rb +++ b/spec/requests/api/ci/jobs_spec.rb @@ -750,11 +750,7 @@ RSpec.describe API::Ci::Jobs, feature_category: :continuous_integration do end end - context 'when ci_debug_trace is set to true' do - before_all do - create(:ci_instance_variable, key: 'CI_DEBUG_TRACE', value: true) - end - + shared_examples_for "additional access criteria" do where(:public_builds, :user_project_role, :expected_status) do true | 'developer' | :ok true | 'guest' | :forbidden @@ -776,30 +772,28 @@ RSpec.describe API::Ci::Jobs, feature_category: :continuous_integration do end end - context 'when ci_debug_services is set to true' do - before_all do - create(:ci_instance_variable, key: 'CI_DEBUG_SERVICES', value: true) + describe 'when metadata debug_trace_enabled is set to true' do + before do + job.metadata.update!(debug_trace_enabled: true) end - where(:public_builds, :user_project_role, :expected_status) do - true | 'developer' | :ok - true | 'guest' | :forbidden - false | 'developer' | :ok - false | 'guest' | :forbidden - end + it_behaves_like "additional access criteria" + end - with_them do - before do - project.update!(public_builds: public_builds) - project.add_role(user, user_project_role) + context 'when ci_debug_trace is set to true' do + before_all do + create(:ci_instance_variable, key: 'CI_DEBUG_TRACE', value: true) + end - get api("/projects/#{project.id}/jobs/#{job.id}/trace", api_user) - end + it_behaves_like "additional access criteria" + end - it 'renders successfully to authorized users' do - expect(response).to have_gitlab_http_status(expected_status) - end + context 'when ci_debug_services is set to true' do + before_all do + create(:ci_instance_variable, key: 'CI_DEBUG_SERVICES', value: true) end + + it_behaves_like "additional access criteria" end end -- cgit v1.2.1