From 80847b44f1965d20a2ed6e98a3fa4bf4c1194515 Mon Sep 17 00:00:00 2001 From: Jon Yurek Date: Thu, 20 Apr 2017 23:31:28 -0400 Subject: [PATCH] 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. --- .travis.yml | 5 ---- Gemfile | 1 + README.md | 30 ++++++++++++++++++- Rakefile | 2 +- features/step_definitions/rails_steps.rb | 15 +++++++++- features/step_definitions/s3_steps.rb | 2 +- features/support/env.rb | 1 + gemfiles/4.2.gemfile | 1 + gemfiles/5.0.gemfile | 1 + .../io_adapters/attachment_adapter.rb | 10 +++++-- lib/paperclip/io_adapters/data_uri_adapter.rb | 12 ++++---- .../io_adapters/empty_string_adapter.rb | 10 +++++-- lib/paperclip/io_adapters/file_adapter.rb | 14 ++++++--- .../io_adapters/http_url_proxy_adapter.rb | 10 +++---- lib/paperclip/io_adapters/identity_adapter.rb | 11 ++++--- lib/paperclip/io_adapters/nil_adapter.rb | 13 ++++---- lib/paperclip/io_adapters/registry.rb | 4 +++ lib/paperclip/io_adapters/stringio_adapter.rb | 11 ++++--- .../io_adapters/uploaded_file_adapter.rb | 10 +++++-- lib/paperclip/io_adapters/uri_adapter.rb | 19 ++++++------ .../io_adapters/data_uri_adapter_spec.rb | 6 ++++ .../http_url_proxy_adapter_spec.rb | 5 ++++ .../paperclip/io_adapters/uri_adapter_spec.rb | 5 ++++ spec/paperclip/storage/s3_spec.rb | 7 +++-- 24 files changed, 147 insertions(+), 58 deletions(-) diff --git a/.travis.yml b/.travis.yml index 557c69f01..4c21dd2cb 100644 --- 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 index 98ee80d44..9de7c7440 100644 --- 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 index 9d15a7907..74a3515fc 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,6 @@ https://github.com/thoughtbot/paperclip/releases - - [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 index 219ea450f..ed7f61d6a 100644 --- 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 index 6035c6490..bbe15b0a7 100644 --- 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 index 7ec127345..f27f3f1af 100644 --- 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 index 113d64f2c..03bbd22e3 100644 --- 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 index ebabce6e4..7acc8eb32 100644 --- 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 index fab3aed76..b29822d02 100644 --- 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 index a329b19ee..f44e8423a 100644 --- 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 index 296d70def..5bc31f308 100644 --- 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 index 2f0a7439d..073dc8da9 100644 --- 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 index 00319e8dc..cc127bb0a 100644 --- 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 index fd334c6b1..8f568a5b5 100644 --- 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 index 9be025a88..6ec4237fa 100644 --- 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 index 26566b8e7..20c73f6a4 100644 --- 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 index 4a328d24b..7eb295868 100644 --- 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 index 03defdc5e..03da9c407 100644 --- 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 index f219df965..b1dee925b 100644 --- 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 diff --git a/lib/paperclip/io_adapters/uri_adapter.rb b/lib/paperclip/io_adapters/uri_adapter.rb index f2a0eaffc..ccee5dc63 100644 --- a/lib/paperclip/io_adapters/uri_adapter.rb +++ b/lib/paperclip/io_adapters/uri_adapter.rb @@ -1,9 +1,15 @@ -require 'open-uri' +require "open-uri" module Paperclip class UriAdapter < AbstractAdapter attr_writer :content_type + def self.register + Paperclip.io_adapters.register self do |target| + target.is_a?(URI) + end + end + def initialize(target, options = {}) super @content = download_content @@ -28,9 +34,8 @@ def content_type_from_content end def filename_from_content_disposition - if @content.meta.has_key?("content-disposition") - matches = @content.meta["content-disposition"]. - match(/filename="([^"]*)"/) + if @content.meta.key?("content-disposition") + matches = @content.meta["content-disposition"].match(/filename="([^"]*)"/) matches[1] if matches end end @@ -50,7 +55,7 @@ def download_content end def copy_to_tempfile(src) - while data = src.read(16*1024) + while data = src.read(16 * 1024) destination.write(data) end src.close @@ -59,7 +64,3 @@ def copy_to_tempfile(src) end end end - -Paperclip.io_adapters.register Paperclip::UriAdapter do |target| - target.kind_of?(URI) -end diff --git a/spec/paperclip/io_adapters/data_uri_adapter_spec.rb b/spec/paperclip/io_adapters/data_uri_adapter_spec.rb index aaf444dfa..e8fe4815d 100644 --- a/spec/paperclip/io_adapters/data_uri_adapter_spec.rb +++ b/spec/paperclip/io_adapters/data_uri_adapter_spec.rb @@ -1,7 +1,13 @@ require 'spec_helper' describe Paperclip::DataUriAdapter do + before do + Paperclip::DataUriAdapter.register + end + after do + Paperclip.io_adapters.unregister(described_class) + if @subject @subject.close end diff --git a/spec/paperclip/io_adapters/http_url_proxy_adapter_spec.rb b/spec/paperclip/io_adapters/http_url_proxy_adapter_spec.rb index 5bfee7322..5406d9341 100644 --- a/spec/paperclip/io_adapters/http_url_proxy_adapter_spec.rb +++ b/spec/paperclip/io_adapters/http_url_proxy_adapter_spec.rb @@ -7,6 +7,11 @@ @open_return.stubs(:meta).returns({}) Paperclip::HttpUrlProxyAdapter.any_instance. stubs(:download_content).returns(@open_return) + Paperclip::HttpUrlProxyAdapter.register + end + + after do + Paperclip.io_adapters.unregister(described_class) end context "a new instance" do diff --git a/spec/paperclip/io_adapters/uri_adapter_spec.rb b/spec/paperclip/io_adapters/uri_adapter_spec.rb index a36cf010d..9453ee85c 100644 --- a/spec/paperclip/io_adapters/uri_adapter_spec.rb +++ b/spec/paperclip/io_adapters/uri_adapter_spec.rb @@ -8,6 +8,11 @@ @open_return = StringIO.new("xxx") @open_return.stubs(:content_type).returns(content_type) @open_return.stubs(:meta).returns(meta) + Paperclip::UriAdapter.register + end + + after do + Paperclip.io_adapters.unregister(described_class) end context "a new instance" do diff --git a/spec/paperclip/storage/s3_spec.rb b/spec/paperclip/storage/s3_spec.rb index 29f4425a2..99d31b1e4 100644 --- a/spec/paperclip/storage/s3_spec.rb +++ b/spec/paperclip/storage/s3_spec.rb @@ -256,7 +256,7 @@ def aws2_add_region end it "uses the S3 bucket with the correct host name" do - assert_equal "s3-ap-northeast-1.amazonaws.com", + assert_equal "s3.ap-northeast-1.amazonaws.com", @dummy.avatar.s3_bucket.client.config.endpoint.host end end @@ -821,8 +821,9 @@ def counter it "gets the right s3_host_name in development" do rails_env("development") do - assert_match %r{^s3-ap-northeast-1.amazonaws.com}, @dummy.avatar.s3_host_name - assert_match %r{^s3-ap-northeast-1.amazonaws.com}, + assert_match %r{^s3.ap-northeast-1.amazonaws.com}, + @dummy.avatar.s3_host_name + assert_match %r{^s3.ap-northeast-1.amazonaws.com}, @dummy.avatar.s3_bucket.client.config.endpoint.host end end