From a5821e611ec64f3674710bc3b40277ae8d598687 Mon Sep 17 00:00:00 2001 From: MAEDA Go Date: Sun, 16 Nov 2025 21:32:08 +0900 Subject: [PATCH 4/4] Detect attachment content type from file contents instead of trusting client-provided values --- app/assets/javascripts/attachments.js | 1 - app/controllers/attachments_controller.rb | 1 - app/models/attachment.rb | 21 ++++++++----------- test/integration/api_test/attachments_test.rb | 2 +- test/integration/attachments_test.rb | 6 +++--- test/unit/attachment_test.rb | 15 ++++++++++--- 6 files changed, 25 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/attachments.js b/app/assets/javascripts/attachments.js index 0d823a63f..6979a3cb1 100644 --- a/app/assets/javascripts/attachments.js +++ b/app/assets/javascripts/attachments.js @@ -120,7 +120,6 @@ function uploadBlob(blob, uploadUrl, attachmentId, options) { uploadUrl = uploadUrl + '?attachment_id=' + attachmentId; if (blob instanceof window.Blob) { uploadUrl += '&filename=' + encodeURIComponent(blob.name); - uploadUrl += '&content_type=' + encodeURIComponent(blob.type); } return $.ajax(uploadUrl, { diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index 90c3c7070..c6df87197 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -108,7 +108,6 @@ class AttachmentsController < ApplicationController @attachment = Attachment.new(:file => raw_request_body) @attachment.author = User.current @attachment.filename = params[:filename].presence || Redmine::Utils.random_hex(16) - @attachment.content_type = params[:content_type].presence saved = @attachment.save respond_to do |format| diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 07348cd61..168043c23 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -83,7 +83,7 @@ class Attachment < ApplicationRecord cattr_accessor :thumbnails_storage_path @@thumbnails_storage_path = File.join(Rails.root, "tmp", "thumbnails") - before_create :files_to_final_location + before_create :set_content_type, :files_to_final_location after_commit :delete_from_disk, :on => :destroy after_commit :reuse_existing_file_if_possible, :on => :create after_rollback :delete_from_disk, :on => :create @@ -118,9 +118,6 @@ class Attachment < ApplicationRecord self.filename = @temp_file.original_filename self.filename.force_encoding("UTF-8") end - if @temp_file.respond_to?(:content_type) - self.content_type = @temp_file.content_type.to_s.chomp - end self.filesize = @temp_file.size end end @@ -134,6 +131,14 @@ class Attachment < ApplicationRecord filename end + def set_content_type + return unless @temp_file + + io = @temp_file.respond_to?(:read) ? @temp_file : StringIO.new(@temp_file) + self.content_type = Marcel::MimeType.for(io, name: filename) + @temp_file.rewind if @temp_file.respond_to?(:rewind) + end + # Copies the temporary file to its final location # and computes its hash def files_to_final_location @@ -157,14 +162,6 @@ class Attachment < ApplicationRecord self.digest = sha.hexdigest end @temp_file = nil - - if content_type.blank? && filename.present? - self.content_type = Redmine::MimeType.of(filename) - end - # Don't save the content type if it's longer than the authorized length - if self.content_type && self.content_type.length > 255 - self.content_type = nil - end end # Deletes the file from the file system if it's not referenced by other attachments diff --git a/test/integration/api_test/attachments_test.rb b/test/integration/api_test/attachments_test.rb index 5b0e8315b..caf327d6a 100644 --- a/test/integration/api_test/attachments_test.rb +++ b/test/integration/api_test/attachments_test.rb @@ -142,7 +142,7 @@ class Redmine::ApiTest::AttachmentsTest < Redmine::ApiTest::Base assert_nil attachment.container assert_equal 2, attachment.author_id assert_equal 'File content'.size, attachment.filesize - assert attachment.content_type.blank? + assert_equal 'application/octet-stream', attachment.content_type assert attachment.filename.present? assert_match %r{\d+_[0-9a-z]+}, attachment.diskfile assert File.exist?(attachment.diskfile) diff --git a/test/integration/attachments_test.rb b/test/integration/attachments_test.rb index 30162cb12..53d4bafb8 100644 --- a/test/integration/attachments_test.rb +++ b/test/integration/attachments_test.rb @@ -33,12 +33,12 @@ class AttachmentsTest < Redmine::IntegrationTest assert_equal 'text/plain', attachment.content_type end - def test_upload_should_accept_content_type_param + def test_upload_should_detect_file_content_type log_user('jsmith', 'jsmith') assert_difference 'Attachment.count' do post( - "/uploads.js?attachment_id=1&filename=foo&content_type=image/jpeg", - :params => "File content", + "/uploads.js?attachment_id=1&filename=foo", + :params => "\xFF\xD8\xFF\xD9", # JPEG file signature :headers => {"CONTENT_TYPE" => 'application/octet-stream'}) assert_response :success end diff --git a/test/unit/attachment_test.rb b/test/unit/attachment_test.rb index 7bfa9311f..1571713d3 100644 --- a/test/unit/attachment_test.rb +++ b/test/unit/attachment_test.rb @@ -54,14 +54,23 @@ class AttachmentTest < ActiveSupport::TestCase assert_equal 59, File.size(a.diskfile) end - def test_create_should_clear_content_type_if_too_long + def test_create_should_not_trust_given_content_type_attribute a = Attachment.new(:container => Issue.find(1), - :file => uploaded_test_file("testfile.txt", "text/plain"), + :file => uploaded_test_file("testfile.txt", nil), :author => User.find(1), :content_type => 'a'*300) assert a.save a.reload - assert_nil a.content_type + assert_equal 'text/plain', a.content_type + end + + def test_create_should_not_trust_client_declared_content_type + a = Attachment.new(:container => Issue.find(1), + :file => uploaded_test_file("testfile.txt", "image/jpeg"), + :author => User.find(1)) + assert a.save + a.reload + assert_equal 'text/plain', a.content_type end def test_shorted_filename_if_too_long -- 2.50.1