New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No uri adapters #2435

Merged
merged 1 commit into from Jan 23, 2018

Conversation

Projects
None yet
10 participants
@jyurek
Contributor

jyurek commented Apr 21, 2017

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.

@@ -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)

This comment has been minimized.

@houndci-bot

houndci-bot Apr 21, 2017

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

@houndci-bot

houndci-bot Apr 21, 2017

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

Show outdated Hide outdated features/step_definitions/rails_steps.rb Outdated
@@ -58,6 +59,16 @@
end
end
Given /^I comment out lines that contain "([^"]+)" in "([^"]+)"$/ do |contains, glob|

This comment has been minimized.

@houndci-bot

houndci-bot Apr 21, 2017

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

houndci-bot Apr 21, 2017

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.

@@ -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)

This comment has been minimized.

@houndci-bot

houndci-bot Apr 21, 2017

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

@houndci-bot

houndci-bot Apr 21, 2017

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

Show outdated Hide outdated features/step_definitions/rails_steps.rb Outdated
@@ -58,6 +59,16 @@
end
end
Given /^I comment out lines that contain "([^"]+)" in "([^"]+)"$/ do |contains, glob|

This comment has been minimized.

@houndci-bot

houndci-bot Apr 21, 2017

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

houndci-bot Apr 21, 2017

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.

@@ -7,5 +7,6 @@
World(RSpec::Matchers)
Before do
aruba.config.command_launcher = ENV.fetch("DEBUG", nil) ? :debug : :spawn

This comment has been minimized.

@mike-burns

mike-burns Apr 21, 2017

Member

Nice.

@mike-burns

mike-burns Apr 21, 2017

Member

Nice.

@gabebw

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

Show outdated Hide outdated lib/paperclip/io_adapters/data_uri_adapter.rb Outdated
Show outdated Hide outdated lib/paperclip/io_adapters/http_url_proxy_adapter.rb Outdated
Show outdated Hide outdated features/step_definitions/rails_steps.rb Outdated
Show outdated Hide outdated README.md Outdated
Show outdated Hide outdated README.md Outdated
@reedloden

This comment has been minimized.

Show comment
Hide comment
@reedloden

reedloden Nov 13, 2017

This issue has been assigned CVE-2017-0889.

reedloden commented Nov 13, 2017

This issue has been assigned CVE-2017-0889.

@rongutierrez

This comment has been minimized.

Show comment
Hide comment
@rongutierrez

rongutierrez Nov 17, 2017

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.

rongutierrez commented Nov 17, 2017

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.

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

This comment has been minimized.

@houndci-bot

houndci-bot Jan 23, 2018

Line is too long. [82/80]

@houndci-bot

houndci-bot Jan 23, 2018

Line is too long. [82/80]

Show outdated Hide outdated lib/paperclip/io_adapters/registry.rb Outdated
@@ -49,6 +50,16 @@
end
end
Given /^I comment out lines that contain "([^"]+)" in "([^"]+)"$/ do |contains, glob|

This comment has been minimized.

@houndci-bot

houndci-bot Jan 23, 2018

Line is too long. [85/80]

@houndci-bot

houndci-bot Jan 23, 2018

Line is too long. [85/80]

@@ -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"

This comment has been minimized.

@houndci-bot

houndci-bot Jan 23, 2018

Line is too long. [86/80]

@houndci-bot

houndci-bot Jan 23, 2018

Line is too long. [86/80]

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.

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

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mike-burns mike-burns deleted the jy-no-uri-adapters branch Jan 23, 2018

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

@juanibiapina

This comment has been minimized.

Show comment
Hide comment
@juanibiapina

juanibiapina Jan 30, 2018

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

juanibiapina commented Jan 30, 2018

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

@jvanbaarsen

This comment has been minimized.

Show comment
Hide comment
@jvanbaarsen

jvanbaarsen Jan 30, 2018

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

jvanbaarsen commented Jan 30, 2018

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

@juanibiapina

This comment has been minimized.

Show comment
Hide comment
@juanibiapina

juanibiapina Jan 30, 2018

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.

juanibiapina commented Jan 30, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Kevinrob

Kevinrob Feb 1, 2018

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 commented Feb 1, 2018

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 😄.

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

This comment has been minimized.

@Kevinrob

Kevinrob Feb 1, 2018

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

@Kevinrob

Kevinrob Feb 1, 2018

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

This comment has been minimized.

@pouellet
@pouellet

pouellet Feb 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment