Skip to content

Commit

Permalink
(PUP-11772) Resolve Security/Eval
Browse files Browse the repository at this point in the history
Both actions and functions/data types already define arbitrary code and are
loaded from trusted locations, so using eval isn't any worse.

I updated the ActionBuilder to delegate specific methods to the action. For
example, if an action calls the DSL method `summary "something"`, then the
ActionBuilder will call the corresponding setter on the Action, e.g.
Action#summary = "something".

The Action code is bit more complicated because the arity of the block passed to
`when_invoked=` may be 0, positive or negative, depending on whether it accepts
optional arguments. Since we don't support Ruby 1.8 - 2.6, it could be improved
in the future to not call `eval`, but I didn't feel like bothering.
  • Loading branch information
joshcooper committed Nov 21, 2023
1 parent 283ba4c commit 1e4316b
Show file tree
Hide file tree
Showing 6 changed files with 11 additions and 22 deletions.
8 changes: 0 additions & 8 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -918,14 +918,6 @@ Naming/VariableName:
Naming/VariableNumber:
Enabled: false

Security/Eval:
Exclude:
- 'lib/puppet/interface/action.rb'
- 'lib/puppet/interface/action_builder.rb'
- 'lib/puppet/pops/loader/ruby_data_type_instantiator.rb'
- 'lib/puppet/pops/loader/ruby_function_instantiator.rb'
- 'lib/puppet/pops/loader/ruby_legacy_function_instantiator.rb'

# Configuration parameters: EnforcedStyle, AllowModifiersOnSymbols.
# SupportedStyles: inline, group
Style/AccessModifierDeclarations:
Expand Down
6 changes: 4 additions & 2 deletions lib/puppet/interface/action.rb
Original file line number Diff line number Diff line change
Expand Up @@ -265,12 +265,14 @@ def #{@name}(#{decl.join(", ")})
end
WRAPPER

# It should be possible to rewrite this code to use `define_method`
# instead of `class/instance_eval` since Ruby 1.8 is long dead.
if @face.is_a?(Class)
@face.class_eval do eval wrapper, nil, file, line end
@face.class_eval do eval wrapper, nil, file, line end # rubocop:disable Security/Eval
@face.send(:define_method, internal_name, &block)
@when_invoked = @face.instance_method(name)
else
@face.instance_eval do eval wrapper, nil, file, line end
@face.instance_eval do eval wrapper, nil, file, line end # rubocop:disable Security/Eval
@face.meta_def(internal_name, &block)
@when_invoked = @face.method(name).unbind
end
Expand Down
13 changes: 4 additions & 9 deletions lib/puppet/interface/action_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
# within the context of a new instance of this class.
# @api public
class Puppet::Interface::ActionBuilder
extend Forwardable

# The action under construction
# @return [Puppet::Interface::Action]
# @api private
Expand Down Expand Up @@ -142,15 +144,8 @@ def render_as(value = nil)
property = setter.to_s.chomp('=')

unless method_defined? property
# Using eval because the argument handling semantics are less awful than
# when we use the define_method/block version. The later warns on older
# Ruby versions if you pass the wrong number of arguments, but carries
# on, which is totally not what we want. --daniel 2011-04-18
eval <<-METHOD
def #{property}(value)
@action.#{property} = value
end
METHOD
# ActionBuilder#<property> delegates to Action#<setter>
def_delegator :@action, setter, property
end
end

Expand Down
2 changes: 1 addition & 1 deletion lib/puppet/pops/loader/ruby_data_type_instantiator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def self.create(loader, typed_name, source_ref, ruby_code_string)
# make the private loader available in a binding to allow it to be passed on
loader_for_type = loader.private_loader
here = get_binding(loader_for_type)
created = eval(ruby_code_string, here, source_ref, 1)
created = eval(ruby_code_string, here, source_ref, 1) # rubocop:disable Security/Eval
unless created.is_a?(Puppet::Pops::Types::PAnyType)
raise ArgumentError, _("The code loaded from %{source_ref} did not produce a data type when evaluated. Got '%{klass}'") % { source_ref: source_ref, klass: created.class }
end
Expand Down
2 changes: 1 addition & 1 deletion lib/puppet/pops/loader/ruby_function_instantiator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def self.create(loader, typed_name, source_ref, ruby_code_string)
# make the private loader available in a binding to allow it to be passed on
loader_for_function = loader.private_loader
here = get_binding(loader_for_function)
created = eval(ruby_code_string, here, source_ref, 1)
created = eval(ruby_code_string, here, source_ref, 1) # rubocop:disable Security/Eval
unless created.is_a?(Class)
raise ArgumentError, _("The code loaded from %{source_ref} did not produce a Function class when evaluated. Got '%{klass}'") % { source_ref: source_ref, klass: created.class }
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def self.create(loader, typed_name, source_ref, ruby_code_string)
# This will do the 3x loading and define the "function_<name>" and "real_function_<name>" methods
# in the anonymous module used to hold function definitions.
#
func_info = eval(ruby_code_string, here, source_ref, 1)
func_info = eval(ruby_code_string, here, source_ref, 1) # rubocop:disable Security/Eval

# Validate what was loaded
unless func_info.is_a?(Hash)
Expand Down

0 comments on commit 1e4316b

Please sign in to comment.