Active Storage: incorrect defaults

https://github.com/rails/rails/pull/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 @@ class ActiveStorage::Blob < ActiveStorage::Record
     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 @@ module ActiveStorage
         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)
 

guides/source/configuring.md

@@ -1545,7 +1545,7 @@ can transform through ImageMagick.
 By default, this is defined as:
 
 ```ruby
-config.active_storage.variable_content_types = %w(image/png image/gif image/jpg image/jpeg image/pjpeg image/tiff image/bmp image/vnd.adobe.photoshop image/vnd.microsoft.icon image/webp image/avif image/heic image/heif)
+config.active_storage.variable_content_types = %w(image/png image/gif image/jpeg image/tiff image/vnd.adobe.photoshop image/vnd.microsoft.icon image/webp image/avif image/heic image/heif)
 ```
 
 #### `config.active_storage.web_image_content_types`
@@ -1557,7 +1557,7 @@ If you want to use `WebP` or `AVIF` variants in your application you can add
 By default, this is defined as:
 
 ```ruby
-config.active_storage.web_image_content_types = %w(image/png image/jpeg image/jpg image/gif)
+config.active_storage.web_image_content_types = %w(image/png image/jpeg image/gif)
 ```
 
 #### `config.active_storage.content_types_to_serve_as_binary`
@@ -1566,7 +1566,7 @@ Accepts an array of strings indicating the content types that Active Storage wil
 By default, this is defined as:
 
 ```ruby
-config.active_storage.content_types_to_serve_as_binary = %w(text/html text/javascript image/svg+xml application/postscript application/x-shockwave-flash text/xml application/xml application/xhtml+xml application/mathml+xml text/cache-manifest)
+config.active_storage.content_types_to_serve_as_binary = %w(text/html image/svg+xml application/postscript application/x-shockwave-flash text/xml application/xml application/xhtml+xml application/mathml+xml text/cache-manifest)
 ```
 
 #### `config.active_storage.content_types_allowed_inline`
@@ -1575,7 +1575,15 @@ Accepts an array of strings indicating the content types that Active Storage all
 By default, this is defined as:
 
 ```ruby
-config.active_storage.content_types_allowed_inline` = %w(image/png image/gif image/jpg image/jpeg image/tiff image/bmp image/vnd.adobe.photoshop image/vnd.microsoft.icon application/pdf)
+config.active_storage.content_types_allowed_inline` = %w(image/png image/gif image/jpeg image/tiff image/vnd.adobe.photoshop image/vnd.microsoft.icon application/pdf)
+```
+
+#### `config.active_storage.silence_invalid_content_types_warning`
+
+Since Rails 7, Active Storage will warn if you use an invalid content type that was incorrectly supported in Rails 6. You can use this config to turn the warning off.
+
+```ruby
+config.active_storage.silence_invalid_content_types_warning = false
 ```
 
 #### `config.active_storage.queues.analysis`