[ActiveStorage] Disable variant options when false or nil present

[jacobsmith]

#32917

In response to #32917

In the current implementation, ActiveStorage passes all options to the underlying processor,
including when a key has a value of false.

For example, passing:

avatar.variant(resize: "100x100", monochrome: false, flip: "-90")

will return a monochrome image (or an error, pending on ImageMagick configuration) because
it passes -monochrome false to the command (but the command line does not allow disabling
flags this way, as usually a user would omit the flag entirely to disable that feature).

This fix only passes those keys forward to the underlying processor if the value responds to
present?. In practice, this means that false or nil will be filtered out before going
to the processor.

One possible use case would be for a user to be able to apply different filters to an avatar.
The code might look something like:

  variant_options = {
    monochrome: params[:monochrome],
    resize:     params[:resize]
  }

  avatar.variant(*variant_options)

Obviously some sanitization may be beneficial in a real-world scenario, but this type of
configuration object could be used in many other places as well.

( This is my first PR to Rails, so please let me know if I can update anything! Thanks! )

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

[jacobsmith]

I noticed one of the Travis CI builds had no output for 10 minutes and was terminated. From the logs it does not appear to be related to this PR. I’m not sure how to restart the build to get CI to green.

[georgeclaghorn]

Thanks!

This is an intentional behavior change, albeit a subtle one, so I think it merits mention in the changelog. Please add an entry for it.

[georgeclaghorn]

I’d prefer to push this condition down to pass_transform_argument rather than repeat it at each callsite:

   def pass_transform_argument(command, method, argument)
     if eligible_argument?(argument)
       command.public_send(method, argument)
-    else
+    elsif argument
       command.public_send(method)
     end
   end

[georgeclaghorn]

✂️ this extraneous newline.

[georgeclaghorn]

There’s no need for tap here. Move the argument condition to the else statement above:

if name.to_s == "combine_options"
  # ...
elsif argument.present?
  list << [name, argument]
end

[jacobsmith]

I ended up moving the line to list << [name, argument] like it was previously, but I had to wrap the entirety of the inject block in a list.tap call to guard against the new case where neither conditional fires. Without that, it implicitly returns nil, which causes the inject method to fail.

[jacobsmith]

Thanks for the feedback @georgeclaghorn - I was actually just working on a slightly different version because I wasn't happy with the conditionals being littered throughout the code. I was able to condense all of the changes into a single call site (the initializer) with all of the tests still passing. I'd love to hear your thoughts on something along the lines of:

  def initialize(transformations)
    combine_options = transformations.delete(:combine_options)

    @transformations = if combine_options
      { combine_options: combine_options.keep_if { |key, value| value.present? } }
    else
      transformations.keep_if { |key, value| value.present? }
    end
  end

I would personally tend to pull out a helper function remove_falsy_values which would lead to:

  def initialize(transformations)
    combine_options = transformations.delete(:combine_options)

    @transformations = if combine_options
      { combine_options: remove_falsy_values(combine_options) }
    else
       remove_falsy_values(transformations)
    end
  end

There feels like there should be an even simpler way of condensing that logic, but I'm having trouble because combine_options needs to be the key in the hash if present.

I'll look at adding info to the changelog but would love your thoughts on if I should make a new commit with the sanitization check in the initializer or leave it as-is.

Thanks!

[georgeclaghorn]

I was actually just working on a slightly different version because I wasn't happy with the conditionals being littered throughout the code.

I wouldn’t sweat it. Three of the four new conditionals will go away in 6.1 because we intend to drop support for using MiniMagick directly.

[georgeclaghorn]

Ah, I see the point of the tap now. 👌

[jacobsmith]

@georgeclaghorn I collapsed this down to one line and all of the tests pass, though I'm not entirely sure of the ramifications. I know you worked on this previously around supporting the layers option according to the git log, so if you happen to know if this breaks any current behavior, I'd be more than happy to write a new test for that and update this method accordingly.

From looking at the code, it seems like the only time this would happen is if a method without an argument were passed. However, given the current API where a Hash is passed to variant, I'm not sure under what conditions that would be possible. Does the API currently support an Array of symbols? (i.e., [:monochrome])

[georgeclaghorn]

We take the else branch when the argument is true or blank (e.g. false or nil).

As far as I can tell, this change results in a different mogrify command. I’d preserve the existing structure.

[georgeclaghorn]

Great! Can you please squash your commits into one?

[georgeclaghorn]

✂️

[jacobsmith]

@georgeclaghorn These commits have all been squashed. Please let me know if there's anything else I can do - I appreciate all your feedback! It's been an enjoyable first PR to Rails (:

[georgeclaghorn]

Thanks again!

[lucfranken]

@jacobsmith Nice PR!

In your example I see potential risk:

 variant_options = {
    monochrome: params[:monochrome],
    resize:     params[:resize]
  }

  avatar.variant(*variant_options)

You also state correctly:

Obviously some sanitization may be beneficial in a real-world scenario, but this type of
configuration object could be used in many other places as well.

Doesn't there need to be a huge warning for passing params directly into this variant method?

This example may get copied by users without even being aware that this is highly dangerous? A simple example issue could be resizing the image so huge it uses up too much memory.

The docs don't mention any of this:

http://guides.rubyonrails.org/active_storage_overview.html#transforming-images

In the security guide this ( http://guides.rubyonrails.org/security.html#file-downloads & http://guides.rubyonrails.org/security.html#command-line-injection ) may be interesting but I am not sure that a user would understand that his params potentially get executed.

[jacobsmith]

Ah, that is a very good point @lucfranken regarding command line injection. My initial thought would be to have some sort of whitelist of params that are "within normal bounds" - i.e., you can resize an image up to 2,000 x 2,000 pixels, but anything extra would require a special flag. Obviously every option would need a whitelist specified, and I'm not sure how feasible that is (I believe imagemagick has hundreds of options).

It's that hard line of giving the developer enough power to do what they need, but trying to put in safeguards to make sure things act as you would expect (i.e., I'm used to Rails doing some params sanitization, ActiveRecord doing some sanitization, etc.). I'd love to hear additional thoughts on ways to provide some additional safety? I could even see a more restrictive .safe_mode API or something like that users could opt-in to if they wanted extra protection.

[lucfranken]

I don't know the gem enough but it seems that there is something which validates the input, issues have been before:

https://packetstormsecurity.com/files/120777/Ruby-Gem-Minimagic-Command-Execution.html

minimagick/minimagick#131

That being said I am not sure what happens if you try the sql injection like attacks on it.