thoughtbot/paperclip

Content Type Validation not failing for spoofed video file

NeoElit opened this issue · 4 comments

For the following model

class User < ActiveRecord::Base
  has_attached_file :snap, :styles => { :thumb => "100x100>" }, :default_url => "/images/:style/missing.png"
  validates_attachment_content_type :snap, :content_type => /\Avideo\/.*\Z/
end

on paperclip '4.3.1', rails '4.2.6', content spoof detector doesn't seem to work when simply renaming data3.pdf file to data3.mp4
According to docs, it should be working since 'file' command is identifying it properly.

file -b --mime /tmp/data3.mp4    =>>   application/pdf; charset=binary

Am I missing something here? Or is it a bug?

Dug a little deeper.
Seems like issue pops because
calculated_type_mismatch? fails since media_types_from_name contains higher level type "application" which is also returned by calculated_media_type, which means almost all contents other than the basic ones will be allowed.

For a pdf file with mp4 extension:
calculated_content_type => "application/pdf"
calculated_media_type => "application"
media_types_from_name => ["application", "audio", "video", "video"]

So I guess spoof detection will fail on all higher level content type checks.

Instead of

def calculated_media_type
  @calculated_media_type ||= calculated_content_type.split("/").first
end

how about matching with sub type for spoof detection?

def calculated_media_type
  @calculated_media_type ||= calculated_content_type.split("/").last.split(';').first
end

Not sure about it's side effects though.

Currently the following patch works for me:

module Paperclip
  class MediaTypeSpoofDetector

    def supplied_type_mismatch?
      supplied_media_type.present? && !media_types_from_name.include?(supplied_media_type)
    end

    def supplied_media_type
      @content_type.split("/").last
    end

    def media_types_from_name
      @media_types_from_name ||= content_types_from_name.collect(&:sub_type)
    end

    def calculated_media_type
      @calculated_media_type ||= calculated_content_type.split("/").last.split(';').first
    end

  end
end

Looks like we found a workaround, closing.