shrinerb / shrine

File Attachment toolkit for Ruby applications
https://shrinerb.com
MIT License
3.18k stars 275 forks source link

File extension incorrectly parsed #545

Closed andresespinosapc closed 2 years ago

andresespinosapc commented 3 years ago

I'm not sure if this is a problem of this library, Down or Shrine itself.

I have a url that has query params, something like this: https://google.com/image.jpg?test.w23fs The problem is that the extension is incorrectly parsed as .w23fs instead of .jpg. This makes my image processor (vips) to throw an error.

jrochkind commented 3 years ago

I think I ran into something like this once too, which I worked around somehow, but am having trouble remembering or finding the details.

Can you provide more details to reproduce your problem, perhaps the smallest possible Uploader source code example that will reproduce the problem? That would make it easier folks to consider a possible fix or workaround suggestion.

andresespinosapc commented 3 years ago

I'll make the example with Rails. In the intializer I have:

Shrine.storages = {
  cache: Shrine::Storage::Url.new,
  store: Shrine::Storage::FileSystem.new('public', prefix: 'uploads')
}

Then I can create a record with record = Model.create!(image_data: { id: 'https://myurl.com/image.jpg?test.w23fs', storage: 'cache' })

And the error happens when promoting: record.image_attacher.atomic_promote. After promoting, record.image_data is something like this:

{
  id: '3f4b7c6a0b23e2498098f683af8234b2.w23fs',
  storage: 'store'
}

Then I get this error Vips::Error: No known saver for '/tmp/image_processing20210714-16206-13qgy7t.w23fs'. This happens when creating the derivatives (I have an option to create the derivatives on promote) and I'm pretty sure it's because of the filename. If I could change the filename to have the correct extension this wouldn't happen.

janko commented 3 years ago

This seems to be a bug in Shrine. Shrine::UploadedFile#download uses UploadedFile#extension for the tempfile extension, which in this case wrongly returns w23fs instead of jpg. The #extension method does have some logic for handling URLs, but it doesn't handle URLs of this type, so it will need to be fixed.

As a workaround, probably this monkey patch would work:

class Shrine::UploadedFile
  def extension
    if storage.is_a?(Shrine::Storage::Url)
      path = URI.parse(id).path
      result = File.extname(path)[1..-1]
      result.downcase
    else
      super
    end
  end
end
andresespinosapc commented 3 years ago

Thanks! Your answer saved me, I was searching through all the source code for hours looking where it came from