Remove implementation-dependent information from an error message

The message for CarrierWave::ProcessingError used to include
- the name of the underlying image processor (RMagick, MiniMagick or
  libvips) and
- the original error message produced by it.

The message is used for ActiveRecord validation, and it is often shown for
an end-user. This behavior has some downsides:
- The message may look cryptic and is confusing for end-users. A typical
  end-user has no idea what is RMagick, for example.
- The information the message carries may be valuable for an adversary.
  Especially, the image processor may not be careful enough to mask
  sensitive information in an error message it produces. e.g. MiniMagick
  may include stderr of ImageMagick, which may contain file paths and
  reveal underlying ImageMagick configurations, in an error message.

This change solves the problems by simply removing those information.
You may still retrieve the original error message with
CarrierWave::ProcessingError#cause for debugging or programatic error
handling.
This commit is contained in:
Akihiko Odaki 2021-05-31 21:05:49 +09:00
parent 229594fb2a
commit 7af7db3636
8 changed files with 19 additions and 22 deletions

View File

@ -1000,8 +1000,7 @@ errors:
extension_denylist_error: "You are not allowed to upload %{extension} files, prohibited types: %{prohibited_types}"
content_type_allowlist_error: "You are not allowed to upload %{content_type} files, allowed types: %{allowed_types}"
content_type_denylist_error: "You are not allowed to upload %{content_type} files"
rmagick_processing_error: "Failed to manipulate with rmagick, maybe it is not an image?"
mini_magick_processing_error: "Failed to manipulate with MiniMagick, maybe it is not an image? Original Error: %{e}"
processing_error: "Failed to manipulate, maybe it is not an image?"
min_size_error: "File size should be greater than %{min_size}"
max_size_error: "File size should be less than %{max_size}"
```

View File

@ -8,8 +8,6 @@ en:
extension_denylist_error: "You are not allowed to upload %{extension} files, prohibited types: %{prohibited_types}"
content_type_allowlist_error: "You are not allowed to upload %{content_type} files, allowed types: %{allowed_types}"
content_type_denylist_error: "You are not allowed to upload %{content_type} files"
rmagick_processing_error: "Failed to manipulate with rmagick, maybe it is not an image?"
mini_magick_processing_error: "Failed to manipulate with MiniMagick, maybe it is not an image? Original Error: %{e}"
vips_processing_error: "Failed to manipulate with vips, maybe it is not an image? Original Error: %{e}"
processing_error: "Failed to manipulate, maybe it is not an image?"
min_size_error: "File size should be greater than %{min_size}"
max_size_error: "File size should be less than %{max_size}"

View File

@ -263,8 +263,8 @@ module CarrierWave
FileUtils.mv image.path, current_path
image.run_command("identify", current_path)
rescue ::MiniMagick::Error, ::MiniMagick::Invalid => e
message = I18n.translate(:"errors.messages.mini_magick_processing_error", :e => e)
rescue ::MiniMagick::Error, ::MiniMagick::Invalid
message = I18n.translate(:"errors.messages.processing_error")
raise CarrierWave::ProcessingError, message
ensure
image.destroy! if image
@ -309,8 +309,8 @@ module CarrierWave
file.content_type = Marcel::Magic.by_path(move_to).try(:type)
file.move_to(move_to, permissions, directory_permissions)
end
rescue ::MiniMagick::Error, ::MiniMagick::Invalid => e
message = I18n.translate(:"errors.messages.mini_magick_processing_error", :e => e)
rescue ::MiniMagick::Error, ::MiniMagick::Invalid
message = I18n.translate(:"errors.messages.processing_error")
raise CarrierWave::ProcessingError, message
end

View File

@ -370,8 +370,8 @@ module CarrierWave
end
destroy_image(frames)
rescue ::Magick::ImageMagickError => e
raise CarrierWave::ProcessingError, I18n.translate(:"errors.messages.rmagick_processing_error", :e => e)
rescue ::Magick::ImageMagickError
raise CarrierWave::ProcessingError, I18n.translate(:"errors.messages.processing_error")
end
private

View File

@ -262,8 +262,8 @@ module CarrierWave
file.content_type = Marcel::Magic.by_path(move_to).try(:type)
file.move_to(move_to, permissions, directory_permissions)
end
rescue ::Vips::Error => e
message = I18n.translate(:"errors.messages.vips_processing_error", :e => e)
rescue ::Vips::Error
message = I18n.translate(:"errors.messages.processing_error")
raise CarrierWave::ProcessingError, message
end

View File

@ -228,16 +228,16 @@ describe CarrierWave::MiniMagick do
before { File.open(instance.current_path, 'w') { |f| f.puts "bogus" } }
it "fails to process a non image file" do
expect { instance.resize_to_limit(200, 200) }.to raise_exception(CarrierWave::ProcessingError, /^Failed to manipulate with MiniMagick, maybe it is not an image\?/)
expect { instance.resize_to_limit(200, 200) }.to raise_exception(CarrierWave::ProcessingError, /^Failed to manipulate, maybe it is not an image\?/)
end
it "uses I18n" do
change_locale_and_store_translations(:nl, :errors => {
:messages => {
:mini_magick_processing_error => "Kon bestand niet met MiniMagick bewerken, misschien is het geen beeld bestand?"
:processing_error => "Kon bestand niet bewerken, misschien is het geen beeld bestand?"
}
}) do
expect {instance.resize_to_limit(200, 200)}.to raise_exception(CarrierWave::ProcessingError, /^Kon bestand niet met MiniMagick bewerken, misschien is het geen beeld bestand\?/)
expect {instance.resize_to_limit(200, 200)}.to raise_exception(CarrierWave::ProcessingError, /^Kon bestand niet bewerken, misschien is het geen beeld bestand\?/)
end
end

View File

@ -318,16 +318,16 @@ describe CarrierWave::RMagick, :rmagick => true do
end
it "fails to process a non image file" do
expect {instance.resize_to_limit(200, 200)}.to raise_exception(CarrierWave::ProcessingError, /^Failed to manipulate with rmagick, maybe it is not an image\?/)
expect {instance.resize_to_limit(200, 200)}.to raise_exception(CarrierWave::ProcessingError, /^Failed to manipulate, maybe it is not an image\?/)
end
it "uses I18n" do
change_locale_and_store_translations(:nl, :errors => {
:messages => {
:rmagick_processing_error => "Kon bestand niet met rmagick bewerken, misschien is het geen beeld bestand?"
:processing_error => "Kon bestand niet bewerken, misschien is het geen beeld bestand?"
}
}) do
expect {instance.resize_to_limit(200, 200)}.to raise_exception(CarrierWave::ProcessingError, /^Kon bestand niet met rmagick bewerken, misschien is het geen beeld bestand\?/)
expect {instance.resize_to_limit(200, 200)}.to raise_exception(CarrierWave::ProcessingError, /^Kon bestand niet bewerken, misschien is het geen beeld bestand\?/)
end
end

View File

@ -228,16 +228,16 @@ describe CarrierWave::Vips do
before { File.open(instance.current_path, 'w') { |f| f.puts "bogus" } }
it "fails to process a non image file" do
expect { instance.resize_to_limit(200, 200) }.to raise_exception(CarrierWave::ProcessingError, /^Failed to manipulate with vips, maybe it is not an image\?/)
expect { instance.resize_to_limit(200, 200) }.to raise_exception(CarrierWave::ProcessingError, /^Failed to manipulate, maybe it is not an image\?/)
end
it "uses I18n" do
change_locale_and_store_translations(:nl, :errors => {
:messages => {
:vips_processing_error => "Kon bestand niet met vips bewerken, misschien is het geen beeld bestand?"
:processing_error => "Kon bestand niet bewerken, misschien is het geen beeld bestand?"
}
}) do
expect {instance.resize_to_limit(200, 200)}.to raise_exception(CarrierWave::ProcessingError, /^Kon bestand niet met vips bewerken, misschien is het geen beeld bestand?\?/)
expect {instance.resize_to_limit(200, 200)}.to raise_exception(CarrierWave::ProcessingError, /^Kon bestand niet bewerken, misschien is het geen beeld bestand?\?/)
end
end