From 0fa491188456332bacddb6310d86014cd8c87688 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Mon, 3 Aug 2026 04:13:34 +0200 Subject: Believe the file over the browser about what was uploaded --- app/models/concerns/file_attachment.rb | 54 ++++++++++++++++++++++++++++++---- config/locales/de.yml | 2 ++ config/locales/en.yml | 2 ++ test/models/asset_test.rb | 23 +++++++++++++++ 4 files changed, 76 insertions(+), 5 deletions(-) diff --git a/app/models/concerns/file_attachment.rb b/app/models/concerns/file_attachment.rb index fd0cabed..38b0b706 100644 --- a/app/models/concerns/file_attachment.rb +++ b/app/models/concerns/file_attachment.rb @@ -37,6 +37,20 @@ module FileAttachment "MAGICK_CONFIGURE_PATH" => Rails.root.join("config", "imagemagick").to_s }.freeze + # ImageMagick picks its decoder from the file, not from the multipart + # header. Where the two disagree, believe ImageMagick: otherwise + # generate_all_variants dispatches on a claim and hands a file to a + # pipeline built for something else. + MAGICK_FORMAT_CONTENT_TYPES = { + "JPEG" => "image/jpeg", + "PNG" => "image/png", + "GIF" => "image/gif", + "WEBP" => "image/webp", + "SVG" => "image/svg+xml", + "MSVG" => "image/svg+xml", + "PDF" => "application/pdf" + }.freeze + IMAGE_CONTENT_TYPES = %w[image/jpeg image/gif image/png image/webp].freeze VECTOR_CONTENT_TYPES = %w[image/svg+xml].freeze RASTERIZED_CONTENT_TYPES = %w[application/pdf].freeze @@ -88,6 +102,7 @@ module FileAttachment after_initialize :build_upload_proxy after_save :process_upload before_destroy :delete_upload_files + validate :upload_must_be_readable_when_it_claims_to_be_an_image end def upload=(uploaded_file) @@ -95,7 +110,10 @@ module FileAttachment @pending_upload = uploaded_file # Populate the database columns immediately so validations can use them self.upload_file_name = sanitize_filename(uploaded_file.original_filename) - self.upload_content_type = uploaded_file.content_type.to_s.split(';').first.strip + detected = detected_content_type(uploaded_file) + @upload_unreadable = detected.nil? + self.upload_content_type = detected || + uploaded_file.content_type.to_s.split(';').first.strip self.upload_file_size = uploaded_file.size self.upload_updated_at = Time.current build_upload_proxy @@ -160,7 +178,9 @@ module FileAttachment STYLES.each do |style, options| dest_path = file_path(style) FileUtils.mkdir_p(File.dirname(dest_path)) - system(MAGICK_ENV, "magick", original_path, *extra_args, *options[:args], dest_path) + if !system(MAGICK_ENV, "magick", original_path, *extra_args, *options[:args], dest_path) + Rails.logger.warn("Asset##{id}: magick failed for #{style} of #{upload_file_name}") + end end end @@ -180,7 +200,7 @@ module FileAttachment dest_path = file_path(:og) FileUtils.mkdir_p(File.dirname(dest_path)) - if og_full_bleed?(original_path) + ok = if og_full_bleed?(original_path) system(MAGICK_ENV, "magick", "#{original_path}[0]", "-resize", "#{OG_WIDTH}x#{OG_HEIGHT}^", "-gravity", "center", "-extent", "#{OG_WIDTH}x#{OG_HEIGHT}", @@ -189,6 +209,9 @@ module FileAttachment else system(MAGICK_ENV, *og_template_command(dest_path)) end + + Rails.logger.warn("Asset##{id}: magick failed for og of #{upload_file_name}") unless ok + ok end def generate_all_variants(original_path) @@ -214,8 +237,11 @@ module FileAttachment end def source_dimensions(path) - out, status = Open3.capture2(MAGICK_ENV, "magick", "identify", "-format", "%w %h", "#{path}[0]") - return nil unless status.success? + out, err, status = Open3.capture3(MAGICK_ENV, "magick", "identify", "-format", "%w %h", "#{path}[0]") + unless status.success? + Rails.logger.warn("Asset##{id}: magick identify failed for #{path}: #{err.lines.first&.strip}") + return nil + end width, height = out.split.map(&:to_i) (width.positive? && height.positive?) ? [width, height] : nil @@ -288,12 +314,30 @@ module FileAttachment end end + def detected_content_type(uploaded_file) + path = uploaded_file.try(:tempfile).try(:path) || uploaded_file.try(:path) + return nil if path.blank? || !File.exist?(path) + + out, _err, status = Open3.capture3(MAGICK_ENV, "magick", "identify", "-format", "%m", "#{path}[0]") + return nil unless status.success? + + MAGICK_FORMAT_CONTENT_TYPES[out.strip.upcase] + rescue StandardError + nil + end def delete_upload_files dir = upload_root.join(id.to_s) FileUtils.rm_rf(dir) if Dir.exist?(dir) end + def upload_must_be_readable_when_it_claims_to_be_an_image + return unless @upload_unreadable + return unless IMAGE_CONTENT_TYPES.include?(upload_content_type) + + errors.add(:upload, :unreadable_image) + end + def file_path(style) upload_root.join(id.to_s, style.to_s, variant_filename(style)).to_s end diff --git a/config/locales/de.yml b/config/locales/de.yml index 027fcd44..b3319e61 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -159,6 +159,8 @@ de: attributes: base: not_permitted: "Dieses Asset ist an geschützte Seiten angehängt; nur die Redaktion darf es löschen" + upload: + unreadable_image: "konnte nicht als Bild gelesen werden. Möglicherweise ist sie beschädigt oder hat ein anderes Format, als der Name vermuten lässt." tags: index: diff --git a/config/locales/en.yml b/config/locales/en.yml index 256c8540..ccca0ad0 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -105,6 +105,8 @@ en: attributes: base: not_permitted: "Only Redaktion members may change assets related to nodes in this section" + upload: + unreadable_image: "could not be read as an image. It may be damaged, or not the format its name suggests." tags: index: diff --git a/test/models/asset_test.rb b/test/models/asset_test.rb index ab1cc5df..2677681b 100644 --- a/test/models/asset_test.rb +++ b/test/models/asset_test.rb @@ -1,4 +1,5 @@ require 'test_helper' +require "rack/test/uploaded_file" class AssetTest < ActiveSupport::TestCase @@ -53,4 +54,26 @@ class AssetTest < ActiveSupport::TestCase assert asset.has_credit? assert_not asset.show_credit? end + + test "an upload that claims to be an image but is not is refused" do + Tempfile.create(["fake", ".jpg"]) do |f| + f.write("this is not a jpeg") + f.flush + + asset = Asset.new(:name => "fake") + asset.upload = Rack::Test::UploadedFile.new(f.path, "image/jpeg") + + assert_not asset.valid? + assert_includes asset.errors.full_messages.to_sentence, + I18n.t("activerecord.errors.models.asset.attributes.upload.unreadable_image") + end + end + + test "a real image is accepted and its type comes from the file" do + asset = Asset.new(:name => "real") + asset.upload = Rack::Test::UploadedFile.new(file_fixture("test_image.png"), "image/jpeg") + + assert asset.valid?, asset.errors.full_messages.to_sentence + assert_equal "image/png", asset.upload_content_type + end end -- cgit v1.3