From 6a67bdbbef6c0d9ec905d531dbe681ef696c370b Mon Sep 17 00:00:00 2001 From: Radamanthus Batnag Date: Thu, 5 Jun 2025 20:38:15 +0800 Subject: [PATCH 1/3] Enable SSRF for Generic Package Registry When calling present_carrierwave_file! from generic package registry endpoints, add the necessary send_url parameters for SSRF protection. This is behind the feature flag generic_package_registry_ssrf_protection Co-authored-by: DANGER_GITLAB_API_TOKEN Co-authored-by: GitLab Duo --- .../packages/package_files_controller.rb | 19 +++- ...neric_package_registry_ssrf_protection.yml | 10 ++ lib/api/helpers/packages_helpers.rb | 19 +++- spec/lib/api/helpers/packages_helpers_spec.rb | 63 +++++++++++++ spec/requests/api/generic_packages_spec.rb | 15 ++- .../packages/package_files_controller_spec.rb | 93 ++++++++++++++----- .../package_registry_ssrf_examples.rb | 66 +++++++++++++ 7 files changed, 261 insertions(+), 24 deletions(-) create mode 100644 config/feature_flags/gitlab_com_derisk/generic_package_registry_ssrf_protection.yml create mode 100644 spec/support/shared_examples/package_registry_ssrf_examples.rb diff --git a/app/controllers/projects/packages/package_files_controller.rb b/app/controllers/projects/packages/package_files_controller.rb index 0222e09f54abdd..9fd6828a6d4a85 100644 --- a/app/controllers/projects/packages/package_files_controller.rb +++ b/app/controllers/projects/packages/package_files_controller.rb @@ -13,7 +13,24 @@ def download package_file.package.touch_last_downloaded_at - send_upload(package_file.file, attachment: package_file.file_name_for_download) + send_upload(package_file.file, attachment: package_file.file_name_for_download, ssrf_params: ssrf_params) + end + + private + + def ssrf_params + return {} unless package_file.package.generic? + return {} if Feature.disabled?(:generic_package_registry_ssrf_protection, project) + + { + ssrf_filter: true, + allow_localhost: allow_localhost?, + allowed_endpoints: ObjectStoreSettings.enabled_endpoint_uris + } + end + + def allow_localhost? + Gitlab.dev_or_test_env? || Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? end end end diff --git a/config/feature_flags/gitlab_com_derisk/generic_package_registry_ssrf_protection.yml b/config/feature_flags/gitlab_com_derisk/generic_package_registry_ssrf_protection.yml new file mode 100644 index 00000000000000..2250ae9cb842ed --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/generic_package_registry_ssrf_protection.yml @@ -0,0 +1,10 @@ +--- +name: generic_package_registry_ssrf_protection +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/520294 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/193902 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/547452 +milestone: '18.2' +group: group::package registry +type: gitlab_com_derisk +default_enabled: false diff --git a/lib/api/helpers/packages_helpers.rb b/lib/api/helpers/packages_helpers.rb index a7b2795253a56e..3627244b3e416c 100644 --- a/lib/api/helpers/packages_helpers.rb +++ b/lib/api/helpers/packages_helpers.rb @@ -139,10 +139,12 @@ def track_package_event(action, scope, **args) def present_package_file!(package_file, supports_direct_download: true, content_disposition: nil) package_file.package.touch_last_downloaded_at + present_carrierwave_file!( package_file.file, supports_direct_download: supports_direct_download, - content_disposition: content_disposition + content_disposition: content_disposition, + extra_send_url_params: ssrf_protection_params(package_file.package) ) end @@ -158,6 +160,21 @@ def protect_package!(package_name, package_type) forbidden!('Package protected.') if service_response[:protection_rule_exists?] end + def ssrf_protection_params(package) + return {} unless package.generic? + + if Feature.enabled?(:generic_package_registry_ssrf_protection, package.project) + { + ssrf_filter: true, + allow_localhost: Gitlab.dev_or_test_env? || + Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?, + allowed_endpoints: ObjectStoreSettings.enabled_endpoint_uris + } + else + {} + end + end + private def track_snowplow_event(action_name, snowplow_event_name, category, args) diff --git a/spec/lib/api/helpers/packages_helpers_spec.rb b/spec/lib/api/helpers/packages_helpers_spec.rb index 2121958a83f6e5..95f91de11fa2d9 100644 --- a/spec/lib/api/helpers/packages_helpers_spec.rb +++ b/spec/lib/api/helpers/packages_helpers_spec.rb @@ -459,4 +459,67 @@ it_behaves_like params[:expected_shared_example] end end + + describe '#ssrf_protection_params' + let(:allowed_endpoints) { ['http://127.0.0.1:9000'] } + + subject { helper.ssrf_protection_params(package) } + + before do + allow(ObjectStoreSettings).to receive(:enabled_endpoint_uris).and_return(allowed_endpoints) + end + + it 'returns the extra_send_url params for SSRF protection' do + expect(subject).to eq({ + ssrf_filter: true, + allow_localhost: true, + allowed_endpoints: allowed_endpoints + }) + end + + context 'when local requests are not allowed' do + before do + allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) + end + + it 'sets allow_localhost to false' do + expect(subject).to eq({ + ssrf_filter: true, + allow_localhost: false, + allowed_endpoints: allowed_endpoints + }) + end + end + + context 'with no allowed endpoints' do + let(:allowed_endpoints) { [] } + + it 'returns empty allowed_endpoints array' do + expect(subject).to eq({ + ssrf_filter: true, + allow_localhost: true, + allowed_endpoints: [] + }) + end + end + + context 'with generic_package_registry_ssrf_protection disabled' do + before do + stub_feature_flags(generic_package_registry_ssrf_protection: false) + end + + it { is_expected.to eq({}) } + end + + ::Packages::Package.package_types.except('generic').each_key do |package_type| + context "with a #{package_type} package" do + before do + package.update_column(:package_type, package_type) + end + + it { is_expected.to eq({}) } + end + end + end end diff --git a/spec/requests/api/generic_packages_spec.rb b/spec/requests/api/generic_packages_spec.rb index 87f9d251094d79..5997518a68fdab 100644 --- a/spec/requests/api/generic_packages_spec.rb +++ b/spec/requests/api/generic_packages_spec.rb @@ -1135,19 +1135,32 @@ def upload_file( "attachment; filename=\"#{package_file.file_name}\"; filename*=UTF-8\'\'#{package_file.file_name}" end + let(:expected_headers) do + { + allow_localhost: true, + allowed_endpoints: [], + response_headers: { + 'Content-Disposition' => dispositon_header + }, + ssrf_filter: true + } + end + before do stub_package_file_object_storage(proxy_download: true) end it 'sends a file with response-content-disposition and filename' do expect(::Gitlab::Workhorse).to receive(:send_url) - .with(instance_of(String), { response_headers: { 'Content-Disposition' => dispositon_header } }) + .with(instance_of(String), expected_headers) .and_call_original download expect(response).to have_gitlab_http_status(:ok) end + + it_behaves_like 'package registry SSRF protection' end end diff --git a/spec/requests/projects/packages/package_files_controller_spec.rb b/spec/requests/projects/packages/package_files_controller_spec.rb index 5cb71e15619b4a..736b07b492f096 100644 --- a/spec/requests/projects/packages/package_files_controller_spec.rb +++ b/spec/requests/projects/packages/package_files_controller_spec.rb @@ -25,33 +25,84 @@ .to eq(%(attachment; filename="#{filename}"; filename*=UTF-8''#{filename})) end - context 'when the fog provider is Google and on .com', :saas do + context 'with remote object storage' do let(:package_file) { create(:package_file, :object_storage, package: package, file_name: filename) } - before do - stub_package_file_object_storage( - config: Gitlab.config.packages.object_store.merge(connection: { - provider: 'Google', - google_storage_access_key_id: 'test-access-id', - google_storage_secret_access_key: 'secret' - }), - proxy_download: true - ) + context 'when the fog provider is Google and on .com', :saas do + before do + stub_package_file_object_storage( + config: Gitlab.config.packages.object_store.merge(connection: { + provider: 'Google', + google_storage_access_key_id: 'test-access-id', + google_storage_secret_access_key: 'secret' + }), + proxy_download: true + ) + end + + it 'send the correct headers' do + subject + + command, encoded_params = response.headers[::Gitlab::Workhorse::SEND_DATA_HEADER].split(':') + params = Gitlab::Json.parse(Base64.urlsafe_decode64(encoded_params)) + + expect(command).to eq('send-url') + expect(params['URL']).to include( + %(response-content-disposition=attachment%3B%20filename%3D%22#{filename}), + 'x-goog-custom-audit-gitlab-project', + 'x-goog-custom-audit-gitlab-namespace', + 'x-goog-custom-audit-gitlab-size-bytes' + ) + end end - it 'send the correct headers' do - subject + context 'with SSRF protection' do + before do + stub_package_file_object_storage(proxy_download: true) + end + + it 'passes SSRF protection parameters to Workhorse send_url', :aggregate_failures do + expect(Gitlab::Workhorse).to receive(:send_url).with( + an_instance_of(String), + hash_including( + ssrf_filter: true, + allow_localhost: true, + allowed_endpoints: [] + ) + ).and_call_original + + subject + + expect(response).to have_gitlab_http_status(:ok) + end + + context 'when local requests are not allowed' do + before do + allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) + end + + it 'sets allow_localhost to false' do + expect(Gitlab::Workhorse).to receive(:send_url).with( + an_instance_of(String), + hash_including(allow_localhost: false) + ).and_call_original + + subject + end + end + + context 'when package_registry_ssrf_protection feature flag is disabled' do + before do + stub_feature_flags(generic_package_registry_ssrf_protection: false) + end - command, encoded_params = response.headers[::Gitlab::Workhorse::SEND_DATA_HEADER].split(':') - params = Gitlab::Json.parse(Base64.urlsafe_decode64(encoded_params)) + it 'does not pass SSRF protection parameters' do + expect(Gitlab::Workhorse).to receive(:send_url).with(an_instance_of(String)).and_call_original - expect(command).to eq('send-url') - expect(params['URL']).to include( - %(response-content-disposition=attachment%3B%20filename%3D%22#{filename}), - 'x-goog-custom-audit-gitlab-project', - 'x-goog-custom-audit-gitlab-namespace', - 'x-goog-custom-audit-gitlab-size-bytes' - ) + subject + end + end end end diff --git a/spec/support/shared_examples/package_registry_ssrf_examples.rb b/spec/support/shared_examples/package_registry_ssrf_examples.rb new file mode 100644 index 00000000000000..a129ee9d2730cd --- /dev/null +++ b/spec/support/shared_examples/package_registry_ssrf_examples.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'package registry SSRF protection' do + it 'passes SSRF protection parameters to Workhorse' do + expect(Gitlab::Workhorse).to receive(:send_url).with( + an_instance_of(String), + hash_including( + ssrf_filter: true, + allow_localhost: true, + allowed_endpoints: [] + ) + ).and_call_original + + subject + + expect(response).to have_gitlab_http_status(:ok) + end + + context 'with custom object store endpoints' do + let(:custom_endpoints) { ['https://custom-storage.example.com', 'https://backup.example.com'] } + + before do + allow(ObjectStoreSettings).to receive(:enabled_endpoint_uris).and_return(custom_endpoints) + end + + it 'includes custom endpoints in allowed_endpoints' do + expect(Gitlab::Workhorse).to receive(:send_url).with( + an_instance_of(String), + hash_including(allowed_endpoints: custom_endpoints) + ).and_call_original + + subject + end + end + + context 'when local requests are not allowed' do + before do + allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) + end + + it 'sets allow_localhost to false' do + expect(Gitlab::Workhorse).to receive(:send_url).with( + an_instance_of(String), + hash_including(allow_localhost: false) + ).and_call_original + + subject + end + end + + context 'when package_registry_ssrf_protection is disabled' do + before do + stub_feature_flags(generic_package_registry_ssrf_protection: false) + end + + it 'does not pass SSRF protection parameters' do + expect(Gitlab::Workhorse).to receive(:send_url).with( + an_instance_of(String), + hash_not_including(:ssrf_filter, :allow_localhost, :allowed_endpoints) + ).and_call_original + + subject + end + end +end -- GitLab From 7d022891246bfe112e030d7b5c70db524e2c5db9 Mon Sep 17 00:00:00 2001 From: Radamanthus Batnag Date: Tue, 24 Jun 2025 20:59:53 +0800 Subject: [PATCH 2/3] Refactoring suggested by Duo Co-authored-by: GitLab Duo --- .../packages/package_files_controller.rb | 18 +-- app/models/packages/ssrf_protection.rb | 32 +++++ lib/api/helpers/packages_helpers.rb | 14 +-- spec/lib/api/helpers/packages_helpers_spec.rb | 10 +- spec/models/packages/ssrf_protection_spec.rb | 111 ++++++++++++++++++ spec/requests/api/generic_packages_spec.rb | 4 +- .../packages/package_files_controller_spec.rb | 7 +- .../package_registry_ssrf_examples.rb | 4 +- 8 files changed, 164 insertions(+), 36 deletions(-) create mode 100644 app/models/packages/ssrf_protection.rb create mode 100644 spec/models/packages/ssrf_protection_spec.rb diff --git a/app/controllers/projects/packages/package_files_controller.rb b/app/controllers/projects/packages/package_files_controller.rb index 9fd6828a6d4a85..25694f4e1d6065 100644 --- a/app/controllers/projects/packages/package_files_controller.rb +++ b/app/controllers/projects/packages/package_files_controller.rb @@ -13,24 +13,14 @@ def download package_file.package.touch_last_downloaded_at - send_upload(package_file.file, attachment: package_file.file_name_for_download, ssrf_params: ssrf_params) + send_upload(package_file.file, attachment: package_file.file_name_for_download, + ssrf_params: ssrf_protection_params(package_file.package)) end private - def ssrf_params - return {} unless package_file.package.generic? - return {} if Feature.disabled?(:generic_package_registry_ssrf_protection, project) - - { - ssrf_filter: true, - allow_localhost: allow_localhost?, - allowed_endpoints: ObjectStoreSettings.enabled_endpoint_uris - } - end - - def allow_localhost? - Gitlab.dev_or_test_env? || Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? + def ssrf_protection_params(package) + ::Packages::SsrfProtection.params_for(package) end end end diff --git a/app/models/packages/ssrf_protection.rb b/app/models/packages/ssrf_protection.rb new file mode 100644 index 00000000000000..f5dad619dbefcd --- /dev/null +++ b/app/models/packages/ssrf_protection.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Packages + module SsrfProtection + def self.params_for(package) + return {} unless package + return {} unless package_feature_enabled?(package) + + { + ssrf_filter: true, + allow_localhost: allow_localhost?, + allowed_endpoints: ObjectStoreSettings.enabled_endpoint_uris + } + end + + def self.allow_localhost? + Gitlab.dev_or_test_env? || Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? + end + + def self.package_feature_enabled?(package) + case package.package_type.to_sym + when :generic + Feature.enabled?(:generic_package_registry_ssrf_protection, package.project) + # Future package types can be added here + # when :npm + # Feature.enabled?(:npm_package_registry_ssrf_protection, package.project) + else + false + end + end + end +end diff --git a/lib/api/helpers/packages_helpers.rb b/lib/api/helpers/packages_helpers.rb index 3627244b3e416c..19a2cd9ea668e8 100644 --- a/lib/api/helpers/packages_helpers.rb +++ b/lib/api/helpers/packages_helpers.rb @@ -139,7 +139,6 @@ def track_package_event(action, scope, **args) def present_package_file!(package_file, supports_direct_download: true, content_disposition: nil) package_file.package.touch_last_downloaded_at - present_carrierwave_file!( package_file.file, supports_direct_download: supports_direct_download, @@ -161,18 +160,7 @@ def protect_package!(package_name, package_type) end def ssrf_protection_params(package) - return {} unless package.generic? - - if Feature.enabled?(:generic_package_registry_ssrf_protection, package.project) - { - ssrf_filter: true, - allow_localhost: Gitlab.dev_or_test_env? || - Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?, - allowed_endpoints: ObjectStoreSettings.enabled_endpoint_uris - } - else - {} - end + ::Packages::SsrfProtection.params_for(package) end private diff --git a/spec/lib/api/helpers/packages_helpers_spec.rb b/spec/lib/api/helpers/packages_helpers_spec.rb index 95f91de11fa2d9..79e6b2cf86cd3b 100644 --- a/spec/lib/api/helpers/packages_helpers_spec.rb +++ b/spec/lib/api/helpers/packages_helpers_spec.rb @@ -460,7 +460,7 @@ end end - describe '#ssrf_protection_params' + describe '#ssrf_protection_params' do let(:allowed_endpoints) { ['http://127.0.0.1:9000'] } subject { helper.ssrf_protection_params(package) } @@ -477,6 +477,12 @@ }) end + context 'when package is nil' do + subject { helper.ssrf_protection_params(nil) } + + it { is_expected.to eq({}) } + end + context 'when local requests are not allowed' do before do allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) @@ -515,7 +521,7 @@ ::Packages::Package.package_types.except('generic').each_key do |package_type| context "with a #{package_type} package" do before do - package.update_column(:package_type, package_type) + package.update!(package_type: package_type) end it { is_expected.to eq({}) } diff --git a/spec/models/packages/ssrf_protection_spec.rb b/spec/models/packages/ssrf_protection_spec.rb new file mode 100644 index 00000000000000..c42ce8fa3c66aa --- /dev/null +++ b/spec/models/packages/ssrf_protection_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::SsrfProtection, type: :model, feature_category: :package_registry do + let_it_be(:project) { create(:project) } + let_it_be(:generic_package) { create(:generic_package, project: project) } + let_it_be(:unsupported_package) { create(:conan_package, project: project) } + + describe '.params_for' do + context 'when package is nil' do + it 'returns empty hash' do + expect(described_class.params_for(nil)).to eq({}) + end + end + + context 'when package type is not supported' do + it 'returns empty hash' do + expect(described_class.params_for(unsupported_package)).to eq({}) + end + end + + context 'when package type is supported' do + it 'returns SSRF protection params for generic package' do + result = described_class.params_for(generic_package) + + expect(result).to include( + ssrf_filter: true, + allow_localhost: true, + allowed_endpoints: ObjectStoreSettings.enabled_endpoint_uris + ) + end + + context 'when in production environment' do + before do + allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) + end + + context 'when local requests are allowed from webhooks' do + before do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) + end + + it 'allows localhost' do + result = described_class.params_for(generic_package) + expect(result[:allow_localhost]).to be true + end + end + + context 'when local requests are not allowed from webhooks' do + before do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) + end + + it 'does not allow localhost' do + result = described_class.params_for(generic_package) + expect(result[:allow_localhost]).to be false + end + end + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(generic_package_registry_ssrf_protection: false) + end + + it 'returns empty hash for generic package' do + expect(described_class.params_for(generic_package)).to eq({}) + end + end + end + end + + describe '.allow_localhost?' do + context 'when in development environment' do + before do + allow(Gitlab).to receive(:dev_or_test_env?).and_return(true) + end + + it 'returns true' do + expect(described_class.allow_localhost?).to be true + end + end + + context 'when in production environment' do + before do + allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) + end + + context 'when local requests are allowed from webhooks' do + before do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) + end + + it 'returns true' do + expect(described_class.allow_localhost?).to be true + end + end + + context 'when local requests are not allowed from webhooks' do + before do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) + end + + it 'returns false' do + expect(described_class.allow_localhost?).to be false + end + end + end + end +end diff --git a/spec/requests/api/generic_packages_spec.rb b/spec/requests/api/generic_packages_spec.rb index 5997518a68fdab..312e23b51d1888 100644 --- a/spec/requests/api/generic_packages_spec.rb +++ b/spec/requests/api/generic_packages_spec.rb @@ -1131,7 +1131,7 @@ def upload_file( end context 'when direct download is disabled' do - let(:dispositon_header) do + let(:disposition_header) do "attachment; filename=\"#{package_file.file_name}\"; filename*=UTF-8\'\'#{package_file.file_name}" end @@ -1140,7 +1140,7 @@ def upload_file( allow_localhost: true, allowed_endpoints: [], response_headers: { - 'Content-Disposition' => dispositon_header + 'Content-Disposition' => disposition_header }, ssrf_filter: true } diff --git a/spec/requests/projects/packages/package_files_controller_spec.rb b/spec/requests/projects/packages/package_files_controller_spec.rb index 736b07b492f096..2bb8f1059affb0 100644 --- a/spec/requests/projects/packages/package_files_controller_spec.rb +++ b/spec/requests/projects/packages/package_files_controller_spec.rb @@ -92,14 +92,15 @@ end end - context 'when package_registry_ssrf_protection feature flag is disabled' do + context 'when generic_package_registry_ssrf_protection feature flag is disabled' do before do stub_feature_flags(generic_package_registry_ssrf_protection: false) end it 'does not pass SSRF protection parameters' do - expect(Gitlab::Workhorse).to receive(:send_url).with(an_instance_of(String)).and_call_original - + expect(Gitlab::Workhorse).to receive(:send_url).with( + an_instance_of(String) + ).and_call_original subject end end diff --git a/spec/support/shared_examples/package_registry_ssrf_examples.rb b/spec/support/shared_examples/package_registry_ssrf_examples.rb index a129ee9d2730cd..40029cd8ca08fa 100644 --- a/spec/support/shared_examples/package_registry_ssrf_examples.rb +++ b/spec/support/shared_examples/package_registry_ssrf_examples.rb @@ -49,7 +49,7 @@ end end - context 'when package_registry_ssrf_protection is disabled' do + context 'when generic_package_registry_ssrf_protection is disabled' do before do stub_feature_flags(generic_package_registry_ssrf_protection: false) end @@ -57,7 +57,7 @@ it 'does not pass SSRF protection parameters' do expect(Gitlab::Workhorse).to receive(:send_url).with( an_instance_of(String), - hash_not_including(:ssrf_filter, :allow_localhost, :allowed_endpoints) + hash_excluding(:ssrf_filter, :allow_localhost, :allowed_endpoints) ).and_call_original subject -- GitLab From 35d578ed6f30425d7b8d555959d2317d554d1eab Mon Sep 17 00:00:00 2001 From: Radamanthus Batnag Date: Thu, 3 Jul 2025 17:49:14 +0800 Subject: [PATCH 3/3] Implement backend reviewer feedback --- .../packages/package_files_controller.rb | 8 +-- lib/api/helpers/packages_helpers.rb | 6 +- .../packages/ssrf_protection.rb | 2 +- spec/lib/api/helpers/packages_helpers_spec.rb | 69 ------------------- .../packages/ssrf_protection_spec.rb | 2 +- .../packages/package_files_controller_spec.rb | 46 +------------ .../package_registry_ssrf_examples.rb | 25 ++----- 7 files changed, 12 insertions(+), 146 deletions(-) rename {app/models => lib}/packages/ssrf_protection.rb (97%) rename spec/{models => lib}/packages/ssrf_protection_spec.rb (97%) diff --git a/app/controllers/projects/packages/package_files_controller.rb b/app/controllers/projects/packages/package_files_controller.rb index 25694f4e1d6065..0b3ab1ae1ad999 100644 --- a/app/controllers/projects/packages/package_files_controller.rb +++ b/app/controllers/projects/packages/package_files_controller.rb @@ -14,13 +14,7 @@ def download package_file.package.touch_last_downloaded_at send_upload(package_file.file, attachment: package_file.file_name_for_download, - ssrf_params: ssrf_protection_params(package_file.package)) - end - - private - - def ssrf_protection_params(package) - ::Packages::SsrfProtection.params_for(package) + ssrf_params: ::Packages::SsrfProtection.params_for(package_file.package)) end end end diff --git a/lib/api/helpers/packages_helpers.rb b/lib/api/helpers/packages_helpers.rb index 19a2cd9ea668e8..36055a71ffa3a9 100644 --- a/lib/api/helpers/packages_helpers.rb +++ b/lib/api/helpers/packages_helpers.rb @@ -143,7 +143,7 @@ def present_package_file!(package_file, supports_direct_download: true, content_ package_file.file, supports_direct_download: supports_direct_download, content_disposition: content_disposition, - extra_send_url_params: ssrf_protection_params(package_file.package) + extra_send_url_params: ::Packages::SsrfProtection.params_for(package_file.package) ) end @@ -159,10 +159,6 @@ def protect_package!(package_name, package_type) forbidden!('Package protected.') if service_response[:protection_rule_exists?] end - def ssrf_protection_params(package) - ::Packages::SsrfProtection.params_for(package) - end - private def track_snowplow_event(action_name, snowplow_event_name, category, args) diff --git a/app/models/packages/ssrf_protection.rb b/lib/packages/ssrf_protection.rb similarity index 97% rename from app/models/packages/ssrf_protection.rb rename to lib/packages/ssrf_protection.rb index f5dad619dbefcd..6b42c4decaa15f 100644 --- a/app/models/packages/ssrf_protection.rb +++ b/lib/packages/ssrf_protection.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Packages - module SsrfProtection + class SsrfProtection def self.params_for(package) return {} unless package return {} unless package_feature_enabled?(package) diff --git a/spec/lib/api/helpers/packages_helpers_spec.rb b/spec/lib/api/helpers/packages_helpers_spec.rb index 79e6b2cf86cd3b..2121958a83f6e5 100644 --- a/spec/lib/api/helpers/packages_helpers_spec.rb +++ b/spec/lib/api/helpers/packages_helpers_spec.rb @@ -459,73 +459,4 @@ it_behaves_like params[:expected_shared_example] end end - - describe '#ssrf_protection_params' do - let(:allowed_endpoints) { ['http://127.0.0.1:9000'] } - - subject { helper.ssrf_protection_params(package) } - - before do - allow(ObjectStoreSettings).to receive(:enabled_endpoint_uris).and_return(allowed_endpoints) - end - - it 'returns the extra_send_url params for SSRF protection' do - expect(subject).to eq({ - ssrf_filter: true, - allow_localhost: true, - allowed_endpoints: allowed_endpoints - }) - end - - context 'when package is nil' do - subject { helper.ssrf_protection_params(nil) } - - it { is_expected.to eq({}) } - end - - context 'when local requests are not allowed' do - before do - allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) - stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) - end - - it 'sets allow_localhost to false' do - expect(subject).to eq({ - ssrf_filter: true, - allow_localhost: false, - allowed_endpoints: allowed_endpoints - }) - end - end - - context 'with no allowed endpoints' do - let(:allowed_endpoints) { [] } - - it 'returns empty allowed_endpoints array' do - expect(subject).to eq({ - ssrf_filter: true, - allow_localhost: true, - allowed_endpoints: [] - }) - end - end - - context 'with generic_package_registry_ssrf_protection disabled' do - before do - stub_feature_flags(generic_package_registry_ssrf_protection: false) - end - - it { is_expected.to eq({}) } - end - - ::Packages::Package.package_types.except('generic').each_key do |package_type| - context "with a #{package_type} package" do - before do - package.update!(package_type: package_type) - end - - it { is_expected.to eq({}) } - end - end - end end diff --git a/spec/models/packages/ssrf_protection_spec.rb b/spec/lib/packages/ssrf_protection_spec.rb similarity index 97% rename from spec/models/packages/ssrf_protection_spec.rb rename to spec/lib/packages/ssrf_protection_spec.rb index c42ce8fa3c66aa..4e3c9cc3290a48 100644 --- a/spec/models/packages/ssrf_protection_spec.rb +++ b/spec/lib/packages/ssrf_protection_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Packages::SsrfProtection, type: :model, feature_category: :package_registry do +RSpec.describe Packages::SsrfProtection, feature_category: :package_registry do let_it_be(:project) { create(:project) } let_it_be(:generic_package) { create(:generic_package, project: project) } let_it_be(:unsupported_package) { create(:conan_package, project: project) } diff --git a/spec/requests/projects/packages/package_files_controller_spec.rb b/spec/requests/projects/packages/package_files_controller_spec.rb index 2bb8f1059affb0..a68866de8931c5 100644 --- a/spec/requests/projects/packages/package_files_controller_spec.rb +++ b/spec/requests/projects/packages/package_files_controller_spec.rb @@ -56,54 +56,12 @@ end end - context 'with SSRF protection' do + context 'when direct download is disabled' do before do stub_package_file_object_storage(proxy_download: true) end - it 'passes SSRF protection parameters to Workhorse send_url', :aggregate_failures do - expect(Gitlab::Workhorse).to receive(:send_url).with( - an_instance_of(String), - hash_including( - ssrf_filter: true, - allow_localhost: true, - allowed_endpoints: [] - ) - ).and_call_original - - subject - - expect(response).to have_gitlab_http_status(:ok) - end - - context 'when local requests are not allowed' do - before do - allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) - stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) - end - - it 'sets allow_localhost to false' do - expect(Gitlab::Workhorse).to receive(:send_url).with( - an_instance_of(String), - hash_including(allow_localhost: false) - ).and_call_original - - subject - end - end - - context 'when generic_package_registry_ssrf_protection feature flag is disabled' do - before do - stub_feature_flags(generic_package_registry_ssrf_protection: false) - end - - it 'does not pass SSRF protection parameters' do - expect(Gitlab::Workhorse).to receive(:send_url).with( - an_instance_of(String) - ).and_call_original - subject - end - end + it_behaves_like 'package registry SSRF protection' end end diff --git a/spec/support/shared_examples/package_registry_ssrf_examples.rb b/spec/support/shared_examples/package_registry_ssrf_examples.rb index 40029cd8ca08fa..500011ba228749 100644 --- a/spec/support/shared_examples/package_registry_ssrf_examples.rb +++ b/spec/support/shared_examples/package_registry_ssrf_examples.rb @@ -1,21 +1,6 @@ # frozen_string_literal: true RSpec.shared_examples 'package registry SSRF protection' do - it 'passes SSRF protection parameters to Workhorse' do - expect(Gitlab::Workhorse).to receive(:send_url).with( - an_instance_of(String), - hash_including( - ssrf_filter: true, - allow_localhost: true, - allowed_endpoints: [] - ) - ).and_call_original - - subject - - expect(response).to have_gitlab_http_status(:ok) - end - context 'with custom object store endpoints' do let(:custom_endpoints) { ['https://custom-storage.example.com', 'https://backup.example.com'] } @@ -55,10 +40,12 @@ end it 'does not pass SSRF protection parameters' do - expect(Gitlab::Workhorse).to receive(:send_url).with( - an_instance_of(String), - hash_excluding(:ssrf_filter, :allow_localhost, :allowed_endpoints) - ).and_call_original + expect(Gitlab::Workhorse).to receive(:send_url).and_wrap_original do |method, *args| + expect(args.first).to be_an_instance_of(String) + expect(args.last).not_to include(:ssrf_filter, :allow_localhost, :allowed_endpoints) if args.last.is_a?(Hash) + + method.call(*args) + end subject end -- GitLab