Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove implementation-dependent information from an error message #2499

Merged
merged 1 commit into from
Jun 13, 2021

Conversation

akihikodaki
Copy link
Contributor

The message for CarrierWave::ProcessingError used to include

  • the name of the underlying image processor (RMagick or MiniMagick) 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.

Alternative solutions

I'm aware this change may be a bit controversial and have ideas of alternative solutions for the case. Please tell me if you think they are more preferred or you have yet another idea; I'm willing to amend this change so that you feel comfortable with it.

Adding an option

This changes the existing behavior and is breaking by nature. CarrierWave may have an opt-in configuration to enable the new behavior to avoid disruption. Such an option may also be useful to change the behavior depending on environments (e.g. development v.s. production).

Adding a logging infrastructure

With this change, you will no longer see the original error message when printing the error message, which is a common strategy when you encounter an unexpected exception. Although you can still use CarrierWave::ProcessingError#cause, it may make debugging difficult. An alternative logging may heal the pain. I actually have two options for logging.

Just Rails.logger.error in CarrierWave::Validations::ActiveModel

This is so simple that it does not require any more description. However it does not work if you do not use Ruby on Rails, of course.

Add a method to retrieve an user-facing error message to CarrierWave::UploadError

Leave the error message exposed with CarrierWave::UploadError#message as is, and instead add a method to retrieve a message which does not include potentially sensitive information. This is complicated, but developers can expect an exception carries a detailed message.

@mshibuya
Copy link
Member

Sorry for replying late.

This change will be controversial, I understand your concern but some developers will still want the errors that are caused by the users' misbehavior to be shown to the users.
In the long run, I guess

Add a method to retrieve an user-facing error message to CarrierWave::UploadError

will be the best option.
Do you have any idea on how to implement it?

@mshibuya
Copy link
Member

Could you resolve the conflict?
The change in #2500 introduced CarrierWave::Vips processor so its error message also needs to be taken care of.

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.
@akihikodaki
Copy link
Contributor Author

Could you resolve the conflict?
The change in #2500 introduced CarrierWave::Vips processor so its error message also needs to be taken care of.

7af7db3: rebased to commit 229594f.

@mshibuya mshibuya added this to the Release v3.0.0 milestone Jun 5, 2021
@mshibuya mshibuya merged commit b7506ed into carrierwaveuploader:master Jun 13, 2021
@mshibuya
Copy link
Member

Merging in, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants