diff --git a/app/controllers/concerns/send_file_upload.rb b/app/controllers/concerns/send_file_upload.rb index 5e44679dbfa8ffdc21039afb90b8be0d4c8f3a9b..0266adcee4f3029c9e3c680c4a07bc45210a242b 100644 --- a/app/controllers/concerns/send_file_upload.rb +++ b/app/controllers/concerns/send_file_upload.rb @@ -44,17 +44,7 @@ def send_upload( def content_type_for(attachment) return '' unless attachment - guess_content_type(attachment) - end - - def guess_content_type(filename) - types = MIME::Types.type_for(filename) - - if types.present? - types.first.content_type - else - "application/octet-stream" - end + ::Gitlab::Utils::MimeType.from_filename(attachment) end private diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb index 43a09150b70ea3291e68bfda2e0995ca13db1857..75eca566b4e802f0370eb3b9ae3c517a98c37ea5 100644 --- a/app/controllers/projects/jobs_controller.rb +++ b/app/controllers/projects/jobs_controller.rb @@ -265,7 +265,7 @@ def build_path(build) end def raw_trace_content_disposition(raw_data) - mime_type = Gitlab::Utils::MimeType.from_string(raw_data) + mime_type = ::Gitlab::Utils::MimeType.from_string(raw_data) # if mime_type is nil can also represent 'text/plain' return 'inline' if mime_type.nil? || mime_type == 'text/plain' diff --git a/app/services/lfs/file_transformer.rb b/app/services/lfs/file_transformer.rb index ef227b6e20f732a43590c861d10372961615bc5b..477c17dcecea0d9a24b47f3a200c686da027a9d4 100644 --- a/app/services/lfs/file_transformer.rb +++ b/app/services/lfs/file_transformer.rb @@ -72,7 +72,7 @@ def cached_attributes # rubocop: disable CodeReuse/ActiveRecord def create_lfs_object!(lfs_pointer_file, file_content, detect_content_type) LfsObject.find_or_create_by(oid: lfs_pointer_file.sha256, size: lfs_pointer_file.size) do |lfs_object| - lfs_object.file = if detect_content_type && (content_type = Gitlab::Utils::MimeType.from_string(file_content)) + lfs_object.file = if detect_content_type && (content_type = ::Gitlab::Utils::MimeType.from_string(file_content)) CarrierWaveStringFile.new_file( file_content: file_content, filename: '', diff --git a/app/uploaders/content_type_whitelist.rb b/app/uploaders/content_type_whitelist.rb index 82c6b9b3a6114d08a7fb6befdbc4cc4c51dea5b0..4c9d180dab4966c23262f0f4feeacecd3b81ea80 100644 --- a/app/uploaders/content_type_whitelist.rb +++ b/app/uploaders/content_type_whitelist.rb @@ -43,7 +43,7 @@ def whitelisted_content_type?(content_type) def mime_magic_content_type(path) if path File.open(path) do |file| - Gitlab::Utils::MimeType.from_io(file) || 'invalid/invalid' + ::Gitlab::Utils::MimeType.from_io(file) || 'invalid/invalid' end end rescue Errno::ENOENT 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 new file mode 100644 index 0000000000000000000000000000000000000000..1e8eea0897acb66171de81466c74af55bbfad343 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/packages_generic_package_content_type.yml @@ -0,0 +1,10 @@ +--- +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/193331 +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 91071ec078175158729c709a24e60aa87e5b2bfd..ffe6232b7685a456726c821d2d84df4e1c8ff209 100644 --- a/lib/api/generic_packages.rb +++ b/lib/api/generic_packages.rb @@ -151,7 +151,8 @@ class GenericPackages < ::API::Base track_package_event('pull_package', :generic, project: project, namespace: project.namespace) - present_package_file!(package_file, content_disposition: :attachment) + content_type = detect_content_type(package_file, project) + present_package_file!(package_file, content_disposition: :attachment, content_type: content_type) end end end @@ -179,6 +180,18 @@ 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 detect_content_type(package_file, project) + return unless Feature.enabled?(:packages_generic_package_content_type, project) + + type = ::Gitlab::Utils::MimeType.from_filename(package_file.file_name) + + # Content-type `text/javascript` will trigger Rails' cross-origin JavaScript protection. + # Therefore we return `text/plain` for JavaScript files. + type = 'text/plain' if package_file.file.file_storage? && (File.extname(package_file.file_name) == '.js') + + type + end end end end diff --git a/lib/api/helpers/packages_helpers.rb b/lib/api/helpers/packages_helpers.rb index a7b2795253a56e33af659e03a1e7e39e0357fba6..ae212e5e0af9e4221a503e2d53f31dff4438d684 100644 --- a/lib/api/helpers/packages_helpers.rb +++ b/lib/api/helpers/packages_helpers.rb @@ -137,14 +137,18 @@ def track_package_event(action, scope, **args) end end - def present_package_file!(package_file, supports_direct_download: true, content_disposition: nil) + # rubocop: disable Layout/LineLength -- to avoid parameters on the next line + def present_package_file!(package_file, supports_direct_download: true, content_disposition: nil, content_type: 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, + content_type: content_type ) end + # rubocop: enable Layout/LineLength def protect_package!(package_name, package_type) service_response = diff --git a/lib/gitlab/sanitizers/exif.rb b/lib/gitlab/sanitizers/exif.rb index dee9e5f3d0e42f0997d2d628f67563d910171723..08fc9f90ac0ed9dd410ea5287051fdc8bcd048cc 100644 --- a/lib/gitlab/sanitizers/exif.rb +++ b/lib/gitlab/sanitizers/exif.rb @@ -169,7 +169,7 @@ def fetch_upload_to_file(uploader, dir) end def check_for_allowed_types(contents, raise_error: true) - mime_type = Gitlab::Utils::MimeType.from_string(contents) + mime_type = ::Gitlab::Utils::MimeType.from_string(contents) allowed = ALLOWED_MIME_TYPES.include?(mime_type) if !allowed && raise_error diff --git a/lib/gitlab/utils/mime_type.rb b/lib/gitlab/utils/mime_type.rb index d82092a2d2c12d21fcf7697ce5e12a509d9b9328..089be3fec7a838de4a6a1085007a7867625d69d9 100644 --- a/lib/gitlab/utils/mime_type.rb +++ b/lib/gitlab/utils/mime_type.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true require 'magic' +require 'mime/types' # This wraps calls to a gem which support mime type detection. # We use the `ruby-magic` gem instead of `mimemagic` due to licensing issues @@ -19,6 +20,16 @@ def from_string(string) string.type end + + def from_filename(filename, default: 'application/octet-stream') + return default unless filename.is_a?(String) + + types = ::MIME::Types.type_for(filename) + + return default unless types.present? + + types.first.content_type + end end end end diff --git a/spec/lib/gitlab/utils/mime_type_spec.rb b/spec/lib/gitlab/utils/mime_type_spec.rb index e8b369362243ccca0a0802ef3001dc8bb27489e3..a8473ea46bef944743e313965d03ce084ff79a7c 100644 --- a/spec/lib/gitlab/utils/mime_type_spec.rb +++ b/spec/lib/gitlab/utils/mime_type_spec.rb @@ -1,57 +1,89 @@ # frozen_string_literal: true -require "fast_spec_helper" -require "rspec/parameterized" +require 'fast_spec_helper' +require 'rspec/parameterized' RSpec.describe Gitlab::Utils::MimeType do - describe ".from_io" do + describe '.from_io' do subject { described_class.from_io(io) } - context "input isn't an IO" do - let(:io) { "test" } + context 'input is not an IO' do + let(:io) { 'test' } - it "returns nil" do + it 'returns nil' do expect(subject).to be_nil end end - context "input is a file" do + context 'input is a file' do using RSpec::Parameterized::TableSyntax where(:fixture, :mime_type) do - "banana_sample.gif" | "image/gif" - "rails_sample.jpg" | "image/jpeg" - "rails_sample.png" | "image/png" - "rails_sample.bmp" | "image/bmp" - "rails_sample.tif" | "image/tiff" - "sample.ico" | "image/vnd.microsoft.icon" - "sample_doc.md" | "text/plain" - "csv_empty.csv" | "application/x-empty" + 'banana_sample.gif' | 'image/gif' + 'rails_sample.jpg' | 'image/jpeg' + 'rails_sample.png' | 'image/png' + 'rails_sample.bmp' | 'image/bmp' + 'rails_sample.tif' | 'image/tiff' + 'sample.ico' | 'image/vnd.microsoft.icon' + 'sample_doc.md' | 'text/plain' + 'csv_empty.csv' | 'application/x-empty' end with_them do - let(:io) { File.open(File.join(__dir__, "../../../fixtures", fixture)) } + let(:io) { File.open(File.join(__dir__, '../../../fixtures', fixture)) } it { is_expected.to eq(mime_type) } end end end - describe ".from_string" do + describe '.from_string' do subject { described_class.from_string(str) } - context "input isn't a string" do + context 'input is not a string' do let(:str) { nil } - it "returns nil" do - expect(subject).to be_nil - end + it { is_expected.to be_nil } end - context "input is a string" do - let(:str) { "plain text" } + context 'input is a string' do + let(:str) { 'plain text' } it { is_expected.to eq('text/plain') } end end + + describe '.from_filename' do + using RSpec::Parameterized::TableSyntax + + context 'when default value is not given' do + 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) } + end + end + + context 'when default value is given' do + 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) } + end + end + end end diff --git a/spec/requests/api/generic_packages_spec.rb b/spec/requests/api/generic_packages_spec.rb index 87f9d251094d79867685903f78966a2e578e4d28..338c9854c061e39b1081a10787e2fc2ce3b5a4e4 100644 --- a/spec/requests/api/generic_packages_spec.rb +++ b/spec/requests/api/generic_packages_spec.rb @@ -1118,16 +1118,37 @@ def upload_file( "response-content-disposition=attachment%3B%20filename%3D%22#{package_file.file_name}" end + let(:content_type_param) { 'response-content-type=application%2Fgzip' } + before do stub_package_file_object_storage end - it 'includes response-content-disposition and filename in the redirect file URL' do + it 'includes response-content-disposition, response-content-type and filename in the redirect file URL' do download - expect(response.parsed_body).to include(disposition_param) + expect(response.parsed_body).to include(content_type_param, content_type_param) expect(response).to have_gitlab_http_status(:redirect) end + + context 'when feature :packages_generic_package_content_type is disabled' do + before do + stub_feature_flags(packages_generic_package_content_type: false) + end + + it 'does not include content type param' do + download + + expect(response.parsed_body).not_to include(content_type_param) + end + + it 'includes response-content-disposition and filename in the redirect file URL' do + download + + expect(response.parsed_body).to include(disposition_param) + expect(response).to have_gitlab_http_status(:redirect) + end + end end context 'when direct download is disabled' do @@ -1139,15 +1160,46 @@ def upload_file( stub_package_file_object_storage(proxy_download: true) end - it 'sends a file with response-content-disposition and filename' do + it 'sends a file with response-content-disposition, response-content-type and filename' do expect(::Gitlab::Workhorse).to receive(:send_url) - .with(instance_of(String), { response_headers: { 'Content-Disposition' => dispositon_header } }) - .and_call_original + .with(instance_of(String), { + response_headers: { + 'Content-Disposition' => dispositon_header, + 'Content-Type' => 'application/gzip' + } + } + ).and_call_original download expect(response).to have_gitlab_http_status(:ok) end + + context 'when feature :packages_generic_package_content_type is disabled' do + before do + stub_feature_flags(packages_generic_package_content_type: false) + end + + it 'does not send file with Content-Type' do + expect(::Gitlab::Workhorse).to receive(:send_url) + .with(instance_of(String), { response_headers: hash_not_including('Content-Type') }) + .and_call_original + + download + + expect(response).to have_gitlab_http_status(:ok) + 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 } }) + .and_call_original + + download + + expect(response).to have_gitlab_http_status(:ok) + end + end end end