Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveStorage accepts broken images and throws exceptions when creating variants #91

Closed
Signum opened this issue Nov 18, 2020 · 2 comments
Labels
help wanted Extra attention is needed

Comments

@Signum
Copy link

Signum commented Nov 18, 2020

TL;DR There is no way to detect broken image files. So exceptions are thrown later when ActiveStorage tries to render variants.

I originally submitted this issue to rails/rails#40493 but was directed here.

Steps to reproduce

Create a controller action that receives uploaded image files. A part from my Rails application:

def upload_receive
  params[:file].each do |img|
    new_screenshot = @package.screenshots.new(image: img)
    
    if new_screenshot.valid?  
      new_screenshot.save
    end
  end
end

My model:

class Screenshot < ApplicationRecord
  has_one_attached :image
  validates :image, attached: true, content_type: [:png]

  def medium_image
    if self.image.attached?
      self.image.variant(
        resize_to_limit: [670, 600]
      ).processed
    end
  end
end

(The validation uses the 'active_storage_validations' gem but the problem is most certainly not coming from there.)

This works for intact PNG files. A deliberately broken PNG file will be accepted and saved though. My test file is 5 KB of zero bytes that I named "broken.png" and uploaded to Rails.

Expected behavior

One of…

  1. ActiveStorage detect broken image files by trying to open them with Minimagick/Imagemagick as a validation.
  2. ActiveStorage offers a common validation to detect broken images. (I tried to write my own custom validator to run the file through Minimagick but it I could not find a way to access the blob's data in the model before a ".save".)
  3. Creating variants does not just throw an exception.

Actual behavior

The file gets saved by ActiveStorage with content_type='image/png'. I suspect that 'marcel' and 'mimemagick' (which I believe are used for the content type detection) can't figure out what this file is and just look at the '.png' suffix.

Once a template tries to render a variant using the Screenshot.medium_image method it will call Minimagick and fail:

convert /tmp/ActiveStorage-21670-… failed with error:
convert-im6.q16: improper image header `/tmp/ActiveStorage-21670-…' @ error/png.c/ReadPNGImage/4092.
convert-im6.q16: no images defined `/tmp/image_processing….png' @ error/convert.c/ConvertImageCommand/3258.

System configuration

Rails version: 6.0.3.4

Ruby version: 2.6.5p114

@igorkasyanchuk igorkasyanchuk added the help wanted Extra attention is needed label Nov 19, 2020
@samiggapps
Copy link

I did find this answer on SO which helped fix my issue
https://stackoverflow.com/a/60084096/7913423

@Mth0158
Copy link
Collaborator

Mth0158 commented Dec 8, 2023

For your information, this issue was solved in October 2022 by @kukicola as mentioned in the thread.
The solution is to add the processable_image validator as shown below. It will validate that the image can be processed by MiniMagick or Vips.

class Screenshot < ApplicationRecord
  has_one_attached :image
  validates :image, attached: true, content_type: [:png], processable_image: true

  def medium_image
    if self.image.attached?
      self.image.variant(
        resize_to_limit: [670, 600]
      ).processed
    end
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants