Ruby check-unsafe-reflection-methods false positive for "some_value.tap"

[nightpool]

Describe the bug
Brakeman has a rule designed to catch the unsafe usage of tap using a "blockified" symbol generated from user input. For example, the following code is vulnerable, because it relies on using user input to determine which method to call:

  User.new.tap(&params[:method].to_sym)

https://github.com/presidentbeef/brakeman/blob/main/lib/brakeman/checks/check_unsafe_reflection_methods.rb

In fact, the actual vulnerable code in this case is the & operator itself. This operator is designed to turn a symbol into a "block" by referencing the function with the same name as the symbol. This would be an issue when using & on any symbol derived from user input, not just ones passed to tap (tap is just one of the most common cases). For example, User.all.map(&params[:method].to_sym) would be similarly vulnerable.

However, the Semgrep rule this was turned into is very overly broad—it flags as a security vulnerability any usage of the tap method at all even when such usage doesn't involve reflection. For example, it flags params[:user_id].to_i.tap { |user_id| Notifier.remind_user(user_id) } as "vulnerable code", even thought that code doesn't contain any reflection whatsoever.

This is a deviation between the brakeman rule (which checks whether user-input variable is provided as an argument to the tap function, e.g. .tap(user_input)), and the semgrep rule, which appears to flag whether the user input is used as the receiver of the tap function.

This can get extremely silly, for example, this simplified example of a place this rule was flagged by Semgrep in our codebase:

  def load_page_data(target, **options)
    rendering_context = {campaign_override: params[:campaign_override]}
    options = options.merge(rendering_context: rendering_context)
    serialize_data(target, options).tap do |serialized_data|
      log_data_layer if serialized_data[:has_data_layer]
    end
  end

To Reproduce

# ok: check-unsafe-reflection-methods
params[:user_id].to_i.tap { }

Expected behavior

Test passes.

Priority
How important is this to you?

• P2: annoying but not blocking me

[nightpool]

I believe this was fixed by @normprovost in this PR: #3265