Processable image validation seems not working in all cases

[floganz]

Hi,

I've noticed that processable_image validation works kind of strange. It's possible to create any invalid image file (like create .txt file and then change extension to .png) and upload it even if that validation enabled in the model.

Here is test application I've created to show the issue.
There's both instruction how to reproduce it locally via interface and test that indicated the same behaviour. Sample images also added to that repo.

I'm using Vips for image analysis and it looks like all needed for vips libs installed.
Running vips --vips-config on my machine next.

Output

enable debug: false
enable deprecated: true
enable modules: true
enable cplusplus: true
enable RAD load/save: true
enable Analyze7 load/save: true
enable PPM load/save: true
enable GIF load: true
use fftw for FFTs: true
accelerate loops with ORC: true
ICC profile support with lcms: true
zlib: true
text rendering with pangocairo: true
font file support with fontconfig: true
EXIF metadata support with libexif: true
JPEG load/save with libjpeg: true
JXL load/save with libjxl: true (dynamic module: true)
JPEG2000 load/save with OpenJPEG: true
PNG load/save with libspng: true
PNG load/save with libpng: false
selected quantisation package: imagequant
TIFF load/save with libtiff: true
image pyramid save with libgsf: true
HEIC/AVIF load/save with libheif: true (dynamic module: true)
WebP load/save with libwebp: true
PDF load with PDFium: false
PDF load with poppler-glib: true (dynamic module: true)
SVG load with librsvg: true
EXR load with OpenEXR: true
OpenSlide load: true (dynamic module: true)
Matlab load with libmatio: true
NIfTI load/save with niftiio: false
FITS load/save with cfitsio: true
GIF save with cgif: true
selected Magick package: MagickCore (dynamic module: true)
Magick API version: magick7
Magick load: true
Magick save: true

[Mth0158]

Thanks @floganz for this issue.
It's a bit late, but I'll try to investigate it in the coming days. Do you have any update on that issue btw?

[floganz]

I've debugged gem a bit and discovered track trace for this issue and possible solution. However I'm lacking context why it was made in that way, maybe you know why. @Mth0158

So, basically for processable_image is responsible ProcessableImageValidator class which delegated decision of file being image or not to another class Metadata.new(file).valid?, e.g. here.

Inside Metadata defined #valid? which seems OK.

def valid?
  read_image
  true
rescue InvalidImageError
  false
end

But #read_image has suspicious behaviour

def read_image
  # different ways to build a path to file
  ...
  image = new_image_from_path(tempfile.path) # or other way to get path to file

  raise InvalidImageError unless valid_image?(image)
  yield image if block_given?
rescue LoadError, NameError
  logger.info "Skipping image analysis because the mini_magick or ruby-vips gem isn't installed"
  {}
rescue exception_class => error
  logger.error "Skipping image analysis due to an #{exception_class.name.split('::').map(&:downcase).join(' ').capitalize} error: #{error.message}"
  {}
ensure
  image = nil
end

So here we have two rescues: rescue LoadError, NameError and rescue exception_class => error. Both will just skip the check, if those are triggered we're not checking for file being opened and silently return nothing to #valid? which in the end returns true in these cases.

What I don't get why if we could not read the file at all we return true? My understanding that if file cannot be read it's should invalid unless opposite is confirmed.

So, my guess on fixing it is to modify rescues in the #read_image so that they throw InvalidImageError together with those prints in the log.

[Mth0158]

Hi @floganz
Thanks for the investigation, it helps a lot. I'll have a look at it in the following days :)
@igorkasyanchuk do you have more context for this issue? It's a bit suspicious that we return true with valid? when skipping analysis?

[igorkasyanchuk]

Hi @floganz @Mth0158 to be honest I don't remember, maybe it was not me who wrote it 🤣 . I would only suggest starting with specs that fail.

[Mth0158]

Hi @floganz,
Long time no see! I will start working on a refacto of the MiniMagick and Vips analyzers soon, I will let you know when it's done so you can fix this issue.

[Mth0158]

It should be fixed by #299 (0 byte size test case). I am closing the issue, feel free to reopen it if it's necessary. The update should be available in the coming release (1.3.5)