Active Storage: incorrect defaults

rails/rails#42225 identified that some of the content types used as defaults by Active Storage aren't recognized by `mini_mime`. This means that in practice code like [this](https://github.com/rails/rails/pull/42225/files#diff-7a3ec24c556b138abdbd67066ab5125b73528e45891d83142e417d3944194128R116) will crash or not function correctly. In [this](https://github.com/rails/rails/pull/42225/files#diff-c2010824d2d2e8d841ff4fc058c264c12d870e893025b153e6de571fba6b6c6cR194) example, a file with content_type `image/jpg` is treated as a PNG by the representer, since `image/jpg` isn't a valid content type according to `mini_mime`.

I don't think the default content_types should include formats that have never actually worked, so I'm proposing we remove them from the defaults.

activestorage/CHANGELOG.md

@@ -1,3 +1,14 @@
+*   Invalid default content types are deprecated
+
+    Blobs created with content_type `image/jpg`, `image/pjpeg`, `image/bmp`, `text/javascript` will now produce
+    a deprecation warning, since these are not valid content types.
+
+    These content types will be removed from the defaults in Rails 7.1.
+
+    You can set `config.active_storage.silence_invalid_content_types_warning = true` to dismiss the warning.
+
+    *Alex Ghiculescu*
+
 ## Rails 7.0.0.alpha2 (September 15, 2021) ##
 
 *   No changes.

activestorage/app/models/active_storage/blob.rb

@@ -308,6 +308,31 @@ def service
     services.fetch(service_name)
   end
 
+  def content_type=(value)
+    unless ActiveStorage.silence_invalid_content_types_warning
+      if INVALID_VARIABLE_CONTENT_TYPES_DEPRECATED_IN_RAILS_7.include?(value)
+        ActiveSupport::Deprecation.warn(<<-MSG.squish)
+          #{value} is not a valid content type, it should not be used when creating a blob, and support for it will be removed in Rails 7.1.
+          If you want to keep supporting this content type past Rails 7.1, add it to `config.active_storage.variable_content_types`.
+          Dismiss this warning by setting `config.active_storage.silence_invalid_content_types_warning = true`.
+        MSG
+      end
+
+      if INVALID_VARIABLE_CONTENT_TYPES_TO_SERVE_AS_BINARY_DEPRECATED_IN_RAILS_7.include?(value)
+        ActiveSupport::Deprecation.warn(<<-MSG.squish)
+          #{value} is not a valid content type, it should not be used when creating a blob, and support for it will be removed in Rails 7.1.
+          If you want to keep supporting this content type past Rails 7.1, add it to `config.active_storage.content_types_to_serve_as_binary`.
+          Dismiss this warning by setting `config.active_storage.silence_invalid_content_types_warning = true`.
+        MSG
+      end
+    end
+
+    super
+  end
+
+  INVALID_VARIABLE_CONTENT_TYPES_DEPRECATED_IN_RAILS_7 = ["image/jpg", "image/pjpeg", "image/bmp"]
+  INVALID_VARIABLE_CONTENT_TYPES_TO_SERVE_AS_BINARY_DEPRECATED_IN_RAILS_7 = ["text/javascript"]
+
   private
     def compute_checksum_in_chunks(io)
       OpenSSL::Digest::MD5.new.tap do |checksum|

activestorage/lib/active_storage.rb

@@ -71,6 +71,8 @@ module ActiveStorage
 
   mattr_accessor :video_preview_arguments, default: "-y -vframes 1 -f image2"
 
+  mattr_accessor :silence_invalid_content_types_warning, default: false
+
   module Transformers
     extend ActiveSupport::Autoload
 

activestorage/lib/active_storage/engine.rb

@@ -101,6 +101,8 @@ class Engine < Rails::Engine # :nodoc:
         ActiveStorage.binary_content_type = app.config.active_storage.binary_content_type || "application/octet-stream"
         ActiveStorage.video_preview_arguments = app.config.active_storage.video_preview_arguments || "-y -vframes 1 -f image2"
 
+        ActiveStorage.silence_invalid_content_types_warning = app.config.active_storage.silence_invalid_content_types_warning || false
+
         ActiveStorage.replace_on_assign_to_many = app.config.active_storage.replace_on_assign_to_many || false
         ActiveStorage.track_variants = app.config.active_storage.track_variants || false
       end

activestorage/test/engine_test.rb

@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "database/setup"
+
+class ActiveStorage::EngineTest < ActiveSupport::TestCase
+  test "all default content types are recognized by mini_mime" do
+    exceptions = ActiveStorage::Blob::INVALID_VARIABLE_CONTENT_TYPES_DEPRECATED_IN_RAILS_7 + ActiveStorage::Blob::INVALID_VARIABLE_CONTENT_TYPES_TO_SERVE_AS_BINARY_DEPRECATED_IN_RAILS_7
+
+    ActiveStorage.variable_content_types.each do |content_type|
+      next if exceptions.include?(content_type) # remove this line in Rails 7.1
+
+      assert_equal content_type, MiniMime.lookup_by_content_type(content_type).content_type
+    end
+
+    ActiveStorage.web_image_content_types.each do |content_type|
+      next if exceptions.include?(content_type) # remove this line in Rails 7.1
+
+      assert_equal content_type, MiniMime.lookup_by_content_type(content_type).content_type
+    end
+
+    ActiveStorage.content_types_to_serve_as_binary.each do |content_type|
+      next if exceptions.include?(content_type) # remove this line in Rails 7.1
+
+      assert_equal content_type, MiniMime.lookup_by_content_type(content_type).content_type
+    end
+
+    ActiveStorage.content_types_allowed_inline.each do |content_type|
+      next if exceptions.include?(content_type) # remove this line in Rails 7.1
+
+      assert_equal content_type, MiniMime.lookup_by_content_type(content_type).content_type
+    end
+  end
+end

activestorage/test/models/blob_test.rb

@@ -293,6 +293,32 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
+  test "warning if blob is created with invalid mime type" do
+    assert_deprecated do
+      create_blob(filename: "funky.jpg", content_type: "image/jpg")
+    end
+
+    assert_not_deprecated do
+      create_blob(filename: "funky.jpg", content_type: "image/jpeg")
+    end
+  end
+
+  test "warning if blob is created with invalid mime type can be disabled" do
+    warning_was = ActiveStorage.silence_invalid_content_types_warning
+    ActiveStorage.silence_invalid_content_types_warning = true
+
+    assert_not_deprecated do
+      create_blob(filename: "funky.jpg", content_type: "image/jpg")
+    end
+
+    assert_not_deprecated do
+      create_blob(filename: "funky.jpg", content_type: "image/jpeg")
+    end
+
+  ensure
+    ActiveStorage.silence_invalid_content_types_warning = warning_was
+  end
+
   private
     def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil, service_name: :local)
       filename ||= blob.filename

activestorage/test/models/variant_test.rb

@@ -104,7 +104,7 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
   end
 
   test "resized variation of BMP blob" do
-    blob = create_file_blob(filename: "colors.bmp", content_type: "image/bmp")
+    blob = create_file_blob(filename: "colors.bmp", content_type: "image/x-bmp")
     variant = blob.variant(resize_to_limit: [15, 15]).processed
     assert_match(/colors\.png/, variant.url)