Remove the automatic loading of URI Adapters

Remove the URI adapters. Few people use them by default and they can allow insight into the internal networks of the server. If you want to enable them, add (for example) `Paperclip.DataUriAdapter.register` to your `config/initializers/paperclip.rb` file. This is related to CVE-2017-0889. Elsewhere fix CI: it's `s3.us-west-2` now, with a dot.
kreeti · Jan 23, 2018 · 80847b4 · 80847b4
1 parent c794f6d
commit 80847b4
Show file tree

Hide file tree

Showing 24 changed files with 147 additions and 58 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -7,11 +7,6 @@ rvm:
   - 2.3
   - 2.4
 
-# FIXME: fails with modern bundler
-before_install:
-  - rvm @global do gem uninstall bundler -a -x
-  - rvm @global do gem install bundler -v 1.12.5
-
 script: "bundle exec rake clean spec cucumber"
 
 addons:

diff --git a/Gemfile b/Gemfile
@@ -12,4 +12,5 @@ group :development, :test do
   gem 'mime-types'
   gem 'builder'
   gem 'rubocop', require: false
+  gem 'rspec'
 end
diff --git a/README.md b/README.md
@@ -17,7 +17,6 @@ https://github.com/thoughtbot/paperclip/releases
 <!-- START doctoc generated TOC please keep comment here to allow auto update -->
 <!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
 
-
 - [Requirements](#requirements)
   - [Ruby and Rails](#ruby-and-rails)
   - [Image Processor](#image-processor)
@@ -43,6 +42,7 @@ https://github.com/thoughtbot/paperclip/releases
   - [Vintage Syntax](#vintage-syntax)
 - [Storage](#storage)
   - [Understanding Storage](#understanding-storage)
+- [IO Adapters](#io-adapters)
 - [Post Processing](#post-processing)
 - [Custom Attachment Processors](#custom-attachment-processors)
 - [Events](#events)
@@ -608,6 +608,34 @@ variables.
 
 ---
 
+IO Adapters
+-----------
+
+When a file is uploaded or attached, it can be in one of a few different input
+forms, from Rails' UploadedFile object to a StringIO to a Tempfile or even a
+simple String that is a URL that points to an image.
+
+Paperclip will accept, by default, many of these sources. It also is capable of
+handling even more with a little configuration. The IO Adapters that handle
+images from non-local sources are not enabled by default. They can be enabled by
+adding a line similar to the following into `config/initializers/paperclip.rb`:
+
+```ruby
+Paperclip::DataUriAdapter.register
+```
+
+It's best to only enable a remote-loading adapter if you need it. Otherwise
+there's a chance that someone can gain insight into your internal network
+structure using it as a vector.
+
+The following adapters are *not* loaded by default:
+
+* `Paperclip::UriAdapter` - which accepts a `URI` instance.
+* `Paperclip::HttpUrlProxyAdapter` - which accepts a `http` string.
+* `Paperclip::DataUriAdapter` - which accepts a Base64-encoded `data:` string.
+
+---
+
 Post Processing
 ---------------
 

diff --git a/Rakefile b/Rakefile
@@ -9,7 +9,7 @@ task :default => [:clean, :all]
 desc 'Test the paperclip plugin under all supported Rails versions.'
 task :all do |t|
   if ENV['BUNDLE_GEMFILE']
-    exec('rake spec cucumber')
+    exec('rake spec && cucumber')
   else
     exec("rm -f gemfiles/*.lock")
     Rake::Task["appraisal:gemfiles"].execute

diff --git a/features/step_definitions/rails_steps.rb b/features/step_definitions/rails_steps.rb
@@ -22,6 +22,7 @@
       gem "rubysl", :platform => :rbx
       """
     And I remove turbolinks
+    And I comment out lines that contain "action_mailer" in "config/environments/*.rb"
     And I empty the application.js file
     And I configure the application to use "paperclip" from this project
   }
@@ -49,6 +50,16 @@
   end
 end
 
+Given /^I comment out lines that contain "([^"]+)" in "([^"]+)"$/ do |contains, glob|
+  cd(".") do
+    Dir.glob(glob).each do |file|
+      transform_file(file) do |content|
+        content.gsub(/^(.*?#{contains}.*?)$/) { |line| "# #{line}" }
+      end
+    end
+  end
+end
+
 Given /^I attach :attachment$/ do
   attach_attachment("attachment")
 end
@@ -138,8 +149,10 @@ def attach_attachment(name, definition = nil)
 
 Given /^I start the rails application$/ do
   cd(".") do
+    require "rails"
     require "./config/environment"
-    require "capybara/rails"
+    require "capybara"
+    Capybara.app = Rails.application
   end
 end
 

diff --git a/features/step_definitions/s3_steps.rb b/features/step_definitions/s3_steps.rb
@@ -1,6 +1,6 @@
 When /^I attach the file "([^"]*)" to "([^"]*)" on S3$/ do |file_path, field|
   definition = Paperclip::AttachmentRegistry.definitions_for(User)[field.downcase.to_sym]
-  path = "https://paperclip.s3-us-west-2.amazonaws.com#{definition[:path]}"
+  path = "https://paperclip.s3.us-west-2.amazonaws.com#{definition[:path]}"
   path.gsub!(':filename', File.basename(file_path))
   path.gsub!(/:([^\/\.]+)/) do |match|
     "([^\/\.]+)"

diff --git a/features/support/env.rb b/features/support/env.rb
@@ -7,5 +7,6 @@
 World(RSpec::Matchers)
 
 Before do
+  aruba.config.command_launcher = ENV.fetch("DEBUG", nil) ? :debug : :spawn
   @aruba_timeout_seconds = 120
 end
diff --git a/gemfiles/4.2.gemfile b/gemfiles/4.2.gemfile
@@ -11,6 +11,7 @@ group :development, :test do
   gem "mime-types"
   gem "builder"
   gem "rubocop", :require => false
+  gem "rspec"
 end
 
 gemspec :path => "../"
diff --git a/gemfiles/5.0.gemfile b/gemfiles/5.0.gemfile
@@ -11,6 +11,7 @@ group :development, :test do
   gem "mime-types"
   gem "builder"
   gem "rubocop", :require => false
+  gem "rspec"
 end
 
 gemspec :path => "../"
diff --git a/lib/paperclip/io_adapters/attachment_adapter.rb b/lib/paperclip/io_adapters/attachment_adapter.rb
@@ -1,5 +1,11 @@
 module Paperclip
   class AttachmentAdapter < AbstractAdapter
+    def self.register
+      Paperclip.io_adapters.register self do |target|
+        Paperclip::Attachment === target || Paperclip::Style === target
+      end
+    end
+
     def initialize(target, options = {})
       super
       @target, @style = case target
@@ -32,6 +38,4 @@ def copy_to_tempfile(source)
   end
 end
 
-Paperclip.io_adapters.register Paperclip::AttachmentAdapter do |target|
-  Paperclip::Attachment === target || Paperclip::Style === target
-end
+Paperclip::AttachmentAdapter.register
diff --git a/lib/paperclip/io_adapters/data_uri_adapter.rb b/lib/paperclip/io_adapters/data_uri_adapter.rb
@@ -1,5 +1,10 @@
 module Paperclip
   class DataUriAdapter < StringioAdapter
+    def self.register
+      Paperclip.io_adapters.register self do |target|
+        String === target && target =~ REGEXP
+      end
+    end
 
     REGEXP = /\Adata:([-\w]+\/[-\w\+\.]+)?;base64,(.*)/m
 
@@ -11,12 +16,7 @@ def initialize(target_uri, options = {})
 
     def extract_target(uri)
       data_uri_parts = uri.match(REGEXP) || []
-      StringIO.new(Base64.decode64(data_uri_parts[2] || ''))
+      StringIO.new(Base64.decode64(data_uri_parts[2] || ""))
     end
-
   end
 end
-
-Paperclip.io_adapters.register Paperclip::DataUriAdapter do |target|
-  String === target && target =~ Paperclip::DataUriAdapter::REGEXP
-end
diff --git a/lib/paperclip/io_adapters/empty_string_adapter.rb b/lib/paperclip/io_adapters/empty_string_adapter.rb
@@ -1,5 +1,11 @@
 module Paperclip
   class EmptyStringAdapter < AbstractAdapter
+    def self.register
+      Paperclip.io_adapters.register self do |target|
+        target.is_a?(String) && target.empty?
+      end
+    end
+
     def nil?
       false
     end
@@ -10,6 +16,4 @@ def assignment?
   end
 end
 
-Paperclip.io_adapters.register Paperclip::EmptyStringAdapter do |target|
-  target.is_a?(String) && target.empty?
-end
+Paperclip::EmptyStringAdapter.register
diff --git a/lib/paperclip/io_adapters/file_adapter.rb b/lib/paperclip/io_adapters/file_adapter.rb
@@ -1,5 +1,11 @@
 module Paperclip
   class FileAdapter < AbstractAdapter
+    def self.register
+      Paperclip.io_adapters.register self do |target|
+        File === target || ::Tempfile === target
+      end
+    end
+
     def initialize(target, options = {})
       super
       cache_current_values
@@ -8,7 +14,9 @@ def initialize(target, options = {})
     private
 
     def cache_current_values
-      self.original_filename = @target.original_filename if @target.respond_to?(:original_filename)
+      if @target.respond_to?(:original_filename)
+        self.original_filename = @target.original_filename
+      end
       self.original_filename ||= File.basename(@target.path)
       @tempfile = copy_to_tempfile(@target)
       @content_type = ContentTypeDetector.new(@target.path).detect
@@ -17,6 +25,4 @@ def cache_current_values
   end
 end
 
-Paperclip.io_adapters.register Paperclip::FileAdapter do |target|
-  File === target || Tempfile === target
-end
+Paperclip::FileAdapter.register
diff --git a/lib/paperclip/io_adapters/http_url_proxy_adapter.rb b/lib/paperclip/io_adapters/http_url_proxy_adapter.rb
@@ -1,15 +1,15 @@
 module Paperclip
   class HttpUrlProxyAdapter < UriAdapter
+    def self.register
+      Paperclip.io_adapters.register self do |target|
+        String === target && target =~ REGEXP
+      end
+    end
 
     REGEXP = /\Ahttps?:\/\//
 
     def initialize(target, options = {})
       super(URI(URI.escape(target)), options)
     end
-
   end
 end
-
-Paperclip.io_adapters.register Paperclip::HttpUrlProxyAdapter do |target|
-  String === target && target =~ Paperclip::HttpUrlProxyAdapter::REGEXP
-end
diff --git a/lib/paperclip/io_adapters/identity_adapter.rb b/lib/paperclip/io_adapters/identity_adapter.rb
@@ -1,5 +1,11 @@
 module Paperclip
   class IdentityAdapter < AbstractAdapter
+    def self.register
+      Paperclip.io_adapters.register Paperclip::IdentityAdapter.new do |target|
+        Paperclip.io_adapters.registered?(target)
+      end
+    end
+
     def initialize
     end
 
@@ -9,7 +15,4 @@ def new(target, _)
   end
 end
 
-Paperclip.io_adapters.register Paperclip::IdentityAdapter.new do |target|
-  Paperclip.io_adapters.registered?(target)
-end
-
+Paperclip::IdentityAdapter.register
diff --git a/lib/paperclip/io_adapters/nil_adapter.rb b/lib/paperclip/io_adapters/nil_adapter.rb
@@ -1,8 +1,13 @@
 module Paperclip
   class NilAdapter < AbstractAdapter
-    def initialize(_target, _options = {})
+    def self.register
+      Paperclip.io_adapters.register self do |target|
+        target.nil? || ((Paperclip::Attachment === target) && !target.present?)
+      end
     end
 
+    def initialize(_target, _options = {}); end
+
     def original_filename
       ""
     end
@@ -19,7 +24,7 @@ def nil?
       true
     end
 
-    def read(*args)
+    def read(*_args)
       nil
     end
 
@@ -29,6 +34,4 @@ def eof?
   end
 end
 
-Paperclip.io_adapters.register Paperclip::NilAdapter do |target|
-  target.nil? || ( (Paperclip::Attachment === target) && !target.present? )
-end
+Paperclip::NilAdapter.register
diff --git a/lib/paperclip/io_adapters/registry.rb b/lib/paperclip/io_adapters/registry.rb
@@ -12,6 +12,10 @@ def register(handler_class, &block)
       @registered_handlers << [block, handler_class]
     end
 
+    def unregister(handler_class)
+      @registered_handlers.reject! { |_, klass| klass == handler_class }
+    end
+
     def handler_for(target)
       @registered_handlers.each do |tester, handler|
         return handler if tester.call(target)

diff --git a/lib/paperclip/io_adapters/stringio_adapter.rb b/lib/paperclip/io_adapters/stringio_adapter.rb
@@ -1,5 +1,11 @@
 module Paperclip
   class StringioAdapter < AbstractAdapter
+    def self.register
+      Paperclip.io_adapters.register self do |target|
+        StringIO === target
+      end
+    end
+
     def initialize(target, options = {})
       super
       cache_current_values
@@ -24,10 +30,7 @@ def copy_to_tempfile(source)
       destination.rewind
       destination
     end
-
   end
 end
 
-Paperclip.io_adapters.register Paperclip::StringioAdapter do |target|
-  StringIO === target
-end
+Paperclip::StringioAdapter.register
diff --git a/lib/paperclip/io_adapters/uploaded_file_adapter.rb b/lib/paperclip/io_adapters/uploaded_file_adapter.rb
@@ -1,5 +1,11 @@
 module Paperclip
   class UploadedFileAdapter < AbstractAdapter
+    def self.register
+      Paperclip.io_adapters.register self do |target|
+        target.class.name.include?("UploadedFile")
+      end
+    end
+
     def initialize(target, options = {})
       super
       cache_current_values
@@ -37,6 +43,4 @@ def determine_content_type
   end
 end
 
-Paperclip.io_adapters.register Paperclip::UploadedFileAdapter do |target|
-  target.class.name.include?("UploadedFile")
-end
+Paperclip::UploadedFileAdapter.register