Merge pull request #3855 from consul/rubocop_open

Add Security/Open rubocop rule

.rubocop.yml

@@ -446,6 +446,9 @@ Security/Eval:
 Security/JSONLoad:
   Enabled: true
 
+Security/Open:
+  Enabled: true
+
 Security/YAMLLoad:
   Enabled: true
 

app/models/concerns/attachable.rb

@@ -26,7 +26,7 @@ module Attachable
   end
 
   def set_cached_attachment_from_attachment
-    self.cached_attachment = if Paperclip::Attachment.default_options[:storage] == :filesystem
+    self.cached_attachment = if filesystem_storage?
                                attachment.path
                              else
                                attachment.url
@@ -34,10 +34,10 @@ module Attachable
   end
 
   def set_attachment_from_cached_attachment
-    if Paperclip::Attachment.default_options[:storage] == :filesystem
+    if filesystem_storage?
       File.open(cached_attachment) { |file| self.attachment = file }
     else
-      self.attachment = URI.open(cached_attachment)
+      self.attachment = URI.parse(cached_attachment).open
     end
   end
 
@@ -51,6 +51,10 @@ module Attachable
 
   private
 
+    def filesystem_storage?
+      Paperclip::Attachment.default_options[:storage] == :filesystem
+    end
+
     def validate_attachment_size
       if association_class && attachment_file_size > max_file_size.megabytes
         errors.add(:attachment, I18n.t("#{model_name.plural}.errors.messages.in_between",

lib/sms_api.rb

@@ -9,7 +9,7 @@ class SMSApi
   def url
     return "" unless end_point_available?
 
-    open(Rails.application.secrets.sms_end_point).base_uri.to_s
+    URI.parse(Rails.application.secrets.sms_end_point).to_s
   end
 
   def authorization

lib/tasks/active_storage.rake

@@ -88,7 +88,7 @@ namespace :active_storage do
       source_file = if paperclip_storage == :filesystem
                       paperclip_attachment.path
                     else
-                      URI.open(paperclip_attachment.url, &:read)
+                      URI.parse(paperclip_attachment.url).open.read
                     end
 
       logger.info "Copying #{paperclip_attachment.url} to active storage"

spec/controllers/proposals_controller_spec.rb

@@ -8,4 +8,38 @@ describe ProposalsController do
       expect { get :index }.to raise_exception(FeatureFlags::FeatureDisabled)
     end
   end
+
+  describe "PUT update" do
+    let(:file) { Rails.root.join("spec/hacked") }
+
+    before do
+      File.delete(file) if File.exist?(file)
+      InvisibleCaptcha.timestamp_enabled = false
+    end
+
+    after { InvisibleCaptcha.timestamp_enabled = true }
+
+    it "ignores malicious cached attachments with remote storages" do
+      allow_any_instance_of(Image).to receive(:filesystem_storage?).and_return(false)
+      user = create(:user)
+      proposal = create(:proposal, author: user)
+      sign_in user
+
+      begin
+        put :update, params: {
+          id: proposal,
+          proposal: {
+            image_attributes: {
+              title: "Hacked!",
+              user_id: user.id,
+              cached_attachment: "| touch #{file}"
+            }
+          }
+        }
+      rescue StandardError
+      ensure
+        expect(file).not_to exist
+      end
+    end
+  end
 end