thoughtbot / paperclip

Easy file attachment management for ActiveRecord
https://thoughtbot.com
Other
9.01k stars 2.43k forks source link

Errno::ENAMETOOLONG: File name too long for images downloaded from long urls using UriAdapter #1246

Closed skyshard closed 9 years ago

skyshard commented 11 years ago

TempfileFactory generates a file with a long name, and then destination.write on line 33 in uri_adapter.rb fails:

https://github.com/thoughtbot/paperclip/blob/62264c65f8b327b8078151e2d4cd589c845a1b5a/lib/paperclip/io_adapters/uri_adapter.rb#L33

It seems that the main problem is that anything after the last '/' in the url is taken to be the filename, so long query strings / long urls will fail

jyurek commented 11 years ago

Can you give me an example of a URL that fails here?

skyshard commented 11 years ago

Here's one (ignoring the content of the image, the url and image itself are still valid): http://s.wordpress.com/mshots/v1/http%3A%2F%2Fwww.google.com%2Furl%3Fsa%3DX%26amp%3Bq%3Dhttp%3A%2F%2Fwww.telecompaper.com%2Fnews%2Fgyft-accepts-bitcoin-payments-in-android-app--943095%26amp%3Bct%3Dga%26amp%3Bcad%3DCAcQARgBIAAoATAAOABA4a7FjAVIAlAAWABiBWVuLVVT%26amp%3Bcd%3DKt202-M0oxw%26amp%3Busg%3DAFQjCNFQq2b9b1xtat59CsMxqoR01LsJUA

Checking back in my logs, I also see this error message:

File name too long - /tmp/article;tt=ipad;plc=long%20island;tablet=o;chn=gadgets%20&%20tech;subc=tech%20gear;sect=tech%20gear;nid=61608931;top=gadgets%20&%20tech;top=tech%20gear;top=ipad;top=ipad;top=iphone;top=ipod%20touch;top=free;top=games;ed=long-island-ny;uid=1839001;etid=46117;pgtp=article;tile=3;pos=3;dc_ref=http_%2F%2Fwww.examiner20130514-2-npvn4d.com%2Farticle%2Ffree-new-apps-for-ipad-iphone-ipod-touch-worth-getting-now;sz=300x250;kw=;ord=803822063

which seems to be an image from this page that may no longer be present

I monkey patched UriAdapter to use the hash of the filename instead, if it's over a certain length/not a normal filename as in the case above. Also, File.extname doesn't give a useful extension in this case anyways, so I added a sanity check there too.

require 'digest/sha2'

module Paperclip
  class UriAdapter 
    MAXIMUM_FILENAME_LENGTH = 160
    MAXIMUM_EXTENSION_LENGTH = 5

    private
    def destination
      if original_filename.length > MAXIMUM_FILENAME_LENGTH
        extension = File.extname(original_filename) 
        modified_filename = Digest::SHA256.hexdigest(original_filename) # shorten the filename to an acceptable length
        @original_filename = modified_filename

        if extension.length < MAXIMUM_EXTENSION_LENGTH
          @destination ||= TempfileFactory.new.generate("#{modified_filename}.#{extension}")
        else
          @destination ||= TempfileFactory.new.generate(modified_filename)
        end
      else
        @destination ||= TempfileFactory.new.generate(original_filename)
      end
    end
  end
end
jyurek commented 11 years ago

Wow, that is a long URL. Would you be able to wrap this up into a pull request with some tests? I'll gladly pull it in.

bilogub commented 11 years ago

this happens not only for UriAdapter the reason is deeper in generate method of TempfileFactory which is used in many places explicitly or implicitly and callstack starts from def assign uploaded_file in Attachment in string file = Paperclip.io_adapters.for(uploaded_file)

inspire22 commented 10 years ago

I too have an issue with users from ipads having large filenames.

This patch doesn't seem to be doing anything when I load it from an initializer with 'require paperclip' before it. Any suggestions?

maclover7 commented 9 years ago

Hi @skyshard and @inspire22 ! Is this still an issue for you in Paperclip; I know this issue is from approximately 2 years ago. If it is still an issue, can you please provide the code that's causing you the error? Thanks!

codeodor commented 9 years ago

This is not only a problem with UriAdapter. I am seeing it intermittently with an image that just has a lot of post-processing going on. In our case, since Thumbnail sets @basename = File.basename(@file.path, @current_format) and since @file is already a Tempfile it has a long name. And then I guess it keeps getting longer with each post-process.

Since Tempfile is already supposed to be unique from the OS, can Paperclip just set @basename to the original filename? Or maybe a hash of the filename? That should guarantee this error does not show up.

maclover7 commented 9 years ago

Would you be willing to send in a PR?

codeodor commented 9 years ago

Yes. It's on my todo list, hopefully I can get to it this weekend.

On Thursday, April 9, 2015, Jon Moss notifications@github.com wrote:

Would you be willing to send in a PR?

— Reply to this email directly or view it on GitHub https://github.com/thoughtbot/paperclip/issues/1246#issuecomment-91384541 .

codeodor commented 9 years ago

I never got around to it this weekend. But I still hope to this week.

codeodor commented 9 years ago

PR is on https://github.com/thoughtbot/paperclip/pull/1833.

codeodor commented 9 years ago

I think all places in paperclip now use paperclip's Tempfile rather than Ruby's, so this should be able to be closed now.

tute commented 9 years ago

Thank you @codeodor!