No uri adapters by jyurek · Pull Request #2435 · thoughtbot/paperclip

jyurek · 2017-04-21T15:26:48Z

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.

Additional note: this needs to be documented, but comments welcome as to where and how.

houndci-bot · 2017-04-21T15:29:56Z

lib/paperclip/io_adapters/uri_adapter.rb


    def copy_to_tempfile(src)
-      while data = src.read(16*1024)
+      while data = src.read(16 * 1024)


Assignment in condition - you probably meant to use ==.

houndci-bot · 2017-04-21T15:29:56Z

features/step_definitions/rails_steps.rb

 end

+Given /^I comment out lines that contain "([^"]+)" in "([^"]+)"$/ do |contains, glob|
+  cd (".") do


(...) interpreted as grouped expression.

houndci-bot · 2017-04-21T15:29:56Z

features/step_definitions/rails_steps.rb

  end
 end

+Given /^I comment out lines that contain "([^"]+)" in "([^"]+)"$/ do |contains, glob|


Ambiguous regexp literal. Parenthesize the method arguments if it's surely a regexp literal, or add a whitespace to the right of the / if it should be a division.

houndci-bot · 2017-04-21T15:29:56Z

lib/paperclip/io_adapters/uri_adapter.rb


    def copy_to_tempfile(src)
-      while data = src.read(16*1024)
+      while data = src.read(16 * 1024)


Assignment in condition - you probably meant to use ==.

houndci-bot · 2017-04-21T15:29:56Z

features/step_definitions/rails_steps.rb

 end

+Given /^I comment out lines that contain "([^"]+)" in "([^"]+)"$/ do |contains, glob|
+  cd (".") do


(...) interpreted as grouped expression.

It might be clearer to write this with no space between cd and (".").

houndci-bot · 2017-04-21T15:29:56Z

features/step_definitions/rails_steps.rb

  end
 end

+Given /^I comment out lines that contain "([^"]+)" in "([^"]+)"$/ do |contains, glob|


Ambiguous regexp literal. Parenthesize the method arguments if it's surely a regexp literal, or add a whitespace to the right of the / if it should be a division.

mike-burns

Yeah totally -- I like the direction you took this. In the least I'm glad to have an .unregister method, but the explicit .register method is nice, too.

Also quite happy to have some of the adapters not be the default.

LGTM, but we need to get some docs on this in a future commit.

mike-burns · 2017-04-21T15:42:43Z

features/support/env.rb

 World(RSpec::Matchers)

 Before do
+  aruba.config.command_launcher = ENV.fetch("DEBUG", nil) ? :debug : :spawn


gabebw

Some small code comments; I don't know enough to offer overarching feedback.

gabebw · 2017-04-21T16:17:34Z

lib/paperclip/io_adapters/data_uri_adapter.rb

  class DataUriAdapter < StringioAdapter
+    def self.register
+      Paperclip.io_adapters.register self do |target|
+        String === target && target =~ Paperclip::DataUriAdapter::REGEXP


Could this be REGEXP, without the class qualifiers, since it's in this class?

gabebw · 2017-04-21T16:17:53Z

lib/paperclip/io_adapters/http_url_proxy_adapter.rb

  class HttpUrlProxyAdapter < UriAdapter
+    def self.register
+      Paperclip.io_adapters.register self do |target|
+        String === target && target =~ Paperclip::HttpUrlProxyAdapter::REGEXP


Same here about REGEXP.

gabebw · 2017-04-21T16:18:32Z

features/step_definitions/rails_steps.rb

 end

+Given /^I comment out lines that contain "([^"]+)" in "([^"]+)"$/ do |contains, glob|
+  cd (".") do


It might be clearer to write this with no space between cd and (".").

mike-burns

Nice!

mike-burns · 2017-04-21T18:01:59Z

README.md

+IO Adapters
+-----------
+
+When a file is uploaded or attached, it an be in one of a few different input


What's documentation without some typos?

reedloden · 2017-04-26T18:55:27Z

README.md

+adding a line similar to the following into `config/initializers/paperclip.rb`:
+
+```ruby
+Paperclip::DataUriAdapter.regsiter


reedloden · 2017-11-13T18:24:04Z

This issue has been assigned CVE-2017-0889.

rongutierrez · 2017-11-17T16:45:02Z

Any idea when this fix will be released? By making the CVE public you are making this high risk issue public without an available fix for users.

houndci-bot · 2018-01-23T14:55:26Z

lib/paperclip/io_adapters/uri_adapter.rb

-        matches = @content.meta["content-disposition"].
-          match(/filename="([^"]*)"/)
+      if @content.meta.key?("content-disposition")
+        matches = @content.meta["content-disposition"].match(/filename="([^"]*)"/)


Line is too long. [82/80]

houndci-bot · 2018-01-23T14:55:27Z

lib/paperclip/io_adapters/registry.rb

    end

+    def unregister(handler_class)
+      @registered_handlers.reject! {|_, klass| klass == handler_class }


Space between { and | missing.

houndci-bot · 2018-01-23T14:55:27Z

features/step_definitions/rails_steps.rb

  end
 end

+Given /^I comment out lines that contain "([^"]+)" in "([^"]+)"$/ do |contains, glob|


Line is too long. [85/80]

houndci-bot · 2018-01-23T14:55:27Z

features/step_definitions/rails_steps.rb

      gem "rubysl", :platform => :rbx
      """
    And I remove turbolinks
+    And I comment out lines that contain "action_mailer" in "config/environments/*.rb"


Line is too long. [86/80]

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.

See: - thoughtbot/paperclip#2435 - https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-0889

juanibiapina · 2018-01-30T11:28:12Z

This breaks API compatibility. Should have been a major bump.

jvanbaarsen · 2018-01-30T13:46:56Z

@juanibiapina What exactly does it break? (Can be helpful for other people to determine if they can upgrade or not)

juanibiapina · 2018-01-30T15:59:11Z

Now you have to manually enable the handlers, which means you need to make a code change for your application to continue working, otherwise you get Paperclip::AdapterRegistry::NoHandlerError. If you must do a code change, that's a major bump.

That means if you correctly configured bundler to automatically upgrade minor versions, your tests will (hopefully) blow up.

Kevinrob · 2018-02-01T20:40:25Z

Like @juanibiapina said, we lost few hours to understand why ours tests failed after upgrading...
A major version would have been nice. At least, a warning in the changelog 😄.

Kevinrob · 2018-02-01T20:54:30Z

README.md

+
+* `Paperclip::UriAdapter` - which accepts a `URI` instance.
+* `Paperclip::HttpUrlProxyAdapter` - which accepts a `http` string.
+* `Paperclip::DataUriAdapter` - which accepts a Base64-encoded `data:` string.


What is the risk with DataUriAdapter? How can we use it safely?

See - #2530 (comment)

houndci-bot reviewed Apr 21, 2017

View reviewed changes

mike-burns reviewed Apr 21, 2017

View reviewed changes

gabebw reviewed Apr 21, 2017

View reviewed changes

mike-burns reviewed Apr 21, 2017

View reviewed changes

reedloden reviewed Apr 26, 2017

View reviewed changes

guihatano mentioned this pull request Jan 22, 2018

Security vulnerability in version 5.1.0 #2530

Closed

mike-burns force-pushed the jy-no-uri-adapters branch from dde37e7 to 7b9bac8 Compare January 23, 2018 14:55

houndci-bot reviewed Jan 23, 2018

View reviewed changes

mike-burns force-pushed the jy-no-uri-adapters branch from b2e5eee to 80847b4 Compare January 23, 2018 16:29

mike-burns merged commit 80847b4 into master Jan 23, 2018

mike-burns deleted the jy-no-uri-adapters branch January 23, 2018 18:41

jankeesvw mentioned this pull request Jan 24, 2018

Upgrade to Paperclip 5.2.0 spree/spree#8558

Merged

This was referenced Jan 25, 2018

update paperclip gem when their fix is merged and released otris-systems/the-idb#449

Closed

update paperclip gem when their fix is merged and released otris-systems/the-idb#451

Closed

bertocq mentioned this pull request Jan 29, 2018

Upgrade Paperclip to 5.2.1 for security reasons consuldemocracy/consuldemocracy#2393

Merged

buren added a commit to justarrived/just_match_api that referenced this pull request Jan 29, 2018

Explicitly register Paperclip data URI adapter

d36fa80

See: - thoughtbot/paperclip#2435 - https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-0889

Kevinrob reviewed Feb 1, 2018

View reviewed changes

zjullion mentioned this pull request Feb 1, 2018

The 'paperclip' gem has security vulnerabilities and should be updated spree/spree#8576

Closed

abcang mentioned this pull request Feb 2, 2018

Fix saving of oEmbed image mastodon/mastodon#6409

Merged

zaksoup mentioned this pull request Feb 7, 2018

Github notifying us of CVE-2017-0889 for 5.2.0 #2547

Closed

roshats mentioned this pull request Apr 7, 2018

How to disable specific adapters? #2359

Closed

Uh oh!

Conversation

jyurek commented Apr 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-burns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabebw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-burns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reedloden commented Nov 13, 2017

Uh oh!

rongutierrez commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanibiapina commented Jan 30, 2018

Uh oh!

jvanbaarsen commented Jan 30, 2018

Uh oh!

juanibiapina commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kevinrob commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

rongutierrez commented Nov 17, 2017 •

edited

Loading

juanibiapina commented Jan 30, 2018 •

edited

Loading