Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Replace illegal characters in file extension #988

Closed
wants to merge 2 commits into from

3 participants

@felipecsl

We found issues with paperclip when the image file extension contains colon, eg: https://p.twimg.com/AzvR1RsCUAEhBhV.jpg:large

This is very common for twitter images. In our case, it caused an error in ImageMagick identify when checking the image dimensions via image.queued_for_write[:original] before saving it.

The command was failing since bash was getting confused with the .jpg:large[0].

This replaces any colons in the extension with underscores.

@travisbot

This pull request fails (merged 508a93d into 70f0f1f).

@travisbot

This pull request fails (merged f85be6e into 70f0f1f).

@felipecsl

Can anyone help debug these test failures? Testing locally, even when reverting my changes, these tests still fail. Does anyone know of any flakyness around that?
Thanks!

@jyurek
Admin

@felipecsl I think the problem here is the result of HFS using colons as path separators, right? Why does this problem only affect extensions? Might it be the case that filenames themselves are affected?

@felipecsl

@jyurek as far as I can tell, the filename is already being cleaned up by Attachment#cleanup_filename, so any colons there are already being replaced. However, there was no cleaning up for the file extension, which is where I was having issues.

@jyurek
Admin

The problem is that the cleanup_filename happens after the file has been processed, which means that the command line gets to it before the cleanup_filename method.

I've committed a fix for this in master (83d8ec5), and it should solve the problem. I'm going to close this, but if this doesn't work please let me know.

@jyurek jyurek closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 8, 2012
  1. @felipecsl
  2. @felipecsl

    Add new test file

    felipecsl authored
This page is out of date. Refresh to see the latest.
View
7 lib/paperclip/tempfile_factory.rb
@@ -2,6 +2,7 @@ module Paperclip
class TempfileFactory
ILLEGAL_FILENAME_CHARACTERS = /^~/
+ ILLEGAL_EXTENSION_CHARACTERS = /:/
def generate(name)
@name = name
@@ -11,11 +12,11 @@ def generate(name)
end
def extension
- File.extname(@name)
+ File.extname(@name).gsub(ILLEGAL_EXTENSION_CHARACTERS, '_')
end
def basename
- File.basename(@name, extension).gsub(ILLEGAL_FILENAME_CHARACTERS, '_')
+ File.basename(@name, File.extname(@name)).gsub(ILLEGAL_FILENAME_CHARACTERS, '_')
end
end
-end
+end
View
20 test/attachment_test.rb
@@ -869,6 +869,26 @@ def do_after_all; end
end
end
+ context 'Attachment with filename extension containing special characters' do
+ setup do
+ @old_defaults = Paperclip::Attachment.default_options.dup
+ Paperclip::Attachment.default_options.merge!({
+ :path => ":rails_root/:attachment/:class/:style/:id/:basename.:extension"
+ })
+ FileUtils.rm_rf("tmp")
+ rebuild_model
+ @instance = Dummy.new
+ @instance.stubs(:id).returns 123
+ @attachment = Paperclip::Attachment.new(:avatar, @instance)
+ @file = File.new(fixture_file("5k.png:large"), 'rb')
+ end
+
+ should 'rename file the extension' do
+ @attachment.assign(@file)
+ assert_match @attachment.queued_for_write[:original].path, /png_large/
+ end
+ end
+
context "An attachment" do
setup do
@old_defaults = Paperclip::Attachment.default_options.dup
View
BIN  test/fixtures/5k.png:large
Binary file not shown
Something went wrong with that request. Please try again.