From 7e6fb6aa5e9b3086b498db0dfebb94334c1efb1a Mon Sep 17 00:00:00 2001 From: Axel von Bertoldi Date: Thu, 22 Sep 2022 12:00:26 -0600 Subject: [PATCH 01/10] Restrict access to job logs when CI_DEBUG_SERVICES is enabled Apply then same treatment to job log access when CI_DEBUG_SERVICES is enabled as when CI_DEBUG_TRACE is enabled. Namely, only allow access to project members/developers if CI_DEBUG_SERVICES is enabled. --- app/models/ci/build.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 1bacd65b5747d4..13279b19a3a62b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -1040,7 +1040,8 @@ 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_TRACE']&.value&.casecmp('true') == 0 || + variables['CI_DEBUG_SERVICES']&.value&.casecmp('true') == 0 end def drop_with_exit_code!(failure_reason, exit_code) -- GitLab From e01f2e264ac58f97b327c7bb4a9033fb6649def7 Mon Sep 17 00:00:00 2001 From: Axel von Bertoldi Date: Thu, 20 Oct 2022 12:57:09 -0600 Subject: [PATCH 02/10] Add permissions unit test --- .../projects/jobs/permissions_spec.rb | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/spec/features/projects/jobs/permissions_spec.rb b/spec/features/projects/jobs/permissions_spec.rb index b6019944071c22..fb4b55191f94a9 100644 --- a/spec/features/projects/jobs/permissions_spec.rb +++ b/spec/features/projects/jobs/permissions_spec.rb @@ -211,4 +211,67 @@ 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' do + let_it_be(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) } + + where(:public_builds, :user_project_role, :ci_debug_services, :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_services) + 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 + 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_services, :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_services) + 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) + end + end + end + end end -- GitLab From 9a3e0309294ef638db28f4114553b2610a3c5674 Mon Sep 17 00:00:00 2001 From: Axel von Bertoldi Date: Thu, 20 Oct 2022 13:04:36 -0600 Subject: [PATCH 03/10] Add jobs_controller unit tests --- .../projects/jobs_controller_spec.rb | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index 58627c4dc746fe..6c846f1a6754f7 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -660,6 +660,38 @@ def get_show(**extra_params) 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 } + + 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) + end + end + end end context 'when job has a live trace' do @@ -1217,6 +1249,40 @@ def post_erase end end end + + context 'when CI_DEBUG_SERVICES enabled' do + before do + create(:ci_instance_variable, key: 'CI_DEBUG_SERVICES', value: 'true') + end + + context 'with proper permissions for debug logging on a project' do + let(:user) { developer } + + before do + sign_in(user) + end + + it 'returns response ok' do + response = subject + + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'without proper permissions for debug logging on a project' do + let(:user) { reporter } + + before do + sign_in(user) + end + + it 'returns response forbidden' do + response = subject + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + end end context 'when job has a live trace' do -- GitLab From 29f34aae898482e2ec5239436f5bd00e21192fc5 Mon Sep 17 00:00:00 2001 From: Axel von Bertoldi Date: Thu, 20 Oct 2022 13:09:25 -0600 Subject: [PATCH 04/10] Add build unit tests --- spec/models/ci/build_spec.rb | 54 ++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 9c83c1df10c784..b3313c09ef6f85 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -5095,6 +5095,60 @@ def run_job_without_exception context 'when CI_DEBUG_TRACE is not in variables' do it { is_expected.to eq false } 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') + end + + it { is_expected.to eq true } + end + + context 'when in group variables' do + before do + create(:ci_group_variable, key: 'CI_DEBUG_SERVICES', value: 'true', group: project.group) + end + + it { is_expected.to eq true } + end + + context 'when in pipeline variables' do + before do + create(:ci_pipeline_variable, key: 'CI_DEBUG_SERVICES', value: 'true', pipeline: pipeline) + end + + it { is_expected.to eq true } + end + + context 'when in project variables' do + before do + create(:ci_variable, key: 'CI_DEBUG_SERVICES', value: 'true', project: project) + end + + it { is_expected.to eq true } + end + + context 'when in job variables' do + before do + create(:ci_job_variable, key: 'CI_DEBUG_SERVICES', value: 'true', job: build) + end + + it { is_expected.to eq true } + end + + context 'when in yaml variables' do + before do + build.update!(yaml_variables: [{ key: :CI_DEBUG_SERVICES, value: '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 end describe '#drop_with_exit_code!' do -- GitLab From 23c11fbecff7b68a24682999f5c92de200eb9362 Mon Sep 17 00:00:00 2001 From: Axel von Bertoldi Date: Thu, 20 Oct 2022 13:13:31 -0600 Subject: [PATCH 05/10] Add jobs unit tests --- spec/requests/api/ci/jobs_spec.rb | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/spec/requests/api/ci/jobs_spec.rb b/spec/requests/api/ci/jobs_spec.rb index 0e17db516f4573..33579c6f1ddc94 100644 --- a/spec/requests/api/ci/jobs_spec.rb +++ b/spec/requests/api/ci/jobs_spec.rb @@ -606,6 +606,32 @@ def go end 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) + end + + where(:public_builds, :user_project_role, :expected_status) do + true | 'developer' | :ok + true | 'guest' | :forbidden + false | 'developer' | :ok + false | 'guest' | :forbidden + end + + with_them do + before do + project.update!(public_builds: public_builds) + project.add_role(user, user_project_role) + + get api("/projects/#{project.id}/jobs/#{job.id}/trace", api_user) + end + + it 'renders trace to authorized users' do + expect(response).to have_gitlab_http_status(expected_status) + end + end + end end describe 'POST /projects/:id/jobs/:job_id/cancel' do -- GitLab From e1fa1afb1d3c9aa3483c7bd5a4f4709cf39eb51d Mon Sep 17 00:00:00 2001 From: Axel von Bertoldi Date: Fri, 21 Oct 2022 08:59:15 -0600 Subject: [PATCH 06/10] Update messages and associated unit test --- app/controllers/projects/application_controller.rb | 6 +++--- locale/gitlab.pot | 2 +- spec/controllers/projects/artifacts_controller_spec.rb | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 2256471047da48..df3a95fc962795 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -38,9 +38,9 @@ def authorize_read_build_trace! if build.debug_mode? access_denied!( _('You must have developer or higher permissions in the associated project to view job logs when debug trace ' \ - "is enabled. To disable debug trace, set the 'CI_DEBUG_TRACE' variable to 'false' in your pipeline " \ - 'configuration or CI/CD settings. If you need to view this job log, a project maintainer or owner must add ' \ - 'you to the project with developer permissions or higher.') + "is enabled. To disable debug trace, set the 'CI_DEBUG_TRACE' and 'CI_DEBUG_SERVICES' variables to 'false' " \ + 'in your pipeline configuration or CI/CD settings. If you need to view this job log, a project maintainer ' \ + 'or owner must add you to the project with developer permissions or higher.') ) else access_denied!(_('The current user is not authorized to access the job log.')) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 133b3505e1b8fb..26b9d36ceb6e56 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -46729,7 +46729,7 @@ msgstr "" msgid "You must be logged in to search across all of GitLab" msgstr "" -msgid "You must have developer or higher permissions in the associated project to view job logs when debug trace is enabled. To disable debug trace, set the 'CI_DEBUG_TRACE' variable to 'false' in your pipeline configuration or CI/CD settings. If you need to view this job log, a project maintainer or owner must add you to the project with developer permissions or higher." +msgid "You must have developer or higher permissions in the associated project to view job logs when debug trace is enabled. To disable debug trace, set the 'CI_DEBUG_TRACE' and 'CI_DEBUG_SERVICES' variables to 'false' in your pipeline configuration or CI/CD settings. If you need to view this job log, a project maintainer or owner must add you to the project with developer permissions or higher." msgstr "" msgid "You must have maintainer access to force delete a lock" diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb index 808e67eff3dfce..60f83d65bacc50 100644 --- a/spec/controllers/projects/artifacts_controller_spec.rb +++ b/spec/controllers/projects/artifacts_controller_spec.rb @@ -228,8 +228,9 @@ def download_artifact(extra_params = {}) expect(response).to have_gitlab_http_status(:forbidden) expect(response.body).to include( 'You must have developer or higher permissions in the associated project to view job logs when debug trace is enabled. ' \ - 'To disable debug trace, set the 'CI_DEBUG_TRACE' variable to 'false' in your pipeline configuration or CI/CD settings. ' \ - 'If you need to view this job log, a project maintainer or owner must add you to the project with developer permissions or higher.' + 'To disable debug trace, set the 'CI_DEBUG_TRACE' and 'CI_DEBUG_SERVICES' variables to 'false' ' \ + 'in your pipeline configuration or CI/CD settings. If you need to view this job log, a project maintainer or owner must ' \ + 'add you to the project with developer permissions or higher.' ) end end -- GitLab From c502024b7dbe3fec783d76707116ea46310f27b2 Mon Sep 17 00:00:00 2001 From: Axel von Bertoldi Date: Fri, 28 Oct 2022 08:45:49 -0600 Subject: [PATCH 07/10] Combine two tests into one And add tests cases for nil user --- .../projects/jobs/permissions_spec.rb | 43 ++++++------------- 1 file changed, 12 insertions(+), 31 deletions(-) diff --git a/spec/features/projects/jobs/permissions_spec.rb b/spec/features/projects/jobs/permissions_spec.rb index fb4b55191f94a9..740d009d6b87ae 100644 --- a/spec/features/projects/jobs/permissions_spec.rb +++ b/spec/features/projects/jobs/permissions_spec.rb @@ -215,54 +215,35 @@ context 'with CI_DEBUG_SERVICES' do let_it_be(:ci_instance_variable) { create(:ci_instance_variable, key: 'CI_DEBUG_SERVICES') } - describe 'trace endpoint' do - let_it_be(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) } - - where(:public_builds, :user_project_role, :ci_debug_services, :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_services) - 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 - end - - describe 'raw page' do + 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) - project.add_role(user, user_project_role) + 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) + + expect(status_code).to eq(expected_status_code) end it 'renders raw trace to authorized users' do -- GitLab From c4ec676e4cb39ee8f18373681d7e5b2c553908ba Mon Sep 17 00:00:00 2001 From: Axel von Bertoldi Date: Fri, 28 Oct 2022 12:03:21 -0600 Subject: [PATCH 08/10] Reword rspec it --- spec/requests/api/ci/jobs_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/ci/jobs_spec.rb b/spec/requests/api/ci/jobs_spec.rb index 33579c6f1ddc94..c1b7461f444c81 100644 --- a/spec/requests/api/ci/jobs_spec.rb +++ b/spec/requests/api/ci/jobs_spec.rb @@ -627,7 +627,7 @@ def go get api("/projects/#{project.id}/jobs/#{job.id}/trace", api_user) end - it 'renders trace to authorized users' do + it 'renders successfully to authorized users' do expect(response).to have_gitlab_http_status(expected_status) end end -- GitLab From 8b090b3d2eb99dcb8b63c5dfff7dae0b6a748502 Mon Sep 17 00:00:00 2001 From: Axel von Bertoldi Date: Fri, 28 Oct 2022 14:27:34 -0600 Subject: [PATCH 09/10] Combine specs into one --- .../projects/jobs_controller_spec.rb | 83 +++++++------------ 1 file changed, 32 insertions(+), 51 deletions(-) diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index 6c846f1a6754f7..3dc89365530353 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -1216,70 +1216,51 @@ def post_erase expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" end - context 'when CI_DEBUG_TRACE enabled' do - before do - create(:ci_instance_variable, key: 'CI_DEBUG_TRACE', value: 'true') + 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' end - context 'with proper permissions for debug logging on a project' do - let(:user) { developer } - + with_them do before do - sign_in(user) - end - - it 'returns response ok' do - response = subject - - expect(response).to have_gitlab_http_status(:ok) + create(:ci_instance_variable, key: 'CI_DEBUG_TRACE', value: ci_debug_trace) + create(:ci_instance_variable, key: 'CI_DEBUG_SERVICES', value: ci_debug_services) end - end - - context 'without proper permissions for debug logging on a project' do - let(:user) { reporter } - - before do - sign_in(user) - end - - it 'returns response forbidden' do - response = subject - - expect(response).to have_gitlab_http_status(:forbidden) - end - end - end - context 'when CI_DEBUG_SERVICES enabled' do - before do - create(:ci_instance_variable, key: 'CI_DEBUG_SERVICES', value: 'true') - end + context 'with proper permissions for debug logging on a project' do + let(:user) { developer } - context 'with proper permissions for debug logging on a project' do - let(:user) { developer } - - before do - sign_in(user) - end + before do + sign_in(user) + end - it 'returns response ok' do - response = subject + it 'returns response ok' do + response = subject - expect(response).to have_gitlab_http_status(:ok) + expect(response).to have_gitlab_http_status(:ok) + end end - end - context 'without proper permissions for debug logging on a project' do - let(:user) { reporter } + context 'without proper permissions for debug logging on a project' do + let(:user) { reporter } - before do - sign_in(user) - end + before do + sign_in(user) + end - it 'returns response forbidden' do - response = subject + it 'returns response forbidden if dev mode enabled' do + response = subject - expect(response).to have_gitlab_http_status(:forbidden) + if ci_debug_trace == 'true' || ci_debug_services == 'true' + expect(response).to have_gitlab_http_status(:forbidden) + else + expect(response).to have_gitlab_http_status(:ok) + end + end end end end -- GitLab From 239bf72a1517a12c755c977b41329e7815a40c07 Mon Sep 17 00:00:00 2001 From: Axel von Bertoldi Date: Sun, 30 Oct 2022 21:40:03 -0600 Subject: [PATCH 10/10] Rework message to not use "need to" --- app/controllers/projects/application_controller.rb | 2 +- locale/gitlab.pot | 2 +- spec/controllers/projects/artifacts_controller_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index df3a95fc962795..25b83aed78a456 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -39,7 +39,7 @@ def authorize_read_build_trace! access_denied!( _('You must have developer or higher permissions in the associated project to view job logs when debug trace ' \ "is enabled. To disable debug trace, set the 'CI_DEBUG_TRACE' and 'CI_DEBUG_SERVICES' variables to 'false' " \ - 'in your pipeline configuration or CI/CD settings. If you need to view this job log, a project maintainer ' \ + 'in your pipeline configuration or CI/CD settings. If you must view this job log, a project maintainer ' \ 'or owner must add you to the project with developer permissions or higher.') ) else diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 26b9d36ceb6e56..13a4c6d1fb0ce9 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -46729,7 +46729,7 @@ msgstr "" msgid "You must be logged in to search across all of GitLab" msgstr "" -msgid "You must have developer or higher permissions in the associated project to view job logs when debug trace is enabled. To disable debug trace, set the 'CI_DEBUG_TRACE' and 'CI_DEBUG_SERVICES' variables to 'false' in your pipeline configuration or CI/CD settings. If you need to view this job log, a project maintainer or owner must add you to the project with developer permissions or higher." +msgid "You must have developer or higher permissions in the associated project to view job logs when debug trace is enabled. To disable debug trace, set the 'CI_DEBUG_TRACE' and 'CI_DEBUG_SERVICES' variables to 'false' in your pipeline configuration or CI/CD settings. If you must view this job log, a project maintainer or owner must add you to the project with developer permissions or higher." msgstr "" msgid "You must have maintainer access to force delete a lock" diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb index 60f83d65bacc50..d6165c66b35e22 100644 --- a/spec/controllers/projects/artifacts_controller_spec.rb +++ b/spec/controllers/projects/artifacts_controller_spec.rb @@ -229,7 +229,7 @@ def download_artifact(extra_params = {}) expect(response.body).to include( 'You must have developer or higher permissions in the associated project to view job logs when debug trace is enabled. ' \ 'To disable debug trace, set the 'CI_DEBUG_TRACE' and 'CI_DEBUG_SERVICES' variables to 'false' ' \ - 'in your pipeline configuration or CI/CD settings. If you need to view this job log, a project maintainer or owner must ' \ + 'in your pipeline configuration or CI/CD settings. If you must view this job log, a project maintainer or owner must ' \ 'add you to the project with developer permissions or higher.' ) end -- GitLab