Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use RESTRICT_ON_SEND #195

Merged
merged 1 commit into from Feb 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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