From 41c092d9d8801ac88fbfdfff8dfdc9262444cc86 Mon Sep 17 00:00:00 2001
From: Jon Yurek <jyurek@thoughtbot.com>
Date: Fri, 24 Jan 2014 14:36:35 -0500
Subject: [PATCH] Add media-type spoof detection

---
 lib/paperclip.rb                              |  1 +
 lib/paperclip/attachment.rb                   | 14 +++++++-
 lib/paperclip/content_type_detector.rb        | 14 +-------
 lib/paperclip/has_attached_file.rb            |  5 +++
 lib/paperclip/io_adapters/abstract_adapter.rb |  2 +-
 .../io_adapters/attachment_adapter.rb         |  8 ++---
 lib/paperclip/io_adapters/data_uri_adapter.rb | 13 +++----
 lib/paperclip/io_adapters/stringio_adapter.rb | 18 +++++-----
 lib/paperclip/media_type_spoof_detector.rb    | 36 +++++++++++++++++++
 lib/paperclip/tempfile_factory.rb             |  6 +++-
 lib/paperclip/validators.rb                   |  1 +
 .../media_type_spoof_detection_validator.rb   | 27 ++++++++++++++
 test/content_type_detector_test.rb            | 10 ------
 test/fixtures/empty.html                      |  1 +
 test/io_adapters/abstract_adapter_test.rb     |  1 +
 test/io_adapters/attachment_adapter_test.rb   |  2 +-
 test/media_type_spoof_detector_test.rb        | 28 +++++++++++++++
 test/tempfile_factory_test.rb                 | 14 +++++++-
 .../attachment_presence_validator_test.rb     |  2 +-
 ...dia_type_spoof_detection_validator_test.rb | 12 +++++++
 20 files changed, 165 insertions(+), 50 deletions(-)
 create mode 100644 lib/paperclip/media_type_spoof_detector.rb
 create mode 100644 lib/paperclip/validators/media_type_spoof_detection_validator.rb
 create mode 100644 test/fixtures/empty.html
 create mode 100644 test/media_type_spoof_detector_test.rb
 create mode 100644 test/validators/media_type_spoof_detection_validator_test.rb

diff --git a/lib/paperclip.rb b/lib/paperclip.rb
index bce14fc03..c247f0690 100644
--- a/lib/paperclip.rb
+++ b/lib/paperclip.rb
@@ -43,6 +43,7 @@
 require 'paperclip/storage'
 require 'paperclip/callbacks'
 require 'paperclip/file_command_content_type_detector'
+require 'paperclip/media_type_spoof_detector'
 require 'paperclip/content_type_detector'
 require 'paperclip/glue'
 require 'paperclip/errors'
diff --git a/lib/paperclip/attachment.rb b/lib/paperclip/attachment.rb
index 1aa0323ce..739ee95e4 100644
--- a/lib/paperclip/attachment.rb
+++ b/lib/paperclip/attachment.rb
@@ -166,6 +166,18 @@ def path(style_name = default_style)
       path.respond_to?(:unescape) ? path.unescape : path
     end
 
+    # :nodoc:
+    def staged_path(style_name = default_style)
+      if staged?
+        @queued_for_write[style_name].path
+      end
+    end
+
+    # :nodoc:
+    def staged?
+      ! @queued_for_write.empty?
+    end
+
     # Alias to +url+
     def to_s style_name = default_style
       url(style_name)
@@ -485,7 +497,7 @@ def flush_errors #:nodoc:
       end
     end
 
-    # called by storage after the writes are flushed and before @queued_for_writes is cleared
+    # called by storage after the writes are flushed and before @queued_for_write is cleared
     def after_flush_writes
       @queued_for_write.each do |style, file|
         file.close unless file.closed?
diff --git a/lib/paperclip/content_type_detector.rb b/lib/paperclip/content_type_detector.rb
index 233a93cbb..67cda6b18 100644
--- a/lib/paperclip/content_type_detector.rb
+++ b/lib/paperclip/content_type_detector.rb
@@ -32,10 +32,6 @@ def detect
         EMPTY_TYPE
       elsif calculated_type_matches.any?
         calculated_type_matches.first
-      elsif official_type_matches.any?
-        official_type_matches.first
-      elsif unofficial_type_matches.any?
-        unofficial_type_matches.first
       else
         type_from_file_command || SENSIBLE_DEFAULT
       end.to_s
@@ -46,7 +42,7 @@ def detect
     def empty_file?
       File.exists?(@filename) && File.size(@filename) == 0
     end
-    
+
     alias :empty? :empty_file?
 
     def blank_name?
@@ -61,14 +57,6 @@ def calculated_type_matches
       possible_types.select{|content_type| content_type == type_from_file_command }
     end
 
-    def official_type_matches
-      possible_types.reject{|content_type| content_type.match(/\/x-/) }
-    end
-
-    def unofficial_type_matches
-      possible_types.select{|content_type| content_type.match(/\/x-/) }
-    end
-
     def type_from_file_command
       @type_from_file_command ||= FileCommandContentTypeDetector.new(@filename).detect
     end
diff --git a/lib/paperclip/has_attached_file.rb b/lib/paperclip/has_attached_file.rb
index 2bbb3f27c..3b39e7d75 100644
--- a/lib/paperclip/has_attached_file.rb
+++ b/lib/paperclip/has_attached_file.rb
@@ -18,6 +18,7 @@ def define
       register_new_attachment
       add_active_record_callbacks
       add_paperclip_callbacks
+      add_required_validations
     end
 
     private
@@ -77,6 +78,10 @@ def register_new_attachment
       Paperclip::AttachmentRegistry.register(@klass, @name, @options)
     end
 
+    def add_required_validations
+      @klass.validates_media_type_spoof_detection @name
+    end
+
     def add_active_record_callbacks
       name = @name
       @klass.send(:after_save) { send(name).send(:save) }
diff --git a/lib/paperclip/io_adapters/abstract_adapter.rb b/lib/paperclip/io_adapters/abstract_adapter.rb
index 3a8d2ddf1..021df5212 100644
--- a/lib/paperclip/io_adapters/abstract_adapter.rb
+++ b/lib/paperclip/io_adapters/abstract_adapter.rb
@@ -34,7 +34,7 @@ def assignment?
     private
 
     def destination
-      @destination ||= TempfileFactory.new.generate(original_filename)
+      @destination ||= TempfileFactory.new.generate
     end
 
     def copy_to_tempfile(src)
diff --git a/lib/paperclip/io_adapters/attachment_adapter.rb b/lib/paperclip/io_adapters/attachment_adapter.rb
index 79fe41206..8319617f6 100644
--- a/lib/paperclip/io_adapters/attachment_adapter.rb
+++ b/lib/paperclip/io_adapters/attachment_adapter.rb
@@ -20,11 +20,11 @@ def cache_current_values
       @size = @tempfile.size || @target.size
     end
 
-    def copy_to_tempfile(src)
-      if src.respond_to? :copy_to_local_file
-        src.copy_to_local_file(@style, destination.path)
+    def copy_to_tempfile(source)
+      if source.staged?
+        FileUtils.cp(source.staged_path(@style), destination.path)
       else
-        FileUtils.cp(src.path(@style), destination.path)
+        source.copy_to_local_file(@style, destination.path)
       end
       destination
     end
diff --git a/lib/paperclip/io_adapters/data_uri_adapter.rb b/lib/paperclip/io_adapters/data_uri_adapter.rb
index 0a158c9e2..2f4a14a7c 100644
--- a/lib/paperclip/io_adapters/data_uri_adapter.rb
+++ b/lib/paperclip/io_adapters/data_uri_adapter.rb
@@ -4,19 +4,14 @@ class DataUriAdapter < StringioAdapter
     REGEXP = /\Adata:([-\w]+\/[-\w\+]+);base64,(.*)/m
 
     def initialize(target_uri)
-      @target_uri = target_uri
-      cache_current_values
-      @tempfile = copy_to_tempfile
+      super(extract_target(target_uri))
     end
 
     private
 
-    def cache_current_values
-      self.original_filename = 'base64.txt'
-      data_uri_parts ||= @target_uri.match(REGEXP) || []
-      @content_type = data_uri_parts[1] || 'text/plain'
-      @target = StringIO.new(Base64.decode64(data_uri_parts[2] || ''))
-      @size = @target.size
+    def extract_target(uri)
+      data_uri_parts = uri.match(REGEXP) || []
+      StringIO.new(Base64.decode64(data_uri_parts[2] || ''))
     end
 
   end
diff --git a/lib/paperclip/io_adapters/stringio_adapter.rb b/lib/paperclip/io_adapters/stringio_adapter.rb
index 6a7d4c5dd..7cf1b95a5 100644
--- a/lib/paperclip/io_adapters/stringio_adapter.rb
+++ b/lib/paperclip/io_adapters/stringio_adapter.rb
@@ -2,8 +2,8 @@ module Paperclip
   class StringioAdapter < AbstractAdapter
     def initialize(target)
       @target = target
-      cache_current_values
       @tempfile = copy_to_tempfile
+      cache_current_values
     end
 
     attr_writer :content_type
@@ -11,13 +11,10 @@ def initialize(target)
     private
 
     def cache_current_values
-      @original_filename = @target.original_filename if @target.respond_to?(:original_filename)
-      @original_filename ||= "stringio.txt"
-      self.original_filename = @original_filename.strip
-
-      @content_type = @target.content_type if @target.respond_to?(:content_type)
-      @content_type ||= "text/plain"
-
+      @content_type = ContentTypeDetector.new(@tempfile.path).detect
+      original_filename = @target.original_filename if @target.respond_to?(:original_filename)
+      original_filename ||= "data.#{extension_for(@content_type)}"
+      self.original_filename = original_filename.strip
       @size = @target.size
     end
 
@@ -29,6 +26,11 @@ def copy_to_tempfile
       destination
     end
 
+    def extension_for(content_type)
+      type = MIME::Types[content_type].first
+      type && type.extensions.first
+    end
+
   end
 end
 
diff --git a/lib/paperclip/media_type_spoof_detector.rb b/lib/paperclip/media_type_spoof_detector.rb
new file mode 100644
index 000000000..b0ec3215f
--- /dev/null
+++ b/lib/paperclip/media_type_spoof_detector.rb
@@ -0,0 +1,36 @@
+module Paperclip
+  class MediaTypeSpoofDetector
+    def self.using(file, name)
+      new(file, name)
+    end
+
+    def initialize(file, name)
+      @file = file
+      @name = name
+    end
+
+    def spoofed?
+      if ! @name.blank?
+        ! supplied_file_media_type.include?(calculated_media_type)
+      end
+    end
+
+    private
+
+    def supplied_file_media_type
+      MIME::Types.type_for(@name).collect(&:media_type)
+    end
+
+    def calculated_media_type
+      type_from_file_command.split("/").first
+    end
+
+    def type_from_file_command
+      begin
+        Paperclip.run("file", "-b --mime-type :file", :file => @file.path)
+      rescue Cocaine::CommandLineError
+        ""
+      end
+    end
+  end
+end
diff --git a/lib/paperclip/tempfile_factory.rb b/lib/paperclip/tempfile_factory.rb
index ef636e637..00aa5829b 100644
--- a/lib/paperclip/tempfile_factory.rb
+++ b/lib/paperclip/tempfile_factory.rb
@@ -1,7 +1,7 @@
 module Paperclip
   class TempfileFactory
 
-    def generate(name)
+    def generate(name = random_name)
       @name = name
       file = Tempfile.new([basename, extension])
       file.binmode
@@ -15,5 +15,9 @@ def extension
     def basename
       Digest::MD5.hexdigest(File.basename(@name, extension))
     end
+
+    def random_name
+      SecureRandom.uuid
+    end
   end
 end
diff --git a/lib/paperclip/validators.rb b/lib/paperclip/validators.rb
index 573856e05..de7676c04 100644
--- a/lib/paperclip/validators.rb
+++ b/lib/paperclip/validators.rb
@@ -3,6 +3,7 @@
 require 'paperclip/validators/attachment_content_type_validator'
 require 'paperclip/validators/attachment_presence_validator'
 require 'paperclip/validators/attachment_size_validator'
+require 'paperclip/validators/media_type_spoof_detection_validator'
 
 module Paperclip
   module Validators
diff --git a/lib/paperclip/validators/media_type_spoof_detection_validator.rb b/lib/paperclip/validators/media_type_spoof_detection_validator.rb
new file mode 100644
index 000000000..851c5ff94
--- /dev/null
+++ b/lib/paperclip/validators/media_type_spoof_detection_validator.rb
@@ -0,0 +1,27 @@
+require 'active_model/validations/presence'
+
+module Paperclip
+  module Validators
+    class MediaTypeSpoofDetectionValidator < ActiveModel::EachValidator
+      def validate_each(record, attribute, value)
+        adapter = Paperclip.io_adapters.for(value)
+        if Paperclip::MediaTypeSpoofDetector.using(adapter, value.original_filename).spoofed?
+          record.errors.add(attribute, :spoofed_media_type)
+        end
+      end
+    end
+
+    module HelperMethods
+      # Places ActiveModel validations on the presence of a file.
+      # Options:
+      # * +if+: A lambda or name of an instance method. Validation will only
+      #   be run if this lambda or method returns true.
+      # * +unless+: Same as +if+ but validates if lambda or method returns false.
+      def validates_media_type_spoof_detection(*attr_names)
+        options = _merge_attributes(attr_names)
+        validates_with MediaTypeSpoofDetectionValidator, options.dup
+        validate_before_processing MediaTypeSpoofDetectionValidator, options.dup
+      end
+    end
+  end
+end
diff --git a/test/content_type_detector_test.rb b/test/content_type_detector_test.rb
index df187e62c..0102976ed 100644
--- a/test/content_type_detector_test.rb
+++ b/test/content_type_detector_test.rb
@@ -18,16 +18,6 @@ class ContentTypeDetectorTest < Test::Unit::TestCase
     assert_equal "video/mp4", Paperclip::ContentTypeDetector.new(@filename).detect
   end
 
-  should 'find the first result that matches from the official types' do
-    @filename = "/path/to/something.bmp"
-    assert_equal "image/bmp", Paperclip::ContentTypeDetector.new(@filename).detect
-  end
-
-  should 'find the first unofficial result for this filename if no official ones exist' do
-    @filename = "/path/to/something.aiff"
-    assert_equal "audio/x-aiff", Paperclip::ContentTypeDetector.new(@filename).detect
-  end
-
   should 'find the right type in the list via the file command' do
     @filename = "#{Dir.tmpdir}/something.hahalolnotreal"
     File.open(@filename, "w+") do |file|
diff --git a/test/fixtures/empty.html b/test/fixtures/empty.html
new file mode 100644
index 000000000..18ecdcb79
--- /dev/null
+++ b/test/fixtures/empty.html
@@ -0,0 +1 @@
+<html></html>
diff --git a/test/io_adapters/abstract_adapter_test.rb b/test/io_adapters/abstract_adapter_test.rb
index 7c15bb841..a18ac08b8 100644
--- a/test/io_adapters/abstract_adapter_test.rb
+++ b/test/io_adapters/abstract_adapter_test.rb
@@ -13,6 +13,7 @@ def content_type
     setup do
       @adapter = TestAdapter.new
       @adapter.stubs(:path).returns("image.png")
+      Paperclip.stubs(:run).returns("image/png\n")
     end
 
     should "return the content type without newline" do
diff --git a/test/io_adapters/attachment_adapter_test.rb b/test/io_adapters/attachment_adapter_test.rb
index 60069290f..d685fab39 100644
--- a/test/io_adapters/attachment_adapter_test.rb
+++ b/test/io_adapters/attachment_adapter_test.rb
@@ -75,7 +75,7 @@ def setup
     end
 
     should "not generate paths that include restricted characters" do
-      assert_no_match /:/, @subject.path
+      assert_no_match(/:/, @subject.path)
     end
 
     should "not generate filenames that include restricted characters" do
diff --git a/test/media_type_spoof_detector_test.rb b/test/media_type_spoof_detector_test.rb
new file mode 100644
index 000000000..a7337bf25
--- /dev/null
+++ b/test/media_type_spoof_detector_test.rb
@@ -0,0 +1,28 @@
+require './test/helper'
+
+class MediaTypeSpoofDetectorTest < Test::Unit::TestCase
+  should 'reject a file that is named .html and identifies as PNG' do
+    file = File.open(fixture_file("5k.png"))
+    assert Paperclip::MediaTypeSpoofDetector.using(file, "5k.html").spoofed?
+  end
+
+  should 'not reject a file that is named .jpg and identifies as PNG' do
+    file = File.open(fixture_file("5k.png"))
+    assert ! Paperclip::MediaTypeSpoofDetector.using(file, "5k.jpg").spoofed?
+  end
+
+  should 'not reject a file that is named .html and identifies as HTML' do
+    file = File.open(fixture_file("empty.html"))
+    assert ! Paperclip::MediaTypeSpoofDetector.using(file, "empty.html").spoofed?
+  end
+
+  should 'not reject a file that does not have a name' do
+    file = File.open(fixture_file("empty.html"))
+    assert ! Paperclip::MediaTypeSpoofDetector.using(file, "").spoofed?
+  end
+
+  should 'not reject when the supplied file is an IOAdapter' do
+    adapter = Paperclip.io_adapters.for(File.new(fixture_file("5k.png")))
+    assert ! Paperclip::MediaTypeSpoofDetector.using(adapter, adapter.original_filename).spoofed?
+  end
+end
diff --git a/test/tempfile_factory_test.rb b/test/tempfile_factory_test.rb
index c89dda520..52c0c79a1 100644
--- a/test/tempfile_factory_test.rb
+++ b/test/tempfile_factory_test.rb
@@ -3,15 +3,27 @@
 class Paperclip::TempfileFactoryTest < Test::Unit::TestCase
   should "be able to generate a tempfile with the right name" do
     file = subject.generate("omg.png")
+    assert File.extname(file.path), "png"
   end
+
   should "be able to generate a tempfile with the right name with a tilde at the beginning" do
     file = subject.generate("~omg.png")
+    assert File.extname(file.path), "png"
   end
+
   should "be able to generate a tempfile with the right name with a tilde at the end" do
     file = subject.generate("omg.png~")
+    assert File.extname(file.path), "png"
   end
+
   should "be able to generate a tempfile from a file with a really long name" do
-    filename = "#{"longfilename" * 100}.txt"
+    filename = "#{"longfilename" * 100}.png"
     file = subject.generate(filename)
+    assert File.extname(file.path), "png"
+  end
+
+  should 'be able to take nothing as a parameter and not error' do
+   file = subject.generate
+   assert File.exists?(file.path)
   end
 end
diff --git a/test/validators/attachment_presence_validator_test.rb b/test/validators/attachment_presence_validator_test.rb
index 9ae930459..ebde39dc6 100644
--- a/test/validators/attachment_presence_validator_test.rb
+++ b/test/validators/attachment_presence_validator_test.rb
@@ -60,7 +60,7 @@ def build_validator(options={})
   context "with attachment" do
     setup do
       build_validator
-      @dummy.avatar = StringIO.new('.')
+      @dummy.avatar = StringIO.new('.\n')
       @validator.validate(@dummy)
     end
 
diff --git a/test/validators/media_type_spoof_detection_validator_test.rb b/test/validators/media_type_spoof_detection_validator_test.rb
new file mode 100644
index 000000000..6b76eb7d4
--- /dev/null
+++ b/test/validators/media_type_spoof_detection_validator_test.rb
@@ -0,0 +1,12 @@
+require './test/helper'
+
+class MediaTypeSpoofDetectionValidatorTest < Test::Unit::TestCase
+  def setup
+    rebuild_model
+    @dummy = Dummy.new
+  end
+
+  should "be on the attachment without being explicitly added" do
+    assert Dummy.validators_on(:avatar).any?{ |validator| validator.kind == :media_type_spoof_detection }
+  end
+end