diff --git a/.rubocop.yml b/.rubocop.yml index 483b38bbe..147fbb070 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -446,6 +446,9 @@ Security/Eval: Security/JSONLoad: Enabled: true +Security/Open: + Enabled: true + Security/YAMLLoad: Enabled: true diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index 1fbe15512..9fb8c7a80 100644 --- a/app/models/concerns/attachable.rb +++ b/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", diff --git a/lib/sms_api.rb b/lib/sms_api.rb index 64a25e7e0..e3187b710 100644 --- a/lib/sms_api.rb +++ b/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 diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index d548ab21d..548d5cfc9 100644 --- a/lib/tasks/active_storage.rake +++ b/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" diff --git a/spec/controllers/proposals_controller_spec.rb b/spec/controllers/proposals_controller_spec.rb index 05e1cc279..ae586123b 100644 --- a/spec/controllers/proposals_controller_spec.rb +++ b/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