From adf23d90519af8f48e4b7d76349fd2c9652d7839 Mon Sep 17 00:00:00 2001 From: Sylvia Shen Date: Mon, 28 Jul 2025 16:02:35 +0200 Subject: [PATCH 1/4] Stop log content type determination --- app/controllers/concerns/send_file_upload.rb | 8 ++--- .../packages/package_files_controller.rb | 4 +-- lib/api/generic_packages.rb | 7 ---- lib/gitlab/utils/mime_type.rb | 12 ++----- spec/lib/gitlab/utils/mime_type_spec.rb | 32 +++++++------------ spec/requests/api/generic_packages_spec.rb | 24 -------------- .../packages/package_files_controller_spec.rb | 32 ------------------- 7 files changed, 19 insertions(+), 100 deletions(-) diff --git a/app/controllers/concerns/send_file_upload.rb b/app/controllers/concerns/send_file_upload.rb index 634255dff2757f..0266adcee4f302 100644 --- a/app/controllers/concerns/send_file_upload.rb +++ b/app/controllers/concerns/send_file_upload.rb @@ -3,8 +3,8 @@ module SendFileUpload def send_upload( file_upload, send_params: {}, redirect_params: {}, attachment: nil, proxy: false, - disposition: 'attachment', ssrf_params: {}, log_enabled: false) - content_type = content_type_for(attachment, log_enabled) + disposition: 'attachment', ssrf_params: {}) + content_type = content_type_for(attachment) if attachment response_disposition = ActionDispatch::Http::ContentDisposition.format(disposition: disposition, @@ -41,10 +41,10 @@ def send_upload( end end - def content_type_for(attachment, log_enabled) + def content_type_for(attachment) return '' unless attachment - ::Gitlab::Utils::MimeType.from_filename(attachment, log_enabled: log_enabled) + ::Gitlab::Utils::MimeType.from_filename(attachment) end private diff --git a/app/controllers/projects/packages/package_files_controller.rb b/app/controllers/projects/packages/package_files_controller.rb index eaaaf5ba54fd3b..ad185ee9f217cc 100644 --- a/app/controllers/projects/packages/package_files_controller.rb +++ b/app/controllers/projects/packages/package_files_controller.rb @@ -13,10 +13,8 @@ def download package_file.package.touch_last_downloaded_at - log_enabled = package_file.package.generic? && Feature.enabled?(:packages_generic_package_content_type, project) - send_upload(package_file.file, attachment: package_file.file_name_for_download, - ssrf_params: ::Packages::SsrfProtection.params_for(package_file.package), log_enabled: log_enabled) + ssrf_params: ::Packages::SsrfProtection.params_for(package_file.package)) end end end diff --git a/lib/api/generic_packages.rb b/lib/api/generic_packages.rb index 309394eb1fe375..f77750cdfb1a71 100644 --- a/lib/api/generic_packages.rb +++ b/lib/api/generic_packages.rb @@ -147,7 +147,6 @@ class GenericPackages < ::API::Base package = ::Packages::Generic::PackageFinder.new(project).execute!(params[:package_name], params[:package_version]) package_file = ::Packages::PackageFileFinder.new(package, encoded_file_name).execute! - determine_content_type(package_file) track_package_event('pull_package', :generic, project: project, namespace: project.namespace) present_package_file!(package_file, content_disposition: :attachment) @@ -178,12 +177,6 @@ def encoded_file_name file_name = [declared_params[:path], declared_params[:file_name]].compact.join('/') declared_params[:path].present? ? URI.encode_uri_component(file_name) : file_name end - - def determine_content_type(package_file) - return unless Feature.enabled?(:packages_generic_package_content_type, package_file.project) - - ::Gitlab::Utils::MimeType.from_filename(package_file.file_name, log_enabled: true) - end end end end diff --git a/lib/gitlab/utils/mime_type.rb b/lib/gitlab/utils/mime_type.rb index 9a2ec61dfa64e4..645a05d03be128 100644 --- a/lib/gitlab/utils/mime_type.rb +++ b/lib/gitlab/utils/mime_type.rb @@ -21,20 +21,12 @@ def from_string(string) string.type end - def from_filename(filename, default: 'application/octet-stream', log_enabled: false) + def from_filename(filename, default: 'application/octet-stream') return default unless filename.is_a?(String) types = ::MIME::Types.type_for(filename) - content_type = types.any? ? types.first.content_type : default - if log_enabled - # Temporarily log the detected content types as JSON to build the allowed content type list - # When we remove this line, `log_enabled` needs to be removed as well - # https://gitlab.com/gitlab-org/gitlab/-/issues/444768 - ::Gitlab::AppJsonLogger.info(determined_content_type: content_type) - end - - content_type + types.any? ? types.first.content_type : default end end end diff --git a/spec/lib/gitlab/utils/mime_type_spec.rb b/spec/lib/gitlab/utils/mime_type_spec.rb index 3b7d600acb0953..46786a370ae75f 100644 --- a/spec/lib/gitlab/utils/mime_type_spec.rb +++ b/spec/lib/gitlab/utils/mime_type_spec.rb @@ -75,19 +75,15 @@ end context 'when default value is not given' do - where(:filename, :mime_type, :log_enabled, :log_example) do - 1 | 'application/octet-stream' | false | 'not log determination' - 'test.tf' | 'application/octet-stream' | false | 'not log determination' - 'test.css' | 'text/css' | false | 'not log determination' - 'test.js' | 'text/javascript' | false | 'not log determination' - 1 | 'application/octet-stream' | true | 'not log determination' - 'test.tf' | 'application/octet-stream' | true | 'log determination' - 'test.css' | 'text/css' | true | 'log determination' - 'test.js' | 'text/javascript' | true | 'log determination' + where(:filename, :mime_type, :log_example) do + 1 | 'application/octet-stream' | 'not log determination' + 'test.tf' | 'application/octet-stream' | 'not log determination' + 'test.css' | 'text/css' | 'not log determination' + 'test.js' | 'text/javascript' | 'not log determination' end with_them do - subject { described_class.from_filename(filename, log_enabled: log_enabled) } + subject { described_class.from_filename(filename) } it { is_expected.to eq(mime_type) } @@ -96,19 +92,15 @@ end context 'when default value is given' do - where(:filename, :default, :mime_type, :log_enabled, :log_example) do - 1 | nil | nil | false | 'not log determination' - 'test.tf' | nil | nil | false | 'not log determination' - 'test.tf' | 'text/plain' | 'text/plain' | false | 'not log determination' - 'test.css' | 'text/plain' | 'text/css' | false | 'not log determination' - 1 | nil | nil | true | 'not log determination' - 'test.tf' | nil | nil | true | 'log determination' - 'test.tf' | 'text/plain' | 'text/plain' | true | 'log determination' - 'test.css' | 'text/plain' | 'text/css' | true | 'log determination' + where(:filename, :default, :mime_type, :log_example) do + 1 | nil | nil | 'not log determination' + 'test.tf' | nil | nil | 'not log determination' + 'test.tf' | 'text/plain' | 'text/plain' | 'not log determination' + 'test.css' | 'text/plain' | 'text/css' | 'not log determination' end with_them do - subject { described_class.from_filename(filename, default: default, log_enabled: log_enabled) } + subject { described_class.from_filename(filename, default: default) } it { is_expected.to eq(mime_type) } diff --git a/spec/requests/api/generic_packages_spec.rb b/spec/requests/api/generic_packages_spec.rb index b45c6c8b1896fc..bc0dce73153f35 100644 --- a/spec/requests/api/generic_packages_spec.rb +++ b/spec/requests/api/generic_packages_spec.rb @@ -1121,28 +1121,6 @@ def upload_file( end end - shared_examples 'log content type determination' do - it 'logs content type determination' do - expect(Gitlab::AppJsonLogger).to receive(:info).with( - determined_content_type: 'application/gzip' - ) - - subject - end - - context 'when feature flag is disabled' do - before do - stub_feature_flags(packages_generic_package_content_type: false) - end - - it 'does not log content type determination' do - expect(Gitlab::AppJsonLogger).not_to receive(:info) - - subject - end - end - end - context 'when object storage is enabled' do let(:package_file) { create(:package_file, :generic, :object_storage, package: package) } @@ -1161,8 +1139,6 @@ def upload_file( stub_package_file_object_storage end - it_behaves_like 'log content type determination' - it 'includes response-content-disposition and filename in the redirect file URL' do download diff --git a/spec/requests/projects/packages/package_files_controller_spec.rb b/spec/requests/projects/packages/package_files_controller_spec.rb index 2f832c9e987a4f..a68866de8931c5 100644 --- a/spec/requests/projects/packages/package_files_controller_spec.rb +++ b/spec/requests/projects/packages/package_files_controller_spec.rb @@ -25,38 +25,6 @@ .to eq(%(attachment; filename="#{filename}"; filename*=UTF-8''#{filename})) end - it 'logs content type determination' do - expect(Gitlab::AppJsonLogger).to receive(:info).with( - determined_content_type: 'application/zip' - ) - - subject - end - - shared_examples 'not log' do - it 'does not enable log' do - expect(Gitlab::AppJsonLogger).not_to receive(:info) - - subject - end - end - - context 'when package type is not generic' do - before do - package.update!(package_type: 'npm') - end - - it_behaves_like 'not log' - end - - context 'when feature flag is disabled' do - before do - stub_feature_flags(packages_generic_package_content_type: false) - end - - it_behaves_like 'not log' - end - context 'with remote object storage' do let(:package_file) { create(:package_file, :object_storage, package: package, file_name: filename) } -- GitLab From 09ad27e8c96c95e4aa7c67a105c3ff3680836a3b Mon Sep 17 00:00:00 2001 From: Sylvia Shen Date: Mon, 28 Jul 2025 16:09:29 +0200 Subject: [PATCH 2/4] Remove packages_generic_package_content_type FF definition This removes the feature flag and related logging after data collection completion. Changelog: removed --- .../packages_generic_package_content_type.yml | 10 ---------- 1 file changed, 10 deletions(-) delete mode 100644 config/feature_flags/gitlab_com_derisk/packages_generic_package_content_type.yml diff --git a/config/feature_flags/gitlab_com_derisk/packages_generic_package_content_type.yml b/config/feature_flags/gitlab_com_derisk/packages_generic_package_content_type.yml deleted file mode 100644 index 8ebb9a583fc39d..00000000000000 --- a/config/feature_flags/gitlab_com_derisk/packages_generic_package_content_type.yml +++ /dev/null @@ -1,10 +0,0 @@ ---- -name: packages_generic_package_content_type -description: Return generic package file with corresponding content type -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/444768 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/196427 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/547424 -milestone: '18.2' -group: group::package registry -type: gitlab_com_derisk -default_enabled: false -- GitLab From d1967c448b0c7f398d34da748f1e242395f21b37 Mon Sep 17 00:00:00 2001 From: Sylvia Shen Date: Mon, 28 Jul 2025 16:48:13 +0200 Subject: [PATCH 3/4] Clean up tests --- spec/requests/api/generic_packages_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/requests/api/generic_packages_spec.rb b/spec/requests/api/generic_packages_spec.rb index bc0dce73153f35..307ddc9393bf99 100644 --- a/spec/requests/api/generic_packages_spec.rb +++ b/spec/requests/api/generic_packages_spec.rb @@ -1167,8 +1167,6 @@ def upload_file( stub_package_file_object_storage(proxy_download: true) end - it_behaves_like 'log content type determination' - it 'sends a file with response-content-disposition and filename' do expect(::Gitlab::Workhorse).to receive(:send_url) .with(instance_of(String), expected_headers) -- GitLab From d0deb15ace12fb109eaa5663ab8fe11c83942976 Mon Sep 17 00:00:00 2001 From: Sylvia Shen Date: Tue, 29 Jul 2025 10:30:16 +0200 Subject: [PATCH 4/4] Cleanup test cases --- spec/lib/gitlab/utils/mime_type_spec.rb | 42 ++++++------------------- 1 file changed, 10 insertions(+), 32 deletions(-) diff --git a/spec/lib/gitlab/utils/mime_type_spec.rb b/spec/lib/gitlab/utils/mime_type_spec.rb index 46786a370ae75f..4fdf7329b0f624 100644 --- a/spec/lib/gitlab/utils/mime_type_spec.rb +++ b/spec/lib/gitlab/utils/mime_type_spec.rb @@ -56,55 +56,33 @@ describe '.from_filename' do using RSpec::Parameterized::TableSyntax - shared_examples 'log determination' do - it 'logs the determination' do - expect(Gitlab::AppJsonLogger).to receive(:info).with( - determined_content_type: mime_type - ) - - subject - end - end - - shared_examples 'not log determination' do - it 'does not log the determination' do - expect(Gitlab::AppJsonLogger).not_to receive(:info) - - subject - end - end - context 'when default value is not given' do - where(:filename, :mime_type, :log_example) do - 1 | 'application/octet-stream' | 'not log determination' - 'test.tf' | 'application/octet-stream' | 'not log determination' - 'test.css' | 'text/css' | 'not log determination' - 'test.js' | 'text/javascript' | 'not log determination' + where(:filename, :mime_type) do + 1 | 'application/octet-stream' + 'test.tf' | 'application/octet-stream' + 'test.css' | 'text/css' + 'test.js' | 'text/javascript' end with_them do subject { described_class.from_filename(filename) } it { is_expected.to eq(mime_type) } - - it_behaves_like params[:log_example] end end context 'when default value is given' do - where(:filename, :default, :mime_type, :log_example) do - 1 | nil | nil | 'not log determination' - 'test.tf' | nil | nil | 'not log determination' - 'test.tf' | 'text/plain' | 'text/plain' | 'not log determination' - 'test.css' | 'text/plain' | 'text/css' | 'not log determination' + where(:filename, :default, :mime_type) do + 1 | nil | nil + 'test.tf' | nil | nil + 'test.tf' | 'text/plain' | 'text/plain' + 'test.css' | 'text/plain' | 'text/css' end with_them do subject { described_class.from_filename(filename, default: default) } it { is_expected.to eq(mime_type) } - - it_behaves_like params[:log_example] end end end -- GitLab