diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-11-30 04:46:00 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-11-30 04:46:05 +0000 |
commit | 17a3845ec7407bd4b3f1087a9f540ce7bc985dfd (patch) | |
tree | 55558934c2782e10e39e6a67338fba7d4bf060ec | |
parent | 1138bd0efdab7855032791a5c8b23ed216b0991b (diff) | |
download | gitlab-ce-17a3845ec7407bd4b3f1087a9f540ce7bc985dfd.tar.gz |
Add latest changes from gitlab-org/security/gitlab@15-5-stable-ee
-rw-r--r-- | app/models/ci/build_runner_session.rb | 20 | ||||
-rw-r--r-- | app/models/user.rb | 2 | ||||
-rw-r--r-- | app/policies/packages/policies/group_policy.rb | 2 | ||||
-rw-r--r-- | app/policies/packages/policies/project_policy.rb | 2 | ||||
-rw-r--r-- | lib/api/ci/runner.rb | 3 | ||||
-rw-r--r-- | lib/gitlab/hook_data/project_builder.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/hook_data/project_member_builder.rb | 2 | ||||
-rw-r--r-- | spec/controllers/projects/jobs_controller_spec.rb | 5 | ||||
-rw-r--r-- | spec/factories/ci/builds.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/hook_data/project_builder_spec.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/hook_data/project_member_builder_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/ci/build_runner_session_spec.rb | 57 | ||||
-rw-r--r-- | spec/requests/api/ci/runner/jobs_request_post_spec.rb | 35 |
13 files changed, 116 insertions, 22 deletions
diff --git a/app/models/ci/build_runner_session.rb b/app/models/ci/build_runner_session.rb index c6dbb5d0a43..0f37ce70964 100644 --- a/app/models/ci/build_runner_session.rb +++ b/app/models/ci/build_runner_session.rb @@ -13,14 +13,15 @@ module Ci belongs_to :build, class_name: 'Ci::Build', inverse_of: :runner_session validates :build, presence: true - validates :url, addressable_url: { schemes: %w(https) } + validates :url, public_url: { schemes: %w(https) } def terminal_specification - wss_url = Gitlab::UrlHelpers.as_wss(self.url) + wss_url = Gitlab::UrlHelpers.as_wss(Addressable::URI.escape(self.url)) return {} unless wss_url.present? - wss_url = "#{wss_url}/exec" - channel_specification(wss_url, TERMINAL_SUBPROTOCOL) + parsed_wss_url = URI.parse(wss_url) + parsed_wss_url.path += '/exec' + channel_specification(parsed_wss_url, TERMINAL_SUBPROTOCOL) end def service_specification(service: nil, path: nil, port: nil, subprotocols: nil) @@ -28,20 +29,21 @@ module Ci port = port.presence || DEFAULT_PORT_NAME service = service.presence || DEFAULT_SERVICE_NAME - url = "#{self.url}/proxy/#{service}/#{port}/#{path}" + parsed_url = URI.parse(Addressable::URI.escape(self.url)) + parsed_url.path += "/proxy/#{service}/#{port}/#{path}" subprotocols = subprotocols.presence || ::Ci::BuildRunnerSession::TERMINAL_SUBPROTOCOL - channel_specification(url, subprotocols) + channel_specification(parsed_url, subprotocols) end private - def channel_specification(url, subprotocol) - return {} if subprotocol.blank? || url.blank? + def channel_specification(parsed_url, subprotocol) + return {} if subprotocol.blank? || parsed_url.blank? { subprotocols: Array(subprotocol), - url: url, + url: Addressable::URI.unescape(parsed_url.to_s), headers: { Authorization: [authorization.presence] }.compact, ca_pem: certificate.presence } diff --git a/app/models/user.rb b/app/models/user.rb index 6d198fc755b..ff34b00187c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1574,7 +1574,7 @@ class User < ApplicationRecord name: name, username: username, avatar_url: avatar_url(only_path: false), - email: public_email.presence || _('[REDACTED]') + email: webhook_email } end diff --git a/app/policies/packages/policies/group_policy.rb b/app/policies/packages/policies/group_policy.rb index 32dbcb1b65b..d8c20c7a90a 100644 --- a/app/policies/packages/policies/group_policy.rb +++ b/app/policies/packages/policies/group_policy.rb @@ -25,3 +25,5 @@ module Packages end end end + +Packages::Policies::GroupPolicy.prepend_mod_with('Packages::Policies::GroupPolicy') diff --git a/app/policies/packages/policies/project_policy.rb b/app/policies/packages/policies/project_policy.rb index c754d24349a..0fb5953f2aa 100644 --- a/app/policies/packages/policies/project_policy.rb +++ b/app/policies/packages/policies/project_policy.rb @@ -52,3 +52,5 @@ module Packages end end end + +Packages::Policies::ProjectPolicy.prepend_mod_with('Packages::Policies::ProjectPolicy') diff --git a/lib/api/ci/runner.rb b/lib/api/ci/runner.rb index 2d2dcc544f9..a56601d3b2f 100644 --- a/lib/api/ci/runner.rb +++ b/lib/api/ci/runner.rb @@ -94,7 +94,8 @@ module API success Entities::Ci::JobRequest::Response http_codes [[201, 'Job was scheduled'], [204, 'No job for Runner'], - [403, 'Forbidden']] + [403, 'Forbidden'], + [409, 'Conflict']] end params do requires :token, type: String, desc: %q(Runner's authentication token) diff --git a/lib/gitlab/hook_data/project_builder.rb b/lib/gitlab/hook_data/project_builder.rb index ebd97d3ab1b..aec842e061f 100644 --- a/lib/gitlab/hook_data/project_builder.rb +++ b/lib/gitlab/hook_data/project_builder.rb @@ -57,7 +57,7 @@ module Gitlab end def user_email(user) - user.respond_to?(:email) ? user.email : "" + user.respond_to?(:webhook_email) ? user.webhook_email : "" end def event_specific_project_data(event) diff --git a/lib/gitlab/hook_data/project_member_builder.rb b/lib/gitlab/hook_data/project_member_builder.rb index 2ee61705ec1..be5bde356de 100644 --- a/lib/gitlab/hook_data/project_member_builder.rb +++ b/lib/gitlab/hook_data/project_member_builder.rb @@ -43,7 +43,7 @@ module Gitlab project_id: project.id, user_username: project_member.user.username, user_name: project_member.user.name, - user_email: project_member.user.email, + user_email: project_member.user.webhook_email, user_id: project_member.user.id, access_level: project_member.human_access, project_visibility: project.visibility diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index 556dd23c135..58627c4dc74 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -1380,7 +1380,7 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do { 'Channel' => { 'Subprotocols' => ["terminal.gitlab.com"], - 'Url' => 'wss://localhost/proxy/build/default_port/', + 'Url' => 'wss://gitlab.example.com/proxy/build/default_port/', 'Header' => { 'Authorization' => [nil] }, @@ -1536,7 +1536,8 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do allow(Gitlab::Workhorse).to receive(:verify_api_request!).and_return(nil) expect(job.runner_session_url).to start_with('https://') - expect(Gitlab::Workhorse).to receive(:channel_websocket).with(a_hash_including(url: "wss://localhost/proxy/build/default_port/")) + expect(Gitlab::Workhorse).to receive(:channel_websocket) + .with(a_hash_including(url: "wss://gitlab.example.com/proxy/build/default_port/")) make_request end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 9a3b2837ab8..8396ef480c7 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -716,7 +716,7 @@ FactoryBot.define do trait :with_runner_session do after(:build) do |build| - build.build_runner_session(url: 'https://localhost') + build.build_runner_session(url: 'https://gitlab.example.com') end end diff --git a/spec/lib/gitlab/hook_data/project_builder_spec.rb b/spec/lib/gitlab/hook_data/project_builder_spec.rb index 729712510ea..f80faac563d 100644 --- a/spec/lib/gitlab/hook_data/project_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/project_builder_spec.rb @@ -29,8 +29,8 @@ RSpec.describe Gitlab::HookData::ProjectBuilder do expect(data[:path_with_namespace]).to eq(project.full_path) expect(data[:project_id]).to eq(project.id) expect(data[:owner_name]).to eq('John') - expect(data[:owner_email]).to eq('john@example.com') - expect(data[:owners]).to contain_exactly({ name: 'John', email: 'john@example.com' }) + expect(data[:owner_email]).to eq(_('[REDACTED]')) + expect(data[:owners]).to contain_exactly({ name: 'John', email: _('[REDACTED]') }) expect(data[:project_visibility]).to eq('internal') end end diff --git a/spec/lib/gitlab/hook_data/project_member_builder_spec.rb b/spec/lib/gitlab/hook_data/project_member_builder_spec.rb index 76446adf7b7..ea71c5442f4 100644 --- a/spec/lib/gitlab/hook_data/project_member_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/project_member_builder_spec.rb @@ -27,7 +27,7 @@ RSpec.describe Gitlab::HookData::ProjectMemberBuilder do expect(data[:user_username]).to eq('johndoe') expect(data[:user_name]).to eq('John Doe') expect(data[:user_id]).to eq(user.id) - expect(data[:user_email]).to eq('john@example.com') + expect(data[:user_email]).to eq(_('[REDACTED]')) expect(data[:access_level]).to eq('Developer') expect(data[:project_visibility]).to eq('internal') end diff --git a/spec/models/ci/build_runner_session_spec.rb b/spec/models/ci/build_runner_session_spec.rb index ed5ed456d7b..8dfe854511c 100644 --- a/spec/models/ci/build_runner_session_spec.rb +++ b/spec/models/ci/build_runner_session_spec.rb @@ -13,6 +13,45 @@ RSpec.describe Ci::BuildRunnerSession, model: true do it { is_expected.to validate_presence_of(:build) } it { is_expected.to validate_presence_of(:url).with_message('must be a valid URL') } + context 'url validation of local web hook address' do + let(:url) { 'https://127.0.0.1:7777' } + + subject(:build_with_local_runner_session_url) do + create(:ci_build).tap { |b| b.update!(runner_session_attributes: { url: url }) } + end + + context 'with allow_local_requests_from_web_hooks_and_services? stubbed' do + before do + allow(ApplicationSetting).to receive(:current).and_return(ApplicationSetting.new) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: allow_local_requests) + end + + context 'as returning true' do + let(:allow_local_requests) { true } + + it 'creates a new session', :aggregate_failures do + session = build_with_local_runner_session_url.reload.runner_session + + expect(session.errors).to be_empty + expect(session).to be_a(Ci::BuildRunnerSession) + expect(session.url).to eq(url) + end + end + + context 'as returning false' do + let(:allow_local_requests) { false } + + it 'does not create a new session' do + expect { build_with_local_runner_session_url }.to raise_error(ActiveRecord::RecordInvalid) do |err| + expect(err.record.errors.full_messages).to include( + 'Runner session url is blocked: Requests to localhost are not allowed' + ) + end + end + end + end + end + context 'nested attribute assignment' do it 'creates a new session' do simple_build = create(:ci_build) @@ -49,6 +88,12 @@ RSpec.describe Ci::BuildRunnerSession, model: true do expect(specification).to be_empty end + it 'returns url with appended query if url has query' do + subject.url = 'https://new.example.com:7777/some_path?dummy=' + + expect(specification[:url]).to eq('wss://new.example.com:7777/some_path/exec?dummy=') + end + context 'when url is present' do it 'returns ca_pem nil if empty certificate' do subject.certificate = '' @@ -72,7 +117,7 @@ RSpec.describe Ci::BuildRunnerSession, model: true do let(:specification) { subject.service_specification(service: service, port: port, path: path, subprotocols: subprotocols) } it 'returns service proxy url' do - expect(specification[:url]).to eq "https://localhost/proxy/#{service}/#{port}/#{path}" + expect(specification[:url]).to eq "https://gitlab.example.com/proxy/#{service}/#{port}/#{path}" end it 'returns default service proxy websocket subprotocol' do @@ -85,11 +130,17 @@ RSpec.describe Ci::BuildRunnerSession, model: true do expect(specification).to be_empty end + it 'returns url with appended query if url has query' do + subject.url = 'https://new.example.com:7777/some_path?dummy=' + + expect(specification[:url]).to eq("https://new.example.com:7777/some_path/proxy/#{service}/#{port}/#{path}?dummy=") + end + context 'when port is not present' do let(:port) { nil } it 'uses the default port name' do - expect(specification[:url]).to eq "https://localhost/proxy/#{service}/default_port/#{path}" + expect(specification[:url]).to eq "https://gitlab.example.com/proxy/#{service}/default_port/#{path}" end end @@ -97,7 +148,7 @@ RSpec.describe Ci::BuildRunnerSession, model: true do let(:service) { '' } it 'uses the service name "build" as default' do - expect(specification[:url]).to eq "https://localhost/proxy/build/#{port}/#{path}" + expect(specification[:url]).to eq "https://gitlab.example.com/proxy/build/#{port}/#{path}" end end diff --git a/spec/requests/api/ci/runner/jobs_request_post_spec.rb b/spec/requests/api/ci/runner/jobs_request_post_spec.rb index d4f734e7bdd..baa16b052a0 100644 --- a/spec/requests/api/ci/runner/jobs_request_post_spec.rb +++ b/spec/requests/api/ci/runner/jobs_request_post_spec.rb @@ -949,6 +949,41 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do end end + context 'with session url set to local URL' do + let(:job_params) { { session: { url: 'https://127.0.0.1:7777' } } } + + context 'with allow_local_requests_from_web_hooks_and_services? stubbed' do + before do + allow(ApplicationSetting).to receive(:current).and_return(ApplicationSetting.new) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: allow_local_requests) + ci_build + end + + let(:ci_build) { create(:ci_build, :pending, :queued, pipeline: pipeline) } + + context 'as returning true' do + let(:allow_local_requests) { true } + + it 'creates a new session' do + request_job(**job_params) + + expect(response).to have_gitlab_http_status(:created) + end + end + + context 'as returning false' do + let(:allow_local_requests) { false } + + it 'returns :unprocessable_entity status code', :aggregate_failures do + request_job(**job_params) + + expect(response).to have_gitlab_http_status(:conflict) + expect(response.body).to include('409 Conflict') + end + end + end + end + def request_job(token = runner.token, **params) new_params = params.merge(token: token, last_update: last_update) post api('/jobs/request'), params: new_params.to_json, headers: { 'User-Agent' => user_agent, 'Content-Type': 'application/json' } |