From e0f91d13f3f0604bc40eb552dc709b17d6d7afd9 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 | 23 ++++++-------- .../lib/acts_as_attachable.rb | 7 ++++- test/integration/api_test/attachments_test.rb | 2 +- test/integration/api_test/files_test.rb | 4 +-- test/integration/attachments_test.rb | 8 ++--- test/unit/attachment_test.rb | 31 +++++++++++++++++-- 8 files changed, 50 insertions(+), 27 deletions(-) diff --git a/app/assets/javascripts/attachments.js b/app/assets/javascripts/attachments.js index 1650386b8..edaa6a615 100644 --- a/app/assets/javascripts/attachments.js +++ b/app/assets/javascripts/attachments.js @@ -122,7 +122,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 18795891d..179f5221f 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -110,7 +110,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 d0dd3a71f..c8f62c4ac 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -83,12 +83,12 @@ 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 - safe_attributes 'filename', 'content_type', 'description' + safe_attributes 'filename', 'description' # Returns an unsaved copy of the attachment def copy(attributes=nil) @@ -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/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb b/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb index f068d80b3..b78f5a679 100644 --- a/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb +++ b/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb @@ -127,7 +127,12 @@ module Redmine next end a.filename = attachment['filename'] unless attachment['filename'].blank? - a.content_type = attachment['content_type'] unless attachment['content_type'].blank? + # Re-evaluate MIME type after rename. Marcel uses the filename as a hint + if a.filename_changed? + File.open(a.diskfile, 'rb') do |io| + a.content_type = Marcel::MimeType.for(io, name: a.filename) + end + end end next unless a a.description = attachment['description'].to_s.strip 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/api_test/files_test.rb b/test/integration/api_test/files_test.rb index 10361aab3..27e015af6 100644 --- a/test/integration/api_test/files_test.rb +++ b/test/integration/api_test/files_test.rb @@ -83,7 +83,7 @@ class Redmine::ApiTest::FilesTest < Redmine::ApiTest::Base assert_response :bad_request end - test "POST /projects/:project_id/files.json should accept :filename, :description, :content_type as optional parameters" do + test "POST /projects/:project_id/files.json should accept :filename, :description as optional parameters" do set_tmp_attachments_directory post( '/uploads.xml', @@ -94,7 +94,6 @@ class Redmine::ApiTest::FilesTest < Redmine::ApiTest::Base { "file": { "filename": "New filename", "description": "New description", - "content_type": "application/txt", "token": "#{token}" } } @@ -106,7 +105,6 @@ class Redmine::ApiTest::FilesTest < Redmine::ApiTest::Base assert_response :success assert_equal "New filename", Attachment.last.filename assert_equal "New description", Attachment.last.description - assert_equal "application/txt", Attachment.last.content_type end test "POST /projects/:project_id/files.json should accept :version_id to attach the files to a version" do diff --git a/test/integration/attachments_test.rb b/test/integration/attachments_test.rb index 30162cb12..5bb69e9bf 100644 --- a/test/integration/attachments_test.rb +++ b/test/integration/attachments_test.rb @@ -33,17 +33,17 @@ 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 => "\xCA\xFE\xBA\xBE", # Java class file magic bytes :headers => {"CONTENT_TYPE" => 'application/octet-stream'}) assert_response :success end attachment = Attachment.order(:id => :desc).first - assert_equal 'image/jpeg', attachment.content_type + assert_equal 'application/java-vm', attachment.content_type end def test_upload_as_js_and_attach_to_an_issue diff --git a/test/unit/attachment_test.rb b/test/unit/attachment_test.rb index 7bfa9311f..8716646a1 100644 --- a/test/unit/attachment_test.rb +++ b/test/unit/attachment_test.rb @@ -54,14 +54,25 @@ 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 + # Text file upload with a long bogus content type 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 + # PDF file uploaded with a wrong content type "text/plain" + a = Attachment.new(:container => Issue.find(1), + :file => uploaded_test_file("hello.pdf", "text/plain"), + :author => User.find(1)) + assert a.save + a.reload + assert_equal 'application/pdf', a.content_type end def test_shorted_filename_if_too_long @@ -413,6 +424,20 @@ class AttachmentTest < ActiveSupport::TestCase assert_equal 'text/plain', attachment.content_type end + test "Attachment.attach_files should recalculate the content_type when filename changes" do + @project = Project.find(1) + attachment = Attachment.create!( + :file => mock_file_with_options(:original_filename => "test.bin", :content => "\x00\x01\x02".b), + :author_id => 1, + :created_on => 2.days.ago + ) + assert_equal 'application/octet-stream', attachment.content_type + + Attachment.attach_files(@project, {'1' => {'token' => attachment.token, 'filename' => 'renamed.txt'}}) + attachment.reload + assert_equal 'text/plain', attachment.content_type + end + def test_update_digest_to_sha256_should_update_digest set_fixtures_attachments_directory attachment = Attachment.find 6 -- 2.50.1