UnsafeReflection requires array to be defined with values strictly in the context of the execution

[zhisme]

Background

Brakeman version: 5.4.0
Rails version: 6.1.7.1
Ruby version: 3.0.3

Link to Rails application code: ?

False Positive

Full warning from Brakeman: ?

Confidence: Medium
Category: Remote Code Execution
Check: UnsafeReflection
Message: Unsafe reflection method `constantize` called on model attribute
Code: Template::ALLOWED_TYPES.find do  (t == Template.find_by(:code => code).type)  end.constantize
File: app/services/finder.rb
Line: 40

Relevant code:

    def template_klass
    template_db = Template.find_by(code: code)
    return if template_db.blank?

    template_type = Template::ALLOWED_TYPES.find { |t| t == template_db.type }

    raise UnknownTemplate unless template_type

    template_type.constantize
  end

This code is not producing any warnings

  def template_klass
    template_db = Template.find_by(code: code)
    return if template_db.blank?

    template_type = ['type1', 'type2'].find { |t| t == template_db.type }

    raise UnknownTemplate unless template_type

    template_type.constantize
  end

Why might this be a false positive?
Why is it forcing to duplicate constants for the codebase, it should allow constants from other classes and not to be so much verbose. What do you think?

[zhisme]

it is even allowing such construction

ALLOWED_TYPES = Template::ALLOWED_TYPES.freeze

  def template_klass
    template_db = Template.find_by(code: code)
    return if template_db.blank?

    template_type = ALLOWED_TYPES.find { |t| t == template_db.type }

    raise UnknownTemplate unless template_type

    template_type.constantize
  end

imo it is a bug

[presidentbeef]

🤔 Yes, sounds like a bug if using ALLOWED_TYPES = Template::ALLOWED_TYPES.freeze does not generate a finding but not using it does.

Replicating Ruby's constant lookup is challenging, so this isn't terribly surprising to me.

How is Template::ALLOWED_TYPES defined? Guessing something like

class Template
  ALLOWED_TYPES = [...]

?

[zhisme]

@presidentbeef yes, it is defined like you've mentioned

class Template
  ALLOWED_TYPES = ['type1', 'type2'].freeze
  # ...
end

do you have any guesses/propositions where to look? I can handle PR with your support

[presidentbeef]

I believe it comes down to this method that adds in constants. Note the context parameter that does not get used at all.

That parameter includes information about the module/class/method where the constant is defined - you can see where the info is passed in here.

So the solution will likely involve converting that information into a more accurate constant name in this method.

[presidentbeef]

I think this might have been fixed with #1981