diff --git a/app/controllers/concerns/send_file_upload.rb b/app/controllers/concerns/send_file_upload.rb index 634255dff2757f8fce3e7bc20b9fba45979b7642..0266adcee4f3029c9e3c680c4a07bc45210a242b 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 eaaaf5ba54fd3b4d5e3d27df23d320bbcca4a4df..ad185ee9f217ccf9e6eae9046dcee3946637da27 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/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 8ebb9a583fc39dd067e9757122fe9556535a082c..0000000000000000000000000000000000000000 --- 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 diff --git a/lib/api/generic_packages.rb b/lib/api/generic_packages.rb index 309394eb1fe3752463f7f11bbe2607dddadf338a..f77750cdfb1a71868167584b4faec32edeae873e 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 9a2ec61dfa64e485ca7c2b651b7277fd97b38641..645a05d03be128abb909cbe646b233b25554a1eb 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 3b7d600acb0953b3f092b2ffc6547783ce4211cd..4fdf7329b0f62415e9068006d8c8b765c95f5693 100644 --- a/spec/lib/gitlab/utils/mime_type_spec.rb +++ b/spec/lib/gitlab/utils/mime_type_spec.rb @@ -56,63 +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_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) 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, log_enabled: log_enabled) } + 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_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) 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, log_enabled: log_enabled) } + subject { described_class.from_filename(filename, default: default) } it { is_expected.to eq(mime_type) } - - it_behaves_like params[:log_example] end end end diff --git a/spec/requests/api/generic_packages_spec.rb b/spec/requests/api/generic_packages_spec.rb index b45c6c8b1896fcc346e3cfc78c2b9b59dfd11b0a..307ddc9393bf991264df1d98e2207899f686fb1a 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 @@ -1191,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) diff --git a/spec/requests/projects/packages/package_files_controller_spec.rb b/spec/requests/projects/packages/package_files_controller_spec.rb index 2f832c9e987a4fbbfd03fc622cbf74ba0e7c80f4..a68866de8931c5f48af6dbef888d85a2fd9d3b6f 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) }