Skip to content

Commit

Permalink
Use RESTRICT_ON_SEND (#195)
Browse files Browse the repository at this point in the history
Follow up rubocop/rubocop#8365.

This PR uses `RESTRICT_ON_SEND` to restrict callbacks `on_send` to specific method names only.
https://docs.rubocop.org/rubocop/1.46/development.html#implementation
  • Loading branch information
koic committed Feb 27, 2023
1 parent baa383b commit edfbe1b
Show file tree
Hide file tree
Showing 9 changed files with 47 additions and 41 deletions.
3 changes: 2 additions & 1 deletion rubocop-airbnb/lib/rubocop/cop/airbnb/default_scope.rb
Expand Up @@ -8,9 +8,10 @@ class DefaultScope < Base
'refactor data access patterns since the scope becomes part '\
'of every query unless explicitly excluded, even when it is '\
'unnecessary or incidental to the desired logic.'.freeze
RESTRICT_ON_SEND = %i(default_scope).freeze

def on_send(node)
return unless node.command?(:default_scope)
return if node.receiver

add_offense(node)
end
Expand Down
Expand Up @@ -6,9 +6,10 @@ module Airbnb
class FactoryClassUseString < Base
MSG = 'Instead of :class => MyClass, use :class => "MyClass". ' \
"This enables faster spec startup time and faster Zeus reload time.".freeze
RESTRICT_ON_SEND = %i(factory).freeze

def on_send(node)
return unless node.command?(:factory)
return if node.receiver

class_pair = class_node(node)

Expand Down
Expand Up @@ -5,11 +5,9 @@ module Airbnb
# mass assignment. It's a lazy, potentially dangerous approach that should be discouraged.
class MassAssignmentAccessibleModifier < Base
MSG = 'Do no override and objects mass assignment restrictions.'.freeze
RESTRICT_ON_SEND = %i(accessible=).freeze

def on_send(node)
_receiver, method_name, *_args = *node

return unless method_name == :accessible=
add_offense(node, message: MSG)
end
end
Expand Down
7 changes: 6 additions & 1 deletion rubocop-airbnb/lib/rubocop/cop/airbnb/no_timeout.rb
Expand Up @@ -8,9 +8,14 @@ class NoTimeout < Base
'It can also cause logic errors since it can raise in ' \
'any callee scope. Use client library timeouts and monitoring to ' \
'ensure proper timing behavior for web requests.'.freeze
RESTRICT_ON_SEND = %i(timeout).freeze

def_node_matcher :timeout_const?, <<~PATTERN
(const {cbase nil?} :Timeout)
PATTERN

def on_send(node)
return unless node.source.start_with?('Timeout.timeout')
return unless timeout_const?(node.receiver)
add_offense(node, message: MSG)
end
end
Expand Down
7 changes: 2 additions & 5 deletions rubocop-airbnb/lib/rubocop/cop/airbnb/phrase_bundle_keys.rb
Expand Up @@ -27,10 +27,11 @@ module Airbnb
class PhraseBundleKeys < Base
MESSAGE =
'Phrase bundle keys should match their translation keys.'.freeze
RESTRICT_ON_SEND = %i(t).freeze

def on_send(node)
parent = node.parent
if t_call?(node) && in_phrase_bundle_class?(node) && parent.pair_type?
if in_phrase_bundle_class?(node) && parent.pair_type?
hash_key = parent.children[0]
unless hash_key.children[0] == node.children[2].children[0]
add_offense(hash_key, message: MESSAGE)
Expand All @@ -57,10 +58,6 @@ def const_phrase_bundle_children(node)
e.children[1] == :PhraseBundle
end
end

def t_call?(node)
node.children[1] == :t
end
end
end
end
Expand Down
Expand Up @@ -3,7 +3,14 @@ module Cop
module Airbnb
# Disallow ActiveRecord calls that pass interpolated or added strings as an argument.
class RiskyActiverecordInvocation < Base
VULNERABLE_AR_METHODS = [
MSG = 'Passing a string computed by interpolation or addition to an ActiveRecord ' \
'method is likely to lead to SQL injection. Use hash or parameterized syntax. For ' \
'more information, see ' \
'http://guides.rubyonrails.org/security.html#sql-injection-countermeasures and ' \
'https://rails-sqli.org/rails3. If you have confirmed with Security that this is a ' \
'safe usage of this style, disable this alert with ' \
'`# rubocop:disable Airbnb/RiskyActiverecordInvocation`.'.freeze
RESTRICT_ON_SEND = [
:delete_all,
:destroy_all,
:exists?,
Expand All @@ -22,29 +29,15 @@ class RiskyActiverecordInvocation < Base
:update_all,
:where,
].freeze
MSG = 'Passing a string computed by interpolation or addition to an ActiveRecord ' \
'method is likely to lead to SQL injection. Use hash or parameterized syntax. For ' \
'more information, see ' \
'http://guides.rubyonrails.org/security.html#sql-injection-countermeasures and ' \
'https://rails-sqli.org/rails3. If you have confirmed with Security that this is a ' \
'safe usage of this style, disable this alert with ' \
'`# rubocop:disable Airbnb/RiskyActiverecordInvocation`.'.freeze
def on_send(node)
receiver, method_name, *_args = *node

return if receiver.nil?
return unless vulnerable_ar_method?(method_name)
if !includes_interpolation?(_args) && !includes_sum?(_args)
return if node.receiver.nil?
if !includes_interpolation?(node.arguments) && !includes_sum?(node.arguments)
return
end

add_offense(node)
end

def vulnerable_ar_method?(method)
VULNERABLE_AR_METHODS.include?(method)
end

# Return true if the first arg is a :dstr that has non-:str components
def includes_interpolation?(args)
!args.first.nil? &&
Expand Down
Expand Up @@ -40,6 +40,7 @@ class RspecEnvironmentModification < Base
def_node_matcher :rails_env_assignment, '(send (const nil? :Rails) :env= ...)'

MESSAGE = "Do not stub or set Rails.env in specs. Use the `stub_env` method instead".freeze
RESTRICT_ON_SEND = %i(to stub env=).freeze

def on_send(node)
path = node.source_range.source_buffer.name
Expand Down
25 changes: 12 additions & 13 deletions rubocop-airbnb/lib/rubocop/cop/airbnb/unsafe_yaml_marshal.rb
Expand Up @@ -6,35 +6,34 @@ class UnsafeYamlMarshal < Base
MSG = 'Using unsafe YAML parsing methods on untrusted input can lead ' \
'to remote code execution. Use `safe_load`, `parse`, `parse_file`, or ' \
'`parse_stream` instead'.freeze
RESTRICT_ON_SEND = %i(load load_documents load_file load_stream).freeze

def on_send(node)
receiver, method_name, *_args = *node
return if node.receiver.nil?
return unless node.receiver.const_type?

return if receiver.nil?
return unless receiver.const_type?

check_yaml(node, receiver, method_name, *_args)
check_marshal(node, receiver, method_name, *_args)
check_yaml(node)
check_marshal(node)
rescue => e
puts e
puts e.backtrace
raise
end

def check_yaml(node, receiver, method_name, *_args)
return unless ['YAML', 'Psych'].include?(receiver.const_name)
return unless [:load, :load_documents, :load_file, :load_stream].include?(method_name)
def check_yaml(node)
const_name = node.receiver.const_name
return unless ['YAML', 'Psych'].include?(const_name)

message = "Using `#{receiver.const_name}.#{method_name}` on untrusted input can lead " \
message = "Using `#{const_name}.#{node.method_name}` on untrusted input can lead " \
"to remote code execution. Use `safe_load`, `parse`, `parse_file`, or " \
"`parse_stream` instead"

add_offense(node, message: message)
end

def check_marshal(node, receiver, method_name, *_args)
return unless receiver.const_name == 'Marshal'
return unless method_name == :load
def check_marshal(node)
return unless node.receiver.const_name == 'Marshal'
return unless node.method?(:load)

message = 'Using `Marshal.load` on untrusted input can lead to remote code execution. ' \
'Restructure your code to not use Marshal'
Expand Down
11 changes: 11 additions & 0 deletions rubocop-airbnb/spec/rubocop/cop/airbnb/no_timeout_spec.rb
Expand Up @@ -11,6 +11,17 @@ def some_method(a)
RUBY
end

it 'rejects ::Timeout.timeout' do
expect_offense(<<~RUBY)
def some_method(a)
::Timeout.timeout(5) do
^^^^^^^^^^^^^^^^^^^^ Do not use Timeout.timeout. [...]
some_other_method(a)
end
end
RUBY
end

it 'accepts foo.timeout' do
expect_no_offenses(<<~RUBY)
def some_method(a)
Expand Down

0 comments on commit edfbe1b

Please sign in to comment.