From c44e63adcf4e30fe8f73a6b108e00fa121e5eba9 Mon Sep 17 00:00:00 2001 From: MAEDA Go Date: Sun, 16 Nov 2025 21:32:08 +0900 Subject: [PATCH 5/6] Detect attachment content type from file contents instead of trusting client-provided values --- app/assets/javascripts/attachments.js | 1 - app/controllers/attachments_controller.rb | 3 +- app/models/attachment.rb | 45 +++++++++----- app/models/mail_handler.rb | 3 +- .../lib/acts_as_attachable.rb | 1 - 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 | 58 ++++++++++++++++++- 9 files changed, 96 insertions(+), 29 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 b2619aa34..ff436f90c 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -112,7 +112,8 @@ 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 + # The content type is detected from the file contents on save, + # so params[:content_type] is ignored saved = @attachment.save respond_to do |format| diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 4d9974dce..6b548fb78 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -90,12 +90,13 @@ class Attachment < ApplicationRecord File.join(storage_path, 'derived_cache', 'markdownized_previews') end - before_create :files_to_final_location + before_create :set_content_type, :files_to_final_location + before_update :reset_content_type, :if => :filename_changed? 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) @@ -125,9 +126,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 @@ -141,6 +139,35 @@ 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) + if io.respond_to?(:rewind) + io.rewind + else + # Marcel rewinds the io while reading it, which a plain reader may not + # support; detect the type from the filename alone in that case + io = nil + end + # The filename is passed as a hint because many formats, plain text and + # Markdown among them, cannot be identified from their contents alone. + # Without it they would all be detected as application/octet-stream. + self.content_type = Marcel::MimeType.for(io, name: filename) + @temp_file.rewind if @temp_file.respond_to?(:rewind) + end + + # Re-detects the content type from the file on disk. Needed after a rename + # because the filename is used as a hint for contents that cannot be + # identified on their own. + def reset_content_type + return unless readable? + + File.open(diskfile, 'rb') do |io| + self.content_type = Marcel::MimeType.for(io, name: filename) + end + end + # Copies the temporary file to its final location # and computes its hash def files_to_final_location @@ -164,14 +191,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/app/models/mail_handler.rb b/app/models/mail_handler.rb index b4e1c52c8..13257f1d4 100644 --- a/app/models/mail_handler.rb +++ b/app/models/mail_handler.rb @@ -352,8 +352,7 @@ class MailHandler < ActionMailer::Base obj.attachments << Attachment.create(:container => obj, :file => attachment.body.decoded, :filename => attachment.filename, - :author => user, - :content_type => attachment.mime_type) + :author => user) end end end 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..97e172eb6 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,6 @@ module Redmine next end a.filename = attachment['filename'] unless attachment['filename'].blank? - a.content_type = attachment['content_type'] unless attachment['content_type'].blank? 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 737ecf15f..999cf5cdd 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 a7f456a35..ac8b8dd61 100644 --- a/test/unit/attachment_test.rb +++ b/test/unit/attachment_test.rb @@ -59,14 +59,66 @@ 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_client_declared_content_type + # PDF file uploaded with a wrong content type "text/plain" + # and a long bogus content_type attribute a = Attachment.new(:container => Issue.find(1), - :file => uploaded_test_file("testfile.txt", "text/plain"), + :file => uploaded_test_file("hello.pdf", "text/plain"), :author => User.find(1), :content_type => 'a'*300) assert a.save a.reload - assert_nil a.content_type + assert_equal 'application/pdf', a.content_type + end + + def test_create_should_accept_a_file_that_cannot_be_rewound + # A minimal reader like the ones plugins may pass to Attachment#file= + reader = Class.new do + def initialize(content) + @content = content + end + + def size + @content.bytesize + end + + def read(*args) + if @eof + false + else + @eof = true + @content + end + end + end.new("hello world\n") + + a = Attachment.new(:file => reader, :filename => 'note.txt', :author_id => 1) + assert a.save + # The contents cannot be inspected without consuming the reader, + # so the content type is detected from the filename + assert_equal 'text/plain', a.content_type + assert_equal "hello world\n", File.read(a.diskfile) + end + + def test_rename_should_recalculate_the_content_type + a = Attachment.create!( + :file => mock_file_with_options(:original_filename => 'test.bin', :content => "\x00\x01\x02".b), + :author_id => 1 + ) + assert_equal 'application/octet-stream', a.content_type + + assert a.update(:filename => 'renamed.txt') + assert_equal 'text/plain', a.reload.content_type + end + + def test_rename_should_not_fail_when_the_file_is_missing + a = Attachment.create!( + :file => mock_file_with_options(:original_filename => 'test.bin', :content => "\x00\x01\x02".b), + :author_id => 1 + ) + File.delete(a.diskfile) + + assert a.update(:filename => 'renamed.txt') end def test_shorted_filename_if_too_long -- 2.50.1