Make fingerprints configurable to prepare for a new default in the future #2229

bdewater · 2016-06-14T10:06:19Z

This is the continuation of the work by @medcat in #1708. I've rebased his work on master and made separate commits addressing changes requested in that pull request. If the maintainers of the gem give the green light I will squash the commits (preserving authorship) for clean commit history 😄

houndci-bot · 2016-06-14T10:06:28Z

spec/paperclip/io_adapters/attachment_adapter_spec.rb

@@ -92,7 +92,7 @@
      FileUtils.cp @attachment.queued_for_write[:thumb].path, @thumb.path

      @attachment.save
-      @subject = Paperclip.io_adapters.for(@attachment.styles[:thumb])
+      @subject = Paperclip.io_adapters.for(@attachment.styles[:thumb], hash_digest: Digest::SHA1)


Line is too long. [97/80]

bdewater · 2016-07-01T14:35:39Z

@tute I've again rebased on master now that 5.0 is released. I assume it is too late now to change the default - following semantic versioning that would be 6.0. Could we work on getting the changes in with MD5 as default so that changing the default in the future is only a small change?

tute · 2016-08-16T14:09:36Z

Could we work on getting the changes in with MD5 as default so that changing the default in the future is only a small change?

This makes perfect sense to me. Thank you!

bdewater · 2016-08-17T12:39:15Z

Rebased again @tute !

tute · 2016-08-19T00:42:52Z

NEWS

@@ -1,6 +1,22 @@
 master:
 * (port from 4.3) Improvement: the `URI adapter` now uses the content-disposition header to name the downloaded file.

+* Improvement: make the fingerprint digest configurable per attachment. The
+  default remains MD5 but this will change in a future version because it is
+  not considered secure anymore against intentional file corruption.


Can you gather resources on this assertion, so we provide more information to paperclip users?

tute · 2016-08-19T01:09:01Z

This PR looks great, thank you for your great work, @bdewater! Left a few comments. Can you please address the ones you agree with on different commits, so as to ease the code review?

I might not be getting something around the SHA1 vs SHA256. In that case, please explain why we have both.

Thank you!

houndci-bot · 2016-08-19T08:37:52Z

lib/paperclip/attachment.rb

@@ -33,6 +33,7 @@ def self.default_options
        :use_timestamp         => true,
        :whiny                 => Paperclip.options[:whiny] || Paperclip.options[:whiny_thumbnails],
        :validate_media_type   => true,
+        :adapter_options       => { },


Use the new Ruby 1.9 hash syntax.
Space inside empty hash literal braces detected.

tute · 2016-08-19T14:05:12Z

Thanks for your fast and great work, @bdewater! Left a comment around where to put the default hash function. Also, why do we mention SHA1 in the tests? Should we use the previous, or the new one?

bdewater · 2016-08-19T18:38:50Z

When moving the default hash location those S3 specs break in the same way as with the identity adapter 😠 I'll try some more in the weekend.

houndci-bot · 2016-08-19T18:38:54Z

spec/paperclip/io_adapters/abstract_adapter_spec.rb

+      it "returns a fingerprint" do
+        expect(subject.fingerprint).to be_a String
+        expect(subject.fingerprint).
+          to eq "243d7ce1099719df25f600f1c369c629fb979f88d5a01dbe7d0d48c8e6715bb1"


Line is too long. [82/80]

bdewater · 2016-08-22T12:29:45Z

I believe I've addressed all your comments @tute, I'll squash and rebase again if you agree it's ready for merging.

tute · 2016-08-23T20:27:37Z

Left one question. Please do squash and rebase. Thank you so much!

tute · 2016-08-23T20:33:10Z

lib/paperclip/io_adapters/identity_adapter.rb

@@ -1,12 +1,12 @@
 module Paperclip
  class IdentityAdapter < AbstractAdapter
-    def new(adapter)
+    def self.new(adapter, _)


Why do we call this adapter, but target in other places?

tute · 2016-08-23T20:35:40Z

Looking good. After we fix IdentityAdapter and make it look like the other adapters we can merge. Thanks a ton. :)

bdewater · 2016-08-23T20:54:50Z

Done :)!

tute · 2016-08-23T21:30:27Z

lib/paperclip/io_adapters/identity_adapter.rb

@@ -1,12 +1,12 @@
 module Paperclip
  class IdentityAdapter < AbstractAdapter
-    def new(adapter)
-      adapter


This is very confusing to me. Do we need this identity adapter? Do we need it to have a new class or instance method? The current PR changes how the Ruby language works (.new initializes and returns an instance), and that is not right. What happens if we drop this adapter?

Perhaps @jyurek could shine some light on what this adapter is supposed to do because it confused me as well. Removing it raises these errors:

Failures: 1) Paperclip::Attachment An attachment with a processor that returns original file when assigned #calls #make and doesn't unlink the original file Failure/Error: raise NoHandlerError.new("No handler found for #{target.inspect}") Paperclip::AdapterRegistry::NoHandlerError: No handler found for Paperclip::StringioAdapter: data # ./lib/paperclip/io_adapters/registry.rb:19:in `handler_for' # ./lib/paperclip/io_adapters/registry.rb:29:in `for' # ./lib/paperclip/attachment.rb:539:in `post_process_style' # ./lib/paperclip/attachment.rb:520:in `block in post_process_styles' # ./lib/paperclip/attachment.rb:519:in `each' # ./lib/paperclip/attachment.rb:519:in `post_process_styles' # ./lib/paperclip/attachment.rb:511:in `block (2 levels) in post_process' # ./lib/paperclip/callbacks.rb:38:in `run_paperclip_callbacks' # ./lib/paperclip/attachment.rb:509:in `block in post_process' # ./lib/paperclip/callbacks.rb:38:in `run_paperclip_callbacks' # ./lib/paperclip/attachment.rb:508:in `post_process' # ./lib/paperclip/attachment.rb:464:in `post_process_file' # ./lib/paperclip/attachment.rb:113:in `assign' # ./lib/paperclip/has_attached_file.rb:66:in `block in define_setter' # ./spec/paperclip/attachment_spec.rb:749:in `block (4 levels) in <top (required)>' 2) Paperclip::Storage::S3 An attachment that uses S3 for storage and has a question mark in file name returns a replaced version for path Failure/Error: raise NoHandlerError.new("No handler found for #{target.inspect}") Paperclip::AdapterRegistry::NoHandlerError: No handler found for Paperclip::StringioAdapter: question?mark.png # ./lib/paperclip/io_adapters/registry.rb:19:in `handler_for' # ./lib/paperclip/io_adapters/registry.rb:29:in `for' # ./lib/paperclip/attachment.rb:101:in `assign' # ./lib/paperclip/has_attached_file.rb:66:in `block in define_setter' # ./spec/paperclip/storage/s3_spec.rb:463:in `block (3 levels) in <top (required)>' 3) Paperclip::Storage::S3 An attachment that uses S3 for storage and has a question mark in file name returns a replaced version for url Failure/Error: raise NoHandlerError.new("No handler found for #{target.inspect}") Paperclip::AdapterRegistry::NoHandlerError: No handler found for Paperclip::StringioAdapter: question?mark.png # ./lib/paperclip/io_adapters/registry.rb:19:in `handler_for' # ./lib/paperclip/io_adapters/registry.rb:29:in `for' # ./lib/paperclip/attachment.rb:101:in `assign' # ./lib/paperclip/has_attached_file.rb:66:in `block in define_setter' # ./spec/paperclip/storage/s3_spec.rb:463:in `block (3 levels) in <top (required)>' Finished in 29.39 seconds (files took 1.05 seconds to load) 1084 examples, 3 failures Failed examples: rspec ./spec/paperclip/attachment_spec.rb:746 # Paperclip::Attachment An attachment with a processor that returns original file when assigned #calls #make and doesn't unlink the original file rspec ./spec/paperclip/storage/s3_spec.rb:468 # Paperclip::Storage::S3 An attachment that uses S3 for storage and has a question mark in file name returns a replaced version for path rspec ./spec/paperclip/storage/s3_spec.rb:472 # Paperclip::Storage::S3 An attachment that uses S3 for storage and has a question mark in file name returns a replaced version for url

I've reverted overriding #new in favor of the original implementation (overriding initialize to take no arguments and new as an instance method), I agree that it is less confusing. I've poked around a bit but it seems that one would need to refactor a lot in and around the adapter registry to get rid of this adapter. That seems out of scope for this PR.

Adapters now accept an options parameter, that currently specifies the type of hash digest to use. The default value remains MD5, but can be specified to be any OpenSSL-supported digest. The specs are modified to reflect that. The task just reassigns all of the attachments, thereby regenerating their fingerprints.

tute · 2016-08-24T14:25:41Z

This is a great change. Thanks so much for all your work! 🎉

houndci-bot reviewed Jun 14, 2016
View reviewed changes

tute mentioned this pull request Aug 16, 2016

Change MD5 Fingerprints to SHA1 Fingerprints #1708

Closed

tute reviewed Aug 19, 2016
View reviewed changes

bdewater changed the title ~~Change MD5 fingerprints to SHA256~~ Make fingerprints configurable to prepare for a new default in the future Aug 19, 2016

houndci-bot reviewed Aug 19, 2016
View reviewed changes

tute reviewed Aug 23, 2016
View reviewed changes

tute merged commit 5202acb into thoughtbot:master Aug 24, 2016

bdewater deleted the sha256 branch July 23, 2017 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make fingerprints configurable to prepare for a new default in the future #2229

Make fingerprints configurable to prepare for a new default in the future #2229

bdewater commented Jun 14, 2016

houndci-bot Jun 14, 2016

bdewater commented Jul 1, 2016

tute commented Aug 16, 2016

bdewater commented Aug 17, 2016

tute Aug 19, 2016

tute commented Aug 19, 2016

houndci-bot Aug 19, 2016

tute commented Aug 19, 2016

bdewater commented Aug 19, 2016

houndci-bot Aug 19, 2016

bdewater commented Aug 22, 2016

tute commented Aug 23, 2016

tute Aug 23, 2016

tute commented Aug 23, 2016

bdewater commented Aug 23, 2016

tute Aug 23, 2016

bdewater Aug 23, 2016 •

edited

Loading

bdewater Aug 24, 2016 •

edited

Loading

tute commented Aug 24, 2016

Make fingerprints configurable to prepare for a new default in the future #2229

Make fingerprints configurable to prepare for a new default in the future #2229

Conversation

bdewater commented Jun 14, 2016

houndci-bot Jun 14, 2016

Choose a reason for hiding this comment

bdewater commented Jul 1, 2016

tute commented Aug 16, 2016

bdewater commented Aug 17, 2016

tute Aug 19, 2016

Choose a reason for hiding this comment

tute commented Aug 19, 2016

houndci-bot Aug 19, 2016

Choose a reason for hiding this comment

tute commented Aug 19, 2016

bdewater commented Aug 19, 2016

houndci-bot Aug 19, 2016

Choose a reason for hiding this comment

bdewater commented Aug 22, 2016

tute commented Aug 23, 2016

tute Aug 23, 2016

Choose a reason for hiding this comment

tute commented Aug 23, 2016

bdewater commented Aug 23, 2016

tute Aug 23, 2016

Choose a reason for hiding this comment

bdewater Aug 23, 2016 • edited Loading

Choose a reason for hiding this comment

bdewater Aug 24, 2016 • edited Loading

Choose a reason for hiding this comment

tute commented Aug 24, 2016

bdewater Aug 23, 2016 •

edited

Loading

bdewater Aug 24, 2016 •

edited

Loading