Parse cached attachment URLs with remote storages

In commit 5a4921a1a we replaced `URI.parse` with `URI.open` due to some
issues during our tests with S3.

However, there are some security issues with `URI.open` [1], since it
might allow some users to execute code on the server.

So we're using `URI.parse#open` instead.

[1] https://docs.rubocop.org/rubocop/cops_security.html#securityopen
This commit is contained in:
Javi Martín
2021-11-10 23:18:37 +01:00
parent e5fbd34eac
commit 5519518cfb
3 changed files with 36 additions and 2 deletions

View File

@@ -37,7 +37,7 @@ module Attachable
if filesystem_storage? if filesystem_storage?
File.open(cached_attachment) { |file| self.attachment = file } File.open(cached_attachment) { |file| self.attachment = file }
else else
self.attachment = URI.open(cached_attachment) self.attachment = URI.parse(cached_attachment).open
end end
end end

View File

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

View File

@@ -8,4 +8,38 @@ describe ProposalsController do
expect { get :index }.to raise_exception(FeatureFlags::FeatureDisabled) expect { get :index }.to raise_exception(FeatureFlags::FeatureDisabled)
end end
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 end