Replacing all special characters to underscore

Fixes #703

ChangeLog

+2012-01-27  Prem Sichanugrist <psichanugrist@thoughtbot.com>
+
+* Paperclip will now replace all the special characters in the filename to an
+  underscore. This is a more desired behavior against having to deal with URL
+  escaping and unescaping later. You can see the list of blacklist characters
+  in `lib/paperclip/attachment.rb`
+
 2012-01-20	Jon Yurek <jyurek@thoughtbot.com>
 2012-01-20	Jon Yurek <jyurek@thoughtbot.com>
 
 
 	* lib/paperclip/railtie.rb (insert): Hide ActiveRecord-specific stuff in
 	* lib/paperclip/railtie.rb (insert): Hide ActiveRecord-specific stuff in

lib/paperclip/attachment.rb

@@ -108,7 +108,7 @@ module Paperclip
       uploaded_filename ||= uploaded_file.original_filename
       uploaded_filename ||= uploaded_file.original_filename
       stores_fingerprint             = @instance.respond_to?("#{name}_fingerprint".to_sym)
       stores_fingerprint             = @instance.respond_to?("#{name}_fingerprint".to_sym)
       @queued_for_write[:original]   = to_tempfile(uploaded_file)
       @queued_for_write[:original]   = to_tempfile(uploaded_file)
-      instance_write(:file_name,       uploaded_filename.strip)
+      instance_write(:file_name,       cleanup_filename(uploaded_filename.strip))
       instance_write(:content_type,    uploaded_file.content_type.to_s.strip)
       instance_write(:content_type,    uploaded_file.content_type.to_s.strip)
       instance_write(:file_size,       uploaded_file.size.to_i)
       instance_write(:file_size,       uploaded_file.size.to_i)
       instance_write(:fingerprint,     generate_fingerprint(uploaded_file)) if stores_fingerprint
       instance_write(:fingerprint,     generate_fingerprint(uploaded_file)) if stores_fingerprint
@@ -478,5 +478,8 @@ module Paperclip
       end
       end
     end
     end
 
 
+    def cleanup_filename(filename)
+      filename.gsub(/[&$+,\/:;=?@<>\[\]\{\}\|\\\^~%# ]/, '_')
+    end
   end
   end
 end
 end

test/attachment_test.rb

@@ -666,7 +666,7 @@ class AttachmentTest < Test::Unit::TestCase
   context "Assigning an attachment" do
   context "Assigning an attachment" do
     setup do
     setup do
       rebuild_model :styles => { :something => "100x100#" }
       rebuild_model :styles => { :something => "100x100#" }
-      @file  = StringIO.new(".")
+      @file = StringIO.new(".")
       @file.stubs(:original_filename).returns("5k.png\n\n")
       @file.stubs(:original_filename).returns("5k.png\n\n")
       @file.stubs(:content_type).returns("image/png\n\n")
       @file.stubs(:content_type).returns("image/png\n\n")
       @file.stubs(:to_tempfile).returns(@file)
       @file.stubs(:to_tempfile).returns(@file)
@@ -713,6 +713,25 @@ class AttachmentTest < Test::Unit::TestCase
     end
     end
   end
   end
 
 
+  context "Attachment with reserved filename" do
+    "&$+,/:;=?@<>[]{}|\^~%# ".split(//).each do |character|
+      context "with character #{character}" do
+        setup do
+          rebuild_model
+
+          file = StringIO.new(".")
+          file.stubs(:original_filename).returns("file#{character}name.png")
+          @dummy = Dummy.new
+          @dummy.avatar = file
+        end
+
+        should "convert special character into underscore" do
+          assert_equal "file_name.png", @dummy.avatar.original_filename
+        end
+      end
+    end
+  end
+
   context "Attachment with uppercase extension and a default style" do
   context "Attachment with uppercase extension and a default style" do
     setup do
     setup do
       @old_defaults = Paperclip::Attachment.default_options.dup
       @old_defaults = Paperclip::Attachment.default_options.dup

test/storage/filesystem_test.rb

@@ -44,12 +44,12 @@ class FileSystemTest < Test::Unit::TestCase
         assert File.exists?(@dummy.avatar.path)
         assert File.exists?(@dummy.avatar.path)
       end
       end
 
 
-      should "store the path unescaped" do
-        assert_match /\/spaced file\.png/, @dummy.avatar.path
+      should "return a replaced version for path" do
+        assert_match /.+\/spaced_file\.png/, @dummy.avatar.path
       end
       end
 
 
-      should "return an escaped version of URL" do
-        assert_match /\/spaced%20file\.png/, @dummy.avatar.url
+      should "return a replaced version for url" do
+        assert_match /.+\/spaced_file\.png/, @dummy.avatar.url
       end
       end
     end
     end
   end
   end

test/storage/s3_live_test.rb

@@ -80,12 +80,12 @@ unless ENV["S3_BUCKET"].blank?
         @dummy.save
         @dummy.save
       end
       end
 
 
-      should "return an unescaped version for path" do
-        assert_match /.+\/spaced file\.png/, @dummy.avatar.path
+      should "return a replaced version for path" do
+        assert_match /.+\/spaced_file\.png/, @dummy.avatar.path
       end
       end
 
 
-      should "return an escaped version for url" do
-        assert_match /.+\/spaced%20file\.png/, @dummy.avatar.url
+      should "return a replaced version for url" do
+        assert_match /.+\/spaced_file\.png/, @dummy.avatar.url
       end
       end
 
 
       should "be accessible" do
       should "be accessible" do

test/storage/s3_test.rb

@@ -222,12 +222,12 @@ class S3Test < Test::Unit::TestCase
       @dummy.avatar = File.new(fixture_file('spaced file.png'), 'rb')
       @dummy.avatar = File.new(fixture_file('spaced file.png'), 'rb')
     end
     end
 
 
-    should "return an unescaped version for path" do
-      assert_match /.+\/spaced file\.png/, @dummy.avatar.path
+    should "return a replaced version for path" do
+      assert_match /.+\/spaced_file\.png/, @dummy.avatar.path
     end
     end
 
 
-    should "return an escaped version for url" do
-      assert_match /.+\/spaced%20file\.png/, @dummy.avatar.url
+    should "return a replaced version for url" do
+      assert_match /.+\/spaced_file\.png/, @dummy.avatar.url
     end
     end
   end
   end
 
 
@@ -248,12 +248,12 @@ class S3Test < Test::Unit::TestCase
       @dummy.save
       @dummy.save
     end
     end
 
 
-    should "return an unescaped version for path" do
-      assert_match /.+\/question\?mark\.png/, @dummy.avatar.path
+    should "return a replaced version for path" do
+      assert_match /.+\/question_mark\.png/, @dummy.avatar.path
     end
     end
 
 
-    should "return an escaped version for url" do
-      assert_match /.+\/question%3Fmark\.png/, @dummy.avatar.url
+    should "return a replaced version for url" do
+      assert_match /.+\/question_mark\.png/, @dummy.avatar.url
     end
     end
   end
   end